Coverity checks that the file descriptor return value of open(2) is
checked and used. Normally this is helpful in identifying real bugs but
vhost-user-blk-test opens /dev/null as stdin and stdout after fork.

In this case we don't need to look at the return value because these
open(2) calls cannot fail in any reasonable environment. We already know
their return values ahead of time since we closed stdin and stdout
previously. open(2) is guaranteed to pick the lowest available fd
number.

Silence Coverity by introducing code that checks what we already know to
be true.

** CID 1453270:  Resource leaks  (RESOURCE_LEAK)
/qemu/tests/qtest/vhost-user-blk-test.c: 920 in start_vhost_user_blk()

________________________________________________________________________________________________________
*** CID 1453270:  Resource leaks  (RESOURCE_LEAK)
/qemu/tests/qtest/vhost-user-blk-test.c: 920 in start_vhost_user_blk()
914              * Close standard file descriptors so tap-driver.pl pipe detects when
915              * our parent terminates.
916              */
917             close(0);
918             close(1);
919             open("/dev/null", O_RDONLY);
>>>     CID 1453270:  Resource leaks  (RESOURCE_LEAK)
>>>     Ignoring handle opened by "open("/dev/null", 1)" leaks it.
920             open("/dev/null", O_WRONLY);
921
922             execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL);
923             exit(1);
924         }
925         g_string_free(storage_daemon_command, true);

** CID 1453269:  Error handling issues  (NEGATIVE_RETURNS)
/qemu/tests/qtest/vhost-user-blk-test.c: 829 in create_listen_socket()

________________________________________________________________________________________________________
*** CID 1453269:  Error handling issues  (NEGATIVE_RETURNS)
/qemu/tests/qtest/vhost-user-blk-test.c: 829 in create_listen_socket()
823         char *path;
824
825         /* No race because our pid makes the path unique */
826         path = g_strdup_printf("/tmp/qtest-%d-sock.XXXXXX", getpid());
827         tmp_fd = mkstemp(path);
828         g_assert_cmpint(tmp_fd, >=, 0);
>>>     CID 1453269:  Error handling issues  (NEGATIVE_RETURNS)
>>>     "tmp_fd" is passed to a parameter that cannot be negative.
829         close(tmp_fd);
830         unlink(path);
831
832         *fd = qtest_socket_server(path);
833         g_test_queue_destroy(destroy_file, path);
834         return path;

** CID 1453268:    (CHECKED_RETURN)
/qemu/tests/qtest/vhost-user-blk-test.c: 920 in start_vhost_user_blk()
/qemu/tests/qtest/vhost-user-blk-test.c: 919 in start_vhost_user_blk()

________________________________________________________________________________________________________
*** CID 1453268:    (CHECKED_RETURN)
/qemu/tests/qtest/vhost-user-blk-test.c: 920 in start_vhost_user_blk()
914              * Close standard file descriptors so tap-driver.pl pipe detects when
915              * our parent terminates.
916              */
917             close(0);
918             close(1);
919             open("/dev/null", O_RDONLY);
>>>     CID 1453268:    (CHECKED_RETURN)
>>>     Calling "open("/dev/null", 1)" without checking return value. This library function may fail and return an error code. [Note: The source code implementation of the function has been overridden by a builtin model.]
920             open("/dev/null", O_WRONLY);
921
922             execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL);
923             exit(1);
924         }
925         g_string_free(storage_daemon_command, true);
/qemu/tests/qtest/vhost-user-blk-test.c: 919 in start_vhost_user_blk()
913             /*
914              * Close standard file descriptors so tap-driver.pl pipe detects when
915              * our parent terminates.
916              */
917             close(0);
918             close(1);
>>>     CID 1453268:    (CHECKED_RETURN)
>>>     Calling "open("/dev/null", 0)" without checking return value. This library function may fail and return an error code. [Note: The source code implementation of the function has been overridden by a builtin model.]
919             open("/dev/null", O_RDONLY);
920             open("/dev/null", O_WRONLY);
921
922             execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL);
923             exit(1);
924         }

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qtest/vhost-user-blk-test.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c
index 8796c74ca4..581e283a03 100644
--- a/tests/qtest/vhost-user-blk-test.c
+++ b/tests/qtest/vhost-user-blk-test.c
@@ -910,14 +910,18 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances,
                    storage_daemon_command->str);
     pid_t pid = fork();
     if (pid == 0) {
+        int fd;
+
         /*
          * Close standard file descriptors so tap-driver.pl pipe detects when
          * our parent terminates.
          */
         close(0);
+        fd = open("/dev/null", O_RDONLY);
+        g_assert_cmpint(fd, ==, 0);
         close(1);
-        open("/dev/null", O_RDONLY);
-        open("/dev/null", O_WRONLY);
+        fd = open("/dev/null", O_WRONLY);
+        assert(fd == 1);
 
         execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL);
         exit(1);
-- 
2.31.1

On Wed, 26 May 2021 at 10:16, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Coverity checks that the file descriptor return value of open(2) is
> checked and used. Normally this is helpful in identifying real bugs but
> vhost-user-blk-test opens /dev/null as stdin and stdout after fork.
>
> In this case we don't need to look at the return value because these
> open(2) calls cannot fail in any reasonable environment. We already know
> their return values ahead of time since we closed stdin and stdout
> previously. open(2) is guaranteed to pick the lowest available fd
> number.
>
> Silence Coverity by introducing code that checks what we already know to
> be true.

> diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c
> index 8796c74ca4..581e283a03 100644
> --- a/tests/qtest/vhost-user-blk-test.c
> +++ b/tests/qtest/vhost-user-blk-test.c
> @@ -910,14 +910,18 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances,
>                     storage_daemon_command->str);
>      pid_t pid = fork();
>      if (pid == 0) {
> +        int fd;
> +
>          /*
>           * Close standard file descriptors so tap-driver.pl pipe detects when
>           * our parent terminates.
>           */
>          close(0);
> +        fd = open("/dev/null", O_RDONLY);
> +        g_assert_cmpint(fd, ==, 0);
>          close(1);
> -        open("/dev/null", O_RDONLY);
> -        open("/dev/null", O_WRONLY);
> +        fd = open("/dev/null", O_WRONLY);
> +        assert(fd == 1);


Why use a different assert type for the two asserts?

thanks
-- PMM

On Sun, May 30, 2021 at 08:05:49PM +0100, Peter Maydell wrote:
> On Wed, 26 May 2021 at 10:16, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > Coverity checks that the file descriptor return value of open(2) is
> > checked and used. Normally this is helpful in identifying real bugs but
> > vhost-user-blk-test opens /dev/null as stdin and stdout after fork.
> >
> > In this case we don't need to look at the return value because these
> > open(2) calls cannot fail in any reasonable environment. We already know
> > their return values ahead of time since we closed stdin and stdout
> > previously. open(2) is guaranteed to pick the lowest available fd
> > number.
> >
> > Silence Coverity by introducing code that checks what we already know to
> > be true.
> 
> > diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c
> > index 8796c74ca4..581e283a03 100644
> > --- a/tests/qtest/vhost-user-blk-test.c
> > +++ b/tests/qtest/vhost-user-blk-test.c
> > @@ -910,14 +910,18 @@ static void start_vhost_user_blk(GString *cmd_line, int vus_instances,
> >                     storage_daemon_command->str);
> >      pid_t pid = fork();
> >      if (pid == 0) {
> > +        int fd;
> > +
> >          /*
> >           * Close standard file descriptors so tap-driver.pl pipe detects when
> >           * our parent terminates.
> >           */
> >          close(0);
> > +        fd = open("/dev/null", O_RDONLY);
> > +        g_assert_cmpint(fd, ==, 0);
> >          close(1);
> > -        open("/dev/null", O_RDONLY);
> > -        open("/dev/null", O_WRONLY);
> > +        fd = open("/dev/null", O_WRONLY);
> > +        assert(fd == 1);
> 
> 
> Why use a different assert type for the two asserts?

Thanks for pointing this out. I will send a v2 that consistently uses
g_assert_cmpint().

Stefan