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

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 08, 2016 at 09:12:45AM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> 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.

> 
> > -        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.

Thanks.
Peter