Re: [PATCH v2 1/2] vhost-user-blk-test: fix Coverity open(2) false positives

[Philippe Mathieu-Daudé]

On 6/1/21 5:57 PM, Stefan Hajnoczi 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.
> 
> ** 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>