From 80e9791a93b856ae959cf0efa04cee53390ed000 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 25 Mar 2024 10:40:59 +0000 Subject: [PATCH 1/7] tests/qtest/npcm7xx_emc_test: Don't leak cmd_line In test_rx() and test_tx() we allocate a GString *cmd_line but never free it. This is pretty harmless in a test case, but Coverity spotted it. Resolves: Coverity CID 1507122 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Thomas Huth Message-id: 20240312183810.557768-2-peter.maydell@linaro.org --- tests/qtest/npcm7xx_emc-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qtest/npcm7xx_emc-test.c b/tests/qtest/npcm7xx_emc-test.c index 63f6cadb5c..2e1a1a6d70 100644 --- a/tests/qtest/npcm7xx_emc-test.c +++ b/tests/qtest/npcm7xx_emc-test.c @@ -789,7 +789,7 @@ static void emc_test_ptle(QTestState *qts, const EMCModule *mod, int fd) static void test_tx(gconstpointer test_data) { const TestData *td = test_data; - GString *cmd_line = g_string_new("-machine quanta-gsj"); + g_autoptr(GString) cmd_line = g_string_new("-machine quanta-gsj"); int *test_sockets = packet_test_init(emc_module_index(td->module), cmd_line); QTestState *qts = qtest_init(cmd_line->str); @@ -814,7 +814,7 @@ static void test_tx(gconstpointer test_data) static void test_rx(gconstpointer test_data) { const TestData *td = test_data; - GString *cmd_line = g_string_new("-machine quanta-gsj"); + g_autoptr(GString) cmd_line = g_string_new("-machine quanta-gsj"); int *test_sockets = packet_test_init(emc_module_index(td->module), cmd_line); QTestState *qts = qtest_init(cmd_line->str); From e921e00d4ba6840063d69cb637331d0dc4905e4b Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 25 Mar 2024 10:41:00 +0000 Subject: [PATCH 2/7] tests/unit/socket-helpers: Don't close(-1) In socket_check_afunix_support() we call socket(PF_UNIX, SOCK_STREAM, 0) to see if it works, but we call close() on the result whether it worked or not. Only close the fd if the socket() call succeeded. Spotted by Coverity. Resolves: Coverity CID 1497481 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Thomas Huth Message-id: 20240312183810.557768-3-peter.maydell@linaro.org --- tests/unit/socket-helpers.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/socket-helpers.c b/tests/unit/socket-helpers.c index 6de27baee2..f3439cc4e5 100644 --- a/tests/unit/socket-helpers.c +++ b/tests/unit/socket-helpers.c @@ -160,7 +160,6 @@ void socket_check_afunix_support(bool *has_afunix) int fd; fd = socket(PF_UNIX, SOCK_STREAM, 0); - close(fd); #ifdef _WIN32 *has_afunix = (fd != (int)INVALID_SOCKET); @@ -168,5 +167,8 @@ void socket_check_afunix_support(bool *has_afunix) *has_afunix = (fd >= 0); #endif + if (*has_afunix) { + close(fd); + } return; } From bed150be5b94ee499384fa6d052c0cb398a20d95 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 25 Mar 2024 10:41:00 +0000 Subject: [PATCH 3/7] net/af-xdp.c: Don't leak sock_fds array in net_init_af_xdp() In net_init_af_xdp() we parse the arguments and allocate a buffer of ints into sock_fds. However, although we free this in the error exit path, we don't ever free it in the successful return path. Coverity spots this leak. Switch to g_autofree so we don't need to manually free the array. Resolves: Coverity CID 1534906 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Thomas Huth Message-id: 20240312183810.557768-4-peter.maydell@linaro.org --- net/af-xdp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/net/af-xdp.c b/net/af-xdp.c index 38e600703a..01c5fb914e 100644 --- a/net/af-xdp.c +++ b/net/af-xdp.c @@ -446,7 +446,7 @@ int net_init_af_xdp(const Netdev *netdev, NetClientState *nc, *nc0 = NULL; unsigned int ifindex; uint32_t prog_id = 0; - int *sock_fds = NULL; + g_autofree int *sock_fds = NULL; int64_t i, queues; Error *err = NULL; AFXDPState *s; @@ -516,7 +516,6 @@ int net_init_af_xdp(const Netdev *netdev, return 0; err: - g_free(sock_fds); if (nc0) { qemu_del_net_client(nc0); } From c67f7580697198800c57ced59f1dfbce1aaeb4ae Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 25 Mar 2024 10:41:00 +0000 Subject: [PATCH 4/7] hw/misc/pca9554: Correct error check bounds in get/set pin functions In pca9554_get_pin() and pca9554_set_pin(), we try to detect an incorrect pin value, but we get the condition wrong, using ">" when ">=" was intended. This has no actual effect, because in pca9554_initfn() we use the correct test when creating the properties and so we'll never be called with an out of range value. However, Coverity complains about the mismatch between the check and the later use of the pin value in a shift operation. Use the correct condition. Resolves: Coverity CID 1534917 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Thomas Huth Message-id: 20240312183810.557768-5-peter.maydell@linaro.org --- hw/misc/pca9554.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c index 778b32e443..5e31696797 100644 --- a/hw/misc/pca9554.c +++ b/hw/misc/pca9554.c @@ -160,7 +160,7 @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name, error_setg(errp, "%s: error reading %s", __func__, name); return; } - if (pin < 0 || pin > PCA9554_PIN_COUNT) { + if (pin < 0 || pin >= PCA9554_PIN_COUNT) { error_setg(errp, "%s invalid pin %s", __func__, name); return; } @@ -187,7 +187,7 @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name, error_setg(errp, "%s: error reading %s", __func__, name); return; } - if (pin < 0 || pin > PCA9554_PIN_COUNT) { + if (pin < 0 || pin >= PCA9554_PIN_COUNT) { error_setg(errp, "%s invalid pin %s", __func__, name); return; } From b13ba381ca4d0b3e96a9e5bd138a1f3e11b5a637 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 25 Mar 2024 10:41:01 +0000 Subject: [PATCH 5/7] hw/nvram/mac_nvram: Report failure to write data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There's no way for the macio_nvram device to report failure to write data, but we can at least report it to the user with error_report() as we do in other devices like xlnx-efuse. Spotted by Coverity. Resolves: Coverity CID 1507628 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Thomas Huth Reviewed-by: Philippe Mathieu-Daudé Message-id: 20240312183810.557768-6-peter.maydell@linaro.org --- hw/nvram/mac_nvram.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c index 5f9d16fb3e..fe9df9fa35 100644 --- a/hw/nvram/mac_nvram.c +++ b/hw/nvram/mac_nvram.c @@ -33,6 +33,7 @@ #include "migration/vmstate.h" #include "qemu/cutils.h" #include "qemu/module.h" +#include "qemu/error-report.h" #include "trace.h" #include @@ -48,7 +49,10 @@ static void macio_nvram_writeb(void *opaque, hwaddr addr, trace_macio_nvram_write(addr, value); s->data[addr] = value; if (s->blk) { - blk_pwrite(s->blk, addr, 1, &s->data[addr], 0); + if (blk_pwrite(s->blk, addr, 1, &s->data[addr], 0) < 0) { + error_report("%s: write of NVRAM data to backing store failed", + blk_name(s->blk)); + } } } From 43199b13938aa693b2f10d8d17c59eebfe4e40c5 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 25 Mar 2024 10:41:01 +0000 Subject: [PATCH 6/7] tests/unit/test-throttle: Avoid unintended integer division MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In test_compute_wait() we do double units = bkt.max / 10; which does an integer division and then assigns it to a double variable, and similarly later on in the expression for an assertion. Use 10.0 so that we do a floating point division and calculate the exact value, rather than doing an integer division. Spotted by Coverity. Resolves: Coverity CID 1432564 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Thomas Huth Reviewed-by: Philippe Mathieu-Daudé Message-id: 20240312183810.557768-7-peter.maydell@linaro.org --- tests/unit/test-throttle.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c index 2146cfacd3..24032a0266 100644 --- a/tests/unit/test-throttle.c +++ b/tests/unit/test-throttle.c @@ -127,13 +127,13 @@ static void test_compute_wait(void) bkt.avg = 10; bkt.max = 200; for (i = 0; i < 22; i++) { - double units = bkt.max / 10; + double units = bkt.max / 10.0; bkt.level += units; bkt.burst_level += units; throttle_leak_bucket(&bkt, NANOSECONDS_PER_SECOND / 10); wait = throttle_compute_wait(&bkt); g_assert(double_cmp(bkt.burst_level, 0)); - g_assert(double_cmp(bkt.level, (i + 1) * (bkt.max - bkt.avg) / 10)); + g_assert(double_cmp(bkt.level, (i + 1) * (bkt.max - bkt.avg) / 10.0)); /* We can do bursts for the 2 seconds we have configured in * burst_length. We have 100 extra milliseconds of burst * because bkt.level has been leaking during this time. From fe3e38390126c2202292911c49d46fc7ee4a163a Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 25 Mar 2024 10:41:01 +0000 Subject: [PATCH 7/7] tests/qtest/libqtest.c: Check for g_setenv() failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverity points out that g_setenv() can fail and we don't check for this in qtest_inproc_init(). In practice this will only fail if a memory allocation failed in setenv() or if the caller passed an invalid architecture name (e.g. one with an '=' in it), so rather than requiring the callsite to check for failure, make g_setenv() failure fatal here, similarly to what we did in commit aca68d95c515. Resolves: Coverity CID 1497485 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Thomas Huth Reviewed-by: Philippe Mathieu-Daudé Message-id: 20240312183810.557768-8-peter.maydell@linaro.org --- tests/qtest/libqtest.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index f33a210861..d8f80d335e 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -1814,7 +1814,11 @@ QTestState *qtest_inproc_init(QTestState **s, bool log, const char* arch, * way, qtest_get_arch works for inproc qtest. */ gchar *bin_path = g_strconcat("/qemu-system-", arch, NULL); - g_setenv("QTEST_QEMU_BINARY", bin_path, 0); + if (!g_setenv("QTEST_QEMU_BINARY", bin_path, 0)) { + fprintf(stderr, + "Could not set environment variable QTEST_QEMU_BINARY\n"); + exit(1); + } g_free(bin_path); return qts;