qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/23] misc: Remove variable-length arrays on the stack
@ 2021-05-05 21:10 Philippe Mathieu-Daudé
  2021-05-05 21:10 ` [PATCH 01/23] block/vpc: Avoid dynamic stack allocation Philippe Mathieu-Daudé
                   ` (22 more replies)
  0 siblings, 23 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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




^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 01/23] block/vpc: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-05 21:10 ` [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions Philippe Mathieu-Daudé
                   ` (21 subsequent siblings)
  22 siblings, 0 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
  2021-05-05 21:10 ` [PATCH 01/23] block/vpc: Avoid dynamic stack allocation Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-05 21:12   ` Samuel Thibault
  2021-05-05 21:24   ` Marc-André Lureau
  2021-05-05 21:10 ` [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation Philippe Mathieu-Daudé
                   ` (20 subsequent siblings)
  22 siblings, 2 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	Samuel Thibault, qemu-block, Richard Henderson, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
  2021-05-05 21:10 ` [PATCH 01/23] block/vpc: Avoid dynamic stack allocation Philippe Mathieu-Daudé
  2021-05-05 21:10 ` [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-05 21:14   ` Samuel Thibault
  2021-05-05 21:27   ` Marc-André Lureau
  2021-05-05 21:10 ` [PATCH 04/23] chardev/baum: Avoid " Philippe Mathieu-Daudé
                   ` (19 subsequent siblings)
  22 siblings, 2 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	Samuel Thibault, qemu-block, Richard Henderson, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 04/23] chardev/baum: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-05 21:15   ` Samuel Thibault
  2021-05-05 21:29   ` Marc-André Lureau
  2021-05-05 21:10 ` [PATCH 05/23] io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1 Philippe Mathieu-Daudé
                   ` (18 subsequent siblings)
  22 siblings, 2 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	Samuel Thibault, qemu-block, Richard Henderson, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 05/23] io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 04/23] chardev/baum: Avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-06  8:36   ` Daniel P. Berrangé
  2021-05-05 21:10 ` [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation Philippe Mathieu-Daudé
                   ` (17 subsequent siblings)
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 05/23] io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1 Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-06  8:53   ` Stefan Hajnoczi
  2021-05-05 21:10 ` [PATCH 07/23] hw/block/nvme: Use definition to avoid " Philippe Mathieu-Daudé
                   ` (16 subsequent siblings)
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Stefan Hajnoczi, Marc-André Lureau,
	Paolo Bonzini, 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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (5 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-05 21:22   ` Keith Busch
                     ` (2 more replies)
  2021-05-05 21:10 ` [PATCH 08/23] hw/block/nvme: Avoid " Philippe Mathieu-Daudé
                   ` (15 subsequent siblings)
  22 siblings, 3 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, Max Reitz, Klaus Jensen, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Keith Busch,
	Paolo Bonzini, 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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 08/23] hw/block/nvme: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (6 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 07/23] hw/block/nvme: Use definition to avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-06  6:43   ` Klaus Jensen
  2021-05-05 21:10 ` [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid " Philippe Mathieu-Daudé
                   ` (14 subsequent siblings)
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, Max Reitz, Klaus Jensen, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Keith Busch,
	Paolo Bonzini, 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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (7 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 08/23] hw/block/nvme: Avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-06  3:35   ` Jason Wang
  2021-05-07 16:29   ` Richard Henderson
  2021-05-05 21:10 ` [PATCH 10/23] hw/ppc/pnv: Avoid " Philippe Mathieu-Daudé
                   ` (13 subsequent siblings)
  22 siblings, 2 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Dmitry Fleytman, Daniel P. Berrangé,
	qemu-block, Jason Wang, Richard Henderson, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 10/23] hw/ppc/pnv: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (8 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-06  2:12   ` David Gibson
  2021-05-05 21:10 ` [PATCH 11/23] hw/intc/xics: " Philippe Mathieu-Daudé
                   ` (12 subsequent siblings)
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, Greg Kurz, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini,
	David Gibson, Philippe Mathieu-Daudé,
	Cédric Le Goater

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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 11/23] hw/intc/xics: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (9 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 10/23] hw/ppc/pnv: Avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-06  2:13   ` David Gibson
  2021-05-06  8:22   ` Greg Kurz
  2021-05-05 21:10 ` [PATCH 12/23] hw/i386/multiboot: " Philippe Mathieu-Daudé
                   ` (11 subsequent siblings)
  22 siblings, 2 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, Greg Kurz, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson

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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 12/23] hw/i386/multiboot: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (10 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 11/23] hw/intc/xics: " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-07 16:27   ` Richard Henderson
  2021-05-05 21:10 ` [PATCH 13/23] hw/usb/hcd-xhci: " Philippe Mathieu-Daudé
                   ` (10 subsequent siblings)
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, qemu-block, Michael S. Tsirkin,
	Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 13/23] hw/usb/hcd-xhci: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (11 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 12/23] hw/i386/multiboot: " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-07 16:34   ` Richard Henderson
  2021-05-05 21:10 ` [PATCH 14/23] hw/usb/hcd-ohci: Use definition to avoid " Philippe Mathieu-Daudé
                   ` (9 subsequent siblings)
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 14/23] hw/usb/hcd-ohci: Use definition to avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (12 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 13/23] hw/usb/hcd-xhci: " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-07 16:39   ` Richard Henderson
  2021-05-05 21:10 ` [PATCH 15/23] net: Avoid " Philippe Mathieu-Daudé
                   ` (8 subsequent siblings)
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 15/23] net: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (13 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 14/23] hw/usb/hcd-ohci: Use definition to avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-06  2:15   ` David Gibson
  2021-05-06  7:09   ` Jason Wang
  2021-05-05 21:10 ` [PATCH 16/23] ui/curses: " Philippe Mathieu-Daudé
                   ` (7 subsequent siblings)
  22 siblings, 2 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jiri Pirko, Daniel P. Berrangé,
	qemu-block, Jason Wang, Richard Henderson, Greg Kurz, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini,
	Philippe Mathieu-Daudé,
	David Gibson

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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 16/23] ui/curses: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (14 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 15/23] net: Avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-07 16:42   ` Richard Henderson
  2021-05-05 21:10 ` [PATCH 17/23] ui/spice-display: " Philippe Mathieu-Daudé
                   ` (6 subsequent siblings)
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 17/23] ui/spice-display: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (15 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 16/23] ui/curses: " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-05 21:10 ` [PATCH 18/23] ui/vnc-enc-hextile: Use definitions to avoid " Philippe Mathieu-Daudé
                   ` (5 subsequent siblings)
  22 siblings, 0 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 18/23] ui/vnc-enc-hextile: Use definitions to avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (16 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 17/23] ui/spice-display: " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-07 16:46   ` Richard Henderson
  2021-05-05 21:10 ` [PATCH 19/23] ui/vnc-enc-tight: Avoid " Philippe Mathieu-Daudé
                   ` (4 subsequent siblings)
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 19/23] ui/vnc-enc-tight: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (17 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 18/23] ui/vnc-enc-hextile: Use definitions to avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-05 21:10 ` [PATCH 20/23] util/iov: " Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  22 siblings, 0 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 20/23] util/iov: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (18 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 19/23] ui/vnc-enc-tight: Avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-05 21:10 ` [PATCH 21/23] target/ppc/kvm: " Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  22 siblings, 0 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 21/23] target/ppc/kvm: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (19 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 20/23] util/iov: " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-06  2:16   ` David Gibson
  2021-05-05 21:10 ` [PATCH 22/23] tests/unit/test-vmstate: " Philippe Mathieu-Daudé
  2021-05-05 21:10 ` [PATCH 23/23] configure: Prohibit variable-length allocations by using -Wvla CPPFLAG Philippe Mathieu-Daudé
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	open list:Overall KVM CPUs, qemu-block, Richard Henderson,
	Greg Kurz, qemu-ppc, Gerd Hoffmann, Marc-André Lureau,
	Paolo Bonzini, Philippe Mathieu-Daudé,
	David Gibson

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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 22/23] tests/unit/test-vmstate: Avoid dynamic stack allocation
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (20 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 21/23] target/ppc/kvm: " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-07 16:52   ` Richard Henderson
  2021-05-05 21:10 ` [PATCH 23/23] configure: Prohibit variable-length allocations by using -Wvla CPPFLAG Philippe Mathieu-Daudé
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* [PATCH 23/23] configure: Prohibit variable-length allocations by using -Wvla CPPFLAG
  2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
                   ` (21 preceding siblings ...)
  2021-05-05 21:10 ` [PATCH 22/23] tests/unit/test-vmstate: " Philippe Mathieu-Daudé
@ 2021-05-05 21:10 ` Philippe Mathieu-Daudé
  2021-05-07 16:56   ` Richard Henderson
  22 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 21:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini,
	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



^ permalink raw reply related	[flat|nested] 63+ messages in thread

* Re: [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions
  2021-05-05 21:10 ` [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions Philippe Mathieu-Daudé
@ 2021-05-05 21:12   ` Samuel Thibault
  2021-05-05 21:24   ` Marc-André Lureau
  1 sibling, 0 replies; 63+ messages in thread
From: Samuel Thibault @ 2021-05-05 21:12 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

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
> 


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation Philippe Mathieu-Daudé
@ 2021-05-05 21:14   ` Samuel Thibault
  2021-05-05 21:27   ` Marc-André Lureau
  1 sibling, 0 replies; 63+ messages in thread
From: Samuel Thibault @ 2021-05-05 21:14 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

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
> 


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 04/23] chardev/baum: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 04/23] chardev/baum: Avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:15   ` Samuel Thibault
  2021-05-05 21:29   ` Marc-André Lureau
  1 sibling, 0 replies; 63+ messages in thread
From: Samuel Thibault @ 2021-05-05 21:15 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

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
> 


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 07/23] hw/block/nvme: Use definition to avoid " Philippe Mathieu-Daudé
@ 2021-05-05 21:22   ` Keith Busch
  2021-05-05 22:07     ` Philippe Mathieu-Daudé
  2021-05-06  6:27   ` Klaus Jensen
  2021-05-07 15:59   ` Richard Henderson
  2 siblings, 1 reply; 63+ messages in thread
From: Keith Busch @ 2021-05-05 21:22 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Klaus Jensen,
	Marc-André Lureau

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
> 


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions
  2021-05-05 21:10 ` [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions Philippe Mathieu-Daudé
  2021-05-05 21:12   ` Samuel Thibault
@ 2021-05-05 21:24   ` Marc-André Lureau
  1 sibling, 0 replies; 63+ messages in thread
From: Marc-André Lureau @ 2021-05-05 21:24 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	open list:Block layer core, Richard Henderson, QEMU,
	open list:sPAPR pseries, Gerd Hoffmann, Paolo Bonzini,
	Samuel Thibault

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation Philippe Mathieu-Daudé
  2021-05-05 21:14   ` Samuel Thibault
@ 2021-05-05 21:27   ` Marc-André Lureau
  2021-05-05 21:39     ` Samuel Thibault
  1 sibling, 1 reply; 63+ messages in thread
From: Marc-André Lureau @ 2021-05-05 21:27 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	open list:Block layer core, Richard Henderson, QEMU,
	open list:sPAPR pseries, Gerd Hoffmann, Paolo Bonzini,
	Samuel Thibault

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 04/23] chardev/baum: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 04/23] chardev/baum: Avoid " Philippe Mathieu-Daudé
  2021-05-05 21:15   ` Samuel Thibault
@ 2021-05-05 21:29   ` Marc-André Lureau
  1 sibling, 0 replies; 63+ messages in thread
From: Marc-André Lureau @ 2021-05-05 21:29 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	open list:Block layer core, Richard Henderson, QEMU,
	open list:sPAPR pseries, Gerd Hoffmann, Paolo Bonzini,
	Samuel Thibault

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation
  2021-05-05 21:27   ` Marc-André Lureau
@ 2021-05-05 21:39     ` Samuel Thibault
  0 siblings, 0 replies; 63+ messages in thread
From: Samuel Thibault @ 2021-05-05 21:39 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: Daniel P. Berrangé,
	open list:Block layer core, Richard Henderson, QEMU,
	open list:sPAPR pseries, Gerd Hoffmann, Paolo Bonzini,
	Philippe Mathieu-Daudé

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


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-05 21:22   ` Keith Busch
@ 2021-05-05 22:07     ` Philippe Mathieu-Daudé
  2021-05-05 23:09       ` Eric Blake
  0 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 22:07 UTC (permalink / raw)
  To: Keith Busch
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Klaus Jensen,
	Marc-André Lureau

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



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-05 22:07     ` Philippe Mathieu-Daudé
@ 2021-05-05 23:09       ` Eric Blake
  2021-05-06  0:14         ` Warner Losh
  2021-05-06  2:15         ` Keith Busch
  0 siblings, 2 replies; 63+ messages in thread
From: Eric Blake @ 2021-05-05 23:09 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Keith Busch
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Klaus Jensen,
	Marc-André Lureau

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



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-05 23:09       ` Eric Blake
@ 2021-05-06  0:14         ` Warner Losh
  2021-05-06  2:15         ` Keith Busch
  1 sibling, 0 replies; 63+ messages in thread
From: Warner Losh @ 2021-05-06  0:14 UTC (permalink / raw)
  To: Eric Blake
  Cc: Kevin Wolf, Daniel P. Berrangé,
	open list:Block layer core, Richard Henderson, QEMU Developers,
	Max Reitz, Klaus Jensen, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Keith Busch, Paolo Bonzini,
	Philippe Mathieu-Daudé

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 10/23] hw/ppc/pnv: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 10/23] hw/ppc/pnv: Avoid " Philippe Mathieu-Daudé
@ 2021-05-06  2:12   ` David Gibson
  0 siblings, 0 replies; 63+ messages in thread
From: David Gibson @ 2021-05-06  2:12 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Greg Kurz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau,
	Cédric Le Goater

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 11/23] hw/intc/xics: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 11/23] hw/intc/xics: " Philippe Mathieu-Daudé
@ 2021-05-06  2:13   ` David Gibson
  2021-05-06  8:22   ` Greg Kurz
  1 sibling, 0 replies; 63+ messages in thread
From: David Gibson @ 2021-05-06  2:13 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Greg Kurz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 15/23] net: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 15/23] net: Avoid " Philippe Mathieu-Daudé
@ 2021-05-06  2:15   ` David Gibson
  2021-05-06  7:09   ` Jason Wang
  1 sibling, 0 replies; 63+ messages in thread
From: David Gibson @ 2021-05-06  2:15 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Jiri Pirko, Daniel P. Berrangé,
	qemu-block, Jason Wang, Richard Henderson, qemu-devel, Greg Kurz,
	qemu-ppc, Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-05 23:09       ` Eric Blake
  2021-05-06  0:14         ` Warner Losh
@ 2021-05-06  2:15         ` Keith Busch
  2021-05-06  6:42           ` Philippe Mathieu-Daudé
  2021-05-07 16:22           ` Richard Henderson
  1 sibling, 2 replies; 63+ messages in thread
From: Keith Busch @ 2021-05-06  2:15 UTC (permalink / raw)
  To: Eric Blake
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Klaus Jensen,
	Marc-André Lureau, Philippe Mathieu-Daudé

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.


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 21/23] target/ppc/kvm: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 21/23] target/ppc/kvm: " Philippe Mathieu-Daudé
@ 2021-05-06  2:16   ` David Gibson
  0 siblings, 0 replies; 63+ messages in thread
From: David Gibson @ 2021-05-06  2:16 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	open list:Overall KVM CPUs, qemu-block, Richard Henderson,
	qemu-devel, Greg Kurz, qemu-ppc, Gerd Hoffmann, Paolo Bonzini,
	Marc-André Lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid " Philippe Mathieu-Daudé
@ 2021-05-06  3:35   ` Jason Wang
  2021-05-07 16:29   ` Richard Henderson
  1 sibling, 0 replies; 63+ messages in thread
From: Jason Wang @ 2021-05-06  3:35 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Dmitry Fleytman, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-ppc, Gerd Hoffmann,
	Paolo Bonzini, Marc-André Lureau


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



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 07/23] hw/block/nvme: Use definition to avoid " Philippe Mathieu-Daudé
  2021-05-05 21:22   ` Keith Busch
@ 2021-05-06  6:27   ` Klaus Jensen
  2021-05-07 15:59   ` Richard Henderson
  2 siblings, 0 replies; 63+ messages in thread
From: Klaus Jensen @ 2021-05-06  6:27 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Keith Busch,
	Marc-André Lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-06  2:15         ` Keith Busch
@ 2021-05-06  6:42           ` Philippe Mathieu-Daudé
  2021-05-07 16:22           ` Richard Henderson
  1 sibling, 0 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-06  6:42 UTC (permalink / raw)
  To: Keith Busch, Eric Blake
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Klaus Jensen,
	Marc-André Lureau

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.



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 08/23] hw/block/nvme: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 08/23] hw/block/nvme: Avoid " Philippe Mathieu-Daudé
@ 2021-05-06  6:43   ` Klaus Jensen
  0 siblings, 0 replies; 63+ messages in thread
From: Klaus Jensen @ 2021-05-06  6:43 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Keith Busch,
	Marc-André Lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 15/23] net: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 15/23] net: Avoid " Philippe Mathieu-Daudé
  2021-05-06  2:15   ` David Gibson
@ 2021-05-06  7:09   ` Jason Wang
  1 sibling, 0 replies; 63+ messages in thread
From: Jason Wang @ 2021-05-06  7:09 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Jiri Pirko, Daniel P. Berrangé,
	qemu-block, Richard Henderson, Greg Kurz, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini,
	David Gibson


在 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) {



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 11/23] hw/intc/xics: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 11/23] hw/intc/xics: " Philippe Mathieu-Daudé
  2021-05-06  2:13   ` David Gibson
@ 2021-05-06  8:22   ` Greg Kurz
  2021-05-06 13:52     ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 63+ messages in thread
From: Greg Kurz @ 2021-05-06  8:22 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau,
	David Gibson

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;



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 05/23] io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1
  2021-05-05 21:10 ` [PATCH 05/23] io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1 Philippe Mathieu-Daudé
@ 2021-05-06  8:36   ` Daniel P. Berrangé
  0 siblings, 0 replies; 63+ messages in thread
From: Daniel P. Berrangé @ 2021-05-06  8:36 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-block, Richard Henderson, qemu-devel, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

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



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation Philippe Mathieu-Daudé
@ 2021-05-06  8:53   ` Stefan Hajnoczi
  2021-05-06  9:01     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 63+ messages in thread
From: Stefan Hajnoczi @ 2021-05-06  8:53 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation
  2021-05-06  8:53   ` Stefan Hajnoczi
@ 2021-05-06  9:01     ` Philippe Mathieu-Daudé
  2021-05-06 14:47       ` Stefan Hajnoczi
  0 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-06  9:01 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

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
> 



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 11/23] hw/intc/xics: Avoid dynamic stack allocation
  2021-05-06  8:22   ` Greg Kurz
@ 2021-05-06 13:52     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-06 13:52 UTC (permalink / raw)
  To: Greg Kurz
  Cc: Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau,
	David Gibson

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!



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation
  2021-05-06  9:01     ` Philippe Mathieu-Daudé
@ 2021-05-06 14:47       ` Stefan Hajnoczi
  2021-05-06 15:19         ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 63+ messages in thread
From: Stefan Hajnoczi @ 2021-05-06 14:47 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation
  2021-05-06 14:47       ` Stefan Hajnoczi
@ 2021-05-06 15:19         ` Philippe Mathieu-Daudé
  2021-05-10  9:09           ` Stefan Hajnoczi
  0 siblings, 1 reply; 63+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-06 15:19 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

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.



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 07/23] hw/block/nvme: Use definition to avoid " Philippe Mathieu-Daudé
  2021-05-05 21:22   ` Keith Busch
  2021-05-06  6:27   ` Klaus Jensen
@ 2021-05-07 15:59   ` Richard Henderson
  2 siblings, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 15:59 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Max Reitz, Klaus Jensen, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Keith Busch, Paolo Bonzini

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~


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 07/23] hw/block/nvme: Use definition to avoid dynamic stack allocation
  2021-05-06  2:15         ` Keith Busch
  2021-05-06  6:42           ` Philippe Mathieu-Daudé
@ 2021-05-07 16:22           ` Richard Henderson
  1 sibling, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 16:22 UTC (permalink / raw)
  To: Keith Busch, Eric Blake
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, qemu-devel, Max Reitz, qemu-ppc, Gerd Hoffmann,
	Paolo Bonzini, Klaus Jensen, Marc-André Lureau,
	Philippe Mathieu-Daudé

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~



^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 12/23] hw/i386/multiboot: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 12/23] hw/i386/multiboot: " Philippe Mathieu-Daudé
@ 2021-05-07 16:27   ` Richard Henderson
  0 siblings, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 16:27 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Daniel P. Berrangé,
	Eduardo Habkost, qemu-block, Michael S. Tsirkin, qemu-ppc,
	Gerd Hoffmann, Marc-André Lureau, Paolo Bonzini

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~


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid " Philippe Mathieu-Daudé
  2021-05-06  3:35   ` Jason Wang
@ 2021-05-07 16:29   ` Richard Henderson
  1 sibling, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 16:29 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Dmitry Fleytman, Daniel P. Berrangé,
	qemu-block, Jason Wang, qemu-ppc, Gerd Hoffmann,
	Marc-André Lureau, Paolo Bonzini

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~


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 13/23] hw/usb/hcd-xhci: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 13/23] hw/usb/hcd-xhci: " Philippe Mathieu-Daudé
@ 2021-05-07 16:34   ` Richard Henderson
  0 siblings, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 16:34 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, qemu-ppc, Gerd Hoffmann, Marc-André Lureau,
	Paolo Bonzini

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~


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 14/23] hw/usb/hcd-ohci: Use definition to avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 14/23] hw/usb/hcd-ohci: Use definition to avoid " Philippe Mathieu-Daudé
@ 2021-05-07 16:39   ` Richard Henderson
  0 siblings, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 16:39 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, qemu-ppc, Gerd Hoffmann, Marc-André Lureau,
	Paolo Bonzini

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~


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 16/23] ui/curses: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 16/23] ui/curses: " Philippe Mathieu-Daudé
@ 2021-05-07 16:42   ` Richard Henderson
  0 siblings, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 16:42 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, qemu-ppc, Gerd Hoffmann, Marc-André Lureau,
	Paolo Bonzini

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~


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 18/23] ui/vnc-enc-hextile: Use definitions to avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 18/23] ui/vnc-enc-hextile: Use definitions to avoid " Philippe Mathieu-Daudé
@ 2021-05-07 16:46   ` Richard Henderson
  0 siblings, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 16:46 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, qemu-ppc, Gerd Hoffmann, Marc-André Lureau,
	Paolo Bonzini

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~


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 22/23] tests/unit/test-vmstate: Avoid dynamic stack allocation
  2021-05-05 21:10 ` [PATCH 22/23] tests/unit/test-vmstate: " Philippe Mathieu-Daudé
@ 2021-05-07 16:52   ` Richard Henderson
  0 siblings, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 16:52 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, qemu-ppc, Gerd Hoffmann, Marc-André Lureau,
	Paolo Bonzini

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~


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 23/23] configure: Prohibit variable-length allocations by using -Wvla CPPFLAG
  2021-05-05 21:10 ` [PATCH 23/23] configure: Prohibit variable-length allocations by using -Wvla CPPFLAG Philippe Mathieu-Daudé
@ 2021-05-07 16:56   ` Richard Henderson
  0 siblings, 0 replies; 63+ messages in thread
From: Richard Henderson @ 2021-05-07 16:56 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Daniel P. Berrangé,
	qemu-block, qemu-ppc, Gerd Hoffmann, Marc-André Lureau,
	Paolo Bonzini

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~


^ permalink raw reply	[flat|nested] 63+ messages in thread

* Re: [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation
  2021-05-06 15:19         ` Philippe Mathieu-Daudé
@ 2021-05-10  9:09           ` Stefan Hajnoczi
  0 siblings, 0 replies; 63+ messages in thread
From: Stefan Hajnoczi @ 2021-05-10  9:09 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Kevin Wolf, Daniel P. Berrangé,
	qemu-block, Richard Henderson, qemu-devel, Max Reitz, qemu-ppc,
	Gerd Hoffmann, Paolo Bonzini, Marc-André Lureau

[-- 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 --]

^ permalink raw reply	[flat|nested] 63+ messages in thread

end of thread, other threads:[~2021-05-10  9:10 UTC | newest]

Thread overview: 63+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05 21:10 [PATCH 00/23] misc: Remove variable-length arrays on the stack Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 01/23] block/vpc: Avoid dynamic stack allocation Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 02/23] chardev/baum: Replace magic values by X_MAX / Y_MAX definitions Philippe Mathieu-Daudé
2021-05-05 21:12   ` Samuel Thibault
2021-05-05 21:24   ` Marc-André Lureau
2021-05-05 21:10 ` [PATCH 03/23] chardev/baum: Use definitions to avoid dynamic stack allocation Philippe Mathieu-Daudé
2021-05-05 21:14   ` Samuel Thibault
2021-05-05 21:27   ` Marc-André Lureau
2021-05-05 21:39     ` Samuel Thibault
2021-05-05 21:10 ` [PATCH 04/23] chardev/baum: Avoid " Philippe Mathieu-Daudé
2021-05-05 21:15   ` Samuel Thibault
2021-05-05 21:29   ` Marc-André Lureau
2021-05-05 21:10 ` [PATCH 05/23] io/channel-websock: Replace strlen(const_str) by sizeof(const_str) - 1 Philippe Mathieu-Daudé
2021-05-06  8:36   ` Daniel P. Berrangé
2021-05-05 21:10 ` [PATCH 06/23] hw/block/dataplane/virtio-blk: Avoid dynamic stack allocation Philippe Mathieu-Daudé
2021-05-06  8:53   ` Stefan Hajnoczi
2021-05-06  9:01     ` Philippe Mathieu-Daudé
2021-05-06 14:47       ` Stefan Hajnoczi
2021-05-06 15:19         ` Philippe Mathieu-Daudé
2021-05-10  9:09           ` Stefan Hajnoczi
2021-05-05 21:10 ` [PATCH 07/23] hw/block/nvme: Use definition to avoid " Philippe Mathieu-Daudé
2021-05-05 21:22   ` Keith Busch
2021-05-05 22:07     ` Philippe Mathieu-Daudé
2021-05-05 23:09       ` Eric Blake
2021-05-06  0:14         ` Warner Losh
2021-05-06  2:15         ` Keith Busch
2021-05-06  6:42           ` Philippe Mathieu-Daudé
2021-05-07 16:22           ` Richard Henderson
2021-05-06  6:27   ` Klaus Jensen
2021-05-07 15:59   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 08/23] hw/block/nvme: Avoid " Philippe Mathieu-Daudé
2021-05-06  6:43   ` Klaus Jensen
2021-05-05 21:10 ` [PATCH 09/23] hw/net/e1000e_core: Use definition to avoid " Philippe Mathieu-Daudé
2021-05-06  3:35   ` Jason Wang
2021-05-07 16:29   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 10/23] hw/ppc/pnv: Avoid " Philippe Mathieu-Daudé
2021-05-06  2:12   ` David Gibson
2021-05-05 21:10 ` [PATCH 11/23] hw/intc/xics: " Philippe Mathieu-Daudé
2021-05-06  2:13   ` David Gibson
2021-05-06  8:22   ` Greg Kurz
2021-05-06 13:52     ` Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 12/23] hw/i386/multiboot: " Philippe Mathieu-Daudé
2021-05-07 16:27   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 13/23] hw/usb/hcd-xhci: " Philippe Mathieu-Daudé
2021-05-07 16:34   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 14/23] hw/usb/hcd-ohci: Use definition to avoid " Philippe Mathieu-Daudé
2021-05-07 16:39   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 15/23] net: Avoid " Philippe Mathieu-Daudé
2021-05-06  2:15   ` David Gibson
2021-05-06  7:09   ` Jason Wang
2021-05-05 21:10 ` [PATCH 16/23] ui/curses: " Philippe Mathieu-Daudé
2021-05-07 16:42   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 17/23] ui/spice-display: " Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 18/23] ui/vnc-enc-hextile: Use definitions to avoid " Philippe Mathieu-Daudé
2021-05-07 16:46   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 19/23] ui/vnc-enc-tight: Avoid " Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 20/23] util/iov: " Philippe Mathieu-Daudé
2021-05-05 21:10 ` [PATCH 21/23] target/ppc/kvm: " Philippe Mathieu-Daudé
2021-05-06  2:16   ` David Gibson
2021-05-05 21:10 ` [PATCH 22/23] tests/unit/test-vmstate: " Philippe Mathieu-Daudé
2021-05-07 16:52   ` Richard Henderson
2021-05-05 21:10 ` [PATCH 23/23] configure: Prohibit variable-length allocations by using -Wvla CPPFLAG Philippe Mathieu-Daudé
2021-05-07 16:56   ` Richard Henderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).