[2/2] slirp: Handle error returns from sosendoob()

Message ID 1496679576-14336-3-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series
  • slirp: handle errors in sosendoob()
Related show

Commit Message

Peter Maydell June 5, 2017, 4:19 p.m.
sosendoob() can return a failure code, but all its callers ignore it.
This is OK in sbappend(), as the comment there states -- we will try
again later in sowrite(). Add a (void) cast to tell Coverity so.
In sowrite() we do need to check the return value -- we should handle
a write failure in sosendoob() the same way we handle a write failure
for the normal data.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 slirp/sbuf.c   |  2 +-
 slirp/socket.c | 22 ++++++++++++++++------
 2 files changed, 17 insertions(+), 7 deletions(-)

-- 
2.7.4

Comments

Dr. David Alan Gilbert July 11, 2017, 6:46 p.m. | #1
* Peter Maydell (peter.maydell@linaro.org) wrote:
> sosendoob() can return a failure code, but all its callers ignore it.

> This is OK in sbappend(), as the comment there states -- we will try

> again later in sowrite(). Add a (void) cast to tell Coverity so.

> In sowrite() we do need to check the return value -- we should handle

> a write failure in sosendoob() the same way we handle a write failure

> for the normal data.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


I think this is OK, I do have one worry, which is perhaps there
were errors previously that would just loose OOB but get silently
ignored that perhaps we survived OK.
There's a comment there about seeing EAGAIN or EINTR in the normal
data path and not erroring;   hopefully we don't in the OOB case?

However, it generally seems to be sane, so:


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


> ---

>  slirp/sbuf.c   |  2 +-

>  slirp/socket.c | 22 ++++++++++++++++------

>  2 files changed, 17 insertions(+), 7 deletions(-)

> 

> diff --git a/slirp/sbuf.c b/slirp/sbuf.c

> index 10119d3..912f235 100644

> --- a/slirp/sbuf.c

> +++ b/slirp/sbuf.c

> @@ -91,7 +91,7 @@ sbappend(struct socket *so, struct mbuf *m)

>  	if (so->so_urgc) {

>  		sbappendsb(&so->so_rcv, m);

>  		m_free(m);

> -		sosendoob(so);

> +		(void)sosendoob(so);

>  		return;

>  	}

>  

> diff --git a/slirp/socket.c b/slirp/socket.c

> index a17caa9..84cf13a 100644

> --- a/slirp/socket.c

> +++ b/slirp/socket.c

> @@ -404,7 +404,14 @@ sowrite(struct socket *so)

>  	DEBUG_ARG("so = %p", so);

>  

>  	if (so->so_urgc) {

> -		sosendoob(so);

> +		if (sosendoob(so) < so->so_urgc) {

> +			/* Treat a short write as a fatal error too,

> +			 * rather than continuing on and sending the urgent

> +			 * data as if it were non-urgent and leaving the

> +			 * so_urgc count wrong.

> +			 */

> +			goto err_disconnected;

> +		}

>  		if (sb->sb_cc == 0)

>  			return 0;

>  	}

> @@ -448,11 +455,7 @@ sowrite(struct socket *so)

>  		return 0;

>  

>  	if (nn <= 0) {

> -		DEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n",

> -			so->so_state, errno));

> -		sofcantsendmore(so);

> -		tcp_sockclosed(sototcpcb(so));

> -		return -1;

> +		goto err_disconnected;

>  	}

>  

>  #ifndef HAVE_READV

> @@ -479,6 +482,13 @@ sowrite(struct socket *so)

>  		sofcantsendmore(so);

>  

>  	return nn;

> +

> +err_disconnected:

> +	DEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n",

> +		    so->so_state, errno));

> +	sofcantsendmore(so);

> +	tcp_sockclosed(sototcpcb(so));

> +	return -1;

>  }

>  

>  /*

> -- 

> 2.7.4

> 

> 

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Peter Maydell July 11, 2017, 8:38 p.m. | #2
On 11 July 2017 at 19:46, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:

>> sosendoob() can return a failure code, but all its callers ignore it.

>> This is OK in sbappend(), as the comment there states -- we will try

>> again later in sowrite(). Add a (void) cast to tell Coverity so.

>> In sowrite() we do need to check the return value -- we should handle

>> a write failure in sosendoob() the same way we handle a write failure

>> for the normal data.

>>

>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>

> I think this is OK, I do have one worry, which is perhaps there

> were errors previously that would just loose OOB but get silently

> ignored that perhaps we survived OK.

> There's a comment there about seeing EAGAIN or EINTR in the normal

> data path and not erroring;   hopefully we don't in the OOB case?


Let's hope :-) This way round at least we'll find out if
we ever do.

> However, it generally seems to be sane, so:

>

>

> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


thanks
-- PMM
Samuel Thibault July 11, 2017, 11:24 p.m. | #3
Peter Maydell, on lun. 05 juin 2017 17:19:36 +0100, wrote:
> diff --git a/slirp/socket.c b/slirp/socket.c

> index a17caa9..84cf13a 100644

> --- a/slirp/socket.c

> +++ b/slirp/socket.c

> @@ -404,7 +404,14 @@ sowrite(struct socket *so)

>  	DEBUG_ARG("so = %p", so);

>  

>  	if (so->so_urgc) {

> -		sosendoob(so);

> +		if (sosendoob(so) < so->so_urgc) {


Mmm, I believe one needs to use a copy of so->so_urgc, since sosendoob()
modifies it in the success case?

Samuel

Patch

diff --git a/slirp/sbuf.c b/slirp/sbuf.c
index 10119d3..912f235 100644
--- a/slirp/sbuf.c
+++ b/slirp/sbuf.c
@@ -91,7 +91,7 @@  sbappend(struct socket *so, struct mbuf *m)
 	if (so->so_urgc) {
 		sbappendsb(&so->so_rcv, m);
 		m_free(m);
-		sosendoob(so);
+		(void)sosendoob(so);
 		return;
 	}
 
diff --git a/slirp/socket.c b/slirp/socket.c
index a17caa9..84cf13a 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -404,7 +404,14 @@  sowrite(struct socket *so)
 	DEBUG_ARG("so = %p", so);
 
 	if (so->so_urgc) {
-		sosendoob(so);
+		if (sosendoob(so) < so->so_urgc) {
+			/* Treat a short write as a fatal error too,
+			 * rather than continuing on and sending the urgent
+			 * data as if it were non-urgent and leaving the
+			 * so_urgc count wrong.
+			 */
+			goto err_disconnected;
+		}
 		if (sb->sb_cc == 0)
 			return 0;
 	}
@@ -448,11 +455,7 @@  sowrite(struct socket *so)
 		return 0;
 
 	if (nn <= 0) {
-		DEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n",
-			so->so_state, errno));
-		sofcantsendmore(so);
-		tcp_sockclosed(sototcpcb(so));
-		return -1;
+		goto err_disconnected;
 	}
 
 #ifndef HAVE_READV
@@ -479,6 +482,13 @@  sowrite(struct socket *so)
 		sofcantsendmore(so);
 
 	return nn;
+
+err_disconnected:
+	DEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n",
+		    so->so_state, errno));
+	sofcantsendmore(so);
+	tcp_sockclosed(sototcpcb(so));
+	return -1;
 }
 
 /*