From b65e50cff5451201e2e1d9aac662cc71adfe6f6d 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 5fef88d58..ee24ffc34 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 a1213e343e2663b01d3465b524ce6e169b93fbe0 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 ee24ffc34..cd4c21ced 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 fa17060027864f114d6c4d96265e50a91d8cf523 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 477802282..f5e9dddfe 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 5283a169d9cd3a02e1cff45d55dc390c3b99e3fa 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;