From e2cd0962c27eb018555783f8ce65130bd399389c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 27 Nov 2023 11:00:37 +0000 Subject: [PATCH 1/4] gmessages: Document error case for g_printf_string_upper_bound() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spotted in https://gitlab.gnome.org/GNOME/glib/-/issues/3187. In an ideal world, this API would have been designed with an error return path to begin with — perhaps by returning a `GError` or a `gssize`. We can’t change the API now, though, which leads to this slightly awkward “0 indicates an error or success” pattern. I think that’s justified in this case because: - This API does not see much use. - Format strings tend to be literals, and almost always are non-zero-length, so it tends to be statically possible to determine that the function won’t return zero on success. - If callers do need to differentiate the two zero return value cases, they can just call `g_vsnprintf()` directly instead. Signed-off-by: Philip Withnall Helps: #3187 --- glib/gmessages.c | 17 +++++++++++++++-- glib/tests/test-printf.c | 3 +++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/glib/gmessages.c b/glib/gmessages.c index cad2faadf..a47e347a3 100644 --- a/glib/gmessages.c +++ b/glib/gmessages.c @@ -3391,12 +3391,25 @@ g_printerr (const gchar *format, * Calculates the maximum space needed to store the output * of the sprintf() function. * - * Returns: the maximum space needed to store the formatted string + * If @format or @args are invalid, `0` is returned. This could happen if, for + * example, @format contains an `%lc` or `%ls` placeholder and @args contains a + * wide character which cannot be represented in multibyte encoding. `0` + * can also be returned legitimately if, for example, @format is `%s` and @args + * is an empty string. The caller is responsible for differentiating these two + * return cases if necessary. It is recommended to not use `%lc` or `%ls` + * placeholders in any case, as their behaviour is locale-dependent. + * + * Returns: the maximum space needed to store the formatted string, or `0` on error */ gsize g_printf_string_upper_bound (const gchar *format, va_list args) { gchar c; - return _g_vsnprintf (&c, 1, format, args) + 1; + int count = _g_vsnprintf (&c, 1, format, args); + + if (count < 0) + return 0; + + return count + 1; } diff --git a/glib/tests/test-printf.c b/glib/tests/test-printf.c index 261b03812..faec8e110 100644 --- a/glib/tests/test-printf.c +++ b/glib/tests/test-printf.c @@ -897,6 +897,9 @@ test_upper_bound (void) res = upper_bound ("bla %s %d: %g\n", "bla", 123, 0.123); g_assert_cmpint (res, ==, 20); + + res = upper_bound ("Invalid case: %ls", L"\xD800" /* incomplete surrogate pair */); + g_assert_cmpint (res, ==, 0); } #if !defined(__APPLE__) && !defined(__FreeBSD__) From 3c6052f31880ec9eabffa8ac3fdabf42218eef9a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 27 Nov 2023 11:08:00 +0000 Subject: [PATCH 2/4] gprintf: Fix invalid size allocation in `g_vasprintf()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As per the previous commit, it’s possible for `g_printf_string_upper_bound()` to return an error. We need to catch and handle that error in `g_vasprintf()` to avoid it trying to write to a `NULL` string allocation and crashing. So, call `g_vsnprintf()` directly instead of calling `g_printf_string_upper_bound()`, so that the error case can be handled. There was already a test for some of this behaviour (`test_vasprintf_invalid_format_placeholder()`). Because it tested an invalid format string, the `_g_vsprintf()` call bailed out before checking whether the buffer it had been passed was `NULL`. The new test has a valid format string, but an invalid arg. When running the tests locally, I have disabled the `USE_SYSTEM_PRINTF` and `HAVE_VASPRINTF` code paths in `g_vasprintf()`. Signed-off-by: Philip Withnall Fixes: #3187 --- glib/gprintf.c | 22 ++++++++++++++++------ glib/tests/test-printf.c | 25 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/glib/gprintf.c b/glib/gprintf.c index a0ccef9ab..19642d82b 100644 --- a/glib/gprintf.c +++ b/glib/gprintf.c @@ -359,19 +359,29 @@ g_vasprintf (gchar **string, { va_list args2; + char c; + int max_len; va_copy (args2, args); - *string = g_new (gchar, g_printf_string_upper_bound (format, args)); + max_len = _g_vsnprintf (&c, 1, format, args); + if (max_len < 0) + { + /* This can happen if @format contains `%ls` or `%lc` and @args contains + * something not representable in the current locale’s encoding (which + * should be UTF-8, but ymmv). Basically: don’t use `%ls` or `%lc`. */ + va_end (args2); + *string = NULL; + return -1; + } + + *string = g_new (gchar, (size_t) max_len + 1); len = _g_vsprintf (*string, format, args2); va_end (args2); - if (len < 0) - { - g_free (*string); - *string = NULL; - } + /* _g_vsprintf() should have exactly the same failure modes as _g_vsnprintf() */ + g_assert (len >= 0); } #endif diff --git a/glib/tests/test-printf.c b/glib/tests/test-printf.c index faec8e110..432685e3f 100644 --- a/glib/tests/test-printf.c +++ b/glib/tests/test-printf.c @@ -948,6 +948,30 @@ test_vasprintf_invalid_format_placeholder (void) #endif } +static void +test_vasprintf_invalid_wide_string (void) +{ +#if !defined(__APPLE__) && !defined(__FreeBSD__) + gint len = 0; + gchar *buf = "some non-null string"; +#endif + + g_test_summary ("Test error handling for invalid wide strings in g_vasprintf()"); + +#if !defined(__APPLE__) && !defined(__FreeBSD__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat" +#pragma GCC diagnostic ignored "-Wformat-extra-args" + len = test_vasprintf_va (&buf, "%ls", L"\xD800" /* incomplete surrogate pair */); +#pragma GCC diagnostic pop + + g_assert_cmpint (len, ==, -1); + g_assert_null (buf); +#else + g_test_skip ("vasprintf() placeholder checks on BSDs are less strict"); +#endif +} + int main (int argc, char *argv[]) @@ -989,6 +1013,7 @@ main (int argc, g_test_add_func ("/sprintf/upper-bound", test_upper_bound); g_test_add_func ("/vasprintf/invalid-format-placeholder", test_vasprintf_invalid_format_placeholder); + g_test_add_func ("/vasprintf/invalid-wide-string", test_vasprintf_invalid_wide_string); return g_test_run(); } From c56bc6d8d94d6cefd8c05105df88de92d05df62e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 27 Nov 2023 11:15:43 +0000 Subject: [PATCH 3/4] gstrfuncs: Add missing annotations to g_strdup_vprintf() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s possible for the function to fail for the same reasons `g_vasprintf()` would fail. Signed-off-by: Philip Withnall Helps: #3187 --- glib/gstrfuncs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/glib/gstrfuncs.c b/glib/gstrfuncs.c index 58d098c07..506b7c3ea 100644 --- a/glib/gstrfuncs.c +++ b/glib/gstrfuncs.c @@ -542,7 +542,7 @@ g_stpcpy (gchar *dest, * See also g_vasprintf(), which offers the same functionality, but * additionally returns the length of the allocated string. * - * Returns: a newly-allocated string holding the result + * Returns: (nullable) (transfer full): a newly-allocated string holding the result */ gchar* g_strdup_vprintf (const gchar *format, @@ -570,7 +570,7 @@ g_strdup_vprintf (const gchar *format, * contains `%lc` or `%ls` conversions, which can fail if no multibyte * representation is available for the given character. * - * Returns: a newly-allocated string holding the result + * Returns: (nullable) (transfer full): a newly-allocated string holding the result */ gchar* g_strdup_printf (const gchar *format, From 707ae4bd79c969c05f24a33ea259ad03836c8e0a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 27 Nov 2023 11:18:03 +0000 Subject: [PATCH 4/4] gstring: Add critical warning to g_string_append_vprintf() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was previously possible for this to silently fail, which isn’t great for program correctness. Instead, raise a critical warning so the programmer knows to either validate their Unicode inputs, or fix their format placeholders. Signed-off-by: Philip Withnall Helps: #3187 --- glib/gstring.c | 4 ++++ glib/tests/string.c | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/glib/gstring.c b/glib/gstring.c index 79a8d2248..463bfeccc 100644 --- a/glib/gstring.c +++ b/glib/gstring.c @@ -1221,6 +1221,10 @@ g_string_append_vprintf (GString *string, string->len += len; g_free (buf); } + else + { + g_critical ("Failed to append to string: invalid format/args passed to g_vasprintf()"); + } } /** diff --git a/glib/tests/string.c b/glib/tests/string.c index 932a311de..bb29304c4 100644 --- a/glib/tests/string.c +++ b/glib/tests/string.c @@ -319,11 +319,17 @@ test_string_append_vprintf (void) string_append_vprintf_va (string, "some %s placeholders", "format"); + if (g_test_undefined ()) + { + g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL, + "Failed to append to string*"); #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wformat" #pragma GCC diagnostic ignored "-Wformat-extra-args" string_append_vprintf_va (string, "%l", "invalid"); #pragma GCC diagnostic pop + g_test_assert_expected_messages (); + } g_assert_cmpstr (string->str, ==, "firsthalfsome format placeholders");