Message ID | 20210601155755.216949-2-stefanha@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | vhost-user-blk-test and vdagent Coverity fixes | expand |
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>
On 01/06/2021 17.57, 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(-) > > diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c > index 8796c74ca4..c5c4667759 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); > + g_assert_cmpint(fd, ==, 1); > > execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL); > exit(1); > Reviewed-by: Thomas Huth <thuth@redhat.com>
diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c index 8796c74ca4..c5c4667759 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); + g_assert_cmpint(fd, ==, 1); execlp("/bin/sh", "sh", "-c", storage_daemon_command->str, NULL); exit(1);