1 | The following changes since commit e3debd5e7d0ce031356024878a0a18b9d109354a: | 1 | It's been quiet on the arm front this week, so all I have is |
---|---|---|---|
2 | these coverity fixes I posted a while back... | ||
2 | 3 | ||
3 | Merge tag 'pull-request-2023-03-24' of https://gitlab.com/thuth/qemu into staging (2023-03-24 16:08:46 +0000) | 4 | -- PMM |
5 | |||
6 | The following changes since commit 853546f8128476eefb701d4a55b2781bb3a46faa: | ||
7 | |||
8 | Merge tag 'pull-loongarch-20240322' of https://gitlab.com/gaosong/qemu into staging (2024-03-22 10:59:57 +0000) | ||
4 | 9 | ||
5 | are available in the Git repository at: | 10 | are available in the Git repository at: |
6 | 11 | ||
7 | https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20230328 | 12 | https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20240325 |
8 | 13 | ||
9 | for you to fetch changes up to 46e3b237c52e0c48bfd81bce020b51fbe300b23a: | 14 | for you to fetch changes up to 55c79639d553c1b7a82b4cde781ad5f316f45b0e: |
10 | 15 | ||
11 | target/arm/gdbstub: Only advertise M-profile features if TCG available (2023-03-28 10:53:40 +0100) | 16 | tests/qtest/libqtest.c: Check for g_setenv() failure (2024-03-25 10:41:01 +0000) |
12 | 17 | ||
13 | ---------------------------------------------------------------- | 18 | ---------------------------------------------------------------- |
14 | target-arm queue: | 19 | target-arm queue: |
15 | * fix part of the "TCG-disabled builds are broken" issue | 20 | * Fixes for seven minor coverity issues |
16 | 21 | ||
17 | ---------------------------------------------------------------- | 22 | ---------------------------------------------------------------- |
18 | Philippe Mathieu-Daudé (1): | 23 | Peter Maydell (7): |
19 | target/arm/gdbstub: Only advertise M-profile features if TCG available | 24 | tests/qtest/npcm7xx_emc_test: Don't leak cmd_line |
25 | tests/unit/socket-helpers: Don't close(-1) | ||
26 | net/af-xdp.c: Don't leak sock_fds array in net_init_af_xdp() | ||
27 | hw/misc/pca9554: Correct error check bounds in get/set pin functions | ||
28 | hw/nvram/mac_nvram: Report failure to write data | ||
29 | tests/unit/test-throttle: Avoid unintended integer division | ||
30 | tests/qtest/libqtest.c: Check for g_setenv() failure | ||
20 | 31 | ||
21 | target/arm/gdbstub.c | 5 +++-- | 32 | hw/misc/pca9554.c | 4 ++-- |
22 | 1 file changed, 3 insertions(+), 2 deletions(-) | 33 | hw/nvram/mac_nvram.c | 5 ++++- |
23 | 34 | net/af-xdp.c | 3 +-- | |
35 | tests/qtest/libqtest.c | 6 +++++- | ||
36 | tests/qtest/npcm7xx_emc-test.c | 4 ++-- | ||
37 | tests/unit/socket-helpers.c | 4 +++- | ||
38 | tests/unit/test-throttle.c | 4 ++-- | ||
39 | 7 files changed, 19 insertions(+), 11 deletions(-) | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | In test_rx() and test_tx() we allocate a GString *cmd_line | ||
2 | but never free it. This is pretty harmless in a test case, but | ||
3 | Coverity spotted it. | ||
1 | 4 | ||
5 | Resolves: Coverity CID 1507122 | ||
6 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
7 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
8 | Reviewed-by: Thomas Huth <thuth@redhat.com> | ||
9 | Message-id: 20240312183810.557768-2-peter.maydell@linaro.org | ||
10 | --- | ||
11 | tests/qtest/npcm7xx_emc-test.c | 4 ++-- | ||
12 | 1 file changed, 2 insertions(+), 2 deletions(-) | ||
13 | |||
14 | diff --git a/tests/qtest/npcm7xx_emc-test.c b/tests/qtest/npcm7xx_emc-test.c | ||
15 | index XXXXXXX..XXXXXXX 100644 | ||
16 | --- a/tests/qtest/npcm7xx_emc-test.c | ||
17 | +++ b/tests/qtest/npcm7xx_emc-test.c | ||
18 | @@ -XXX,XX +XXX,XX @@ static void emc_test_ptle(QTestState *qts, const EMCModule *mod, int fd) | ||
19 | static void test_tx(gconstpointer test_data) | ||
20 | { | ||
21 | const TestData *td = test_data; | ||
22 | - GString *cmd_line = g_string_new("-machine quanta-gsj"); | ||
23 | + g_autoptr(GString) cmd_line = g_string_new("-machine quanta-gsj"); | ||
24 | int *test_sockets = packet_test_init(emc_module_index(td->module), | ||
25 | cmd_line); | ||
26 | QTestState *qts = qtest_init(cmd_line->str); | ||
27 | @@ -XXX,XX +XXX,XX @@ static void test_tx(gconstpointer test_data) | ||
28 | static void test_rx(gconstpointer test_data) | ||
29 | { | ||
30 | const TestData *td = test_data; | ||
31 | - GString *cmd_line = g_string_new("-machine quanta-gsj"); | ||
32 | + g_autoptr(GString) cmd_line = g_string_new("-machine quanta-gsj"); | ||
33 | int *test_sockets = packet_test_init(emc_module_index(td->module), | ||
34 | cmd_line); | ||
35 | QTestState *qts = qtest_init(cmd_line->str); | ||
36 | -- | ||
37 | 2.34.1 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | In socket_check_afunix_support() we call socket(PF_UNIX, SOCK_STREAM, 0) | ||
2 | to see if it works, but we call close() on the result whether it | ||
3 | worked or not. Only close the fd if the socket() call succeeded. | ||
4 | Spotted by Coverity. | ||
1 | 5 | ||
6 | Resolves: Coverity CID 1497481 | ||
7 | |||
8 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
9 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
10 | Reviewed-by: Thomas Huth <thuth@redhat.com> | ||
11 | Message-id: 20240312183810.557768-3-peter.maydell@linaro.org | ||
12 | --- | ||
13 | tests/unit/socket-helpers.c | 4 +++- | ||
14 | 1 file changed, 3 insertions(+), 1 deletion(-) | ||
15 | |||
16 | diff --git a/tests/unit/socket-helpers.c b/tests/unit/socket-helpers.c | ||
17 | index XXXXXXX..XXXXXXX 100644 | ||
18 | --- a/tests/unit/socket-helpers.c | ||
19 | +++ b/tests/unit/socket-helpers.c | ||
20 | @@ -XXX,XX +XXX,XX @@ void socket_check_afunix_support(bool *has_afunix) | ||
21 | int fd; | ||
22 | |||
23 | fd = socket(PF_UNIX, SOCK_STREAM, 0); | ||
24 | - close(fd); | ||
25 | |||
26 | #ifdef _WIN32 | ||
27 | *has_afunix = (fd != (int)INVALID_SOCKET); | ||
28 | @@ -XXX,XX +XXX,XX @@ void socket_check_afunix_support(bool *has_afunix) | ||
29 | *has_afunix = (fd >= 0); | ||
30 | #endif | ||
31 | |||
32 | + if (*has_afunix) { | ||
33 | + close(fd); | ||
34 | + } | ||
35 | return; | ||
36 | } | ||
37 | -- | ||
38 | 2.34.1 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | In net_init_af_xdp() we parse the arguments and allocate | ||
2 | a buffer of ints into sock_fds. However, although we | ||
3 | free this in the error exit path, we don't ever free it | ||
4 | in the successful return path. Coverity spots this leak. | ||
1 | 5 | ||
6 | Switch to g_autofree so we don't need to manually free the | ||
7 | array. | ||
8 | |||
9 | Resolves: Coverity CID 1534906 | ||
10 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
11 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
12 | Reviewed-by: Thomas Huth <thuth@redhat.com> | ||
13 | Message-id: 20240312183810.557768-4-peter.maydell@linaro.org | ||
14 | --- | ||
15 | net/af-xdp.c | 3 +-- | ||
16 | 1 file changed, 1 insertion(+), 2 deletions(-) | ||
17 | |||
18 | diff --git a/net/af-xdp.c b/net/af-xdp.c | ||
19 | index XXXXXXX..XXXXXXX 100644 | ||
20 | --- a/net/af-xdp.c | ||
21 | +++ b/net/af-xdp.c | ||
22 | @@ -XXX,XX +XXX,XX @@ int net_init_af_xdp(const Netdev *netdev, | ||
23 | NetClientState *nc, *nc0 = NULL; | ||
24 | unsigned int ifindex; | ||
25 | uint32_t prog_id = 0; | ||
26 | - int *sock_fds = NULL; | ||
27 | + g_autofree int *sock_fds = NULL; | ||
28 | int64_t i, queues; | ||
29 | Error *err = NULL; | ||
30 | AFXDPState *s; | ||
31 | @@ -XXX,XX +XXX,XX @@ int net_init_af_xdp(const Netdev *netdev, | ||
32 | return 0; | ||
33 | |||
34 | err: | ||
35 | - g_free(sock_fds); | ||
36 | if (nc0) { | ||
37 | qemu_del_net_client(nc0); | ||
38 | } | ||
39 | -- | ||
40 | 2.34.1 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | In pca9554_get_pin() and pca9554_set_pin(), we try to detect an | ||
2 | incorrect pin value, but we get the condition wrong, using ">" | ||
3 | when ">=" was intended. | ||
1 | 4 | ||
5 | This has no actual effect, because in pca9554_initfn() we | ||
6 | use the correct test when creating the properties and so | ||
7 | we'll never be called with an out of range value. However, | ||
8 | Coverity complains about the mismatch between the check and | ||
9 | the later use of the pin value in a shift operation. | ||
10 | |||
11 | Use the correct condition. | ||
12 | |||
13 | Resolves: Coverity CID 1534917 | ||
14 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
15 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
16 | Reviewed-by: Thomas Huth <thuth@redhat.com> | ||
17 | Message-id: 20240312183810.557768-5-peter.maydell@linaro.org | ||
18 | --- | ||
19 | hw/misc/pca9554.c | 4 ++-- | ||
20 | 1 file changed, 2 insertions(+), 2 deletions(-) | ||
21 | |||
22 | diff --git a/hw/misc/pca9554.c b/hw/misc/pca9554.c | ||
23 | index XXXXXXX..XXXXXXX 100644 | ||
24 | --- a/hw/misc/pca9554.c | ||
25 | +++ b/hw/misc/pca9554.c | ||
26 | @@ -XXX,XX +XXX,XX @@ static void pca9554_get_pin(Object *obj, Visitor *v, const char *name, | ||
27 | error_setg(errp, "%s: error reading %s", __func__, name); | ||
28 | return; | ||
29 | } | ||
30 | - if (pin < 0 || pin > PCA9554_PIN_COUNT) { | ||
31 | + if (pin < 0 || pin >= PCA9554_PIN_COUNT) { | ||
32 | error_setg(errp, "%s invalid pin %s", __func__, name); | ||
33 | return; | ||
34 | } | ||
35 | @@ -XXX,XX +XXX,XX @@ static void pca9554_set_pin(Object *obj, Visitor *v, const char *name, | ||
36 | error_setg(errp, "%s: error reading %s", __func__, name); | ||
37 | return; | ||
38 | } | ||
39 | - if (pin < 0 || pin > PCA9554_PIN_COUNT) { | ||
40 | + if (pin < 0 || pin >= PCA9554_PIN_COUNT) { | ||
41 | error_setg(errp, "%s invalid pin %s", __func__, name); | ||
42 | return; | ||
43 | } | ||
44 | -- | ||
45 | 2.34.1 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | There's no way for the macio_nvram device to report failure to write | ||
2 | data, but we can at least report it to the user with error_report() | ||
3 | as we do in other devices like xlnx-efuse. | ||
1 | 4 | ||
5 | Spotted by Coverity. | ||
6 | |||
7 | Resolves: Coverity CID 1507628 | ||
8 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
9 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
10 | Reviewed-by: Thomas Huth <thuth@redhat.com> | ||
11 | Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> | ||
12 | Message-id: 20240312183810.557768-6-peter.maydell@linaro.org | ||
13 | --- | ||
14 | hw/nvram/mac_nvram.c | 5 ++++- | ||
15 | 1 file changed, 4 insertions(+), 1 deletion(-) | ||
16 | |||
17 | diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c | ||
18 | index XXXXXXX..XXXXXXX 100644 | ||
19 | --- a/hw/nvram/mac_nvram.c | ||
20 | +++ b/hw/nvram/mac_nvram.c | ||
21 | @@ -XXX,XX +XXX,XX @@ static void macio_nvram_writeb(void *opaque, hwaddr addr, | ||
22 | trace_macio_nvram_write(addr, value); | ||
23 | s->data[addr] = value; | ||
24 | if (s->blk) { | ||
25 | - blk_pwrite(s->blk, addr, 1, &s->data[addr], 0); | ||
26 | + if (blk_pwrite(s->blk, addr, 1, &s->data[addr], 0) < 0) { | ||
27 | + error_report("%s: write of NVRAM data to backing store failed", | ||
28 | + blk_name(s->blk)); | ||
29 | + } | ||
30 | } | ||
31 | } | ||
32 | |||
33 | -- | ||
34 | 2.34.1 | ||
35 | |||
36 | diff view generated by jsdifflib |
New patch | |||
---|---|---|---|
1 | In test_compute_wait() we do | ||
2 | double units = bkt.max / 10; | ||
3 | which does an integer division and then assigns it to a double variable, | ||
4 | and similarly later on in the expression for an assertion. | ||
1 | 5 | ||
6 | Use 10.0 so that we do a floating point division and calculate the | ||
7 | exact value, rather than doing an integer division. | ||
8 | |||
9 | Spotted by Coverity. | ||
10 | |||
11 | Resolves: Coverity CID 1432564 | ||
12 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
13 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
14 | Reviewed-by: Thomas Huth <thuth@redhat.com> | ||
15 | Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> | ||
16 | Message-id: 20240312183810.557768-7-peter.maydell@linaro.org | ||
17 | --- | ||
18 | tests/unit/test-throttle.c | 4 ++-- | ||
19 | 1 file changed, 2 insertions(+), 2 deletions(-) | ||
20 | |||
21 | diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c | ||
22 | index XXXXXXX..XXXXXXX 100644 | ||
23 | --- a/tests/unit/test-throttle.c | ||
24 | +++ b/tests/unit/test-throttle.c | ||
25 | @@ -XXX,XX +XXX,XX @@ static void test_compute_wait(void) | ||
26 | bkt.avg = 10; | ||
27 | bkt.max = 200; | ||
28 | for (i = 0; i < 22; i++) { | ||
29 | - double units = bkt.max / 10; | ||
30 | + double units = bkt.max / 10.0; | ||
31 | bkt.level += units; | ||
32 | bkt.burst_level += units; | ||
33 | throttle_leak_bucket(&bkt, NANOSECONDS_PER_SECOND / 10); | ||
34 | wait = throttle_compute_wait(&bkt); | ||
35 | g_assert(double_cmp(bkt.burst_level, 0)); | ||
36 | - g_assert(double_cmp(bkt.level, (i + 1) * (bkt.max - bkt.avg) / 10)); | ||
37 | + g_assert(double_cmp(bkt.level, (i + 1) * (bkt.max - bkt.avg) / 10.0)); | ||
38 | /* We can do bursts for the 2 seconds we have configured in | ||
39 | * burst_length. We have 100 extra milliseconds of burst | ||
40 | * because bkt.level has been leaking during this time. | ||
41 | -- | ||
42 | 2.34.1 | ||
43 | |||
44 | diff view generated by jsdifflib |
1 | From: Philippe Mathieu-Daudé <philmd@linaro.org> | 1 | Coverity points out that g_setenv() can fail and we don't |
---|---|---|---|
2 | check for this in qtest_inproc_init(). In practice this will | ||
3 | only fail if a memory allocation failed in setenv() or if | ||
4 | the caller passed an invalid architecture name (e.g. one | ||
5 | with an '=' in it), so rather than requiring the callsite | ||
6 | to check for failure, make g_setenv() failure fatal here, | ||
7 | similarly to what we did in commit aca68d95c515. | ||
2 | 8 | ||
3 | Cortex-M profile is only emulable from TCG accelerator. Restrict | 9 | Resolves: Coverity CID 1497485 |
4 | the GDBstub features to its availability in order to avoid a link | 10 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
5 | error when TCG is not enabled: | 11 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> |
12 | Reviewed-by: Thomas Huth <thuth@redhat.com> | ||
13 | Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> | ||
14 | Message-id: 20240312183810.557768-8-peter.maydell@linaro.org | ||
15 | --- | ||
16 | tests/qtest/libqtest.c | 6 +++++- | ||
17 | 1 file changed, 5 insertions(+), 1 deletion(-) | ||
6 | 18 | ||
7 | Undefined symbols for architecture arm64: | 19 | diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c |
8 | "_arm_v7m_get_sp_ptr", referenced from: | ||
9 | _m_sysreg_get in target_arm_gdbstub.c.o | ||
10 | "_arm_v7m_mrs_control", referenced from: | ||
11 | _arm_gdb_get_m_systemreg in target_arm_gdbstub.c.o | ||
12 | ld: symbol(s) not found for architecture arm64 | ||
13 | clang: error: linker command failed with exit code 1 (use -v to see invocation) | ||
14 | |||
15 | Fixes: 7d8b28b8b5 ("target/arm: Implement gdbstub m-profile systemreg and secext") | ||
16 | Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> | ||
17 | Reviewed-by: Richard Henderson <richard.henderson@linaro.org> | ||
18 | Reviewed-by: Alex Bennée <alex.bennee@linaro.org> | ||
19 | Message-id: 20230322142902.69511-3-philmd@linaro.org | ||
20 | [PMM: add #include since I cherry-picked this patch from the series] | ||
21 | Signed-off-by: Peter Maydell <peter.maydell@linaro.org> | ||
22 | --- | ||
23 | target/arm/gdbstub.c | 5 +++-- | ||
24 | 1 file changed, 3 insertions(+), 2 deletions(-) | ||
25 | |||
26 | diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c | ||
27 | index XXXXXXX..XXXXXXX 100644 | 20 | index XXXXXXX..XXXXXXX 100644 |
28 | --- a/target/arm/gdbstub.c | 21 | --- a/tests/qtest/libqtest.c |
29 | +++ b/target/arm/gdbstub.c | 22 | +++ b/tests/qtest/libqtest.c |
30 | @@ -XXX,XX +XXX,XX @@ | 23 | @@ -XXX,XX +XXX,XX @@ QTestState *qtest_inproc_init(QTestState **s, bool log, const char* arch, |
31 | #include "cpu.h" | 24 | * way, qtest_get_arch works for inproc qtest. |
32 | #include "exec/gdbstub.h" | 25 | */ |
33 | #include "gdbstub/helpers.h" | 26 | gchar *bin_path = g_strconcat("/qemu-system-", arch, NULL); |
34 | +#include "sysemu/tcg.h" | 27 | - g_setenv("QTEST_QEMU_BINARY", bin_path, 0); |
35 | #include "internals.h" | 28 | + if (!g_setenv("QTEST_QEMU_BINARY", bin_path, 0)) { |
36 | #include "cpregs.h" | 29 | + fprintf(stderr, |
37 | 30 | + "Could not set environment variable QTEST_QEMU_BINARY\n"); | |
38 | @@ -XXX,XX +XXX,XX @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) | 31 | + exit(1); |
39 | 2, "arm-vfp-sysregs.xml", 0); | 32 | + } |
40 | } | 33 | g_free(bin_path); |
41 | } | 34 | |
42 | - if (cpu_isar_feature(aa32_mve, cpu)) { | 35 | return qts; |
43 | + if (cpu_isar_feature(aa32_mve, cpu) && tcg_enabled()) { | ||
44 | gdb_register_coprocessor(cs, mve_gdb_get_reg, mve_gdb_set_reg, | ||
45 | 1, "arm-m-profile-mve.xml", 0); | ||
46 | } | ||
47 | @@ -XXX,XX +XXX,XX @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) | ||
48 | arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs), | ||
49 | "system-registers.xml", 0); | ||
50 | |||
51 | - if (arm_feature(env, ARM_FEATURE_M)) { | ||
52 | + if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) { | ||
53 | gdb_register_coprocessor(cs, | ||
54 | arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg, | ||
55 | arm_gen_dynamic_m_systemreg_xml(cs, cs->gdb_num_regs), | ||
56 | -- | 36 | -- |
57 | 2.34.1 | 37 | 2.34.1 |
58 | 38 | ||
59 | 39 | diff view generated by jsdifflib |