[Qemu-devel] [PATCH for-2.9 v2] tests/bios-tables-test: Don't pass addresses of packed struct fields

[Qemu-devel] [PATCH for-2.9 v2] tests/bios-tables-test: Don't pass addresses of packed struct fields

[Peter Maydell]

Passing the address of a field in a packed struct to a function
that expects a pointer to normally aligned data will result in
a SEGBUS on architectures like SPARC that have strict alignment
requirements.

Pass addresses of local variables rather than addresses of packed
structure fields to glib functions like g_file_get_contents() to
avoid this bug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
v1->v2 changes: put the assignments after we check the error
status of the glib function, rather than before (makes no
practical difference since we will just assert out anyway,
but logically the right way round.)

 tests/bios-tables-test.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 88dbf97..a519921 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -261,8 +261,11 @@ static void dump_aml_files(test_data *data, bool rebuild)
             fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
                         S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
         } else {
-            fd = g_file_open_tmp("aml-XXXXXX", &sdt->aml_file, &error);
+            gchar *name;
+
+            fd = g_file_open_tmp("aml-XXXXXX", &name, &error);
             g_assert_no_error(error);
+            sdt->aml_file = name;
         }
         g_assert(fd >= 0);
 
@@ -291,9 +294,11 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
     gchar *out, *out_err;
     gboolean ret;
     int i;
+    gchar *name;
 
-    fd = g_file_open_tmp("asl-XXXXXX.dsl", &sdt->asl_file, &error);
+    fd = g_file_open_tmp("asl-XXXXXX.dsl", &name, &error);
     g_assert_no_error(error);
+    sdt->asl_file = name;
     close(fd);
 
     /* build command line */
@@ -314,10 +319,14 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
     ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
     g_assert_no_error(error);
     if (ret) {
-        ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
-                                  &sdt->asl_len, &error);
+        gchar *contents;
+        gsize len;
+
+        ret = g_file_get_contents(sdt->asl_file, &contents, &len, &error);
         g_assert(ret);
         g_assert_no_error(error);
+        sdt->asl = contents;
+        sdt->asl_len = len;
         ret = (sdt->asl_len > 0);
     }
 
@@ -371,6 +380,8 @@ static GArray *load_expected_aml(test_data *data)
         uint32_t signature;
         gchar *aml_file = NULL;
         const char *ext = data->variant ? data->variant : "";
+        gchar *aml_contents;
+        gsize aml_length;
 
         sdt = &g_array_index(data->tables, AcpiSdtTable, i);
 
@@ -397,12 +408,13 @@ try_again:
         if (getenv("V")) {
             fprintf(stderr, "\nUsing expected file '%s'\n", aml_file);
         }
-        ret = g_file_get_contents(aml_file, &exp_sdt.aml,
-                                  &exp_sdt.aml_len, &error);
+        ret = g_file_get_contents(aml_file, &aml_contents, &aml_length, &error);
         g_assert(ret);
         g_assert_no_error(error);
-        g_assert(exp_sdt.aml);
-        g_assert(exp_sdt.aml_len);
+        g_assert(aml_contents);
+        g_assert(aml_length);
+        exp_sdt.aml = aml_contents;
+        exp_sdt.aml_len = aml_length;
 
         g_array_append_val(exp_tables, exp_sdt);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9 v2] tests/bios-tables-test: Don't pass addresses of packed struct fields

[Eric Blake]

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

On 03/28/2017 04:40 AM, Peter Maydell wrote:
> Passing the address of a field in a packed struct to a function
> that expects a pointer to normally aligned data will result in
> a SEGBUS on architectures like SPARC that have strict alignment

s/SEGBUG/SIGBUS/

> requirements.
> 
> Pass addresses of local variables rather than addresses of packed
> structure fields to glib functions like g_file_get_contents() to
> avoid this bug.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 v2] tests/bios-tables-test: Don't pass addresses of packed struct fields

[Philippe Mathieu-Daudé]

Hi Peter,

On 03/28/2017 06:40 AM, Peter Maydell wrote:
> Passing the address of a field in a packed struct to a function
> that expects a pointer to normally aligned data will result in
> a SEGBUS on architectures like SPARC that have strict alignment
> requirements.
>
> Pass addresses of local variables rather than addresses of packed
> structure fields to glib functions like g_file_get_contents() to
> avoid this bug.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
> v1->v2 changes: put the assignments after we check the error
> status of the glib function, rather than before (makes no
> practical difference since we will just assert out anyway,
> but logically the right way round.)
>
>  tests/bios-tables-test.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 88dbf97..a519921 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -261,8 +261,11 @@ static void dump_aml_files(test_data *data, bool rebuild)
>              fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
>                          S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
>          } else {
> -            fd = g_file_open_tmp("aml-XXXXXX", &sdt->aml_file, &error);
> +            gchar *name;

should we add some comment like /* local variable used to avoid 
alignment issues */ in case later one find it clever to save few bytes 
on stack without reading git history?

> +
> +            fd = g_file_open_tmp("aml-XXXXXX", &name, &error);
>              g_assert_no_error(error);
> +            sdt->aml_file = name;
>          }
>          g_assert(fd >= 0);
>
> @@ -291,9 +294,11 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
>      gchar *out, *out_err;
>      gboolean ret;
>      int i;
> +    gchar *name;
>
> -    fd = g_file_open_tmp("asl-XXXXXX.dsl", &sdt->asl_file, &error);
> +    fd = g_file_open_tmp("asl-XXXXXX.dsl", &name, &error);
>      g_assert_no_error(error);
> +    sdt->asl_file = name;
>      close(fd);
>
>      /* build command line */
> @@ -314,10 +319,14 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
>      ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
>      g_assert_no_error(error);
>      if (ret) {
> -        ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
> -                                  &sdt->asl_len, &error);
> +        gchar *contents;
> +        gsize len;
> +
> +        ret = g_file_get_contents(sdt->asl_file, &contents, &len, &error);
>          g_assert(ret);
>          g_assert_no_error(error);
> +        sdt->asl = contents;
> +        sdt->asl_len = len;
>          ret = (sdt->asl_len > 0);
>      }
>
> @@ -371,6 +380,8 @@ static GArray *load_expected_aml(test_data *data)
>          uint32_t signature;
>          gchar *aml_file = NULL;
>          const char *ext = data->variant ? data->variant : "";
> +        gchar *aml_contents;
> +        gsize aml_length;
>
>          sdt = &g_array_index(data->tables, AcpiSdtTable, i);
>
> @@ -397,12 +408,13 @@ try_again:
>          if (getenv("V")) {
>              fprintf(stderr, "\nUsing expected file '%s'\n", aml_file);
>          }
> -        ret = g_file_get_contents(aml_file, &exp_sdt.aml,
> -                                  &exp_sdt.aml_len, &error);
> +        ret = g_file_get_contents(aml_file, &aml_contents, &aml_length, &error);
>          g_assert(ret);
>          g_assert_no_error(error);
> -        g_assert(exp_sdt.aml);
> -        g_assert(exp_sdt.aml_len);
> +        g_assert(aml_contents);
> +        g_assert(aml_length);
> +        exp_sdt.aml = aml_contents;
> +        exp_sdt.aml_len = aml_length;
>
>          g_array_append_val(exp_tables, exp_sdt);
>      }
>

Re: [Qemu-devel] [PATCH for-2.9 v2] tests/bios-tables-test: Don't pass addresses of packed struct fields

[Peter Maydell]

On 28 March 2017 at 14:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> should we add some comment like /* local variable used to avoid alignment
> issues */ in case later one find it clever to save few bytes on stack
> without reading git history?

Hmm. There's no obvious single place where that could be put,
and I tend to think that if you're going to use packed structures
then "caution needed" is implicit. When we have these bug fixes
in then the plan is to add sparc to the set of standard merge
build tests, which will catch accidental reversions of this
fix (and eventually clang 4 will be more widespread which will
warn about this.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.9 v2] tests/bios-tables-test: Don't pass addresses of packed struct fields

[Michael S. Tsirkin]

On Tue, Mar 28, 2017 at 10:40:17AM +0100, Peter Maydell wrote:
> Passing the address of a field in a packed struct to a function
> that expects a pointer to normally aligned data will result in
> a SEGBUS on architectures like SPARC that have strict alignment
> requirements.
> 
> Pass addresses of local variables rather than addresses of packed
> structure fields to glib functions like g_file_get_contents() to
> avoid this bug.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
> v1->v2 changes: put the assignments after we check the error
> status of the glib function, rather than before (makes no
> practical difference since we will just assert out anyway,
> but logically the right way round.)
> 
>  tests/bios-tables-test.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 88dbf97..a519921 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -261,8 +261,11 @@ static void dump_aml_files(test_data *data, bool rebuild)
>              fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
>                          S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
>          } else {
> -            fd = g_file_open_tmp("aml-XXXXXX", &sdt->aml_file, &error);
> +            gchar *name;
> +
> +            fd = g_file_open_tmp("aml-XXXXXX", &name, &error);
>              g_assert_no_error(error);
> +            sdt->aml_file = name;
>          }
>          g_assert(fd >= 0);
>  
> @@ -291,9 +294,11 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
>      gchar *out, *out_err;
>      gboolean ret;
>      int i;
> +    gchar *name;
>  
> -    fd = g_file_open_tmp("asl-XXXXXX.dsl", &sdt->asl_file, &error);
> +    fd = g_file_open_tmp("asl-XXXXXX.dsl", &name, &error);
>      g_assert_no_error(error);
> +    sdt->asl_file = name;
>      close(fd);
>  
>      /* build command line */
> @@ -314,10 +319,14 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
>      ret = g_spawn_command_line_sync(command_line->str, &out, &out_err, NULL, &error);
>      g_assert_no_error(error);
>      if (ret) {
> -        ret = g_file_get_contents(sdt->asl_file, (gchar **)&sdt->asl,
> -                                  &sdt->asl_len, &error);
> +        gchar *contents;
> +        gsize len;
> +
> +        ret = g_file_get_contents(sdt->asl_file, &contents, &len, &error);
>          g_assert(ret);
>          g_assert_no_error(error);
> +        sdt->asl = contents;
> +        sdt->asl_len = len;
>          ret = (sdt->asl_len > 0);
>      }
>  
> @@ -371,6 +380,8 @@ static GArray *load_expected_aml(test_data *data)
>          uint32_t signature;
>          gchar *aml_file = NULL;
>          const char *ext = data->variant ? data->variant : "";
> +        gchar *aml_contents;
> +        gsize aml_length;
>  
>          sdt = &g_array_index(data->tables, AcpiSdtTable, i);
>  
> @@ -397,12 +408,13 @@ try_again:
>          if (getenv("V")) {
>              fprintf(stderr, "\nUsing expected file '%s'\n", aml_file);
>          }
> -        ret = g_file_get_contents(aml_file, &exp_sdt.aml,
> -                                  &exp_sdt.aml_len, &error);
> +        ret = g_file_get_contents(aml_file, &aml_contents, &aml_length, &error);
>          g_assert(ret);
>          g_assert_no_error(error);
> -        g_assert(exp_sdt.aml);
> -        g_assert(exp_sdt.aml_len);
> +        g_assert(aml_contents);
> +        g_assert(aml_length);
> +        exp_sdt.aml = aml_contents;
> +        exp_sdt.aml_len = aml_length;
>  
>          g_array_append_val(exp_tables, exp_sdt);
>      }
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH for-2.9 v2] tests/bios-tables-test: Don't pass addresses of packed struct fields

[Michael S. Tsirkin]

On Thu, Mar 30, 2017 at 04:08:59PM +0100, Peter Maydell wrote:
> On 28 March 2017 at 14:30, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > should we add some comment like /* local variable used to avoid alignment
> > issues */ in case later one find it clever to save few bytes on stack
> > without reading git history?
> 
> Hmm. There's no obvious single place where that could be put,
> and I tend to think that if you're going to use packed structures
> then "caution needed" is implicit. When we have these bug fixes
> in then the plan is to add sparc to the set of standard merge
> build tests, which will catch accidental reversions of this
> fix (and eventually clang 4 will be more widespread which will
> warn about this.)
> 
> thanks
> -- PMM

Let's apply this for now but I really don't remember why did we
make it packed in the 1st place.

-- 
MST