[PATCH v2 0/4] block: Miscellaneous minor Coverity fixes

[PATCH v2 0/4] block: Miscellaneous minor Coverity fixes

[Peter Maydell]

This patchset is the remaining stragglers from my
first set of "minor Coverity fixes" patches posted a
couple of months back:
https://patchew.org/QEMU/20240731143617.3391947-1-peter.maydell@linaro.org/
Of that series, patches 3, 4, 5 and 6 are upstream now.

In this v2 series:
 * patch 1 (old patch 2) has had a long line wrapped;
   already reviewed
 * patch 2 (old patch 7) now has hex2decimal() return "UINT_MAX"
   instead of "-1"; already reviewed
 * patch 3 is new, and fixes an error in an iotests reference
   output file that's been lurking around for a few years now
 * patch 4 (replacement for old patch 1) takes Kevin Wolf's
   suggestion to make vdi SECTOR_SIZE a 64-bit constant so we
   don't have to carefully cast it every time we want to use
   it in a multiplication

Patches 3 and 4 need review; patch 4 passes "./check -vdi"
except that 297 (the python-linter) fails for unrelated reasons.

thanks
-- PMM

Peter Maydell (4):
  block/gluster: Use g_autofree for string in qemu_gluster_parse_json()
  block/ssh.c: Don't double-check that characters are hex digits
  tests/qemu-iotests/211.out: Update to expect MapEntry 'compressed'
    field
  block/vdi.c: Make SECTOR_SIZE constant 64-bits

 block/gluster.c            |  7 ++-----
 block/ssh.c                | 12 +++++++-----
 block/vdi.c                |  4 ++--
 tests/qemu-iotests/211.out |  6 +++---
 4 files changed, 14 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] block/gluster: Use g_autofree for string in qemu_gluster_parse_json()

[Peter Maydell]

In the loop in qemu_gluster_parse_json() we do:

    char *str = NULL;
    for(...) {
        str = g_strdup_printf(...);
        ...
        if (various errors) {
            goto out;
        }
        ...
        g_free(str);
        str = NULL;
    }
    return 0;
out:
    various cleanups;
    g_free(str);
    ...
    return -errno;

Coverity correctly complains that the assignment "str = NULL" at the
end of the loop is unnecessary, because we will either go back to the
top of the loop and overwrite it, or else we will exit the loop and
then exit the function without ever reading str again. The assignment
is there as defensive coding to ensure that str is only non-NULL if
it's a live allocation, so this is intentional.

We can make Coverity happier and simplify the code here by using
g_autofree, since we never need 'str' outside the loop.

Resolves: Coverity CID 1527385
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v1->v2: wrap overlong line
---
 block/gluster.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index f03d05251ef..e9c038042b3 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -514,7 +514,6 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
     SocketAddressList **tail;
     QDict *backing_options = NULL;
     Error *local_err = NULL;
-    char *str = NULL;
     const char *ptr;
     int i, type, num_servers;
 
@@ -547,7 +546,8 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
     tail = &gconf->server;
 
     for (i = 0; i < num_servers; i++) {
-        str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.", i);
+        g_autofree char *str = g_strdup_printf(GLUSTER_OPT_SERVER_PATTERN"%d.",
+                                               i);
         qdict_extract_subqdict(options, &backing_options, str);
 
         /* create opts info from runtime_type_opts list */
@@ -658,8 +658,6 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf,
 
         qobject_unref(backing_options);
         backing_options = NULL;
-        g_free(str);
-        str = NULL;
     }
 
     return 0;
@@ -668,7 +666,6 @@ out:
     error_propagate(errp, local_err);
     qapi_free_SocketAddress(gsconf);
     qemu_opts_del(opts);
-    g_free(str);
     qobject_unref(backing_options);
     errno = EINVAL;
     return -errno;
-- 
2.34.1

Re: [PATCH v2 1/4] block/gluster: Use g_autofree for string in qemu_gluster_parse_json()

[Richard Henderson]

On 10/8/24 09:47, Peter Maydell wrote:
> In the loop in qemu_gluster_parse_json() we do:
> 
>      char *str = NULL;
>      for(...) {
>          str = g_strdup_printf(...);
>          ...
>          if (various errors) {
>              goto out;
>          }
>          ...
>          g_free(str);
>          str = NULL;
>      }
>      return 0;
> out:
>      various cleanups;
>      g_free(str);
>      ...
>      return -errno;
> 
> Coverity correctly complains that the assignment "str = NULL" at the
> end of the loop is unnecessary, because we will either go back to the
> top of the loop and overwrite it, or else we will exit the loop and
> then exit the function without ever reading str again. The assignment
> is there as defensive coding to ensure that str is only non-NULL if
> it's a live allocation, so this is intentional.
> 
> We can make Coverity happier and simplify the code here by using
> g_autofree, since we never need 'str' outside the loop.
> 
> Resolves: Coverity CID 1527385
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> Reviewed-by: Kevin Wolf<kwolf@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
> v1->v2: wrap overlong line
> ---
>   block/gluster.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)

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

r~

[PATCH v2 2/4] block/ssh.c: Don't double-check that characters are hex digits

[Peter Maydell]

In compare_fingerprint() we effectively check whether the characters
in the fingerprint are valid hex digits twice: first we do so with
qemu_isxdigit(), but then the hex2decimal() function also has a code
path where it effectively detects an invalid digit and returns -1.
This causes Coverity to complain because it thinks that we might use
that -1 value in an expression where it would be an integer overflow.

Avoid the double-check of hex digit validity by testing the return
values from hex2decimal() rather than doing separate calls to
qemu_isxdigit().

Since this means we now use the illegal-character return value
from hex2decimal(), rewrite it from "-1" to "UINT_MAX", which
has the same effect since the return type is "unsigned" but
looks less confusing at the callsites when we detect it with
"c0 > 0xf".

Resolves: Coverity CID 1547813
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
v1->v2: make hex2decimal() return UINT_MAX, not -1
---
 block/ssh.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 871e1d47534..9f8140bcb68 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -364,7 +364,7 @@ static unsigned hex2decimal(char ch)
         return 10 + (ch - 'A');
     }
 
-    return -1;
+    return UINT_MAX;
 }
 
 /* Compare the binary fingerprint (hash of host key) with the
@@ -376,13 +376,15 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
     unsigned c;
 
     while (len > 0) {
+        unsigned c0, c1;
         while (*host_key_check == ':')
             host_key_check++;
-        if (!qemu_isxdigit(host_key_check[0]) ||
-            !qemu_isxdigit(host_key_check[1]))
+        c0 = hex2decimal(host_key_check[0]);
+        c1 = hex2decimal(host_key_check[1]);
+        if (c0 > 0xf || c1 > 0xf) {
             return 1;
-        c = hex2decimal(host_key_check[0]) * 16 +
-            hex2decimal(host_key_check[1]);
+        }
+        c = c0 * 16 + c1;
         if (c - *fingerprint != 0)
             return c - *fingerprint;
         fingerprint++;
-- 
2.34.1

Re: [PATCH v2 2/4] block/ssh.c: Don't double-check that characters are hex digits

[Richard Henderson]

On 10/8/24 09:47, Peter Maydell wrote:
> In compare_fingerprint() we effectively check whether the characters
> in the fingerprint are valid hex digits twice: first we do so with
> qemu_isxdigit(), but then the hex2decimal() function also has a code
> path where it effectively detects an invalid digit and returns -1.
> This causes Coverity to complain because it thinks that we might use
> that -1 value in an expression where it would be an integer overflow.
> 
> Avoid the double-check of hex digit validity by testing the return
> values from hex2decimal() rather than doing separate calls to
> qemu_isxdigit().
> 
> Since this means we now use the illegal-character return value
> from hex2decimal(), rewrite it from "-1" to "UINT_MAX", which
> has the same effect since the return type is "unsigned" but
> looks less confusing at the callsites when we detect it with
> "c0 > 0xf".
> 
> Resolves: Coverity CID 1547813
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> Reviewed-by: Kevin Wolf<kwolf@redhat.com>
> ---
> v1->v2: make hex2decimal() return UINT_MAX, not -1
> ---
>   block/ssh.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)

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

r~

[PATCH v2 3/4] tests/qemu-iotests/211.out: Update to expect MapEntry 'compressed' field

[Peter Maydell]

In commit 52b10c9c0c68e90f in 2023 the QAPI MapEntry struct was
updated to add a 'compressed' field. That commit updated a number
of iotest expected-output files, but missed 211, which is vdi
specific. The result is that
 ./check -vdi
and more specifically
 ./check -vdi 211
fails because the expected and actual output don't match.

Update the reference output.

Cc: qemu-stable@nongnu.org
Fixes: 52b10c9c0c68e90f ("qemu-img: map: report compressed data blocks")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qemu-iotests/211.out | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/211.out b/tests/qemu-iotests/211.out
index f02c75409ca..ff9f9a6913a 100644
--- a/tests/qemu-iotests/211.out
+++ b/tests/qemu-iotests/211.out
@@ -17,7 +17,7 @@ file format: IMGFMT
 virtual size: 128 MiB (134217728 bytes)
 cluster_size: 1048576
 
-[{"data": false, "depth": 0, "length": 134217728, "present": true, "start": 0, "zero": true}]
+[{"compressed": false, "data": false, "depth": 0, "length": 134217728, "present": true, "start": 0, "zero": true}]
 === Successful image creation (explicit defaults) ===
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vdi", "size": 0}}}
@@ -35,7 +35,7 @@ file format: IMGFMT
 virtual size: 64 MiB (67108864 bytes)
 cluster_size: 1048576
 
-[{"data": false, "depth": 0, "length": 67108864, "present": true, "start": 0, "zero": true}]
+[{"compressed": false, "data": false, "depth": 0, "length": 67108864, "present": true, "start": 0, "zero": true}]
 === Successful image creation (with non-default options) ===
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-t.vdi", "size": 0}}}
@@ -53,7 +53,7 @@ file format: IMGFMT
 virtual size: 32 MiB (33554432 bytes)
 cluster_size: 1048576
 
-[{"data": true, "depth": 0, "length": 3072, "offset": 1024, "present": true, "start": 0, "zero": false}, {"data": true, "depth": 0, "length": 33551360, "offset": 4096, "present": true, "start": 3072, "zero": true}]
+[{"compressed": false, "data": true, "depth": 0, "length": 3072, "offset": 1024, "present": true, "start": 0, "zero": false}, {"compressed": false, "data": true, "depth": 0, "length": 33551360, "offset": 4096, "present": true, "start": 3072, "zero": true}]
 === Invalid BlockdevRef ===
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vdi", "file": "this doesn't exist", "size": 33554432}}}
-- 
2.34.1

[PATCH v2 4/4] block/vdi.c: Make SECTOR_SIZE constant 64-bits

[Peter Maydell]

Make the VDI SECTOR_SIZE define be a 64-bit constant; this matches
how we define BDRV_SECTOR_SIZE.  The benefit is that it means that we
don't need to carefully cast to 64-bits when doing operations like
"n_sectors * SECTOR_SIZE" to avoid doing a 32x32->32 multiply, which
might overflow, and which Coverity and other static analysers tend to
warn about.

The specific potential overflow Coverity is highlighting is the one
at the end of vdi_co_pwritev() where we write out n_sectors sectors
to the block map.  This is very unlikely to actually overflow, since
the block map has 4 bytes per block and the maximum number of blocks
in the image must fit into a 32-bit integer.  So this commit is not
fixing a real-world bug.

An inspection of all the places currently using SECTOR_SIZE in the
file shows none which care about the change in its type, except for
one call to error_setg() which needs the format string adjusting.

Resolves: Coverity CID 1508076
Suggested-by: Kevin Wolf <kwolf@redhat.com>
---
v1->v2: v1 just added the (uint64_t) cast for the specific
issue that Coverity was warning about; v2 is entirely different
and takes the approach Kevin suggested in review on v1.
---
 block/vdi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 149e15c8314..26f7638f1fc 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -87,7 +87,7 @@
 /* Command line option for static images. */
 #define BLOCK_OPT_STATIC "static"
 
-#define SECTOR_SIZE 512
+#define SECTOR_SIZE 512ULL
 #define DEFAULT_CLUSTER_SIZE 1048576
 /* Note: can't use 1 * MiB, because it's passed to stringify() */
 
@@ -442,7 +442,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     } else if (header.sector_size != SECTOR_SIZE) {
         error_setg(errp, "unsupported VDI image (sector size %" PRIu32
-                   " is not %u)", header.sector_size, SECTOR_SIZE);
+                   " is not %llu)", header.sector_size, SECTOR_SIZE);
         ret = -ENOTSUP;
         goto fail;
     } else if (header.block_size != DEFAULT_CLUSTER_SIZE) {
-- 
2.34.1

Re: [PATCH v2 0/4] block: Miscellaneous minor Coverity fixes

[Kevin Wolf]

Am 08.10.2024 um 18:47 hat Peter Maydell geschrieben:
> This patchset is the remaining stragglers from my
> first set of "minor Coverity fixes" patches posted a
> couple of months back:
> https://patchew.org/QEMU/20240731143617.3391947-1-peter.maydell@linaro.org/
> Of that series, patches 3, 4, 5 and 6 are upstream now.
> 
> In this v2 series:
>  * patch 1 (old patch 2) has had a long line wrapped;
>    already reviewed
>  * patch 2 (old patch 7) now has hex2decimal() return "UINT_MAX"
>    instead of "-1"; already reviewed
>  * patch 3 is new, and fixes an error in an iotests reference
>    output file that's been lurking around for a few years now
>  * patch 4 (replacement for old patch 1) takes Kevin Wolf's
>    suggestion to make vdi SECTOR_SIZE a 64-bit constant so we
>    don't have to carefully cast it every time we want to use
>    it in a multiplication
> 
> Patches 3 and 4 need review; patch 4 passes "./check -vdi"
> except that 297 (the python-linter) fails for unrelated reasons.

Thanks, applied to the block branch.

Kevin