From: Stefan Hajnoczi <stefanha@redhat.com>
To: qemu-devel@nongnu.org
Cc: Laurent Vivier <lvivier@redhat.com>,
Kevin Wolf <kwolf@redhat.com>, Thomas Huth <thuth@redhat.com>,
qemu-block@nongnu.org, Coiby Xu <Coiby.Xu@gmail.com>,
Gerd Hoffmann <kraxel@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH 1/3] vhost-user-blk-test: fix Coverity open(2) false positives
Date: Wed, 26 May 2021 10:12:46 +0100 [thread overview]
Message-ID: <20210526091248.434459-2-stefanha@redhat.com> (raw)
In-Reply-To: <20210526091248.434459-1-stefanha@redhat.com>
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
next prev parent reply other threads:[~2021-05-26 9:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-26 9:12 [PATCH 0/3] vhost-user-blk-test and vdagent Coverity fixes Stefan Hajnoczi
2021-05-26 9:12 ` Stefan Hajnoczi [this message]
2021-05-30 19:05 ` [PATCH 1/3] vhost-user-blk-test: fix Coverity open(2) false positives Peter Maydell
2021-06-01 15:36 ` Stefan Hajnoczi
2021-05-26 9:12 ` [PATCH 2/3] vhost-user-blk-test: fix Coverity mkstemp(2) umask warning Stefan Hajnoczi
2021-05-30 19:01 ` Peter Maydell
2021-06-01 15:36 ` Stefan Hajnoczi
2021-05-26 9:12 ` [PATCH 3/3] ui/vdagent: fix clipboard info memory leak in error path Stefan Hajnoczi
2021-05-28 7:06 ` Gerd Hoffmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210526091248.434459-2-stefanha@redhat.com \
--to=stefanha@redhat.com \
--cc=Coiby.Xu@gmail.com \
--cc=kraxel@redhat.com \
--cc=kwolf@redhat.com \
--cc=lvivier@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.