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

[Markus Armbruster <armbru@redhat.com>]

Peter Xu <peterx@redhat.com> writes:

> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: qemu-block@nongnu.org
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  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:

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

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