diff mbox

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

Message ID 1490694017-7532-1-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show

Commit Message

Peter Maydell March 28, 2017, 9:40 a.m. UTC
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(-)

-- 
2.7.4

Comments

Eric Blake March 28, 2017, 1:14 p.m. UTC | #1
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
Philippe Mathieu-Daudé March 28, 2017, 1:30 p.m. UTC | #2
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);

>      }

>
Peter Maydell March 30, 2017, 3:08 p.m. UTC | #3
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
Michael S. Tsirkin March 30, 2017, 3:11 p.m. UTC | #4
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
Michael S. Tsirkin March 30, 2017, 3:13 p.m. UTC | #5
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
diff mbox

Patch

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);
     }