[PATCH] qtest: ensure socket_accept() return value is initialized

[PATCH] qtest: ensure socket_accept() return value is initialized

[Daniel P. Berrangé]

If the very first iteration of the loop breaks early due to the
dead QEMU process, 'ret' is never initialized. Ensure it is set
to -1 to represent an invalid FD number.

CID 1688281
Reported-by: Markus Armbruster <armbru@redhat.com>
Fixes: c12ad5378739 (qtest: fail fast when QEMU terminates early)
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qtest/libqtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 9e7a2d43a18..899727d6d35 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -121,7 +121,7 @@ static int qtest_socket_accept(QTestState *s, int sock)
 {
     struct sockaddr_un addr;
     socklen_t addrlen;
-    int ret;
+    int ret = -1;
     size_t i;
 #ifndef _WIN32
     struct timeval timeout = { .tv_sec = 0,
-- 
2.56.0

Re: [PATCH] qtest: ensure socket_accept() return value is initialized

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> If the very first iteration of the loop breaks early due to the
> dead QEMU process, 'ret' is never initialized. Ensure it is set
> to -1 to represent an invalid FD number.
>
> CID 1688281
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Fixes: c12ad5378739 (qtest: fail fast when QEMU terminates early)
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qtest/libqtest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 9e7a2d43a18..899727d6d35 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -121,7 +121,7 @@ static int qtest_socket_accept(QTestState *s, int sock)
>  {
>      struct sockaddr_un addr;
>      socklen_t addrlen;
> -    int ret;
> +    int ret = -1;
>      size_t i;
>  #ifndef _WIN32
>      struct timeval timeout = { .tv_sec = 0,

Hmm, we do the same thing in several different ways:

   static int qtest_socket_accept(QTestState *s, int sock)
   {
       struct sockaddr_un addr;
       socklen_t addrlen;
       int ret;
       size_t i;
   #ifndef _WIN32
       struct timeval timeout = { .tv_sec = 0,
                                  .tv_usec = ACCEPT_TIMEOUT_MS * 1000ul };
   #else
       DWORD timeout = ACCEPT_TIMEOUT_MS;
   #endif

       if (setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO,
                      (void *)&timeout, sizeof(timeout))) {
           fprintf(stderr, "%s failed to set SO_RCVTIMEO: %s\n",
                   __func__, strerror(errno));
           close(sock);
           return -1;

(A) Free resources and return failure.

       }

       for (i = 0; i < ACCEPT_RETRIES ; i++) {
           if (!qtest_probe_child(s)) {
               fprintf(stderr,
                       "child process unexpectedly exited, skipping socket accept\n");
               goto cleanup;

(B) Jump to exit, where we free resources and return @ret.

           }

           addrlen = sizeof(addr);
           ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
           if (ret == -1) {
               if (errno == EINTR || errno == EAGAIN) {
                   continue;
               } else {
                   fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
               }
           }

(C) Return @ret.

Hmm, should we close(sock) on failure here?

           return ret;
       }

    cleanup:
       close(sock);

       return ret;
   }

All three ways can work fine.  But using all three next to each other I
find somewhat grating.  Matter of taste.  I'm not demanding you change
this.

Re: [PATCH] qtest: ensure socket_accept() return value is initialized

[Daniel P. Berrangé]

On Mon, Oct 26, 2026 at 02:54:32PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > If the very first iteration of the loop breaks early due to the
> > dead QEMU process, 'ret' is never initialized. Ensure it is set
> > to -1 to represent an invalid FD number.
> >
> > CID 1688281
> > Reported-by: Markus Armbruster <armbru@redhat.com>
> > Fixes: c12ad5378739 (qtest: fail fast when QEMU terminates early)
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  tests/qtest/libqtest.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > index 9e7a2d43a18..899727d6d35 100644
> > --- a/tests/qtest/libqtest.c
> > +++ b/tests/qtest/libqtest.c
> > @@ -121,7 +121,7 @@ static int qtest_socket_accept(QTestState *s, int sock)
> >  {
> >      struct sockaddr_un addr;
> >      socklen_t addrlen;
> > -    int ret;
> > +    int ret = -1;
> >      size_t i;
> >  #ifndef _WIN32
> >      struct timeval timeout = { .tv_sec = 0,
> 
> Hmm, we do the same thing in several different ways:
> 
>    static int qtest_socket_accept(QTestState *s, int sock)
>    {
>        struct sockaddr_un addr;
>        socklen_t addrlen;
>        int ret;
>        size_t i;
>    #ifndef _WIN32
>        struct timeval timeout = { .tv_sec = 0,
>                                   .tv_usec = ACCEPT_TIMEOUT_MS * 1000ul };
>    #else
>        DWORD timeout = ACCEPT_TIMEOUT_MS;
>    #endif
> 
>        if (setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO,
>                       (void *)&timeout, sizeof(timeout))) {
>            fprintf(stderr, "%s failed to set SO_RCVTIMEO: %s\n",
>                    __func__, strerror(errno));
>            close(sock);
>            return -1;
> 
> (A) Free resources and return failure.
> 
>        }
> 
>        for (i = 0; i < ACCEPT_RETRIES ; i++) {
>            if (!qtest_probe_child(s)) {
>                fprintf(stderr,
>                        "child process unexpectedly exited, skipping socket accept\n");
>                goto cleanup;
> 
> (B) Jump to exit, where we free resources and return @ret.
> 
>            }
> 
>            addrlen = sizeof(addr);
>            ret = accept(sock, (struct sockaddr *)&addr, &addrlen);
>            if (ret == -1) {
>                if (errno == EINTR || errno == EAGAIN) {
>                    continue;
>                } else {
>                    fprintf(stderr, "%s failed: %s\n", __func__, strerror(errno));
>                }
>            }
> 
> (C) Return @ret.
> 
> Hmm, should we close(sock) on failure here?

Opps, yes, you are right - we should in fact fall through to
the cleanup: label, as the intention is 'sock' is closed in
all, as the caller expects socket_accept() to close the sock
in all cases. That's a second bug in my original patch.

This is pretty unpleasant semantics though - the caller owns
the socket and should own closing it.

>            return ret;
>        }
> 
>     cleanup:
>        close(sock);
> 
>        return ret;
>    }
> 
> All three ways can work fine.  But using all three next to each other I
> find somewhat grating.  Matter of taste.  I'm not demanding you change
> this.
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH] qtest: ensure socket_accept() return value is initialized

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Oct 26, 2026 at 02:54:32PM +0100, Markus Armbruster wrote:

[...]

>> Hmm, should we close(sock) on failure here?
>
> Opps, yes, you are right - we should in fact fall through to
> the cleanup: label, as the intention is 'sock' is closed in
> all, as the caller expects socket_accept() to close the sock
> in all cases. That's a second bug in my original patch.
>
> This is pretty unpleasant semantics though - the caller owns
> the socket and should own closing it.

Oh yes.  Even more so since the function is called *_accept().

[...]