| Message ID | 20231110083654.277345-1-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-8.2] test-resv-mem: Fix CID 1523911 | expand |
On 10/11/2023 09.36, Eric Auger wrote: > Coverity complains about passing "&expected" to "run_range_inverse_array", > which dereferences null "expected". I guess the problem is that the > compare_ranges() loop dereferences 'e' without testing it. However the > loop condition is based on 'ranges' which is garanteed to have > the same length as 'expected' given the g_assert_cmpint() just > before the loop. So the code looks safe to me. > > Nevertheless adding a test on expected before the loop to get rid of the > warning. > > Fixes: CID 1523901 > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Reported-by: Coverity (CID 1523901) > > --- > > Hope this fixes the Coverity warning as I cannot test. > --- > tests/unit/test-resv-mem.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/tests/unit/test-resv-mem.c b/tests/unit/test-resv-mem.c > index 5963274e2c..cd8f7318cc 100644 > --- a/tests/unit/test-resv-mem.c > +++ b/tests/unit/test-resv-mem.c > @@ -44,6 +44,10 @@ static void compare_ranges(const char *prefix, GList *ranges, > print_ranges("out", ranges); > print_ranges("expected", expected); > #endif > + if (!expected) { > + g_assert_true(!ranges); > + return; > + } > g_assert_cmpint(g_list_length(ranges), ==, g_list_length(expected)); > for (l = ranges, e = expected; l ; l = l->next, e = e->next) { > Range *r = (Range *)l->data; Reviewed-by: Thomas Huth <thuth@redhat.com> I'll queue it (unless somebody else wants to take this?).
On 11/13/23 08:21, Thomas Huth wrote: > On 10/11/2023 09.36, Eric Auger wrote: >> Coverity complains about passing "&expected" to "run_range_inverse_array", >> which dereferences null "expected". I guess the problem is that the >> compare_ranges() loop dereferences 'e' without testing it. However the >> loop condition is based on 'ranges' which is garanteed to have >> the same length as 'expected' given the g_assert_cmpint() just >> before the loop. So the code looks safe to me. >> >> Nevertheless adding a test on expected before the loop to get rid of the >> warning. >> >> Fixes: CID 1523901 >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Reported-by: Coverity (CID 1523901) >> >> --- >> >> Hope this fixes the Coverity warning as I cannot test. >> --- >> tests/unit/test-resv-mem.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/tests/unit/test-resv-mem.c b/tests/unit/test-resv-mem.c >> index 5963274e2c..cd8f7318cc 100644 >> --- a/tests/unit/test-resv-mem.c >> +++ b/tests/unit/test-resv-mem.c >> @@ -44,6 +44,10 @@ static void compare_ranges(const char *prefix, GList *ranges, >> print_ranges("out", ranges); >> print_ranges("expected", expected); >> #endif >> + if (!expected) { >> + g_assert_true(!ranges); >> + return; >> + } >> g_assert_cmpint(g_list_length(ranges), ==, g_list_length(expected)); >> for (l = ranges, e = expected; l ; l = l->next, e = e->next) { >> Range *r = (Range *)l->data; > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > I'll queue it (unless somebody else wants to take this?). There is also another fix for the same series : https://lore.kernel.org/qemu-devel/20231109170715.259520-1-eric.auger@redhat.com/ I was waiting to have a little more for VFIO to queue both but they could go through your queue also. Thanks, C.
On 13/11/2023 08.56, Cédric Le Goater wrote: > On 11/13/23 08:21, Thomas Huth wrote: >> On 10/11/2023 09.36, Eric Auger wrote: >>> Coverity complains about passing "&expected" to "run_range_inverse_array", >>> which dereferences null "expected". I guess the problem is that the >>> compare_ranges() loop dereferences 'e' without testing it. However the >>> loop condition is based on 'ranges' which is garanteed to have >>> the same length as 'expected' given the g_assert_cmpint() just >>> before the loop. So the code looks safe to me. >>> >>> Nevertheless adding a test on expected before the loop to get rid of the >>> warning. >>> >>> Fixes: CID 1523901 >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> Reported-by: Coverity (CID 1523901) >>> >>> --- >>> >>> Hope this fixes the Coverity warning as I cannot test. >>> --- >>> tests/unit/test-resv-mem.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/tests/unit/test-resv-mem.c b/tests/unit/test-resv-mem.c >>> index 5963274e2c..cd8f7318cc 100644 >>> --- a/tests/unit/test-resv-mem.c >>> +++ b/tests/unit/test-resv-mem.c >>> @@ -44,6 +44,10 @@ static void compare_ranges(const char *prefix, GList >>> *ranges, >>> print_ranges("out", ranges); >>> print_ranges("expected", expected); >>> #endif >>> + if (!expected) { >>> + g_assert_true(!ranges); >>> + return; >>> + } >>> g_assert_cmpint(g_list_length(ranges), ==, g_list_length(expected)); >>> for (l = ranges, e = expected; l ; l = l->next, e = e->next) { >>> Range *r = (Range *)l->data; >> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> >> I'll queue it (unless somebody else wants to take this?). > > There is also another fix for the same series : > > > https://lore.kernel.org/qemu-devel/20231109170715.259520-1-eric.auger@redhat.com/ > > I was waiting to have a little more for VFIO to queue both but > they could go through your queue also. I'm just looking at patches that affect tests/ ... the other fix is not related to that, so I won't pick up that one. Thomas
diff --git a/tests/unit/test-resv-mem.c b/tests/unit/test-resv-mem.c index 5963274e2c..cd8f7318cc 100644 --- a/tests/unit/test-resv-mem.c +++ b/tests/unit/test-resv-mem.c @@ -44,6 +44,10 @@ static void compare_ranges(const char *prefix, GList *ranges, print_ranges("out", ranges); print_ranges("expected", expected); #endif + if (!expected) { + g_assert_true(!ranges); + return; + } g_assert_cmpint(g_list_length(ranges), ==, g_list_length(expected)); for (l = ranges, e = expected; l ; l = l->next, e = e->next) { Range *r = (Range *)l->data;
Coverity complains about passing "&expected" to "run_range_inverse_array", which dereferences null "expected". I guess the problem is that the compare_ranges() loop dereferences 'e' without testing it. However the loop condition is based on 'ranges' which is garanteed to have the same length as 'expected' given the g_assert_cmpint() just before the loop. So the code looks safe to me. Nevertheless adding a test on expected before the loop to get rid of the warning. Fixes: CID 1523901 Signed-off-by: Eric Auger <eric.auger@redhat.com> Reported-by: Coverity (CID 1523901) --- Hope this fixes the Coverity warning as I cannot test. --- tests/unit/test-resv-mem.c | 4 ++++ 1 file changed, 4 insertions(+)