From f068347b1419213be20e3647f5d4f1d505bdcba4 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 29 Dec 2022 17:38:04 +0000 Subject: [PATCH 1/4] gvariant: Factor out some common calls to g_variant_get_type_string() When printing a `GVariant`. This introduces no functional changes, but should speed things up a little bit when printing out arrays. Signed-off-by: Philip Withnall --- glib/gvariant.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index 95904bd38..8f0c237db 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -2213,15 +2213,16 @@ g_variant_print_string (GVariant *value, GString *string, gboolean type_annotate) { + const gchar *value_type_string = g_variant_get_type_string (value); + if G_UNLIKELY (string == NULL) string = g_string_new (NULL); - switch (g_variant_classify (value)) + switch (value_type_string[0]) { case G_VARIANT_CLASS_MAYBE: if (type_annotate) - g_string_append_printf (string, "@%s ", - g_variant_get_type_string (value)); + g_string_append_printf (string, "@%s ", value_type_string); if (g_variant_n_children (value)) { @@ -2265,7 +2266,7 @@ g_variant_print_string (GVariant *value, * if the first two characters are 'ay' then it's a bytestring. * under certain conditions we print those as strings. */ - if (g_variant_get_type_string (value)[1] == 'y') + if (value_type_string[1] == 'y') { const gchar *str; gsize size; @@ -2307,7 +2308,7 @@ g_variant_print_string (GVariant *value, * dictionary entries (ie: a dictionary) so we print that * differently. */ - if (g_variant_get_type_string (value)[1] == '{') + if (value_type_string[1] == '{') /* dictionary */ { const gchar *comma = ""; @@ -2316,8 +2317,7 @@ g_variant_print_string (GVariant *value, if ((n = g_variant_n_children (value)) == 0) { if (type_annotate) - g_string_append_printf (string, "@%s ", - g_variant_get_type_string (value)); + g_string_append_printf (string, "@%s ", value_type_string); g_string_append (string, "{}"); break; } @@ -2353,8 +2353,7 @@ g_variant_print_string (GVariant *value, if ((n = g_variant_n_children (value)) == 0) { if (type_annotate) - g_string_append_printf (string, "@%s ", - g_variant_get_type_string (value)); + g_string_append_printf (string, "@%s ", value_type_string); g_string_append (string, "[]"); break; } From 53702d68c4ee7b939a725ec4a5d707a2bdead1e9 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 29 Dec 2022 18:42:44 +0000 Subject: [PATCH 2/4] gvariant: Optimise g_variant_print() for nested maybes As with commit 9ae59bd647882bcb33103331255a5149d2fb90d2, deeply nested maybes in an array can be exploited by a malicious caller to cause a geometric increase in processing time and number of `GVariant` instances handled by the `g_variant_print()` code. Optimise this by skipping recursing through most of the chain of maybes, thus avoiding all the setup checks in each recursive call. Signed-off-by: Philip Withnall oss-fuzz#54577 --- glib/gvariant.c | 44 +++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/glib/gvariant.c b/glib/gvariant.c index 8f0c237db..cef0216c5 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -2226,8 +2226,9 @@ g_variant_print_string (GVariant *value, if (g_variant_n_children (value)) { - gchar *printed_child; - GVariant *element; + const GVariantType *base_type; + guint i, depth; + GVariant *element = NULL; /* Nested maybes: * @@ -2241,19 +2242,36 @@ g_variant_print_string (GVariant *value, * "just" is actually exactly the case where we have a nested * Nothing. * - * Instead of searching for that nested Nothing, we just print - * the contained value into a separate string and see if we - * end up with "nothing" at the end of it. If so, we need to - * add "just" at our level. + * Search for the nested Nothing, to save a lot of recursion if there + * are multiple levels of maybes. */ - element = g_variant_get_child_value (value, 0); - printed_child = g_variant_print (element, FALSE); - g_variant_unref (element); + for (depth = 0, base_type = g_variant_get_type (value); + g_variant_type_is_maybe (base_type); + depth++, base_type = g_variant_type_element (base_type)); - if (g_str_has_suffix (printed_child, "nothing")) - g_string_append (string, "just "); - g_string_append (string, printed_child); - g_free (printed_child); + element = g_variant_ref (value); + for (i = 0; i < depth && element != NULL; i++) + { + GVariant *new_element = g_variant_n_children (element) ? g_variant_get_child_value (element, 0) : NULL; + g_variant_unref (element); + element = g_steal_pointer (&new_element); + } + + if (element == NULL) + { + /* One of the maybes was Nothing, so print out the right number of + * justs. */ + for (; i > 1; i--) + g_string_append (string, "just "); + g_string_append (string, "nothing"); + } + else + { + /* There are no Nothings, so print out the child with no prefixes. */ + g_variant_print_string (element, string, FALSE); + } + + g_clear_pointer (&element, g_variant_unref); } else g_string_append (string, "nothing"); From 8373f328bd1e996a1521eed3f962af873cfa14a0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 29 Dec 2022 18:53:35 +0000 Subject: [PATCH 3/4] gvariant: Remove redundant g_variant_serialised_n_children() calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These functions were previously calling `g_variant_serialised_n_children()` twice just to validate the input, in the case that the input was a serialised variant. That’s not necessary, and checking the number of children in a serialised variant is not necessarily cheap. Move the checks around so that the number of children is only checked once on each code path. This doesn’t introduce any functional changes. Signed-off-by: Philip Withnall --- glib/gvariant-core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c index 57bbf88cc..3bb313532 100644 --- a/glib/gvariant-core.c +++ b/glib/gvariant-core.c @@ -1138,11 +1138,13 @@ GVariant * g_variant_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_serialised_get_child() does its own checks on index_ */ + g_return_val_if_fail (index_ < g_variant_n_children (value), NULL); + g_variant_lock (value); if (~value->state & STATE_SERIALISED) @@ -1233,11 +1235,13 @@ 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_serialised_get_child() does its own checks on index_ */ + g_return_val_if_fail (index_ < g_variant_n_children (value), NULL); + g_variant_lock (value); if (~value->state & STATE_SERIALISED) From ab8cdb07dece52f0226a1d0346157b6f172ed1ba Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 29 Dec 2022 18:54:53 +0000 Subject: [PATCH 4/4] gvariant: Remove some unnecessary type assertions on a hot path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While checking the validity of a `GVariantTypeInfo` is good, this code path is very hot, and I’ve never seen these assertions catch a bug in practice. Lean more towards the performance side of the performance/correctness tradeoff in this case, by removing the assertions here. They remain in place in a number of other `GVariantTypeInfo` code paths, so invalid `GVariantTypeInfo` pointers should hopefully still be caught quickly. Signed-off-by: Philip Withnall --- glib/gvarianttypeinfo.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/glib/gvarianttypeinfo.c b/glib/gvarianttypeinfo.c index 2b310581c..4e2e9c5b4 100644 --- a/glib/gvarianttypeinfo.c +++ b/glib/gvarianttypeinfo.c @@ -247,8 +247,6 @@ g_variant_type_info_query (GVariantTypeInfo *info, guint *alignment, gsize *fixed_size) { - g_variant_type_info_check (info, 0); - if (alignment) *alignment = info->alignment;