| Message ID | 1539981546-10596-7-git-send-email-Liam.Merwick@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | off-by-one and NULL pointer accesses detected by static analysis | expand |
On 19.10.18 22:39, Liam Merwick wrote: > A NULL 'list' passed into function dump_qlist() isn't correctly > validated and can be passed to qlist_first() where it is dereferenced. > > Given that dump_qlist() is static, and callers already do the right > thing, just add an assert to catch future potential bugs (plus the > added benefit of suppressing a warning from a static analysis tool > and removing this noise will help us better find real issues). But can't you fix the tool? My opinion is still that large parts of our code do not assert that some parameter is not NULL, and I think it isn't a good idea to make them assert that. I don't know what makes this function special, and I wonder why it is special to your tool -- as I've said in the last version, dump_qdict() is basically the same in this regard. I wonder why your tool doesn't mind that. Can you not whitelist something as false positives? I know we have a lot of those in Coverity, and we just mark them as such, and that's it. Finally, one could argue that the nonnull GCC function attribute would be a better fit, actually. But overall, I just don't think it's a good idea to start changing the code to accommodate for false positives in static analyzers, because in my experience the number of false positives only rises with time. Max > Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > block/qapi.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block/qapi.c b/block/qapi.c > index c66f949db839..e81be604217c 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation, > const QListEntry *entry; > int i = 0; > > + assert(list); > + > for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) { > QType type = qobject_type(entry->value); > bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); >
On 05/11/18 00:07, Max Reitz wrote: > On 19.10.18 22:39, Liam Merwick wrote: >> A NULL 'list' passed into function dump_qlist() isn't correctly >> validated and can be passed to qlist_first() where it is dereferenced. >> >> Given that dump_qlist() is static, and callers already do the right >> thing, just add an assert to catch future potential bugs (plus the >> added benefit of suppressing a warning from a static analysis tool >> and removing this noise will help us better find real issues). > > But can't you fix the tool? I don't have access to the tool source but have been filing bugs against it as I run it on the QEMU codebase and discover false positives. > My opinion is still that large parts of our > code do not assert that some parameter is not NULL, and I think it isn't > a good idea to make them assert that. Yeah, that can be a slippery slope.... > I don't know what makes this > function special, and I wonder why it is special to your tool -- as I've > said in the last version, dump_qdict() is basically the same in this > regard. I wonder why your tool doesn't mind that. > I had gone though the code paths to try to see how the tool was happy with one and not the other - the implementation differed slightly w.r.t macro usage but I couldn't see any obvious reason. > Can you not whitelist something as false positives? I know we have a > lot of those in Coverity, and we just mark them as such, and that's it. Yeah, I can flag this as a FP and have it fall off my list. I'll will drop this patch in v5 Regards, Liam > > Finally, one could argue that the nonnull GCC function attribute would > be a better fit, actually. > > But overall, I just don't think it's a good idea to start changing the > code to accommodate for false positives in static analyzers, because in > my experience the number of false positives only rises with time. > > Max > >> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> block/qapi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/block/qapi.c b/block/qapi.c >> index c66f949db839..e81be604217c 100644 >> --- a/block/qapi.c >> +++ b/block/qapi.c >> @@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation, >> const QListEntry *entry; >> int i = 0; >> >> + assert(list); >> + >> for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) { >> QType type = qobject_type(entry->value); >> bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST); >> > >
diff --git a/block/qapi.c b/block/qapi.c index c66f949db839..e81be604217c 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation, const QListEntry *entry; int i = 0; + assert(list); + for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) { QType type = qobject_type(entry->value); bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);