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

[Fam Zheng]

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?

> 
>            char key[256];
>            int i;
>    
>            assert(strlen(entry->key) + 1 <= ARRAY_SIZE(key));
> 

<...>

Fam