| Message ID | 20240731143617.3391947-8-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | block: Miscellaneous minor Coverity fixes | expand |
Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben: > 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. If it's a Coverity issue, I think you want to mention the CID, too. > Avoid the double-check of hex digit validity by testing the return > values from hex2decimal() rather than doing separate calls to > qemu_isxdigit(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Could alternatively have put a g_assert_non_reached() in > hex2decimal(), but this seemed better to me. I don't like that hex2decimal() returns -1 when its result is unsigned, which is why you had to write the check as > 0xf rather than < 0. So in this sense, g_assert_non_reached() would look better to me. But we could also just change it to return UINT_MAX instead, which should be the same, just written in a less confusing way. > block/ssh.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/block/ssh.c b/block/ssh.c > index 27d582e0e3d..510dd208aba 100644 > --- a/block/ssh.c > +++ b/block/ssh.c > @@ -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++; Reviewed-by: Kevin Wolf <kwolf@redhat.com>
On Wed, 31 Jul 2024 at 16:21, Kevin Wolf <kwolf@redhat.com> wrote: > > Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben: > > 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. > > If it's a Coverity issue, I think you want to mention the CID, too. Yes; Resolves: Coverity CID 1547813 > > Avoid the double-check of hex digit validity by testing the return > > values from hex2decimal() rather than doing separate calls to > > qemu_isxdigit(). > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > Could alternatively have put a g_assert_non_reached() in > > hex2decimal(), but this seemed better to me. > > I don't like that hex2decimal() returns -1 when its result is unsigned, > which is why you had to write the check as > 0xf rather than < 0. So in > this sense, g_assert_non_reached() would look better to me. But we could > also just change it to return UINT_MAX instead, which should be the > same, just written in a less confusing way. I was not super happy with the -1 either. Happy to change that to 'return UINT_MAX'. -- PMM
diff --git a/block/ssh.c b/block/ssh.c index 27d582e0e3d..510dd208aba 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -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++;
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(). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Could alternatively have put a g_assert_non_reached() in hex2decimal(), but this seemed better to me. --- block/ssh.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)