From 0ace82d74064d55522aa2c3e13c8bf0796b90822 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 4 Feb 2021 13:41:21 +0000 Subject: glib: Use g_memdup2() instead of g_memdup() in obvious places MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert all the call sites which use `g_memdup()`’s length argument trivially (for example, by passing a `sizeof()` or an existing `gsize` variable), so that they use `g_memdup2()` instead. In almost all of these cases the use of `g_memdup()` would not have caused problems, but it will soon be deprecated, so best port away from it In particular, this fixes an overflow within `g_bytes_new()`, identified as GHSL-2021-045 (aka CVE-2021-27219) by GHSL team member Kevin Backhouse. Adapted for GLib 2.58 by Simon McVittie. Signed-off-by: Philip Withnall Fixes: CVE-2021-27219 Fixes: GHSL-2021-045 Helps: #2319 (cherry picked from commit 0736b7c1e7cf4232c5d7eb2b0fbfe9be81bd3baa) [Backport to 2.58: Omit changes to ghash.c, will be a separate commit] [Backport to 2.58: Omit changes to giochannel.c, not needed in this branch] [Backport to 2.58: Omit changes to uri test, not needed in this branch] Signed-off-by: Simon McVittie --- glib/gbytes.c | 6 ++++-- glib/gdir.c | 3 ++- glib/gslice.c | 3 ++- glib/gtestutils.c | 3 ++- glib/gvariant.c | 7 ++++--- glib/gvarianttype.c | 3 ++- glib/tests/array-test.c | 4 +++- glib/tests/option-context.c | 6 ++++-- 8 files changed, 23 insertions(+), 12 deletions(-) diff --git a/glib/gbytes.c b/glib/gbytes.c index 7b72886e5..84c87e4cf 100644 --- a/glib/gbytes.c +++ b/glib/gbytes.c @@ -34,6 +34,8 @@ #include +#include "gstrfuncsprivate.h" + /** * GBytes: * @@ -95,7 +97,7 @@ g_bytes_new (gconstpointer data, { g_return_val_if_fail (data != NULL || size == 0, NULL); - return g_bytes_new_take (g_memdup (data, size), size); + return g_bytes_new_take (g_memdup2 (data, size), size); } /** @@ -499,7 +501,7 @@ g_bytes_unref_to_data (GBytes *bytes, * Copy: Non g_malloc (or compatible) allocator, or static memory, * so we have to copy, and then unref. */ - result = g_memdup (bytes->data, bytes->size); + result = g_memdup2 (bytes->data, bytes->size); *size = bytes->size; g_bytes_unref (bytes); } diff --git a/glib/gdir.c b/glib/gdir.c index cb4ad0b2f..9d955d57f 100644 --- a/glib/gdir.c +++ b/glib/gdir.c @@ -37,6 +37,7 @@ #include "gconvert.h" #include "gfileutils.h" #include "gstrfuncs.h" +#include "gstrfuncsprivate.h" #include "gtestutils.h" #include "glibintl.h" @@ -113,7 +114,7 @@ g_dir_open_with_errno (const gchar *path, return NULL; #endif - return g_memdup (&dir, sizeof dir); + return g_memdup2 (&dir, sizeof dir); } /** diff --git a/glib/gslice.c b/glib/gslice.c index d1b1fc639..db3331b10 100644 --- a/glib/gslice.c +++ b/glib/gslice.c @@ -41,6 +41,7 @@ #include "gmain.h" #include "gmem.h" /* gslice.h */ #include "gstrfuncs.h" +#include "gstrfuncsprivate.h" #include "gutils.h" #include "gtrashstack.h" #include "gtestutils.h" @@ -349,7 +350,7 @@ g_slice_get_config_state (GSliceConfig ckey, array[i++] = allocator->contention_counters[address]; array[i++] = allocator_get_magazine_threshold (allocator, address); *n_values = i; - return g_memdup (array, sizeof (array[0]) * *n_values); + return g_memdup2 (array, sizeof (array[0]) * *n_values); default: return NULL; } diff --git a/glib/gtestutils.c b/glib/gtestutils.c index 7b29c274e..f484fd321 100644 --- a/glib/gtestutils.c +++ b/glib/gtestutils.c @@ -49,6 +49,7 @@ #include "gpattern.h" #include "grand.h" #include "gstrfuncs.h" +#include "gstrfuncsprivate.h" #include "gtimer.h" #include "gslice.h" #include "gspawn.h" @@ -3461,7 +3462,7 @@ g_test_log_extract (GTestLogBuffer *tbuffer) if (p <= tbuffer->data->str + mlength) { g_string_erase (tbuffer->data, 0, mlength); - tbuffer->msgs = g_slist_prepend (tbuffer->msgs, g_memdup (&msg, sizeof (msg))); + tbuffer->msgs = g_slist_prepend (tbuffer->msgs, g_memdup2 (&msg, sizeof (msg))); return TRUE; } diff --git a/glib/gvariant.c b/glib/gvariant.c index d45b487ad..de35c3edc 100644 --- a/glib/gvariant.c +++ b/glib/gvariant.c @@ -33,6 +33,7 @@ #include +#include "gstrfuncsprivate.h" /** * SECTION:gvariant @@ -720,7 +721,7 @@ g_variant_new_variant (GVariant *value) g_variant_ref_sink (value); return g_variant_new_from_children (G_VARIANT_TYPE_VARIANT, - g_memdup (&value, sizeof value), + g_memdup2 (&value, sizeof value), 1, g_variant_is_trusted (value)); } @@ -1224,7 +1225,7 @@ g_variant_new_fixed_array (const GVariantType *element_type, return NULL; } - data = g_memdup (elements, n_elements * element_size); + data = g_memdup2 (elements, n_elements * element_size); value = g_variant_new_from_data (array_type, data, n_elements * element_size, FALSE, g_free, data); @@ -1901,7 +1902,7 @@ g_variant_dup_bytestring (GVariant *value, if (length) *length = size; - return g_memdup (original, size + 1); + return g_memdup2 (original, size + 1); } /** diff --git a/glib/gvarianttype.c b/glib/gvarianttype.c index f64be97f9..59ae6aa44 100644 --- a/glib/gvarianttype.c +++ b/glib/gvarianttype.c @@ -28,6 +28,7 @@ #include +#include "gstrfuncsprivate.h" /** * SECTION:gvarianttype @@ -1177,7 +1178,7 @@ g_variant_type_new_tuple (const GVariantType * const *items, g_assert (offset < sizeof buffer); buffer[offset++] = ')'; - return (GVariantType *) g_memdup (buffer, offset); + return (GVariantType *) g_memdup2 (buffer, offset); } /** diff --git a/glib/tests/array-test.c b/glib/tests/array-test.c index e0a6109eb..faa60610d 100644 --- a/glib/tests/array-test.c +++ b/glib/tests/array-test.c @@ -30,6 +30,8 @@ #include #include "glib.h" +#include "gstrfuncsprivate.h" + /* Test data to be passed to any function which calls g_array_new(), providing * the parameters for that call. Most #GArray tests should be repeated for all * possible values of #ArrayTestData. */ @@ -1177,7 +1179,7 @@ byte_array_new_take (void) GByteArray *gbarray; guint8 *data; - data = g_memdup ("woooweeewow", 11); + data = g_memdup2 ("woooweeewow", 11); gbarray = g_byte_array_new_take (data, 11); g_assert (gbarray->data == data); g_assert_cmpuint (gbarray->len, ==, 11); diff --git a/glib/tests/option-context.c b/glib/tests/option-context.c index 34ebfaaf6..7125e2285 100644 --- a/glib/tests/option-context.c +++ b/glib/tests/option-context.c @@ -27,6 +27,8 @@ #include #include +#include "gstrfuncsprivate.h" + static GOptionEntry main_entries[] = { { "main-switch", 0, 0, G_OPTION_ARG_NONE, NULL, @@ -256,7 +258,7 @@ join_stringv (int argc, char **argv) static char ** copy_stringv (char **argv, int argc) { - return g_memdup (argv, sizeof (char *) * (argc + 1)); + return g_memdup2 (argv, sizeof (char *) * (argc + 1)); } static void @@ -2318,7 +2320,7 @@ test_group_parse (void) g_option_context_add_group (context, group); argv = split_string ("program --test arg1 -f arg2 --group-test arg3 --frob arg4 -z arg5", &argc); - orig_argv = g_memdup (argv, (argc + 1) * sizeof (char *)); + orig_argv = g_memdup2 (argv, (argc + 1) * sizeof (char *)); retval = g_option_context_parse (context, &argc, &argv, &error); -- cgit v1.2.1 From 3e0bb3bf0f4b6ac43ab8aea531d804748b9b19a3 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 18 Mar 2021 10:31:00 +0000 Subject: ghash: Use g_memdup2() instead of g_memdup() Backport of part of commit 0736b7c1e7cf4232c5d7eb2b0fbfe9be81bd3baa to the simpler structure of the GHashTable code in glib-2-58. Helps: #2319 Signed-off-by: Simon McVittie --- glib/ghash.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/glib/ghash.c b/glib/ghash.c index 433004cf8..e36a7a8ed 100644 --- a/glib/ghash.c +++ b/glib/ghash.c @@ -34,6 +34,7 @@ #include "glib-private.h" #include "gstrfuncs.h" +#include "gstrfuncsprivate.h" #include "gatomic.h" #include "gtestutils.h" #include "gslice.h" @@ -967,7 +968,7 @@ g_hash_table_insert_node (GHashTable *hash_table, * split the table. */ if (G_UNLIKELY (hash_table->keys == hash_table->values && hash_table->keys[node_index] != new_value)) - hash_table->values = g_memdup (hash_table->keys, sizeof (gpointer) * hash_table->size); + hash_table->values = g_memdup2 (hash_table->keys, sizeof (gpointer) * hash_table->size); /* Step 3: Actually do the write */ hash_table->values[node_index] = new_value; -- cgit v1.2.1 From c921c82636681575be9a2669c1d7d8de1add43cd Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 4 Feb 2021 13:39:25 +0000 Subject: gobject: Use g_memdup2() instead of g_memdup() in obvious places MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert all the call sites which use `g_memdup()`’s length argument trivially (for example, by passing a `sizeof()`), so that they use `g_memdup2()` instead. In almost all of these cases the use of `g_memdup()` would not have caused problems, but it will soon be deprecated, so best port away from it. Signed-off-by: Philip Withnall Helps: #2319 (cherry picked from commit 6110caea45b235420b98cd41d845cc92238f6781) --- gobject/gsignal.c | 3 ++- gobject/gtype.c | 9 +++++---- gobject/gtypemodule.c | 3 ++- gobject/tests/param.c | 4 +++- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/gobject/gsignal.c b/gobject/gsignal.c index 76f1dc93c..4296eb290 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -28,6 +28,7 @@ #include #include "gsignal.h" +#include "gstrfuncsprivate.h" #include "gtype-private.h" #include "gbsearcharray.h" #include "gvaluecollector.h" @@ -1724,7 +1725,7 @@ g_signal_newv (const gchar *signal_name, node->single_va_closure_is_valid = FALSE; node->flags = signal_flags & G_SIGNAL_FLAGS_MASK; node->n_params = n_params; - node->param_types = g_memdup (param_types, sizeof (GType) * n_params); + node->param_types = g_memdup2 (param_types, sizeof (GType) * n_params); node->return_type = return_type; node->class_closure_bsa = NULL; if (accumulator) diff --git a/gobject/gtype.c b/gobject/gtype.c index 1acc0a075..bee83bdf5 100644 --- a/gobject/gtype.c +++ b/gobject/gtype.c @@ -33,6 +33,7 @@ #include "glib-private.h" #include "gconstructor.h" +#include "gstrfuncsprivate.h" #ifdef G_OS_WIN32 #include @@ -1467,7 +1468,7 @@ type_add_interface_Wm (TypeNode *node, iholder->next = iface_node_get_holders_L (iface); iface_node_set_holders_W (iface, iholder); iholder->instance_type = NODE_TYPE (node); - iholder->info = info ? g_memdup (info, sizeof (*info)) : NULL; + iholder->info = info ? g_memdup2 (info, sizeof (*info)) : NULL; iholder->plugin = plugin; /* create an iface entry for this type */ @@ -1728,7 +1729,7 @@ type_iface_retrieve_holder_info_Wm (TypeNode *iface, INVALID_RECURSION ("g_type_plugin_*", iholder->plugin, NODE_NAME (iface)); check_interface_info_I (iface, instance_type, &tmp_info); - iholder->info = g_memdup (&tmp_info, sizeof (tmp_info)); + iholder->info = g_memdup2 (&tmp_info, sizeof (tmp_info)); } return iholder; /* we don't modify write lock upon returning NULL */ @@ -2013,10 +2014,10 @@ type_iface_vtable_base_init_Wm (TypeNode *iface, IFaceEntry *pentry = type_lookup_iface_entry_L (pnode, iface); if (pentry) - vtable = g_memdup (pentry->vtable, iface->data->iface.vtable_size); + vtable = g_memdup2 (pentry->vtable, iface->data->iface.vtable_size); } if (!vtable) - vtable = g_memdup (iface->data->iface.dflt_vtable, iface->data->iface.vtable_size); + vtable = g_memdup2 (iface->data->iface.dflt_vtable, iface->data->iface.vtable_size); entry->vtable = vtable; vtable->g_type = NODE_TYPE (iface); vtable->g_instance_type = NODE_TYPE (node); diff --git a/gobject/gtypemodule.c b/gobject/gtypemodule.c index 4ecaf8c88..20911fafd 100644 --- a/gobject/gtypemodule.c +++ b/gobject/gtypemodule.c @@ -19,6 +19,7 @@ #include +#include "gstrfuncsprivate.h" #include "gtypeplugin.h" #include "gtypemodule.h" @@ -436,7 +437,7 @@ g_type_module_register_type (GTypeModule *module, module_type_info->loaded = TRUE; module_type_info->info = *type_info; if (type_info->value_table) - module_type_info->info.value_table = g_memdup (type_info->value_table, + module_type_info->info.value_table = g_memdup2 (type_info->value_table, sizeof (GTypeValueTable)); return module_type_info->type; diff --git a/gobject/tests/param.c b/gobject/tests/param.c index 758289bf8..971cff162 100644 --- a/gobject/tests/param.c +++ b/gobject/tests/param.c @@ -2,6 +2,8 @@ #include #include +#include "gstrfuncsprivate.h" + static void test_param_value (void) { @@ -851,7 +853,7 @@ main (int argc, char *argv[]) test_path = g_strdup_printf ("/param/implement/subprocess/%d-%d-%d-%d", data.change_this_flag, data.change_this_type, data.use_this_flag, data.use_this_type); - test_data = g_memdup (&data, sizeof (TestParamImplementData)); + test_data = g_memdup2 (&data, sizeof (TestParamImplementData)); g_test_add_data_func_full (test_path, test_data, test_param_implement_child, g_free); g_free (test_path); } -- cgit v1.2.1 From 2424eeaf90034057a71408f60eaf57608b24d731 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 4 Feb 2021 13:37:56 +0000 Subject: gio: Use g_memdup2() instead of g_memdup() in obvious places MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert all the call sites which use `g_memdup()`’s length argument trivially (for example, by passing a `sizeof()`), so that they use `g_memdup2()` instead. In almost all of these cases the use of `g_memdup()` would not have caused problems, but it will soon be deprecated, so best port away from it. Signed-off-by: Philip Withnall Helps: #2319 (cherry picked from commit be8834340a2d928ece82025463ae23dee2c333d0) --- gio/gdbusconnection.c | 5 +++-- gio/gdbusinterfaceskeleton.c | 3 ++- gio/gfile.c | 7 ++++--- gio/gsettingsschema.c | 5 +++-- gio/gwin32registrykey.c | 8 +++++--- gio/tests/async-close-output-stream.c | 6 ++++-- gio/tests/gdbus-export.c | 5 +++-- gio/win32/gwinhttpfile.c | 9 +++++---- 8 files changed, 29 insertions(+), 19 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 7270365b6..85ed1a3b3 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -110,6 +110,7 @@ #include "gasyncinitable.h" #include "giostream.h" #include "gasyncresult.h" +#include "gstrfuncsprivate.h" #include "gtask.h" #ifdef G_OS_UNIX @@ -3961,7 +3962,7 @@ _g_dbus_interface_vtable_copy (const GDBusInterfaceVTable *vtable) /* Don't waste memory by copying padding - remember to update this * when changing struct _GDBusInterfaceVTable in gdbusconnection.h */ - return g_memdup ((gconstpointer) vtable, 3 * sizeof (gpointer)); + return g_memdup2 ((gconstpointer) vtable, 3 * sizeof (gpointer)); } static void @@ -3978,7 +3979,7 @@ _g_dbus_subtree_vtable_copy (const GDBusSubtreeVTable *vtable) /* Don't waste memory by copying padding - remember to update this * when changing struct _GDBusSubtreeVTable in gdbusconnection.h */ - return g_memdup ((gconstpointer) vtable, 3 * sizeof (gpointer)); + return g_memdup2 ((gconstpointer) vtable, 3 * sizeof (gpointer)); } static void diff --git a/gio/gdbusinterfaceskeleton.c b/gio/gdbusinterfaceskeleton.c index 96bd520aa..672604c49 100644 --- a/gio/gdbusinterfaceskeleton.c +++ b/gio/gdbusinterfaceskeleton.c @@ -27,6 +27,7 @@ #include "gdbusprivate.h" #include "gdbusmethodinvocation.h" #include "gdbusconnection.h" +#include "gstrfuncsprivate.h" #include "gtask.h" #include "gioerror.h" @@ -697,7 +698,7 @@ add_connection_locked (GDBusInterfaceSkeleton *interface_, * properly before building the hooked_vtable, so we create it * once at the last minute. */ - interface_->priv->hooked_vtable = g_memdup (g_dbus_interface_skeleton_get_vtable (interface_), sizeof (GDBusInterfaceVTable)); + interface_->priv->hooked_vtable = g_memdup2 (g_dbus_interface_skeleton_get_vtable (interface_), sizeof (GDBusInterfaceVTable)); interface_->priv->hooked_vtable->method_call = skeleton_intercept_handle_method_call; } diff --git a/gio/gfile.c b/gio/gfile.c index a5709a4cc..d2314a463 100644 --- a/gio/gfile.c +++ b/gio/gfile.c @@ -60,6 +60,7 @@ #include "gasyncresult.h" #include "gioerror.h" #include "glibintl.h" +#include "gstrfuncsprivate.h" /** @@ -7738,7 +7739,7 @@ measure_disk_usage_progress (gboolean reporting, g_main_context_invoke_full (g_task_get_context (task), g_task_get_priority (task), measure_disk_usage_invoke_progress, - g_memdup (&progress, sizeof progress), + g_memdup2 (&progress, sizeof progress), g_free); } @@ -7756,7 +7757,7 @@ measure_disk_usage_thread (GTask *task, data->progress_callback ? measure_disk_usage_progress : NULL, task, &result.disk_usage, &result.num_dirs, &result.num_files, &error)) - g_task_return_pointer (task, g_memdup (&result, sizeof result), g_free); + g_task_return_pointer (task, g_memdup2 (&result, sizeof result), g_free); else g_task_return_error (task, error); } @@ -7780,7 +7781,7 @@ g_file_real_measure_disk_usage_async (GFile *file, task = g_task_new (file, cancellable, callback, user_data); g_task_set_source_tag (task, g_file_real_measure_disk_usage_async); - g_task_set_task_data (task, g_memdup (&data, sizeof data), g_free); + g_task_set_task_data (task, g_memdup2 (&data, sizeof data), g_free); g_task_set_priority (task, io_priority); g_task_run_in_thread (task, measure_disk_usage_thread); diff --git a/gio/gsettingsschema.c b/gio/gsettingsschema.c index 38c9d78b9..e23dd8c89 100644 --- a/gio/gsettingsschema.c +++ b/gio/gsettingsschema.c @@ -20,6 +20,7 @@ #include "gsettingsschema-internal.h" #include "gsettings.h" +#include "gstrfuncsprivate.h" #include "gvdb/gvdb-reader.h" #include "strinfo.c" @@ -1057,9 +1058,9 @@ g_settings_schema_list_children (GSettingsSchema *schema) if (g_str_has_suffix (key, "/")) { - gint length = strlen (key); + gsize length = strlen (key); - strv[j] = g_memdup (key, length); + strv[j] = g_memdup2 (key, length); strv[j][length - 1] = '\0'; j++; } diff --git a/gio/gwin32registrykey.c b/gio/gwin32registrykey.c index c19fede4e..619fd48af 100644 --- a/gio/gwin32registrykey.c +++ b/gio/gwin32registrykey.c @@ -28,6 +28,8 @@ #include #include +#include "gstrfuncsprivate.h" + #ifndef _WDMDDK_ typedef enum _KEY_INFORMATION_CLASS { KeyBasicInformation, @@ -247,7 +249,7 @@ g_win32_registry_value_iter_copy (const GWin32RegistryValueIter *iter) new_iter->value_name_size = iter->value_name_size; if (iter->value_data != NULL) - new_iter->value_data = g_memdup (iter->value_data, iter->value_data_size); + new_iter->value_data = g_memdup2 (iter->value_data, iter->value_data_size); new_iter->value_data_size = iter->value_data_size; @@ -268,8 +270,8 @@ g_win32_registry_value_iter_copy (const GWin32RegistryValueIter *iter) new_iter->value_data_expanded_charsize = iter->value_data_expanded_charsize; if (iter->value_data_expanded_u8 != NULL) - new_iter->value_data_expanded_u8 = g_memdup (iter->value_data_expanded_u8, - iter->value_data_expanded_charsize); + new_iter->value_data_expanded_u8 = g_memdup2 (iter->value_data_expanded_u8, + iter->value_data_expanded_charsize); new_iter->value_data_expanded_u8_size = iter->value_data_expanded_charsize; diff --git a/gio/tests/async-close-output-stream.c b/gio/tests/async-close-output-stream.c index 5f6620275..d3f97a119 100644 --- a/gio/tests/async-close-output-stream.c +++ b/gio/tests/async-close-output-stream.c @@ -24,6 +24,8 @@ #include #include +#include "gstrfuncsprivate.h" + #define DATA_TO_WRITE "Hello world\n" typedef struct @@ -147,9 +149,9 @@ prepare_data (SetupData *data, data->expected_size = g_memory_output_stream_get_data_size (G_MEMORY_OUTPUT_STREAM (data->data_stream)); - g_assert_cmpint (data->expected_size, >, 0); + g_assert_cmpuint (data->expected_size, >, 0); - data->expected_output = g_memdup (written, (guint)data->expected_size); + data->expected_output = g_memdup2 (written, data->expected_size); /* then recreate the streams and prepare them for the asynchronous close */ destroy_streams (data); diff --git a/gio/tests/gdbus-export.c b/gio/tests/gdbus-export.c index 4d6d3a43e..544b88653 100644 --- a/gio/tests/gdbus-export.c +++ b/gio/tests/gdbus-export.c @@ -23,6 +23,7 @@ #include #include "gdbus-tests.h" +#include "gstrfuncsprivate.h" /* all tests rely on a shared mainloop */ static GMainLoop *loop = NULL; @@ -652,7 +653,7 @@ subtree_introspect (GDBusConnection *connection, g_assert_not_reached (); } - return g_memdup (interfaces, 2 * sizeof (void *)); + return g_memdup2 (interfaces, 2 * sizeof (void *)); } static const GDBusInterfaceVTable * @@ -708,7 +709,7 @@ dynamic_subtree_introspect (GDBusConnection *connection, { const GDBusInterfaceInfo *interfaces[2] = { &dyna_interface_info, NULL }; - return g_memdup (interfaces, 2 * sizeof (void *)); + return g_memdup2 (interfaces, 2 * sizeof (void *)); } static const GDBusInterfaceVTable * diff --git a/gio/win32/gwinhttpfile.c b/gio/win32/gwinhttpfile.c index d5df16d91..f424d21cc 100644 --- a/gio/win32/gwinhttpfile.c +++ b/gio/win32/gwinhttpfile.c @@ -29,6 +29,7 @@ #include "gio/gfile.h" #include "gio/gfileattribute.h" #include "gio/gfileinfo.h" +#include "gstrfuncsprivate.h" #include "gwinhttpfile.h" #include "gwinhttpfileinputstream.h" #include "gwinhttpfileoutputstream.h" @@ -393,10 +394,10 @@ g_winhttp_file_resolve_relative_path (GFile *file, child = g_object_new (G_TYPE_WINHTTP_FILE, NULL); child->vfs = winhttp_file->vfs; child->url = winhttp_file->url; - child->url.lpszScheme = g_memdup (winhttp_file->url.lpszScheme, (winhttp_file->url.dwSchemeLength+1)*2); - child->url.lpszHostName = g_memdup (winhttp_file->url.lpszHostName, (winhttp_file->url.dwHostNameLength+1)*2); - child->url.lpszUserName = g_memdup (winhttp_file->url.lpszUserName, (winhttp_file->url.dwUserNameLength+1)*2); - child->url.lpszPassword = g_memdup (winhttp_file->url.lpszPassword, (winhttp_file->url.dwPasswordLength+1)*2); + child->url.lpszScheme = g_memdup2 (winhttp_file->url.lpszScheme, (winhttp_file->url.dwSchemeLength+1)*2); + child->url.lpszHostName = g_memdup2 (winhttp_file->url.lpszHostName, (winhttp_file->url.dwHostNameLength+1)*2); + child->url.lpszUserName = g_memdup2 (winhttp_file->url.lpszUserName, (winhttp_file->url.dwUserNameLength+1)*2); + child->url.lpszPassword = g_memdup2 (winhttp_file->url.lpszPassword, (winhttp_file->url.dwPasswordLength+1)*2); child->url.lpszUrlPath = wnew_path; child->url.dwUrlPathLength = wcslen (wnew_path); child->url.lpszExtraInfo = NULL; -- cgit v1.2.1 From 1436fedbab6eca8c8836075bb213c001acca85b6 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 18 Mar 2021 10:33:32 +0000 Subject: gvariant test: Use g_memdup2 This code no longer existed on the glib-2-66 branch, but it's present in glib-2-58. It's easier to verify that all potentially problematic g_memdup() uses have been replaced if we replace these too. Helps: #2319 Signed-off-by: Simon McVittie --- glib/tests/gvariant.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c index 18800f980..e6ac3241b 100644 --- a/glib/tests/gvariant.c +++ b/glib/tests/gvariant.c @@ -17,6 +17,7 @@ #include #include #include +#include "gstrfuncsprivate.h" #define BASIC "bynqiuxthdsog?" #define N_BASIC (G_N_ELEMENTS (BASIC) - 1) @@ -4779,7 +4780,7 @@ test_normal_checking_tuples (void) GVariant *variant = NULL; GVariant *normal_variant = NULL; - aligned_data = g_memdup (data, size); /* guarantee alignment */ + aligned_data = g_memdup2 (data, size); /* guarantee alignment */ variant = g_variant_new_from_data (G_VARIANT_TYPE_VARIANT, aligned_data, size, FALSE, NULL, NULL); g_assert_nonnull (variant); @@ -4908,7 +4909,7 @@ test_normal_checking_array_offsets (void) GVariant *variant = NULL; GVariant *normal_variant = NULL; - aligned_data = g_memdup (data, size); /* guarantee alignment */ + aligned_data = g_memdup2 (data, size); /* guarantee alignment */ variant = g_variant_new_from_data (G_VARIANT_TYPE_VARIANT, aligned_data, size, FALSE, NULL, NULL); g_assert_nonnull (variant); @@ -4935,7 +4936,7 @@ test_normal_checking_tuple_offsets (void) GVariant *variant = NULL; GVariant *normal_variant = NULL; - aligned_data = g_memdup (data, size); /* guarantee alignment */ + aligned_data = g_memdup2 (data, size); /* guarantee alignment */ variant = g_variant_new_from_data (G_VARIANT_TYPE_VARIANT, aligned_data, size, FALSE, NULL, NULL); g_assert_nonnull (variant); @@ -4962,7 +4963,7 @@ test_normal_checking_empty_object_path (void) GVariant *variant = NULL; GVariant *normal_variant = NULL; - aligned_data = g_memdup (data, size); /* guarantee alignment */ + aligned_data = g_memdup2 (data, size); /* guarantee alignment */ variant = g_variant_new_from_data (G_VARIANT_TYPE_VARIANT, aligned_data, size, FALSE, NULL, NULL); g_assert_nonnull (variant); -- cgit v1.2.1 From 5c26b6a7f656feeb09ecccb573e6ac6b9e9dbfd8 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 4 Feb 2021 16:12:24 +0000 Subject: gwinhttpfile: Avoid arithmetic overflow when calculating a size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The members of `URL_COMPONENTS` (`winhttp_file->url`) are `DWORD`s, i.e. 32-bit unsigned integers. Adding to and multiplying them may cause them to overflow the unsigned integer bounds, even if the result is passed to `g_memdup2()` which accepts a `gsize`. Cast the `URL_COMPONENTS` members to `gsize` first to ensure that the arithmetic is done in terms of `gsize`s rather than unsigned integers. Spotted by Sebastian Dröge. Signed-off-by: Philip Withnall Helps: #2319 (cherry picked from commit 0cbad673215ec8a049b7fe2ff44b0beed31b376e) --- gio/win32/gwinhttpfile.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gio/win32/gwinhttpfile.c b/gio/win32/gwinhttpfile.c index f424d21cc..e98031a98 100644 --- a/gio/win32/gwinhttpfile.c +++ b/gio/win32/gwinhttpfile.c @@ -394,10 +394,10 @@ g_winhttp_file_resolve_relative_path (GFile *file, child = g_object_new (G_TYPE_WINHTTP_FILE, NULL); child->vfs = winhttp_file->vfs; child->url = winhttp_file->url; - child->url.lpszScheme = g_memdup2 (winhttp_file->url.lpszScheme, (winhttp_file->url.dwSchemeLength+1)*2); - child->url.lpszHostName = g_memdup2 (winhttp_file->url.lpszHostName, (winhttp_file->url.dwHostNameLength+1)*2); - child->url.lpszUserName = g_memdup2 (winhttp_file->url.lpszUserName, (winhttp_file->url.dwUserNameLength+1)*2); - child->url.lpszPassword = g_memdup2 (winhttp_file->url.lpszPassword, (winhttp_file->url.dwPasswordLength+1)*2); + child->url.lpszScheme = g_memdup2 (winhttp_file->url.lpszScheme, ((gsize) winhttp_file->url.dwSchemeLength + 1) * 2); + child->url.lpszHostName = g_memdup2 (winhttp_file->url.lpszHostName, ((gsize) winhttp_file->url.dwHostNameLength + 1) * 2); + child->url.lpszUserName = g_memdup2 (winhttp_file->url.lpszUserName, ((gsize) winhttp_file->url.dwUserNameLength + 1) * 2); + child->url.lpszPassword = g_memdup2 (winhttp_file->url.lpszPassword, ((gsize) winhttp_file->url.dwPasswordLength + 1) * 2); child->url.lpszUrlPath = wnew_path; child->url.dwUrlPathLength = wcslen (wnew_path); child->url.lpszExtraInfo = NULL; -- cgit v1.2.1 From 7f308de341f2fd76d3f42f3e0e626683fef6ff0c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 4 Feb 2021 13:50:37 +0000 Subject: gwin32: Use gsize internally in g_wcsdup() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows it to handle strings up to length `G_MAXSIZE` — previously it would overflow with such strings. Update the several copies of it identically. Adapted for GLib 2.58 by Simon McVittie. Signed-off-by: Philip Withnall Helps: #2319 [Backport to 2.58 branch: g_wcsdup() existed in different places] Signed-off-by: Simon McVittie --- gio/gwin32appinfo.c | 2 +- gio/gwin32registrykey.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gio/gwin32appinfo.c b/gio/gwin32appinfo.c index 9f335b370..2a0fe380d 100644 --- a/gio/gwin32appinfo.c +++ b/gio/gwin32appinfo.c @@ -472,7 +472,7 @@ g_wcsdup (const gunichar2 *str, gssize str_size) str_size = wcslen (str) + 1; str_size *= sizeof (gunichar2); } - return g_memdup (str, str_size); + return g_memdup2 (str, str_size); } #define URL_ASSOCIATIONS L"HKEY_CURRENT_USER\\Software\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\" diff --git a/gio/gwin32registrykey.c b/gio/gwin32registrykey.c index 619fd48af..1c5526315 100644 --- a/gio/gwin32registrykey.c +++ b/gio/gwin32registrykey.c @@ -136,7 +136,7 @@ g_wcsdup (const gunichar2 *str, str_size = wcslen (str) + 1; str_size *= sizeof (gunichar2); } - return g_memdup (str, str_size); + return g_memdup2 (str, str_size); } /** -- cgit v1.2.1 From 84d7b850d76a46ba4a0cfeea5b4ecaa3a42e9b9e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 24 Feb 2021 17:33:38 +0000 Subject: glocalfileoutputstream: Fix a typo in a comment Signed-off-by: Philip Withnall (cherry picked from commit 78420a75aeb70569a8cd79fa0fea7b786b6f785f) --- gio/glocalfileoutputstream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c index 57d2d5dfe..8a68be3de 100644 --- a/gio/glocalfileoutputstream.c +++ b/gio/glocalfileoutputstream.c @@ -755,7 +755,7 @@ handle_overwrite_open (const char *filename, mode = mode_from_flags_or_info (flags, reference_info); /* We only need read access to the original file if we are creating a backup. - * We also add O_CREATE to avoid a race if the file was just removed */ + * We also add O_CREAT to avoid a race if the file was just removed */ if (create_backup || readable) open_flags = O_RDWR | O_CREAT | O_BINARY; else -- cgit v1.2.1 From 42020509ac9f0a13ced21871c7f5762f65bc142d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 24 Feb 2021 17:34:32 +0000 Subject: tests: Stop using g_test_bug_base() in file tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since a following commit is going to add a new test which references Gitlab, so it’s best to move the URI bases inside the test cases. Backported to GLib 2.58 by Simon McVittie. Signed-off-by: Philip Withnall (cherry-picked from commit 32d3d02a50e7dcec5f4cf7908e7ac88d575d8fc5) [GLib 2.58.x did not allow g_test_bug() without g_test_bug_base(), so use an empty string as the base] Signed-off-by: Simon McVittie --- gio/tests/file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gio/tests/file.c b/gio/tests/file.c index d2f147419..9c2af641f 100644 --- a/gio/tests/file.c +++ b/gio/tests/file.c @@ -679,7 +679,7 @@ test_replace_cancel (void) guint count; GError *error = NULL; - g_test_bug ("629301"); + g_test_bug ("https://bugzilla.gnome.org/629301"); path = g_dir_make_tmp ("g_file_replace_cancel_XXXXXX", &error); g_assert_no_error (error); @@ -1167,7 +1167,7 @@ main (int argc, char *argv[]) { g_test_init (&argc, &argv, NULL); - g_test_bug_base ("http://bugzilla.gnome.org/"); + g_test_bug_base (""); g_test_add_func ("/file/basic", test_basic); g_test_add_func ("/file/build-filename", test_build_filename); -- cgit v1.2.1 From b38fca2149a086b64198dd5b8ffe12209c513ad0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 10 Mar 2021 16:05:55 +0000 Subject: glocalfileoutputstream: Factor out a flag check This clarifies the code a little. It introduces no functional changes. Signed-off-by: Philip Withnall (cherry picked from commit ce0eb088a68171eed3ac217cb92a72e36eb57d1b) --- gio/glocalfileoutputstream.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c index 8a68be3de..8e0e793ff 100644 --- a/gio/glocalfileoutputstream.c +++ b/gio/glocalfileoutputstream.c @@ -751,6 +751,7 @@ handle_overwrite_open (const char *filename, int res; int mode; int errsv; + gboolean replace_destination_set = (flags & G_FILE_CREATE_REPLACE_DESTINATION); mode = mode_from_flags_or_info (flags, reference_info); @@ -858,7 +859,7 @@ handle_overwrite_open (const char *filename, * to a backup file and rewrite the contents of the file. */ - if ((flags & G_FILE_CREATE_REPLACE_DESTINATION) || + if (replace_destination_set || (!(original_stat.st_nlink > 1) && !is_symlink)) { char *dirname, *tmp_filename; @@ -877,7 +878,7 @@ handle_overwrite_open (const char *filename, /* try to keep permissions (unless replacing) */ - if ( ! (flags & G_FILE_CREATE_REPLACE_DESTINATION) && + if (!replace_destination_set && ( #ifdef HAVE_FCHOWN fchown (tmpfd, original_stat.st_uid, original_stat.st_gid) == -1 || @@ -1016,7 +1017,7 @@ handle_overwrite_open (const char *filename, } } - if (flags & G_FILE_CREATE_REPLACE_DESTINATION) + if (replace_destination_set) { g_close (fd, NULL); -- cgit v1.2.1 From 4e64a27f4f170860ddcf835ca6858bda09911a23 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 24 Feb 2021 17:36:07 +0000 Subject: glocalfileoutputstream: Fix CREATE_REPLACE_DESTINATION with symlinks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `G_FILE_CREATE_REPLACE_DESTINATION` flag is equivalent to unlinking the destination file and re-creating it from scratch. That did previously work, but in the process the code would call `open(O_CREAT)` on the file. If the file was a dangling symlink, this would create the destination file (empty). That’s not an intended side-effect, and has security implications if the symlink is controlled by a lower-privileged process. Fix that by not opening the destination file if it’s a symlink, and adjusting the rest of the code to cope with - the fact that `fd == -1` is not an error iff `is_symlink` is true, - and that `original_stat` will contain the `lstat()` results for the symlink now, rather than the `stat()` results for its target (again, iff `is_symlink` is true). This means that the target of the dangling symlink is no longer created, which was the bug. The symlink itself continues to be replaced (as before) with the new file — this is the intended behaviour of `g_file_replace()`. The behaviour for non-symlink cases, or cases where the symlink was not dangling, should be unchanged. Includes a unit test. Resolves CVE-2021-28153 (glib#2325). Backported to GLib 2.58 by Simon McVittie. Signed-off-by: Philip Withnall (cherry-picked from commit 317b3b587058a05dca95d56dac26568c5b098d33) [Backport to 2.58.x: replace g_local_file_fstat with fstat] [Backport to 2.58.x: replace g_local_file_lstat with lstat] [Backport to 2.58.x: replace _g_stat_mode with direct access to st_mode] [Backport to 2.58.x: don't call g_test_summary()] Signed-off-by: Simon McVittie --- gio/glocalfileoutputstream.c | 49 ++++++++++++++------ gio/tests/file.c | 107 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 14 deletions(-) diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c index 8e0e793ff..2be51ae12 100644 --- a/gio/glocalfileoutputstream.c +++ b/gio/glocalfileoutputstream.c @@ -779,16 +779,22 @@ handle_overwrite_open (const char *filename, /* Could be a symlink, or it could be a regular ELOOP error, * but then the next open will fail too. */ is_symlink = TRUE; - fd = g_open (filename, open_flags, mode); + if (!replace_destination_set) + fd = g_open (filename, open_flags, mode); } -#else - fd = g_open (filename, open_flags, mode); - errsv = errno; +#else /* if !O_NOFOLLOW */ /* This is racy, but we do it as soon as possible to minimize the race */ is_symlink = g_file_test (filename, G_FILE_TEST_IS_SYMLINK); + + if (!is_symlink || !replace_destination_set) + { + fd = g_open (filename, open_flags, mode); + errsv = errno; + } #endif - if (fd == -1) + if (fd == -1 && + (!is_symlink || !replace_destination_set)) { char *display_name = g_filename_display_name (filename); g_set_error (error, G_IO_ERROR, @@ -802,7 +808,10 @@ handle_overwrite_open (const char *filename, #ifdef G_OS_WIN32 res = GLIB_PRIVATE_CALL (g_win32_fstat) (fd, &original_stat); #else - res = fstat (fd, &original_stat); + if (!is_symlink) + res = fstat (fd, &original_stat); + else + res = lstat (filename, &original_stat); #endif errsv = errno; @@ -821,16 +830,27 @@ handle_overwrite_open (const char *filename, if (!S_ISREG (original_stat.st_mode)) { if (S_ISDIR (original_stat.st_mode)) - g_set_error_literal (error, - G_IO_ERROR, - G_IO_ERROR_IS_DIRECTORY, - _("Target file is a directory")); - else - g_set_error_literal (error, + { + g_set_error_literal (error, + G_IO_ERROR, + G_IO_ERROR_IS_DIRECTORY, + _("Target file is a directory")); + goto err_out; + } + else if (!is_symlink || +#ifdef S_ISLNK + !S_ISLNK (original_stat.st_mode) +#else + FALSE +#endif + ) + { + g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_NOT_REGULAR_FILE, _("Target file is not a regular file")); - goto err_out; + goto err_out; + } } if (etag != NULL) @@ -911,7 +931,8 @@ handle_overwrite_open (const char *filename, } } - g_close (fd, NULL); + if (fd >= 0) + g_close (fd, NULL); *temp_filename = tmp_filename; return tmpfd; } diff --git a/gio/tests/file.c b/gio/tests/file.c index 9c2af641f..1bdbe19b4 100644 --- a/gio/tests/file.c +++ b/gio/tests/file.c @@ -787,6 +787,112 @@ test_replace_cancel (void) g_object_unref (tmpdir); } +static void +test_replace_symlink (void) +{ +#ifdef G_OS_UNIX + gchar *tmpdir_path = NULL; + GFile *tmpdir = NULL, *source_file = NULL, *target_file = NULL; + GFileOutputStream *stream = NULL; + const gchar *new_contents = "this is a test message which should be written to source and not target"; + gsize n_written; + GFileEnumerator *enumerator = NULL; + GFileInfo *info = NULL; + gchar *contents = NULL; + gsize length = 0; + GError *local_error = NULL; + + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2325"); + + /* Create a fresh, empty working directory. */ + tmpdir_path = g_dir_make_tmp ("g_file_replace_symlink_XXXXXX", &local_error); + g_assert_no_error (local_error); + tmpdir = g_file_new_for_path (tmpdir_path); + + g_test_message ("Using temporary directory %s", tmpdir_path); + g_free (tmpdir_path); + + /* Create symlink `source` which points to `target`. */ + source_file = g_file_get_child (tmpdir, "source"); + target_file = g_file_get_child (tmpdir, "target"); + g_file_make_symbolic_link (source_file, "target", NULL, &local_error); + g_assert_no_error (local_error); + + /* Ensure that `target` doesn’t exist */ + g_assert_false (g_file_query_exists (target_file, NULL)); + + /* Replace the `source` symlink with a regular file using + * %G_FILE_CREATE_REPLACE_DESTINATION, which should replace it *without* + * following the symlink */ + stream = g_file_replace (source_file, NULL, FALSE /* no backup */, + G_FILE_CREATE_REPLACE_DESTINATION, NULL, &local_error); + g_assert_no_error (local_error); + + g_output_stream_write_all (G_OUTPUT_STREAM (stream), new_contents, strlen (new_contents), + &n_written, NULL, &local_error); + g_assert_no_error (local_error); + g_assert_cmpint (n_written, ==, strlen (new_contents)); + + g_output_stream_close (G_OUTPUT_STREAM (stream), NULL, &local_error); + g_assert_no_error (local_error); + + g_clear_object (&stream); + + /* At this point, there should still only be one file: `source`. It should + * now be a regular file. `target` should not exist. */ + enumerator = g_file_enumerate_children (tmpdir, + G_FILE_ATTRIBUTE_STANDARD_NAME "," + G_FILE_ATTRIBUTE_STANDARD_TYPE, + G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS, NULL, &local_error); + g_assert_no_error (local_error); + + info = g_file_enumerator_next_file (enumerator, NULL, &local_error); + g_assert_no_error (local_error); + g_assert_nonnull (info); + + g_assert_cmpstr (g_file_info_get_name (info), ==, "source"); + g_assert_cmpint (g_file_info_get_file_type (info), ==, G_FILE_TYPE_REGULAR); + + g_clear_object (&info); + + info = g_file_enumerator_next_file (enumerator, NULL, &local_error); + g_assert_no_error (local_error); + g_assert_null (info); + + g_file_enumerator_close (enumerator, NULL, &local_error); + g_assert_no_error (local_error); + g_clear_object (&enumerator); + + /* Double-check that `target` doesn’t exist */ + g_assert_false (g_file_query_exists (target_file, NULL)); + + /* Check the content of `source`. */ + g_file_load_contents (source_file, + NULL, + &contents, + &length, + NULL, + &local_error); + g_assert_no_error (local_error); + g_assert_cmpstr (contents, ==, new_contents); + g_assert_cmpuint (length, ==, strlen (new_contents)); + g_free (contents); + + /* Tidy up. */ + g_file_delete (source_file, NULL, &local_error); + g_assert_no_error (local_error); + + g_file_delete (tmpdir, NULL, &local_error); + g_assert_no_error (local_error); + + g_clear_object (&target_file); + g_clear_object (&source_file); + g_clear_object (&tmpdir); +#else /* if !G_OS_UNIX */ + g_test_skip ("Symlink replacement tests can only be run on Unix") +#endif +} + static void on_file_deleted (GObject *object, GAsyncResult *result, @@ -1182,6 +1288,7 @@ main (int argc, char *argv[]) g_test_add_data_func ("/file/async-create-delete/4096", GINT_TO_POINTER (4096), test_create_delete); g_test_add_func ("/file/replace-load", test_replace_load); g_test_add_func ("/file/replace-cancel", test_replace_cancel); + g_test_add_func ("/file/replace-symlink", test_replace_symlink); g_test_add_func ("/file/async-delete", test_async_delete); #ifdef G_OS_UNIX g_test_add_func ("/file/copy-preserve-mode", test_copy_preserve_mode); -- cgit v1.2.1 From 7bfe2dcebe59517e5d31c63b881c4227029a2fc4 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 24 Feb 2021 17:42:24 +0000 Subject: glocalfileoutputstream: Add a missing O_CLOEXEC flag to replace() Signed-off-by: Philip Withnall (cherry picked from commit 6c6439261bc7a8a0627519848a7222b3e1bd4ffe) --- gio/glocalfileoutputstream.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c index 2be51ae12..fff2439b9 100644 --- a/gio/glocalfileoutputstream.c +++ b/gio/glocalfileoutputstream.c @@ -56,6 +56,12 @@ #define O_BINARY 0 #endif +#ifndef O_CLOEXEC +#define O_CLOEXEC 0 +#else +#define HAVE_O_CLOEXEC 1 +#endif + struct _GLocalFileOutputStreamPrivate { char *tmp_filename; char *original_filename; @@ -1123,7 +1129,7 @@ _g_local_file_output_stream_replace (const char *filename, sync_on_close = FALSE; /* If the file doesn't exist, create it */ - open_flags = O_CREAT | O_EXCL | O_BINARY; + open_flags = O_CREAT | O_EXCL | O_BINARY | O_CLOEXEC; if (readable) open_flags |= O_RDWR; else @@ -1153,8 +1159,11 @@ _g_local_file_output_stream_replace (const char *filename, set_error_from_open_errno (filename, error); return NULL; } - - +#if !defined(HAVE_O_CLOEXEC) && defined(F_SETFD) + else + fcntl (fd, F_SETFD, FD_CLOEXEC); +#endif + stream = g_object_new (G_TYPE_LOCAL_FILE_OUTPUT_STREAM, NULL); stream->priv->fd = fd; stream->priv->sync_on_close = sync_on_close; -- cgit v1.2.1 From af39d83fc71408bca50f2cb3739099f36139281b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 16 Mar 2021 11:36:27 +0000 Subject: glocalfileoutputstream: Tidy up error handling After the recent reworking of this code it was possible for `g_close()` to be called on `fd == -1`, which is invalid. It would have reported an error, were errors not ignored. So it was harmless, but still best to fix. Simplify the error handling by combining both error labels and checking the state of `fd` dynamically. Coverity CID: #1450834 Signed-off-by: Philip Withnall (cherry picked from commit c4b4fecaef5fa6eac63569513511ba6f8674548a) --- gio/glocalfileoutputstream.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/gio/glocalfileoutputstream.c b/gio/glocalfileoutputstream.c index fff2439b9..8d7eadd95 100644 --- a/gio/glocalfileoutputstream.c +++ b/gio/glocalfileoutputstream.c @@ -829,7 +829,7 @@ handle_overwrite_open (const char *filename, _("Error when getting information for file “%s”: %s"), display_name, g_strerror (errsv)); g_free (display_name); - goto err_out; + goto error; } /* not a regular file */ @@ -841,7 +841,7 @@ handle_overwrite_open (const char *filename, G_IO_ERROR, G_IO_ERROR_IS_DIRECTORY, _("Target file is a directory")); - goto err_out; + goto error; } else if (!is_symlink || #ifdef S_ISLNK @@ -855,7 +855,7 @@ handle_overwrite_open (const char *filename, G_IO_ERROR, G_IO_ERROR_NOT_REGULAR_FILE, _("Target file is not a regular file")); - goto err_out; + goto error; } } @@ -869,7 +869,7 @@ handle_overwrite_open (const char *filename, G_IO_ERROR_WRONG_ETAG, _("The file was externally modified")); g_free (current_etag); - goto err_out; + goto error; } g_free (current_etag); } @@ -962,7 +962,7 @@ handle_overwrite_open (const char *filename, G_IO_ERROR_CANT_CREATE_BACKUP, _("Backup file creation failed")); g_free (backup_filename); - goto err_out; + goto error; } bfd = g_open (backup_filename, @@ -976,7 +976,7 @@ handle_overwrite_open (const char *filename, G_IO_ERROR_CANT_CREATE_BACKUP, _("Backup file creation failed")); g_free (backup_filename); - goto err_out; + goto error; } /* If needed, Try to set the group of the backup same as the @@ -993,7 +993,7 @@ handle_overwrite_open (const char *filename, g_unlink (backup_filename); g_close (bfd, NULL); g_free (backup_filename); - goto err_out; + goto error; } if ((original_stat.st_gid != tmp_statbuf.st_gid) && @@ -1010,7 +1010,7 @@ handle_overwrite_open (const char *filename, g_unlink (backup_filename); g_close (bfd, NULL); g_free (backup_filename); - goto err_out; + goto error; } } #endif @@ -1025,7 +1025,7 @@ handle_overwrite_open (const char *filename, g_close (bfd, NULL); g_free (backup_filename); - goto err_out; + goto error; } g_close (bfd, NULL); @@ -1040,7 +1040,7 @@ handle_overwrite_open (const char *filename, g_io_error_from_errno (errsv), _("Error seeking in file: %s"), g_strerror (errsv)); - goto err_out; + goto error; } } @@ -1056,7 +1056,7 @@ handle_overwrite_open (const char *filename, g_io_error_from_errno (errsv), _("Error removing old file: %s"), g_strerror (errsv)); - goto err_out2; + goto error; } if (readable) @@ -1073,7 +1073,7 @@ handle_overwrite_open (const char *filename, _("Error opening file “%s”: %s"), display_name, g_strerror (errsv)); g_free (display_name); - goto err_out2; + goto error; } } else @@ -1091,15 +1091,16 @@ handle_overwrite_open (const char *filename, g_io_error_from_errno (errsv), _("Error truncating file: %s"), g_strerror (errsv)); - goto err_out; + goto error; } } return fd; - err_out: - g_close (fd, NULL); - err_out2: +error: + if (fd >= 0) + g_close (fd, NULL); + return -1; } -- cgit v1.2.1