From 0f2a6c61c9c5e34d57293fb6987b21f3d1feb1cb Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 13 Feb 2018 13:29:23 +0000 Subject: [PATCH 1/3] =?UTF-8?q?gvariant:=20Realign=20data=20on=20construct?= =?UTF-8?q?ion=20if=20it=E2=80=99s=20not=20properly=20aligned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise the GVariant would later fail internal alignment checks, aborting the program. If unaligned data is provided to (for example) g_variant_new_from_data(), it will copy the data into a new aligned allocation. This is slow, but better than crashing. If callers want better performance, they should provide aligned data in their call, and it will not be copied or reallocated. Includes a unit test. Signed-off-by: Philip Withnall https://gitlab.gnome.org/GNOME/glib/issues/1342 --- glib/gvariant-core.c | 46 +++++++++++++++++++++++++++++++++++++++-- glib/gvariant.c | 10 +++++++++ glib/tests/gvariant.c | 48 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 2 deletions(-) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index 815bdf9e0..ef59c7049 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -506,6 +506,10 @@ g_variant_alloc (const GVariantType *type, * * A reference is taken on @bytes. * + * The data in @bytes must be aligned appropriately for the @type being loaded. + * Otherwise this function will internally create a copy of the memory (since + * GLib 2.60) or (in older versions) fail and exit the process. + * * Returns: (transfer none): a new #GVariant with a floating reference * * Since: 2.36 @@ -518,14 +522,50 @@ g_variant_new_from_bytes (const GVariantType *type, GVariant *value; guint alignment; gsize size; + GBytes *owned_bytes = NULL; value = g_variant_alloc (type, TRUE, trusted); - value->contents.serialised.bytes = g_bytes_ref (bytes); - g_variant_type_info_query (value->type_info, &alignment, &size); + /* Ensure the alignment is correct. This is a huge performance hit if it’s + * not correct, but that’s better than aborting if a caller provides data + * with the wrong alignment (which is likely to happen very occasionally, and + * only cause an abort on some architectures — so is unlikely to be caught + * in testing). Callers can always actively ensure they use the correct + * alignment to avoid the performance hit. */ + if ((alignment & (gsize) g_bytes_get_data (bytes, NULL)) != 0) + { +#ifdef HAVE_POSIX_MEMALIGN + gpointer aligned_data = NULL; + gsize aligned_size = g_bytes_get_size (bytes); + + /* posix_memalign() requires the alignment to be a multiple of + * sizeof(void*), and a power of 2. See g_variant_type_info_query() for + * details on the alignment format. */ + if (posix_memalign (&aligned_data, MAX (sizeof (void *), alignment + 1), + aligned_size) != 0) + g_error ("posix_memalign failed"); + + memcpy (aligned_data, g_bytes_get_data (bytes, NULL), aligned_size); + + bytes = owned_bytes = g_bytes_new_with_free_func (aligned_data, + aligned_size, + free, aligned_data); + aligned_data = NULL; +#else + /* NOTE: there may be platforms that lack posix_memalign() and also + * have malloc() that returns non-8-aligned. if so, we need to try + * harder here. + */ + bytes = owned_bytes = g_bytes_new (g_bytes_get_data (bytes, NULL), + g_bytes_get_size (bytes)); +#endif + } + + value->contents.serialised.bytes = g_bytes_ref (bytes); + if (size && g_bytes_get_size (bytes) != size) { /* Creating a fixed-sized GVariant with a bytes of the wrong @@ -543,6 +583,8 @@ g_variant_new_from_bytes (const GVariantType *type, value->contents.serialised.data = g_bytes_get_data (bytes, &value->size); } + g_clear_pointer (&owned_bytes, g_bytes_unref); + return value; } diff --git a/glib/gvariant.c b/glib/gvariant.c index d45b487ad..983d4704c 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -307,6 +307,11 @@ * Constructs a new trusted #GVariant instance from the provided data. * This is used to implement g_variant_new_* for all the basic types. * + * Note: @data must be backed by memory that is aligned appropriately for the + * @type being loaded. Otherwise this function will internally create a copy of + * the memory (since GLib 2.60) or (in older versions) fail and exit the + * process. + * * Returns: a new floating #GVariant */ static GVariant * @@ -5986,6 +5991,11 @@ g_variant_byteswap (GVariant *value) * needed. The exact time of this call is unspecified and might even be * before this function returns. * + * Note: @data must be backed by memory that is aligned appropriately for the + * @type being loaded. Otherwise this function will internally create a copy of + * the memory (since GLib 2.60) or (in older versions) fail and exit the + * process. + * * Returns: (transfer none): a new floating #GVariant of type @type * * Since: 2.24 diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index 5414da540..827e47532 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -4965,6 +4965,51 @@ test_normal_checking_empty_object_path (void) g_variant_unref (variant); } +/* Test that constructing a #GVariant from data which is not correctly aligned + * for the variant type is OK, by loading a variant from data at various offsets + * which are aligned and unaligned. When unaligned, a slow construction path + * should be taken. */ +static void +test_unaligned_construction (void) +{ + const guint8 data[] = { + 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, + 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, + }; + GVariant *variant = NULL; + GVariant *normal_variant = NULL; + gsize i, offset; + const struct { + const GVariantType *type; + gsize size; + gsize max_offset; + } vectors[] = { + { G_VARIANT_TYPE_UINT64, sizeof (guint64), sizeof (guint64) }, + { G_VARIANT_TYPE_UINT32, sizeof (guint32), sizeof (guint32) }, + { G_VARIANT_TYPE_UINT16, sizeof (guint16), sizeof (guint16) }, + { G_VARIANT_TYPE_BYTE, sizeof (guint8), 3 }, + }; + + G_STATIC_ASSERT (sizeof (guint64) * 2 <= sizeof (data)); + + for (i = 0; i < G_N_ELEMENTS (vectors); i++) + { + for (offset = 0; offset < vectors[i].max_offset; offset++) + { + variant = g_variant_new_from_data (vectors[i].type, data + offset, + vectors[i].size, + FALSE, NULL, NULL); + g_assert_nonnull (variant); + + normal_variant = g_variant_get_normal_form (variant); + g_assert_nonnull (normal_variant); + + g_variant_unref (normal_variant); + g_variant_unref (variant); + } + } +} + int main (int argc, char **argv) { @@ -5045,5 +5090,8 @@ main (int argc, char **argv) g_test_add_func ("/gvariant/recursion-limits/array-in-variant", test_recursion_limits_array_in_variant); + g_test_add_func ("/gvariant/unaligned-construction", + test_unaligned_construction); + return g_test_run (); } From 7b0f2e0e348a558b821a27212b82669f969e1526 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 6 Nov 2018 11:24:05 +0000 Subject: [PATCH 2/3] gvariant: Fix some GIR annotations on internal functions Signed-off-by: Philip Withnall --- glib/gvarianttypeinfo.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/glib/gvarianttypeinfo.c b/glib/gvarianttypeinfo.c index 399b610c8..71f59edd6 100644 --- a/glib/gvarianttypeinfo.c +++ b/glib/gvarianttypeinfo.c @@ -220,8 +220,8 @@ g_variant_type_info_get_type_string (GVariantTypeInfo *info) /* < private > * g_variant_type_info_query: * @info: a #GVariantTypeInfo - * @alignment: (nullable): the location to store the alignment, or %NULL - * @fixed_size: (nullable): the location to store the fixed size, or %NULL + * @alignment: (optional): the location to store the alignment, or %NULL + * @fixed_size: (optional): the location to store the fixed size, or %NULL * * Queries @info to determine the alignment requirements and fixed size * (if any) of the type. @@ -329,8 +329,8 @@ g_variant_type_info_element (GVariantTypeInfo *info) /* < private > * g_variant_type_query_element: * @info: a #GVariantTypeInfo for an array or maybe type - * @alignment: (nullable): the location to store the alignment, or %NULL - * @fixed_size: (nullable): the location to store the fixed size, or %NULL + * @alignment: (optional): the location to store the alignment, or %NULL + * @fixed_size: (optional): the location to store the fixed size, or %NULL * * Returns the alignment requires and fixed size (if any) for the * element type of the array. This call is a convenience wrapper around From 409ff69bd1509499637f7088ef00b370ed703ea6 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 6 Nov 2018 11:43:43 +0000 Subject: [PATCH 3/3] gvariant: Re-use g_variant_serialised_check() to check alignment Rather than duplicating the alignment checks when constructing a new GVariant, re-use the alignment checks from GVariantSerialised. This ensures that the same checks are done everywhere in the GVariant code. Signed-off-by: Philip Withnall https://gitlab.gnome.org/GNOME/glib/issues/1342 --- glib/gvariant-core.c | 7 ++++++- glib/gvariant-serialiser.c | 32 +++++++++++++++++--------------- glib/gvariant-serialiser.h | 2 ++ 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index ef59c7049..c539bf756 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -523,6 +523,7 @@ g_variant_new_from_bytes (const GVariantType *type, guint alignment; gsize size; GBytes *owned_bytes = NULL; + GVariantSerialised serialised; value = g_variant_alloc (type, TRUE, trusted); @@ -535,7 +536,11 @@ g_variant_new_from_bytes (const GVariantType *type, * only cause an abort on some architectures — so is unlikely to be caught * in testing). Callers can always actively ensure they use the correct * alignment to avoid the performance hit. */ - if ((alignment & (gsize) g_bytes_get_data (bytes, NULL)) != 0) + serialised.type_info = value->type_info; + serialised.data = (guchar *) g_bytes_get_data (bytes, &serialised.size); + serialised.depth = 0; + + if (!g_variant_serialised_check (serialised)) { #ifdef HAVE_POSIX_MEMALIGN gpointer aligned_data = NULL; diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c index ccf96b4aa..83e9d85b8 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -127,20 +127,24 @@ * * Checks @serialised for validity according to the invariants described * above. + * + * Returns: %TRUE if @serialised is valid; %FALSE otherwise */ -static void +gboolean g_variant_serialised_check (GVariantSerialised serialised) { gsize fixed_size; guint alignment; - g_assert (serialised.type_info != NULL); + if (serialised.type_info == NULL) + return FALSE; g_variant_type_info_query (serialised.type_info, &alignment, &fixed_size); - if (fixed_size) - g_assert_cmpint (serialised.size, ==, fixed_size); - else - g_assert (serialised.size == 0 || serialised.data != NULL); + if (fixed_size != 0 && serialised.size != fixed_size) + return FALSE; + else if (fixed_size == 0 && + !(serialised.size == 0 || serialised.data != NULL)) + return FALSE; /* Depending on the native alignment requirements of the machine, the * compiler will insert either 3 or 7 padding bytes after the char. @@ -167,10 +171,8 @@ g_variant_serialised_check (GVariantSerialised serialised) * Check if this is a small allocation and return without enforcing * the alignment assertion if this is the case. */ - if (serialised.size <= alignment) - return; - - g_assert_cmpint (alignment & (gsize) serialised.data, ==, 0); + return (serialised.size <= alignment || + (alignment & (gsize) serialised.data) == 0); } /* < private > @@ -1355,7 +1357,7 @@ gvs_variant_is_normal (GVariantSerialised value) gsize g_variant_serialised_n_children (GVariantSerialised serialised) { - g_variant_serialised_check (serialised); + g_assert (g_variant_serialised_check (serialised)); DISPATCH_CASES (serialised.type_info, @@ -1392,7 +1394,7 @@ g_variant_serialised_get_child (GVariantSerialised serialised, { GVariantSerialised child; - g_variant_serialised_check (serialised); + g_assert (g_variant_serialised_check (serialised)); if G_LIKELY (index_ < g_variant_serialised_n_children (serialised)) { @@ -1400,7 +1402,7 @@ g_variant_serialised_get_child (GVariantSerialised serialised, child = gvs_/**/,/**/_get_child (serialised, index_); g_assert (child.size || child.data == NULL); - g_variant_serialised_check (child); + g_assert (g_variant_serialised_check (child)); return child; ) @@ -1441,7 +1443,7 @@ g_variant_serialiser_serialise (GVariantSerialised serialised, const gpointer *children, gsize n_children) { - g_variant_serialised_check (serialised); + g_assert (g_variant_serialised_check (serialised)); DISPATCH_CASES (serialised.type_info, @@ -1496,7 +1498,7 @@ g_variant_serialised_byteswap (GVariantSerialised serialised) gsize fixed_size; guint alignment; - g_variant_serialised_check (serialised); + g_assert (g_variant_serialised_check (serialised)); if (!serialised.data) return; diff --git a/glib/gvariant-serialiser.h b/glib/gvariant-serialiser.h index fa4252d36..81343e9ca 100644 --- a/glib/gvariant-serialiser.h +++ b/glib/gvariant-serialiser.h @@ -55,6 +55,8 @@ void g_variant_serialiser_serialise (GVarian gsize n_children); /* misc */ +GLIB_AVAILABLE_IN_2_60 +gboolean g_variant_serialised_check (GVariantSerialised serialised); GLIB_AVAILABLE_IN_ALL gboolean g_variant_serialised_is_normal (GVariantSerialised value); GLIB_AVAILABLE_IN_ALL