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

[Peter Xu]

On Tue, Mar 08, 2016 at 08:04:50PM -0700, Eric Blake wrote:
> On 03/08/2016 07:57 PM, Peter Xu wrote:
> > diff --git a/qobject/qdict.c b/qobject/qdict.c
> > index 9833bd0..dde99e0 100644
> > --- a/qobject/qdict.c
> > +++ b/qobject/qdict.c
> > @@ -704,17 +704,16 @@ 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];
> > +        char indexstr[128], prefix[128];
> >          size_t snprintf_ret;
> > 
> > -        snprintf_ret = snprintf(indexstr, slen, "%s%u", subqdict, i);
> > -        assert(snprintf_ret < slen);
> > +        snprintf_ret = snprintf(indexstr, ARRAY_SIZE(indexstr), "%s%u", 
> > subqdict, i);
> > +        assert(snprintf_ret < ARRAY_SIZE(indexstr));
> 
> sizeof(indexstr) works, and is a bit nicer than ARRAY_SIZE() when
> dealing with char.

Yes, will use it next time.

> 
> But I'm worried that this can trigger an abort() by someone hammering on
> the command line.  Just because we don't expect any QMP command to
> validate with a key name longer than 128 doesn't mean that we don't have
> to deal with a command line with a garbage key name that long.  What's
> wrong with just using g_strdup_printf() and heap-allocating the result,
> avoiding snprintf() and fixed lengths altogether?

Agreed.. And this should only be called once too when open the
quorum device AFAIK, and not critical path too, correct? If so, I'd
like to use g_strdup_printf() in next spin if np.

Thanks.
Peter