Re: [Qemu-devel] [PATCH 1/8] qdict: fix unbounded stack for qdict_array_entries

[Kevin Wolf]

Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
> Cc: Kevin, because he added the array in question.
> 
> Peter Xu <address@hidden> writes:
> 
> > Suggested-by: Paolo Bonzini <address@hidden>
> > CC: Luiz Capitulino <address@hidden>
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  qobject/qdict.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/qobject/qdict.c b/qobject/qdict.c
> > index 9833bd0..eb602a7 100644
> > --- a/qobject/qdict.c
> > +++ b/qobject/qdict.c
> > @@ -704,17 +704,19 @@ int qdict_array_entries(QDict *src, const char 
> > *subqdict)
> >      for (i = 0; i < INT_MAX; i++) {
> >          QObject *subqobj;
> >          int subqdict_entries;
> > -        size_t slen = 32 + subqdict_len;
> > -        char indexstr[slen], prefix[slen];
> > +#define __SLEN_MAX (128)
> > +        char indexstr[__SLEN_MAX], prefix[__SLEN_MAX];
> >          size_t snprintf_ret;
> >  
> > -        snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i);
> > -        assert(snprintf_ret < slen);
> > +        assert(__SLEN_MAX >= 32 + subqdict_len);
> > +
> > +        snprintf_ret = snprintf(indexstr, __SLEN_MAX, "%s%u", subqdict, i);
> > +        assert(snprintf_ret < __SLEN_MAX);
> >  
> >          subqobj = qdict_get(src, indexstr);
> >  
> > -        snprintf_ret = snprintf(prefix, slen, "%s%u.", subqdict, i);
> > -        assert(snprintf_ret < slen);
> > +        snprintf_ret = snprintf(prefix, __SLEN_MAX, "%s%u.", subqdict, i);
> > +        assert(snprintf_ret < __SLEN_MAX);
> >  
> >          subqdict_entries = qdict_count_prefixed_entries(src, prefix);
> >          if (subqdict_entries < 0) {
> > @@ -745,6 +747,7 @@ int qdict_array_entries(QDict *src, const char 
> > *subqdict)
> >      }
> >  
> >      return i;
> > +#undef __SLEN_MAX
> >  }
> >  
> >  /**
> 
> Same arguments as for PATCH 2, except here an argument on the maximum
> length of subqdict would probably be easier.

Yes, these are constant string literals in all callers, including the
one non-test case in quorum.

Let's simply assert a reasonable maximum for subqdict_length. The
minimum we need to allow with the existing callers is 9, and I expect
we'll never get keys longer than 16 characters.

> Unrelated to your patch: I think we've pushed QDict use father than
> sensible.  Encoding multiple keys in a string so you can use a flat
> associative array as your catch-all data structure is appropriate in
> AWK, but in C?  Not so much...

We'll always to that, because it's the command line syntax. What you may
criticise is that we convert QAPI objects to the command line
representation instead of the other way around, but changing that (which
I think would be far from trivial, for relatively little use) wouldn't
get rid of this kind of key parsing, but just move it a bit closer to
the command line handling.

Kevin