Re: [Qemu-devel] [PATCH 2/8] block: fix unbounded stack for dump_qdict

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> On Tue, 03/08 09:12, Markus Armbruster wrote:
>> Peter Xu <address@hidden> writes:
>> 
>> > Suggested-by: Paolo Bonzini <address@hidden>
>> > CC: Markus Armbruster <address@hidden>
>> > CC: Kevin Wolf <address@hidden>
>> > CC: address@hidden
>> > Signed-off-by: Peter Xu <address@hidden>
>> > ---
>> >  block/qapi.c | 5 ++++-
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/block/qapi.c b/block/qapi.c
>> > index db2d3fb..687e577 100644
>> > --- a/block/qapi.c
>> > +++ b/block/qapi.c
>> > @@ -638,9 +638,12 @@ static void dump_qdict(fprintf_function func_fprintf, 
>> > void *f, int indentation,
>> >          QType type = qobject_type(entry->value);
>> >          bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
>> >          const char *format = composite ? "%*s%s:\n" : "%*s%s: ";
>> 
>> Unrelated to your patch: ugh!
>> 
>> Printf formats should be literals whenever possible, to make it easy for
>> the compiler to warn you when you screw up.  It's trivially possible
>> here!  Instead of
>> 
>>            func_fprintf(f, format, indentation * 4, "", key);
>> 
>> do
>> 
>>            func_fprintf(f, "%*s%s:%c", indentation * 4, "", key,
>>                         composite ? '\n', ' ');;
>> 
>> > -        char key[strlen(entry->key) + 1];
>> > +#define __KEY_LEN (256)
>> > +        char key[__KEY_LEN];
>> >          int i;
>> >  
>> > +        assert(strlen(entry->key) + 1 <= __KEY_LEN);
>> > +#undef __KEY_LEN
>> >          /* replace dashes with spaces in key (variable) names */
>> >          for (i = 0; entry->key[i]; i++) {
>> >              key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
>> 
>> I'm afraid this isn't a good idea.  It relies on the non-local argument
>> that nobody will ever put a key longer than 255 into a qdict that gets
>> dumped.  That may even be the case, but you need to *prove* it, not just
>> assert it.  The weakest acceptable proof might be assertions in every
>> place that put keys into a dict that might get dumped.  I suspect that's
>> practical and maintainable only if there's a single place that does it.
>> 
>> If this was a good idea, I'd recommend to avoid the awkward macro:
>
> Also I think the double underscore identifiers are considered reserved in C,
> no?

Correct.  C99 7.1.3 Reserved identifiers: All identifiers that begin
with an underscore and either an uppercase letter or another underscore
are always reserved for any use.

[...]