| Message ID | 1490694017-7532-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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); > } >
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
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
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 --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); }
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