From e5ed410c8c0fe823883b65b293fb2d9c9d12673a Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 2 Dec 2016 10:03:16 +0000 Subject: [PATCH] Avoid calling Standard C string/array functions with NULL arguments glibc string.h declares memcpy() with attribute(nonnull(1,2)), causing calls with NULL arguments to be treated as undefined behaviour. This is consistent with ISO C99 and C11, which state that passing 0 to string functions as an array length does not remove the requirement that the pointer to the array is a valid pointer. gcc -fsanitize=undefined catches this while running OSTree's test suite. Similarly, running the GLib test suite reports similar issues for qsort(), memmove(), memcmp(). Signed-off-by: Simon McVittie Bug: https://bugzilla.gnome.org/show_bug.cgi?id=775510 Reviewed-by: Colin Walters --- gio/gconverterinputstream.c | 12 ++++++++---- gio/gconverteroutputstream.c | 8 +++++--- gio/gdesktopappinfo.c | 13 ++++++++----- gio/gunixsocketaddress.c | 4 +++- gio/gvdb/gvdb-builder.c | 3 ++- glib/garray.c | 9 +++++++++ glib/goption.c | 5 ++++- glib/gstrfuncs.c | 2 +- glib/gtestutils.h | 2 +- gobject/gtype.c | 3 ++- 10 files changed, 43 insertions(+), 18 deletions(-) diff --git a/gio/gconverterinputstream.c b/gio/gconverterinputstream.c index d23b8f269..5fb26cbaf 100644 --- a/gio/gconverterinputstream.c +++ b/gio/gconverterinputstream.c @@ -262,7 +262,9 @@ buffer_read (Buffer *buffer, char *dest, gsize count) { - memcpy (dest, buffer->data + buffer->start, count); + if (count != 0) + memcpy (dest, buffer->data + buffer->start, count); + buffer_consumed (buffer, count); } @@ -293,9 +295,11 @@ grow_buffer (Buffer *buffer) data = g_malloc (size); in_buffer = buffer_data_size (buffer); - memcpy (data, - buffer->data + buffer->start, - in_buffer); + if (in_buffer != 0) + memcpy (data, + buffer->data + buffer->start, + in_buffer); + g_free (buffer->data); buffer->data = data; buffer->end -= buffer->start; diff --git a/gio/gconverteroutputstream.c b/gio/gconverteroutputstream.c index 46616b1a4..eef277eda 100644 --- a/gio/gconverteroutputstream.c +++ b/gio/gconverteroutputstream.c @@ -300,9 +300,11 @@ grow_buffer (Buffer *buffer) data = g_malloc (size); in_buffer = buffer_data_size (buffer); - memcpy (data, - buffer->data + buffer->start, - in_buffer); + if (in_buffer != 0) + memcpy (data, + buffer->data + buffer->start, + in_buffer); + g_free (buffer->data); buffer->data = data; buffer->end -= buffer->start; diff --git a/gio/gdesktopappinfo.c b/gio/gdesktopappinfo.c index 4668ea7a2..81dff27c9 100644 --- a/gio/gdesktopappinfo.c +++ b/gio/gdesktopappinfo.c @@ -506,7 +506,8 @@ add_token_result (const gchar *app_name, static void merge_token_results (gboolean first) { - qsort (static_token_results, static_token_results_size, sizeof (struct search_result), compare_results); + if (static_token_results_size != 0) + qsort (static_token_results, static_token_results_size, sizeof (struct search_result), compare_results); /* If this is the first token then we are basically merging a list with * itself -- we only perform de-duplication. @@ -606,7 +607,8 @@ reset_total_search_results (void) static void sort_total_search_results (void) { - qsort (static_total_results, static_total_results_size, sizeof (struct search_result), compare_categories); + if (static_total_results_size != 0) + qsort (static_total_results, static_total_results_size, sizeof (struct search_result), compare_categories); } static void @@ -620,9 +622,10 @@ merge_directory_results (void) static_total_results = g_renew (struct search_result, static_total_results, static_total_results_allocated); } - memcpy (static_total_results + static_total_results_size, - static_search_results, - static_search_results_size * sizeof (struct search_result)); + if (static_total_results + static_total_results_size != 0) + memcpy (static_total_results + static_total_results_size, + static_search_results, + static_search_results_size * sizeof (struct search_result)); static_total_results_size += static_search_results_size; diff --git a/gio/gunixsocketaddress.c b/gio/gunixsocketaddress.c index f4fc07758..3b2a1c486 100644 --- a/gio/gunixsocketaddress.c +++ b/gio/gunixsocketaddress.c @@ -116,7 +116,9 @@ g_unix_socket_address_set_property (GObject *object, /* Clip to fit in UNIX_PATH_MAX with zero termination or first byte */ len = MIN (array->len, UNIX_PATH_MAX-1); - memcpy (address->priv->path, array->data, len); + if (len != 0) + memcpy (address->priv->path, array->data, len); + address->priv->path[len] = 0; /* Ensure null-terminated */ address->priv->path_len = len; } diff --git a/gio/gvdb/gvdb-builder.c b/gio/gvdb/gvdb-builder.c index 48fe3e41c..43ad06869 100644 --- a/gio/gvdb/gvdb-builder.c +++ b/gio/gvdb/gvdb-builder.c @@ -291,7 +291,8 @@ file_builder_add_string (FileBuilder *fb, chunk->offset = fb->offset; chunk->size = length; chunk->data = g_malloc (length); - memcpy (chunk->data, string, length); + if (length != 0) + memcpy (chunk->data, string, length); *start = guint32_to_le (fb->offset); *size = guint16_to_le (length); diff --git a/glib/garray.c b/glib/garray.c index 0b4bf045b..ac7e58035 100644 --- a/glib/garray.c +++ b/glib/garray.c @@ -415,6 +415,9 @@ g_array_append_vals (GArray *farray, g_return_val_if_fail (array, NULL); + if (len == 0) + return farray; + g_array_maybe_expand (array, len); memcpy (g_array_elt_pos (array, array->len), data, @@ -468,6 +471,9 @@ g_array_prepend_vals (GArray *farray, g_return_val_if_fail (array, NULL); + if (len == 0) + return farray; + g_array_maybe_expand (array, len); memmove (g_array_elt_pos (array, len), g_array_elt_pos (array, 0), @@ -517,6 +523,9 @@ g_array_insert_vals (GArray *farray, g_return_val_if_fail (array, NULL); + if (len == 0) + return farray; + g_array_maybe_expand (array, len); memmove (g_array_elt_pos (array, len + index_), diff --git a/glib/goption.c b/glib/goption.c index e7d35bca8..7a21d1ccc 100644 --- a/glib/goption.c +++ b/glib/goption.c @@ -2361,7 +2361,10 @@ g_option_group_add_entries (GOptionGroup *group, group->entries = g_renew (GOptionEntry, group->entries, group->n_entries + n_entries); - memcpy (group->entries + group->n_entries, entries, sizeof (GOptionEntry) * n_entries); + /* group->entries could be NULL in the trivial case where we add no + * entries to no entries */ + if (n_entries != 0) + memcpy (group->entries + group->n_entries, entries, sizeof (GOptionEntry) * n_entries); for (i = group->n_entries; i < group->n_entries + n_entries; i++) { diff --git a/glib/gstrfuncs.c b/glib/gstrfuncs.c index f8dc0b13f..eb0f1deff 100644 --- a/glib/gstrfuncs.c +++ b/glib/gstrfuncs.c @@ -386,7 +386,7 @@ g_memdup (gconstpointer mem, { gpointer new_mem; - if (mem) + if (mem && byte_size != 0) { new_mem = g_malloc (byte_size); memcpy (new_mem, mem, byte_size); diff --git a/glib/gtestutils.h b/glib/gtestutils.h index d788eb94b..99e237dc0 100644 --- a/glib/gtestutils.h +++ b/glib/gtestutils.h @@ -74,7 +74,7 @@ typedef void (*GTestFixtureFunc) (gpointer fixture, if (__l1 != __l2) \ g_assertion_message_cmpnum (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \ #l1 " (len(" #m1 ")) == " #l2 " (len(" #m2 "))", __l1, "==", __l2, 'i'); \ - else if (memcmp (__m1, __m2, __l1) != 0) \ + else if (__l1 != 0 && memcmp (__m1, __m2, __l1) != 0) \ g_assertion_message (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, \ "assertion failed (" #m1 " == " #m2 ")"); \ } G_STMT_END diff --git a/gobject/gtype.c b/gobject/gtype.c index 48290ae39..f381a7876 100644 --- a/gobject/gtype.c +++ b/gobject/gtype.c @@ -3550,7 +3550,8 @@ g_type_children (GType type, G_READ_LOCK (&type_rw_lock); /* ->children is relocatable */ children = g_new (GType, node->n_children + 1); - memcpy (children, node->children, sizeof (GType) * node->n_children); + if (node->n_children != 0) + memcpy (children, node->children, sizeof (GType) * node->n_children); children[node->n_children] = 0; if (n_children)