[PATCH v2 0/9] misc: Replace alloca() by g_malloc()

[PATCH v2 0/9] misc: Replace alloca() by g_malloc()

[Philippe Mathieu-Daudé]

The ALLOCA(3) man-page mentions its "use is discouraged".
Replace few calls by equivalent GLib malloc().

Last call site is linux-user/.

Since v1:
- Converted more uses (alsaaudio, tpm, pca9552)
- Reworked gdbstub (Alex)
- Simplified PPC/KVM (Greg)

Philippe Mathieu-Daudé (9):
  audio/alsaaudio: Replace ALSA alloca() by malloc() equivalent
  backends/tpm: Replace qemu_mutex_lock calls with QEMU_LOCK_GUARD
  backends/tpm: Replace g_alloca() by g_malloc()
  bsd-user/syscall: Replace alloca() by g_new()
  gdbstub: Constify GdbCmdParseEntry
  gdbstub: Only call cmd_parse_params() with non-NULL command schema
  gdbstub: Replace alloca() + memset(0) by g_new0()
  hw/misc/pca9552: Replace g_newa() by g_new()
  target/ppc/kvm: Replace alloca() by g_malloc()

 audio/alsaaudio.c           | 11 +++++++----
 backends/tpm/tpm_emulator.c | 35 +++++++++++++++--------------------
 bsd-user/syscall.c          |  3 +--
 gdbstub.c                   | 34 +++++++++++++++-------------------
 hw/misc/pca9552.c           |  2 +-
 target/ppc/kvm.c            |  3 +--
 6 files changed, 40 insertions(+), 48 deletions(-)

-- 
2.26.3

[PATCH v2 1/9] audio/alsaaudio: Replace ALSA alloca() by malloc() equivalent

[Philippe Mathieu-Daudé]

The ALLOCA(3) man-page mentions its "use is discouraged".

Define the cleanup functions for the snd_pcm_[hw/sw]_params_t types,
and replace the ALSA alloca() calls by equivalent ALSA malloc().

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

diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
index fcc2f62864f..f39061ebc42 100644
--- a/audio/alsaaudio.c
+++ b/audio/alsaaudio.c
@@ -70,6 +70,9 @@ struct alsa_params_obt {
     snd_pcm_uframes_t samples;
 };
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(snd_pcm_hw_params_t, snd_pcm_hw_params_free)
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(snd_pcm_sw_params_t, snd_pcm_sw_params_free)
+
 static void GCC_FMT_ATTR (2, 3) alsa_logerr (int err, const char *fmt, ...)
 {
     va_list ap;
@@ -410,9 +413,9 @@ static void alsa_dump_info (struct alsa_params_req *req,
 static void alsa_set_threshold (snd_pcm_t *handle, snd_pcm_uframes_t threshold)
 {
     int err;
-    snd_pcm_sw_params_t *sw_params;
+    g_autoptr(snd_pcm_sw_params_t) sw_params = NULL;
 
-    snd_pcm_sw_params_alloca (&sw_params);
+    snd_pcm_sw_params_malloc(&sw_params);
 
     err = snd_pcm_sw_params_current (handle, sw_params);
     if (err < 0) {
@@ -444,7 +447,7 @@ static int alsa_open(bool in, struct alsa_params_req *req,
     AudiodevAlsaOptions *aopts = &dev->u.alsa;
     AudiodevAlsaPerDirectionOptions *apdo = in ? aopts->in : aopts->out;
     snd_pcm_t *handle;
-    snd_pcm_hw_params_t *hw_params;
+    g_autoptr(snd_pcm_hw_params_t) hw_params = NULL;
     int err;
     unsigned int freq, nchannels;
     const char *pcm_name = apdo->has_dev ? apdo->dev : "default";
@@ -455,7 +458,7 @@ static int alsa_open(bool in, struct alsa_params_req *req,
     freq = req->freq;
     nchannels = req->nchannels;
 
-    snd_pcm_hw_params_alloca (&hw_params);
+    snd_pcm_hw_params_malloc(&hw_params);
 
     err = snd_pcm_open (
         &handle,
-- 
2.26.3

[PATCH v2 2/9] backends/tpm: Replace qemu_mutex_lock calls with QEMU_LOCK_GUARD

[Philippe Mathieu-Daudé]

Simplify the tpm_emulator_ctrlcmd() handler by replacing a pair of
qemu_mutex_lock/qemu_mutex_unlock calls by the WITH_QEMU_LOCK_GUARD
macro.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 backends/tpm/tpm_emulator.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index a012adc1934..e5f1063ab6c 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -30,6 +30,7 @@
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "qemu/sockets.h"
+#include "qemu/lockable.h"
 #include "io/channel-socket.h"
 #include "sysemu/tpm_backend.h"
 #include "sysemu/tpm_util.h"
@@ -124,31 +125,26 @@ static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, unsigned long cmd, void *msg,
     uint32_t cmd_no = cpu_to_be32(cmd);
     ssize_t n = sizeof(uint32_t) + msg_len_in;
     uint8_t *buf = NULL;
-    int ret = -1;
 
-    qemu_mutex_lock(&tpm->mutex);
+    WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
+        buf = g_alloca(n);
+        memcpy(buf, &cmd_no, sizeof(cmd_no));
+        memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
 
-    buf = g_alloca(n);
-    memcpy(buf, &cmd_no, sizeof(cmd_no));
-    memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
-
-    n = qemu_chr_fe_write_all(dev, buf, n);
-    if (n <= 0) {
-        goto end;
-    }
-
-    if (msg_len_out != 0) {
-        n = qemu_chr_fe_read_all(dev, msg, msg_len_out);
+        n = qemu_chr_fe_write_all(dev, buf, n);
         if (n <= 0) {
-            goto end;
+            return -1;
+        }
+
+        if (msg_len_out != 0) {
+            n = qemu_chr_fe_read_all(dev, msg, msg_len_out);
+            if (n <= 0) {
+                return -1;
+            }
         }
     }
 
-    ret = 0;
-
-end:
-    qemu_mutex_unlock(&tpm->mutex);
-    return ret;
+    return 0;
 }
 
 static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_emu,
-- 
2.26.3

Re: [PATCH v2 2/9] backends/tpm: Replace qemu_mutex_lock calls with QEMU_LOCK_GUARD

[Stefan Berger]

On 5/6/21 9:37 AM, Philippe Mathieu-Daudé wrote:
> Simplify the tpm_emulator_ctrlcmd() handler by replacing a pair of
> qemu_mutex_lock/qemu_mutex_unlock calls by the WITH_QEMU_LOCK_GUARD
> macro.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

Re: [PATCH v2 2/9] backends/tpm: Replace qemu_mutex_lock calls with QEMU_LOCK_GUARD

[Christophe de Dinechin]

On 2021-05-06 at 15:37 CEST, Philippe Mathieu-Daudé wrote...
> Simplify the tpm_emulator_ctrlcmd() handler by replacing a pair of
> qemu_mutex_lock/qemu_mutex_unlock calls by the WITH_QEMU_LOCK_GUARD
> macro.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  backends/tpm/tpm_emulator.c | 34 +++++++++++++++-------------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index a012adc1934..e5f1063ab6c 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -30,6 +30,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/module.h"
>  #include "qemu/sockets.h"
> +#include "qemu/lockable.h"
>  #include "io/channel-socket.h"
>  #include "sysemu/tpm_backend.h"
>  #include "sysemu/tpm_util.h"
> @@ -124,31 +125,26 @@ static int tpm_emulator_ctrlcmd(TPMEmulator *tpm, unsigned long cmd, void *msg,
>      uint32_t cmd_no = cpu_to_be32(cmd);
>      ssize_t n = sizeof(uint32_t) + msg_len_in;
>      uint8_t *buf = NULL;
> -    int ret = -1;
>
> -    qemu_mutex_lock(&tpm->mutex);
> +    WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
> +        buf = g_alloca(n);
> +        memcpy(buf, &cmd_no, sizeof(cmd_no));
> +        memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
>
> -    buf = g_alloca(n);
> -    memcpy(buf, &cmd_no, sizeof(cmd_no));
> -    memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
> -
> -    n = qemu_chr_fe_write_all(dev, buf, n);
> -    if (n <= 0) {
> -        goto end;
> -    }
> -
> -    if (msg_len_out != 0) {
> -        n = qemu_chr_fe_read_all(dev, msg, msg_len_out);
> +        n = qemu_chr_fe_write_all(dev, buf, n);
>          if (n <= 0) {
> -            goto end;
> +            return -1;
> +        }
> +
> +        if (msg_len_out != 0) {
> +            n = qemu_chr_fe_read_all(dev, msg, msg_len_out);
> +            if (n <= 0) {
> +                return -1;
> +            }
>          }
>      }
>
> -    ret = 0;
> -
> -end:
> -    qemu_mutex_unlock(&tpm->mutex);
> -    return ret;
> +    return 0;
>  }
>
>  static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_emu,

I really like the improvement, but it looks like it does not belong to the
top-level series (i.e. not related to replacing alloca() by g_malloc()).

Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>

--
Cheers,
Christophe de Dinechin (IRC c3d)

[PATCH v2 3/9] backends/tpm: Replace g_alloca() by g_malloc()

[Philippe Mathieu-Daudé]

The ALLOCA(3) man-page mentions its "use is discouraged".

Replace a g_alloca() call by a g_malloc() one, moving the
allocation before the MUTEX guarded block.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 backends/tpm/tpm_emulator.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index e5f1063ab6c..d37a6d563a3 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -124,10 +124,9 @@ 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;
+    g_autofree uint8_t *buf = g_malloc(n);
 
     WITH_QEMU_LOCK_GUARD(&tpm->mutex) {
-        buf = g_alloca(n);
         memcpy(buf, &cmd_no, sizeof(cmd_no));
         memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
 
-- 
2.26.3

Re: [PATCH v2 3/9] backends/tpm: Replace g_alloca() by g_malloc()

[Stefan Berger]

On 5/6/21 9:37 AM, Philippe Mathieu-Daudé wrote:
> The ALLOCA(3) man-page mentions its "use is discouraged".
>
> Replace a g_alloca() call by a g_malloc() one, moving the
> allocation before the MUTEX guarded block.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

[PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()

[Philippe Mathieu-Daudé]

The ALLOCA(3) man-page mentions its "use is discouraged".

Replace it by a g_new() call.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 bsd-user/syscall.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index 4abff796c76..dbee0385ceb 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
     case TARGET_FREEBSD_NR_writev:
         {
             int count = arg3;
-            struct iovec *vec;
+            g_autofree struct iovec *vec = g_new(struct iovec, count);
 
-            vec = alloca(count * sizeof(struct iovec));
             if (lock_iovec(VERIFY_READ, vec, arg2, count, 1) < 0)
                 goto efault;
             ret = get_errno(writev(arg1, vec, count));
-- 
2.26.3

Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()

[Peter Maydell]

On Thu, 6 May 2021 at 15:17, Warner Losh <imp@bsdimp.com> wrote:
>
>
>
> On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> The ALLOCA(3) man-page mentions its "use is discouraged".
>>
>> Replace it by a g_new() call.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  bsd-user/syscall.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
>> index 4abff796c76..dbee0385ceb 100644
>> --- a/bsd-user/syscall.c
>> +++ b/bsd-user/syscall.c
>> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
>>      case TARGET_FREEBSD_NR_writev:
>>          {
>>              int count = arg3;
>> -            struct iovec *vec;
>> +            g_autofree struct iovec *vec = g_new(struct iovec, count);
>
>
> Where is this freed?

g_autofree, so it gets freed when it goes out of scope.
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree

> Also, alloca just moves a stack pointer, where malloc has complex interactions. Are you sure that's a safe change here?

alloca()ing something with size determined by the guest is
definitely not safe :-) malloc as part of "handle this syscall"
is pretty common, at least in linux-user.

thanks
-- PMM

Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()

[Philippe Mathieu-Daudé]

On 5/6/21 4:48 PM, Warner Losh wrote:
> 
> 
> On Thu, May 6, 2021 at 8:21 AM Peter Maydell <peter.maydell@linaro.org
> <mailto:peter.maydell@linaro.org>> wrote:
> 
>     On Thu, 6 May 2021 at 15:17, Warner Losh <imp@bsdimp.com
>     <mailto:imp@bsdimp.com>> wrote:
>     >
>     >
>     >
>     > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé
>     <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
>     >>
>     >> The ALLOCA(3) man-page mentions its "use is discouraged".
>     >>
>     >> Replace it by a g_new() call.
>     >>
>     >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     >> ---
>     >>  bsd-user/syscall.c | 3 +--
>     >>  1 file changed, 1 insertion(+), 2 deletions(-)
>     >>
>     >> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
>     >> index 4abff796c76..dbee0385ceb 100644
>     >> --- a/bsd-user/syscall.c
>     >> +++ b/bsd-user/syscall.c
>     >> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env,
>     int num, abi_long arg1,
>     >>      case TARGET_FREEBSD_NR_writev:
>     >>          {
>     >>              int count = arg3;
>     >> -            struct iovec *vec;
>     >> +            g_autofree struct iovec *vec = g_new(struct iovec,
>     count);
>     >
>     >
>     > Where is this freed?
> 
>     g_autofree, so it gets freed when it goes out of scope.
>     https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree
>     <https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree>
> 
> 
> Ah. I'd missed that feature and annotation...  Too many years seeing
> patches like
> this in other projects where a feature like this wasn't there to save
> the day...
> 
> This means you can ignore my other message as equally misinformed.

This also shows my commit description is poor.

>     > Also, alloca just moves a stack pointer, where malloc has complex
>     interactions. Are you sure that's a safe change here?
> 
>     alloca()ing something with size determined by the guest is
>     definitely not safe :-) malloc as part of "handle this syscall"
>     is pretty common, at least in linux-user.
> 
> 
> Well, since this is userland emulation, the normal process boundaries
> will fix that. allocating from
> the heap is little different..., so while unsafe, it's the domain of
> $SOMEONE_ELSE to enforce
> the safety. linux-user has many similar usages for both malloc and
> alloca, and it's ok for the
> same reason.
> 
> Doing a quick grep on my blitz[*] branch in the bsd-user fork shows that
> alloca is used almost
> exclusively there. There's 24 calls to alloca in the bsd-user code.
> There's almost no malloc
> calls (7) in that at all outside the image loader: all but one of them
> are confined to sysctl
> emulation with the last one used to keep track of thread state in new
> threads...
> linux-user has a similar profile (20 alloca's and 14 mallocs outside the
> loader),
> but with more mallocs in other paths than just sysctl (which isn't present).
> 
> I had no plans on migrating to anything else...

I considered excluding user emulation (both Linux/BSD) by enabling
CFLAGS=-Walloca for everything except user-mode, but Meson doesn't
support adding flags to a specific source set:
https://github.com/mesonbuild/meson/issues/1367#issuecomment-277929767

  Q: Is it possible to add a flag to a specific file? I have some
     generated code that's freaking the compiler out and I don't
     feel like mucking with the generator.

  A: We don't support per-file compiler flags by design. It interacts
     very poorly with other parts, especially precompiled headers.
     The real solution is to fix the generator to not produce garbage.
     Obviously this is often not possible so the solution to that is,
     as mentioned above, build a static library with the specific
     compiler args. This has the added benefit that it makes this
     workaround explicit and visible rather than hiding things in
     random locations.

Then Paolo suggested to convert all alloca() calls instead.

Regards,

Phil.

Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()

[Eric Blake]

On 5/6/21 9:16 AM, Warner Losh wrote:
> On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé <philmd@redhat.com>
> wrote:
> 
>> The ALLOCA(3) man-page mentions its "use is discouraged".
>>
>> Replace it by a g_new() call.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  bsd-user/syscall.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
>> index 4abff796c76..dbee0385ceb 100644
>> --- a/bsd-user/syscall.c
>> +++ b/bsd-user/syscall.c
>> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num,
>> abi_long arg1,
>>      case TARGET_FREEBSD_NR_writev:
>>          {
>>              int count = arg3;
>> -            struct iovec *vec;
>> +            g_autofree struct iovec *vec = g_new(struct iovec, count);
>>
> 
> Where is this freed? Also, alloca just moves a stack pointer, where malloc
> has complex interactions. Are you sure that's a safe change here?

It's freed any time the g_autofree variable goes out of scope (that's
what the g_autofree macro is for).  Yes, the change is safe, although
you are right that switching to malloc is going to be a bit more
heavyweight than what alloca used.  What's more, it adds safety: if
count was under user control, a user could pass a value that could cause
alloca to allocate more than 4k and accidentally mess up stack guard
pages, while malloc() uses the heap and therefore cannot cause stack bugs.

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

Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()

[Eric Blake]

On 5/6/21 9:55 AM, Warner Losh wrote:

>>> Where is this freed? Also, alloca just moves a stack pointer, where
>> malloc
>>> has complex interactions. Are you sure that's a safe change here?
>>
>> It's freed any time the g_autofree variable goes out of scope (that's
>> what the g_autofree macro is for).  Yes, the change is safe, although
>> you are right that switching to malloc is going to be a bit more
>> heavyweight than what alloca used.  What's more, it adds safety: if
>> count was under user control, a user could pass a value that could cause
>> alloca to allocate more than 4k and accidentally mess up stack guard
>> pages, while malloc() uses the heap and therefore cannot cause stack bugs.
>>
> 
> I'm not sure I understand that argument, since we're not compiling bsd-user
> with the stack-smash-protection stuff enabled, so there's no guard pages
> involved. The stack can grow quite large and the unmapped page at
> the end of the stack would catch any overflows. Since these allocations
> are on the top of the stack, they won't overflow into any other frames
> and subsequent calls are made with them already in place.

With alloca() on user-controlled size, the user can set up the size to
be larger than the unmapped guard page, at which point you CANNOT catch
the stack overflow because the alloca can skip the guard page and wrap
into other valid memory.  Compiling with stack-smash-protection stuff
enabled will catch such a bad alloca(); but the issue at hand here is
not when stack-smash-protection is enabled, but the more common case
when it is disabled (at which point the only protection you have is the
guard page, but improper use of alloca() can bypass the guard page).
Not all alloca() arguments are under user control, but it is easier as a
matter of policy to blindly avoid alloca() than it is to audit which
calls have safe sizes and therefore will not risk user control bypassing
stack guards.

> 
> malloc, on the other hand, involves taking out a number of mutexes
> and similar things to obtain the memory, which may not necessarily
> be safe in all the contexts system calls can be called from. System
> calls are, typically, async safe and can be called from signal handlers.
> alloca() is async safe, while malloc() is not. So changing the calls
> from alloca to malloc makes calls to system calls in signal handlers
> unsafe and potentially introducing buggy behavior as a result.

Correct, use of malloc() is not safe within signal handlers. But these
calls are not within signal handlers - or am _I_ missing something?  Is
the point of *-user code to emulate syscalls that are reachable from
code installed in a signal handler, at which point introducing an
async-unsafe call to malloc in our emulation is indeed putting the
application at risk of a malloc deadlock?

Ultimately, we're trading one maintenance headache (determining which
alloca() size calls might be under user control) for another
(determining that malloca() calls are not in a signal context), but the
latter is far more common such that we can use existing tooling to make
that conversion safely (both in the fact that the compiler has flags to
warn about alloca() usage, and in the fact that Coverity is good at
flagging improper uses of malloc() such as within a function reachable
from something installed in a signal handler).  But I'm not familiar
enough with the bsd/linux-user code to know if your point about having
to use only async-safe functionalities is a valid limitation on our
emulation.

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

Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()

[Eric Blake]

On 5/6/21 10:30 AM, Warner Losh wrote:

> 
> But for the real answer, I need to contact the original authors of
> this part of the code (they are no longer involved day-to-day in
> the bsd-user efforts) to see if this scenario is possible or not. If
> it's easy to find out that way, we can either know this is safe to
> do, or if effort is needed to make it safe. At present, I've seen
> enough and chatted enough with others to be concerned that
> the change would break proper emulation.

Do we have a feel for the maximum amount of memory being used by the
various alloca() replaced in this series?  If so, can we just
stack-allocate an array of bytes of the maximum size needed?  Then we
avoid alloca() but also avoid the dynamic memory management that
malloc() would introduce.  Basically, it boils down to auditing why the
alloca() is safe, and once we know that, replacing the variable-sized
precise alloca() with its counterpart statically-sized array allocation,
at the expense of some wasted stack space when the runtime size does not
use the full compile-time maximum size.

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

Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()

[Peter Maydell]

On Thu, 6 May 2021 at 16:46, Eric Blake <eblake@redhat.com> wrote:
>
> On 5/6/21 10:30 AM, Warner Losh wrote:
>
> >
> > But for the real answer, I need to contact the original authors of
> > this part of the code (they are no longer involved day-to-day in
> > the bsd-user efforts) to see if this scenario is possible or not. If
> > it's easy to find out that way, we can either know this is safe to
> > do, or if effort is needed to make it safe. At present, I've seen
> > enough and chatted enough with others to be concerned that
> > the change would break proper emulation.
>
> Do we have a feel for the maximum amount of memory being used by the
> various alloca() replaced in this series?  If so, can we just
> stack-allocate an array of bytes of the maximum size needed?

In *-user the allocas are generally of the form "guest passed
us a random number, allocate that many structs/whatevers". (In this
specific bsd-user example it's the writev syscall and it's "however
many struct iovecs the guest passed".) So there is no upper limit.

The right thing to do here is probably to use g_try_malloc() and return
ENOMEM or whatever on failure. The use of alloca, at least in the
linux-user code, is purely old lazy coding based on "in practice
real world guest binaries don't allocate very many of these so
we can get away with shoving them on the stack".

thanks
-- PMM

Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new()

[Peter Maydell]

On Thu, 6 May 2021 at 15:57, Warner Losh <imp@bsdimp.com> wrote:
> malloc, on the other hand, involves taking out a number of mutexes
> and similar things to obtain the memory, which may not necessarily
> be safe in all the contexts system calls can be called from. System
> calls are, typically, async safe and can be called from signal handlers.
> alloca() is async safe, while malloc() is not. So changing the calls
> from alloca to malloc makes calls to system calls in signal handlers
> unsafe and potentially introducing buggy behavior as a result.

malloc() should definitely be fine in this context. The syscall
emulation is called after the cpu_loop() in bsd-user has called
cpu_exec(). cpu_exec() calls into the JIT, which will malloc
all over the place if it needs to in the course of JITting things.

This code should never be being called from a (host) signal handler.
In upstream the signal handling code for bsd-user appears to
be missing entirely. For linux-user when we take a host signal
we merely arrange for the guest to take a guest signal, we
don't try to execute guest code directly from the host handler.

(There are some pretty hairy races in emulated signal handling:
we did a big overhaul of the linux-user code in that area a
while back. If your bsd-user code doesn't have the 'safe_syscall'
stuff it probably needs it. But that's more about races between
"guest code wants to execute a syscall" and "incoming signal"
where we could fail to exit EINTR from an emulated syscall if
the signal arrives in a bad window.)

thanks
-- PMM

[PATCH v2 5/9] gdbstub: Constify GdbCmdParseEntry

[Philippe Mathieu-Daudé]

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 9103ffc9028..83d47c67325 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1981,7 +1981,7 @@ static void handle_v_kill(GdbCmdContext *gdb_ctx, void *user_ctx)
     exit(0);
 }
 
-static GdbCmdParseEntry gdb_v_commands_table[] = {
+static const GdbCmdParseEntry gdb_v_commands_table[] = {
     /* Order is important if has same prefix */
     {
         .handler = handle_v_cont_query,
@@ -2324,7 +2324,7 @@ static void handle_set_qemu_phy_mem_mode(GdbCmdContext *gdb_ctx, void *user_ctx)
 }
 #endif
 
-static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
+static const GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
     /* Order is important if has same prefix */
     {
         .handler = handle_query_qemu_sstepbits,
@@ -2342,7 +2342,7 @@ static GdbCmdParseEntry gdb_gen_query_set_common_table[] = {
     },
 };
 
-static GdbCmdParseEntry gdb_gen_query_table[] = {
+static const GdbCmdParseEntry gdb_gen_query_table[] = {
     {
         .handler = handle_query_curr_tid,
         .cmd = "C",
@@ -2420,7 +2420,7 @@ static GdbCmdParseEntry gdb_gen_query_table[] = {
 #endif
 };
 
-static GdbCmdParseEntry gdb_gen_set_table[] = {
+static const GdbCmdParseEntry gdb_gen_set_table[] = {
     /* Order is important if has same prefix */
     {
         .handler = handle_set_qemu_sstep,
-- 
2.26.3

[PATCH v2 6/9] gdbstub: Only call cmd_parse_params() with non-NULL command schema

[Philippe Mathieu-Daudé]

Move the NULL check on command schema buffer from the callee
cmd_parse_params() to the single caller, process_string_cmd().

This simplifies the process_string_cmd() logic.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 83d47c67325..7cee2fb0f1f 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1368,12 +1368,9 @@ static int cmd_parse_params(const char *data, const char *schema,
     int curr_param;
     const char *curr_schema, *curr_data;
 
+    assert(schema);
     *num_params = 0;
 
-    if (!schema) {
-        return 0;
-    }
-
     curr_schema = schema;
     curr_param = 0;
     curr_data = data;
@@ -1471,7 +1468,7 @@ static inline int startswith(const char *string, const char *pattern)
 static int process_string_cmd(void *user_ctx, const char *data,
                               const GdbCmdParseEntry *cmds, int num_cmds)
 {
-    int i, schema_len, max_num_params = 0;
+    int i;
     GdbCmdContext gdb_ctx;
 
     if (!cmds) {
@@ -1488,21 +1485,21 @@ static int process_string_cmd(void *user_ctx, const char *data,
         }
 
         if (cmd->schema) {
-            schema_len = strlen(cmd->schema);
+            int schema_len = strlen(cmd->schema);
+            int max_num_params = schema_len / 2;
+
             if (schema_len % 2) {
                 return -2;
             }
 
-            max_num_params = schema_len / 2;
-        }
+            gdb_ctx.params = (GdbCmdVariant *)alloca(sizeof(*gdb_ctx.params)
+                                                     * max_num_params);
+            memset(gdb_ctx.params, 0, sizeof(*gdb_ctx.params) * max_num_params);
 
-        gdb_ctx.params =
-            (GdbCmdVariant *)alloca(sizeof(*gdb_ctx.params) * max_num_params);
-        memset(gdb_ctx.params, 0, sizeof(*gdb_ctx.params) * max_num_params);
-
-        if (cmd_parse_params(&data[strlen(cmd->cmd)], cmd->schema,
-                             gdb_ctx.params, &gdb_ctx.num_params)) {
-            return -1;
+            if (cmd_parse_params(&data[strlen(cmd->cmd)], cmd->schema,
+                                 gdb_ctx.params, &gdb_ctx.num_params)) {
+                return -1;
+            }
         }
 
         cmd->handler(&gdb_ctx, user_ctx);
-- 
2.26.3

Re: [PATCH v2 6/9] gdbstub: Only call cmd_parse_params() with non-NULL command schema

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Move the NULL check on command schema buffer from the callee
> cmd_parse_params() to the single caller, process_string_cmd().
>
> This simplifies the process_string_cmd() logic.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH v2 7/9] gdbstub: Replace alloca() + memset(0) by g_new0()

[Philippe Mathieu-Daudé]

The ALLOCA(3) man-page mentions its "use is discouraged".

Replace the alloca() and memset(0) calls by g_new0().

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

diff --git a/gdbstub.c b/gdbstub.c
index 7cee2fb0f1f..666053bf590 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1487,14 +1487,13 @@ static int process_string_cmd(void *user_ctx, const char *data,
         if (cmd->schema) {
             int schema_len = strlen(cmd->schema);
             int max_num_params = schema_len / 2;
+            g_autofree GdbCmdVariant *params = NULL;
 
             if (schema_len % 2) {
                 return -2;
             }
 
-            gdb_ctx.params = (GdbCmdVariant *)alloca(sizeof(*gdb_ctx.params)
-                                                     * max_num_params);
-            memset(gdb_ctx.params, 0, sizeof(*gdb_ctx.params) * max_num_params);
+            gdb_ctx.params = params = g_new0(GdbCmdVariant, max_num_params);
 
             if (cmd_parse_params(&data[strlen(cmd->cmd)], cmd->schema,
                                  gdb_ctx.params, &gdb_ctx.num_params)) {
-- 
2.26.3

Re: [PATCH v2 7/9] gdbstub: Replace alloca() + memset(0) by g_new0()

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> The ALLOCA(3) man-page mentions its "use is discouraged".
>
> Replace the alloca() and memset(0) calls by g_new0().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Please see:

  Subject: [ALT PATCH] gdbstub: Replace GdbCmdContext with plain g_array()
  Date: Thu,  6 May 2021 17:07:41 +0100
  Message-Id: <20210506160741.9841-1-alex.bennee@linaro.org>

which also includes elements of 6/9 which can be kept split off.

> ---
>  gdbstub.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 7cee2fb0f1f..666053bf590 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1487,14 +1487,13 @@ static int process_string_cmd(void *user_ctx, const char *data,
>          if (cmd->schema) {
>              int schema_len = strlen(cmd->schema);
>              int max_num_params = schema_len / 2;
> +            g_autofree GdbCmdVariant *params = NULL;
>  
>              if (schema_len % 2) {
>                  return -2;
>              }
>  
> -            gdb_ctx.params = (GdbCmdVariant *)alloca(sizeof(*gdb_ctx.params)
> -                                                     * max_num_params);
> -            memset(gdb_ctx.params, 0, sizeof(*gdb_ctx.params) * max_num_params);
> +            gdb_ctx.params = params = g_new0(GdbCmdVariant, max_num_params);
>  
>              if (cmd_parse_params(&data[strlen(cmd->cmd)], cmd->schema,
>                                   gdb_ctx.params, &gdb_ctx.num_params)) {


-- 
Alex Bennée

[PATCH v2 8/9] hw/misc/pca9552: Replace g_newa() by g_new()

[Philippe Mathieu-Daudé]

The ALLOCA(3) man-page mentions its "use is discouraged".

Replace the g_newa() call by g_new().

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

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index b7686e27d7f..facf103cbfb 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -71,7 +71,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);
+        g_autofree char *buf = g_new(char, k->pin_count + 1);
 
         for (i = 0; i < k->pin_count; i++) {
             if (extract32(pins_status, i, 1)) {
-- 
2.26.3

[PATCH v2 9/9] target/ppc/kvm: Replace alloca() by g_malloc()

[Philippe Mathieu-Daudé]

The ALLOCA(3) man-page mentions its "use is discouraged".

Replace it by a g_malloc() call.

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

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 104a308abb5..63c458e2211 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2698,11 +2698,10 @@ int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
 int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
                            uint16_t n_valid, uint16_t n_invalid, Error **errp)
 {
-    struct kvm_get_htab_header *buf;
     size_t chunksize = sizeof(*buf) + n_valid * HASH_PTE_SIZE_64;
+    g_autofree struct kvm_get_htab_header *buf = g_malloc(chunksize);
     ssize_t rc;
 
-    buf = alloca(chunksize);
     buf->index = index;
     buf->n_valid = n_valid;
     buf->n_invalid = n_invalid;
-- 
2.26.3

Re: [PATCH v2 9/9] target/ppc/kvm: Replace alloca() by g_malloc()

[Greg Kurz]

On Thu,  6 May 2021 15:37:58 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> The ALLOCA(3) man-page mentions its "use is discouraged".
> 
> Replace it by a g_malloc() call.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

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

>  target/ppc/kvm.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb5..63c458e2211 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2698,11 +2698,10 @@ int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
>  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>                             uint16_t n_valid, uint16_t n_invalid, Error **errp)
>  {
> -    struct kvm_get_htab_header *buf;
>      size_t chunksize = sizeof(*buf) + n_valid * HASH_PTE_SIZE_64;
> +    g_autofree struct kvm_get_htab_header *buf = g_malloc(chunksize);
>      ssize_t rc;
>  
> -    buf = alloca(chunksize);
>      buf->index = index;
>      buf->n_valid = n_valid;
>      buf->n_invalid = n_invalid;

Re: [PATCH v2 9/9] target/ppc/kvm: Replace alloca() by g_malloc()

[David Gibson]

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

On Thu, May 06, 2021 at 03:37:58PM +0200, Philippe Mathieu-Daudé wrote:
> The ALLOCA(3) man-page mentions its "use is discouraged".
> 
> Replace it by a g_malloc() call.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  target/ppc/kvm.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb5..63c458e2211 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2698,11 +2698,10 @@ int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
>  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>                             uint16_t n_valid, uint16_t n_invalid, Error **errp)
>  {
> -    struct kvm_get_htab_header *buf;
>      size_t chunksize = sizeof(*buf) + n_valid * HASH_PTE_SIZE_64;
> +    g_autofree struct kvm_get_htab_header *buf = g_malloc(chunksize);

Um.. that doesn't look like it would compile, since you use
sizeof(*buf) before declaring buf.

>      ssize_t rc;
>  
> -    buf = alloca(chunksize);
>      buf->index = index;
>      buf->n_valid = n_valid;
>      buf->n_invalid = n_invalid;

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

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

Re: [PATCH v2 0/9] misc: Replace alloca() by g_malloc()

[Eric Blake]

On 5/6/21 9:22 AM, Warner Losh wrote:
> On Thu, May 6, 2021 at 7:39 AM Philippe Mathieu-Daudé <philmd@redhat.com>
> wrote:
> 
>> The ALLOCA(3) man-page mentions its "use is discouraged".
>> Replace few calls by equivalent GLib malloc().
>>
> 
> Except g_alloc and g_malloc are not at all the same, and you can't drop in
> replace one with the other.
> 
> g_alloc allocates stack space on the calling frame that's automatically
> freed when the function returns.
> g_malloc allocates space from the heap, and calls to it must be matched
> with calls to g_free().
> 
> These patches don't do the latter, as far as I can tell, and so introduce
> memory leaks unless there's something I've missed.

You missed the g_autofree, whose job is to call g_free() on all points
in the control flow where the malloc()d memory goes out of scope
(equivalent in expressive power to alloca()d memory going out of scope).
 Although the code is arguably a bit slower (as heap manipulations are
not as cheap as stack manipulations), in the long run that speed penalty
is worth the safety factor (since stack manipulations under user control
are inherently unsafe).

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