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);
> }
>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org