[PATCH 0/7] various: 7 minor Coverity fixes

[PATCH 0/7] various: 7 minor Coverity fixes

[Peter Maydell]

This patchset fixes seven minor Coverity issues:
 * four minor nits in test code
 * a happens-once memory leak in the af-xdp netdev
 * a harmless wrong-bounds-check in pca9554
 * add a report-write-errors-to-user in mac_nvram

I don't think any of these are worth backporting to stable;
I just picked them as easy fixes that reduce the number of open
Coverity issues we have.

thanks
-- PMM

Peter Maydell (7):
  tests/qtest/npcm7xx_emc_test: Don't leak cmd_line
  tests/unit/socket-helpers: Don't close(-1)
  net/af-xdp.c: Don't leak sock_fds array in net_init_af_xdp()
  hw/misc/pca9554: Correct error check bounds in get/set pin functions
  hw/nvram/mac_nvram: Report failure to write data
  tests/unit/test-throttle: Avoid unintended integer division
  tests/qtest/libqtest.c: Check for g_setenv() failure

 hw/misc/pca9554.c              | 4 ++--
 hw/nvram/mac_nvram.c           | 5 ++++-
 net/af-xdp.c                   | 3 +--
 tests/qtest/libqtest.c         | 6 +++++-
 tests/qtest/npcm7xx_emc-test.c | 4 ++--
 tests/unit/socket-helpers.c    | 4 +++-
 tests/unit/test-throttle.c     | 4 ++--
 7 files changed, 19 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH 1/7] tests/qtest/npcm7xx_emc_test: Don't leak cmd_line

[Peter Maydell]

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 <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 63f6cadb5cc..2e1a1a6d702 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);
-- 
2.34.1

Re: [PATCH 1/7] tests/qtest/npcm7xx_emc_test: Don't leak cmd_line

[Thomas Huth]

On 12/03/2024 19.38, Peter Maydell wrote:
> 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 <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 63f6cadb5cc..2e1a1a6d702 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);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/7] tests/qtest/npcm7xx_emc_test: Don't leak cmd_line

[Thomas Huth]

On 12/03/2024 19.38, Peter Maydell wrote:
> 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 <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 63f6cadb5cc..2e1a1a6d702 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);

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 2/7] tests/unit/socket-helpers: Don't close(-1)

[Peter Maydell]

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 <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 6de27baee2e..f3439cc4e52 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;
 }
-- 
2.34.1

Re: [PATCH 2/7] tests/unit/socket-helpers: Don't close(-1)

[Thomas Huth]

On 12/03/2024 19.38, Peter Maydell wrote:
> 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 <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 6de27baee2e..f3439cc4e52 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;
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 3/7] net/af-xdp.c: Don't leak sock_fds array in net_init_af_xdp()

[Peter Maydell]

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 <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 38e600703a3..01c5fb914ec 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);
     }
-- 
2.34.1

[PATCH 4/7] hw/misc/pca9554: Correct error check bounds in get/set pin functions

[Peter Maydell]

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 <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 778b32e4430..5e31696797d 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;
     }
-- 
2.34.1

[PATCH 5/7] hw/nvram/mac_nvram: Report failure to write data

[Peter Maydell]

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 <peter.maydell@linaro.org>
---
 hw/nvram/mac_nvram.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
index 5f9d16fb3e3..59277fbc776 100644
--- a/hw/nvram/mac_nvram.c
+++ b/hw/nvram/mac_nvram.c
@@ -48,7 +48,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));
+        }
     }
 }
 
-- 
2.34.1

Re: [PATCH 5/7] hw/nvram/mac_nvram: Report failure to write data

[Philippe Mathieu-Daudé]

On 12/3/24 19:38, Peter Maydell wrote:
> 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 <peter.maydell@linaro.org>
> ---
>   hw/nvram/mac_nvram.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 6/7] tests/unit/test-throttle: Avoid unintended integer division

[Peter Maydell]

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 <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 2146cfacd36..24032a02667 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.
-- 
2.34.1

Re: [PATCH 6/7] tests/unit/test-throttle: Avoid unintended integer division

[Thomas Huth]

On 12/03/2024 19.38, Peter Maydell wrote:
> 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 <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 2146cfacd36..24032a02667 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.

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 7/7] tests/qtest/libqtest.c: Check for g_setenv() failure

[Peter Maydell]

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 <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 f33a2108610..d8f80d335e7 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;
-- 
2.34.1

Re: [PATCH 7/7] tests/qtest/libqtest.c: Check for g_setenv() failure

[Philippe Mathieu-Daudé]

On 12/3/24 19:38, Peter Maydell wrote:
> 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 <peter.maydell@linaro.org>
> ---
>   tests/qtest/libqtest.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/7] various: 7 minor Coverity fixes

[Richard Henderson]

On 3/12/24 08:38, Peter Maydell wrote:
> Peter Maydell (7):
>    tests/qtest/npcm7xx_emc_test: Don't leak cmd_line
>    tests/unit/socket-helpers: Don't close(-1)
>    net/af-xdp.c: Don't leak sock_fds array in net_init_af_xdp()
>    hw/misc/pca9554: Correct error check bounds in get/set pin functions
>    hw/nvram/mac_nvram: Report failure to write data
>    tests/unit/test-throttle: Avoid unintended integer division
>    tests/qtest/libqtest.c: Check for g_setenv() failure

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~