Anthony Liguori <anth...@codemonkey.ws> writes: > On 02/05/2010 06:12 AM, Luiz Capitulino wrote: >> On Thu, 04 Feb 2010 16:31:46 -0600 >> Anthony Liguori<anth...@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<lcapitul...@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.
