[Qemu-devel] [PATCH 0/8] Fix several unbounded stack usage

[Qemu-devel] [PATCH 0/8] Fix several unbounded stack usage

[Peter Xu]

Suggested by Paolo.

Ths patchset fixes several of the warnings generated by
"-Wstack-usage=100000".  There are about 20-30 unbound stack cases
during my build, and this patch is only fixing several of them,
those which are obvious and easy.  For the rest, most of them need
some knowledge on specific area (e.g., USB, net, block) to have a
better assessment on the limitiation values, and are not covered in
this patchset.

One thing to mention about patch 4: I still cannot figure out why
the function xhci_dma_write_u32s() cannot be inlined in short
time... However, the current fix can at least keep the code behavior
not changed while making it stack bounded.  Please let me know if
anyone knows.

Thanks.
Peter

CC: Markus Armbruster <armbru@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Richard Henderson <rth@twiddle.net>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Luiz Capitulino <lcapitulino@redhat.com>
CC: qemu-block@nongnu.org

Peter Xu (8):
  qdict: fix unbounded stack for qdict_array_entries
  block: fix unbounded stack for dump_qdict
  usb: fix unbounded stack for ohci_td_pkt
  usb: fix unbounded stack for xhci_dma_write_u32s
  usb: fix unbounded stack for inotify_watchfn
  usb: fix unbounded stack for usb_mtp_add_str
  migration: fix unbounded stack for source_return_path_thread
  hw/i386: fix unbounded stack for load_multiboot

 block/qapi.c          |  5 ++++-
 hw/i386/multiboot.c   | 10 +++++++++-
 hw/usb/dev-mtp.c      | 13 +++++++++----
 hw/usb/hcd-ohci.c     |  7 ++++---
 hw/usb/hcd-xhci.c     | 12 ++++++++----
 migration/migration.c |  7 ++++---
 qobject/qdict.c       | 15 +++++++++------
 7 files changed, 47 insertions(+), 22 deletions(-)

-- 
2.4.3

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

[Peter Xu]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
CC: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 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
 }
 
 /**
-- 
2.4.3

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 <peterx@redhat.com> writes:

> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  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...

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 <peterx@redhat.com> writes:
> 
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Luiz Capitulino <lcapitulino@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

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 <peterx@redhat.com> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]

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 <peterx@redhat.com> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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 <peterx@redhat.com> 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

[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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3031 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> 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 14:23 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> 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 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.

The waste is probably the weakest of the arguments, especially since
this isn't in a hot path. I just mentioned it for completeness.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.03.2016 um 14:23 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> 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

[Peter Xu]

On Wed, Mar 09, 2016 at 10:03:51PM +0100, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> 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

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

[Peter Xu]

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: ";
-        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];
-- 
2.4.3

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

[Markus Armbruster]

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

Re: [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 <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:

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

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Tue, 03/08 09:12, Markus Armbruster wrote:
>> 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:
>
> Also I think the double underscore identifiers are considered reserved in C,
> no?

Correct.  C99 7.1.3 Reserved identifiers: All identifiers that begin
with an underscore and either an uppercase letter or another underscore
are always reserved for any use.

[...]

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

[Peter Xu]

On Tue, Mar 08, 2016 at 02:47:31PM +0100, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> > Also I think the double underscore identifiers are considered reserved in C,
> > no?
> 
> Correct.  C99 7.1.3 Reserved identifiers: All identifiers that begin
> with an underscore and either an uppercase letter or another underscore
> are always reserved for any use.
> 
> [...]

Fam, Markus,

Thanks to point out. Will fix.

Peter

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

[Peter Xu]

On Tue, Mar 08, 2016 at 09:12:45AM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> >          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', ' ');;

Yes, I can do that too. Do you think I should split the patchset
into several smaller ones possibly? So that I can use a 2/2 for this
specific function, one for the printf() issue, one for the stack
allocation issue.

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

Yes. Actually I doubted whether I should do this change... since I
am not experienced enough to estimate a value which will be 100%
working all the time. Here I just assumed 256 is big enough for
qdict keys, which indeed is lack of proof.

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

Yes, possibly! I forgot ARRAY_SIZE() macro. Thanks to point out.

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

Thanks for all the suggestions and ideas.

To avoid the limitation of 256 (and also consider this path is not
hot path), I'd like to choose (1) if you would not mind, in a split
patch maybe.

Thanks.
Peter

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 08, 2016 at 09:12:45AM +0100, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> >          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', ' ');;
>
> Yes, I can do that too. Do you think I should split the patchset
> into several smaller ones possibly? So that I can use a 2/2 for this
> specific function, one for the printf() issue, one for the stack
> allocation issue.

Since the format string cleanup and the unbounded stack issue aren't
related, separate patches make sense.

>> 
>> > -        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.
>
> Yes. Actually I doubted whether I should do this change... since I
> am not experienced enough to estimate a value which will be 100%
> working all the time. Here I just assumed 256 is big enough for
> qdict keys, which indeed is lack of proof.
>
>> 
>> 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));
>
> Yes, possibly! I forgot ARRAY_SIZE() macro. Thanks to point out.
>
>> 
>> 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 '-'.
>
> Thanks for all the suggestions and ideas.
>
> To avoid the limitation of 256 (and also consider this path is not
> hot path), I'd like to choose (1) if you would not mind, in a split
> patch maybe.

Sounds good to me.

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

[Paolo Bonzini]

On 08/03/2016 09:12, Markus Armbruster wrote:
> 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.

I think this is the best.  You can just g_strdup, modify in place, print
and free.

Paolo

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

[Peter Xu]

On Tue, Mar 08, 2016 at 01:17:03PM +0100, Paolo Bonzini wrote:
> 
> 
> On 08/03/2016 09:12, Markus Armbruster wrote:
> > 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.
> 
> I think this is the best.  You can just g_strdup, modify in place, print
> and free.

g_strdup() will bring one more loop? One to copy the strings, one
for replacing "-" to " ". Though I will first need to replace
g_malloc0() with g_malloc(), which seems more suitable here. :)

Thanks!
Peter

[Qemu-devel] [PATCH 3/8] usb: fix unbounded stack for ohci_td_pkt

[Peter Xu]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/usb/hcd-ohci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 17ed461..c3cd4e2 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -936,11 +936,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
 #ifdef trace_event_get_state
 static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
 {
+#define __TEMP_WIDTH (16)
     bool print16 = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_SHORT);
     bool printall = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_FULL);
-    const int width = 16;
     int i;
-    char tmp[3 * width + 1];
+    char tmp[3 * __TEMP_WIDTH + 1];
     char *p = tmp;
 
     if (!printall && !print16) {
@@ -948,7 +948,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
     }
 
     for (i = 0; ; i++) {
-        if (i && (!(i % width) || (i == len))) {
+        if (i && (!(i % __TEMP_WIDTH) || (i == len))) {
             if (!printall) {
                 trace_usb_ohci_td_pkt_short(msg, tmp);
                 break;
@@ -963,6 +963,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
 
         p += sprintf(p, " %.2x", buf[i]);
     }
+#undef __TEMP_WIDTH
 }
 #else
 static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
-- 
2.4.3

Re: [Qemu-devel] [PATCH 3/8] usb: fix unbounded stack for ohci_td_pkt

[Paolo Bonzini]

On 08/03/2016 08:00, Peter Xu wrote:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/usb/hcd-ohci.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 17ed461..c3cd4e2 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -936,11 +936,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
>  #ifdef trace_event_get_state
>  static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
>  {
> +#define __TEMP_WIDTH (16)
>      bool print16 = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_SHORT);
>      bool printall = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_FULL);
> -    const int width = 16;
>      int i;
> -    char tmp[3 * width + 1];
> +    char tmp[3 * __TEMP_WIDTH + 1];
>      char *p = tmp;
>  
>      if (!printall && !print16) {
> @@ -948,7 +948,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
>      }
>  
>      for (i = 0; ; i++) {
> -        if (i && (!(i % width) || (i == len))) {
> +        if (i && (!(i % __TEMP_WIDTH) || (i == len))) {
>              if (!printall) {
>                  trace_usb_ohci_td_pkt_short(msg, tmp);
>                  break;
> @@ -963,6 +963,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
>  
>          p += sprintf(p, " %.2x", buf[i]);
>      }
> +#undef __TEMP_WIDTH
>  }
>  #else
>  static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> 

This is a compiler false positive.  You can change "i % width" to

   p - tmp == ARRAY_SIZE(tmp) - 1

if you want to avoid it, but I'd just ignore this one.

Paolo

Re: [Qemu-devel] [PATCH 3/8] usb: fix unbounded stack for ohci_td_pkt

[Peter Xu]

On Tue, Mar 08, 2016 at 01:20:45PM +0100, Paolo Bonzini wrote:
> 
> 
> On 08/03/2016 08:00, Peter Xu wrote:
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/usb/hcd-ohci.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> > index 17ed461..c3cd4e2 100644
> > --- a/hw/usb/hcd-ohci.c
> > +++ b/hw/usb/hcd-ohci.c
> > @@ -936,11 +936,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
> >  #ifdef trace_event_get_state
> >  static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> >  {
> > +#define __TEMP_WIDTH (16)
> >      bool print16 = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_SHORT);
> >      bool printall = !!trace_event_get_state(TRACE_USB_OHCI_TD_PKT_FULL);
> > -    const int width = 16;
> >      int i;
> > -    char tmp[3 * width + 1];
> > +    char tmp[3 * __TEMP_WIDTH + 1];
> >      char *p = tmp;
> >  
> >      if (!printall && !print16) {
> > @@ -948,7 +948,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> >      }
> >  
> >      for (i = 0; ; i++) {
> > -        if (i && (!(i % width) || (i == len))) {
> > +        if (i && (!(i % __TEMP_WIDTH) || (i == len))) {
> >              if (!printall) {
> >                  trace_usb_ohci_td_pkt_short(msg, tmp);
> >                  break;
> > @@ -963,6 +963,7 @@ static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> >  
> >          p += sprintf(p, " %.2x", buf[i]);
> >      }
> > +#undef __TEMP_WIDTH
> >  }
> >  #else
> >  static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
> > 
> 
> This is a compiler false positive.  You can change "i % width" to
> 
>    p - tmp == ARRAY_SIZE(tmp) - 1
> 
> if you want to avoid it, but I'd just ignore this one.

Then I'd like to drop this patch for now.

Btw, do you know why the compiler got this false positive? Since we
declared width as constant. Is it a... "bug" or a "feature"?

Thanks.
Peter

[Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Peter Xu]

First of all, this function cannot be inlined even with always_inline,
so removing inline.

After that, make its stack bounded.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/usb/hcd-xhci.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 44b6f8c..3dd5a02 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -694,18 +694,22 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr,
     }
 }
 
-static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
-                                       uint32_t *buf, size_t len)
+static void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
+                                uint32_t *buf, size_t len)
 {
     int i;
-    uint32_t tmp[len / sizeof(uint32_t)];
+    uint32_t n = len / sizeof(uint32_t);
+#define __BUF_SIZE (12)
+    uint32_t tmp[__BUF_SIZE];
 
+    assert(__BUF_SIZE >= n);
     assert((len % sizeof(uint32_t)) == 0);
 
-    for (i = 0; i < (len / sizeof(uint32_t)); i++) {
+    for (i = 0; i < n; i++) {
         tmp[i] = cpu_to_le32(buf[i]);
     }
     pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len);
+#undef __BUF_SIZE
 }
 
 static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort *uport)
-- 
2.4.3

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Peter Maydell]

On 8 March 2016 at 14:00, Peter Xu <peterx@redhat.com> wrote:
> First of all, this function cannot be inlined even with always_inline,
> so removing inline.

Please don't mix two different changes in one patch.

> After that, make its stack bounded.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/usb/hcd-xhci.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 44b6f8c..3dd5a02 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -694,18 +694,22 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr,
>      }
>  }
>
> -static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
> -                                       uint32_t *buf, size_t len)
> +static void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
> +                                uint32_t *buf, size_t len)
>  {
>      int i;
> -    uint32_t tmp[len / sizeof(uint32_t)];
> +    uint32_t n = len / sizeof(uint32_t);
> +#define __BUF_SIZE (12)
> +    uint32_t tmp[__BUF_SIZE];
>
> +    assert(__BUF_SIZE >= n);
>      assert((len % sizeof(uint32_t)) == 0);
>
> -    for (i = 0; i < (len / sizeof(uint32_t)); i++) {
> +    for (i = 0; i < n; i++) {
>          tmp[i] = cpu_to_le32(buf[i]);
>      }
>      pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len);
> +#undef __BUF_SIZE

All the patches in this series seem to be following the
same pattern of #defining an arbitrary fixed length for
the array. This does not at all seem to me to be an
improvement. We should be avoiding unbounded stack
allocations, but we need to do this by either changing
the code to work correctly without an unbounded allocation
or by using a heap allocation instead of a stack allocation.

In some cases, like this one, the original code isn't even
unbounded -- we always call this function with a length
parameter which is a small compile time constant, so the
stack usage is definitely bounded. So this change is making
the code uglier and less flexible for no benefit that I
can see.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Peter Xu]

On Tue, Mar 08, 2016 at 02:26:36PM +0700, Peter Maydell wrote:
> On 8 March 2016 at 14:00, Peter Xu <peterx@redhat.com> wrote:
> > First of all, this function cannot be inlined even with always_inline,
> > so removing inline.
> 
> Please don't mix two different changes in one patch.

Sorry. Will follow this.

> 
> > After that, make its stack bounded.
> >
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/usb/hcd-xhci.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > index 44b6f8c..3dd5a02 100644
> > --- a/hw/usb/hcd-xhci.c
> > +++ b/hw/usb/hcd-xhci.c
> > @@ -694,18 +694,22 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr,
> >      }
> >  }
> >
> > -static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
> > -                                       uint32_t *buf, size_t len)
> > +static void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
> > +                                uint32_t *buf, size_t len)
> >  {
> >      int i;
> > -    uint32_t tmp[len / sizeof(uint32_t)];
> > +    uint32_t n = len / sizeof(uint32_t);
> > +#define __BUF_SIZE (12)
> > +    uint32_t tmp[__BUF_SIZE];
> >
> > +    assert(__BUF_SIZE >= n);
> >      assert((len % sizeof(uint32_t)) == 0);
> >
> > -    for (i = 0; i < (len / sizeof(uint32_t)); i++) {
> > +    for (i = 0; i < n; i++) {
> >          tmp[i] = cpu_to_le32(buf[i]);
> >      }
> >      pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len);
> > +#undef __BUF_SIZE
> 
> All the patches in this series seem to be following the
> same pattern of #defining an arbitrary fixed length for
> the array. This does not at all seem to me to be an
> improvement. We should be avoiding unbounded stack
> allocations, but we need to do this by either changing
> the code to work correctly without an unbounded allocation
> or by using a heap allocation instead of a stack allocation.
> 
> In some cases, like this one, the original code isn't even
> unbounded -- we always call this function with a length
> parameter which is a small compile time constant, so the
> stack usage is definitely bounded. So this change is making
> the code uglier and less flexible for no benefit that I
> can see.

I was trying to avoid the "stack unlimited" warning. There will be a
warning generated before applying this patch. The patch solved
this. Though I'd say this patch might not be good enough. :(

Then, will drop this one too if I have no further better
idea.

Thanks for review.

Peter

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Paolo Bonzini]

On 08/03/2016 08:00, Peter Xu wrote:
> First of all, this function cannot be inlined even with always_inline,
> so removing inline.

Why?  always_inline fixes the error for me.

>      int i;
> -    uint32_t tmp[len / sizeof(uint32_t)];
> +    uint32_t n = len / sizeof(uint32_t);
> +#define __BUF_SIZE (12)
> +    uint32_t tmp[__BUF_SIZE];
>  
> +    assert(__BUF_SIZE >= n);

Instead of a #define, you can use ARRAY_SIZE(tmp).

Paolo

>      assert((len % sizeof(uint32_t)) == 0);
>  
> -    for (i = 0; i < (len / sizeof(uint32_t)); i++) {
> +    for (i = 0; i < n; i++) {
>          tmp[i] = cpu_to_le32(buf[i]);
>      }
>      pci_dma_write(PCI_DEVICE(xhci), addr, tmp, len);
> +#undef __BUF_SIZE
>  }
>  
>  static XHCIPort *xhci_lookup_port(XHCIState *xhci, struct USBPort *uport)
> 

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Peter Xu]

On Tue, Mar 08, 2016 at 01:21:52PM +0100, Paolo Bonzini wrote:
> 
> 
> On 08/03/2016 08:00, Peter Xu wrote:
> > First of all, this function cannot be inlined even with always_inline,
> > so removing inline.
> 
> Why?  always_inline fixes the error for me.

I tried this patch:

-----------------

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 44b6f8c..961fd78 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -694,7 +694,7 @@ static inline void xhci_dma_read_u32s(XHCIState *xhci, dma_addr_t addr,
     }
 }

-static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
+static QEMU_ARTIFICIAL void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
                                        uint32_t *buf, size_t len)
 {
     int i;

-----------------

What I got is:

/root/git/qemu/hw/usb/hcd-xhci.c:699:1: warning: ‘artificial’ attribute ignored [-Wattributes]
 {
 ^
/root/git/qemu/hw/usb/hcd-xhci.c:697:56: warning: always_inline function might not be inlinable [-Wattributes]
 static QEMU_ARTIFICIAL void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr,
                                                        ^

GCC version:

pxdev:bin# gcc -v
Using built-in specs.
COLLECT_GCC=/bin/gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.8.5/lto-wrapper
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-initfini-array --disable-libgcj --with-isl=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/isl-install --with-cloog=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/cloog-install --enable-gnu-indirect-function --with-tune=generic --with-arch_32=x86-64 --build=x86_64-redhat-linux
Thread model: posix
gcc version 4.8.5 20150623 (Red Hat 4.8.5-4) (GCC)

Do you know why "might not be inlinable"? Failed to figure it out
myself as mentioned in cover letter..

> 
> >      int i;
> > -    uint32_t tmp[len / sizeof(uint32_t)];
> > +    uint32_t n = len / sizeof(uint32_t);
> > +#define __BUF_SIZE (12)
> > +    uint32_t tmp[__BUF_SIZE];
> >  
> > +    assert(__BUF_SIZE >= n);
> 
> Instead of a #define, you can use ARRAY_SIZE(tmp).

Will do when needed. Thanks!

Peter

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Paolo Bonzini]

On 09/03/2016 06:08, Peter Xu wrote:
> pxdev:bin# gcc -v
> Using built-in specs.
> COLLECT_GCC=/bin/gcc
> COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.8.5/lto-wrapper
> Target: x86_64-redhat-linux
> Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-initfini-array --disable-libgcj --with-isl=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/isl-install --with-cloog=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/cloog-install --enable-gnu-indirect-function --with-tune=generic --with-arch_32=x86-64 --build=x86_64-redhat-linux
> Thread model: posix
> gcc version 4.8.5 20150623 (Red Hat 4.8.5-4) (GCC)
> 
> Do you know why "might not be inlinable"? Failed to figure it out
> myself as mentioned in cover letter..

It's just a difference in compiler versions.  But ARRAY_SIZE should be
enough to fix it.

Paolo

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Peter Xu]

On Wed, Mar 09, 2016 at 08:53:19AM +0100, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 06:08, Peter Xu wrote:
> > pxdev:bin# gcc -v
> > Using built-in specs.
> > COLLECT_GCC=/bin/gcc
> > COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.8.5/lto-wrapper
> > Target: x86_64-redhat-linux
> > Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-initfini-array --disable-libgcj --with-isl=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/isl-install --with-cloog=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/cloog-install --enable-gnu-indirect-function --with-tune=generic --with-arch_32=x86-64 --build=x86_64-redhat-linux
> > Thread model: posix
> > gcc version 4.8.5 20150623 (Red Hat 4.8.5-4) (GCC)
> > 
> > Do you know why "might not be inlinable"? Failed to figure it out
> > myself as mentioned in cover letter..
> 
> It's just a difference in compiler versions.  But ARRAY_SIZE should be
> enough to fix it.

It's dynamically allocated in stack, can we still use ARRAY_SIZE in
this case?

Maybe for this case, best to use both stack and heap? malloc only if
buffer big enough.

Peter

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Mar 09, 2016 at 08:53:19AM +0100, Paolo Bonzini wrote:
>> 
>> 
>> On 09/03/2016 06:08, Peter Xu wrote:
>> > pxdev:bin# gcc -v
>> > Using built-in specs.
>> > COLLECT_GCC=/bin/gcc
>> > COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.8.5/lto-wrapper
>> > Target: x86_64-redhat-linux
>> > Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-initfini-array --disable-libgcj --with-isl=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/isl-install --with-cloog=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/cloog-install --enable-gnu-indirect-function --with-tune=generic --with-arch_32=x86-64 --build=x86_64-redhat-linux
>> > Thread model: posix
>> > gcc version 4.8.5 20150623 (Red Hat 4.8.5-4) (GCC)
>> > 
>> > Do you know why "might not be inlinable"? Failed to figure it out
>> > myself as mentioned in cover letter..
>> 
>> It's just a difference in compiler versions.  But ARRAY_SIZE should be
>> enough to fix it.
>
> It's dynamically allocated in stack, can we still use ARRAY_SIZE in
> this case?

ARRAY_SIZE(x) is defined as (sizeof(x) / sizeof((x)[0])).  Works when x
is of array type (variable length array is fine).  Screws up when x is
of *pointer* type.

C99  6.5.3.4:

    The sizeof operator yields the size (in bytes) of its operand, which
    may be an expression or the parenthesized name of a type.  The size
    is determined from the type of the operand.  The result is an
    integer.  If the type of the operand is a variable length array
    type, the operand is evaluated; otherwise, the operand is not
    evaluated and the result is an integer constant.

[...]

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Peter Xu]

On Wed, Mar 09, 2016 at 09:34:50AM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> > It's dynamically allocated in stack, can we still use ARRAY_SIZE in
> > this case?
> 
> ARRAY_SIZE(x) is defined as (sizeof(x) / sizeof((x)[0])).  Works when x
> is of array type (variable length array is fine).  Screws up when x is
> of *pointer* type.
> 
> C99  6.5.3.4:
> 
>     The sizeof operator yields the size (in bytes) of its operand, which
>     may be an expression or the parenthesized name of a type.  The size
>     is determined from the type of the operand.  The result is an
>     integer.  If the type of the operand is a variable length array
>     type, the operand is evaluated; otherwise, the operand is not
>     evaluated and the result is an integer constant.

Good to know it. Thanks! :) 

However, ARRAY_SIZE() still cannot help solving the unbounded stack
issue, right?

Peter

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Mar 09, 2016 at 09:34:50AM +0100, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> > It's dynamically allocated in stack, can we still use ARRAY_SIZE in
>> > this case?
>> 
>> ARRAY_SIZE(x) is defined as (sizeof(x) / sizeof((x)[0])).  Works when x
>> is of array type (variable length array is fine).  Screws up when x is
>> of *pointer* type.
>> 
>> C99  6.5.3.4:
>> 
>>     The sizeof operator yields the size (in bytes) of its operand, which
>>     may be an expression or the parenthesized name of a type.  The size
>>     is determined from the type of the operand.  The result is an
>>     integer.  If the type of the operand is a variable length array
>>     type, the operand is evaluated; otherwise, the operand is not
>>     evaluated and the result is an integer constant.
>
> Good to know it. Thanks! :) 
>
> However, ARRAY_SIZE() still cannot help solving the unbounded stack
> issue, right?

Measuring the size of the array doesn't change the size of the array :)

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Paolo Bonzini]

On 09/03/2016 09:07, Peter Xu wrote:
>>> > > pxdev:bin# gcc -v
>>> > > Using built-in specs.
>>> > > COLLECT_GCC=/bin/gcc
>>> > > COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.8.5/lto-wrapper
>>> > > Target: x86_64-redhat-linux
>>> > > Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-initfini-array --disable-libgcj --with-isl=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/isl-install --with-cloog=/builddir/build/BUILD/gcc-4.8.5-20150702/obj-x86_64-redhat-linux/cloog-install --enable-gnu-indirect-function --with-tune=generic --with-arch_32=x86-64 --build=x86_64-redhat-linux
>>> > > Thread model: posix
>>> > > gcc version 4.8.5 20150623 (Red Hat 4.8.5-4) (GCC)
>>> > > 
>>> > > Do you know why "might not be inlinable"? Failed to figure it out
>>> > > myself as mentioned in cover letter..
>> > 
>> > It's just a difference in compiler versions.  But ARRAY_SIZE should be
>> > enough to fix it.
> It's dynamically allocated in stack, can we still use ARRAY_SIZE in
> this case?
> 
> Maybe for this case, best to use both stack and heap? malloc only if
> buffer big enough.

If you look at users, they only write about 20 bytes at most.  My
suggestion is to use your patch, and replace

    assert(__BUF_SIZE >= n);

with

    assert(n < ARRAY_SIZE(tmp));

Then you don't need the #define.

Paolo

Re: [Qemu-devel] [PATCH 4/8] usb: fix unbounded stack for xhci_dma_write_u32s

[Peter Xu]

On Wed, Mar 09, 2016 at 01:59:03PM +0100, Paolo Bonzini wrote:
> If you look at users, they only write about 20 bytes at most.  My
> suggestion is to use your patch, and replace
> 
>     assert(__BUF_SIZE >= n);
> 
> with
> 
>     assert(n < ARRAY_SIZE(tmp));
> 
> Then you don't need the #define.

Okay. Will fix and post another one.

Thanks.
Peter

[Qemu-devel] [PATCH 5/8] usb: fix unbounded stack for inotify_watchfn

[Peter Xu]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/usb/dev-mtp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 7391783..e6dae2f 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -432,13 +432,13 @@ static void inotify_watchfn(void *arg)
 {
     MTPState *s = arg;
     ssize_t bytes;
+#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1)
     /* From the man page: atleast one event can be read */
-    int len = sizeof(struct inotify_event) + NAME_MAX + 1;
     int pos;
-    char buf[len];
+    char buf[__BUF_LEN];
 
     for (;;) {
-        bytes = read(s->inotifyfd, buf, len);
+        bytes = read(s->inotifyfd, buf, __BUF_LEN);
         pos = 0;
 
         if (bytes <= 0) {
@@ -534,6 +534,7 @@ static void inotify_watchfn(void *arg)
             }
         }
     }
+#undef __BUF_LEN
 }
 
 static int usb_mtp_inotify_init(MTPState *s)
-- 
2.4.3

Re: [Qemu-devel] [PATCH 5/8] usb: fix unbounded stack for inotify_watchfn

[Peter Maydell]

On 8 March 2016 at 14:00, Peter Xu <peterx@redhat.com> wrote:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 7391783..e6dae2f 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -432,13 +432,13 @@ static void inotify_watchfn(void *arg)
>  {
>      MTPState *s = arg;
>      ssize_t bytes;
> +#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1)
>      /* From the man page: atleast one event can be read */
> -    int len = sizeof(struct inotify_event) + NAME_MAX + 1;
>      int pos;
> -    char buf[len];
> +    char buf[__BUF_LEN];

The commit message subject says this is fixing an unbounded
stack usage, but (a) this array wasn't unbounded in size
(b) the change doesn't change the size we allocate.
What are you trying to do here?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/8] usb: fix unbounded stack for inotify_watchfn

[Paolo Bonzini]

On 08/03/2016 08:20, Peter Maydell wrote:
>> > +#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1)
>> >      /* From the man page: atleast one event can be read */
>> > -    int len = sizeof(struct inotify_event) + NAME_MAX + 1;
>> >      int pos;
>> > -    char buf[len];
>> > +    char buf[__BUF_LEN];
> The commit message subject says this is fixing an unbounded
> stack usage, but (a) this array wasn't unbounded in size
> (b) the change doesn't change the size we allocate.
> What are you trying to do here?

I suspect it's just fixing a false positive in the compiler.

Paolo

Re: [Qemu-devel] [PATCH 5/8] usb: fix unbounded stack for inotify_watchfn

[Peter Xu]

On Tue, Mar 08, 2016 at 01:22:46PM +0100, Paolo Bonzini wrote:
> 
> 
> On 08/03/2016 08:20, Peter Maydell wrote:
> >> > +#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1)
> >> >      /* From the man page: atleast one event can be read */
> >> > -    int len = sizeof(struct inotify_event) + NAME_MAX + 1;
> >> >      int pos;
> >> > -    char buf[len];
> >> > +    char buf[__BUF_LEN];
> > The commit message subject says this is fixing an unbounded
> > stack usage, but (a) this array wasn't unbounded in size
> > (b) the change doesn't change the size we allocate.
> > What are you trying to do here?

Sorry. I should be more clear to say "it avoids one warning during
compilation" rather than saying "fix unbounded stack usage", while
it's not.

> 
> I suspect it's just fixing a false positive in the compiler.
> 
> Paolo

Yes. I will avoid touching these kinds of places in the code next
time I guess... only when necessary. Since this one is easy, I'd
like to send another standalone patch, using sizeof(). rather than
macros, to avoid the warning.

Thanks.
Peter

Re: [Qemu-devel] [PATCH 5/8] usb: fix unbounded stack for inotify_watchfn

[Paolo Bonzini]

On 08/03/2016 08:00, Peter Xu wrote:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 7391783..e6dae2f 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -432,13 +432,13 @@ static void inotify_watchfn(void *arg)
>  {
>      MTPState *s = arg;
>      ssize_t bytes;
> +#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1)
>      /* From the man page: atleast one event can be read */
> -    int len = sizeof(struct inotify_event) + NAME_MAX + 1;
>      int pos;
> -    char buf[len];
> +    char buf[__BUF_LEN];
>  
>      for (;;) {
> -        bytes = read(s->inotifyfd, buf, len);
> +        bytes = read(s->inotifyfd, buf, __BUF_LEN);

Again, here you can use ARRAY_SIZE(buf) and avoid the macro.

Paolo

>          pos = 0;
>  
>          if (bytes <= 0) {
> @@ -534,6 +534,7 @@ static void inotify_watchfn(void *arg)
>              }
>          }
>      }
> +#undef __BUF_LEN
>  }
>  
>  static int usb_mtp_inotify_init(MTPState *s)
> 

Re: [Qemu-devel] [PATCH 5/8] usb: fix unbounded stack for inotify_watchfn

[Peter Xu]

On Tue, Mar 08, 2016 at 01:22:19PM +0100, Paolo Bonzini wrote:
> >      for (;;) {
> > -        bytes = read(s->inotifyfd, buf, len);
> > +        bytes = read(s->inotifyfd, buf, __BUF_LEN);
> 
> Again, here you can use ARRAY_SIZE(buf) and avoid the macro.

Yes, will fix. Thanks!

Peter

[Qemu-devel] [PATCH 6/8] usb: fix unbounded stack for usb_mtp_add_str

[Peter Xu]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/usb/dev-mtp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index e6dae2f..40fe26e 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -718,16 +718,20 @@ static void usb_mtp_add_wstr(MTPData *data, const wchar_t *str)
 
 static void usb_mtp_add_str(MTPData *data, const char *str)
 {
+#define __WSTR_LEN (256)
     uint32_t len = strlen(str)+1;
-    wchar_t wstr[len];
+    wchar_t wstr[__WSTR_LEN];
     size_t ret;
 
+    assert(len <= __WSTR_LEN);
+
     ret = mbstowcs(wstr, str, len);
     if (ret == -1) {
         usb_mtp_add_wstr(data, L"Oops");
     } else {
         usb_mtp_add_wstr(data, wstr);
     }
+#undef __WSTR_LEN
 }
 
 static void usb_mtp_add_time(MTPData *data, time_t time)
-- 
2.4.3

Re: [Qemu-devel] [PATCH 6/8] usb: fix unbounded stack for usb_mtp_add_str

[Gerd Hoffmann]

>  static void usb_mtp_add_str(MTPData *data, const char *str)
>  {
> +#define __WSTR_LEN (256)
>      uint32_t len = strlen(str)+1;
> -    wchar_t wstr[len];
> +    wchar_t wstr[__WSTR_LEN];

I think we should g_malloc() here.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 6/8] usb: fix unbounded stack for usb_mtp_add_str

[Peter Xu]

On Tue, Mar 08, 2016 at 09:10:44AM +0100, Gerd Hoffmann wrote:
> >  static void usb_mtp_add_str(MTPData *data, const char *str)
> >  {
> > +#define __WSTR_LEN (256)
> >      uint32_t len = strlen(str)+1;
> > -    wchar_t wstr[len];
> > +    wchar_t wstr[__WSTR_LEN];
> 
> I think we should g_malloc() here.

Agree. Will fix. Thanks.

Peter

[Qemu-devel] [PATCH 7/8] migration: fix unbounded stack for source_return_path_thread

[Peter Xu]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0129d9f..f1a3976 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1265,11 +1265,11 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
  */
 static void *source_return_path_thread(void *opaque)
 {
+#define __MAX_LEN (512)
     MigrationState *ms = opaque;
     QEMUFile *rp = ms->rp_state.from_dst_file;
     uint16_t header_len, header_type;
-    const int max_len = 512;
-    uint8_t buf[max_len];
+    uint8_t buf[__MAX_LEN];
     uint32_t tmp32, sibling_error;
     ram_addr_t start = 0; /* =0 to silence warning */
     size_t  len = 0, expected_len;
@@ -1292,7 +1292,7 @@ static void *source_return_path_thread(void *opaque)
 
         if ((rp_cmd_args[header_type].len != -1 &&
             header_len != rp_cmd_args[header_type].len) ||
-            header_len > max_len) {
+            header_len > __MAX_LEN) {
             error_report("RP: Received '%s' message (0x%04x) with"
                     "incorrect length %d expecting %zu",
                     rp_cmd_args[header_type].name, header_type, header_len,
@@ -1372,6 +1372,7 @@ out:
     ms->rp_state.from_dst_file = NULL;
     qemu_fclose(rp);
     return NULL;
+#undef __MAX_LEN
 }
 
 static int open_return_path_on_source(MigrationState *ms)
-- 
2.4.3

Re: [Qemu-devel] [PATCH 7/8] migration: fix unbounded stack for source_return_path_thread

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 0129d9f..f1a3976 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1265,11 +1265,11 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
>   */
>  static void *source_return_path_thread(void *opaque)
>  {
> +#define __MAX_LEN (512)
>      MigrationState *ms = opaque;
>      QEMUFile *rp = ms->rp_state.from_dst_file;
>      uint16_t header_len, header_type;
> -    const int max_len = 512;
> -    uint8_t buf[max_len];
> +    uint8_t buf[__MAX_LEN];
>      uint32_t tmp32, sibling_error;
>      ram_addr_t start = 0; /* =0 to silence warning */
>      size_t  len = 0, expected_len;
> @@ -1292,7 +1292,7 @@ static void *source_return_path_thread(void *opaque)
>  
>          if ((rp_cmd_args[header_type].len != -1 &&
>              header_len != rp_cmd_args[header_type].len) ||
> -            header_len > max_len) {
> +            header_len > __MAX_LEN) {
>              error_report("RP: Received '%s' message (0x%04x) with"
>                      "incorrect length %d expecting %zu",
>                      rp_cmd_args[header_type].name, header_type, header_len,
> @@ -1372,6 +1372,7 @@ out:
>      ms->rp_state.from_dst_file = NULL;
>      qemu_fclose(rp);
>      return NULL;
> +#undef __MAX_LEN
>  }
>  
>  static int open_return_path_on_source(MigrationState *ms)

Reviewed-by: Juan Quintela <quintela@redhat.com>

Do you want to push this through the migration tree or directly?
It is up to you.

Later, Juan.

Re: [Qemu-devel] [PATCH 7/8] migration: fix unbounded stack for source_return_path_thread

[Paolo Bonzini]

On 08/03/2016 10:48, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  migration/migration.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 0129d9f..f1a3976 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1265,11 +1265,11 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
>>   */
>>  static void *source_return_path_thread(void *opaque)
>>  {
>> +#define __MAX_LEN (512)
>>      MigrationState *ms = opaque;
>>      QEMUFile *rp = ms->rp_state.from_dst_file;
>>      uint16_t header_len, header_type;
>> -    const int max_len = 512;
>> -    uint8_t buf[max_len];
>> +    uint8_t buf[__MAX_LEN];
>>      uint32_t tmp32, sibling_error;
>>      ram_addr_t start = 0; /* =0 to silence warning */
>>      size_t  len = 0, expected_len;
>> @@ -1292,7 +1292,7 @@ static void *source_return_path_thread(void *opaque)
>>  
>>          if ((rp_cmd_args[header_type].len != -1 &&
>>              header_len != rp_cmd_args[header_type].len) ||
>> -            header_len > max_len) {
>> +            header_len > __MAX_LEN) {
>>              error_report("RP: Received '%s' message (0x%04x) with"
>>                      "incorrect length %d expecting %zu",
>>                      rp_cmd_args[header_type].name, header_type, header_len,
>> @@ -1372,6 +1372,7 @@ out:
>>      ms->rp_state.from_dst_file = NULL;
>>      qemu_fclose(rp);
>>      return NULL;
>> +#undef __MAX_LEN
>>  }
>>  
>>  static int open_return_path_on_source(MigrationState *ms)
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Not really, because __ is restricted by the C standard and should not be
used in QEMU (besides the problems pointed out by other reviewers for
other patches in the series).

Paolo

Re: [Qemu-devel] [PATCH 7/8] migration: fix unbounded stack for source_return_path_thread

[Paolo Bonzini]

On 08/03/2016 08:00, Peter Xu wrote:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0129d9f..f1a3976 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1265,11 +1265,11 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
>   */
>  static void *source_return_path_thread(void *opaque)
>  {
> +#define __MAX_LEN (512)
>      MigrationState *ms = opaque;
>      QEMUFile *rp = ms->rp_state.from_dst_file;
>      uint16_t header_len, header_type;
> -    const int max_len = 512;
> -    uint8_t buf[max_len];
> +    uint8_t buf[__MAX_LEN];
>      uint32_t tmp32, sibling_error;
>      ram_addr_t start = 0; /* =0 to silence warning */
>      size_t  len = 0, expected_len;
> @@ -1292,7 +1292,7 @@ static void *source_return_path_thread(void *opaque)
>  
>          if ((rp_cmd_args[header_type].len != -1 &&
>              header_len != rp_cmd_args[header_type].len) ||
> -            header_len > max_len) {
> +            header_len > __MAX_LEN) {
>              error_report("RP: Received '%s' message (0x%04x) with"
>                      "incorrect length %d expecting %zu",
>                      rp_cmd_args[header_type].name, header_type, header_len,
> @@ -1372,6 +1372,7 @@ out:
>      ms->rp_state.from_dst_file = NULL;
>      qemu_fclose(rp);
>      return NULL;
> +#undef __MAX_LEN
>  }
>  
>  static int open_return_path_on_source(MigrationState *ms)
> 

Another compiler false positive that you can fix with ARRAY_SIZE.

Paolo

Re: [Qemu-devel] [PATCH 7/8] migration: fix unbounded stack for source_return_path_thread

[Peter Xu]

On Tue, Mar 08, 2016 at 01:26:24PM +0100, Paolo Bonzini wrote:
> 
> 
> On 08/03/2016 08:00, Peter Xu wrote:
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Juan Quintela <quintela@redhat.com>
> > CC: Amit Shah <amit.shah@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 0129d9f..f1a3976 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1265,11 +1265,11 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
> >   */
> >  static void *source_return_path_thread(void *opaque)
> >  {
> > +#define __MAX_LEN (512)
> >      MigrationState *ms = opaque;
> >      QEMUFile *rp = ms->rp_state.from_dst_file;
> >      uint16_t header_len, header_type;
> > -    const int max_len = 512;
> > -    uint8_t buf[max_len];
> > +    uint8_t buf[__MAX_LEN];
> >      uint32_t tmp32, sibling_error;
> >      ram_addr_t start = 0; /* =0 to silence warning */
> >      size_t  len = 0, expected_len;
> > @@ -1292,7 +1292,7 @@ static void *source_return_path_thread(void *opaque)
> >  
> >          if ((rp_cmd_args[header_type].len != -1 &&
> >              header_len != rp_cmd_args[header_type].len) ||
> > -            header_len > max_len) {
> > +            header_len > __MAX_LEN) {
> >              error_report("RP: Received '%s' message (0x%04x) with"
> >                      "incorrect length %d expecting %zu",
> >                      rp_cmd_args[header_type].name, header_type, header_len,
> > @@ -1372,6 +1372,7 @@ out:
> >      ms->rp_state.from_dst_file = NULL;
> >      qemu_fclose(rp);
> >      return NULL;
> > +#undef __MAX_LEN
> >  }
> >  
> >  static int open_return_path_on_source(MigrationState *ms)
> > 
> 
> Another compiler false positive that you can fix with ARRAY_SIZE.

Yes, will fix.

Thanks.
Peter

[Qemu-devel] [PATCH 8/8] hw/i386: fix unbounded stack for load_multiboot

[Peter Xu]

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Richard Henderson <rth@twiddle.net>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/multiboot.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 9e164e6..0eecb9a 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -159,6 +159,12 @@ int load_multiboot(FWCfgState *fw_cfg,
     uint8_t *mb_bootinfo_data;
     uint32_t cmdline_len;
 
+#define __KERN_FNAME_LEN (1024)
+#define __KERN_CMDLINE_LEN (4096)
+
+    assert(strlen(kernel_filename) + 1 >= __KERN_FNAME_LEN);
+    assert(strlen(kernel_cmdline) + 1 >= __KERN_CMDLINE_LEN);
+
     /* Ok, let's see if it is a multiboot image.
        The header is 12x32bit long, so the latest entry may be 8192 - 48. */
     for (i = 0; i < (8192 - 48); i += 4) {
@@ -324,7 +330,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     }
 
     /* Commandline support */
-    char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
+    char kcmdline[__KERN_FNAME_LEN + __KERN_CMDLINE_LEN];
     snprintf(kcmdline, sizeof(kcmdline), "%s %s",
              kernel_filename, kernel_cmdline);
     stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
@@ -370,4 +376,6 @@ int load_multiboot(FWCfgState *fw_cfg,
     nb_option_roms++;
 
     return 1; /* yes, we are multiboot */
+#undef __KERN_FNAME_LEN
+#undef __KERN_CMDLINE_LEN
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH 8/8] hw/i386: fix unbounded stack for load_multiboot

[Peter Maydell]

On 8 March 2016 at 14:00, Peter Xu <peterx@redhat.com> wrote:
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Richard Henderson <rth@twiddle.net>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/multiboot.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 9e164e6..0eecb9a 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -159,6 +159,12 @@ int load_multiboot(FWCfgState *fw_cfg,
>      uint8_t *mb_bootinfo_data;
>      uint32_t cmdline_len;
>
> +#define __KERN_FNAME_LEN (1024)
> +#define __KERN_CMDLINE_LEN (4096)

Where do these choices of length restriction come from?

> +
> +    assert(strlen(kernel_filename) + 1 >= __KERN_FNAME_LEN);
> +    assert(strlen(kernel_cmdline) + 1 >= __KERN_CMDLINE_LEN);
> +
>      /* Ok, let's see if it is a multiboot image.
>         The header is 12x32bit long, so the latest entry may be 8192 - 48. */
>      for (i = 0; i < (8192 - 48); i += 4) {
> @@ -324,7 +330,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>      }
>
>      /* Commandline support */
> -    char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
> +    char kcmdline[__KERN_FNAME_LEN + __KERN_CMDLINE_LEN];
>      snprintf(kcmdline, sizeof(kcmdline), "%s %s",
>               kernel_filename, kernel_cmdline);

Why can't we just dynamically allocate the command line
(ie g_strdup_printf() it) ?

>      stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
> @@ -370,4 +376,6 @@ int load_multiboot(FWCfgState *fw_cfg,
>      nb_option_roms++;
>
>      return 1; /* yes, we are multiboot */
> +#undef __KERN_FNAME_LEN
> +#undef __KERN_CMDLINE_LEN

thanks
-- PMM

Re: [Qemu-devel] [PATCH 8/8] hw/i386: fix unbounded stack for load_multiboot

[Paolo Bonzini]

On 08/03/2016 08:00, Peter Xu wrote:
> @@ -159,6 +159,12 @@ int load_multiboot(FWCfgState *fw_cfg,
>      uint8_t *mb_bootinfo_data;
>      uint32_t cmdline_len;
>  
> +#define __KERN_FNAME_LEN (1024)
> +#define __KERN_CMDLINE_LEN (4096)
> +
> +    assert(strlen(kernel_filename) + 1 >= __KERN_FNAME_LEN);
> +    assert(strlen(kernel_cmdline) + 1 >= __KERN_CMDLINE_LEN);
> +
>      /* Ok, let's see if it is a multiboot image.
>         The header is 12x32bit long, so the latest entry may be 8192 - 48. */
>      for (i = 0; i < (8192 - 48); i += 4) {
> @@ -324,7 +330,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>      }
>  
>      /* Commandline support */
> -    char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
> +    char kcmdline[__KERN_FNAME_LEN + __KERN_CMDLINE_LEN];
>      snprintf(kcmdline, sizeof(kcmdline), "%s %s",
>               kernel_filename, kernel_cmdline);
>      stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
> @@ -370,4 +376,6 @@ int load_multiboot(FWCfgState *fw_cfg,
>      nb_option_roms++;
>  
>      return 1; /* yes, we are multiboot */
> +#undef __KERN_FNAME_LEN
> +#undef __KERN_CMDLINE_LEN

Just put it in the heap using g_strdup_printf.

Paolo

Re: [Qemu-devel] [PATCH 8/8] hw/i386: fix unbounded stack for load_multiboot

[Peter Xu]

On Tue, Mar 08, 2016 at 01:29:21PM +0100, Paolo Bonzini wrote:
> 
> 
> On 08/03/2016 08:00, Peter Xu wrote:
> > @@ -159,6 +159,12 @@ int load_multiboot(FWCfgState *fw_cfg,
> >      uint8_t *mb_bootinfo_data;
> >      uint32_t cmdline_len;
> >  
> > +#define __KERN_FNAME_LEN (1024)
> > +#define __KERN_CMDLINE_LEN (4096)
> > +
> > +    assert(strlen(kernel_filename) + 1 >= __KERN_FNAME_LEN);
> > +    assert(strlen(kernel_cmdline) + 1 >= __KERN_CMDLINE_LEN);
> > +
> >      /* Ok, let's see if it is a multiboot image.
> >         The header is 12x32bit long, so the latest entry may be 8192 - 48. */
> >      for (i = 0; i < (8192 - 48); i += 4) {
> > @@ -324,7 +330,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> >      }
> >  
> >      /* Commandline support */
> > -    char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
> > +    char kcmdline[__KERN_FNAME_LEN + __KERN_CMDLINE_LEN];
> >      snprintf(kcmdline, sizeof(kcmdline), "%s %s",
> >               kernel_filename, kernel_cmdline);
> >      stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
> > @@ -370,4 +376,6 @@ int load_multiboot(FWCfgState *fw_cfg,
> >      nb_option_roms++;
> >  
> >      return 1; /* yes, we are multiboot */
> > +#undef __KERN_FNAME_LEN
> > +#undef __KERN_CMDLINE_LEN
> 
> Just put it in the heap using g_strdup_printf.

Will fix and send standalone again. Thanks.

Peter