Patchwork [1/4] qjson: Improve debugging

login
register
mail settings
Submitter Luiz Capitulino
Date Feb. 4, 2010, 8:13 p.m.
Message ID <1265314396-6583-2-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/44549/
State New
Headers show

Comments

Luiz Capitulino - Feb. 4, 2010, 8:13 p.m.
Add an assert() to qobject_from_jsonf() to assure that the returned
QObject is not NULL. Currently this is duplicated in the callers.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qjson.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Anthony Liguori - Feb. 4, 2010, 10:31 p.m.
On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
> Add an assert() to qobject_from_jsonf() to assure that the returned
> QObject is not NULL. Currently this is duplicated in the callers.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
>   qjson.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/qjson.c b/qjson.c
> index 9ad8a91..0922c06 100644
> --- a/qjson.c
> +++ b/qjson.c
> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>       obj = qobject_from_jsonv(string,&ap);
>       va_end(ap);
>
> +    assert(obj != NULL);
>    

This is wrong.  We may get JSON from an untrusted source.  Callers need 
to deal with failure appropriately.

It just so happens that we only parse JSON from an untrusted source via 
qobject_from_json(), but the trust relationship is not obvious given the 
two functions in their current form.

Regards,

Anthony Liguori

>       return obj;
>   }
>
>
Markus Armbruster - Feb. 5, 2010, 9:13 a.m.
Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
>> Add an assert() to qobject_from_jsonf() to assure that the returned
>> QObject is not NULL. Currently this is duplicated in the callers.
>>
>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>> ---
>>   qjson.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/qjson.c b/qjson.c
>> index 9ad8a91..0922c06 100644
>> --- a/qjson.c
>> +++ b/qjson.c
>> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>>       obj = qobject_from_jsonv(string,&ap);
>>       va_end(ap);
>>
>> +    assert(obj != NULL);
>>    
>
> This is wrong.  We may get JSON from an untrusted source.  Callers
> need to deal with failure appropriately.
>
> It just so happens that we only parse JSON from an untrusted source
> via qobject_from_json(), but the trust relationship is not obvious
> given the two functions in their current form.

We have many uses of qobject_from_jsonf() with a literal argument, and
more to come.  Making them all deal with failure would be tedious and
clutter the code.  What about a wrapper function that cannot fail?
Luiz Capitulino - Feb. 5, 2010, 12:12 p.m.
On Thu, 04 Feb 2010 16:31:46 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
> > Add an assert() to qobject_from_jsonf() to assure that the returned
> > QObject is not NULL. Currently this is duplicated in the callers.
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > ---
> >   qjson.c |    1 +
> >   1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/qjson.c b/qjson.c
> > index 9ad8a91..0922c06 100644
> > --- a/qjson.c
> > +++ b/qjson.c
> > @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
> >       obj = qobject_from_jsonv(string,&ap);
> >       va_end(ap);
> >
> > +    assert(obj != NULL);
> >    
> 
> This is wrong.  We may get JSON from an untrusted source.  Callers need 
> to deal with failure appropriately.

 What kind of untrusted source? This function is only used by handlers
and assuming that the only possible error here is bad syntax, not having
this check in the source will only duplicate it in the users.

> It just so happens that we only parse JSON from an untrusted source via 
> qobject_from_json(), but the trust relationship is not obvious given the 
> two functions in their current form.

 Not exactly, qobject_from_json() is not even being currently used.

 We parse JSON data from clients by using the low-level parser API,
that's by calling json_message_parser_feed() to read the input and
then calling json_parser_parse() when we have collected enough data.

 qobject_from_jsonf() is only used internally, by handlers.

 Both, qobject_from_jsonf() and qobject_from_json() are _wrappers_ to
qobject_from_jsonv(), which uses the low-level API directly.

 So, having the assert() in qobject_from_jsonf() should only
affect handlers, which seems fine to me.
Luiz Capitulino - Feb. 5, 2010, 12:13 p.m.
On Fri, 05 Feb 2010 10:13:33 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:
> 
> > On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
> >> Add an assert() to qobject_from_jsonf() to assure that the returned
> >> QObject is not NULL. Currently this is duplicated in the callers.
> >>
> >> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >> ---
> >>   qjson.c |    1 +
> >>   1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/qjson.c b/qjson.c
> >> index 9ad8a91..0922c06 100644
> >> --- a/qjson.c
> >> +++ b/qjson.c
> >> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
> >>       obj = qobject_from_jsonv(string,&ap);
> >>       va_end(ap);
> >>
> >> +    assert(obj != NULL);
> >>    
> >
> > This is wrong.  We may get JSON from an untrusted source.  Callers
> > need to deal with failure appropriately.
> >
> > It just so happens that we only parse JSON from an untrusted source
> > via qobject_from_json(), but the trust relationship is not obvious
> > given the two functions in their current form.
> 
> We have many uses of qobject_from_jsonf() with a literal argument, and
> more to come.  Making them all deal with failure would be tedious and
> clutter the code.  What about a wrapper function that cannot fail?

 As far as I can understand, qobject_from_jsonf() is supposed to
be that wrapper already.
Anthony Liguori - Feb. 5, 2010, 3:54 p.m.
On 02/05/2010 06:12 AM, Luiz Capitulino wrote:
> On Thu, 04 Feb 2010 16:31:46 -0600
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
>>      
>>> Add an assert() to qobject_from_jsonf() to assure that the returned
>>> QObject is not NULL. Currently this is duplicated in the callers.
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>> ---
>>>    qjson.c |    1 +
>>>    1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/qjson.c b/qjson.c
>>> index 9ad8a91..0922c06 100644
>>> --- a/qjson.c
>>> +++ b/qjson.c
>>> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>>>        obj = qobject_from_jsonv(string,&ap);
>>>        va_end(ap);
>>>
>>> +    assert(obj != NULL);
>>>
>>>        
>> This is wrong.  We may get JSON from an untrusted source.  Callers need
>> to deal with failure appropriately.
>>      
>   What kind of untrusted source? This function is only used by handlers
> and assuming that the only possible error here is bad syntax, not having
> this check in the source will only duplicate it in the users.
>    

I don't know yet, but there's nothing about this function that indicates 
that it cannot handle malformed JSON.  I don't think it's a reasonable 
expectation either.

There are absolutely ways to mitigate this.  You can use GCC macros to 
enforce at compile time that the string argument is always a literal and 
never a user supplied string.

Run time asserts are a terrible way to deal with reasonably expected errors.

Regards,

Anthony Liguori
Markus Armbruster - Feb. 5, 2010, 5:14 p.m.
Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/05/2010 06:12 AM, Luiz Capitulino wrote:
>> On Thu, 04 Feb 2010 16:31:46 -0600
>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>
>>    
>>> On 02/04/2010 02:13 PM, Luiz Capitulino wrote:
>>>      
>>>> Add an assert() to qobject_from_jsonf() to assure that the returned
>>>> QObject is not NULL. Currently this is duplicated in the callers.
>>>>
>>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>>> ---
>>>>    qjson.c |    1 +
>>>>    1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/qjson.c b/qjson.c
>>>> index 9ad8a91..0922c06 100644
>>>> --- a/qjson.c
>>>> +++ b/qjson.c
>>>> @@ -62,6 +62,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>>>>        obj = qobject_from_jsonv(string,&ap);
>>>>        va_end(ap);
>>>>
>>>> +    assert(obj != NULL);
>>>>
>>>>        
>>> This is wrong.  We may get JSON from an untrusted source.  Callers need
>>> to deal with failure appropriately.
>>>      
>>   What kind of untrusted source? This function is only used by handlers
>> and assuming that the only possible error here is bad syntax, not having
>> this check in the source will only duplicate it in the users.
>>    
>
> I don't know yet, but there's nothing about this function that
> indicates that it cannot handle malformed JSON.  I don't think it's a
> reasonable expectation either.
>
> There are absolutely ways to mitigate this.  You can use GCC macros to
> enforce at compile time that the string argument is always a literal
> and never a user supplied string.

A string literal always comes from the programmer, not the user, but the
converse is not true.  Therefore, I don't see why we should make the
function unusable with non-literal arguments.  But if you really want
-Wformat-nonliteral, you know where to find it :)

> Run time asserts are a terrible way to deal with reasonably expected errors.

Yes.  But what's reasonably expected entirely depends on the contract
between the function and its callers.

I think we need a function that cannot fail and shouldn't used with
untrusted arguments (for what it's worth, that's how we use
qobject_from_jsonf() now).  Having related functions with different
contracts is fine with me.
Luiz Capitulino - Feb. 8, 2010, 11:22 a.m.
On Fri, 05 Feb 2010 18:14:41 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:

[...]

> Yes.  But what's reasonably expected entirely depends on the contract
> between the function and its callers.
> 
> I think we need a function that cannot fail and shouldn't used with
> untrusted arguments (for what it's worth, that's how we use
> qobject_from_jsonf() now).  Having related functions with different
> contracts is fine with me.

 I completely agree.
Anthony Liguori - Feb. 8, 2010, 2:53 p.m.
On 02/05/2010 11:14 AM, Markus Armbruster wrote:
>> Run time asserts are a terrible way to deal with reasonably expected errors.
>>      
> Yes.  But what's reasonably expected entirely depends on the contract
> between the function and its callers.
>
> I think we need a function that cannot fail and shouldn't used with
> untrusted arguments (for what it's worth, that's how we use
> qobject_from_jsonf() now).  Having related functions with different
> contracts is fine with me.
>    

I think the key point is that if we're going to establish these 
contracts, it must be obvious.

A reasonable programmer is going to assume that if a function can return 
a NULL, it can possibly return an error.  If you want to deviate from 
those semantics, you either have to name the function appropriately or 
put a big comment above the declaration explaining the semantics.

Regards,

Anthony Liguori
Luiz Capitulino - Feb. 8, 2010, 3:05 p.m.
On Mon, 08 Feb 2010 08:53:26 -0600
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 02/05/2010 11:14 AM, Markus Armbruster wrote:
> >> Run time asserts are a terrible way to deal with reasonably expected errors.
> >>      
> > Yes.  But what's reasonably expected entirely depends on the contract
> > between the function and its callers.
> >
> > I think we need a function that cannot fail and shouldn't used with
> > untrusted arguments (for what it's worth, that's how we use
> > qobject_from_jsonf() now).  Having related functions with different
> > contracts is fine with me.
> >    
> 
> I think the key point is that if we're going to establish these 
> contracts, it must be obvious.
> 
> A reasonable programmer is going to assume that if a function can return 
> a NULL, it can possibly return an error.  If you want to deviate from 
> those semantics, you either have to name the function appropriately or 
> put a big comment above the declaration explaining the semantics.

 Given that qobject_from_jsonf() is already a good and long name, I
prefer to add the comment.

 I will do that and re-submit.

Patch

diff --git a/qjson.c b/qjson.c
index 9ad8a91..0922c06 100644
--- a/qjson.c
+++ b/qjson.c
@@ -62,6 +62,7 @@  QObject *qobject_from_jsonf(const char *string, ...)
     obj = qobject_from_jsonv(string, &ap);
     va_end(ap);
 
+    assert(obj != NULL);
     return obj;
 }