[PATCH 00/23] misc: Remove variable-length arrays on the stack

[PATCH 00/23] misc: Remove variable-length arrays on the stack

[Philippe Mathieu-Daudé]

Hi,

This series is inspired by Gerd Hoffmann and CVE-2021-3527.
It removes all uses of variable-length arrays in the repository,
then enable the '-Wvla' warning to avoid new code using vla
to be merged.

Mostly trivial patches using GLib autofree.

Please review,

Phil.

(based on usb-20210505-pull-request tag)
Based-on: 20210505130716.1128420-1-kraxel@redhat.com

Philippe Mathieu-Daudé (23):
  block/vpc: Avoid dynamic stack allocation
  chardev/baum: Replace magic values by X_MAX / Y_MAX definitions
  chardev/baum: Use definitions to avoid dynamic stack allocation
  chardev/baum: Avoid dynamic stack allocation
  io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1
  hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation
  hw/block/nvme: Use definition to avoid dynamic stack allocation
  hw/block/nvme: Avoid dynamic stack allocation
  hw/net/e1000e_core: Use definition to avoid dynamic stack allocation
  hw/ppc/pnv: Avoid dynamic stack allocation
  hw/intc/xics: Avoid dynamic stack allocation
  hw/i386/multiboot: Avoid dynamic stack allocation
  hw/usb/hcd-xhci: Avoid dynamic stack allocation
  hw/usb/hcd-ohci: Use definition to avoid dynamic stack allocation
  net: Avoid dynamic stack allocation
  ui/curses: Avoid dynamic stack allocation
  ui/spice-display: Avoid dynamic stack allocation
  ui/vnc-enc-hextile: Use definitions to avoid dynamic stack allocation
  ui/vnc-enc-tight: Avoid dynamic stack allocation
  util/iov: Avoid dynamic stack allocation
  target/ppc/kvm: Avoid dynamic stack allocation
  tests/unit/test-vmstate: Avoid dynamic stack allocation
  configure: Prohibit variable-length allocations by using -Wvla CPPFLAG

 configure                       |  2 +-
 ui/vnc-enc-hextile-template.h   |  3 ++-
 block/vpc.c                     |  4 ++--
 chardev/baum.c                  | 22 +++++++++++++---------
 hw/block/dataplane/virtio-blk.c |  7 ++++---
 hw/block/nvme.c                 | 17 +++++++++--------
 hw/i386/multiboot.c             |  5 ++---
 hw/intc/xics.c                  |  2 +-
 hw/net/e1000e_core.c            |  7 ++++---
 hw/net/fsl_etsec/rings.c        |  9 ++++-----
 hw/net/rocker/rocker_of_dpa.c   |  2 +-
 hw/ppc/pnv.c                    |  4 ++--
 hw/ppc/spapr.c                  |  8 ++++----
 hw/ppc/spapr_pci_nvlink2.c      |  2 +-
 hw/usb/hcd-ohci.c               |  7 ++++---
 hw/usb/hcd-xhci.c               |  2 +-
 io/channel-websock.c            |  2 +-
 net/dump.c                      |  2 +-
 net/tap.c                       |  2 +-
 target/ppc/kvm.c                |  2 +-
 tests/unit/test-vmstate.c       |  7 +++----
 ui/curses.c                     |  2 +-
 ui/spice-display.c              |  2 +-
 ui/vnc-enc-tight.c              | 11 ++++++-----
 util/iov.c                      |  2 +-
 25 files changed, 71 insertions(+), 64 deletions(-)

-- 
2.26.3

[PATCH 01/23] block/vpc: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/vpc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 17a705b482a..9ed144331fd 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -509,7 +509,7 @@ static inline int64_t get_image_offset(BlockDriverState *bs, uint64_t offset,
        miss sparse read optimization, but it's not a problem in terms of
        correctness. */
     if (write && (s->last_bitmap_offset != bitmap_offset)) {
-        uint8_t bitmap[s->bitmap_size];
+        g_autofree uint8_t *bitmap = g_malloc(s->bitmap_size);
         int r;
 
         s->last_bitmap_offset = bitmap_offset;
@@ -556,7 +556,7 @@ static int64_t alloc_block(BlockDriverState *bs, int64_t offset)
     int64_t bat_offset;
     uint32_t index, bat_value;
     int ret;
-    uint8_t bitmap[s->bitmap_size];
+    g_autofree uint8_t *bitmap = g_malloc(s->bitmap_size);
 
     /* Check if sector_num is valid */
     if ((offset < 0) || (offset > bs->total_sectors * BDRV_SECTOR_SIZE)) {
-- 
2.26.3

[PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions

[Philippe Mathieu-Daudé]

Replace '84' magic value by the X_MAX definition, and '1' by Y_MAX.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 chardev/baum.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/chardev/baum.c b/chardev/baum.c
index 5deca778bc4..adc3d7b3b56 100644
--- a/chardev/baum.c
+++ b/chardev/baum.c
@@ -87,6 +87,9 @@
 
 #define BUF_SIZE 256
 
+#define X_MAX   84
+#define Y_MAX   1
+
 struct BaumChardev {
     Chardev parent;
 
@@ -244,11 +247,11 @@ static int baum_deferred_init(BaumChardev *baum)
         brlapi_perror("baum: brlapi__getDisplaySize");
         return 0;
     }
-    if (baum->y > 1) {
-        baum->y = 1;
+    if (baum->y > Y_MAX) {
+        baum->y = Y_MAX;
     }
-    if (baum->x > 84) {
-        baum->x = 84;
+    if (baum->x > X_MAX) {
+        baum->x = X_MAX;
     }
 
     con = qemu_console_lookup_by_index(0);
-- 
2.26.3

Re: [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions

[Samuel Thibault]

Philippe Mathieu-Daudé, le mer. 05 mai 2021 23:10:26 +0200, a ecrit:
> Replace '84' magic value by the X_MAX definition, and '1' by Y_MAX.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  chardev/baum.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/chardev/baum.c b/chardev/baum.c
> index 5deca778bc4..adc3d7b3b56 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -87,6 +87,9 @@
>  
>  #define BUF_SIZE 256
>  
> +#define X_MAX   84
> +#define Y_MAX   1
> +
>  struct BaumChardev {
>      Chardev parent;
>  
> @@ -244,11 +247,11 @@ static int baum_deferred_init(BaumChardev *baum)
>          brlapi_perror("baum: brlapi__getDisplaySize");
>          return 0;
>      }
> -    if (baum->y > 1) {
> -        baum->y = 1;
> +    if (baum->y > Y_MAX) {
> +        baum->y = Y_MAX;
>      }
> -    if (baum->x > 84) {
> -        baum->x = 84;
> +    if (baum->x > X_MAX) {
> +        baum->x = X_MAX;
>      }
>  
>      con = qemu_console_lookup_by_index(0);
> -- 
> 2.26.3
> 

Re: [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions

[Marc-André Lureau]

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

On Thu, May 6, 2021 at 1:13 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> Replace '84' magic value by the X_MAX definition, and '1' by Y_MAX.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/baum.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/chardev/baum.c b/chardev/baum.c
> index 5deca778bc4..adc3d7b3b56 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -87,6 +87,9 @@
>
>  #define BUF_SIZE 256
>
> +#define X_MAX   84
> +#define Y_MAX   1
> +
>  struct BaumChardev {
>      Chardev parent;
>
> @@ -244,11 +247,11 @@ static int baum_deferred_init(BaumChardev *baum)
>          brlapi_perror("baum: brlapi__getDisplaySize");
>          return 0;
>      }
> -    if (baum->y > 1) {
> -        baum->y = 1;
> +    if (baum->y > Y_MAX) {
> +        baum->y = Y_MAX;
>      }
> -    if (baum->x > 84) {
> -        baum->x = 84;
> +    if (baum->x > X_MAX) {
> +        baum->x = X_MAX;
>      }
>
>      con = qemu_console_lookup_by_index(0);
> --
> 2.26.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2041 bytes --]

[PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

We know 'x * y' will be at most 'X_MAX * Y_MAX' (which is not
a big value, it is actually 84). Instead of having the compiler
use variable-length array, declare an array able to hold the
maximum 'x * y'.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 chardev/baum.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/chardev/baum.c b/chardev/baum.c
index adc3d7b3b56..0822e9ed5f3 100644
--- a/chardev/baum.c
+++ b/chardev/baum.c
@@ -383,9 +383,9 @@ static int baum_eat_packet(BaumChardev *baum, const uint8_t *buf, int len)
     switch (req) {
     case BAUM_REQ_DisplayData:
     {
-        uint8_t cells[baum->x * baum->y], c;
-        uint8_t text[baum->x * baum->y];
-        uint8_t zero[baum->x * baum->y];
+        uint8_t cells[X_MAX * Y_MAX], c;
+        uint8_t text[X_MAX * Y_MAX];
+        uint8_t zero[X_MAX * Y_MAX];
         int cursor = BRLAPI_CURSOR_OFF;
         int i;
 
@@ -408,7 +408,7 @@ static int baum_eat_packet(BaumChardev *baum, const uint8_t *buf, int len)
         }
         timer_del(baum->cellCount_timer);
 
-        memset(zero, 0, sizeof(zero));
+        memset(zero, 0, baum->x * baum->y);
 
         brlapi_writeArguments_t wa = {
             .displayNumber = BRLAPI_DISPLAY_DEFAULT,
-- 
2.26.3

Re: [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation

[Samuel Thibault]

Philippe Mathieu-Daudé, le mer. 05 mai 2021 23:10:27 +0200, a ecrit:
> We know 'x * y' will be at most 'X_MAX * Y_MAX' (which is not
> a big value, it is actually 84). Instead of having the compiler
> use variable-length array, declare an array able to hold the
> maximum 'x * y'.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  chardev/baum.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/chardev/baum.c b/chardev/baum.c
> index adc3d7b3b56..0822e9ed5f3 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -383,9 +383,9 @@ static int baum_eat_packet(BaumChardev *baum, const uint8_t *buf, int len)
>      switch (req) {
>      case BAUM_REQ_DisplayData:
>      {
> -        uint8_t cells[baum->x * baum->y], c;
> -        uint8_t text[baum->x * baum->y];
> -        uint8_t zero[baum->x * baum->y];
> +        uint8_t cells[X_MAX * Y_MAX], c;
> +        uint8_t text[X_MAX * Y_MAX];
> +        uint8_t zero[X_MAX * Y_MAX];
>          int cursor = BRLAPI_CURSOR_OFF;
>          int i;
>  
> @@ -408,7 +408,7 @@ static int baum_eat_packet(BaumChardev *baum, const uint8_t *buf, int len)
>          }
>          timer_del(baum->cellCount_timer);
>  
> -        memset(zero, 0, sizeof(zero));
> +        memset(zero, 0, baum->x * baum->y);
>  
>          brlapi_writeArguments_t wa = {
>              .displayNumber = BRLAPI_DISPLAY_DEFAULT,
> -- 
> 2.26.3
> 

Re: [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation

[Marc-André Lureau]

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

On Thu, May 6, 2021 at 1:14 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> We know 'x * y' will be at most 'X_MAX * Y_MAX' (which is not
> a big value, it is actually 84). Instead of having the compiler
> use variable-length array, declare an array able to hold the
> maximum 'x * y'.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  chardev/baum.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/chardev/baum.c b/chardev/baum.c
> index adc3d7b3b56..0822e9ed5f3 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -383,9 +383,9 @@ static int baum_eat_packet(BaumChardev *baum, const
> uint8_t *buf, int len)
>      switch (req) {
>      case BAUM_REQ_DisplayData:
>      {
> -        uint8_t cells[baum->x * baum->y], c;
> -        uint8_t text[baum->x * baum->y];
> -        uint8_t zero[baum->x * baum->y];
> +        uint8_t cells[X_MAX * Y_MAX], c;
> +        uint8_t text[X_MAX * Y_MAX];
> +        uint8_t zero[X_MAX * Y_MAX];
>          int cursor = BRLAPI_CURSOR_OFF;
>          int i;
>
> @@ -408,7 +408,7 @@ static int baum_eat_packet(BaumChardev *baum, const
> uint8_t *buf, int len)
>          }
>          timer_del(baum->cellCount_timer);
>
> -        memset(zero, 0, sizeof(zero));
> +        memset(zero, 0, baum->x * baum->y);
>

eh, I would have left the sizeof(zero)..


>          brlapi_writeArguments_t wa = {
>              .displayNumber = BRLAPI_DISPLAY_DEFAULT,
> --
> 2.26.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2659 bytes --]

Re: [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation

[Samuel Thibault]

Marc-André Lureau, le jeu. 06 mai 2021 01:27:25 +0400, a ecrit:
>     @@ -408,7 +408,7 @@ static int baum_eat_packet(BaumChardev *baum, const
>     uint8_t *buf, int len)
>              }
>              timer_del(baum->cellCount_timer);
> 
>     -        memset(zero, 0, sizeof(zero));
>     +        memset(zero, 0, baum->x * baum->y);
> 
> 
> eh, I would have left the sizeof(zero)..

I was wondering too, but below this it's clear that only
baum->x * baum->y is getting used.

Samuel

[PATCH 04/23] chardev/baum: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 chardev/baum.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/chardev/baum.c b/chardev/baum.c
index 0822e9ed5f3..bc09cda3471 100644
--- a/chardev/baum.c
+++ b/chardev/baum.c
@@ -299,7 +299,8 @@ static void baum_chr_accept_input(struct Chardev *chr)
 static void baum_write_packet(BaumChardev *baum, const uint8_t *buf, int len)
 {
     Chardev *chr = CHARDEV(baum);
-    uint8_t io_buf[1 + 2 * len], *cur = io_buf;
+    g_autofree uint8_t *io_buf = g_malloc(1 + 2 * len);
+    uint8_t *cur = io_buf;
     int room;
     *cur++ = ESC;
     while (len--)
-- 
2.26.3

Re: [PATCH 04/23] chardev/baum: Avoid dynamic stack allocation

[Samuel Thibault]

Philippe Mathieu-Daudé, le mer. 05 mai 2021 23:10:28 +0200, a ecrit:
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  chardev/baum.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/chardev/baum.c b/chardev/baum.c
> index 0822e9ed5f3..bc09cda3471 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -299,7 +299,8 @@ static void baum_chr_accept_input(struct Chardev *chr)
>  static void baum_write_packet(BaumChardev *baum, const uint8_t *buf, int len)
>  {
>      Chardev *chr = CHARDEV(baum);
> -    uint8_t io_buf[1 + 2 * len], *cur = io_buf;
> +    g_autofree uint8_t *io_buf = g_malloc(1 + 2 * len);
> +    uint8_t *cur = io_buf;
>      int room;
>      *cur++ = ESC;
>      while (len--)
> -- 
> 2.26.3
> 

Re: [PATCH 04/23] chardev/baum: Avoid dynamic stack allocation

[Marc-André Lureau]

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

On Thu, May 6, 2021 at 1:15 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> Use autofree heap allocation instead of variable-length
> array on the stack.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  chardev/baum.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/baum.c b/chardev/baum.c
> index 0822e9ed5f3..bc09cda3471 100644
> --- a/chardev/baum.c
> +++ b/chardev/baum.c
> @@ -299,7 +299,8 @@ static void baum_chr_accept_input(struct Chardev *chr)
>  static void baum_write_packet(BaumChardev *baum, const uint8_t *buf, int
> len)
>  {
>      Chardev *chr = CHARDEV(baum);
> -    uint8_t io_buf[1 + 2 * len], *cur = io_buf;
> +    g_autofree uint8_t *io_buf = g_malloc(1 + 2 * len);
>

fwiw, for non-bottleneck code, I would simply use g_malloc0() everywhere,
ymmv

+    uint8_t *cur = io_buf;
>      int room;
>      *cur++ = ESC;
>      while (len--)
> --
> 2.26.3
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1969 bytes --]

[PATCH 05/23] io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1

[Philippe Mathieu-Daudé]

The combined_key[... QIO_CHANNEL_WEBSOCK_GUID_LEN ...] array in
qio_channel_websock_handshake_send_res_ok() expands to a call
to strlen(QIO_CHANNEL_WEBSOCK_GUID), and the compiler doesn't
realize the string is const, so consider combined_key[] being
a variable-length array.

To remove the variable-length array, we provide it a hint to
the compiler by using sizeof() - 1 instead of strlen().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 io/channel-websock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 03c1f7cb62f..cd7bba6bde7 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -32,7 +32,7 @@
 
 #define QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN 24
 #define QIO_CHANNEL_WEBSOCK_GUID "258EAFA5-E914-47DA-95CA-C5AB0DC85B11"
-#define QIO_CHANNEL_WEBSOCK_GUID_LEN strlen(QIO_CHANNEL_WEBSOCK_GUID)
+#define QIO_CHANNEL_WEBSOCK_GUID_LEN (sizeof(QIO_CHANNEL_WEBSOCK_GUID) - 1)
 
 #define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "sec-websocket-protocol"
 #define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "sec-websocket-version"
-- 
2.26.3

Re: [PATCH 05/23] io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1

[Daniel P. Berrangé]

On Wed, May 05, 2021 at 11:10:29PM +0200, Philippe Mathieu-Daudé wrote:
> The combined_key[... QIO_CHANNEL_WEBSOCK_GUID_LEN ...] array in
> qio_channel_websock_handshake_send_res_ok() expands to a call
> to strlen(QIO_CHANNEL_WEBSOCK_GUID), and the compiler doesn't
> realize the string is const, so consider combined_key[] being
> a variable-length array.
> 
> To remove the variable-length array, we provide it a hint to
> the compiler by using sizeof() - 1 instead of strlen().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  io/channel-websock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index e9050c8987e..53f5e4d8aa6 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -60,11 +60,12 @@ static void notify_guest_bh(void *opaque)
 {
     VirtIOBlockDataPlane *s = opaque;
     unsigned nvqs = s->conf->num_queues;
-    unsigned long bitmap[BITS_TO_LONGS(nvqs)];
+    long bitmap_nr = BITS_TO_LONGS(nvqs);
+    g_autofree unsigned long *bitmap = g_new(unsigned long, bitmap_nr);
     unsigned j;
 
-    memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
-    memset(s->batch_notify_vqs, 0, sizeof(bitmap));
+    memcpy(bitmap, s->batch_notify_vqs, bitmap_nr * sizeof(*bitmap));
+    memset(s->batch_notify_vqs, 0, bitmap_nr * sizeof(*bitmap));
 
     for (j = 0; j < nvqs; j += BITS_PER_LONG) {
         unsigned long bits = bitmap[j / BITS_PER_LONG];
-- 
2.26.3

Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation

[Stefan Hajnoczi]

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

On Wed, May 05, 2021 at 11:10:30PM +0200, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Why? This is a performance-critical code path and BITS_TO_LONGS(nvqs) is
small.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

On 5/6/21 10:53 AM, Stefan Hajnoczi wrote:
> On Wed, May 05, 2021 at 11:10:30PM +0200, Philippe Mathieu-Daudé wrote:
>> Use autofree heap allocation instead of variable-length
>> array on the stack.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/block/dataplane/virtio-blk.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Why?

The motivation behind removing all variable-length allocations
(and adding CPPFLAG+=-Wvla at the end) is to avoid security
vulnerabilities such CVE-2021-3527.

> This is a performance-critical code path and BITS_TO_LONGS(nvqs) is
> small.

OK, having looked better at nvqs, I suppose this is preferred:

-- >8 --
@@ -60,7 +60,7 @@ static void notify_guest_bh(void *opaque)
 {
     VirtIOBlockDataPlane *s = opaque;
     unsigned nvqs = s->conf->num_queues;
-    unsigned long bitmap[BITS_TO_LONGS(nvqs)];
+    unsigned long bitmap[BITS_TO_LONGS(VIRTIO_QUEUE_MAX)];
     unsigned j;

     memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
---

Would that work for you?

> 
> Stefan
> 

Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation

[Stefan Hajnoczi]

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

On Thu, May 06, 2021 at 11:01:47AM +0200, Philippe Mathieu-Daudé wrote:
> On 5/6/21 10:53 AM, Stefan Hajnoczi wrote:
> > On Wed, May 05, 2021 at 11:10:30PM +0200, Philippe Mathieu-Daudé wrote:
> >> Use autofree heap allocation instead of variable-length
> >> array on the stack.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  hw/block/dataplane/virtio-blk.c | 7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > Why?
> 
> The motivation behind removing all variable-length allocations
> (and adding CPPFLAG+=-Wvla at the end) is to avoid security
> vulnerabilities such CVE-2021-3527.

I see. Please mention it in the commit description. There could be other
reasons for this change, like minimizing stack usage, so I wasn't sure
why.

> > This is a performance-critical code path and BITS_TO_LONGS(nvqs) is
> > small.
> 
> OK, having looked better at nvqs, I suppose this is preferred:
> 
> -- >8 --
> @@ -60,7 +60,7 @@ static void notify_guest_bh(void *opaque)
>  {
>      VirtIOBlockDataPlane *s = opaque;
>      unsigned nvqs = s->conf->num_queues;
> -    unsigned long bitmap[BITS_TO_LONGS(nvqs)];
> +    unsigned long bitmap[BITS_TO_LONGS(VIRTIO_QUEUE_MAX)];
>      unsigned j;
> 
>      memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
> ---
> 
> Would that work for you?

It's a little risky since s->batch_notify_vqs does not have
sizeof(bitmap). That makes uninitialized data and buffer overflows more
likely. Your example has the bug:

  memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
                                      ^^^^^^^^^^^^^^
  Accesses beyond the end of s->batch_notify_vqs[].

Can we eliminate bitmap[] entirely by using bitops.h APIs on
s->batch_notify_vqs instead?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

On 5/6/21 4:47 PM, Stefan Hajnoczi wrote:
> On Thu, May 06, 2021 at 11:01:47AM +0200, Philippe Mathieu-Daudé wrote:
>> On 5/6/21 10:53 AM, Stefan Hajnoczi wrote:
>>> On Wed, May 05, 2021 at 11:10:30PM +0200, Philippe Mathieu-Daudé wrote:
>>>> Use autofree heap allocation instead of variable-length
>>>> array on the stack.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  hw/block/dataplane/virtio-blk.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> Why?
>>
>> The motivation behind removing all variable-length allocations
>> (and adding CPPFLAG+=-Wvla at the end) is to avoid security
>> vulnerabilities such CVE-2021-3527.
> 
> I see. Please mention it in the commit description. There could be other
> reasons for this change, like minimizing stack usage, so I wasn't sure
> why.

Fair enough.

>>> This is a performance-critical code path and BITS_TO_LONGS(nvqs) is
>>> small.
>>
>> OK, having looked better at nvqs, I suppose this is preferred:
>>
>> -- >8 --
>> @@ -60,7 +60,7 @@ static void notify_guest_bh(void *opaque)
>>  {
>>      VirtIOBlockDataPlane *s = opaque;
>>      unsigned nvqs = s->conf->num_queues;
>> -    unsigned long bitmap[BITS_TO_LONGS(nvqs)];
>> +    unsigned long bitmap[BITS_TO_LONGS(VIRTIO_QUEUE_MAX)];
>>      unsigned j;
>>
>>      memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
>> ---
>>
>> Would that work for you?
> 
> It's a little risky since s->batch_notify_vqs does not have
> sizeof(bitmap). That makes uninitialized data and buffer overflows more
> likely. Your example has the bug:
> 
>   memcpy(bitmap, s->batch_notify_vqs, sizeof(bitmap));
>                                       ^^^^^^^^^^^^^^
>   Accesses beyond the end of s->batch_notify_vqs[].

Doh I missed that :/

> Can we eliminate bitmap[] entirely by using bitops.h APIs on
> s->batch_notify_vqs instead?

You meant the bitmap.h API I assume, OK, better idea!

Thanks,

Phil.

Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation

[Stefan Hajnoczi]

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

On Thu, May 06, 2021 at 05:19:31PM +0200, Philippe Mathieu-Daudé wrote:
> > Can we eliminate bitmap[] entirely by using bitops.h APIs on
> > s->batch_notify_vqs instead?
> 
> You meant the bitmap.h API I assume, OK, better idea!

I did mean include/qemu/bitops.h. Not sure I see a suitable bitmap.h API
but in any case bitmap.h includes bitops.h :-).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
a constant! Help it by using a definitions instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5fe082ec34c..2f6d4925826 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -812,7 +812,7 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, NvmeSg *sg, NvmeSglDescriptor sgl,
      * descriptors and segment chain) than the command transfer size, so it is
      * not bounded by MDTS.
      */
-    const int SEG_CHUNK_SIZE = 256;
+#define SEG_CHUNK_SIZE 256
 
     NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
     uint64_t nsgld;
-- 
2.26.3

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Keith Busch]

On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote:
> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
> a constant! Help it by using a definitions instead.

I don't understand. It's labeled 'const', so any reasonable compiler
will place it in the 'text' segment of the executable rather than on the
stack. While that's compiler specific, is there really a compiler doing
something bad with this? If not, I do prefer the 'const' here if only
because it limits the symbol scope ('static const' is also preferred if
that helps).

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 5fe082ec34c..2f6d4925826 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -812,7 +812,7 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, NvmeSg *sg, NvmeSglDescriptor sgl,
>       * descriptors and segment chain) than the command transfer size, so it is
>       * not bounded by MDTS.
>       */
> -    const int SEG_CHUNK_SIZE = 256;
> +#define SEG_CHUNK_SIZE 256
>  
>      NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
>      uint64_t nsgld;
> -- 
> 2.26.3
> 

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

+Eric

On 5/5/21 11:22 PM, Keith Busch wrote:
> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote:
>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
>> a constant! Help it by using a definitions instead.
> 
> I don't understand.

Neither do I TBH...

> It's labeled 'const', so any reasonable compiler
> will place it in the 'text' segment of the executable rather than on the
> stack. While that's compiler specific, is there really a compiler doing
> something bad with this? If not, I do prefer the 'const' here if only
> because it limits the symbol scope ('static const' is also preferred if
> that helps).

Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC)

Both static+const / const trigger:

hw/block/nvme.c: In function ‘nvme_map_sgl’:
hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array
‘segment’ [-Werror=vla]
  818 |     NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
      |     ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/block/nvme.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 5fe082ec34c..2f6d4925826 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -812,7 +812,7 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, NvmeSg *sg, NvmeSglDescriptor sgl,
>>       * descriptors and segment chain) than the command transfer size, so it is
>>       * not bounded by MDTS.
>>       */
>> -    const int SEG_CHUNK_SIZE = 256;
>> +#define SEG_CHUNK_SIZE 256
>>  
>>      NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
>>      uint64_t nsgld;
>> -- 
>> 2.26.3
>>
> 

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Eric Blake]

On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote:
> +Eric
> 
> On 5/5/21 11:22 PM, Keith Busch wrote:
>> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote:
>>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
>>> a constant! Help it by using a definitions instead.
>>
>> I don't understand.
> 
> Neither do I TBH...
> 
>> It's labeled 'const', so any reasonable compiler
>> will place it in the 'text' segment of the executable rather than on the
>> stack. While that's compiler specific, is there really a compiler doing
>> something bad with this? If not, I do prefer the 'const' here if only
>> because it limits the symbol scope ('static const' is also preferred if
>> that helps).
> 
> Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC)
> 
> Both static+const / const trigger:
> 
> hw/block/nvme.c: In function ‘nvme_map_sgl’:
> hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array
> ‘segment’ [-Werror=vla]
>   818 |     NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
>       |     ^~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors

C99 6.7.5.2 paragraph 4:
"If the size is an integer constant expression and the element type has
a known constant size, the array type is not a variable length array
type; otherwise, the array type is a variable length array type."

6.6 paragraph 6:
"An integer constant expression shall have integer type and shall only
have operands that are integer constants, enumeration constants,
character constants, sizeof expressions whose results are integer
constants, and floating constants that are the immediate operands of
casts. Cast operators in an integer constant expression shall only
convert arithmetic types to integer types, except as part of an operand
to the sizeof operator."

Notably absent from that list are 'const int' variables, which even
though they act as constants (in that the name always represents the
same value), do not actually qualify as such under C99 rules.  Yes, it's
a pain.  What's more, 6.6 paragraph 10:

"An implementation may accept other forms of constant expressions."

which means it _should_ be possible for the compiler to do what we want.
 But just because it is permitted does not make it actually work. :(

And while C17 expands the list of integer constant expressions to
include _Alignof expressions, it does not add any wording to permit
const variables.

https://stackoverflow.com/questions/62354105/why-is-const-int-x-5-not-a-constant-expression-in-c
helps with this explanation:
"The thing to remember (and yes, this is a bit counterintuitive) is that
const doesn't mean constant. A constant expression is, roughly, one that
can be evaluated at compile time (like 2+2 or 42). The const type
qualifier, even though its name is obviously derived from the English
word "constant", really means "read-only".

Consider, for example, that these are a perfectly valid declarations:

const int r = rand();
const time_t now = time(NULL);

The const just means that you can't modify the value of r or now after
they've been initialized. Those values clearly cannot be determined
until execution time."

And C++ _does_ support named constants, but we're using C, not C++.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Warner Losh]

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

On Wed, May 5, 2021, 5:10 PM Eric Blake <eblake@redhat.com> wrote:

> On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote:
> > +Eric
> >
> > On 5/5/21 11:22 PM, Keith Busch wrote:
> >> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote:
> >>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
> >>> a constant! Help it by using a definitions instead.
> >>
> >> I don't understand.
> >
> > Neither do I TBH...
> >
> >> It's labeled 'const', so any reasonable compiler
> >> will place it in the 'text' segment of the executable rather than on the
> >> stack. While that's compiler specific, is there really a compiler doing
> >> something bad with this? If not, I do prefer the 'const' here if only
> >> because it limits the symbol scope ('static const' is also preferred if
> >> that helps).
> >
> > Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC)
> >
> > Both static+const / const trigger:
> >
> > hw/block/nvme.c: In function ‘nvme_map_sgl’:
> > hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array
> > ‘segment’ [-Werror=vla]
> >   818 |     NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
> >       |     ^~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
>
> C99 6.7.5.2 paragraph 4:
> "If the size is an integer constant expression and the element type has
> a known constant size, the array type is not a variable length array
> type; otherwise, the array type is a variable length array type."
>
> 6.6 paragraph 6:
> "An integer constant expression shall have integer type and shall only
> have operands that are integer constants, enumeration constants,
> character constants, sizeof expressions whose results are integer
> constants, and floating constants that are the immediate operands of
> casts. Cast operators in an integer constant expression shall only
> convert arithmetic types to integer types, except as part of an operand
> to the sizeof operator."
>
> Notably absent from that list are 'const int' variables, which even
> though they act as constants (in that the name always represents the
> same value), do not actually qualify as such under C99 rules.  Yes, it's
> a pain.  What's more, 6.6 paragraph 10:
>
> "An implementation may accept other forms of constant expressions."
>
> which means it _should_ be possible for the compiler to do what we want.
>  But just because it is permitted does not make it actually work. :(
>
> And while C17 expands the list of integer constant expressions to
> include _Alignof expressions, it does not add any wording to permit
> const variables.
>
>
> https://stackoverflow.com/questions/62354105/why-is-const-int-x-5-not-a-constant-expression-in-c
> helps with this explanation:
> "The thing to remember (and yes, this is a bit counterintuitive) is that
> const doesn't mean constant. A constant expression is, roughly, one that
> can be evaluated at compile time (like 2+2 or 42). The const type
> qualifier, even though its name is obviously derived from the English
> word "constant", really means "read-only".
>
> Consider, for example, that these are a perfectly valid declarations:
>
> const int r = rand();
> const time_t now = time(NULL);
>
> The const just means that you can't modify the value of r or now after
> they've been initialized. Those values clearly cannot be determined
> until execution time."
>
> And C++ _does_ support named constants, but we're using C, not C++.
>

Enum is as close as it gets in C if you are eschewing #define.

Warner

-- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
>
>
>

[-- Attachment #2: Type: text/html, Size: 5084 bytes --]

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Keith Busch]

On Wed, May 05, 2021 at 06:09:10PM -0500, Eric Blake wrote:
> On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote:
> > +Eric
> > 
> > On 5/5/21 11:22 PM, Keith Busch wrote:
> >> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote:
> >>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
> >>> a constant! Help it by using a definitions instead.
> >>
> >> I don't understand.
> > 
> > Neither do I TBH...
> > 
> >> It's labeled 'const', so any reasonable compiler
> >> will place it in the 'text' segment of the executable rather than on the
> >> stack. While that's compiler specific, is there really a compiler doing
> >> something bad with this? If not, I do prefer the 'const' here if only
> >> because it limits the symbol scope ('static const' is also preferred if
> >> that helps).
> > 
> > Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC)
> > 
> > Both static+const / const trigger:
> > 
> > hw/block/nvme.c: In function ‘nvme_map_sgl’:
> > hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array
> > ‘segment’ [-Werror=vla]
> >   818 |     NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
> >       |     ^~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> 
> C99 6.7.5.2 paragraph 4:
> "If the size is an integer constant expression and the element type has
> a known constant size, the array type is not a variable length array
> type; otherwise, the array type is a variable length array type."
> 
> 6.6 paragraph 6:
> "An integer constant expression shall have integer type and shall only
> have operands that are integer constants, enumeration constants,
> character constants, sizeof expressions whose results are integer
> constants, and floating constants that are the immediate operands of
> casts. Cast operators in an integer constant expression shall only
> convert arithmetic types to integer types, except as part of an operand
> to the sizeof operator."
> 
> Notably absent from that list are 'const int' variables, which even
> though they act as constants (in that the name always represents the
> same value), do not actually qualify as such under C99 rules.  Yes, it's
> a pain.  What's more, 6.6 paragraph 10:
> 
> "An implementation may accept other forms of constant expressions."
> 
> which means it _should_ be possible for the compiler to do what we want.
>  But just because it is permitted does not make it actually work. :(

Thank you, that makes sense. In this case, we are talking about an
integer constant expression for the value of a 'const' symbol. That
seems like perfect fodder for a compiler to optimize. I understand it's
not obligated to do that, but I assumed it would.

Anyway, Philippe, I have no objection if you want to push forward with
this series.

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

On 5/6/21 4:15 AM, Keith Busch wrote:
> On Wed, May 05, 2021 at 06:09:10PM -0500, Eric Blake wrote:
>> On 5/5/21 5:07 PM, Philippe Mathieu-Daudé wrote:
>>> +Eric
>>>
>>> On 5/5/21 11:22 PM, Keith Busch wrote:
>>>> On Wed, May 05, 2021 at 11:10:31PM +0200, Philippe Mathieu-Daudé wrote:
>>>>> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
>>>>> a constant! Help it by using a definitions instead.
>>>>
>>>> I don't understand.
>>>
>>> Neither do I TBH...
>>>
>>>> It's labeled 'const', so any reasonable compiler
>>>> will place it in the 'text' segment of the executable rather than on the
>>>> stack. While that's compiler specific, is there really a compiler doing
>>>> something bad with this? If not, I do prefer the 'const' here if only
>>>> because it limits the symbol scope ('static const' is also preferred if
>>>> that helps).
>>>
>>> Using: gcc version 10.2.1 20201125 (Red Hat 10.2.1-9) (GCC)
>>>
>>> Both static+const / const trigger:
>>>
>>> hw/block/nvme.c: In function ‘nvme_map_sgl’:
>>> hw/block/nvme.c:818:5: error: ISO C90 forbids variable length array
>>> ‘segment’ [-Werror=vla]
>>>   818 |     NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
>>>       |     ^~~~~~~~~~~~~~~~~
>>> cc1: all warnings being treated as errors
>>
>> C99 6.7.5.2 paragraph 4:
>> "If the size is an integer constant expression and the element type has
>> a known constant size, the array type is not a variable length array
>> type; otherwise, the array type is a variable length array type."
>>
>> 6.6 paragraph 6:
>> "An integer constant expression shall have integer type and shall only
>> have operands that are integer constants, enumeration constants,
>> character constants, sizeof expressions whose results are integer
>> constants, and floating constants that are the immediate operands of
>> casts. Cast operators in an integer constant expression shall only
>> convert arithmetic types to integer types, except as part of an operand
>> to the sizeof operator."
>>
>> Notably absent from that list are 'const int' variables, which even
>> though they act as constants (in that the name always represents the
>> same value), do not actually qualify as such under C99 rules.  Yes, it's
>> a pain.  What's more, 6.6 paragraph 10:
>>
>> "An implementation may accept other forms of constant expressions."
>>
>> which means it _should_ be possible for the compiler to do what we want.
>>  But just because it is permitted does not make it actually work. :(
> 
> Thank you, that makes sense. In this case, we are talking about an
> integer constant expression for the value of a 'const' symbol. That
> seems like perfect fodder for a compiler to optimize. I understand it's
> not obligated to do that, but I assumed it would.
> 
> Anyway, Philippe, I have no objection if you want to push forward with
> this series.

Thanks both. I'll amend Eric explanation in the commit description.

Regards,

Phil.

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Richard Henderson]

On 5/5/21 7:15 PM, Keith Busch wrote:
> Thank you, that makes sense. In this case, we are talking about an
> integer constant expression for the value of a 'const' symbol. That
> seems like perfect fodder for a compiler to optimize. I understand it's
> not obligated to do that, but I assumed it would.

It probably did compile the code such that there is no variable-sized 
allocation from the stack.  That optimization does not change the *semantics* 
of the code as written -- the only reason this compiles is via use of a 
variable length array.


r~

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Klaus Jensen]

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

On May  5 23:10, Philippe Mathieu-Daudé wrote:
>The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
>a constant! Help it by using a definitions instead.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> hw/block/nvme.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 5fe082ec34c..2f6d4925826 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -812,7 +812,7 @@ static uint16_t nvme_map_sgl(NvmeCtrl *n, NvmeSg *sg, NvmeSglDescriptor sgl,
>      * descriptors and segment chain) than the command transfer size, so it is
>      * not bounded by MDTS.
>      */
>-    const int SEG_CHUNK_SIZE = 256;
>+#define SEG_CHUNK_SIZE 256
>
>     NvmeSglDescriptor segment[SEG_CHUNK_SIZE], *sgld, *last_sgld;
>     uint64_t nsgld;
>-- 
>2.26.3
>
>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation

[Richard Henderson]

On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote:
> The compiler isn't clever enough to figure 'SEG_CHUNK_SIZE' is
> a constant! Help it by using a definitions instead.

This isn't about being clever or not, it's semantics.

In C++, "const int" is a proper constant, but in C it is a variable with a 
constant value.  Thus the use of the symbol in the array bounds is, by 
definition, variable.


> -    const int SEG_CHUNK_SIZE = 256;
> +#define SEG_CHUNK_SIZE 256

enum { SEG_CHUNK_SIZE = 256 };

would retain the function scope for the symbol, if that's desirable.


r~

[PATCH 08/23] hw/block/nvme: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/nvme.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2f6d4925826..905c4bb57af 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -652,7 +652,7 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
     len -= trans_len;
     if (len) {
         if (len > n->page_size) {
-            uint64_t prp_list[n->max_prp_ents];
+            g_autofree uint64_t *prp_list = NULL;
             uint32_t nents, prp_trans;
             int i = 0;
 
@@ -662,8 +662,10 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
              * that offset.
              */
             nents = (n->page_size - (prp2 & (n->page_size - 1))) >> 3;
-            prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
-            ret = nvme_addr_read(n, prp2, (void *)prp_list, prp_trans);
+            prp_trans = MIN(n->max_prp_ents, nents);
+            prp_list = g_new(uint64_t, prp_trans);
+            ret = nvme_addr_read(n, prp2, (void *)prp_list,
+                                 prp_trans * sizeof(uint64_t));
             if (ret) {
                 trace_pci_nvme_err_addr_read(prp2);
                 status = NVME_DATA_TRAS_ERROR;
@@ -682,9 +684,8 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
                     i = 0;
                     nents = (len + n->page_size - 1) >> n->page_bits;
                     nents = MIN(nents, n->max_prp_ents);
-                    prp_trans = nents * sizeof(uint64_t);
                     ret = nvme_addr_read(n, prp_ent, (void *)prp_list,
-                                         prp_trans);
+                                         nents * sizeof(uint64_t));
                     if (ret) {
                         trace_pci_nvme_err_addr_read(prp_ent);
                         status = NVME_DATA_TRAS_ERROR;
@@ -2510,10 +2511,10 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
     if (attr & NVME_DSMGMT_AD) {
         int64_t offset;
         size_t len;
-        NvmeDsmRange range[nr];
+        g_autofree NvmeDsmRange *range = g_new(NvmeDsmRange, nr);
         uintptr_t *discards = (uintptr_t *)&req->opaque;
 
-        status = nvme_h2c(n, (uint8_t *)range, sizeof(range), req);
+        status = nvme_h2c(n, (uint8_t *)range, sizeof(*range) * nr, req);
         if (status) {
             return status;
         }
-- 
2.26.3

Re: [PATCH 08/23] hw/block/nvme: Avoid dynamic stack allocation

[Klaus Jensen]

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

On May  5 23:10, Philippe Mathieu-Daudé wrote:
>Use autofree heap allocation instead of variable-length
>array on the stack.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> hw/block/nvme.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 2f6d4925826..905c4bb57af 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -652,7 +652,7 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
>     len -= trans_len;
>     if (len) {
>         if (len > n->page_size) {
>-            uint64_t prp_list[n->max_prp_ents];
>+            g_autofree uint64_t *prp_list = NULL;
>             uint32_t nents, prp_trans;
>             int i = 0;
>
>@@ -662,8 +662,10 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
>              * that offset.
>              */
>             nents = (n->page_size - (prp2 & (n->page_size - 1))) >> 3;
>-            prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
>-            ret = nvme_addr_read(n, prp2, (void *)prp_list, prp_trans);
>+            prp_trans = MIN(n->max_prp_ents, nents);
>+            prp_list = g_new(uint64_t, prp_trans);
>+            ret = nvme_addr_read(n, prp2, (void *)prp_list,
>+                                 prp_trans * sizeof(uint64_t));

prp_trans determines how much we must transfer, not the size of the 
prp_list. Subsequent PRP lists may contain more than nents PRPs, so this 
may now go out of bounds.

Just do the allocation when prp_list is declared:

     g_autofree uint64_t *prp_list = g_new(uint64_t, n->max_prp_ents);

>             if (ret) {
>                 trace_pci_nvme_err_addr_read(prp2);
>                 status = NVME_DATA_TRAS_ERROR;
>@@ -682,9 +684,8 @@ static uint16_t nvme_map_prp(NvmeCtrl *n, NvmeSg *sg, uint64_t prp1,
>                     i = 0;
>                     nents = (len + n->page_size - 1) >> n->page_bits;
>                     nents = MIN(nents, n->max_prp_ents);
>-                    prp_trans = nents * sizeof(uint64_t);
>                     ret = nvme_addr_read(n, prp_ent, (void *)prp_list,
>-                                         prp_trans);
>+                                         nents * sizeof(uint64_t));
>                     if (ret) {
>                         trace_pci_nvme_err_addr_read(prp_ent);
>                         status = NVME_DATA_TRAS_ERROR;
>@@ -2510,10 +2511,10 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest *req)
>     if (attr & NVME_DSMGMT_AD) {
>         int64_t offset;
>         size_t len;
>-        NvmeDsmRange range[nr];
>+        g_autofree NvmeDsmRange *range = g_new(NvmeDsmRange, nr);
>         uintptr_t *discards = (uintptr_t *)&req->opaque;
>
>-        status = nvme_h2c(n, (uint8_t *)range, sizeof(range), req);
>+        status = nvme_h2c(n, (uint8_t *)range, sizeof(*range) * nr, req);
>         if (status) {
>             return status;
>         }

DSM change LGTM.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 09/23] hw/net/e1000e_core: Use definition to avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

The compiler isn't clever enough to figure 'min_buf_size'
is a constant, so help it by using a definitions instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/e1000e_core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index b75f2ab8fc1..4b1d4521a50 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -1621,15 +1621,16 @@ e1000e_rx_fix_l4_csum(E1000ECore *core, struct NetRxPkt *pkt)
     }
 }
 
+/* Min. octets in an ethernet frame sans FCS */
+#define MIN_BUF_SIZE 60
+
 ssize_t
 e1000e_receive_iov(E1000ECore *core, const struct iovec *iov, int iovcnt)
 {
     static const int maximum_ethernet_hdr_len = (14 + 4);
-    /* Min. octets in an ethernet frame sans FCS */
-    static const int min_buf_size = 60;
 
     uint32_t n = 0;
-    uint8_t min_buf[min_buf_size];
+    uint8_t min_buf[MIN_BUF_SIZE];
     struct iovec min_iov;
     uint8_t *filter_buf;
     size_t size, orig_size;
-- 
2.26.3

Re: [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid dynamic stack allocation

[Jason Wang]

在 2021/5/6 上午5:10, Philippe Mathieu-Daudé 写道:
> The compiler isn't clever enough to figure 'min_buf_size'
> is a constant, so help it by using a definitions instead.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   hw/net/e1000e_core.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index b75f2ab8fc1..4b1d4521a50 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -1621,15 +1621,16 @@ e1000e_rx_fix_l4_csum(E1000ECore *core, struct NetRxPkt *pkt)
>       }
>   }
>   
> +/* Min. octets in an ethernet frame sans FCS */
> +#define MIN_BUF_SIZE 60
> +
>   ssize_t
>   e1000e_receive_iov(E1000ECore *core, const struct iovec *iov, int iovcnt)
>   {
>       static const int maximum_ethernet_hdr_len = (14 + 4);
> -    /* Min. octets in an ethernet frame sans FCS */
> -    static const int min_buf_size = 60;
>   
>       uint32_t n = 0;
> -    uint8_t min_buf[min_buf_size];
> +    uint8_t min_buf[MIN_BUF_SIZE];
>       struct iovec min_iov;
>       uint8_t *filter_buf;
>       size_t size, orig_size;

Re: [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid dynamic stack allocation

[Richard Henderson]

On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote:
> The compiler isn't clever enough to figure 'min_buf_size'
> is a constant, so help it by using a definitions instead.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   hw/net/e1000e_core.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

Fix the commit message along the lines of patch 7.
But the patch itself looks fine.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 10/23] hw/ppc/pnv: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/ppc/pnv.c               | 4 ++--
 hw/ppc/spapr.c             | 8 ++++----
 hw/ppc/spapr_pci_nvlink2.c | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 77af846cdfe..f6e3d37b751 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -141,7 +141,7 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
     int smt_threads = CPU_CORE(pc)->nr_threads;
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
-    uint32_t servers_prop[smt_threads];
+    g_autofree uint32_t *servers_prop = g_new(uint32_t, smt_threads);
     int i;
     uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
                        0xffffffff, 0xffffffff};
@@ -244,7 +244,7 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
         servers_prop[i] = cpu_to_be32(pc->pir + i);
     }
     _FDT((fdt_setprop(fdt, offset, "ibm,ppc-interrupt-server#s",
-                       servers_prop, sizeof(servers_prop))));
+                       servers_prop, sizeof(*servers_prop) * smt_threads)));
 }
 
 static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 529ff056dd2..31c2c0d97bf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -176,8 +176,8 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
                                   int smt_threads)
 {
     int i, ret = 0;
-    uint32_t servers_prop[smt_threads];
-    uint32_t gservers_prop[smt_threads * 2];
+    g_autofree uint32_t *servers_prop = g_new(uint32_t, smt_threads);
+    g_autofree uint32_t *gservers_prop = g_new(uint32_t, smt_threads * 2);
     int index = spapr_get_vcpu_id(cpu);
 
     if (cpu->compat_pvr) {
@@ -195,12 +195,12 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
         gservers_prop[i*2 + 1] = 0;
     }
     ret = fdt_setprop(fdt, offset, "ibm,ppc-interrupt-server#s",
-                      servers_prop, sizeof(servers_prop));
+                      servers_prop, sizeof(*servers_prop) * smt_threads);
     if (ret < 0) {
         return ret;
     }
     ret = fdt_setprop(fdt, offset, "ibm,ppc-interrupt-gserver#s",
-                      gservers_prop, sizeof(gservers_prop));
+                      gservers_prop, sizeof(*gservers_prop) * smt_threads * 2);
 
     return ret;
 }
diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
index 8ef9b40a18d..bb61adb114c 100644
--- a/hw/ppc/spapr_pci_nvlink2.c
+++ b/hw/ppc/spapr_pci_nvlink2.c
@@ -401,7 +401,7 @@ void spapr_phb_nvgpu_populate_pcidev_dt(PCIDevice *dev, void *fdt, int offset,
             continue;
         }
         if (dev == nvslot->gpdev) {
-            uint32_t npus[nvslot->linknum];
+            g_autofree uint32_t *npus = g_new(uint32_t, nvslot->linknum);
 
             for (j = 0; j < nvslot->linknum; ++j) {
                 PCIDevice *npdev = nvslot->links[j].npdev;
-- 
2.26.3

Re: [PATCH 10/23] hw/ppc/pnv: Avoid dynamic stack allocation

[David Gibson]

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

On Wed, May 05, 2021 at 11:10:34PM +0200, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/pnv.c               | 4 ++--
>  hw/ppc/spapr.c             | 8 ++++----
>  hw/ppc/spapr_pci_nvlink2.c | 2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 77af846cdfe..f6e3d37b751 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -141,7 +141,7 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
>      int smt_threads = CPU_CORE(pc)->nr_threads;
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> -    uint32_t servers_prop[smt_threads];
> +    g_autofree uint32_t *servers_prop = g_new(uint32_t, smt_threads);
>      int i;
>      uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
>                         0xffffffff, 0xffffffff};
> @@ -244,7 +244,7 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
>          servers_prop[i] = cpu_to_be32(pc->pir + i);
>      }
>      _FDT((fdt_setprop(fdt, offset, "ibm,ppc-interrupt-server#s",
> -                       servers_prop, sizeof(servers_prop))));
> +                       servers_prop, sizeof(*servers_prop) * smt_threads)));
>  }
>  
>  static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 529ff056dd2..31c2c0d97bf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -176,8 +176,8 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
>                                    int smt_threads)
>  {
>      int i, ret = 0;
> -    uint32_t servers_prop[smt_threads];
> -    uint32_t gservers_prop[smt_threads * 2];
> +    g_autofree uint32_t *servers_prop = g_new(uint32_t, smt_threads);
> +    g_autofree uint32_t *gservers_prop = g_new(uint32_t, smt_threads * 2);
>      int index = spapr_get_vcpu_id(cpu);
>  
>      if (cpu->compat_pvr) {
> @@ -195,12 +195,12 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
>          gservers_prop[i*2 + 1] = 0;
>      }
>      ret = fdt_setprop(fdt, offset, "ibm,ppc-interrupt-server#s",
> -                      servers_prop, sizeof(servers_prop));
> +                      servers_prop, sizeof(*servers_prop) * smt_threads);
>      if (ret < 0) {
>          return ret;
>      }
>      ret = fdt_setprop(fdt, offset, "ibm,ppc-interrupt-gserver#s",
> -                      gservers_prop, sizeof(gservers_prop));
> +                      gservers_prop, sizeof(*gservers_prop) * smt_threads * 2);
>  
>      return ret;
>  }
> diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
> index 8ef9b40a18d..bb61adb114c 100644
> --- a/hw/ppc/spapr_pci_nvlink2.c
> +++ b/hw/ppc/spapr_pci_nvlink2.c
> @@ -401,7 +401,7 @@ void spapr_phb_nvgpu_populate_pcidev_dt(PCIDevice *dev, void *fdt, int offset,
>              continue;
>          }
>          if (dev == nvslot->gpdev) {
> -            uint32_t npus[nvslot->linknum];
> +            g_autofree uint32_t *npus = g_new(uint32_t, nvslot->linknum);
>  
>              for (j = 0; j < nvslot->linknum; ++j) {
>                  PCIDevice *npdev = nvslot->links[j].npdev;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 11/23] hw/intc/xics: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/intc/xics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 68f9d44feb4..c293d00d5c4 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -566,8 +566,8 @@ static void ics_reset_irq(ICSIRQState *irq)
 static void ics_reset(DeviceState *dev)
 {
     ICSState *ics = ICS(dev);
+    g_autofree uint8_t *flags = g_malloc(ics->nr_irqs);
     int i;
-    uint8_t flags[ics->nr_irqs];
 
     for (i = 0; i < ics->nr_irqs; i++) {
         flags[i] = ics->irqs[i].flags;
-- 
2.26.3

Re: [PATCH 11/23] hw/intc/xics: Avoid dynamic stack allocation

[David Gibson]

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

On Wed, May 05, 2021 at 11:10:35PM +0200, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/intc/xics.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 68f9d44feb4..c293d00d5c4 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -566,8 +566,8 @@ static void ics_reset_irq(ICSIRQState *irq)
>  static void ics_reset(DeviceState *dev)
>  {
>      ICSState *ics = ICS(dev);
> +    g_autofree uint8_t *flags = g_malloc(ics->nr_irqs);
>      int i;
> -    uint8_t flags[ics->nr_irqs];
>  
>      for (i = 0; i < ics->nr_irqs; i++) {
>          flags[i] = ics->irqs[i].flags;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 11/23] hw/intc/xics: Avoid dynamic stack allocation

[Greg Kurz]

On Wed,  5 May 2021 23:10:35 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/intc/xics.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 68f9d44feb4..c293d00d5c4 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -566,8 +566,8 @@ static void ics_reset_irq(ICSIRQState *irq)
>  static void ics_reset(DeviceState *dev)
>  {
>      ICSState *ics = ICS(dev);
> +    g_autofree uint8_t *flags = g_malloc(ics->nr_irqs);

I would have made it g_new(uint8_t, ics->nr_irqs) so that changes
in the type of 'flags' that could potentially change the allocated
size are safely detected.

This is unlikely though, so:

Reviewed-by: Greg Kurz <groug@kaod.org>

>      int i;
> -    uint8_t flags[ics->nr_irqs];
>  
>      for (i = 0; i < ics->nr_irqs; i++) {
>          flags[i] = ics->irqs[i].flags;

Re: [PATCH 11/23] hw/intc/xics: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

On 5/6/21 10:22 AM, Greg Kurz wrote:
> On Wed,  5 May 2021 23:10:35 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Use autofree heap allocation instead of variable-length
>> array on the stack.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/intc/xics.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)

>> +    g_autofree uint8_t *flags = g_malloc(ics->nr_irqs);
> 
> I would have made it g_new(uint8_t, ics->nr_irqs) so that changes
> in the type of 'flags' that could potentially change the allocated
> size are safely detected.

OK, will update.

> This is unlikely though, so:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks!

[PATCH 12/23] hw/i386/multiboot: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length array on
the stack. Replace the snprintf() call by g_strdup_printf().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/multiboot.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 9e7d69d4705..ccd197603b1 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -161,6 +161,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     uint8_t *mb_bootinfo_data;
     uint32_t cmdline_len;
     GList *mods = NULL;
+    g_autofree char *kcmdline = NULL;
 
     /* Ok, let's see if it is a multiboot image.
        The header is 12x32bit long, so the latest entry may be 8192 - 48. */
@@ -360,9 +361,7 @@ int load_multiboot(FWCfgState *fw_cfg,
     }
 
     /* Commandline support */
-    char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
-    snprintf(kcmdline, sizeof(kcmdline), "%s %s",
-             kernel_filename, kernel_cmdline);
+    kcmdline = g_strdup_printf("%s %s", kernel_filename, kernel_cmdline);
     stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
 
     stl_p(bootinfo + MBI_BOOTLOADER, mb_add_bootloader(&mbs, bootloader_name));
-- 
2.26.3

Re: [PATCH 12/23] hw/i386/multiboot: Avoid dynamic stack allocation

[Richard Henderson]

On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length array on
> the stack. Replace the snprintf() call by g_strdup_printf().
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   hw/i386/multiboot.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 13/23] hw/usb/hcd-xhci: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/hcd-xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 7acfb8137bc..59a267e3c8b 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2387,7 +2387,7 @@ static void xhci_detach_slot(XHCIState *xhci, USBPort *uport)
 static TRBCCode xhci_get_port_bandwidth(XHCIState *xhci, uint64_t pctx)
 {
     dma_addr_t ctx;
-    uint8_t bw_ctx[xhci->numports+1];
+    g_autofree uint8_t *bw_ctx = g_malloc(xhci->numports + 1);
 
     DPRINTF("xhci_get_port_bandwidth()\n");
 
-- 
2.26.3

Re: [PATCH 13/23] hw/usb/hcd-xhci: Avoid dynamic stack allocation

[Richard Henderson]

On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/usb/hcd-xhci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 7acfb8137bc..59a267e3c8b 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -2387,7 +2387,7 @@ static void xhci_detach_slot(XHCIState *xhci, USBPort *uport)
>   static TRBCCode xhci_get_port_bandwidth(XHCIState *xhci, uint64_t pctx)
>   {
>       dma_addr_t ctx;
> -    uint8_t bw_ctx[xhci->numports+1];
> +    g_autofree uint8_t *bw_ctx = g_malloc(xhci->numports + 1);


There is a later use of sizeof(bw_ctx), which is now broken.

Also, I think you might as well remove this buffer entirely and use the address 
space memset routine you recently added.


r~

[PATCH 14/23] hw/usb/hcd-ohci: Use definition to avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

The compiler isn't clever enough to figure 'width' is a constant,
so help it by using a definitions instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@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 1cf2816772c..d090eee673d 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -894,13 +894,14 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
     return 1;
 }
 
+#define HEX_CHAR_PER_LINE 16
+
 static void ohci_td_pkt(const char *msg, const uint8_t *buf, size_t len)
 {
     bool print16;
     bool printall;
-    const int width = 16;
     int i;
-    char tmp[3 * width + 1];
+    char tmp[3 * HEX_CHAR_PER_LINE + 1];
     char *p = tmp;
 
     print16 = !!trace_event_get_state_backends(TRACE_USB_OHCI_TD_PKT_SHORT);
@@ -911,7 +912,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 % HEX_CHAR_PER_LINE) || (i == len))) {
             if (!printall) {
                 trace_usb_ohci_td_pkt_short(msg, tmp);
                 break;
-- 
2.26.3

Re: [PATCH 14/23] hw/usb/hcd-ohci: Use definition to avoid dynamic stack allocation

[Richard Henderson]

On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote:
> The compiler isn't clever enough to figure 'width' is a constant,
> so help it by using a definitions instead.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   hw/usb/hcd-ohci.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)

Same comment about the commit message.  Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 15/23] net: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/fsl_etsec/rings.c      | 9 ++++-----
 hw/net/rocker/rocker_of_dpa.c | 2 +-
 net/dump.c                    | 2 +-
 net/tap.c                     | 2 +-
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
index 8f084464155..1abdcb5a29c 100644
--- a/hw/net/fsl_etsec/rings.c
+++ b/hw/net/fsl_etsec/rings.c
@@ -381,8 +381,6 @@ static void fill_rx_bd(eTSEC          *etsec,
     uint16_t to_write;
     hwaddr   bufptr = bd->bufptr +
         ((hwaddr)(etsec->regs[TBDBPH].value & 0xF) << 32);
-    uint8_t  padd[etsec->rx_padding];
-    uint8_t  rem;
 
     RING_DEBUG("eTSEC fill Rx buffer @ 0x%016" HWADDR_PRIx
                " size:%zu(padding + crc:%u) + fcb:%u\n",
@@ -423,11 +421,12 @@ static void fill_rx_bd(eTSEC          *etsec,
         /* The remaining bytes are only for padding which is not actually
          * allocated in the data buffer.
          */
-
-        rem = MIN(etsec->regs[MRBLR].value - bd->length, etsec->rx_padding);
+        uint8_t  rem = MIN(etsec->regs[MRBLR].value - bd->length,
+                           etsec->rx_padding);
 
         if (rem > 0) {
-            memset(padd, 0x0, sizeof(padd));
+            g_autofree uint8_t *padd = g_malloc0(etsec->rx_padding);
+
             etsec->rx_padding -= rem;
             *size             -= rem;
             bd->length        += rem;
diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
index b3b8c5bb6d4..3e400ceaef7 100644
--- a/hw/net/rocker/rocker_of_dpa.c
+++ b/hw/net/rocker/rocker_of_dpa.c
@@ -1043,7 +1043,7 @@ static void of_dpa_flow_ig_tbl(OfDpaFlowContext *fc, uint32_t tbl_id)
 static ssize_t of_dpa_ig(World *world, uint32_t pport,
                          const struct iovec *iov, int iovcnt)
 {
-    struct iovec iov_copy[iovcnt + 2];
+    g_autofree struct iovec *iov_copy = g_new(struct iovec, iovcnt + 2);
     OfDpaFlowContext fc = {
         .of_dpa = world_private(world),
         .in_pport = pport,
diff --git a/net/dump.c b/net/dump.c
index 4d538d82a69..b830302e27d 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -68,7 +68,7 @@ static ssize_t dump_receive_iov(DumpState *s, const struct iovec *iov, int cnt)
     int64_t ts;
     int caplen;
     size_t size = iov_size(iov, cnt);
-    struct iovec dumpiov[cnt + 1];
+    g_autofree struct iovec *dumpiov = g_new(struct iovec, cnt + 1);
 
     /* Early return in case of previous error. */
     if (s->fd < 0) {
diff --git a/net/tap.c b/net/tap.c
index bae895e2874..2b9ed8a2cd8 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -120,7 +120,7 @@ static ssize_t tap_receive_iov(NetClientState *nc, const struct iovec *iov,
 {
     TAPState *s = DO_UPCAST(TAPState, nc, nc);
     const struct iovec *iovp = iov;
-    struct iovec iov_copy[iovcnt + 1];
+    g_autofree struct iovec *iov_copy = g_new(struct iovec, iovcnt + 1);
     struct virtio_net_hdr_mrg_rxbuf hdr = { };
 
     if (s->host_vnet_hdr_len && !s->using_vnet_hdr) {
-- 
2.26.3

Re: [PATCH 15/23] net: Avoid dynamic stack allocation

[David Gibson]

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

On Wed, May 05, 2021 at 11:10:39PM +0200, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

fsl_etsec parts
Acked-by: David Gibson <david@gibson.dropbear.id.au>


> ---
>  hw/net/fsl_etsec/rings.c      | 9 ++++-----
>  hw/net/rocker/rocker_of_dpa.c | 2 +-
>  net/dump.c                    | 2 +-
>  net/tap.c                     | 2 +-
>  4 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 8f084464155..1abdcb5a29c 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -381,8 +381,6 @@ static void fill_rx_bd(eTSEC          *etsec,
>      uint16_t to_write;
>      hwaddr   bufptr = bd->bufptr +
>          ((hwaddr)(etsec->regs[TBDBPH].value & 0xF) << 32);
> -    uint8_t  padd[etsec->rx_padding];
> -    uint8_t  rem;
>  
>      RING_DEBUG("eTSEC fill Rx buffer @ 0x%016" HWADDR_PRIx
>                 " size:%zu(padding + crc:%u) + fcb:%u\n",
> @@ -423,11 +421,12 @@ static void fill_rx_bd(eTSEC          *etsec,
>          /* The remaining bytes are only for padding which is not actually
>           * allocated in the data buffer.
>           */
> -
> -        rem = MIN(etsec->regs[MRBLR].value - bd->length, etsec->rx_padding);
> +        uint8_t  rem = MIN(etsec->regs[MRBLR].value - bd->length,
> +                           etsec->rx_padding);
>  
>          if (rem > 0) {
> -            memset(padd, 0x0, sizeof(padd));
> +            g_autofree uint8_t *padd = g_malloc0(etsec->rx_padding);
> +
>              etsec->rx_padding -= rem;
>              *size             -= rem;
>              bd->length        += rem;
> diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
> index b3b8c5bb6d4..3e400ceaef7 100644
> --- a/hw/net/rocker/rocker_of_dpa.c
> +++ b/hw/net/rocker/rocker_of_dpa.c
> @@ -1043,7 +1043,7 @@ static void of_dpa_flow_ig_tbl(OfDpaFlowContext *fc, uint32_t tbl_id)
>  static ssize_t of_dpa_ig(World *world, uint32_t pport,
>                           const struct iovec *iov, int iovcnt)
>  {
> -    struct iovec iov_copy[iovcnt + 2];
> +    g_autofree struct iovec *iov_copy = g_new(struct iovec, iovcnt + 2);
>      OfDpaFlowContext fc = {
>          .of_dpa = world_private(world),
>          .in_pport = pport,
> diff --git a/net/dump.c b/net/dump.c
> index 4d538d82a69..b830302e27d 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -68,7 +68,7 @@ static ssize_t dump_receive_iov(DumpState *s, const struct iovec *iov, int cnt)
>      int64_t ts;
>      int caplen;
>      size_t size = iov_size(iov, cnt);
> -    struct iovec dumpiov[cnt + 1];
> +    g_autofree struct iovec *dumpiov = g_new(struct iovec, cnt + 1);
>  
>      /* Early return in case of previous error. */
>      if (s->fd < 0) {
> diff --git a/net/tap.c b/net/tap.c
> index bae895e2874..2b9ed8a2cd8 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -120,7 +120,7 @@ static ssize_t tap_receive_iov(NetClientState *nc, const struct iovec *iov,
>  {
>      TAPState *s = DO_UPCAST(TAPState, nc, nc);
>      const struct iovec *iovp = iov;
> -    struct iovec iov_copy[iovcnt + 1];
> +    g_autofree struct iovec *iov_copy = g_new(struct iovec, iovcnt + 1);
>      struct virtio_net_hdr_mrg_rxbuf hdr = { };
>  
>      if (s->host_vnet_hdr_len && !s->using_vnet_hdr) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 15/23] net: Avoid dynamic stack allocation

[Jason Wang]

在 2021/5/6 上午5:10, Philippe Mathieu-Daudé 写道:
> Use autofree heap allocation instead of variable-length
> array on the stack.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   hw/net/fsl_etsec/rings.c      | 9 ++++-----
>   hw/net/rocker/rocker_of_dpa.c | 2 +-
>   net/dump.c                    | 2 +-
>   net/tap.c                     | 2 +-
>   4 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 8f084464155..1abdcb5a29c 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -381,8 +381,6 @@ static void fill_rx_bd(eTSEC          *etsec,
>       uint16_t to_write;
>       hwaddr   bufptr = bd->bufptr +
>           ((hwaddr)(etsec->regs[TBDBPH].value & 0xF) << 32);
> -    uint8_t  padd[etsec->rx_padding];
> -    uint8_t  rem;
>   
>       RING_DEBUG("eTSEC fill Rx buffer @ 0x%016" HWADDR_PRIx
>                  " size:%zu(padding + crc:%u) + fcb:%u\n",
> @@ -423,11 +421,12 @@ static void fill_rx_bd(eTSEC          *etsec,
>           /* The remaining bytes are only for padding which is not actually
>            * allocated in the data buffer.
>            */
> -
> -        rem = MIN(etsec->regs[MRBLR].value - bd->length, etsec->rx_padding);
> +        uint8_t  rem = MIN(etsec->regs[MRBLR].value - bd->length,
> +                           etsec->rx_padding);
>   
>           if (rem > 0) {
> -            memset(padd, 0x0, sizeof(padd));
> +            g_autofree uint8_t *padd = g_malloc0(etsec->rx_padding);
> +
>               etsec->rx_padding -= rem;
>               *size             -= rem;
>               bd->length        += rem;
> diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
> index b3b8c5bb6d4..3e400ceaef7 100644
> --- a/hw/net/rocker/rocker_of_dpa.c
> +++ b/hw/net/rocker/rocker_of_dpa.c
> @@ -1043,7 +1043,7 @@ static void of_dpa_flow_ig_tbl(OfDpaFlowContext *fc, uint32_t tbl_id)
>   static ssize_t of_dpa_ig(World *world, uint32_t pport,
>                            const struct iovec *iov, int iovcnt)
>   {
> -    struct iovec iov_copy[iovcnt + 2];
> +    g_autofree struct iovec *iov_copy = g_new(struct iovec, iovcnt + 2);
>       OfDpaFlowContext fc = {
>           .of_dpa = world_private(world),
>           .in_pport = pport,
> diff --git a/net/dump.c b/net/dump.c
> index 4d538d82a69..b830302e27d 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -68,7 +68,7 @@ static ssize_t dump_receive_iov(DumpState *s, const struct iovec *iov, int cnt)
>       int64_t ts;
>       int caplen;
>       size_t size = iov_size(iov, cnt);
> -    struct iovec dumpiov[cnt + 1];
> +    g_autofree struct iovec *dumpiov = g_new(struct iovec, cnt + 1);
>   
>       /* Early return in case of previous error. */
>       if (s->fd < 0) {
> diff --git a/net/tap.c b/net/tap.c
> index bae895e2874..2b9ed8a2cd8 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -120,7 +120,7 @@ static ssize_t tap_receive_iov(NetClientState *nc, const struct iovec *iov,
>   {
>       TAPState *s = DO_UPCAST(TAPState, nc, nc);
>       const struct iovec *iovp = iov;
> -    struct iovec iov_copy[iovcnt + 1];
> +    g_autofree struct iovec *iov_copy = g_new(struct iovec, iovcnt + 1);
>       struct virtio_net_hdr_mrg_rxbuf hdr = { };
>   
>       if (s->host_vnet_hdr_len && !s->using_vnet_hdr) {

[PATCH 16/23] ui/curses: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 ui/curses.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/curses.c b/ui/curses.c
index e4f9588c3e8..f490b2d839d 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -65,7 +65,7 @@ static void curses_update(DisplayChangeListener *dcl,
                           int x, int y, int w, int h)
 {
     console_ch_t *line;
-    cchar_t curses_line[width];
+    g_autofree cchar_t *curses_line = g_new(cchar_t, width);
     wchar_t wch[CCHARW_MAX];
     attr_t attrs;
     short colors;
-- 
2.26.3

Re: [PATCH 16/23] ui/curses: Avoid dynamic stack allocation

[Richard Henderson]

On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   ui/curses.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 17/23] ui/spice-display: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 ui/spice-display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index d22781a23d0..61c4259363b 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -188,7 +188,7 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
 {
     static const int blksize = 32;
     int blocks = DIV_ROUND_UP(surface_width(ssd->ds), blksize);
-    int dirty_top[blocks];
+    g_autofree int *dirty_top = g_new(int, blocks);
     int y, yoff1, yoff2, x, xoff, blk, bw;
     int bpp = surface_bytes_per_pixel(ssd->ds);
     uint8_t *guest, *mirror;
-- 
2.26.3

[PATCH 18/23] ui/vnc-enc-hextile: Use definitions to avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

We know 'pf.bytes_per_pixel' will be at most 'VNC_SERVER_FB_BYTES'
(which is actually 4 bytes for 32bpp). Instead of having the compiler
use variable-length array, use this 'small' maximum length and
autofree to allocate the buffer on the heap.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 ui/vnc-enc-hextile-template.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ui/vnc-enc-hextile-template.h b/ui/vnc-enc-hextile-template.h
index 0c56262afff..85e67bd9d88 100644
--- a/ui/vnc-enc-hextile-template.h
+++ b/ui/vnc-enc-hextile-template.h
@@ -25,10 +25,11 @@ static void CONCAT(send_hextile_tile_, NAME)(VncState *vs,
     int bg_count = 0;
     int fg_count = 0;
     int flags = 0;
-    uint8_t data[(vs->client_pf.bytes_per_pixel + 2) * 16 * 16];
+    g_autofree uint8_t *data = g_malloc((VNC_SERVER_FB_BYTES + 2) * 16 * 16);
     int n_data = 0;
     int n_subtiles = 0;
 
+    assert(vs->client_pf.bytes_per_pixel <= VNC_SERVER_FB_BYTES);
     for (j = 0; j < h; j++) {
         for (i = 0; i < w; i++) {
             switch (n_colors) {
-- 
2.26.3

Re: [PATCH 18/23] ui/vnc-enc-hextile: Use definitions to avoid dynamic stack allocation

[Richard Henderson]

On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote:
> We know 'pf.bytes_per_pixel' will be at most 'VNC_SERVER_FB_BYTES'
> (which is actually 4 bytes for 32bpp). Instead of having the compiler
> use variable-length array, use this 'small' maximum length and
> autofree to allocate the buffer on the heap.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   ui/vnc-enc-hextile-template.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Wait, you know the value is small (max 4), and you don't want to allocate a 
constant 1k on the stack?


r~

[PATCH 19/23] ui/vnc-enc-tight: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 ui/vnc-enc-tight.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index cebd35841a9..ff6027cf8d4 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -1097,13 +1097,13 @@ static int send_palette_rect(VncState *vs, int x, int y,
     switch (vs->client_pf.bytes_per_pixel) {
     case 4:
     {
-        size_t old_offset, offset;
-        uint32_t header[palette_size(palette)];
+        size_t old_offset, offset, palette_sz = palette_size(palette);
+        g_autofree uint32_t *header = g_new(uint32_t, palette_sz);
         struct palette_cb_priv priv = { vs, (uint8_t *)header };
 
         old_offset = vs->output.offset;
         palette_iter(palette, write_palette, &priv);
-        vnc_write(vs, header, sizeof(header));
+        vnc_write(vs, header, palette_sz * sizeof(uint32_t));
 
         if (vs->tight->pixel24) {
             tight_pack24(vs, vs->output.buffer + old_offset, colors, &offset);
@@ -1115,11 +1115,12 @@ static int send_palette_rect(VncState *vs, int x, int y,
     }
     case 2:
     {
-        uint16_t header[palette_size(palette)];
+        size_t palette_sz = palette_size(palette);
+        g_autofree uint16_t *header = g_new(uint16_t, palette_sz);
         struct palette_cb_priv priv = { vs, (uint8_t *)header };
 
         palette_iter(palette, write_palette, &priv);
-        vnc_write(vs, header, sizeof(header));
+        vnc_write(vs, header, palette_sz * sizeof(uint16_t));
         tight_encode_indexed_rect16(vs->tight->tight.buffer, w * h, palette);
         break;
     }
-- 
2.26.3

[PATCH 20/23] util/iov: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/iov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/iov.c b/util/iov.c
index 58c7b3eeee5..fc76d717e14 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -623,7 +623,7 @@ static int sortelem_cmp_src_index(const void *a, const void *b)
  */
 void qemu_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src, void *buf)
 {
-    IOVectorSortElem sortelems[src->niov];
+    g_autofree IOVectorSortElem *sortelems = g_new(IOVectorSortElem, src->niov);
     void *last_end;
     int i;
 
-- 
2.26.3

[PATCH 21/23] target/ppc/kvm: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/ppc/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index ae62daddf7d..90d0230eb86 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2660,7 +2660,7 @@ int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
 {
     int64_t starttime = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-    uint8_t buf[bufsize];
+    g_autofree uint8_t *buf = g_malloc(bufsize);
     ssize_t rc;
 
     do {
-- 
2.26.3

Re: [PATCH 21/23] target/ppc/kvm: Avoid dynamic stack allocation

[David Gibson]

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

On Wed, May 05, 2021 at 11:10:45PM +0200, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target/ppc/kvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index ae62daddf7d..90d0230eb86 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2660,7 +2660,7 @@ int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
>  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
>  {
>      int64_t starttime = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -    uint8_t buf[bufsize];
> +    g_autofree uint8_t *buf = g_malloc(bufsize);
>      ssize_t rc;
>  
>      do {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 22/23] tests/unit/test-vmstate: Avoid dynamic stack allocation

[Philippe Mathieu-Daudé]

Use autofree heap allocation instead of variable-length
array on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/unit/test-vmstate.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index a001879585e..2d7ef42d73f 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -86,17 +86,16 @@ static void save_buffer(const uint8_t *buf, size_t buf_size)
 static void compare_vmstate(const uint8_t *wire, size_t size)
 {
     QEMUFile *f = open_test_file(false);
-    uint8_t result[size];
+    g_autofree uint8_t *result = g_malloc(size);
 
     /* read back as binary */
 
-    g_assert_cmpint(qemu_get_buffer(f, result, sizeof(result)), ==,
-                    sizeof(result));
+    g_assert_cmpint(qemu_get_buffer(f, result, size), ==, size);
     g_assert(!qemu_file_get_error(f));
 
     /* Compare that what is on the file is the same that what we
        expected to be there */
-    SUCCESS(memcmp(result, wire, sizeof(result)));
+    SUCCESS(memcmp(result, wire, size));
 
     /* Must reach EOF */
     qemu_get_byte(f);
-- 
2.26.3

Re: [PATCH 22/23] tests/unit/test-vmstate: Avoid dynamic stack allocation

[Richard Henderson]

On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/unit/test-vmstate.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 23/23] configure: Prohibit variable-length allocations by using -Wvla CPPFLAG

[Philippe Mathieu-Daudé]

Now that we converted all variable-length allocations in the
repository, add the -Wvla CPPFLAG to trigger a build failure
if such allocation is used.

This should help avoiding vulnerabilities such CVE-2021-3527
(see commit range 3f67e2e7f13..05a40b172e4).

Inspired-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 4f374b48890..a78ff15b52f 100755
--- a/configure
+++ b/configure
@@ -552,7 +552,7 @@ ARFLAGS="${ARFLAGS-rv}"
 # provides these semantics.)
 QEMU_CFLAGS="-fno-strict-aliasing -fno-common -fwrapv $QEMU_CFLAGS"
 QEMU_CFLAGS="-Wundef -Wwrite-strings -Wmissing-prototypes $QEMU_CFLAGS"
-QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls $QEMU_CFLAGS"
+QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls -Wvla $QEMU_CFLAGS"
 QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE $QEMU_CFLAGS"
 
 # Flags that are needed during configure but later taken care of by Meson
-- 
2.26.3

Re: [PATCH 23/23] configure: Prohibit variable-length allocations by using -Wvla CPPFLAG

[Richard Henderson]

On 5/5/21 2:10 PM, Philippe Mathieu-Daudé wrote:
> Now that we converted all variable-length allocations in the
> repository, add the -Wvla CPPFLAG to trigger a build failure
> if such allocation is used.
> 
> This should help avoiding vulnerabilities such CVE-2021-3527
> (see commit range 3f67e2e7f13..05a40b172e4).
> 
> Inspired-by: Gerd Hoffmann<kraxel@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   configure | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~