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

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> On Tue, Mar 08, 2016 at 09:12:45AM +0100, Markus Armbruster wrote:
>> Peter Xu <address@hidden> writes:
>> >          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', ' ');;
>
> Yes, I can do that too. Do you think I should split the patchset
> into several smaller ones possibly? So that I can use a 2/2 for this
> specific function, one for the printf() issue, one for the stack
> allocation issue.

Since the format string cleanup and the unbounded stack issue aren't
related, separate patches make sense.

>> 
>> > -        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.
>
> Yes. Actually I doubted whether I should do this change... since I
> am not experienced enough to estimate a value which will be 100%
> working all the time. Here I just assumed 256 is big enough for
> qdict keys, which indeed is lack of proof.
>
>> 
>> If this was a good idea, I'd recommend to avoid the awkward macro:
>> 
>>            char key[256];
>>            int i;
>>    
>>            assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key));
>
> Yes, possibly! I forgot ARRAY_SIZE() macro. Thanks to point out.
>
>> 
>> There are several other ways to limit the stack usage:
>> 
>> 1. Move the array from stack to heap.  Fine unless it's on a hot path.
>>    As far as I can tell, this dumping business is for HMP and qemu-io,
>>    i.e. not hot.
>> 
>> 2. Use a stack array for short strings, switch to the heap for large
>>    strings.  More complex, but may be worth it on a hot path where most
>>    strings are short.
>> 
>> 3. Instead of creating a new string with '-' replaced for printing,
>>    print characters.  Can be okay with buffered I/O, but obviously not
>>    on a hot path.
>> 
>> 4. Like 3., but print substrings not containing '-'.
>
> Thanks for all the suggestions and ideas.
>
> To avoid the limitation of 256 (and also consider this path is not
> hot path), I'd like to choose (1) if you would not mind, in a split
> patch maybe.

Sounds good to me.