[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

[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

[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

[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

[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

[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 4/9] bsd-user/syscall: Replace alloca() by g_new()

[Warner Losh]

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

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?

Warner

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

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

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 0/9] misc: Replace alloca() by g_malloc()

[Warner Losh]

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

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.

Warner



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

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

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

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

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

[Warner Losh]

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

On Thu, May 6, 2021 at 8:21 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> 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


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.


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

Warner

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

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

[Warner Losh]

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

On Thu, May 6, 2021 at 8:26 AM Eric Blake <eblake@redhat.com> wrote:

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

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.

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.

Warner

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

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 0/9] misc: Replace alloca() by g_malloc()

[Warner Losh]

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

On Thu, May 6, 2021 at 8:28 AM Eric Blake <eblake@redhat.com> wrote:

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

Yea, I had missed that. Too many years of reviewing patches that
came in for other projects that didn't use that feature. Outside of the
bsd-user context, I'd agree with you that things are safer this way.
But emulating system calls has other considerations, that I've gone
into in another thread.

So please accept my apology for not noticing the g_autofree
in these. It definitely makes what I said here incorrect and
it should be ignored...

Warner

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

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)

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

[Warner Losh]

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

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

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

I'm having trouble understanding how the emulated system calls
can still be async safe if that's done. I might be missing something
in the CPU emulation that would clear up the concern, though. How
threads interact with each-other in emulation is an area I'm still
learning.

Warner

P.S. Of course, no matter what you do, the current in-tree
bsd-user dumps core right away, so it's hard to break it worse
than it already is broken :)...

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

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

[Warner Losh]

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

On Thu, May 6, 2021 at 9:12 AM Eric Blake <eblake@redhat.com> wrote:

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

I'd forgotten that the gap pages are only a single page and things
may be mapped beyond that. For bsd-user, though, any issues that
this cause will be mitigated by the fact that it's running as the user
that we're emulating, so there's no privilege escalations that couldn't
already be hand-coded w/o bsd-user in the loop.


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

My concern is that the user emulation has some elements that are
based on catching signals and interpreting them to control execution
in the guest code. There's a number of places where signals are queued
instead of run right away as well. I've not had time to dive into the
complex implementation to make sure that we'll not wind up in a situation
where we would be trying to call malloc from a signal handler directly.
I'll try to find an answer to that as well.


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

Yea, my faith in Coverity flagging these when there's trips between,
say, arm binary code and user code isn't quite as high as yours.

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.

Warner

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

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

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

[Warner Losh]

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

On Thu, May 6, 2021 at 9:59 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

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

Gotcha. That's the context I was trying to chase down via other
vectors.


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

OK. Then that makes sense to me now. I'll look through the
bsd-user stuff in our branch...


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

Those have not yet been moved over entirely. And it was looking
through those patches that gave me a health respect (fear?) of
issues with mixing of host/guest signals...

Warner

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

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

[Warner Losh]

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

On Thu, May 6, 2021 at 10:04 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

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

I'll convert the ones in our fork to use that pattern prior to
upstreaming.

Warner

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

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

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

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