Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
SUSE:SLE-15-SP1:Update
glib2
glib2-Various-fixes-to-normal-form-handling-in-...
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File glib2-Various-fixes-to-normal-form-handling-in-GVariant.patch of Package glib2
From ef1076e1ef5b5c852c9299bd50a2411324899ed0 Mon Sep 17 00:00:00 2001 From: Philip Withnall <withnall@endlessm.com> Date: Tue, 13 Feb 2018 13:29:23 +0000 Subject: [PATCH 01/26] =?UTF-8?q?gvariant:=20Realign=20data=20on=20constru?= =?UTF-8?q?ction=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 <withnall@endlessm.com> 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 2a59cec75..525879bed 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -308,6 +308,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 * @@ -5966,6 +5971,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 e4c1390a9..abc0b8bae 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -4855,6 +4855,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) { @@ -4935,5 +4980,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 (); } -- 2.41.0 From 31edd46396a84b2b1e1034995002448a5d11d9d9 Mon Sep 17 00:00:00 2001 From: Philip Withnall <withnall@endlessm.com> Date: Tue, 6 Nov 2018 11:24:05 +0000 Subject: [PATCH 02/26] gvariant: Fix some GIR annotations on internal functions Signed-off-by: Philip Withnall <withnall@endlessm.com> --- glib/gvarianttypeinfo.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/glib/gvarianttypeinfo.c b/glib/gvarianttypeinfo.c index 78d029425..4b93935fd 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 -- 2.41.0 From 236cfac3cf33417e2338e9099713a2e1f89c25db Mon Sep 17 00:00:00 2001 From: Philip Withnall <withnall@endlessm.com> Date: Tue, 6 Nov 2018 11:43:43 +0000 Subject: [PATCH 03/26] 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 <withnall@endlessm.com> 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 -- 2.41.0 From 200f5c099d4590c421184e9f345c0683a17f3311 Mon Sep 17 00:00:00 2001 From: Philip Withnall <withnall@endlessm.com> Date: Tue, 18 Aug 2020 09:46:12 +0100 Subject: [PATCH 04/26] gvariant: Ensure GVS.depth is initialised When byteswapping the depth was accidentally left uninitialised. Coverity CID: #1430636 Signed-off-by: Philip Withnall <withnall@endlessm.com> --- glib/gvariant-core.c | 15 +++++++++++++++ glib/gvariant-core.h | 2 ++ glib/gvariant.c | 1 + 3 files changed, 18 insertions(+) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index c539bf756..a7285f13e 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -664,6 +664,21 @@ g_variant_is_trusted (GVariant *value) return (value->state & STATE_TRUSTED) != 0; } +/* < internal > + * g_variant_get_depth: + * @value: a #GVariant + * + * Gets the nesting depth of a #GVariant. This is 0 for a #GVariant with no + * children. + * + * Returns: nesting depth of @value + */ +gsize +g_variant_get_depth (GVariant *value) +{ + return value->depth; +} + /* -- public -- */ /** diff --git a/glib/gvariant-core.h b/glib/gvariant-core.h index fc19b7ec4..fc04711ac 100644 --- a/glib/gvariant-core.h +++ b/glib/gvariant-core.h @@ -34,4 +34,6 @@ gboolean g_variant_is_trusted (GVarian GVariantTypeInfo * g_variant_get_type_info (GVariant *value); +gsize g_variant_get_depth (GVariant *value); + #endif /* __G_VARIANT_CORE_H__ */ diff --git a/glib/gvariant.c b/glib/gvariant.c index 525879bed..c9f220952 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5921,6 +5921,7 @@ g_variant_byteswap (GVariant *value) serialised.type_info = g_variant_get_type_info (trusted); serialised.size = g_variant_get_size (trusted); serialised.data = g_malloc (serialised.size); + serialised.depth = g_variant_get_depth (trusted); g_variant_store (trusted, serialised.data); g_variant_unref (trusted); -- 2.41.0 From 9cf51049d31e6603307e3a87f4ba18148d5126f2 Mon Sep 17 00:00:00 2001 From: William Manley <will@stb-tester.com> Date: Tue, 23 Jun 2020 22:59:58 +0100 Subject: [PATCH 05/26] gvariant-core: Consolidate construction of `GVariantSerialised` So I only need to change it in one place. This introduces no functional changes. Helps: #2121 --- glib/gvariant-core.c | 49 ++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index a7285f13e..35d081be9 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -348,6 +348,27 @@ g_variant_ensure_size (GVariant *value) } } +/* < private > + * g_variant_to_serialised: + * @value: a #GVariant + * + * Gets a GVariantSerialised for a GVariant in state STATE_SERIALISED. + */ +inline static GVariantSerialised +g_variant_to_serialised (GVariant *value) +{ + g_assert (value->state & STATE_SERIALISED); + { + GVariantSerialised serialised = { + value->type_info, + (gpointer) value->contents.serialised.data, + value->size, + value->depth, + }; + return serialised; + } +} + /* < private > * g_variant_serialise: * @value: a #GVariant @@ -1001,16 +1022,8 @@ g_variant_n_children (GVariant *value) g_variant_lock (value); if (value->state & STATE_SERIALISED) - { - GVariantSerialised serialised = { - value->type_info, - (gpointer) value->contents.serialised.data, - value->size, - value->depth, - }; - - n_children = g_variant_serialised_n_children (serialised); - } + n_children = g_variant_serialised_n_children ( + g_variant_to_serialised (value)); else n_children = value->contents.tree.n_children; @@ -1071,12 +1084,7 @@ g_variant_get_child_value (GVariant *value, } { - GVariantSerialised serialised = { - value->type_info, - (gpointer) value->contents.serialised.data, - value->size, - value->depth, - }; + GVariantSerialised serialised = g_variant_to_serialised (value); GVariantSerialised s_child; GVariant *child; @@ -1189,14 +1197,7 @@ g_variant_is_normal_form (GVariant *value) if (value->state & STATE_SERIALISED) { - GVariantSerialised serialised = { - value->type_info, - (gpointer) value->contents.serialised.data, - value->size, - value->depth - }; - - if (g_variant_serialised_is_normal (serialised)) + if (g_variant_serialised_is_normal (g_variant_to_serialised (value))) value->state |= STATE_TRUSTED; } else -- 2.41.0 From 1ad5c5cc080b6216a09edb9e3ed41ecbdeb2198a Mon Sep 17 00:00:00 2001 From: William Manley <will@stb-tester.com> Date: Thu, 25 Jun 2020 17:08:21 +0100 Subject: [PATCH 06/26] gvariant-serialiser: Factor out functions for dealing with framing offsets This introduces no functional changes. Helps: #2121 --- glib/gvariant-serialiser.c | 108 +++++++++++++++++++------------------ 1 file changed, 57 insertions(+), 51 deletions(-) diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c index 83e9d85b8..c7c211414 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -633,30 +633,62 @@ gvs_calculate_total_size (gsize body_size, return body_size + 8 * offsets; } +struct Offsets +{ + gsize data_size; + + guchar *array; + gsize length; + guint offset_size; + + gboolean is_normal; +}; + static gsize -gvs_variable_sized_array_n_children (GVariantSerialised value) +gvs_offsets_get_offset_n (struct Offsets *offsets, + gsize n) +{ + return gvs_read_unaligned_le ( + offsets->array + (offsets->offset_size * n), offsets->offset_size); +} + +static struct Offsets +gvs_variable_sized_array_get_frame_offsets (GVariantSerialised value) { + struct Offsets out = { 0, }; gsize offsets_array_size; - gsize offset_size; gsize last_end; if (value.size == 0) - return 0; - - offset_size = gvs_get_offset_size (value.size); + { + out.is_normal = TRUE; + return out; + } - last_end = gvs_read_unaligned_le (value.data + value.size - - offset_size, offset_size); + out.offset_size = gvs_get_offset_size (value.size); + last_end = gvs_read_unaligned_le (value.data + value.size - out.offset_size, + out.offset_size); if (last_end > value.size) - return 0; + return out; /* offsets not normal */ offsets_array_size = value.size - last_end; - if (offsets_array_size % offset_size) - return 0; + if (offsets_array_size % out.offset_size) + return out; /* offsets not normal */ + + out.data_size = last_end; + out.array = value.data + last_end; + out.length = offsets_array_size / out.offset_size; + out.is_normal = TRUE; - return offsets_array_size / offset_size; + return out; +} + +static gsize +gvs_variable_sized_array_n_children (GVariantSerialised value) +{ + return gvs_variable_sized_array_get_frame_offsets (value).length; } static GVariantSerialised @@ -664,8 +696,9 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, gsize index_) { GVariantSerialised child = { 0, }; - gsize offset_size; - gsize last_end; + + struct Offsets offsets = gvs_variable_sized_array_get_frame_offsets (value); + gsize start; gsize end; @@ -673,18 +706,11 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, g_variant_type_info_ref (child.type_info); child.depth = value.depth + 1; - offset_size = gvs_get_offset_size (value.size); - - last_end = gvs_read_unaligned_le (value.data + value.size - - offset_size, offset_size); - if (index_ > 0) { guint alignment; - start = gvs_read_unaligned_le (value.data + last_end + - (offset_size * (index_ - 1)), - offset_size); + start = gvs_offsets_get_offset_n (&offsets, index_ - 1); g_variant_type_info_query (child.type_info, &alignment, NULL); start += (-start) & alignment; @@ -692,11 +718,9 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, else start = 0; - end = gvs_read_unaligned_le (value.data + last_end + - (offset_size * index_), - offset_size); + end = gvs_offsets_get_offset_n (&offsets, index_); - if (start < end && end <= value.size && end <= last_end) + if (start < end && end <= value.size && end <= offsets.data_size) { child.data = value.data + start; child.size = end - start; @@ -768,34 +792,16 @@ static gboolean gvs_variable_sized_array_is_normal (GVariantSerialised value) { GVariantSerialised child = { 0, }; - gsize offsets_array_size; - guchar *offsets_array; - guint offset_size; guint alignment; - gsize last_end; - gsize length; gsize offset; gsize i; - if (value.size == 0) - return TRUE; - - offset_size = gvs_get_offset_size (value.size); - last_end = gvs_read_unaligned_le (value.data + value.size - - offset_size, offset_size); + struct Offsets offsets = gvs_variable_sized_array_get_frame_offsets (value); - if (last_end > value.size) + if (!offsets.is_normal) return FALSE; - offsets_array_size = value.size - last_end; - - if (offsets_array_size % offset_size) - return FALSE; - - offsets_array = value.data + value.size - offsets_array_size; - length = offsets_array_size / offset_size; - - if (length == 0) + if (value.size != 0 && offsets.length == 0) return FALSE; child.type_info = g_variant_type_info_element (value.type_info); @@ -803,14 +809,14 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) child.depth = value.depth + 1; offset = 0; - for (i = 0; i < length; i++) + for (i = 0; i < offsets.length; i++) { gsize this_end; - this_end = gvs_read_unaligned_le (offsets_array + offset_size * i, - offset_size); + this_end = gvs_read_unaligned_le (offsets.array + offsets.offset_size * i, + offsets.offset_size); - if (this_end < offset || this_end > last_end) + if (this_end < offset || this_end > offsets.data_size) return FALSE; while (offset & alignment) @@ -832,7 +838,7 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) offset = this_end; } - g_assert (offset == last_end); + g_assert (offset == offsets.data_size); return TRUE; } -- 2.41.0 From ccf0d20834353e6c988b6859cb8195ed57d552dc Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Tue, 25 Oct 2022 18:05:52 +0100 Subject: [PATCH 07/26] gvariant: Zero-initialise various GVariantSerialised objects The following few commits will add a couple of new fields to `GVariantSerialised`, and they should be zero-filled by default. Try and pre-empt that a bit by zero-filling `GVariantSerialised` by default in a few places. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Helps: #2121 --- glib/gvariant.c | 2 +- glib/tests/gvariant.c | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index c9f220952..e5c3e1da0 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5913,7 +5913,7 @@ g_variant_byteswap (GVariant *value) if (alignment) /* (potentially) contains multi-byte numeric data */ { - GVariantSerialised serialised; + GVariantSerialised serialised = { 0, }; GVariant *trusted; GBytes *bytes; diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index abc0b8bae..a2d7f9107 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -1441,7 +1441,7 @@ test_maybe (void) for (flavour = 0; flavour < 8; flavour += alignment) { - GVariantSerialised serialised; + GVariantSerialised serialised = { 0, }; GVariantSerialised child; serialised.type_info = type_info; @@ -1565,7 +1565,7 @@ test_array (void) for (flavour = 0; flavour < 8; flavour += alignment) { - GVariantSerialised serialised; + GVariantSerialised serialised = { 0, }; serialised.type_info = array_info; serialised.data = flavoured_malloc (needed_size, flavour); @@ -1729,7 +1729,7 @@ test_tuple (void) for (flavour = 0; flavour < 8; flavour += alignment) { - GVariantSerialised serialised; + GVariantSerialised serialised = { 0, }; serialised.type_info = type_info; serialised.data = flavoured_malloc (needed_size, flavour); @@ -1824,7 +1824,7 @@ test_variant (void) for (flavour = 0; flavour < 8; flavour += alignment) { - GVariantSerialised serialised; + GVariantSerialised serialised = { 0, }; GVariantSerialised child; serialised.type_info = type_info; @@ -2271,7 +2271,7 @@ serialise_tree (TreeInstance *tree, static void test_byteswap (void) { - GVariantSerialised one, two; + GVariantSerialised one = { 0, }, two = { 0, }; TreeInstance *tree; tree = tree_instance_new (NULL, 3); @@ -2305,7 +2305,7 @@ test_byteswaps (void) static void test_fuzz (gdouble *fuzziness) { - GVariantSerialised serialised; + GVariantSerialised serialised = { 0, }; TreeInstance *tree; /* make an instance */ -- 2.41.0 From a7eec667d4a7e61d2fffc03162862ffcea7a89be Mon Sep 17 00:00:00 2001 From: William Manley <will@stb-tester.com> Date: Mon, 29 Jun 2020 16:59:44 +0100 Subject: [PATCH 08/26] =?UTF-8?q?gvariant:=20Don=E2=80=99t=20allow=20child?= =?UTF-8?q?=20elements=20to=20overlap=20with=20each=20other?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If different elements of a variable sized array can overlap with each other then we can cause a `GVariant` to normalise to a much larger type. This commit changes the behaviour of `GVariant` with non-normal form data. If an invalid frame offset is found all subsequent elements are given their default value. When retrieving an element at index `n` we scan the frame offsets up to index `n` and if they are not in order we return an element with the default value for that type. This guarantees that elements don't overlap with each other. We remember the offset we've scanned up to so we don't need to repeat this work on subsequent accesses. We skip these checks for trusted data. Unfortunately this makes random access of untrusted data O(n) — at least on first access. It doesn't affect the algorithmic complexity of accessing elements in order, such as when using the `GVariantIter` interface. Also: the cost of validation will be amortised as the `GVariant` instance is continued to be used. I've implemented this with 4 different functions, 1 for each element size, rather than looping calling `gvs_read_unaligned_le` in the hope that the compiler will find it easy to optimise and should produce fairly tight code. Fixes: #2121 --- glib/gvariant-core.c | 35 ++++++++++++++++ glib/gvariant-serialiser.c | 86 ++++++++++++++++++++++++++++++++++++-- glib/gvariant-serialiser.h | 9 ++++ glib/tests/gvariant.c | 45 ++++++++++++++++++++ 4 files changed, 172 insertions(+), 3 deletions(-) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index 35d081be9..931eb34a7 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -64,6 +64,7 @@ struct _GVariant { GBytes *bytes; gconstpointer data; + gsize ordered_offsets_up_to; } serialised; struct @@ -161,6 +162,24 @@ struct _GVariant * if .data pointed to the appropriate number of nul * bytes. * + * .ordered_offsets_up_to: If ordered_offsets_up_to == n this means that all + * the frame offsets up to and including the frame + * offset determining the end of element n are in + * order. This guarantees that the bytes of element + * n don't overlap with any previous element. + * + * For trusted data this is set to G_MAXSIZE and we + * don't check that the frame offsets are in order. + * + * Note: This doesn't imply the offsets are good in + * any way apart from their ordering. In particular + * offsets may be out of bounds for this value or + * may imply that the data overlaps the frame + * offsets themselves. + * + * This field is only relevant for arrays of non + * fixed width types. + * * .tree: Only valid when the instance is in tree form. * * Note that accesses from other threads could result in @@ -364,6 +383,7 @@ g_variant_to_serialised (GVariant *value) (gpointer) value->contents.serialised.data, value->size, value->depth, + value->contents.serialised.ordered_offsets_up_to, }; return serialised; } @@ -395,6 +415,7 @@ g_variant_serialise (GVariant *value, serialised.size = value->size; serialised.data = data; serialised.depth = value->depth; + serialised.ordered_offsets_up_to = 0; children = (gpointer *) value->contents.tree.children; n_children = value->contents.tree.n_children; @@ -438,6 +459,15 @@ g_variant_fill_gvs (GVariantSerialised *serialised, g_assert (serialised->size == value->size); serialised->depth = value->depth; + if (value->state & STATE_SERIALISED) + { + serialised->ordered_offsets_up_to = value->contents.serialised.ordered_offsets_up_to; + } + else + { + serialised->ordered_offsets_up_to = 0; + } + if (serialised->data) /* g_variant_store() is a public API, so it * it will reacquire the lock if it needs to. @@ -480,6 +510,7 @@ g_variant_ensure_serialised (GVariant *value) bytes = g_bytes_new_take (data, value->size); value->contents.serialised.data = g_bytes_get_data (bytes, NULL); value->contents.serialised.bytes = bytes; + value->contents.serialised.ordered_offsets_up_to = G_MAXSIZE; value->state |= STATE_SERIALISED; } } @@ -560,6 +591,7 @@ g_variant_new_from_bytes (const GVariantType *type, serialised.type_info = value->type_info; serialised.data = (guchar *) g_bytes_get_data (bytes, &serialised.size); serialised.depth = 0; + serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0; if (!g_variant_serialised_check (serialised)) { @@ -609,6 +641,8 @@ g_variant_new_from_bytes (const GVariantType *type, value->contents.serialised.data = g_bytes_get_data (bytes, &value->size); } + value->contents.serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0; + g_clear_pointer (&owned_bytes, g_bytes_unref); return value; @@ -1118,6 +1152,7 @@ g_variant_get_child_value (GVariant *value, child->contents.serialised.bytes = g_bytes_ref (value->contents.serialised.bytes); child->contents.serialised.data = s_child.data; + child->contents.serialised.ordered_offsets_up_to = s_child.ordered_offsets_up_to; return child; } diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c index c7c211414..fe0b1a43c 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -1,6 +1,7 @@ /* * Copyright © 2007, 2008 Ryan Lortie * Copyright © 2010 Codethink Limited + * Copyright © 2020 William Manley * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -264,6 +265,7 @@ gvs_fixed_sized_maybe_get_child (GVariantSerialised value, value.type_info = g_variant_type_info_element (value.type_info); g_variant_type_info_ref (value.type_info); value.depth++; + value.ordered_offsets_up_to = 0; return value; } @@ -295,7 +297,7 @@ gvs_fixed_sized_maybe_serialise (GVariantSerialised value, { if (n_children) { - GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1 }; + GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1, 0 }; gvs_filler (&child, children[0]); } @@ -317,6 +319,7 @@ gvs_fixed_sized_maybe_is_normal (GVariantSerialised value) /* proper element size: "Just". recurse to the child. */ value.type_info = g_variant_type_info_element (value.type_info); value.depth++; + value.ordered_offsets_up_to = 0; return g_variant_serialised_is_normal (value); } @@ -358,6 +361,7 @@ gvs_variable_sized_maybe_get_child (GVariantSerialised value, value.data = NULL; value.depth++; + value.ordered_offsets_up_to = 0; return value; } @@ -388,7 +392,7 @@ gvs_variable_sized_maybe_serialise (GVariantSerialised value, { if (n_children) { - GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1 }; + GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1, 0 }; /* write the data for the child. */ gvs_filler (&child, children[0]); @@ -408,6 +412,7 @@ gvs_variable_sized_maybe_is_normal (GVariantSerialised value) value.type_info = g_variant_type_info_element (value.type_info); value.size--; value.depth++; + value.ordered_offsets_up_to = 0; return g_variant_serialised_is_normal (value); } @@ -691,6 +696,32 @@ gvs_variable_sized_array_n_children (GVariantSerialised value) return gvs_variable_sized_array_get_frame_offsets (value).length; } +/* Find the index of the first out-of-order element in @data, assuming that + * @data is an array of elements of given @type, starting at index @start and + * containing a further @len-@start elements. */ +#define DEFINE_FIND_UNORDERED(type) \ + static gsize \ + find_unordered_##type (const guint8 *data, gsize start, gsize len) \ + { \ + gsize off; \ + type current, previous; \ + \ + memcpy (&previous, data + start * sizeof (current), sizeof (current)); \ + for (off = (start + 1) * sizeof (current); off < len * sizeof (current); off += sizeof (current)) \ + { \ + memcpy (¤t, data + off, sizeof (current)); \ + if (current < previous) \ + break; \ + previous = current; \ + } \ + return off / sizeof (current) - 1; \ + } + +DEFINE_FIND_UNORDERED (guint8); +DEFINE_FIND_UNORDERED (guint16); +DEFINE_FIND_UNORDERED (guint32); +DEFINE_FIND_UNORDERED (guint64); + static GVariantSerialised gvs_variable_sized_array_get_child (GVariantSerialised value, gsize index_) @@ -706,6 +737,49 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, g_variant_type_info_ref (child.type_info); child.depth = value.depth + 1; + /* If the requested @index_ is beyond the set of indices whose framing offsets + * have been checked, check the remaining offsets to see whether they’re + * normal (in order, no overlapping array elements). */ + if (index_ > value.ordered_offsets_up_to) + { + switch (offsets.offset_size) + { + case 1: + { + value.ordered_offsets_up_to = find_unordered_guint8 ( + offsets.array, value.ordered_offsets_up_to, index_ + 1); + break; + } + case 2: + { + value.ordered_offsets_up_to = find_unordered_guint16 ( + offsets.array, value.ordered_offsets_up_to, index_ + 1); + break; + } + case 4: + { + value.ordered_offsets_up_to = find_unordered_guint32 ( + offsets.array, value.ordered_offsets_up_to, index_ + 1); + break; + } + case 8: + { + value.ordered_offsets_up_to = find_unordered_guint64 ( + offsets.array, value.ordered_offsets_up_to, index_ + 1); + break; + } + default: + /* gvs_get_offset_size() only returns maximum 8 */ + g_assert_not_reached (); + } + } + + if (index_ > value.ordered_offsets_up_to) + { + /* Offsets are invalid somewhere, so return an empty child. */ + return child; + } + if (index_ > 0) { guint alignment; @@ -840,6 +914,9 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) g_assert (offset == offsets.data_size); + /* All offsets have now been checked. */ + value.ordered_offsets_up_to = G_MAXSIZE; + return TRUE; } @@ -1072,7 +1149,7 @@ gvs_tuple_is_normal (GVariantSerialised value) for (i = 0; i < length; i++) { const GVariantMemberInfo *member_info; - GVariantSerialised child; + GVariantSerialised child = { 0, }; gsize fixed_size; guint alignment; gsize end; @@ -1132,6 +1209,9 @@ gvs_tuple_is_normal (GVariantSerialised value) offset = end; } + /* All element bounds have been checked above. */ + value.ordered_offsets_up_to = G_MAXSIZE; + { gsize fixed_size; guint alignment; diff --git a/glib/gvariant-serialiser.h b/glib/gvariant-serialiser.h index 81343e9ca..bc636000c 100644 --- a/glib/gvariant-serialiser.h +++ b/glib/gvariant-serialiser.h @@ -29,6 +29,15 @@ typedef struct guchar *data; gsize size; gsize depth; /* same semantics as GVariant.depth */ + + /* If ordered_offsets_up_to == n this means that all the frame offsets up to and + * including the frame offset determining the end of element n are in order. + * This guarantees that the bytes of element n don't overlap with any previous + * element. + * + * This is both read and set by g_variant_serialised_get_child for arrays of + * non-fixed-width types */ + gsize ordered_offsets_up_to; } GVariantSerialised; /* deserialisation */ diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index a2d7f9107..9cd2bd90e 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -1,5 +1,6 @@ /* * Copyright © 2010 Codethink Limited + * Copyright © 2020 William Manley * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1282,6 +1283,7 @@ random_instance_filler (GVariantSerialised *serialised, serialised->size = instance->size; serialised->depth = 0; + serialised->ordered_offsets_up_to = 0; g_assert (serialised->type_info == instance->type_info); g_assert (serialised->size == instance->size); @@ -4807,6 +4809,47 @@ test_normal_checking_array_offsets (void) g_variant_unref (variant); } +/* This is a regression test that we can't have non-normal values that take up + * significantly more space than the normal equivalent, by specifying the + * offset table entries so that array elements overlap. + * + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_832242 */ +static void +test_normal_checking_array_offsets2 (void) +{ + const guint8 data[] = { + 'h', 'i', '\0', + 0x03, 0x00, 0x03, + 0x06, 0x00, 0x06, + 0x09, 0x00, 0x09, + 0x0c, 0x00, 0x0c, + 0x0f, 0x00, 0x0f, + 0x12, 0x00, 0x12, + 0x15, 0x00, 0x15, + }; + gsize size = sizeof (data); + const GVariantType *aaaaaaas = G_VARIANT_TYPE ("aaaaaaas"); + GVariant *variant = NULL; + GVariant *normal_variant = NULL; + GVariant *expected = NULL; + + variant = g_variant_new_from_data (aaaaaaas, data, size, FALSE, NULL, NULL); + g_assert_nonnull (variant); + + normal_variant = g_variant_get_normal_form (variant); + g_assert_nonnull (normal_variant); + g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 2); + + expected = g_variant_new_parsed ( + "[[[[[[['hi', '', ''], [], []], [], []], [], []], [], []], [], []], [], []]"); + g_assert_cmpvariant (expected, variant); + g_assert_cmpvariant (expected, normal_variant); + + g_variant_unref (expected); + g_variant_unref (normal_variant); + g_variant_unref (variant); +} + /* Test that a tuple with invalidly large values in its offset table is * normalised successfully without looping infinitely. */ static void @@ -4970,6 +5013,8 @@ main (int argc, char **argv) test_normal_checking_tuples); g_test_add_func ("/gvariant/normal-checking/array-offsets", test_normal_checking_array_offsets); + g_test_add_func ("/gvariant/normal-checking/array-offsets2", + test_normal_checking_array_offsets2); g_test_add_func ("/gvariant/normal-checking/tuple-offsets", test_normal_checking_tuple_offsets); g_test_add_func ("/gvariant/normal-checking/empty-object-path", -- 2.41.0 From feca25deafb272033112e804d13249c0d111df21 Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Fri, 7 Jan 2022 15:03:52 +0000 Subject: [PATCH 09/26] gvariant-serialiser: Factor out code to get bounds of a tuple member This introduces no functional changes. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Helps: #2121 --- glib/gvariant-serialiser.c | 73 ++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 27 deletions(-) diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c index fe0b1a43c..6f9b36624 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -942,6 +942,51 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) * for the tuple. See the notes in gvarianttypeinfo.h. */ +static void +gvs_tuple_get_member_bounds (GVariantSerialised value, + gsize index_, + gsize offset_size, + gsize *out_member_start, + gsize *out_member_end) +{ + const GVariantMemberInfo *member_info; + gsize member_start, member_end; + + member_info = g_variant_type_info_member_info (value.type_info, index_); + + if (member_info->i + 1) + member_start = gvs_read_unaligned_le (value.data + value.size - + offset_size * (member_info->i + 1), + offset_size); + else + member_start = 0; + + member_start += member_info->a; + member_start &= member_info->b; + member_start |= member_info->c; + + if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST) + member_end = value.size - offset_size * (member_info->i + 1); + + else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED) + { + gsize fixed_size; + + g_variant_type_info_query (member_info->type_info, NULL, &fixed_size); + member_end = member_start + fixed_size; + } + + else /* G_VARIANT_MEMBER_ENDING_OFFSET */ + member_end = gvs_read_unaligned_le (value.data + value.size - + offset_size * (member_info->i + 2), + offset_size); + + if (out_member_start != NULL) + *out_member_start = member_start; + if (out_member_end != NULL) + *out_member_end = member_end; +} + static gsize gvs_tuple_n_children (GVariantSerialised value) { @@ -997,33 +1042,7 @@ gvs_tuple_get_child (GVariantSerialised value, } } - if (member_info->i + 1) - start = gvs_read_unaligned_le (value.data + value.size - - offset_size * (member_info->i + 1), - offset_size); - else - start = 0; - - start += member_info->a; - start &= member_info->b; - start |= member_info->c; - - if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_LAST) - end = value.size - offset_size * (member_info->i + 1); - - else if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED) - { - gsize fixed_size; - - g_variant_type_info_query (child.type_info, NULL, &fixed_size); - end = start + fixed_size; - child.size = fixed_size; - } - - else /* G_VARIANT_MEMBER_ENDING_OFFSET */ - end = gvs_read_unaligned_le (value.data + value.size - - offset_size * (member_info->i + 2), - offset_size); + gvs_tuple_get_member_bounds (value, index_, offset_size, &start, &end); /* The child should not extend into the offset table. */ if (index_ != g_variant_type_info_n_members (value.type_info) - 1) -- 2.41.0 From 6d4773eb51241b8a0164a39d58c357907db47c64 Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Fri, 7 Jan 2022 16:37:29 +0000 Subject: [PATCH 10/26] gvariant-serialiser: Rework child size calculation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reduces a few duplicate calls to `g_variant_type_info_query()` and explains why they’re needed. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Helps: #2121 --- glib/gvariant-serialiser.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c index 6f9b36624..fb759236c 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -1007,14 +1007,18 @@ gvs_tuple_get_child (GVariantSerialised value, child.depth = value.depth + 1; offset_size = gvs_get_offset_size (value.size); + /* Ensure the size is set for fixed-sized children, or + * g_variant_serialised_check() will fail, even if we return + * (child.data == NULL) to indicate an error. */ + if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_FIXED) + g_variant_type_info_query (child.type_info, NULL, &child.size); + /* tuples are the only (potentially) fixed-sized containers, so the * only ones that have to deal with the possibility of having %NULL * data with a non-zero %size if errors occurred elsewhere. */ if G_UNLIKELY (value.data == NULL && value.size != 0) { - g_variant_type_info_query (child.type_info, NULL, &child.size); - /* this can only happen in fixed-sized tuples, * so the child must also be fixed sized. */ @@ -1032,29 +1036,12 @@ gvs_tuple_get_child (GVariantSerialised value, else { if (offset_size * (member_info->i + 1) > value.size) - { - /* if the child is fixed size, return its size. - * if child is not fixed-sized, return size = 0. - */ - g_variant_type_info_query (child.type_info, NULL, &child.size); - - return child; - } + return child; } - gvs_tuple_get_member_bounds (value, index_, offset_size, &start, &end); - /* The child should not extend into the offset table. */ - if (index_ != g_variant_type_info_n_members (value.type_info) - 1) - { - GVariantSerialised last_child; - last_child = gvs_tuple_get_child (value, - g_variant_type_info_n_members (value.type_info) - 1); - last_end = last_child.data + last_child.size - value.data; - g_variant_type_info_unref (last_child.type_info); - } - else - last_end = end; + gvs_tuple_get_member_bounds (value, index_, offset_size, &start, &end); + gvs_tuple_get_member_bounds (value, g_variant_type_info_n_members (value.type_info) - 1, offset_size, NULL, &last_end); if (start < end && end <= value.size && end <= last_end) { -- 2.41.0 From 5e63f22b7b04b56d47e28a2bbb40aad96bc8c83b Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Fri, 7 Jan 2022 16:42:14 +0000 Subject: [PATCH 11/26] =?UTF-8?q?gvariant:=20Don=E2=80=99t=20allow=20child?= =?UTF-8?q?=20elements=20of=20a=20tuple=20to=20overlap=20each=20other?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is similar to the earlier commit which prevents child elements of a variable-sized array from overlapping each other, but this time for tuples. It is based heavily on ideas by William Manley. Tuples are slightly different from variable-sized arrays in that they contain a mixture of fixed and variable sized elements. All but one of the variable sized elements have an entry in the frame offsets table. This means that if we were to just check the ordering of the frame offsets table, the variable sized elements could still overlap interleaving fixed sized elements, which would be bad. Therefore we have to check the elements rather than the frame offsets. The logic of checking the elements up to the index currently being requested, and caching the result in `ordered_offsets_up_to`, means that the algorithmic cost implications are the same for this commit as for variable-sized arrays: an O(N) cost for these checks is amortised out over N accesses to O(1) per access. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Fixes: #2121 --- glib/gvariant-core.c | 6 +- glib/gvariant-serialiser.c | 40 ++++++++ glib/gvariant-serialiser.h | 7 +- glib/gvariant.c | 1 + glib/tests/gvariant.c | 181 +++++++++++++++++++++++++++++++++++++ 5 files changed, 232 insertions(+), 3 deletions(-) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index 931eb34a7..f11814d57 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -1,6 +1,7 @@ /* * Copyright © 2007, 2008 Ryan Lortie * Copyright © 2010 Codethink Limited + * Copyright © 2022 Endless OS Foundation, LLC * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -178,7 +179,7 @@ struct _GVariant * offsets themselves. * * This field is only relevant for arrays of non - * fixed width types. + * fixed width types and for tuples. * * .tree: Only valid when the instance is in tree form. * @@ -1127,6 +1128,9 @@ g_variant_get_child_value (GVariant *value, */ s_child = g_variant_serialised_get_child (serialised, index_); + /* Update the cached ordered_offsets_up_to, since @serialised will be thrown away when this function exits */ + value->contents.serialised.ordered_offsets_up_to = MAX (value->contents.serialised.ordered_offsets_up_to, serialised.ordered_offsets_up_to); + /* Check whether this would cause nesting too deep. If so, return a fake * child. The only situation we expect this to happen in is with a variant, * as all other deeply-nested types have a static type, and hence should diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c index fb759236c..cd4a3e6e4 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -942,6 +942,10 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) * for the tuple. See the notes in gvarianttypeinfo.h. */ +/* Note: This doesn’t guarantee that @out_member_end >= @out_member_start; that + * condition may not hold true for invalid serialised variants. The caller is + * responsible for checking the returned values and handling invalid ones + * appropriately. */ static void gvs_tuple_get_member_bounds (GVariantSerialised value, gsize index_, @@ -1028,6 +1032,42 @@ gvs_tuple_get_child (GVariantSerialised value, return child; } + /* If the requested @index_ is beyond the set of indices whose framing offsets + * have been checked, check the remaining offsets to see whether they’re + * normal (in order, no overlapping tuple elements). + * + * Unlike the checks in gvs_variable_sized_array_get_child(), we have to check + * all the tuple *elements* here, not just all the framing offsets, since + * tuples contain a mix of elements which use framing offsets and ones which + * don’t. None of them are allowed to overlap. */ + if (index_ > value.ordered_offsets_up_to) + { + gsize i, prev_i_end = 0; + + if (value.ordered_offsets_up_to > 0) + gvs_tuple_get_member_bounds (value, value.ordered_offsets_up_to - 1, offset_size, NULL, &prev_i_end); + + for (i = value.ordered_offsets_up_to; i <= index_; i++) + { + gsize i_start, i_end; + + gvs_tuple_get_member_bounds (value, i, offset_size, &i_start, &i_end); + + if (i_start > i_end || i_start < prev_i_end || i_end > value.size) + break; + + prev_i_end = i_end; + } + + value.ordered_offsets_up_to = i - 1; + } + + if (index_ > value.ordered_offsets_up_to) + { + /* Offsets are invalid somewhere, so return an empty child. */ + return child; + } + if (member_info->ending_type == G_VARIANT_MEMBER_ENDING_OFFSET) { if (offset_size * (member_info->i + 2) > value.size) diff --git a/glib/gvariant-serialiser.h b/glib/gvariant-serialiser.h index bc636000c..765bd43b9 100644 --- a/glib/gvariant-serialiser.h +++ b/glib/gvariant-serialiser.h @@ -35,8 +35,11 @@ typedef struct * This guarantees that the bytes of element n don't overlap with any previous * element. * - * This is both read and set by g_variant_serialised_get_child for arrays of - * non-fixed-width types */ + * This is both read and set by g_variant_serialised_get_child() for arrays of + * non-fixed-width types, and for tuples. + * + * Even when dealing with tuples, @ordered_offsets_up_to is an element index, + * rather than an index into the frame offsets. */ gsize ordered_offsets_up_to; } GVariantSerialised; diff --git a/glib/gvariant.c b/glib/gvariant.c index e5c3e1da0..edd1a7a7f 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5922,6 +5922,7 @@ g_variant_byteswap (GVariant *value) serialised.size = g_variant_get_size (trusted); serialised.data = g_malloc (serialised.size); serialised.depth = g_variant_get_depth (trusted); + serialised.ordered_offsets_up_to = G_MAXSIZE; /* operating on the normal form */ g_variant_store (trusted, serialised.data); g_variant_unref (trusted); diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index 9cd2bd90e..d6690f3ea 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -1,6 +1,7 @@ /* * Copyright © 2010 Codethink Limited * Copyright © 2020 William Manley + * Copyright © 2022 Endless OS Foundation, LLC * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1450,6 +1451,7 @@ test_maybe (void) serialised.data = flavoured_malloc (needed_size, flavour); serialised.size = needed_size; serialised.depth = 0; + serialised.ordered_offsets_up_to = 0; g_variant_serialiser_serialise (serialised, random_instance_filler, @@ -1573,6 +1575,7 @@ test_array (void) serialised.data = flavoured_malloc (needed_size, flavour); serialised.size = needed_size; serialised.depth = 0; + serialised.ordered_offsets_up_to = 0; g_variant_serialiser_serialise (serialised, random_instance_filler, (gpointer *) instances, n_children); @@ -1737,6 +1740,7 @@ test_tuple (void) serialised.data = flavoured_malloc (needed_size, flavour); serialised.size = needed_size; serialised.depth = 0; + serialised.ordered_offsets_up_to = 0; g_variant_serialiser_serialise (serialised, random_instance_filler, (gpointer *) instances, n_children); @@ -1833,6 +1837,7 @@ test_variant (void) serialised.data = flavoured_malloc (needed_size, flavour); serialised.size = needed_size; serialised.depth = 0; + serialised.ordered_offsets_up_to = 0; g_variant_serialiser_serialise (serialised, random_instance_filler, (gpointer *) &instance, 1); @@ -4874,6 +4879,176 @@ test_normal_checking_tuple_offsets (void) g_variant_unref (variant); } +/* This is a regression test that we can't have non-normal values that take up + * significantly more space than the normal equivalent, by specifying the + * offset table entries so that tuple elements overlap. + * + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_838503 and + * https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_838513 */ +static void +test_normal_checking_tuple_offsets2 (void) +{ + const GVariantType *data_type = G_VARIANT_TYPE ("(yyaiyyaiyy)"); + const guint8 data[] = { + 0x12, 0x34, 0x56, 0x78, 0x01, + /* + ^───────────────────┘ + + ^^^^^^^^^^ 1st yy + ^^^^^^^^^^ 2nd yy + ^^^^^^^^^^ 3rd yy + ^^^^ Framing offsets + */ + + /* If this variant was encoded normally, it would be something like this: + * 0x12, 0x34, pad, pad, [array bytes], 0x56, 0x78, pad, pad, [array bytes], 0x9A, 0xBC, 0xXX + * ^─────────────────────────────────────────────────────┘ + * + * ^^^^^^^^^^ 1st yy + * ^^^^^^^^^^ 2nd yy + * ^^^^^^^^^^ 3rd yy + * ^^^^ Framing offsets + */ + }; + gsize size = sizeof (data); + GVariant *variant = NULL; + GVariant *normal_variant = NULL; + GVariant *expected = NULL; + + variant = g_variant_new_from_data (data_type, data, size, FALSE, NULL, NULL); + g_assert_nonnull (variant); + + normal_variant = g_variant_get_normal_form (variant); + g_assert_nonnull (normal_variant); + g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 3); + + expected = g_variant_new_parsed ( + "@(yyaiyyaiyy) (0x12, 0x34, [], 0x00, 0x00, [], 0x00, 0x00)"); + g_assert_cmpvariant (expected, variant); + g_assert_cmpvariant (expected, normal_variant); + + g_variant_unref (expected); + g_variant_unref (normal_variant); + g_variant_unref (variant); +} + +/* This is a regression test that overlapping entries in the offset table are + * decoded consistently, even though they’re non-normal. + * + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_910935 */ +static void +test_normal_checking_tuple_offsets3 (void) +{ + /* The expected decoding of this non-normal byte stream is complex. See + * section 2.7.3 (Handling Non-Normal Serialised Data) of the GVariant + * specification. + * + * The rule “Child Values Overlapping Framing Offsets” from the specification + * says that the first `ay` must be decoded as `[0x01]` even though it + * overlaps the first byte of the offset table. However, since commit + * 7eedcd76f7d5b8c98fa60013e1fe6e960bf19df3, GLib explicitly doesn’t allow + * this as it’s exploitable. So the first `ay` must be given a default value. + * + * The second and third `ay`s must be given default values because of rule + * “End Boundary Precedes Start Boundary”. + * + * The `i` must be given a default value because of rule “Start or End + * Boundary of a Child Falls Outside the Container”. + */ + const GVariantType *data_type = G_VARIANT_TYPE ("(ayayiay)"); + const guint8 data[] = { + 0x01, 0x00, 0x02, + /* + ^──┘ + + ^^^^^^^^^^ 1st ay, bytes 0-2 (but given a default value anyway, see above) + 2nd ay, bytes 2-0 + i, bytes 0-4 + 3rd ay, bytes 4-1 + ^^^^^^^^^^ Framing offsets + */ + }; + gsize size = sizeof (data); + GVariant *variant = NULL; + GVariant *normal_variant = NULL; + GVariant *expected = NULL; + + variant = g_variant_new_from_data (data_type, data, size, FALSE, NULL, NULL); + g_assert_nonnull (variant); + + g_assert_false (g_variant_is_normal_form (variant)); + + normal_variant = g_variant_get_normal_form (variant); + g_assert_nonnull (normal_variant); + g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 3); + + expected = g_variant_new_parsed ("@(ayayiay) ([], [], 0, [])"); + g_assert_cmpvariant (expected, variant); + g_assert_cmpvariant (expected, normal_variant); + + g_variant_unref (expected); + g_variant_unref (normal_variant); + g_variant_unref (variant); +} + +/* This is a regression test that overlapping entries in the offset table are + * decoded consistently, even though they’re non-normal. + * + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2121#note_910935 */ +static void +test_normal_checking_tuple_offsets4 (void) +{ + /* The expected decoding of this non-normal byte stream is complex. See + * section 2.7.3 (Handling Non-Normal Serialised Data) of the GVariant + * specification. + * + * The rule “Child Values Overlapping Framing Offsets” from the specification + * says that the first `ay` must be decoded as `[0x01]` even though it + * overlaps the first byte of the offset table. However, since commit + * 7eedcd76f7d5b8c98fa60013e1fe6e960bf19df3, GLib explicitly doesn’t allow + * this as it’s exploitable. So the first `ay` must be given a default value. + * + * The second `ay` must be given a default value because of rule “End Boundary + * Precedes Start Boundary”. + * + * The third `ay` must be given a default value because its framing offsets + * overlap that of the first `ay`. + */ + const GVariantType *data_type = G_VARIANT_TYPE ("(ayayay)"); + const guint8 data[] = { + 0x01, 0x00, 0x02, + /* + ^──┘ + + ^^^^^^^^^^ 1st ay, bytes 0-2 (but given a default value anyway, see above) + 2nd ay, bytes 2-0 + 3rd ay, bytes 0-1 + ^^^^^^^^^^ Framing offsets + */ + }; + gsize size = sizeof (data); + GVariant *variant = NULL; + GVariant *normal_variant = NULL; + GVariant *expected = NULL; + + variant = g_variant_new_from_data (data_type, data, size, FALSE, NULL, NULL); + g_assert_nonnull (variant); + + g_assert_false (g_variant_is_normal_form (variant)); + + normal_variant = g_variant_get_normal_form (variant); + g_assert_nonnull (normal_variant); + g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 3); + + expected = g_variant_new_parsed ("@(ayayay) ([], [], [])"); + g_assert_cmpvariant (expected, variant); + g_assert_cmpvariant (expected, normal_variant); + + g_variant_unref (expected); + g_variant_unref (normal_variant); + g_variant_unref (variant); +} + /* Test that an empty object path is normalised successfully to the base object * path, ‘/’. */ static void @@ -5017,6 +5192,12 @@ main (int argc, char **argv) test_normal_checking_array_offsets2); g_test_add_func ("/gvariant/normal-checking/tuple-offsets", test_normal_checking_tuple_offsets); + g_test_add_func ("/gvariant/normal-checking/tuple-offsets2", + test_normal_checking_tuple_offsets2); + g_test_add_func ("/gvariant/normal-checking/tuple-offsets3", + test_normal_checking_tuple_offsets3); + g_test_add_func ("/gvariant/normal-checking/tuple-offsets4", + test_normal_checking_tuple_offsets4); g_test_add_func ("/gvariant/normal-checking/empty-object-path", test_normal_checking_empty_object_path); -- 2.41.0 From 71b943f0e90335fac705f1851f608b2b5edb49b7 Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Tue, 25 Oct 2022 15:14:14 +0100 Subject: [PATCH 12/26] gvariant: Track checked and ordered offsets independently The past few commits introduced the concept of known-good offsets in the offset table (which is used for variable-width arrays and tuples). Good offsets are ones which are non-overlapping with all the previous offsets in the table. If a bad offset is encountered when indexing into the array or tuple, the cached known-good offset index will not be increased. In this way, all child variants at and beyond the first bad offset can be returned as default values rather than dereferencing potentially invalid data. In this case, there was no information about the fact that the indexes between the highest known-good index and the requested one had been checked already. That could lead to a pathological case where an offset table with an invalid first offset is repeatedly checked in full when trying to access higher-indexed children. Avoid that by storing the index of the highest checked offset in the table, as well as the index of the highest good/ordered offset. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Helps: #2121 --- glib/gvariant-core.c | 28 ++++++++++++++++++++++++ glib/gvariant-serialiser.c | 44 +++++++++++++++++++++++++++----------- glib/gvariant-serialiser.h | 9 ++++++++ glib/gvariant.c | 1 + glib/tests/gvariant.c | 5 +++++ 5 files changed, 75 insertions(+), 12 deletions(-) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index f11814d57..d9548be72 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -66,6 +66,7 @@ struct _GVariant GBytes *bytes; gconstpointer data; gsize ordered_offsets_up_to; + gsize checked_offsets_up_to; } serialised; struct @@ -181,6 +182,24 @@ struct _GVariant * This field is only relevant for arrays of non * fixed width types and for tuples. * + * .checked_offsets_up_to: Similarly to .ordered_offsets_up_to, this stores + * the index of the highest element, n, whose frame + * offsets (and all the preceding frame offsets) + * have been checked for validity. + * + * It is always the case that + * .checked_offsets_up_to ≥ .ordered_offsets_up_to. + * + * If .checked_offsets_up_to == .ordered_offsets_up_to, + * then a bad offset has not been found so far. + * + * If .checked_offsets_up_to > .ordered_offsets_up_to, + * then a bad offset has been found at + * (.ordered_offsets_up_to + 1). + * + * This field is only relevant for arrays of non + * fixed width types and for tuples. + * * .tree: Only valid when the instance is in tree form. * * Note that accesses from other threads could result in @@ -385,6 +404,7 @@ g_variant_to_serialised (GVariant *value) value->size, value->depth, value->contents.serialised.ordered_offsets_up_to, + value->contents.serialised.checked_offsets_up_to, }; return serialised; } @@ -417,6 +437,7 @@ g_variant_serialise (GVariant *value, serialised.data = data; serialised.depth = value->depth; serialised.ordered_offsets_up_to = 0; + serialised.checked_offsets_up_to = 0; children = (gpointer *) value->contents.tree.children; n_children = value->contents.tree.n_children; @@ -463,10 +484,12 @@ g_variant_fill_gvs (GVariantSerialised *serialised, if (value->state & STATE_SERIALISED) { serialised->ordered_offsets_up_to = value->contents.serialised.ordered_offsets_up_to; + serialised->checked_offsets_up_to = value->contents.serialised.checked_offsets_up_to; } else { serialised->ordered_offsets_up_to = 0; + serialised->checked_offsets_up_to = 0; } if (serialised->data) @@ -512,6 +535,7 @@ g_variant_ensure_serialised (GVariant *value) value->contents.serialised.data = g_bytes_get_data (bytes, NULL); value->contents.serialised.bytes = bytes; value->contents.serialised.ordered_offsets_up_to = G_MAXSIZE; + value->contents.serialised.checked_offsets_up_to = G_MAXSIZE; value->state |= STATE_SERIALISED; } } @@ -593,6 +617,7 @@ g_variant_new_from_bytes (const GVariantType *type, serialised.data = (guchar *) g_bytes_get_data (bytes, &serialised.size); serialised.depth = 0; serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0; + serialised.checked_offsets_up_to = trusted ? G_MAXSIZE : 0; if (!g_variant_serialised_check (serialised)) { @@ -643,6 +668,7 @@ g_variant_new_from_bytes (const GVariantType *type, } value->contents.serialised.ordered_offsets_up_to = trusted ? G_MAXSIZE : 0; + value->contents.serialised.checked_offsets_up_to = trusted ? G_MAXSIZE : 0; g_clear_pointer (&owned_bytes, g_bytes_unref); @@ -1130,6 +1156,7 @@ g_variant_get_child_value (GVariant *value, /* Update the cached ordered_offsets_up_to, since @serialised will be thrown away when this function exits */ value->contents.serialised.ordered_offsets_up_to = MAX (value->contents.serialised.ordered_offsets_up_to, serialised.ordered_offsets_up_to); + value->contents.serialised.checked_offsets_up_to = MAX (value->contents.serialised.checked_offsets_up_to, serialised.checked_offsets_up_to); /* Check whether this would cause nesting too deep. If so, return a fake * child. The only situation we expect this to happen in is with a variant, @@ -1157,6 +1184,7 @@ g_variant_get_child_value (GVariant *value, g_bytes_ref (value->contents.serialised.bytes); child->contents.serialised.data = s_child.data; child->contents.serialised.ordered_offsets_up_to = s_child.ordered_offsets_up_to; + child->contents.serialised.checked_offsets_up_to = s_child.checked_offsets_up_to; return child; } diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c index cd4a3e6e4..0bf7243f1 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -120,6 +120,8 @@ * * @depth has no restrictions; the depth of a top-level serialised #GVariant is * zero, and it increases for each level of nested child. + * + * @checked_offsets_up_to is always ≥ @ordered_offsets_up_to */ /* < private > @@ -147,6 +149,9 @@ g_variant_serialised_check (GVariantSerialised serialised) !(serialised.size == 0 || serialised.data != NULL)) return FALSE; + if (serialised.ordered_offsets_up_to > serialised.checked_offsets_up_to) + return FALSE; + /* Depending on the native alignment requirements of the machine, the * compiler will insert either 3 or 7 padding bytes after the char. * This will result in the sizeof() the struct being 12 or 16. @@ -266,6 +271,7 @@ gvs_fixed_sized_maybe_get_child (GVariantSerialised value, g_variant_type_info_ref (value.type_info); value.depth++; value.ordered_offsets_up_to = 0; + value.checked_offsets_up_to = 0; return value; } @@ -297,7 +303,7 @@ gvs_fixed_sized_maybe_serialise (GVariantSerialised value, { if (n_children) { - GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1, 0 }; + GVariantSerialised child = { NULL, value.data, value.size, value.depth + 1, 0, 0 }; gvs_filler (&child, children[0]); } @@ -320,6 +326,7 @@ gvs_fixed_sized_maybe_is_normal (GVariantSerialised value) value.type_info = g_variant_type_info_element (value.type_info); value.depth++; value.ordered_offsets_up_to = 0; + value.checked_offsets_up_to = 0; return g_variant_serialised_is_normal (value); } @@ -362,6 +369,7 @@ gvs_variable_sized_maybe_get_child (GVariantSerialised value, value.depth++; value.ordered_offsets_up_to = 0; + value.checked_offsets_up_to = 0; return value; } @@ -392,7 +400,7 @@ gvs_variable_sized_maybe_serialise (GVariantSerialised value, { if (n_children) { - GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1, 0 }; + GVariantSerialised child = { NULL, value.data, value.size - 1, value.depth + 1, 0, 0 }; /* write the data for the child. */ gvs_filler (&child, children[0]); @@ -413,6 +421,7 @@ gvs_variable_sized_maybe_is_normal (GVariantSerialised value) value.size--; value.depth++; value.ordered_offsets_up_to = 0; + value.checked_offsets_up_to = 0; return g_variant_serialised_is_normal (value); } @@ -739,39 +748,46 @@ gvs_variable_sized_array_get_child (GVariantSerialised value, /* If the requested @index_ is beyond the set of indices whose framing offsets * have been checked, check the remaining offsets to see whether they’re - * normal (in order, no overlapping array elements). */ - if (index_ > value.ordered_offsets_up_to) + * normal (in order, no overlapping array elements). + * + * Don’t bother checking if the highest known-good offset is lower than the + * highest checked offset, as that means there’s an invalid element at that + * index, so there’s no need to check further. */ + if (index_ > value.checked_offsets_up_to && + value.ordered_offsets_up_to == value.checked_offsets_up_to) { switch (offsets.offset_size) { case 1: { value.ordered_offsets_up_to = find_unordered_guint8 ( - offsets.array, value.ordered_offsets_up_to, index_ + 1); + offsets.array, value.checked_offsets_up_to, index_ + 1); break; } case 2: { value.ordered_offsets_up_to = find_unordered_guint16 ( - offsets.array, value.ordered_offsets_up_to, index_ + 1); + offsets.array, value.checked_offsets_up_to, index_ + 1); break; } case 4: { value.ordered_offsets_up_to = find_unordered_guint32 ( - offsets.array, value.ordered_offsets_up_to, index_ + 1); + offsets.array, value.checked_offsets_up_to, index_ + 1); break; } case 8: { value.ordered_offsets_up_to = find_unordered_guint64 ( - offsets.array, value.ordered_offsets_up_to, index_ + 1); + offsets.array, value.checked_offsets_up_to, index_ + 1); break; } default: /* gvs_get_offset_size() only returns maximum 8 */ g_assert_not_reached (); } + + value.checked_offsets_up_to = index_; } if (index_ > value.ordered_offsets_up_to) @@ -916,6 +932,7 @@ gvs_variable_sized_array_is_normal (GVariantSerialised value) /* All offsets have now been checked. */ value.ordered_offsets_up_to = G_MAXSIZE; + value.checked_offsets_up_to = G_MAXSIZE; return TRUE; } @@ -1040,14 +1057,15 @@ gvs_tuple_get_child (GVariantSerialised value, * all the tuple *elements* here, not just all the framing offsets, since * tuples contain a mix of elements which use framing offsets and ones which * don’t. None of them are allowed to overlap. */ - if (index_ > value.ordered_offsets_up_to) + if (index_ > value.checked_offsets_up_to && + value.ordered_offsets_up_to == value.checked_offsets_up_to) { gsize i, prev_i_end = 0; - if (value.ordered_offsets_up_to > 0) - gvs_tuple_get_member_bounds (value, value.ordered_offsets_up_to - 1, offset_size, NULL, &prev_i_end); + if (value.checked_offsets_up_to > 0) + gvs_tuple_get_member_bounds (value, value.checked_offsets_up_to - 1, offset_size, NULL, &prev_i_end); - for (i = value.ordered_offsets_up_to; i <= index_; i++) + for (i = value.checked_offsets_up_to; i <= index_; i++) { gsize i_start, i_end; @@ -1060,6 +1078,7 @@ gvs_tuple_get_child (GVariantSerialised value, } value.ordered_offsets_up_to = i - 1; + value.checked_offsets_up_to = index_; } if (index_ > value.ordered_offsets_up_to) @@ -1257,6 +1276,7 @@ gvs_tuple_is_normal (GVariantSerialised value) /* All element bounds have been checked above. */ value.ordered_offsets_up_to = G_MAXSIZE; + value.checked_offsets_up_to = G_MAXSIZE; { gsize fixed_size; diff --git a/glib/gvariant-serialiser.h b/glib/gvariant-serialiser.h index 765bd43b9..113df2ccc 100644 --- a/glib/gvariant-serialiser.h +++ b/glib/gvariant-serialiser.h @@ -41,6 +41,15 @@ typedef struct * Even when dealing with tuples, @ordered_offsets_up_to is an element index, * rather than an index into the frame offsets. */ gsize ordered_offsets_up_to; + + /* Similar to @ordered_offsets_up_to. This gives the index of the child element + * whose frame offset is the highest in the offset table which has been + * checked so far. + * + * This is always ≥ @ordered_offsets_up_to. It is always an element index. + * + * See documentation in gvariant-core.c for `struct GVariant` for details. */ + gsize checked_offsets_up_to; } GVariantSerialised; /* deserialisation */ diff --git a/glib/gvariant.c b/glib/gvariant.c index edd1a7a7f..fba90fd3f 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5923,6 +5923,7 @@ g_variant_byteswap (GVariant *value) serialised.data = g_malloc (serialised.size); serialised.depth = g_variant_get_depth (trusted); serialised.ordered_offsets_up_to = G_MAXSIZE; /* operating on the normal form */ + serialised.checked_offsets_up_to = G_MAXSIZE; g_variant_store (trusted, serialised.data); g_variant_unref (trusted); diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index d6690f3ea..f16fc2ccb 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -1285,6 +1285,7 @@ random_instance_filler (GVariantSerialised *serialised, serialised->depth = 0; serialised->ordered_offsets_up_to = 0; + serialised->checked_offsets_up_to = 0; g_assert (serialised->type_info == instance->type_info); g_assert (serialised->size == instance->size); @@ -1452,6 +1453,7 @@ test_maybe (void) serialised.size = needed_size; serialised.depth = 0; serialised.ordered_offsets_up_to = 0; + serialised.checked_offsets_up_to = 0; g_variant_serialiser_serialise (serialised, random_instance_filler, @@ -1576,6 +1578,7 @@ test_array (void) serialised.size = needed_size; serialised.depth = 0; serialised.ordered_offsets_up_to = 0; + serialised.checked_offsets_up_to = 0; g_variant_serialiser_serialise (serialised, random_instance_filler, (gpointer *) instances, n_children); @@ -1741,6 +1744,7 @@ test_tuple (void) serialised.size = needed_size; serialised.depth = 0; serialised.ordered_offsets_up_to = 0; + serialised.checked_offsets_up_to = 0; g_variant_serialiser_serialise (serialised, random_instance_filler, (gpointer *) instances, n_children); @@ -1838,6 +1842,7 @@ test_variant (void) serialised.size = needed_size; serialised.depth = 0; serialised.ordered_offsets_up_to = 0; + serialised.checked_offsets_up_to = 0; g_variant_serialiser_serialise (serialised, random_instance_filler, (gpointer *) &instance, 1); -- 2.41.0 From 26697b2c95b0d687dad40927a681c217d73011ad Mon Sep 17 00:00:00 2001 From: Philip Withnall <withnall@endlessm.com> Date: Fri, 12 Jun 2020 18:01:13 +0100 Subject: [PATCH 13/26] tests: Add another test for overlapping offsets in GVariant Signed-off-by: Philip Withnall <withnall@endlessm.com> Helps: #2121 --- glib/tests/gvariant.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index f16fc2ccb..fb56c83c0 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -4795,6 +4795,38 @@ static void test_recursion_limits_array_in_variant(void) { g_variant_unref(wrapper_variant); } +/* Test that a nested array with invalid values in its offset table (which point + * from the inner to the outer array) is normalised successfully without + * looping infinitely. */ +static void +test_normal_checking_array_offsets_overlapped (void) +{ + const guint8 data[] = { + 0x01, 0x00, + }; + gsize size = sizeof (data); + GVariant *variant = NULL; + GVariant *normal_variant = NULL; + GVariant *expected_variant = NULL; + + variant = g_variant_new_from_data (G_VARIANT_TYPE ("aay"), data, size, + FALSE, NULL, NULL); + g_assert_nonnull (variant); + + normal_variant = g_variant_get_normal_form (variant); + g_assert_nonnull (normal_variant); + + expected_variant = g_variant_new_parsed ("[@ay [], []]"); + g_assert_cmpvariant (normal_variant, expected_variant); + + g_assert_cmpmem (g_variant_get_data (normal_variant), g_variant_get_size (normal_variant), + g_variant_get_data (expected_variant), g_variant_get_size (expected_variant)); + + g_variant_unref (expected_variant); + g_variant_unref (normal_variant); + g_variant_unref (variant); +} + /* Test that an array with invalidly large values in its offset table is * normalised successfully without looping infinitely. */ static void @@ -5191,6 +5223,8 @@ main (int argc, char **argv) g_test_add_func ("/gvariant/normal-checking/tuples", test_normal_checking_tuples); + g_test_add_func ("/gvariant/normal-checking/array-offsets/overlapped", + test_normal_checking_array_offsets_overlapped); g_test_add_func ("/gvariant/normal-checking/array-offsets", test_normal_checking_array_offsets); g_test_add_func ("/gvariant/normal-checking/array-offsets2", -- 2.41.0 From 2a908e6e46f725022d5c4a332cfc1b7d43ec3d3b Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Mon, 24 Oct 2022 16:43:23 +0100 Subject: [PATCH 14/26] tests: Disable some random instance tests of GVariants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Building a `GVariant` using entirely random data may result in a non-normally-formed `GVariant`. It’s always possible to read these `GVariant`s, but the API might return default values for some or all of their components. In particular, this can easily happen when randomly generating the offset tables for non-fixed-width container types. If it does happen, bytewise comparison of the parsed `GVariant` with the original bytes will not always match. So skip those checks. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Helps: #2121 --- glib/tests/gvariant.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index fb56c83c0..67804e89d 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -1232,6 +1232,7 @@ random_instance_assert (RandomInstance *instance, GRand *rand; gint i; + g_assert_true (size == 0 || buffer != NULL); g_assert_cmpint ((gsize) buffer & ALIGN_BITS & instance->alignment, ==, 0); g_assert_cmpint (size, ==, instance->size); @@ -1458,10 +1459,13 @@ test_maybe (void) g_variant_serialiser_serialise (serialised, random_instance_filler, (gpointer *) &instance, 1); + child = g_variant_serialised_get_child (serialised, 0); - g_assert (child.type_info == instance->type_info); - random_instance_assert (instance, child.data, child.size); + g_assert_true (child.type_info == instance->type_info); + if (child.data != NULL) /* could be NULL if element is non-normal */ + random_instance_assert (instance, child.data, child.size); g_variant_type_info_unref (child.type_info); + flavoured_free (serialised.data, flavour); } } @@ -1593,8 +1597,9 @@ test_array (void) GVariantSerialised child; child = g_variant_serialised_get_child (serialised, i); - g_assert (child.type_info == instances[i]->type_info); - random_instance_assert (instances[i], child.data, child.size); + g_assert_true (child.type_info == instances[i]->type_info); + if (child.data != NULL) /* could be NULL if element is non-normal */ + random_instance_assert (instances[i], child.data, child.size); g_variant_type_info_unref (child.type_info); } @@ -1759,8 +1764,9 @@ test_tuple (void) GVariantSerialised child; child = g_variant_serialised_get_child (serialised, i); - g_assert (child.type_info == instances[i]->type_info); - random_instance_assert (instances[i], child.data, child.size); + g_assert_true (child.type_info == instances[i]->type_info); + if (child.data != NULL) /* could be NULL if element is non-normal */ + random_instance_assert (instances[i], child.data, child.size); g_variant_type_info_unref (child.type_info); } -- 2.41.0 From 9c5b9d37029091b84f0c0eb09434b6d47137c95b Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Mon, 24 Oct 2022 18:14:57 +0100 Subject: [PATCH 15/26] gvariant: Clarify the docs for g_variant_get_normal_form() Document how non-normal parts of the `GVariant` are handled. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> --- glib/gvariant.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index fba90fd3f..b6afaf540 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5855,7 +5855,9 @@ g_variant_deep_copy (GVariant *value) * marked as trusted and a new reference to it is returned. * * If @value is found not to be in normal form then a new trusted - * #GVariant is created with the same value as @value. + * #GVariant is created with the same value as @value. The non-normal parts of + * @value will be replaced with default values which are guaranteed to be in + * normal form. * * It makes sense to call this function if you've received #GVariant * data from untrusted sources and you want to ensure your serialised -- 2.41.0 From 8467cbbe2f2679f7d7abe6c80c10ab9884430b34 Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Mon, 24 Oct 2022 18:43:55 +0100 Subject: [PATCH 16/26] gvariant: Port g_variant_deep_copy() to count its iterations directly This is equivalent to what `GVariantIter` does, but it means that `g_variant_deep_copy()` is making its own `g_variant_get_child_value()` calls. This will be useful in an upcoming commit, where those child values will be inspected a little more deeply. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Helps: #2121 --- glib/gvariant.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index b6afaf540..2f283fe93 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5782,14 +5782,13 @@ g_variant_deep_copy (GVariant *value) case G_VARIANT_CLASS_VARIANT: { GVariantBuilder builder; - GVariantIter iter; - GVariant *child; + gsize i, n_children; g_variant_builder_init (&builder, g_variant_get_type (value)); - g_variant_iter_init (&iter, value); - while ((child = g_variant_iter_next_value (&iter))) + for (i = 0, n_children = g_variant_n_children (value); i < n_children; i++) { + GVariant *child = g_variant_get_child_value (value, i); g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); g_variant_unref (child); } -- 2.41.0 From d1b160e597fbf75824017d358e0b0009894144a1 Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Tue, 25 Oct 2022 13:03:22 +0100 Subject: [PATCH 17/26] gvariant: Add internal g_variant_maybe_get_child_value() This will be used in a following commit. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Helps: #2540 --- glib/gvariant-core.c | 68 ++++++++++++++++++++++++++++++++++++++++++++ glib/gvariant-core.h | 3 ++ 2 files changed, 71 insertions(+) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index d9548be72..03872cd46 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -1190,6 +1190,74 @@ g_variant_get_child_value (GVariant *value, } } +/** + * g_variant_maybe_get_child_value: + * @value: a container #GVariant + * @index_: the index of the child to fetch + * + * Reads a child item out of a container #GVariant instance, if it is in normal + * form. If it is not in normal form, return %NULL. + * + * This function behaves the same as g_variant_get_child_value(), except that it + * returns %NULL if the child is not in normal form. g_variant_get_child_value() + * would instead return a new default value of the correct type. + * + * This is intended to be used internally to avoid unnecessary #GVariant + * allocations. + * + * The returned value is never floating. You should free it with + * g_variant_unref() when you're done with it. + * + * This function is O(1). + * + * Returns: (transfer full): the child at the specified index + * + * Since: 2.74 + */ +GVariant * +g_variant_maybe_get_child_value (GVariant *value, + gsize index_) +{ + g_return_val_if_fail (index_ < g_variant_n_children (value), NULL); + g_return_val_if_fail (value->depth < G_MAXSIZE, NULL); + + if (~g_atomic_int_get (&value->state) & STATE_SERIALISED) + { + g_variant_lock (value); + + if (~value->state & STATE_SERIALISED) + { + GVariant *child; + + child = g_variant_ref (value->contents.tree.children[index_]); + g_variant_unlock (value); + + return child; + } + + g_variant_unlock (value); + } + + { + GVariantSerialised serialised = g_variant_to_serialised (value); + GVariantSerialised s_child; + + /* get the serializer to extract the serialized data for the child + * from the serialized data for the container + */ + s_child = g_variant_serialised_get_child (serialised, index_); + + if (!(value->state & STATE_TRUSTED) && s_child.data == NULL) + { + g_variant_type_info_unref (s_child.type_info); + return NULL; + } + + g_variant_type_info_unref (s_child.type_info); + return g_variant_get_child_value (value, index_); + } +} + /** * g_variant_store: * @value: the #GVariant to store diff --git a/glib/gvariant-core.h b/glib/gvariant-core.h index fc04711ac..947a98ca0 100644 --- a/glib/gvariant-core.h +++ b/glib/gvariant-core.h @@ -36,4 +36,7 @@ GVariantTypeInfo * g_variant_get_type_info (GVarian gsize g_variant_get_depth (GVariant *value); +GVariant * g_variant_maybe_get_child_value (GVariant *value, + gsize index_); + #endif /* __G_VARIANT_CORE_H__ */ -- 2.41.0 From d532b64b239650e2979458eaf032ddf67c8f38fd Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Tue, 25 Oct 2022 13:03:45 +0100 Subject: [PATCH 18/26] gvariant: Cut allocs of default values for children of non-normal arrays MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This improves a slow case in `g_variant_get_normal_form()` where allocating many identical default values for the children of a variable-sized array which has a malformed offset table would take a lot of time. The fix is to make all child values after the first invalid one be references to the default value emitted for the first invalid one, rather than identical new `GVariant`s. In particular, this fixes a case where an attacker could create an array of length L of very large tuples of size T each, corrupt the offset table so they don’t have to specify the array content, and then induce `g_variant_get_normal_form()` into allocating L×T default values from an input which is significantly smaller than L×T in length. A pre-existing workaround for this issue is for code to call `g_variant_is_normal_form()` before calling `g_variant_get_normal_form()`, and to skip the latter call if the former returns false. This commit improves the behaviour in the case that `g_variant_get_normal_form()` is called anyway. This fix changes the time to run the `fuzz_variant_binary` test on the testcase from oss-fuzz#19777 from >60s (before being terminated) with 2.3GB of memory usage and 580k page faults; to 32s, 8.3MB of memory usage and 1500 page faults (as measured by `time -v`). Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Fixes: #2540 oss-fuzz#19777 --- glib/gvariant.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index 2f283fe93..d8ee37bc3 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5776,7 +5776,6 @@ g_variant_deep_copy (GVariant *value) switch (g_variant_classify (value)) { case G_VARIANT_CLASS_MAYBE: - case G_VARIANT_CLASS_ARRAY: case G_VARIANT_CLASS_TUPLE: case G_VARIANT_CLASS_DICT_ENTRY: case G_VARIANT_CLASS_VARIANT: @@ -5796,6 +5795,71 @@ g_variant_deep_copy (GVariant *value) return g_variant_builder_end (&builder); } + case G_VARIANT_CLASS_ARRAY: + { + GVariantBuilder builder; + gsize i, n_children; + GVariant *first_invalid_child_deep_copy = NULL; + + /* Arrays are in theory treated the same as maybes, tuples, dict entries + * and variants, and could be another case in the above block of code. + * + * However, they have the property that when dealing with non-normal + * data (which is the only time g_variant_deep_copy() is currently + * called) in a variable-sized array, the code above can easily end up + * creating many default child values in order to return an array which + * is of the right length and type, but without containing non-normal + * data. This can happen if the offset table for the array is malformed. + * + * In this case, the code above would end up allocating the same default + * value for each one of the child indexes beyond the first malformed + * entry in the offset table. This can end up being a lot of identical + * allocations of default values, particularly if the non-normal array + * is crafted maliciously. + * + * Avoid that problem by returning a new reference to the same default + * value for every child after the first invalid one. This results in + * returning an equivalent array, in normal form and trusted — but with + * significantly fewer memory allocations. + * + * See https://gitlab.gnome.org/GNOME/glib/-/issues/2540 */ + + g_variant_builder_init (&builder, g_variant_get_type (value)); + + for (i = 0, n_children = g_variant_n_children (value); i < n_children; i++) + { + /* Try maybe_get_child_value() first; if it returns NULL, this child + * is non-normal. get_child_value() would have constructed and + * returned a default value in that case. */ + GVariant *child = g_variant_maybe_get_child_value (value, i); + + if (child != NULL) + { + /* Non-normal children may not always be contiguous, as they may + * be non-normal for reasons other than invalid offset table + * entries. As they are all the same type, they will all have + * the same default value though, so keep that around. */ + g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); + } + else if (child == NULL && first_invalid_child_deep_copy != NULL) + { + g_variant_builder_add_value (&builder, first_invalid_child_deep_copy); + } + else if (child == NULL) + { + child = g_variant_get_child_value (value, i); + first_invalid_child_deep_copy = g_variant_ref_sink (g_variant_deep_copy (child)); + g_variant_builder_add_value (&builder, first_invalid_child_deep_copy); + } + + g_clear_pointer (&child, g_variant_unref); + } + + g_clear_pointer (&first_invalid_child_deep_copy, g_variant_unref); + + return g_variant_builder_end (&builder); + } + case G_VARIANT_CLASS_BOOLEAN: return g_variant_new_boolean (g_variant_get_boolean (value)); -- 2.41.0 From ee7db218f47b8eaa15ea9ae9b2c923a1d995c017 Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Tue, 25 Oct 2022 18:03:56 +0100 Subject: [PATCH 19/26] gvariant: Fix a leak of a GVariantTypeInfo on an error handling path Signed-off-by: Philip Withnall <pwithnall@endlessos.org> --- glib/gvariant-core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index 03872cd46..7a034cc6f 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -1169,6 +1169,7 @@ g_variant_get_child_value (GVariant *value, G_VARIANT_MAX_RECURSION_DEPTH - value->depth) { g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE_VARIANT)); + g_variant_type_info_unref (s_child.type_info); return g_variant_new_tuple (NULL, 0); } -- 2.41.0 From c2feb5c5e9d52d46296be5c5ca266d341218d78e Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Thu, 27 Oct 2022 12:00:04 +0100 Subject: [PATCH 20/26] gvariant-serialiser: Check offset table entry size is minimal The entries in an offset table (which is used for variable sized arrays and tuples containing variable sized members) are sized so that they can address every byte in the overall variant. The specification requires that for a variant to be in normal form, its offset table entries must be the minimum width such that they can address every byte in the variant. That minimality requirement was not checked in `g_variant_is_normal_form()`, leading to two different byte arrays being interpreted as the normal form of a given variant tree. That kind of confusion could potentially be exploited, and is certainly a bug. Fix it by adding the necessary checks on offset table entry width, and unit tests. Spotted by William Manley. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Fixes: #2794 --- glib/gvariant-serialiser.c | 19 +++- glib/tests/gvariant.c | 176 +++++++++++++++++++++++++++++++++++++ 2 files changed, 194 insertions(+), 1 deletion(-) diff --git a/glib/gvariant-serialiser.c b/glib/gvariant-serialiser.c index 0bf7243f1..5aa2cbc42 100644 --- a/glib/gvariant-serialiser.c +++ b/glib/gvariant-serialiser.c @@ -694,6 +694,10 @@ gvs_variable_sized_array_get_frame_offsets (GVariantSerialised value) out.data_size = last_end; out.array = value.data + last_end; out.length = offsets_array_size / out.offset_size; + + if (out.length > 0 && gvs_calculate_total_size (last_end, out.length) != value.size) + return out; /* offset size not minimal */ + out.is_normal = TRUE; return out; @@ -1201,6 +1205,7 @@ gvs_tuple_is_normal (GVariantSerialised value) gsize length; gsize offset; gsize i; + gsize offset_table_size; /* as per the comment in gvs_tuple_get_child() */ if G_UNLIKELY (value.data == NULL && value.size != 0) @@ -1305,7 +1310,19 @@ gvs_tuple_is_normal (GVariantSerialised value) } } - return offset_ptr == offset; + /* @offset_ptr has been counting backwards from the end of the variant, to + * find the beginning of the offset table. @offset has been counting forwards + * from the beginning of the variant to find the end of the data. They should + * have met in the middle. */ + if (offset_ptr != offset) + return FALSE; + + offset_table_size = value.size - offset_ptr; + if (value.size > 0 && + gvs_calculate_total_size (offset, offset_table_size / offset_size) != value.size) + return FALSE; /* offset size not minimal */ + + return TRUE; } /* Variants {{{2 diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index 67804e89d..9671b459a 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -4898,6 +4898,86 @@ test_normal_checking_array_offsets2 (void) g_variant_unref (variant); } +/* Test that an otherwise-valid serialised GVariant is considered non-normal if + * its offset table entries are too wide. + * + * See §2.3.6 (Framing Offsets) of the GVariant specification. */ +static void +test_normal_checking_array_offsets_minimal_sized (void) +{ + GVariantBuilder builder; + gsize i; + GVariant *aay_constructed = NULL; + const guint8 *data = NULL; + guint8 *data_owned = NULL; + GVariant *aay_deserialised = NULL; + GVariant *aay_normalised = NULL; + + /* Construct an array of type aay, consisting of 128 elements which are each + * an empty array, i.e. `[[] * 128]`. This is chosen because the inner + * elements are variable sized (making the outer array variable sized, so it + * must have an offset table), but they are also zero-sized when serialised. + * So the serialised representation of @aay_constructed consists entirely of + * its offset table, which is entirely zeroes. + * + * The array is chosen to be 128 elements long because that means offset + * table entries which are 1 byte long. If the elements in the array were + * non-zero-sized (to the extent that the overall array is ≥256 bytes long), + * the offset table entries would end up being 2 bytes long. */ + g_variant_builder_init (&builder, G_VARIANT_TYPE ("aay")); + + for (i = 0; i < 128; i++) + g_variant_builder_add_value (&builder, g_variant_new_array (G_VARIANT_TYPE_BYTE, NULL, 0)); + + aay_constructed = g_variant_builder_end (&builder); + + /* Verify that the constructed array is in normal form, and its serialised + * form is `b'\0' * 128`. */ + g_assert_true (g_variant_is_normal_form (aay_constructed)); + g_assert_cmpuint (g_variant_n_children (aay_constructed), ==, 128); + g_assert_cmpuint (g_variant_get_size (aay_constructed), ==, 128); + + data = g_variant_get_data (aay_constructed); + for (i = 0; i < g_variant_get_size (aay_constructed); i++) + g_assert_cmpuint (data[i], ==, 0); + + /* Construct a serialised `aay` GVariant which is `b'\0' * 256`. This has to + * be a non-normal form of `[[] * 128]`, with 2-byte-long offset table + * entries, because each offset table entry has to be able to reference all of + * the byte boundaries in the container. All the entries in the offset table + * are zero, so all the elements of the array are zero-sized. */ + data = data_owned = g_malloc0 (256); + aay_deserialised = g_variant_new_from_data (G_VARIANT_TYPE ("aay"), + data, + 256, + FALSE, + g_free, + g_steal_pointer (&data_owned)); + + g_assert_false (g_variant_is_normal_form (aay_deserialised)); + g_assert_cmpuint (g_variant_n_children (aay_deserialised), ==, 128); + g_assert_cmpuint (g_variant_get_size (aay_deserialised), ==, 256); + + data = g_variant_get_data (aay_deserialised); + for (i = 0; i < g_variant_get_size (aay_deserialised); i++) + g_assert_cmpuint (data[i], ==, 0); + + /* Get its normal form. That should change the serialised size. */ + aay_normalised = g_variant_get_normal_form (aay_deserialised); + + g_assert_true (g_variant_is_normal_form (aay_normalised)); + g_assert_cmpuint (g_variant_n_children (aay_normalised), ==, 128); + g_assert_cmpuint (g_variant_get_size (aay_normalised), ==, 128); + + data = g_variant_get_data (aay_normalised); + for (i = 0; i < g_variant_get_size (aay_normalised); i++) + g_assert_cmpuint (data[i], ==, 0); + + g_variant_unref (aay_normalised); + g_variant_unref (aay_deserialised); + g_variant_unref (aay_constructed); +} + /* Test that a tuple with invalidly large values in its offset table is * normalised successfully without looping infinitely. */ static void @@ -5092,6 +5172,98 @@ test_normal_checking_tuple_offsets4 (void) g_variant_unref (variant); } +/* Test that an otherwise-valid serialised GVariant is considered non-normal if + * its offset table entries are too wide. + * + * See §2.3.6 (Framing Offsets) of the GVariant specification. */ +static void +test_normal_checking_tuple_offsets_minimal_sized (void) +{ + GString *type_string = NULL; + GVariantBuilder builder; + gsize i; + GVariant *ray_constructed = NULL; + const guint8 *data = NULL; + guint8 *data_owned = NULL; + GVariant *ray_deserialised = NULL; + GVariant *ray_normalised = NULL; + + /* Construct a tuple of type (ay…ay), consisting of 129 members which are each + * an empty array, i.e. `([] * 129)`. This is chosen because the inner + * members are variable sized, so the outer tuple must have an offset table, + * but they are also zero-sized when serialised. So the serialised + * representation of @ray_constructed consists entirely of its offset table, + * which is entirely zeroes. + * + * The tuple is chosen to be 129 members long because that means it has 128 + * offset table entries which are 1 byte long each. If the members in the + * tuple were non-zero-sized (to the extent that the overall tuple is ≥256 + * bytes long), the offset table entries would end up being 2 bytes long. + * + * 129 members are used unlike 128 array elements in + * test_normal_checking_array_offsets_minimal_sized(), because the last member + * in a tuple never needs an offset table entry. */ + type_string = g_string_new (""); + g_string_append_c (type_string, '('); + for (i = 0; i < 129; i++) + g_string_append (type_string, "ay"); + g_string_append_c (type_string, ')'); + + g_variant_builder_init (&builder, G_VARIANT_TYPE (type_string->str)); + + for (i = 0; i < 129; i++) + g_variant_builder_add_value (&builder, g_variant_new_array (G_VARIANT_TYPE_BYTE, NULL, 0)); + + ray_constructed = g_variant_builder_end (&builder); + + /* Verify that the constructed tuple is in normal form, and its serialised + * form is `b'\0' * 128`. */ + g_assert_true (g_variant_is_normal_form (ray_constructed)); + g_assert_cmpuint (g_variant_n_children (ray_constructed), ==, 129); + g_assert_cmpuint (g_variant_get_size (ray_constructed), ==, 128); + + data = g_variant_get_data (ray_constructed); + for (i = 0; i < g_variant_get_size (ray_constructed); i++) + g_assert_cmpuint (data[i], ==, 0); + + /* Construct a serialised `(ay…ay)` GVariant which is `b'\0' * 256`. This has + * to be a non-normal form of `([] * 129)`, with 2-byte-long offset table + * entries, because each offset table entry has to be able to reference all of + * the byte boundaries in the container. All the entries in the offset table + * are zero, so all the members of the tuple are zero-sized. */ + data = data_owned = g_malloc0 (256); + ray_deserialised = g_variant_new_from_data (G_VARIANT_TYPE (type_string->str), + data, + 256, + FALSE, + g_free, + g_steal_pointer (&data_owned)); + + g_assert_false (g_variant_is_normal_form (ray_deserialised)); + g_assert_cmpuint (g_variant_n_children (ray_deserialised), ==, 129); + g_assert_cmpuint (g_variant_get_size (ray_deserialised), ==, 256); + + data = g_variant_get_data (ray_deserialised); + for (i = 0; i < g_variant_get_size (ray_deserialised); i++) + g_assert_cmpuint (data[i], ==, 0); + + /* Get its normal form. That should change the serialised size. */ + ray_normalised = g_variant_get_normal_form (ray_deserialised); + + g_assert_true (g_variant_is_normal_form (ray_normalised)); + g_assert_cmpuint (g_variant_n_children (ray_normalised), ==, 129); + g_assert_cmpuint (g_variant_get_size (ray_normalised), ==, 128); + + data = g_variant_get_data (ray_normalised); + for (i = 0; i < g_variant_get_size (ray_normalised); i++) + g_assert_cmpuint (data[i], ==, 0); + + g_variant_unref (ray_normalised); + g_variant_unref (ray_deserialised); + g_variant_unref (ray_constructed); + g_string_free (type_string, TRUE); +} + /* Test that an empty object path is normalised successfully to the base object * path, ‘/’. */ static void @@ -5235,6 +5407,8 @@ main (int argc, char **argv) test_normal_checking_array_offsets); g_test_add_func ("/gvariant/normal-checking/array-offsets2", test_normal_checking_array_offsets2); + g_test_add_func ("/gvariant/normal-checking/array-offsets/minimal-sized", + test_normal_checking_array_offsets_minimal_sized); g_test_add_func ("/gvariant/normal-checking/tuple-offsets", test_normal_checking_tuple_offsets); g_test_add_func ("/gvariant/normal-checking/tuple-offsets2", @@ -5243,6 +5417,8 @@ main (int argc, char **argv) test_normal_checking_tuple_offsets3); g_test_add_func ("/gvariant/normal-checking/tuple-offsets4", test_normal_checking_tuple_offsets4); + g_test_add_func ("/gvariant/normal-checking/tuple-offsets/minimal-sized", + test_normal_checking_tuple_offsets_minimal_sized); g_test_add_func ("/gvariant/normal-checking/empty-object-path", test_normal_checking_empty_object_path); -- 2.41.0 From 54f2d04f299a2cc9f7403befd97b25591e8ce263 Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Thu, 27 Oct 2022 16:13:54 +0100 Subject: [PATCH 21/26] gvariant: Fix g_variant_byteswap() returning non-normal data sometimes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If `g_variant_byteswap()` was called on a non-normal variant of a type which doesn’t need byteswapping, it would return a non-normal output. That contradicts the documentation, which says that the return value is always in normal form. Fix the code so it matches the documentation. Includes a unit test. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Helps: #2797 --- glib/gvariant.c | 8 +++++--- glib/tests/gvariant.c | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index d8ee37bc3..1fd8366bd 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5995,14 +5995,16 @@ g_variant_byteswap (GVariant *value) g_variant_serialised_byteswap (serialised); bytes = g_bytes_new_take (serialised.data, serialised.size); - new = g_variant_new_from_bytes (g_variant_get_type (value), bytes, TRUE); + new = g_variant_ref_sink (g_variant_new_from_bytes (g_variant_get_type (value), bytes, TRUE)); g_bytes_unref (bytes); } else /* contains no multi-byte data */ - new = value; + new = g_variant_get_normal_form (value); - return g_variant_ref_sink (new); + g_assert (g_variant_is_trusted (new)); + + return g_steal_pointer (&new); } /** diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index 9671b459a..954a5c6c1 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -3724,6 +3724,29 @@ test_gv_byteswap (void) g_free (string); } +static void +test_gv_byteswap_non_normal_non_aligned (void) +{ + const guint8 data[] = { 0x02 }; + GVariant *v = NULL; + GVariant *v_byteswapped = NULL; + + g_test_summary ("Test that calling g_variant_byteswap() on a variant which " + "is in non-normal form and doesn’t need byteswapping returns " + "the same variant in normal form."); + + v = g_variant_new_from_data (G_VARIANT_TYPE_BOOLEAN, data, sizeof (data), FALSE, NULL, NULL); + g_assert_false (g_variant_is_normal_form (v)); + + v_byteswapped = g_variant_byteswap (v); + g_assert_true (g_variant_is_normal_form (v_byteswapped)); + + g_assert_cmpvariant (v, v_byteswapped); + + g_variant_unref (v); + g_variant_unref (v_byteswapped); +} + static void test_parser (void) { @@ -5374,6 +5397,7 @@ main (int argc, char **argv) g_test_add_func ("/gvariant/builder-memory", test_builder_memory); g_test_add_func ("/gvariant/hashing", test_hashing); g_test_add_func ("/gvariant/byteswap", test_gv_byteswap); + g_test_add_func ("/gvariant/byteswap/non-normal-non-aligned", test_gv_byteswap_non_normal_non_aligned); g_test_add_func ("/gvariant/parser", test_parses); g_test_add_func ("/gvariant/parse-failures", test_parse_failures); g_test_add_func ("/gvariant/parse-positional", test_parse_positional); -- 2.41.0 From 940402d60c3638c13547590b96b9bf7d31f0b875 Mon Sep 17 00:00:00 2001 From: Philip Withnall <pwithnall@endlessos.org> Date: Thu, 27 Oct 2022 22:53:13 +0100 Subject: [PATCH 22/26] gvariant: Allow g_variant_byteswap() to operate on tree-form variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This avoids needing to always serialise a variant before byteswapping it. With variants in non-normal forms, serialisation can result in a large increase in size of the variant, and a lot of allocations for leaf `GVariant`s. This can lead to a denial of service attack. Avoid that by changing byteswapping so that it happens on the tree form of the variant if the input is in non-normal form. If the input is in normal form (either serialised or in tree form), continue using the existing code as byteswapping an already-serialised normal variant is about 3× faster than byteswapping on the equivalent tree form. The existing unit tests cover byteswapping well, but need some adaptation so that they operate on tree form variants too. I considered dropping the serialised byteswapping code and doing all byteswapping on tree-form variants, as that would make maintenance simpler (avoiding having two parallel implementations of byteswapping). However, most inputs to `g_variant_byteswap()` are likely to be serialised variants (coming from a byte array of input from some foreign source) and most of them are going to be in normal form (as corruption and malicious action are rare). So getting rid of the serialised byteswapping code would impose quite a performance penalty on the common case. Signed-off-by: Philip Withnall <pwithnall@endlessos.org> Fixes: #2797 --- glib/gvariant.c | 87 ++++++++++++++++++++++++++++++++----------- glib/tests/gvariant.c | 57 ++++++++++++++++++++++++---- 2 files changed, 115 insertions(+), 29 deletions(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index 1fd8366bd..9547d960f 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -5771,7 +5771,8 @@ g_variant_iter_loop (GVariantIter *iter, /* Serialised data {{{1 */ static GVariant * -g_variant_deep_copy (GVariant *value) +g_variant_deep_copy (GVariant *value, + gboolean byteswap) { switch (g_variant_classify (value)) { @@ -5788,7 +5789,7 @@ g_variant_deep_copy (GVariant *value) for (i = 0, n_children = g_variant_n_children (value); i < n_children; i++) { GVariant *child = g_variant_get_child_value (value, i); - g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); + g_variant_builder_add_value (&builder, g_variant_deep_copy (child, byteswap)); g_variant_unref (child); } @@ -5839,7 +5840,7 @@ g_variant_deep_copy (GVariant *value) * be non-normal for reasons other than invalid offset table * entries. As they are all the same type, they will all have * the same default value though, so keep that around. */ - g_variant_builder_add_value (&builder, g_variant_deep_copy (child)); + g_variant_builder_add_value (&builder, g_variant_deep_copy (child, byteswap)); } else if (child == NULL && first_invalid_child_deep_copy != NULL) { @@ -5848,7 +5849,7 @@ g_variant_deep_copy (GVariant *value) else if (child == NULL) { child = g_variant_get_child_value (value, i); - first_invalid_child_deep_copy = g_variant_ref_sink (g_variant_deep_copy (child)); + first_invalid_child_deep_copy = g_variant_ref_sink (g_variant_deep_copy (child, byteswap)); g_variant_builder_add_value (&builder, first_invalid_child_deep_copy); } @@ -5867,28 +5868,63 @@ g_variant_deep_copy (GVariant *value) return g_variant_new_byte (g_variant_get_byte (value)); case G_VARIANT_CLASS_INT16: - return g_variant_new_int16 (g_variant_get_int16 (value)); + if (byteswap) + return g_variant_new_int16 (GUINT16_SWAP_LE_BE (g_variant_get_int16 (value))); + else + return g_variant_new_int16 (g_variant_get_int16 (value)); case G_VARIANT_CLASS_UINT16: - return g_variant_new_uint16 (g_variant_get_uint16 (value)); + if (byteswap) + return g_variant_new_uint16 (GUINT16_SWAP_LE_BE (g_variant_get_uint16 (value))); + else + return g_variant_new_uint16 (g_variant_get_uint16 (value)); case G_VARIANT_CLASS_INT32: - return g_variant_new_int32 (g_variant_get_int32 (value)); + if (byteswap) + return g_variant_new_int32 (GUINT32_SWAP_LE_BE (g_variant_get_int32 (value))); + else + return g_variant_new_int32 (g_variant_get_int32 (value)); case G_VARIANT_CLASS_UINT32: - return g_variant_new_uint32 (g_variant_get_uint32 (value)); + if (byteswap) + return g_variant_new_uint32 (GUINT32_SWAP_LE_BE (g_variant_get_uint32 (value))); + else + return g_variant_new_uint32 (g_variant_get_uint32 (value)); case G_VARIANT_CLASS_INT64: - return g_variant_new_int64 (g_variant_get_int64 (value)); + if (byteswap) + return g_variant_new_int64 (GUINT64_SWAP_LE_BE (g_variant_get_int64 (value))); + else + return g_variant_new_int64 (g_variant_get_int64 (value)); case G_VARIANT_CLASS_UINT64: - return g_variant_new_uint64 (g_variant_get_uint64 (value)); + if (byteswap) + return g_variant_new_uint64 (GUINT64_SWAP_LE_BE (g_variant_get_uint64 (value))); + else + return g_variant_new_uint64 (g_variant_get_uint64 (value)); case G_VARIANT_CLASS_HANDLE: - return g_variant_new_handle (g_variant_get_handle (value)); + if (byteswap) + return g_variant_new_handle (GUINT32_SWAP_LE_BE (g_variant_get_handle (value))); + else + return g_variant_new_handle (g_variant_get_handle (value)); case G_VARIANT_CLASS_DOUBLE: - return g_variant_new_double (g_variant_get_double (value)); + if (byteswap) + { + /* We have to convert the double to a uint64 here using a union, + * because a cast will round it numerically. */ + union + { + guint64 u64; + gdouble dbl; + } u1, u2; + u1.dbl = g_variant_get_double (value); + u2.u64 = GUINT64_SWAP_LE_BE (u1.u64); + return g_variant_new_double (u2.dbl); + } + else + return g_variant_new_double (g_variant_get_double (value)); case G_VARIANT_CLASS_STRING: return g_variant_new_string (g_variant_get_string (value, NULL)); @@ -5938,7 +5974,7 @@ g_variant_get_normal_form (GVariant *value) if (g_variant_is_normal_form (value)) return g_variant_ref (value); - trusted = g_variant_deep_copy (value); + trusted = g_variant_deep_copy (value, FALSE); g_assert (g_variant_is_trusted (trusted)); return g_variant_ref_sink (trusted); @@ -5958,6 +5994,11 @@ g_variant_get_normal_form (GVariant *value) * contain multi-byte numeric data. That include strings, booleans, * bytes and containers containing only these things (recursively). * + * While this function can safely handle untrusted, non-normal data, it is + * recommended to check whether the input is in normal form beforehand, using + * g_variant_is_normal_form(), and to reject non-normal inputs if your + * application can be strict about what inputs it rejects. + * * The returned value is always in normal form and is marked as trusted. * * Returns: (transfer full): the byteswapped form of @value @@ -5975,22 +6016,21 @@ g_variant_byteswap (GVariant *value) g_variant_type_info_query (type_info, &alignment, NULL); - if (alignment) - /* (potentially) contains multi-byte numeric data */ + if (alignment && g_variant_is_normal_form (value)) { + /* (potentially) contains multi-byte numeric data, but is also already in + * normal form so we can use a faster byteswapping codepath on the + * serialised data */ GVariantSerialised serialised = { 0, }; - GVariant *trusted; GBytes *bytes; - trusted = g_variant_get_normal_form (value); - serialised.type_info = g_variant_get_type_info (trusted); - serialised.size = g_variant_get_size (trusted); + serialised.type_info = g_variant_get_type_info (value); + serialised.size = g_variant_get_size (value); serialised.data = g_malloc (serialised.size); - serialised.depth = g_variant_get_depth (trusted); + serialised.depth = g_variant_get_depth (value); serialised.ordered_offsets_up_to = G_MAXSIZE; /* operating on the normal form */ serialised.checked_offsets_up_to = G_MAXSIZE; - g_variant_store (trusted, serialised.data); - g_variant_unref (trusted); + g_variant_store (value, serialised.data); g_variant_serialised_byteswap (serialised); @@ -5998,6 +6038,9 @@ g_variant_byteswap (GVariant *value) new = g_variant_ref_sink (g_variant_new_from_bytes (g_variant_get_type (value), bytes, TRUE)); g_bytes_unref (bytes); } + else if (alignment) + /* (potentially) contains multi-byte numeric data */ + new = g_variant_ref_sink (g_variant_deep_copy (value, TRUE)); else /* contains no multi-byte data */ new = g_variant_get_normal_form (value); diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index 954a5c6c1..88df708ad 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -2289,24 +2289,67 @@ serialise_tree (TreeInstance *tree, static void test_byteswap (void) { - GVariantSerialised one = { 0, }, two = { 0, }; + GVariantSerialised one = { 0, }, two = { 0, }, three = { 0, }; TreeInstance *tree; - + GVariant *one_variant = NULL; + GVariant *two_variant = NULL; + GVariant *two_byteswapped = NULL; + GVariant *three_variant = NULL; + GVariant *three_byteswapped = NULL; + guint8 *three_data_copy = NULL; + gsize three_size_copy = 0; + + /* Write a tree out twice, once normally and once byteswapped. */ tree = tree_instance_new (NULL, 3); serialise_tree (tree, &one); + one_variant = g_variant_new_from_data (G_VARIANT_TYPE (g_variant_type_info_get_type_string (one.type_info)), + one.data, one.size, FALSE, NULL, NULL); + i_am_writing_byteswapped = TRUE; serialise_tree (tree, &two); + serialise_tree (tree, &three); i_am_writing_byteswapped = FALSE; - g_variant_serialised_byteswap (two); - - g_assert_cmpmem (one.data, one.size, two.data, two.size); - g_assert_cmpuint (one.depth, ==, two.depth); - + /* Swap the first byteswapped one back using the function we want to test. */ + two_variant = g_variant_new_from_data (G_VARIANT_TYPE (g_variant_type_info_get_type_string (two.type_info)), + two.data, two.size, FALSE, NULL, NULL); + two_byteswapped = g_variant_byteswap (two_variant); + + /* Make the second byteswapped one non-normal (hopefully), and then byteswap + * it back using the function we want to test in its non-normal mode. + * This might not work because it’s not necessarily possible to make an + * arbitrary random variant non-normal. Adding a single zero byte to the end + * often makes something non-normal but still readable. */ + three_size_copy = three.size + 1; + three_data_copy = g_malloc (three_size_copy); + memcpy (three_data_copy, three.data, three.size); + three_data_copy[three.size] = '\0'; + + three_variant = g_variant_new_from_data (G_VARIANT_TYPE (g_variant_type_info_get_type_string (three.type_info)), + three_data_copy, three_size_copy, FALSE, NULL, NULL); + three_byteswapped = g_variant_byteswap (three_variant); + + /* Check they’re the same. We can always compare @one_variant and + * @two_byteswapped. We can only compare @two_byteswapped and + * @three_byteswapped if @two_variant and @three_variant are equal: in that + * case, the corruption to @three_variant was enough to make it non-normal but + * not enough to change its value. */ + g_assert_cmpvariant (one_variant, two_byteswapped); + + if (g_variant_equal (two_variant, three_variant)) + g_assert_cmpvariant (two_byteswapped, three_byteswapped); + + g_variant_unref (three_byteswapped); + g_variant_unref (three_variant); + g_variant_unref (two_byteswapped); + g_variant_unref (two_variant); + g_variant_unref (one_variant); tree_instance_free (tree); g_free (one.data); g_free (two.data); + g_free (three.data); + g_free (three_data_copy); } static void -- 2.41.0 From 0ae89685a14844dc81cdc0a6085646f39be0fa2c Mon Sep 17 00:00:00 2001 From: Yu Daike <yu.daike@suse.com> Date: Mon, 14 Aug 2023 20:49:07 +0800 Subject: [PATCH 23/26] Add glib 2.60 version macro for backporting --- glib/gversionmacros.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/glib/gversionmacros.h b/glib/gversionmacros.h index c23c40247..942b7cdef 100644 --- a/glib/gversionmacros.h +++ b/glib/gversionmacros.h @@ -185,6 +185,16 @@ */ #define GLIB_VERSION_2_54 (G_ENCODE_VERSION (2, 54)) +/** + * GLIB_VERSION_2_60: + * + * A macro that evaluates to the 2.60 version of GLib, in a format + * that can be used by the C pre-processor. + * + * Since: 2.60 + */ +#define GLIB_VERSION_2_60 (G_ENCODE_VERSION (2, 60)) + /* evaluates to the current stable version; for development cycles, * this means the next stable target */ @@ -486,4 +496,10 @@ # define GLIB_AVAILABLE_IN_2_54 _GLIB_EXTERN #endif +#if GLIB_VERSION_MAX_ALLOWED < GLIB_VERSION_2_60 +# define GLIB_AVAILABLE_IN_2_60 GLIB_UNAVAILABLE(2, 60) +#else +# define GLIB_AVAILABLE_IN_2_60 _GLIB_EXTERN +#endif + #endif /* __G_VERSION_MACROS_H__ */ -- 2.41.0 From f8e53d38c7a59e3b9e5c3ba4b9bbf7b83789cff0 Mon Sep 17 00:00:00 2001 From: Biswapriyo Nath <nathbappai@gmail.com> Date: Sat, 23 Apr 2022 14:02:10 +0530 Subject: [PATCH 24/26] gtestutils: Include stdlib.h for exit function This fixes warning: implicit declaration of function 'exit' --- glib/gtestutils.h | 1 + 1 file changed, 1 insertion(+) diff --git a/glib/gtestutils.h b/glib/gtestutils.h index ae74aba1d..e61f26e7f 100644 --- a/glib/gtestutils.h +++ b/glib/gtestutils.h @@ -27,6 +27,7 @@ #include <glib/gstring.h> #include <glib/gerror.h> #include <glib/gslist.h> +#include <stdlib.h> G_BEGIN_DECLS -- 2.41.0 From 42b1fc28572944b2e5c8d544480e3b1c2099f65e Mon Sep 17 00:00:00 2001 From: Yu Daike <yu.daike@suse.com> Date: Wed, 16 Aug 2023 14:01:14 +0800 Subject: [PATCH 25/26] Replace g_assert_cmpvariant --- glib/tests/gvariant.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index 88df708ad..b388a3dbd 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -2335,10 +2335,10 @@ test_byteswap (void) * @three_byteswapped if @two_variant and @three_variant are equal: in that * case, the corruption to @three_variant was enough to make it non-normal but * not enough to change its value. */ - g_assert_cmpvariant (one_variant, two_byteswapped); + g_assert (g_variant_equal (one_variant, two_byteswapped)); if (g_variant_equal (two_variant, three_variant)) - g_assert_cmpvariant (two_byteswapped, three_byteswapped); + g_assert (g_variant_equal (two_byteswapped, three_byteswapped)); g_variant_unref (three_byteswapped); g_variant_unref (three_variant); @@ -3784,7 +3784,7 @@ test_gv_byteswap_non_normal_non_aligned (void) v_byteswapped = g_variant_byteswap (v); g_assert_true (g_variant_is_normal_form (v_byteswapped)); - g_assert_cmpvariant (v, v_byteswapped); + g_assert (g_variant_equal (v, v_byteswapped)); g_variant_unref (v); g_variant_unref (v_byteswapped); @@ -4889,7 +4889,7 @@ test_normal_checking_array_offsets_overlapped (void) g_assert_nonnull (normal_variant); expected_variant = g_variant_new_parsed ("[@ay [], []]"); - g_assert_cmpvariant (normal_variant, expected_variant); + g_assert (g_variant_equal (normal_variant, expected_variant)); g_assert_cmpmem (g_variant_get_data (normal_variant), g_variant_get_size (normal_variant), g_variant_get_data (expected_variant), g_variant_get_size (expected_variant)); @@ -4956,8 +4956,8 @@ test_normal_checking_array_offsets2 (void) expected = g_variant_new_parsed ( "[[[[[[['hi', '', ''], [], []], [], []], [], []], [], []], [], []], [], []]"); - g_assert_cmpvariant (expected, variant); - g_assert_cmpvariant (expected, normal_variant); + g_assert (g_variant_equal (expected, variant)); + g_assert (g_variant_equal (expected, normal_variant)); g_variant_unref (expected); g_variant_unref (normal_variant); @@ -5113,8 +5113,8 @@ test_normal_checking_tuple_offsets2 (void) expected = g_variant_new_parsed ( "@(yyaiyyaiyy) (0x12, 0x34, [], 0x00, 0x00, [], 0x00, 0x00)"); - g_assert_cmpvariant (expected, variant); - g_assert_cmpvariant (expected, normal_variant); + g_assert (g_variant_equal (expected, variant)); + g_assert (g_variant_equal (expected, normal_variant)); g_variant_unref (expected); g_variant_unref (normal_variant); @@ -5172,8 +5172,8 @@ test_normal_checking_tuple_offsets3 (void) g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 3); expected = g_variant_new_parsed ("@(ayayiay) ([], [], 0, [])"); - g_assert_cmpvariant (expected, variant); - g_assert_cmpvariant (expected, normal_variant); + g_assert (g_variant_equal (expected, variant)); + g_assert (g_variant_equal (expected, normal_variant)); g_variant_unref (expected); g_variant_unref (normal_variant); @@ -5230,8 +5230,8 @@ test_normal_checking_tuple_offsets4 (void) g_assert_cmpuint (g_variant_get_size (normal_variant), <=, size * 3); expected = g_variant_new_parsed ("@(ayayay) ([], [], [])"); - g_assert_cmpvariant (expected, variant); - g_assert_cmpvariant (expected, normal_variant); + g_assert (g_variant_equal (expected, variant)); + g_assert (g_variant_equal (expected, normal_variant)); g_variant_unref (expected); g_variant_unref (normal_variant); -- 2.41.0 From 0ff2c59c6dd22521a1ad60520a4bf3161d5d609a Mon Sep 17 00:00:00 2001 From: Yu Daike <yu.daike@suse.com> Date: Wed, 16 Aug 2023 14:02:38 +0800 Subject: [PATCH 26/26] Comment out g_test_summary --- glib/tests/gvariant.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index b388a3dbd..28a0b9f60 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -3774,9 +3774,11 @@ test_gv_byteswap_non_normal_non_aligned (void) GVariant *v = NULL; GVariant *v_byteswapped = NULL; + /* g_test_summary ("Test that calling g_variant_byteswap() on a variant which " "is in non-normal form and doesn’t need byteswapping returns " "the same variant in normal form."); + */ v = g_variant_new_from_data (G_VARIANT_TYPE_BOOLEAN, data, sizeof (data), FALSE, NULL, NULL); g_assert_false (g_variant_is_normal_form (v)); -- 2.41.0
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
