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

[Peter Xu]

On Wed, Mar 09, 2016 at 10:03:51PM +0100, Markus Armbruster wrote:
> Kevin Wolf  writes:
> > I think it's unnecessary, but fine with me. I'm just trying to say that
> > making it a fixed 128 byte array on the stack certainly doesn't improve
> > anything.
> 
> It trades a few bytes of stack for a fixed stack frame.  A fixed stack
> frame is a bit more efficient (not that it matters here), and lets us
> scratch the function permanently from the list of stack fram size false
> positives.  I think that's a reasonable trade.

Yes, that's what I want to do. I did fix nothing, but tried to avoid
the warning. Sorry that I made it a wrong title (also in the
following splitted patch). I should say:

"Fix unbounded stack warning for qdict_array_entries"

Rather than:

"Fix unbounded stack for qdict_array_entries"

Thanks.
Peter

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

[Markus Armbruster]

Kevin Wolf  writes:

> Am 09.03.2016 um 14:23 hat Markus Armbruster geschrieben:
>> Kevin Wolf  writes:
>> 
>> > Am 09.03.2016 um 04:04 hat Eric Blake geschrieben:
>> >> On 03/08/2016 07:57 PM, Peter Xu wrote:
>> >> > On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote:
>> >> >> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
>> >> >>> 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.
>> >> > 
>> >> > Hi, Kevin, Markus,
>> >> > 
>> >> > The patch should be trying to do as mentioned above. To make it
>> >> > clearer, how about the following one:
>> >> > 
>> >> > 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.
>> >> 
>> >> 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?
>> >
>> > I can only repeat myself, we're not dealing with user data here, but
>> > with constant literal strings. Put an assert(subqdict_len < 32); at the
>> > beginning of the function and be done with it. Any violation of it is
>> > not unexpected user input, but a caller bug.
>> 
>> The fact that the keys are literals is a non-local, not-quite-obvious
>> argument.
>> 
>> It's non-local, because in qdict.c we don't know what its users store in
>> their qdicts.
>> 
>> It's not quite obvious for the only user so far, quorum_open().  New
>> uses outside the block layer are possible, but seem unlikely; the block
>> layer is the most demanding user of QDicts.
>> 
>> The block layer's use of qdicts and QemuOpts is "interesting" enough for
>> me to call it not quite obvious.  In particular, QemuOpts can either
>> accept a fixed set of keys, or arbitrary keys.  In the latter case,
>> unknown keys should be rejected by the consumer of the QemuOpts.
>> Whether that happens before they can reach qdict_array_entries() is not
>> obvious to me.
>
> And it doesn't matter here because the variable part that determines the
> array size isn't the length of a key in the QDict, but the key name
> prefix we're looking for, i.e. the name of the QAPI array we want to
> parse. Unless you plan to introduce computed field names in QAPI, I
> can't imagine a reason why this could ever be something else than a
> constant string.

I think I see now.

>> We can rely on non-local or subtle arguments when it's worthwhile, but
>> I'm not sure it's worthwhile here.  I'd use g_strdup_printf() and call
>> it a day.
>
> I think it's unnecessary, but fine with me. I'm just trying to say that
> making it a fixed 128 byte array on the stack certainly doesn't improve
> anything.

It trades a few bytes of stack for a fixed stack frame.  A fixed stack
frame is a bit more efficient (not that it matters here), and lets us
scratch the function permanently from the list of stack fram size false
positives.  I think that's a reasonable trade.

[...]

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

[Kevin Wolf]

Am 09.03.2016 um 14:23 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 09.03.2016 um 04:04 hat Eric Blake geschrieben:
> >> On 03/08/2016 07:57 PM, Peter Xu wrote:
> >> > On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote:
> >> >> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
> >> >>> 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.
> >> > 
> >> > Hi, Kevin, Markus,
> >> > 
> >> > The patch should be trying to do as mentioned above. To make it
> >> > clearer, how about the following one:
> >> > 
> >> > 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.
> >> 
> >> 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?
> >
> > I can only repeat myself, we're not dealing with user data here, but
> > with constant literal strings. Put an assert(subqdict_len < 32); at the
> > beginning of the function and be done with it. Any violation of it is
> > not unexpected user input, but a caller bug.
> 
> The fact that the keys are literals is a non-local, not-quite-obvious
> argument.
> 
> It's non-local, because in qdict.c we don't know what its users store in
> their qdicts.
> 
> It's not quite obvious for the only user so far, quorum_open().  New
> uses outside the block layer are possible, but seem unlikely; the block
> layer is the most demanding user of QDicts.
> 
> The block layer's use of qdicts and QemuOpts is "interesting" enough for
> me to call it not quite obvious.  In particular, QemuOpts can either
> accept a fixed set of keys, or arbitrary keys.  In the latter case,
> unknown keys should be rejected by the consumer of the QemuOpts.
> Whether that happens before they can reach qdict_array_entries() is not
> obvious to me.

And it doesn't matter here because the variable part that determines the
array size isn't the length of a key in the QDict, but the key name
prefix we're looking for, i.e. the name of the QAPI array we want to
parse. Unless you plan to introduce computed field names in QAPI, I
can't imagine a reason why this could ever be something else than a
constant string.

> We can rely on non-local or subtle arguments when it's worthwhile, but
> I'm not sure it's worthwhile here.  I'd use g_strdup_printf() and call
> it a day.

I think it's unnecessary, but fine with me. I'm just trying to say that
making it a fixed 128 byte array on the stack certainly doesn't improve
anything.

> >> Two assertions on the snprintf_ret should make sure we are safe,
> >> right?
> >
> > No, asserting after the fact that you haven't just overflown a buffer is
> > generally not a valid way of error handling (especially if you consider
> > that compiling with NDEBUG would make the assert disappear).
> 
> snprintf() doesn't overflow the buffer, unless you pass a silly size.
> Instead, it truncates and returns the untruncated length.  That lets you
> assert it didn't truncate.  Perfectly fine way to assert the buffer
> suffices.  More direct than assertions on the length that imply the
> buffer will suffice.

Yes, that's what I tried to say below, but I messed up the function
name:

> > Of course, the strnlen() would already avoid this, so what the assertion
> > would really catch is string truncation.
> >
> > In summary, the behaviour after your patch would still be correct, but
> > it's pointless, it's less obvious what 

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

[Markus Armbruster]

Kevin Wolf  writes:

> Am 09.03.2016 um 04:04 hat Eric Blake geschrieben:
>> On 03/08/2016 07:57 PM, Peter Xu wrote:
>> > On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote:
>> >> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
>> >>> 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.
>> > 
>> > Hi, Kevin, Markus,
>> > 
>> > The patch should be trying to do as mentioned above. To make it
>> > clearer, how about the following one:
>> > 
>> > 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.
>> 
>> 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?
>
> I can only repeat myself, we're not dealing with user data here, but
> with constant literal strings. Put an assert(subqdict_len < 32); at the
> beginning of the function and be done with it. Any violation of it is
> not unexpected user input, but a caller bug.

The fact that the keys are literals is a non-local, not-quite-obvious
argument.

It's non-local, because in qdict.c we don't know what its users store in
their qdicts.

It's not quite obvious for the only user so far, quorum_open().  New
uses outside the block layer are possible, but seem unlikely; the block
layer is the most demanding user of QDicts.

The block layer's use of qdicts and QemuOpts is "interesting" enough for
me to call it not quite obvious.  In particular, QemuOpts can either
accept a fixed set of keys, or arbitrary keys.  In the latter case,
unknown keys should be rejected by the consumer of the QemuOpts.
Whether that happens before they can reach qdict_array_entries() is not
obvious to me.

We can rely on non-local or subtle arguments when it's worthwhile, but
I'm not sure it's worthwhile here.  I'd use g_strdup_printf() and call
it a day.

>> Two assertions on the snprintf_ret should make sure we are safe,
>> right?
>
> No, asserting after the fact that you haven't just overflown a buffer is
> generally not a valid way of error handling (especially if you consider
> that compiling with NDEBUG would make the assert disappear).

snprintf() doesn't overflow the buffer, unless you pass a silly size.
Instead, it truncates and returns the untruncated length.  That lets you
assert it didn't truncate.  Perfectly fine way to assert the buffer
suffices.  More direct than assertions on the length that imply the
buffer will suffice.

> Of course, the strnlen() would already avoid this, so what the assertion
> would really catch is string truncation.
>
> In summary, the behaviour after your patch would still be correct, but
> it's pointless, it's less obvious what the reason for the array size is
> and it wastes memory on the stack. So I wouldn't do that.

I doubt replacing the variable length array

char indexstr[32 + subqdict_len]

by

char indexstr[128]

and an assertion "indexstr[] suffices" would be a wast.  Sure, we'd use
128 bytes instead of 32 + subqdict_len of stack, and that's roughly 80
bytes more for typical keys, but we'd save the stack pointer fiddling.

Anyway, I'd rather engage in undeniable waste and allocate dynamically.

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

[Kevin Wolf]

Am 09.03.2016 um 04:04 hat Eric Blake geschrieben:
> On 03/08/2016 07:57 PM, Peter Xu wrote:
> > On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote:
> >> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
> >>> 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.
> > 
> > Hi, Kevin, Markus,
> > 
> > The patch should be trying to do as mentioned above. To make it
> > clearer, how about the following one:
> > 
> > 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.
> 
> 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?

I can only repeat myself, we're not dealing with user data here, but
with constant literal strings. Put an assert(subqdict_len < 32); at the
beginning of the function and be done with it. Any violation of it is
not unexpected user input, but a caller bug.

> Two assertions on the snprintf_ret should make sure we are safe,
> right?

No, asserting after the fact that you haven't just overflown a buffer is
generally not a valid way of error handling (especially if you consider
that compiling with NDEBUG would make the assert disappear).

Of course, the strnlen() would already avoid this, so what the assertion
would really catch is string truncation.

In summary, the behaviour after your patch would still be correct, but
it's pointless, it's less obvious what the reason for the array size is
and it wastes memory on the stack. So I wouldn't do that.

Kevin


pgpzM1y7903Jp.pgp
Description: PGP signature

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

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

[Eric Blake]

On 03/08/2016 07:57 PM, Peter Xu wrote:
> On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote:
>> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
>>> 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.
> 
> Hi, Kevin, Markus,
> 
> The patch should be trying to do as mentioned above. To make it
> clearer, how about the following one:
> 
> 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.

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?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

[Peter Xu]

On Tue, Mar 08, 2016 at 11:19:44AM +0100, Kevin Wolf wrote:
> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
> > 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.

Hi, Kevin, Markus,

The patch should be trying to do as mentioned above. To make it
clearer, how about the following one:

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));

 subqobj = qdict_get(src, indexstr);

-snprintf_ret = snprintf(prefix, slen, "%s%u.", subqdict, i);
-assert(snprintf_ret < slen);
+snprintf_ret = snprintf(prefix, ARRAY_SIZE(prefix), "%s%u.", subqdict, 
i);
+assert(snprintf_ret < ARRAY_SIZE(prefix));

 subqdict_entries = qdict_count_prefixed_entries(src, prefix);
 if (subqdict_entries < 0) {

Two assertions on the snprintf_ret should make sure we are safe,
right?

Thanks.
Peter

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

[Daniel P. Berrange]

On Tue, Mar 08, 2016 at 05:30:31PM +0100, Kevin Wolf wrote:
> Am 08.03.2016 um 17:21 hat Eric Blake geschrieben:
> > On 03/08/2016 03:19 AM, Kevin Wolf wrote:
> > > Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
> > >> Cc: Kevin, because he added the array in question.
> > >>
> > >> Peter Xu  writes:
> > >>
> > 
> > >> 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.
> > 
> > I actually WANT us to try that conversion (a great GSoC project, if
> > someone wants it) for 2.7.  We already did it for SocketAddress, and it
> > makes the code easier to maintain when you can just access foo->data
> > instead of doing qdict_lookup(foo, "data") all over the place.
> 
> I think you're underestimating the difference in difficulty between
> using SocketAddress everywhere and using BlockdevOptions everywhere,
> which is the reason why I never even put it on my todo list.
> 
> Of course, I would be fine with your trying anyway, but it would
> probably be fairer if we not let a poor student fail with this.

I think a more sensible/practical first step would be to change the block
layer to using the nested QDicts as its primary representation, instead
of the flat QDicts. In APIs which get a flat qdict (CLI QemuOpts & HMP),
it should really immediately crumple it into a nested QDict before usage.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[Eric Blake]

On 03/08/2016 03:19 AM, Kevin Wolf wrote:
> Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
>> Cc: Kevin, because he added the array in question.
>>
>> Peter Xu  writes:
>>

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

I actually WANT us to try that conversion (a great GSoC project, if
someone wants it) for 2.7.  We already did it for SocketAddress, and it
makes the code easier to maintain when you can just access foo->data
instead of doing qdict_lookup(foo, "data") all over the place.


-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

[Kevin Wolf]

Am 08.03.2016 um 17:21 hat Eric Blake geschrieben:
> On 03/08/2016 03:19 AM, Kevin Wolf wrote:
> > Am 08.03.2016 um 09:22 hat Markus Armbruster geschrieben:
> >> Cc: Kevin, because he added the array in question.
> >>
> >> Peter Xu  writes:
> >>
> 
> >> 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.
> 
> I actually WANT us to try that conversion (a great GSoC project, if
> someone wants it) for 2.7.  We already did it for SocketAddress, and it
> makes the code easier to maintain when you can just access foo->data
> instead of doing qdict_lookup(foo, "data") all over the place.

I think you're underestimating the difference in difficulty between
using SocketAddress everywhere and using BlockdevOptions everywhere,
which is the reason why I never even put it on my todo list.

Of course, I would be fine with your trying anyway, but it would
probably be fairer if we not let a poor student fail with this.

Kevin


pgppAgIQxzLIa.pgp
Description: PGP signature

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  writes:
> 
> > Suggested-by: Paolo Bonzini 
> > CC: Luiz Capitulino 
> > Signed-off-by: Peter Xu 
> > ---
> >  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

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

[Markus Armbruster]

Cc: Kevin, because he added the array in question.

Peter Xu  writes:

> Suggested-by: Paolo Bonzini 
> CC: Luiz Capitulino 
> Signed-off-by: Peter Xu 
> ---
>  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.

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