[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/2] vhost-user-blk-test: fix Coverity open(2) false posit
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2 1/2] vhost-user-blk-test: fix Coverity open(2) false positives |
Date: |
Tue, 1 Jun 2021 18:27:15 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 |
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>