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.mayd...@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4...@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);
     }