[Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Stefan Weil]

Report from smatch:
json-parser.c:474 parse_object(62) error: potential null derefence 'dict'.
json-parser.c:553 parse_array(75) error: potential null derefence 'list'.

Label out can be called with list == NULL.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 json-parser.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/json-parser.c b/json-parser.c
index 457291b..c31c759 100644
--- a/json-parser.c
+++ b/json-parser.c
@@ -471,7 +471,9 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
 
 out:
     parser_context_restore(ctxt, saved_ctxt);
-    QDECREF(dict);
+    if (dict) {
+        QDECREF(dict);
+    }
     return NULL;
 }
 
@@ -550,7 +552,9 @@ static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
 
 out:
     parser_context_restore(ctxt, saved_ctxt);
-    QDECREF(list);
+    if (list) {
+        QDECREF(list);
+    }
     return NULL;
 }
 
-- 
1.7.10

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Luiz Capitulino]

On Sat,  1 Sep 2012 12:52:58 +0200
Stefan Weil <sw@weilnetz.de> wrote:

> Report from smatch:
> json-parser.c:474 parse_object(62) error: potential null derefence 'dict'.
> json-parser.c:553 parse_array(75) error: potential null derefence 'list'.
> 
> Label out can be called with list == NULL.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  json-parser.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/json-parser.c b/json-parser.c
> index 457291b..c31c759 100644
> --- a/json-parser.c
> +++ b/json-parser.c
> @@ -471,7 +471,9 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
>  
>  out:
>      parser_context_restore(ctxt, saved_ctxt);
> -    QDECREF(dict);
> +    if (dict) {
> +        QDECREF(dict);
> +    }

I prefer changing QDECREF() to a nop if obj is NULL.

>      return NULL;
>  }
>  
> @@ -550,7 +552,9 @@ static QObject *parse_array(JSONParserContext *ctxt, va_list *ap)
>  
>  out:
>      parser_context_restore(ctxt, saved_ctxt);
> -    QDECREF(list);
> +    if (list) {
> +        QDECREF(list);
> +    }
>      return NULL;
>  }
>  

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Stefan Weil]

Am 03.09.2012 18:41, schrieb Luiz Capitulino:
> On Sat,  1 Sep 2012 12:52:58 +0200
> Stefan Weil <sw@weilnetz.de> wrote:
>
>> Report from smatch:
>> json-parser.c:474 parse_object(62) error: potential null derefence 'dict'.
>> json-parser.c:553 parse_array(75) error: potential null derefence 'list'.
>>
>> Label out can be called with list == NULL.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>   json-parser.c |    8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/json-parser.c b/json-parser.c
>> index 457291b..c31c759 100644
>> --- a/json-parser.c
>> +++ b/json-parser.c
>> @@ -471,7 +471,9 @@ static QObject *parse_object(JSONParserContext *ctxt, va_list *ap)
>>   
>>   out:
>>       parser_context_restore(ctxt, saved_ctxt);
>> -    QDECREF(dict);
>> +    if (dict) {
>> +        QDECREF(dict);
>> +    }
>
> I prefer changing QDECREF() to a nop if obj is NULL.

That's fine for me, too. If everybody agrees, I'll send two new
patches: one to change QDECREF, one to remove the if statements
from other code locations which use the same pattern as
my original patch.

Cheers,

- sw

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Stefan Weil]

Am 03.09.2012 18:53, schrieb Stefan Weil:
> Am 03.09.2012 18:41, schrieb Luiz Capitulino:
>> On Sat,  1 Sep 2012 12:52:58 +0200
>> Stefan Weil <sw@weilnetz.de> wrote:
>>
>>> Report from smatch:
>>> json-parser.c:474 parse_object(62) error: potential null derefence 
>>> 'dict'.
>>> json-parser.c:553 parse_array(75) error: potential null derefence 
>>> 'list'.
>>>
>>> Label out can be called with list == NULL.
>>>
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>   json-parser.c |    8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/json-parser.c b/json-parser.c
>>> index 457291b..c31c759 100644
>>> --- a/json-parser.c
>>> +++ b/json-parser.c
>>> @@ -471,7 +471,9 @@ static QObject *parse_object(JSONParserContext 
>>> *ctxt, va_list *ap)
>>>     out:
>>>       parser_context_restore(ctxt, saved_ctxt);
>>> -    QDECREF(dict);
>>> +    if (dict) {
>>> +        QDECREF(dict);
>>> +    }
>>
>> I prefer changing QDECREF() to a nop if obj is NULL.
>
> That's fine for me, too. If everybody agrees, I'll send two new
> patches: one to change QDECREF, one to remove the if statements
> from other code locations which use the same pattern as
> my original patch.
>
> Cheers,
>
> - sw
>
>

What about modifying QOBJECT to return NULL if called with a NULL pointer?
That would be a more generic fix for the same problem.

In either case, the code will be a little larger and slower,
but that should not matter because it is not time critical.

Regards,

Stefan W.

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Luiz Capitulino]

On Mon, 03 Sep 2012 19:14:27 +0200
Stefan Weil <sw@weilnetz.de> wrote:

> Am 03.09.2012 18:53, schrieb Stefan Weil:
> > Am 03.09.2012 18:41, schrieb Luiz Capitulino:
> >> On Sat,  1 Sep 2012 12:52:58 +0200
> >> Stefan Weil <sw@weilnetz.de> wrote:
> >>
> >>> Report from smatch:
> >>> json-parser.c:474 parse_object(62) error: potential null derefence 
> >>> 'dict'.
> >>> json-parser.c:553 parse_array(75) error: potential null derefence 
> >>> 'list'.
> >>>
> >>> Label out can be called with list == NULL.
> >>>
> >>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> >>> ---
> >>>   json-parser.c |    8 ++++++--
> >>>   1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/json-parser.c b/json-parser.c
> >>> index 457291b..c31c759 100644
> >>> --- a/json-parser.c
> >>> +++ b/json-parser.c
> >>> @@ -471,7 +471,9 @@ static QObject *parse_object(JSONParserContext 
> >>> *ctxt, va_list *ap)
> >>>     out:
> >>>       parser_context_restore(ctxt, saved_ctxt);
> >>> -    QDECREF(dict);
> >>> +    if (dict) {
> >>> +        QDECREF(dict);
> >>> +    }
> >>
> >> I prefer changing QDECREF() to a nop if obj is NULL.
> >
> > That's fine for me, too. If everybody agrees, I'll send two new
> > patches: one to change QDECREF, one to remove the if statements
> > from other code locations which use the same pattern as
> > my original patch.
> >
> > Cheers,
> >
> > - sw
> >
> >
> 
> What about modifying QOBJECT to return NULL if called with a NULL pointer?
> That would be a more generic fix for the same problem.

I don't like this because it's not obvious, besides, at least in theory
we'd have to change QOBJECT() users to check its return value.

On the other hand, QDECREF() is expected to do nothing if its argument is
NULL.

[Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Stefan Weil]

Report from smatch:
json-parser.c:474 parse_object(62) error: potential null derefence 'dict'.
json-parser.c:553 parse_array(75) error: potential null derefence 'list'.

Label 'out' in json-parser.c can be called with list == NULL
which is passed to QDECREF.

Modify QDECREF to handle a NULL argument (inline function qobject_decref
already handles them, too).

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

I did not change QINCREF because there are currently no errors caused
by that rarely used macro.

This patch can be used instead of the previous patch which fixed
the problem directly in json-parser.c
(see http://patchwork.ozlabs.org/patch/181129/).

Regards,
Stefan Weil

 qobject.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qobject.h b/qobject.h
index d42386d..9124649 100644
--- a/qobject.h
+++ b/qobject.h
@@ -71,7 +71,7 @@ typedef struct QObject {
 
 /* High-level interface for qobject_decref() */
 #define QDECREF(obj)              \
-    qobject_decref(QOBJECT(obj))
+    qobject_decref(obj ? QOBJECT(obj) : NULL)
 
 /* Initialize an object to default values */
 #define QOBJECT_INIT(obj, qtype_type)   \
-- 
1.7.10

Re: [Qemu-devel] [PATCH] json-parser: Fix potential NULL pointer segfault

[Luiz Capitulino]

On Mon,  3 Sep 2012 21:19:11 +0200
Stefan Weil <sw@weilnetz.de> wrote:

> Report from smatch:
> json-parser.c:474 parse_object(62) error: potential null derefence 'dict'.
> json-parser.c:553 parse_array(75) error: potential null derefence 'list'.
> 
> Label 'out' in json-parser.c can be called with list == NULL
> which is passed to QDECREF.
> 
> Modify QDECREF to handle a NULL argument (inline function qobject_decref
> already handles them, too).
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

Applied to qmp-next branch, thanks!

> ---
> 
> I did not change QINCREF because there are currently no errors caused
> by that rarely used macro.
> 
> This patch can be used instead of the previous patch which fixed
> the problem directly in json-parser.c
> (see http://patchwork.ozlabs.org/patch/181129/).
> 
> Regards,
> Stefan Weil
> 
>  qobject.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qobject.h b/qobject.h
> index d42386d..9124649 100644
> --- a/qobject.h
> +++ b/qobject.h
> @@ -71,7 +71,7 @@ typedef struct QObject {
>  
>  /* High-level interface for qobject_decref() */
>  #define QDECREF(obj)              \
> -    qobject_decref(QOBJECT(obj))
> +    qobject_decref(obj ? QOBJECT(obj) : NULL)
>  
>  /* Initialize an object to default values */
>  #define QOBJECT_INIT(obj, qtype_type)   \