| Message ID | 1535644031-848-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 08/30/2018 10:47 AM, 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. But dump_qlist() is static, and it is easy to prove that it will never be called with a NULL 'list' parameter (it's lone caller did switch (qobject_type(obj)), and calls dump_qlist() only for QTYPE_QLIST, which implies that the qobject_to(QList, obj) will succeed and never be NULL). > > This could be resolved by checking if the list is NULL in dump_qlist() > and returning immediately. However, the general case can be handled by > adding a NULL arg check to to qlist_first() and qlist_next() and all > the callers to those functions handle that cleanly. NACK. If anything, I'd be happier with: assert(list); in dump_qlist() to shut up the lint checker, as we do not want to slow down the common case of qlist_first() for something that does not happen. That is, the null dereference in qlist_first() is a feature for detecting buggy code, and not something we need to change. > > Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> > Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com> > Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com> > > --- > include/qapi/qmp/qlist.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h > index 8d2c32ca2863..1ec716e2eb9e 100644 > --- a/include/qapi/qmp/qlist.h > +++ b/include/qapi/qmp/qlist.h > @@ -58,11 +58,17 @@ void qlist_destroy_obj(QObject *obj); > > static inline const QListEntry *qlist_first(const QList *qlist) > { > + if (!qlist) { > + return NULL; > + } > return QTAILQ_FIRST(&qlist->head); > } > > static inline const QListEntry *qlist_next(const QListEntry *entry) > { > + if (!entry) { > + return NULL; > + } > return QTAILQ_NEXT(entry, next); > } > >
On 30/08/18 19:41, Eric Blake wrote: > On 08/30/2018 10:47 AM, 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. > > But dump_qlist() is static, and it is easy to prove that it will never > be called with a NULL 'list' parameter (it's lone caller did switch > (qobject_type(obj)), and calls dump_qlist() only for QTYPE_QLIST, which > implies that the qobject_to(QList, obj) will succeed and never be NULL). > >> >> This could be resolved by checking if the list is NULL in dump_qlist() >> and returning immediately. However, the general case can be handled by >> adding a NULL arg check to to qlist_first() and qlist_next() and all >> the callers to those functions handle that cleanly. > > NACK. If anything, I'd be happier with: > > assert(list); > Thank works for me too. Regards, Liam > in dump_qlist() to shut up the lint checker, as we do not want to slow > down the common case of qlist_first() for something that does not > happen. That is, the null dereference in qlist_first() is a feature for > detecting buggy code, and not something we need to change. > >> >> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> >> Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com> >> Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com> >> >> --- >> include/qapi/qmp/qlist.h | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h >> index 8d2c32ca2863..1ec716e2eb9e 100644 >> --- a/include/qapi/qmp/qlist.h >> +++ b/include/qapi/qmp/qlist.h >> @@ -58,11 +58,17 @@ void qlist_destroy_obj(QObject *obj); >> static inline const QListEntry *qlist_first(const QList *qlist) >> { >> + if (!qlist) { >> + return NULL; >> + } >> return QTAILQ_FIRST(&qlist->head); >> } >> static inline const QListEntry *qlist_next(const QListEntry *entry) >> { >> + if (!entry) { >> + return NULL; >> + } >> return QTAILQ_NEXT(entry, next); >> } >> >
diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h index 8d2c32ca2863..1ec716e2eb9e 100644 --- a/include/qapi/qmp/qlist.h +++ b/include/qapi/qmp/qlist.h @@ -58,11 +58,17 @@ void qlist_destroy_obj(QObject *obj); static inline const QListEntry *qlist_first(const QList *qlist) { + if (!qlist) { + return NULL; + } return QTAILQ_FIRST(&qlist->head); } static inline const QListEntry *qlist_next(const QListEntry *entry) { + if (!entry) { + return NULL; + } return QTAILQ_NEXT(entry, next); }