[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging |
Date: |
Fri, 05 Feb 2010 18:14:41 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Anthony Liguori <address@hidden> writes:
> 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.
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.
- [Qemu-devel] [PATCH v0 0/4]: QMP related fixes, Luiz Capitulino, 2010/02/04
- [Qemu-devel] [PATCH 1/4] qjson: Improve debugging, Luiz Capitulino, 2010/02/04
- Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging, Anthony Liguori, 2010/02/04
- Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging, Luiz Capitulino, 2010/02/05
- Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging, Anthony Liguori, 2010/02/05
- Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging, Luiz Capitulino, 2010/02/08
- Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging, Anthony Liguori, 2010/02/08
- Re: [Qemu-devel] [PATCH 1/4] qjson: Improve debugging, Luiz Capitulino, 2010/02/08
[Qemu-devel] [PATCH 4/4] QMP: Don't leak on connection close, Luiz Capitulino, 2010/02/04
[Qemu-devel] [PATCH 2/4] Monitor: remove unneeded checks, Luiz Capitulino, 2010/02/04
[Qemu-devel] [PATCH 3/4] QError: Don't abort on multiple faults, Luiz Capitulino, 2010/02/04