From 1cf39a3000d8f667e106da38874f11b45df98e42 Mon Sep 17 00:00:00 2001 From: Benjamin Otte Date: Wed, 12 Apr 2023 21:10:21 +0200 Subject: [PATCH 1/6] array: Abort on overflow This is a precautionary assert that will probably only trigger on 32bit OSes. But g_nearest_pow() can overflow. --- glib/garray.c | 1 + 1 file changed, 1 insertion(+) diff --git a/glib/garray.c b/glib/garray.c index efd031b0c..e586bb5fa 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -1068,6 +1068,7 @@ g_array_maybe_expand (GRealArray *array, if (want_len > array->elt_capacity) { gsize want_alloc = g_nearest_pow (g_array_elt_len (array, want_len)); + g_assert (want_alloc >= g_array_elt_len (array, want_len)); want_alloc = MAX (want_alloc, MIN_ARRAY_SIZE); array->data = g_realloc (array->data, want_alloc); From c2b3a6c55706e07533229ee8ee422b12c485679a Mon Sep 17 00:00:00 2001 From: Benjamin Otte Date: Wed, 12 Apr 2023 21:34:34 +0200 Subject: [PATCH 2/6] gfile: Rewrite load_contents() to use realloc() GByteArray is limited to 4GB in size and the current code silently overflows when that happens. Replace both load_contents() and load_contents_async() implementations with a version that uses realloc() and gsize to maintain the array. --- gio/gfile.c | 73 +++++++++++++++++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 25 deletions(-) diff --git a/gio/gfile.c b/gio/gfile.c index d52262f13..c084d75ce 100644 --- a/gio/gfile.c +++ b/gio/gfile.c @@ -7933,7 +7933,8 @@ g_file_load_contents (GFile *file, GError **error) { GFileInputStream *in; - GByteArray *content; + char *data; + gsize size; gsize pos; gssize res; GFileInfo *info; @@ -7945,17 +7946,22 @@ g_file_load_contents (GFile *file, if (in == NULL) return FALSE; - content = g_byte_array_new (); + size = GET_CONTENT_BLOCK_SIZE; + data = g_malloc (GET_CONTENT_BLOCK_SIZE); pos = 0; - g_byte_array_set_size (content, pos + GET_CONTENT_BLOCK_SIZE + 1); while ((res = g_input_stream_read (G_INPUT_STREAM (in), - content->data + pos, + data + pos, GET_CONTENT_BLOCK_SIZE, cancellable, error)) > 0) { pos += res; - g_byte_array_set_size (content, pos + GET_CONTENT_BLOCK_SIZE + 1); + if (size - pos < GET_CONTENT_BLOCK_SIZE) + { + g_assert (size <= G_MAXSIZE / 2); + size *= 2; + data = g_realloc (data, size); + } } if (etag_out) @@ -7980,17 +7986,19 @@ g_file_load_contents (GFile *file, if (res < 0) { /* error is set already */ - g_byte_array_free (content, TRUE); + g_free (data); return FALSE; } if (length) *length = pos; - /* Zero terminate (we got an extra byte allocated for this */ - content->data[pos] = 0; + /* Zero terminate (allocating extra bytes if needed) */ + if (pos >= size) + data = g_realloc (data, pos + 1); + data[pos] = 0; - *contents = (char *)g_byte_array_free (content, FALSE); + *contents = g_steal_pointer (&data); return TRUE; } @@ -7998,7 +8006,8 @@ g_file_load_contents (GFile *file, typedef struct { GTask *task; GFileReadMoreCallback read_more_callback; - GByteArray *content; + char *data; + gsize size; gsize pos; char *etag; } LoadContentsData; @@ -8007,12 +8016,31 @@ typedef struct { static void load_contents_data_free (LoadContentsData *data) { - if (data->content) - g_byte_array_free (data->content, TRUE); + g_clear_pointer (&data->data, g_free); g_free (data->etag); g_free (data); } +static void +load_contents_data_ensure_space (LoadContentsData *data, + gsize space) +{ + if (data->size - data->pos < space) + { + if (data->data == NULL) + { + data->size = space; + data->data = g_malloc (space); + } + else + { + g_assert (data->size <= G_MAXSIZE / 2); + data->size *= 2; + data->data = g_realloc (data->data, data->size); + } + } +} + static void load_contents_close_callback (GObject *obj, GAsyncResult *close_res, @@ -8085,12 +8113,10 @@ load_contents_read_callback (GObject *obj, { data->pos += read_size; - g_byte_array_set_size (data->content, - data->pos + GET_CONTENT_BLOCK_SIZE); - + load_contents_data_ensure_space (data, GET_CONTENT_BLOCK_SIZE); if (data->read_more_callback && - !data->read_more_callback ((char *)data->content->data, data->pos, + !data->read_more_callback (data->data, data->pos, g_async_result_get_user_data (G_ASYNC_RESULT (data->task)))) g_file_input_stream_query_info_async (G_FILE_INPUT_STREAM (stream), G_FILE_ATTRIBUTE_ETAG_VALUE, @@ -8100,7 +8126,7 @@ load_contents_read_callback (GObject *obj, data); else g_input_stream_read_async (stream, - data->content->data + data->pos, + data->data + data->pos, GET_CONTENT_BLOCK_SIZE, 0, g_task_get_cancellable (data->task), @@ -8123,10 +8149,9 @@ load_contents_open_callback (GObject *obj, if (stream) { - g_byte_array_set_size (data->content, - data->pos + GET_CONTENT_BLOCK_SIZE); + load_contents_data_ensure_space (data, GET_CONTENT_BLOCK_SIZE); g_input_stream_read_async (G_INPUT_STREAM (stream), - data->content->data + data->pos, + data->data + data->pos, GET_CONTENT_BLOCK_SIZE, 0, g_task_get_cancellable (data->task), @@ -8176,7 +8201,6 @@ g_file_load_partial_contents_async (GFile *file, data = g_new0 (LoadContentsData, 1); data->read_more_callback = read_more_callback; - data->content = g_byte_array_new (); data->task = g_task_new (file, cancellable, callback, user_data); g_task_set_source_tag (data->task, g_file_load_partial_contents_async); @@ -8245,11 +8269,10 @@ g_file_load_partial_contents_finish (GFile *file, } /* Zero terminate */ - g_byte_array_set_size (data->content, data->pos + 1); - data->content->data[data->pos] = 0; + load_contents_data_ensure_space (data, 1); + data->data[data->pos] = 0; - *contents = (char *)g_byte_array_free (data->content, FALSE); - data->content = NULL; + *contents = g_steal_pointer (&data->data); return TRUE; } From 84f3b1ab9eedb3e26c46d153e480a2e4e0f8dc74 Mon Sep 17 00:00:00 2001 From: Benjamin Otte Date: Mon, 17 Apr 2023 14:20:52 +0200 Subject: [PATCH 3/6] tests: Use g_clear_fd() --- gio/tests/file.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gio/tests/file.c b/gio/tests/file.c index d32d955cd..2adce7894 100644 --- a/gio/tests/file.c +++ b/gio/tests/file.c @@ -6,6 +6,7 @@ #include #include #include +#include #ifdef G_OS_UNIX #include #endif @@ -2742,7 +2743,8 @@ test_load_bytes (void) len = strlen ("test_load_bytes"); ret = write (fd, "test_load_bytes", len); g_assert_cmpint (ret, ==, len); - close (fd); + g_clear_fd (&fd, &error); + g_assert_no_error (error); file = g_file_new_for_path (filename); bytes = g_file_load_bytes (file, NULL, NULL, &error); @@ -2785,6 +2787,7 @@ test_load_bytes_async (void) { LoadBytesAsyncData data = { 0 }; gchar filename[] = "g_file_load_bytes_XXXXXX"; + GError *error = NULL; int len; int fd; int ret; @@ -2794,7 +2797,8 @@ test_load_bytes_async (void) len = strlen ("test_load_bytes_async"); ret = write (fd, "test_load_bytes_async", len); g_assert_cmpint (ret, ==, len); - close (fd); + g_clear_fd (&fd, &error); + g_assert_no_error (error); data.main_loop = g_main_loop_new (NULL, FALSE); data.file = g_file_new_for_path (filename); From 7ad4384af14360f208567e2e5a444c61d3b4338f Mon Sep 17 00:00:00 2001 From: Benjamin Otte Date: Fri, 14 Apr 2023 18:53:44 +0200 Subject: [PATCH 4/6] tests: Add test for >4GB reads with g_file_load_contents() The tests - one for sync, one for async - create a sparse file for this purpose, so this should be cheap on the fileystem. Of course, the test still allocates >4GB of memory for the data that it returns from g_file_load_contents(), I hope the CI test runners can deal with that. --- gio/tests/file.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/gio/tests/file.c b/gio/tests/file.c index 2adce7894..6c9cd6371 100644 --- a/gio/tests/file.c +++ b/gio/tests/file.c @@ -2815,6 +2815,144 @@ test_load_bytes_async (void) g_main_loop_unref (data.main_loop); } +static const gsize testfile_4gb_size = ((gsize) 1 << 32) + (1 << 16); /* 4GB + a bit */ + +/* @filename will be modified as per g_mkstemp() */ +static gboolean +create_testfile_4gb_or_skip (char *filename) +{ + GError *error = NULL; + int fd; + int ret; + + fd = g_mkstemp (filename); + g_assert_cmpint (fd, !=, -1); + ret = ftruncate (fd, testfile_4gb_size); + g_clear_fd (&fd, &error); + g_assert_no_error (error); + if (ret == 1) + { + g_test_skip ("Could not create testfile >4GB"); + g_assert_no_errno (g_unlink (filename)); + return FALSE; + } + + return TRUE; +} + +static void +check_testfile_4gb_contents (const char *data, + gsize len) +{ + gsize i; + + g_assert_nonnull (data); + g_assert_cmpuint (testfile_4gb_size, ==, len); + + for (i = 0; i < testfile_4gb_size; i++) + { + if (data[i] != 0) + break; + } + g_assert_cmpint (i, ==, testfile_4gb_size); +} + +static void +test_load_contents_4gb (void) +{ + char filename[] = "g_file_load_contents_4gb_XXXXXX"; + GError *error = NULL; + gboolean result; + char *data; + gsize len; + GFile *file; + + if (!create_testfile_4gb_or_skip (filename)) + return; + + file = g_file_new_for_path (filename); + result = g_file_load_contents (file, NULL, &data, &len, NULL, &error); + g_assert_no_error (error); + g_assert_true (result); + + check_testfile_4gb_contents (data, len); + + g_file_delete (file, NULL, NULL); + + g_free (data); + g_object_unref (file); +} + +static void +load_contents_4gb_cb (GObject *object, + GAsyncResult *result, + gpointer user_data) +{ + GAsyncResult **result_out = user_data; + + g_assert (*result_out == NULL); + *result_out = g_object_ref (result); + + g_main_context_wakeup (NULL); +} + +static void +test_load_contents_4gb_async (void) +{ + char filename[] = "g_file_load_contents_4gb_async_XXXXXX"; + GFile *file; + GAsyncResult *async_result = NULL; + GError *error = NULL; + char *data; + gsize len; + gboolean ret; + + if (!create_testfile_4gb_or_skip (filename)) + return; + + file = g_file_new_for_path (filename); + g_file_load_contents_async (file, NULL, load_contents_4gb_cb, &async_result); + + while (async_result == NULL) + g_main_context_iteration (NULL, TRUE); + + ret = g_file_load_contents_finish (file, async_result, &data, &len, NULL, &error); + g_assert_no_error (error); + g_assert_true (ret); + + check_testfile_4gb_contents (data, len); + + g_file_delete (file, NULL, NULL); + + g_free (data); + g_object_unref (async_result); + g_object_unref (file); +} + +static void +test_load_bytes_4gb (void) +{ + char filename[] = "g_file_load_bytes_4gb_XXXXXX"; + GError *error = NULL; + GBytes *bytes; + GFile *file; + + if (!create_testfile_4gb_or_skip (filename)) + return; + + file = g_file_new_for_path (filename); + bytes = g_file_load_bytes (file, NULL, NULL, &error); + g_assert_no_error (error); + g_assert_true (bytes); + + check_testfile_4gb_contents (g_bytes_get_data (bytes, NULL), g_bytes_get_size (bytes)); + + g_file_delete (file, NULL, NULL); + + g_bytes_unref (bytes); + g_object_unref (file); +} + static void test_writev_helper (GOutputVector *vectors, gsize n_vectors, @@ -4036,6 +4174,9 @@ main (int argc, char *argv[]) g_test_add_func ("/file/measure-async", test_measure_async); g_test_add_func ("/file/load-bytes", test_load_bytes); g_test_add_func ("/file/load-bytes-async", test_load_bytes_async); + g_test_add_func ("/file/load-bytes-4gb", test_load_bytes_4gb); + g_test_add_func ("/file/load-contents-4gb", test_load_contents_4gb); + g_test_add_func ("/file/load-contents-4gb-async", test_load_contents_4gb_async); g_test_add_func ("/file/writev", test_writev); g_test_add_func ("/file/writev/no-bytes-written", test_writev_no_bytes_written); g_test_add_func ("/file/writev/no-vectors", test_writev_no_vectors); From 724709f022c48c93891ed98a22f1265690248b41 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 17 Jun 2024 12:59:34 +0100 Subject: [PATCH 5/6] tests: Skip >4GB file tests unless running tests in slow/thorough mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They take too long to include in a normal test run. They’ll still be run in CI once a week as part of our scheduled slow test job. Signed-off-by: Philip Withnall --- gio/tests/file.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/gio/tests/file.c b/gio/tests/file.c index 6c9cd6371..d8c5ba1a3 100644 --- a/gio/tests/file.c +++ b/gio/tests/file.c @@ -2825,6 +2825,15 @@ create_testfile_4gb_or_skip (char *filename) int fd; int ret; + /* Reading each 4GB test file takes about 5s on a fast machine, and another 7s + * to compare its contents once it’s been read. That’s too slow for a normal + * test run, and there’s no way to speed it up. */ + if (!g_test_slow ()) + { + g_test_skip ("Skipping slow >4GB file test"); + return FALSE; + } + fd = g_mkstemp (filename); g_assert_cmpint (fd, !=, -1); ret = ftruncate (fd, testfile_4gb_size); From b4085620c686484bafe6340e08f2bc48a80b52a2 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 18 Jun 2024 12:58:51 +0100 Subject: [PATCH 6/6] tests: Improve 4GB file loading test to work on i386 This should test the limits of loading 4GB files on i386 platforms, such as the Hurd CI runner. On such platforms, `sizeof(size_t) == 4`. This should fix the compiler warning from https://gitlab.gnome.org/GNOME/glib/-/jobs/3989442: ``` ../gio/tests/file.c:2931:51: error: left shift count >= width of type [-Werror=shift-count-overflow] 2931 | static const gsize testfile_4gb_size = ((gsize) 1 << 32) + (1 << 16); /* 4GB + a bit */ | ^~ cc1: all warnings being treated as errors ``` Signed-off-by: Philip Withnall --- gio/tests/file.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gio/tests/file.c b/gio/tests/file.c index d8c5ba1a3..660c4e5fd 100644 --- a/gio/tests/file.c +++ b/gio/tests/file.c @@ -2815,7 +2815,12 @@ test_load_bytes_async (void) g_main_loop_unref (data.main_loop); } +#if GLIB_SIZEOF_SIZE_T > 4 static const gsize testfile_4gb_size = ((gsize) 1 << 32) + (1 << 16); /* 4GB + a bit */ +#else +/* Have to make do with something smaller on 32-bit platforms */ +static const gsize testfile_4gb_size = G_MAXSIZE; +#endif /* @filename will be modified as per g_mkstemp() */ static gboolean