| Message ID | 1535739372-24454-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 31.08.18 20:16, 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. > > 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(+) I don't disagree, but I don't see why the program just wouldn't crash if someone passed a NULL pointer. And I don't quite see why anyone would pass a NULL pointer. Of course it's reasonable to just add an assert() to reinforce the contract; but we have so many functions that just take a pointer that they assume to be non-NULL and then immediately dereference it. Nearly every blk_* function takes a BlockBackend that is always assumed to be non-NULL, for instance, and I don't really want to put assert()s into all of them. Or another example: dump_qobject() and dump_qdict() do exactly the same -- if we added an assertion in dump_qlist(), we would actually have to add the very same assertions there, too. So I don't really object this patch (because it's not wrong), but I don't think it's very useful. Max > 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 12/10/18 16:22, Max Reitz wrote: > On 31.08.18 20:16, 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. >> >> 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(+) > > I don't disagree, but I don't see why the program just wouldn't crash if > someone passed a NULL pointer. And I don't quite see why anyone would > pass a NULL pointer. > > Of course it's reasonable to just add an assert() to reinforce the > contract; but we have so many functions that just take a pointer that > they assume to be non-NULL and then immediately dereference it. Nearly > every blk_* function takes a BlockBackend that is always assumed to be > non-NULL, for instance, and I don't really want to put assert()s into > all of them. Or another example: dump_qobject() and dump_qdict() do > exactly the same -- if we added an assertion in dump_qlist(), we would > actually have to add the very same assertions there, too. > > So I don't really object this patch (because it's not wrong), but I > don't think it's very useful. I agree with all the above - however I kept the patch in the series given it was helping reduce the static analysis noise (hopefully making it easier to spot real issues) Regards, Liam > > Max > >> 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);