qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] system: Forbid alloca()
@ 2025-06-05 19:35 Philippe Mathieu-Daudé
  2025-06-05 19:35 ` [PATCH 1/4] hw/gpio/pca9552: Avoid using g_newa() Philippe Mathieu-Daudé
                   ` (8 more replies)
  0 siblings, 9 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-05 19:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Philippe Mathieu-Daudé,
	Daniel P . Berrangé

Eradicate alloca() uses on system code, then enable
-Walloca to prevent new ones to creep back in.

Philippe Mathieu-Daudé (4):
  hw/gpio/pca9552: Avoid using g_newa()
  backends/tpmL Avoid using g_alloca()
  tests/unit/test-char: Avoid using g_alloca()
  buildsys: Prohibit alloca() use on system code

 meson.build                 | 4 ++++
 backends/tpm/tpm_emulator.c | 4 ++--
 hw/gpio/pca9552.c           | 2 +-
 tests/unit/test-char.c      | 3 +--
 4 files changed, 8 insertions(+), 5 deletions(-)

-- 
2.49.0



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

* [PATCH 1/4] hw/gpio/pca9552: Avoid using g_newa()
  2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
@ 2025-06-05 19:35 ` Philippe Mathieu-Daudé
  2025-06-05 21:03   ` Miles Glenn
  2025-06-05 19:35 ` [PATCH 2/4] backends/tpmL Avoid using g_alloca() Philippe Mathieu-Daudé
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-05 19:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Philippe Mathieu-Daudé,
	Daniel P . Berrangé

We have pin_count <= PCA955X_PIN_COUNT_MAX. Having
PCA955X_PIN_COUNT_MAX = 16, it is safe to explicitly
allocate the char buffer on the stack, without g_newa().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/gpio/pca9552.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
index d65c0a2e90f..1e10238b2e0 100644
--- a/hw/gpio/pca9552.c
+++ b/hw/gpio/pca9552.c
@@ -76,7 +76,7 @@ static void pca955x_display_pins_status(PCA955xState *s,
         return;
     }
     if (trace_event_get_state_backends(TRACE_PCA955X_GPIO_STATUS)) {
-        char *buf = g_newa(char, k->pin_count + 1);
+        char buf[PCA955X_PIN_COUNT_MAX + 1];
 
         for (i = 0; i < k->pin_count; i++) {
             if (extract32(pins_status, i, 1)) {
-- 
2.49.0



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

* [PATCH 2/4] backends/tpmL Avoid using g_alloca()
  2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
  2025-06-05 19:35 ` [PATCH 1/4] hw/gpio/pca9552: Avoid using g_newa() Philippe Mathieu-Daudé
@ 2025-06-05 19:35 ` Philippe Mathieu-Daudé
  2025-06-05 21:23   ` BALATON Zoltan
                     ` (2 more replies)
  2025-06-05 19:35 ` [PATCH 3/4] tests/unit/test-char: " Philippe Mathieu-Daudé
                   ` (6 subsequent siblings)
  8 siblings, 3 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-05 19:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Philippe Mathieu-Daudé,
	Daniel P . Berrangé

tpm_emulator_ctrlcmd() is not in hot path.
Use the heap instead of the stack, removing
the g_alloca() call.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 backends/tpm/tpm_emulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 43d350e895d..4a234ab2c0b 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -129,11 +129,11 @@ static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, unsigned long cmd, void *msg,
     CharBackend *dev = &tpm->ctrl_chr;
     uint32_t cmd_no = cpu_to_be32(cmd);
     ssize_t n = sizeof(uint32_t) + msg_len_in;
-    uint8_t *buf = NULL;
     ptm_res res;
 
     WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
-        buf = g_alloca(n);
+        g_autofree uint8_t *buf = g_malloc(n);
+
         memcpy(buf, &cmd_no, sizeof(cmd_no));
         memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
 
-- 
2.49.0



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

* [PATCH 3/4] tests/unit/test-char: Avoid using g_alloca()
  2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
  2025-06-05 19:35 ` [PATCH 1/4] hw/gpio/pca9552: Avoid using g_newa() Philippe Mathieu-Daudé
  2025-06-05 19:35 ` [PATCH 2/4] backends/tpmL Avoid using g_alloca() Philippe Mathieu-Daudé
@ 2025-06-05 19:35 ` Philippe Mathieu-Daudé
  2025-06-05 20:53   ` Pierrick Bouvier
  2025-06-05 19:35 ` [RFC PATCH 4/4] buildsys: Prohibit alloca() use on system code Philippe Mathieu-Daudé
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-05 19:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Philippe Mathieu-Daudé,
	Daniel P . Berrangé

Do not use g_alloca(), simply allocate the CharBackend
structure on the stack.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 tests/unit/test-char.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
index 60a843b79d9..f30a39f61ff 100644
--- a/tests/unit/test-char.c
+++ b/tests/unit/test-char.c
@@ -993,7 +993,7 @@ static void char_udp_test_internal(Chardev *reuse_chr, int sock)
     struct sockaddr_in other;
     SocketIdleData d = { 0, };
     Chardev *chr;
-    CharBackend *be;
+    CharBackend stack_be, *be = &stack_be;
     socklen_t alen = sizeof(other);
     int ret;
     char buf[10];
@@ -1009,7 +1009,6 @@ static void char_udp_test_internal(Chardev *reuse_chr, int sock)
         chr = qemu_chr_new("client", tmp, NULL);
         g_assert_nonnull(chr);
 
-        be = g_alloca(sizeof(CharBackend));
         qemu_chr_fe_init(be, chr, &error_abort);
     }
 
-- 
2.49.0



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

* [RFC PATCH 4/4] buildsys: Prohibit alloca() use on system code
  2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2025-06-05 19:35 ` [PATCH 3/4] tests/unit/test-char: " Philippe Mathieu-Daudé
@ 2025-06-05 19:35 ` Philippe Mathieu-Daudé
  2025-06-05 20:53 ` [PATCH 0/4] system: Forbid alloca() Pierrick Bouvier
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-05 19:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Philippe Mathieu-Daudé,
	Daniel P . Berrangé

Similarly to commit 64c1a544352 ("meson: Enable -Wvla") with
variable length arrays, forbid alloca() uses on system code.

There are few uses on ancient linux-user code, do not bother
there.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 meson.build | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/meson.build b/meson.build
index ef994676fe9..8c6ccb03c71 100644
--- a/meson.build
+++ b/meson.build
@@ -774,6 +774,10 @@ if host_os != 'darwin'
   endif
 endif
 
+if have_system
+  warn_flags += ['-Walloca']
+endif
+
 # Set up C++ compiler flags
 qemu_cxxflags = []
 if 'cpp' in all_languages
-- 
2.49.0



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

* Re: [PATCH 3/4] tests/unit/test-char: Avoid using g_alloca()
  2025-06-05 19:35 ` [PATCH 3/4] tests/unit/test-char: " Philippe Mathieu-Daudé
@ 2025-06-05 20:53   ` Pierrick Bouvier
  2025-06-06  6:09     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 20+ messages in thread
From: Pierrick Bouvier @ 2025-06-05 20:53 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé

On 6/5/25 12:35 PM, Philippe Mathieu-Daudé wrote:
> Do not use g_alloca(), simply allocate the CharBackend
> structure on the stack.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   tests/unit/test-char.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
> index 60a843b79d9..f30a39f61ff 100644
> --- a/tests/unit/test-char.c
> +++ b/tests/unit/test-char.c
> @@ -993,7 +993,7 @@ static void char_udp_test_internal(Chardev *reuse_chr, int sock)
>       struct sockaddr_in other;
>       SocketIdleData d = { 0, };
>       Chardev *chr;
> -    CharBackend *be;
> +    CharBackend stack_be, *be = &stack_be;
>       socklen_t alen = sizeof(other);
>       int ret;
>       char buf[10];
> @@ -1009,7 +1009,6 @@ static void char_udp_test_internal(Chardev *reuse_chr, int sock)
>           chr = qemu_chr_new("client", tmp, NULL);
>           g_assert_nonnull(chr);
>   
> -        be = g_alloca(sizeof(CharBackend));
>           qemu_chr_fe_init(be, chr, &error_abort);
>       }
>   

Would that be more simple to declare the variable, and use &be in the 
function code?


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

* Re: [PATCH 0/4] system: Forbid alloca()
  2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2025-06-05 19:35 ` [RFC PATCH 4/4] buildsys: Prohibit alloca() use on system code Philippe Mathieu-Daudé
@ 2025-06-05 20:53 ` Pierrick Bouvier
  2025-06-06  8:37 ` Peter Maydell
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: Pierrick Bouvier @ 2025-06-05 20:53 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé

On 6/5/25 12:35 PM, Philippe Mathieu-Daudé wrote:
> Eradicate alloca() uses on system code, then enable
> -Walloca to prevent new ones to creep back in.
> 
> Philippe Mathieu-Daudé (4):
>    hw/gpio/pca9552: Avoid using g_newa()
>    backends/tpmL Avoid using g_alloca()
>    tests/unit/test-char: Avoid using g_alloca()
>    buildsys: Prohibit alloca() use on system code
> 
>   meson.build                 | 4 ++++
>   backends/tpm/tpm_emulator.c | 4 ++--
>   hw/gpio/pca9552.c           | 2 +-
>   tests/unit/test-char.c      | 3 +--
>   4 files changed, 8 insertions(+), 5 deletions(-)
> 

Good idea!

For the series:
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>



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

* Re: [PATCH 1/4] hw/gpio/pca9552: Avoid using g_newa()
  2025-06-05 19:35 ` [PATCH 1/4] hw/gpio/pca9552: Avoid using g_newa() Philippe Mathieu-Daudé
@ 2025-06-05 21:03   ` Miles Glenn
  0 siblings, 0 replies; 20+ messages in thread
From: Miles Glenn @ 2025-06-05 21:03 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Marc-André Lureau,
	Peter Maydell, Stefan Hajnoczi, Stefan Berger,
	Daniel P . Berrangé

Reviewed-by: Glenn Miles <milesg@linux.ibm.com>

Thanks!

Glenn

On Thu, 2025-06-05 at 21:35 +0200, Philippe Mathieu-Daudé wrote:
> We have pin_count <= PCA955X_PIN_COUNT_MAX. Having
> PCA955X_PIN_COUNT_MAX = 16, it is safe to explicitly
> allocate the char buffer on the stack, without g_newa().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/gpio/pca9552.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/gpio/pca9552.c b/hw/gpio/pca9552.c
> index d65c0a2e90f..1e10238b2e0 100644
> --- a/hw/gpio/pca9552.c
> +++ b/hw/gpio/pca9552.c
> @@ -76,7 +76,7 @@ static void pca955x_display_pins_status(PCA955xState *s,
>          return;
>      }
>      if (trace_event_get_state_backends(TRACE_PCA955X_GPIO_STATUS)) {
> -        char *buf = g_newa(char, k->pin_count + 1);
> +        char buf[PCA955X_PIN_COUNT_MAX + 1];
>  
>          for (i = 0; i < k->pin_count; i++) {
>              if (extract32(pins_status, i, 1)) {



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

* Re: [PATCH 2/4] backends/tpmL Avoid using g_alloca()
  2025-06-05 19:35 ` [PATCH 2/4] backends/tpmL Avoid using g_alloca() Philippe Mathieu-Daudé
@ 2025-06-05 21:23   ` BALATON Zoltan
  2025-06-06  7:00     ` Philippe Mathieu-Daudé
  2025-06-06  8:14   ` Thomas Huth
  2025-06-08 19:07   ` Stefan Berger
  2 siblings, 1 reply; 20+ messages in thread
From: BALATON Zoltan @ 2025-06-05 21:23 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé

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

On Thu, 5 Jun 2025, Philippe Mathieu-Daudé wrote:
> tpm_emulator_ctrlcmd() is not in hot path.
> Use the heap instead of the stack, removing
> the g_alloca() call.

Typo in subject L -> :

Regards,
BALATON Zoltan

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> backends/tpm/tpm_emulator.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 43d350e895d..4a234ab2c0b 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -129,11 +129,11 @@ static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, unsigned long cmd, void *msg,
>     CharBackend *dev = &tpm->ctrl_chr;
>     uint32_t cmd_no = cpu_to_be32(cmd);
>     ssize_t n = sizeof(uint32_t) + msg_len_in;
> -    uint8_t *buf = NULL;
>     ptm_res res;
>
>     WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
> -        buf = g_alloca(n);
> +        g_autofree uint8_t *buf = g_malloc(n);
> +
>         memcpy(buf, &cmd_no, sizeof(cmd_no));
>         memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
>
>

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

* Re: [PATCH 3/4] tests/unit/test-char: Avoid using g_alloca()
  2025-06-05 20:53   ` Pierrick Bouvier
@ 2025-06-06  6:09     ` Philippe Mathieu-Daudé
  2025-06-06 16:18       ` Pierrick Bouvier
  0 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-06  6:09 UTC (permalink / raw)
  To: Pierrick Bouvier, qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé

On 5/6/25 22:53, Pierrick Bouvier wrote:
> On 6/5/25 12:35 PM, Philippe Mathieu-Daudé wrote:
>> Do not use g_alloca(), simply allocate the CharBackend
>> structure on the stack.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   tests/unit/test-char.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
>> index 60a843b79d9..f30a39f61ff 100644
>> --- a/tests/unit/test-char.c
>> +++ b/tests/unit/test-char.c
>> @@ -993,7 +993,7 @@ static void char_udp_test_internal(Chardev 
>> *reuse_chr, int sock)
>>       struct sockaddr_in other;
>>       SocketIdleData d = { 0, };
>>       Chardev *chr;
>> -    CharBackend *be;
>> +    CharBackend stack_be, *be = &stack_be;
>>       socklen_t alen = sizeof(other);
>>       int ret;
>>       char buf[10];
>> @@ -1009,7 +1009,6 @@ static void char_udp_test_internal(Chardev 
>> *reuse_chr, int sock)
>>           chr = qemu_chr_new("client", tmp, NULL);
>>           g_assert_nonnull(chr);
>> -        be = g_alloca(sizeof(CharBackend));
>>           qemu_chr_fe_init(be, chr, &error_abort);
>>       }
> 
> Would that be more simple to declare the variable, and use &be in the 
> function code?

More context (see reuse_chr ladder):

-- >8 --
@@ -991,45 +991,44 @@ static int make_udp_socket(int *port)
  static void char_udp_test_internal(Chardev *reuse_chr, int sock)
  {
      struct sockaddr_in other;
      SocketIdleData d = { 0, };
      Chardev *chr;
-    CharBackend *be;
+    CharBackend stack_be, *be = &stack_be;
      socklen_t alen = sizeof(other);
      int ret;
      char buf[10];
      char *tmp = NULL;

      if (reuse_chr) {
          chr = reuse_chr;
          be = chr->be;
      } else {
          int port;
          sock = make_udp_socket(&port);
          tmp = g_strdup_printf("udp:127.0.0.1:%d", port);
          chr = qemu_chr_new("client", tmp, NULL);
          g_assert_nonnull(chr);

-        be = g_alloca(sizeof(CharBackend));
          qemu_chr_fe_init(be, chr, &error_abort);
      }
---


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

* Re: [PATCH 2/4] backends/tpmL Avoid using g_alloca()
  2025-06-05 21:23   ` BALATON Zoltan
@ 2025-06-06  7:00     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-06  7:00 UTC (permalink / raw)
  To: BALATON Zoltan
  Cc: qemu-devel, qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé

On 5/6/25 23:23, BALATON Zoltan wrote:
> On Thu, 5 Jun 2025, Philippe Mathieu-Daudé wrote:
>> tpm_emulator_ctrlcmd() is not in hot path.
>> Use the heap instead of the stack, removing
>> the g_alloca() call.
> 
> Typo in subject L -> :

Oops thanks, I hurt my ring finger and have it now tied with the
middle finger; typing like that is slower and makes me do a lot
of typos ;)



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

* Re: [PATCH 2/4] backends/tpmL Avoid using g_alloca()
  2025-06-05 19:35 ` [PATCH 2/4] backends/tpmL Avoid using g_alloca() Philippe Mathieu-Daudé
  2025-06-05 21:23   ` BALATON Zoltan
@ 2025-06-06  8:14   ` Thomas Huth
  2025-06-08 19:09     ` Stefan Berger
  2025-06-08 19:07   ` Stefan Berger
  2 siblings, 1 reply; 20+ messages in thread
From: Thomas Huth @ 2025-06-06  8:14 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé

On 05/06/2025 21.35, Philippe Mathieu-Daudé wrote:
> tpm_emulator_ctrlcmd() is not in hot path.
> Use the heap instead of the stack, removing
> the g_alloca() call.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   backends/tpm/tpm_emulator.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 43d350e895d..4a234ab2c0b 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -129,11 +129,11 @@ static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, unsigned long cmd, void *msg,
>       CharBackend *dev = &tpm->ctrl_chr;
>       uint32_t cmd_no = cpu_to_be32(cmd);
>       ssize_t n = sizeof(uint32_t) + msg_len_in;
> -    uint8_t *buf = NULL;
>       ptm_res res;
>   
>       WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
> -        buf = g_alloca(n);
> +        g_autofree uint8_t *buf = g_malloc(n);
> +
>           memcpy(buf, &cmd_no, sizeof(cmd_no));
>           memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
>   

With the typo fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>



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

* Re: [PATCH 0/4] system: Forbid alloca()
  2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2025-06-05 20:53 ` [PATCH 0/4] system: Forbid alloca() Pierrick Bouvier
@ 2025-06-06  8:37 ` Peter Maydell
  2025-06-06  8:53   ` Philippe Mathieu-Daudé
  2025-06-06  8:44 ` Alex Bennée
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2025-06-06  8:37 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Stefan Hajnoczi, Stefan Berger,
	Daniel P . Berrangé

On Thu, 5 Jun 2025 at 20:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Eradicate alloca() uses on system code, then enable
> -Walloca to prevent new ones to creep back in.
>
> Philippe Mathieu-Daudé (4):
>   hw/gpio/pca9552: Avoid using g_newa()
>   backends/tpmL Avoid using g_alloca()
>   tests/unit/test-char: Avoid using g_alloca()
>   buildsys: Prohibit alloca() use on system code
>
>  meson.build                 | 4 ++++
>  backends/tpm/tpm_emulator.c | 4 ++--
>  hw/gpio/pca9552.c           | 2 +-
>  tests/unit/test-char.c      | 3 +--
>  4 files changed, 8 insertions(+), 5 deletions(-)

There is also a use of alloca() in target/ppc/kvm.c
in kvmppc_load_htab_chunk(), so I suspect that patch 4
here will break compilation on PPC hosts with KVM enabled.

thanks
-- PMM


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

* Re: [PATCH 0/4] system: Forbid alloca()
  2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
                   ` (5 preceding siblings ...)
  2025-06-06  8:37 ` Peter Maydell
@ 2025-06-06  8:44 ` Alex Bennée
  2025-06-09 13:46 ` Stefan Hajnoczi
  2025-06-10  9:29 ` Philippe Mathieu-Daudé
  8 siblings, 0 replies; 20+ messages in thread
From: Alex Bennée @ 2025-06-06  8:44 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Eradicate alloca() uses on system code, then enable
> -Walloca to prevent new ones to creep back in.

Should we also mention it in style.rst:

  Use of the ``malloc/free/realloc/calloc/valloc/memalign/posix_memalign``
  APIs is not allowed in the QEMU codebase. Instead of these routines,

>
> Philippe Mathieu-Daudé (4):
>   hw/gpio/pca9552: Avoid using g_newa()
>   backends/tpmL Avoid using g_alloca()
>   tests/unit/test-char: Avoid using g_alloca()
>   buildsys: Prohibit alloca() use on system code
>
>  meson.build                 | 4 ++++
>  backends/tpm/tpm_emulator.c | 4 ++--
>  hw/gpio/pca9552.c           | 2 +-
>  tests/unit/test-char.c      | 3 +--
>  4 files changed, 8 insertions(+), 5 deletions(-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


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

* Re: [PATCH 0/4] system: Forbid alloca()
  2025-06-06  8:37 ` Peter Maydell
@ 2025-06-06  8:53   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-06  8:53 UTC (permalink / raw)
  To: Peter Maydell
  Cc: qemu-devel, qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Stefan Hajnoczi, Stefan Berger,
	Daniel P . Berrangé

On 6/6/25 10:37, Peter Maydell wrote:
> On Thu, 5 Jun 2025 at 20:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Eradicate alloca() uses on system code, then enable
>> -Walloca to prevent new ones to creep back in.
>>
>> Philippe Mathieu-Daudé (4):
>>    hw/gpio/pca9552: Avoid using g_newa()
>>    backends/tpmL Avoid using g_alloca()
>>    tests/unit/test-char: Avoid using g_alloca()
>>    buildsys: Prohibit alloca() use on system code
>>
>>   meson.build                 | 4 ++++
>>   backends/tpm/tpm_emulator.c | 4 ++--
>>   hw/gpio/pca9552.c           | 2 +-
>>   tests/unit/test-char.c      | 3 +--
>>   4 files changed, 8 insertions(+), 5 deletions(-)
> 
> There is also a use of alloca() in target/ppc/kvm.c
> in kvmppc_load_htab_chunk(), so I suspect that patch 4
> here will break compilation on PPC hosts with KVM enabled.

Oops sorry I missed that one :/


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

* Re: [PATCH 3/4] tests/unit/test-char: Avoid using g_alloca()
  2025-06-06  6:09     ` Philippe Mathieu-Daudé
@ 2025-06-06 16:18       ` Pierrick Bouvier
  0 siblings, 0 replies; 20+ messages in thread
From: Pierrick Bouvier @ 2025-06-06 16:18 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé

On 6/5/25 11:09 PM, Philippe Mathieu-Daudé wrote:
> On 5/6/25 22:53, Pierrick Bouvier wrote:
>> On 6/5/25 12:35 PM, Philippe Mathieu-Daudé wrote:
>>> Do not use g_alloca(), simply allocate the CharBackend
>>> structure on the stack.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    tests/unit/test-char.c | 3 +--
>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
>>> index 60a843b79d9..f30a39f61ff 100644
>>> --- a/tests/unit/test-char.c
>>> +++ b/tests/unit/test-char.c
>>> @@ -993,7 +993,7 @@ static void char_udp_test_internal(Chardev
>>> *reuse_chr, int sock)
>>>        struct sockaddr_in other;
>>>        SocketIdleData d = { 0, };
>>>        Chardev *chr;
>>> -    CharBackend *be;
>>> +    CharBackend stack_be, *be = &stack_be;
>>>        socklen_t alen = sizeof(other);
>>>        int ret;
>>>        char buf[10];
>>> @@ -1009,7 +1009,6 @@ static void char_udp_test_internal(Chardev
>>> *reuse_chr, int sock)
>>>            chr = qemu_chr_new("client", tmp, NULL);
>>>            g_assert_nonnull(chr);
>>> -        be = g_alloca(sizeof(CharBackend));
>>>            qemu_chr_fe_init(be, chr, &error_abort);
>>>        }
>>
>> Would that be more simple to declare the variable, and use &be in the
>> function code?
> 
> More context (see reuse_chr ladder):
> 
> -- >8 --
> @@ -991,45 +991,44 @@ static int make_udp_socket(int *port)
>    static void char_udp_test_internal(Chardev *reuse_chr, int sock)
>    {
>        struct sockaddr_in other;
>        SocketIdleData d = { 0, };
>        Chardev *chr;
> -    CharBackend *be;
> +    CharBackend stack_be, *be = &stack_be;
>        socklen_t alen = sizeof(other);
>        int ret;
>        char buf[10];
>        char *tmp = NULL;
> 
>        if (reuse_chr) {
>            chr = reuse_chr;
>            be = chr->be;
>        } else {
>            int port;
>            sock = make_udp_socket(&port);
>            tmp = g_strdup_printf("udp:127.0.0.1:%d", port);
>            chr = qemu_chr_new("client", tmp, NULL);
>            g_assert_nonnull(chr);
> 
> -        be = g_alloca(sizeof(CharBackend));
>            qemu_chr_fe_init(be, chr, &error_abort);
>        }
> ---

Ok!


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

* Re: [PATCH 2/4] backends/tpmL Avoid using g_alloca()
  2025-06-05 19:35 ` [PATCH 2/4] backends/tpmL Avoid using g_alloca() Philippe Mathieu-Daudé
  2025-06-05 21:23   ` BALATON Zoltan
  2025-06-06  8:14   ` Thomas Huth
@ 2025-06-08 19:07   ` Stefan Berger
  2 siblings, 0 replies; 20+ messages in thread
From: Stefan Berger @ 2025-06-08 19:07 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé



On 6/5/25 3:35 PM, Philippe Mathieu-Daudé wrote:
> tpm_emulator_ctrlcmd() is not in hot path.
> Use the heap instead of the stack, removing
> the g_alloca() call.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   backends/tpm/tpm_emulator.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 43d350e895d..4a234ab2c0b 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -129,11 +129,11 @@ static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, unsigned long cmd, void *msg,
>       CharBackend *dev = &tpm->ctrl_chr;
>       uint32_t cmd_no = cpu_to_be32(cmd);
>       ssize_t n = sizeof(uint32_t) + msg_len_in;
> -    uint8_t *buf = NULL;
>       ptm_res res;
>   
>       WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
> -        buf = g_alloca(n);
> +        g_autofree uint8_t *buf = g_malloc(n);
> +
>           memcpy(buf, &cmd_no, sizeof(cmd_no));
>           memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
>   



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

* Re: [PATCH 2/4] backends/tpmL Avoid using g_alloca()
  2025-06-06  8:14   ` Thomas Huth
@ 2025-06-08 19:09     ` Stefan Berger
  0 siblings, 0 replies; 20+ messages in thread
From: Stefan Berger @ 2025-06-08 19:09 UTC (permalink / raw)
  To: Thomas Huth, Philippe Mathieu-Daudé, qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé



On 6/6/25 4:14 AM, Thomas Huth wrote:
> On 05/06/2025 21.35, Philippe Mathieu-Daudé wrote:
>> tpm_emulator_ctrlcmd() is not in hot path.
>> Use the heap instead of the stack, removing
>> the g_alloca() call.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   backends/tpm/tpm_emulator.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
>> index 43d350e895d..4a234ab2c0b 100644
>> --- a/backends/tpm/tpm_emulator.c
>> +++ b/backends/tpm/tpm_emulator.c
>> @@ -129,11 +129,11 @@ static int tpm_emulator_ctrlcmd(TPMEmulator 
>> *tpm, unsigned long cmd, void *msg,
>>       CharBackend *dev = &tpm->ctrl_chr;
>>       uint32_t cmd_no = cpu_to_be32(cmd);
>>       ssize_t n = sizeof(uint32_t) + msg_len_in;
>> -    uint8_t *buf = NULL;
>>       ptm_res res;
>>       WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
>> -        buf = g_alloca(n);
>> +        g_autofree uint8_t *buf = g_malloc(n);
>> +
>>           memcpy(buf, &cmd_no, sizeof(cmd_no));
>>           memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
> 
> With the typo fixed:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> 
> 



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

* Re: [PATCH 0/4] system: Forbid alloca()
  2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
                   ` (6 preceding siblings ...)
  2025-06-06  8:44 ` Alex Bennée
@ 2025-06-09 13:46 ` Stefan Hajnoczi
  2025-06-10  9:29 ` Philippe Mathieu-Daudé
  8 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2025-06-09 13:46 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Berger,
	Daniel P . Berrangé

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

On Thu, Jun 05, 2025 at 09:35:36PM +0200, Philippe Mathieu-Daudé wrote:
> Eradicate alloca() uses on system code, then enable
> -Walloca to prevent new ones to creep back in.
> 
> Philippe Mathieu-Daudé (4):
>   hw/gpio/pca9552: Avoid using g_newa()
>   backends/tpmL Avoid using g_alloca()
>   tests/unit/test-char: Avoid using g_alloca()
>   buildsys: Prohibit alloca() use on system code
> 
>  meson.build                 | 4 ++++
>  backends/tpm/tpm_emulator.c | 4 ++--
>  hw/gpio/pca9552.c           | 2 +-
>  tests/unit/test-char.c      | 3 +--
>  4 files changed, 8 insertions(+), 5 deletions(-)

Modulo the comments that have already been discussed:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

* Re: [PATCH 0/4] system: Forbid alloca()
  2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
                   ` (7 preceding siblings ...)
  2025-06-09 13:46 ` Stefan Hajnoczi
@ 2025-06-10  9:29 ` Philippe Mathieu-Daudé
  8 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-06-10  9:29 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-ppc, Paolo Bonzini, qemu-arm, Glenn Miles,
	Marc-André Lureau, Peter Maydell, Stefan Hajnoczi,
	Stefan Berger, Daniel P . Berrangé

On 5/6/25 21:35, Philippe Mathieu-Daudé wrote:
> Eradicate alloca() uses on system code, then enable
> -Walloca to prevent new ones to creep back in.
> 
> Philippe Mathieu-Daudé (4):
>    hw/gpio/pca9552: Avoid using g_newa()
>    backends/tpmL Avoid using g_alloca()
>    tests/unit/test-char: Avoid using g_alloca()

Patches 1-3 queued.


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

end of thread, other threads:[~2025-06-10  9:31 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-05 19:35 [PATCH 0/4] system: Forbid alloca() Philippe Mathieu-Daudé
2025-06-05 19:35 ` [PATCH 1/4] hw/gpio/pca9552: Avoid using g_newa() Philippe Mathieu-Daudé
2025-06-05 21:03   ` Miles Glenn
2025-06-05 19:35 ` [PATCH 2/4] backends/tpmL Avoid using g_alloca() Philippe Mathieu-Daudé
2025-06-05 21:23   ` BALATON Zoltan
2025-06-06  7:00     ` Philippe Mathieu-Daudé
2025-06-06  8:14   ` Thomas Huth
2025-06-08 19:09     ` Stefan Berger
2025-06-08 19:07   ` Stefan Berger
2025-06-05 19:35 ` [PATCH 3/4] tests/unit/test-char: " Philippe Mathieu-Daudé
2025-06-05 20:53   ` Pierrick Bouvier
2025-06-06  6:09     ` Philippe Mathieu-Daudé
2025-06-06 16:18       ` Pierrick Bouvier
2025-06-05 19:35 ` [RFC PATCH 4/4] buildsys: Prohibit alloca() use on system code Philippe Mathieu-Daudé
2025-06-05 20:53 ` [PATCH 0/4] system: Forbid alloca() Pierrick Bouvier
2025-06-06  8:37 ` Peter Maydell
2025-06-06  8:53   ` Philippe Mathieu-Daudé
2025-06-06  8:44 ` Alex Bennée
2025-06-09 13:46 ` Stefan Hajnoczi
2025-06-10  9:29 ` Philippe Mathieu-Daudé

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