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(-)
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
* 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
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
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
© 2016 - 2026 Red Hat, Inc.