Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging

[Anthony Liguori]

On 02/05/2010 06:12 AM, Luiz Capitulino wrote:
> On Thu, 04 Feb 2010 16:31:46 -0600
> Anthony Liguori<address@hidden>  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<address@hidden>
> > > ---
> > >    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