json-parser: Fix potential NULL pointer segfault

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(-)

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;
>  }
>

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

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.

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.