It's been quiet on the arm front this week, so all I have is

these coverity fixes I posted a while back...

The following changes since commit 853546f8128476eefb701d4a55b2781bb3a46faa:

Merge tag 'pull-loongarch-20240322' of https://gitlab.com/gaosong/qemu into staging (2024-03-22 10:59:57 +0000)

https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20240325

for you to fetch changes up to 55c79639d553c1b7a82b4cde781ad5f316f45b0e:

tests/qtest/libqtest.c: Check for g_setenv() failure (2024-03-25 10:41:01 +0000)

* Fixes for seven minor coverity issues

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(-)

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.

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

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

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

--- a/tests/qtest/npcm7xx_emc-test.c

+++ b/tests/qtest/npcm7xx_emc-test.c

@@ -XXX,XX +XXX,XX @@ 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);

@@ -XXX,XX +XXX,XX @@ 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

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

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

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

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

--- a/tests/unit/socket-helpers.c

+++ b/tests/unit/socket-helpers.c

@@ -XXX,XX +XXX,XX @@ 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);

@@ -XXX,XX +XXX,XX @@ void socket_check_afunix_support(bool *has_afunix)

*has_afunix = (fd >= 0);

#endif

+ if (*has_afunix) {

+ close(fd);

return;

2.34.1

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.

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

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

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

--- a/net/af-xdp.c

+++ b/net/af-xdp.c

@@ -XXX,XX +XXX,XX @@ 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;

AFXDPState *s;

@@ -XXX,XX +XXX,XX @@ 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

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.

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

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

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

--- a/hw/misc/pca9554.c

+++ b/hw/misc/pca9554.c

@@ -XXX,XX +XXX,XX @@ 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;

}

@@ -XXX,XX +XXX,XX @@ 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

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.

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

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

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

Message-id: 20240312183810.557768-6-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

--- a/hw/nvram/mac_nvram.c

+++ b/hw/nvram/mac_nvram.c

@@ -XXX,XX +XXX,XX @@ 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

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>

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

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

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

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

--- a/tests/unit/test-throttle.c

+++ b/tests/unit/test-throttle.c

@@ -XXX,XX +XXX,XX @@ 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

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.

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

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

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

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

--- a/tests/qtest/libqtest.c

+++ b/tests/qtest/libqtest.c

@@ -XXX,XX +XXX,XX @@ 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