From 7a5d4c2bb7d6ab1539508da5c67adebe78b40809 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 13 Apr 2023 17:03:35 +0100 Subject: [PATCH] gfileutils: Fix potential integer overflow in g_get_current_dir() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In practice, this will never happen. If `getcwd()` returns `ERANGE` whenever the working directory is ≥ `PATH_MAX`, though, the previous implementation of the loop would run until `max_len == G_MAXULONG`, and would then overflow when adding `1` to it for a nul terminator in the allocation. Avoid that problem by always keeping `buffer_len` as a power of two, and relying on `getcwd()` to write a nul terminator within the buffer space it’s given. It seems to do this on all platforms we care about, because the previous version of the code never explicitly wrote a nul terminator anywhere. Signed-off-by: Philip Withnall Fixes: #98 --- glib/gfileutils.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/glib/gfileutils.c b/glib/gfileutils.c index 4ed8171cc..9646c696e 100644 --- a/glib/gfileutils.c +++ b/glib/gfileutils.c @@ -2940,7 +2940,7 @@ g_get_current_dir (void) const gchar *pwd; gchar *buffer = NULL; gchar *dir = NULL; - static gulong max_len = 0; + static gsize buffer_size = 0; struct stat pwdbuf, dotbuf; pwd = g_getenv ("PWD"); @@ -2949,27 +2949,31 @@ g_get_current_dir (void) dotbuf.st_dev == pwdbuf.st_dev && dotbuf.st_ino == pwdbuf.st_ino) return g_strdup (pwd); - if (max_len == 0) - max_len = (G_PATH_LENGTH == -1) ? 2048 : G_PATH_LENGTH; + if (buffer_size == 0) + buffer_size = (G_PATH_LENGTH == -1) ? 2048 : G_PATH_LENGTH; - while (max_len < G_MAXULONG / 2) + while (buffer_size < G_MAXSIZE / 2) { g_free (buffer); - buffer = g_new (gchar, max_len + 1); + buffer = g_new (gchar, buffer_size); *buffer = 0; - dir = getcwd (buffer, max_len); + dir = getcwd (buffer, buffer_size); if (dir || errno != ERANGE) break; - max_len *= 2; + buffer_size *= 2; } + /* Check that getcwd() nul-terminated the string. It should do, but the specs + * don’t actually explicitly state that: + * https://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html */ + g_assert (dir == NULL || strnlen (dir, buffer_size) < buffer_size); + if (!dir || !*buffer) { - /* hm, should we g_error() out here? - * this can happen if e.g. "./" has mode \0000 - */ + /* Fallback return value */ + g_assert (buffer_size >= 2); buffer[0] = G_DIR_SEPARATOR; buffer[1] = 0; }