#1331: buffer overflow fix

glib/garray.c

@@ -107,7 +107,7 @@ struct _GRealArray
 {
   guint8 *data;
   guint   len;
-  guint   alloc;
+  guint   elt_capacity;
   guint   elt_size;
   guint   zero_terminated : 1;
   guint   clear : 1;
@@ -150,7 +150,7 @@ struct _GRealArray
  * Returns: the element of the #GArray at the index given by @i
  */
 
-#define g_array_elt_len(array,i) ((array)->elt_size * (i))
+#define g_array_elt_len(array,i) ((gsize)(array)->elt_size * (i))
 #define g_array_elt_pos(array,i) ((array)->data + g_array_elt_len((array),(i)))
 #define g_array_elt_zero(array, pos, len)                               \
   (memset (g_array_elt_pos ((array), pos), 0,  g_array_elt_len ((array), len)))
@@ -159,7 +159,7 @@ struct _GRealArray
     g_array_elt_zero ((array), (array)->len, 1);                        \
 }G_STMT_END
 
-static guint g_nearest_pow        (guint       num) G_GNUC_CONST;
+static gsize g_nearest_pow        (gsize       num) G_GNUC_CONST;
 static void  g_array_maybe_expand (GRealArray *array,
                                    guint       len);
 
@@ -181,6 +181,7 @@ g_array_new (gboolean zero_terminated,
              guint    elt_size)
 {
   g_return_val_if_fail (elt_size > 0, NULL);
+  g_return_val_if_fail (elt_size <= G_MAXSIZE / 2 - 1, NULL);
 
   return g_array_sized_new (zero_terminated, clear, elt_size, 0);
 }
@@ -232,7 +233,7 @@ g_array_steal (GArray *array,
 
   rarray->data  = NULL;
   rarray->len   = 0;
-  rarray->alloc = 0;
+  rarray->elt_capacity = 0;
   return segment;
 }
 
@@ -261,12 +262,13 @@ g_array_sized_new (gboolean zero_terminated,
   GRealArray *array;
   
   g_return_val_if_fail (elt_size > 0, NULL);
+  g_return_val_if_fail (elt_size <= G_MAXSIZE, NULL);
 
   array = g_slice_new (GRealArray);
 
   array->data            = NULL;
   array->len             = 0;
-  array->alloc           = 0;
+  array->elt_capacity = 0;
   array->zero_terminated = (zero_terminated ? 1 : 0);
   array->clear           = (clear ? 1 : 0);
   array->elt_size        = elt_size;
@@ -471,7 +473,7 @@ array_free (GRealArray     *array,
     {
       array->data            = NULL;
       array->len             = 0;
-      array->alloc           = 0;
+      array->elt_capacity = 0;
     }
   else
     {
@@ -966,22 +968,22 @@ g_array_binary_search (GArray        *array,
   return result;
 }
 
-/* Returns the smallest power of 2 greater than n, or n if
+/* Returns the smallest power of 2 greater than or equal to n,
- * such power does not fit in a guint
+ * or 0 if such power does not fit in a gsize
  */
-static guint
+static gsize
-g_nearest_pow (guint num)
+g_nearest_pow (gsize num)
 {
-  guint n = num - 1;
+  gsize n = num - 1;
 
-  g_assert (num > 0);
+  g_assert (num > 0 && num <= G_MAXSIZE / 2);
 
   n |= n >> 1;
   n |= n >> 2;
   n |= n >> 4;
   n |= n >> 8;
   n |= n >> 16;
-#if SIZEOF_INT == 8
+#if GLIB_SIZEOF_SIZE_T == 8
   n |= n >> 32;
 #endif
 
@@ -992,26 +994,32 @@ static void
 g_array_maybe_expand (GRealArray *array,
                       guint       len)
 {
-  guint want_alloc;
+  guint max_len, want_len;
+ 
+  /* The maximum array length is derived from following constraints:
+   * - The number of bytes must fit into a gsize / 2.
+   * - The number of elements must fit into guint.
+   * - zero terminated arrays must leave space for the terminating element
+   */
+  max_len = MIN (G_MAXSIZE / 2 / array->elt_size, G_MAXUINT) - array->zero_terminated;
 
   /* Detect potential overflow */
-  if G_UNLIKELY ((G_MAXUINT - array->len) < len)
+  if G_UNLIKELY ((max_len - array->len) < len)
     g_error ("adding %u to array would overflow", len);
 
-  want_alloc = g_array_elt_len (array, array->len + len +
+  want_len = array->len + len + array->zero_terminated;
-                                array->zero_terminated);
+  if (want_len > array->elt_capacity)
-
-  if (want_alloc > array->alloc)
     {
-      want_alloc = g_nearest_pow (want_alloc);
+      gsize want_alloc = g_nearest_pow (g_array_elt_len (array, want_len));
       want_alloc = MAX (want_alloc, MIN_ARRAY_SIZE);
 
       array->data = g_realloc (array->data, want_alloc);
 
       if (G_UNLIKELY (g_mem_gc_friendly))
-        memset (array->data + array->alloc, 0, want_alloc - array->alloc);
+        memset (g_array_elt_pos (array, array->elt_capacity), 0,
+                g_array_elt_len (array, want_len - array->elt_capacity));
 
-      array->alloc = want_alloc;
+      array->elt_capacity = want_alloc / array->elt_size;
     }
 }
 
@@ -1297,7 +1305,7 @@ g_array_copy (GArray *array)
 
   new_rarray =
     (GRealArray *) g_array_sized_new (rarray->zero_terminated, rarray->clear,
-                                      rarray->elt_size, rarray->alloc / rarray->elt_size);
+                                      rarray->elt_size, rarray->elt_capacity);
   new_rarray->len = rarray->len;
   if (rarray->len > 0)
     memcpy (new_rarray->data, rarray->data, rarray->len * rarray->elt_size);
@@ -2298,7 +2306,6 @@ g_byte_array_new_take (guint8 *data,
   GRealArray *real;
 
   g_return_val_if_fail (len <= G_MAXUINT, NULL);
-
   array = g_byte_array_new ();
   real = (GRealArray *)array;
   g_assert (real->data == NULL);
@@ -2306,7 +2313,7 @@ g_byte_array_new_take (guint8 *data,
 
   real->data = data;
   real->len = len;
-  real->alloc = len;
+  real->elt_capacity = len;
 
   return array;
 }

glib/tests/array-test.c

@@ -845,6 +845,45 @@ test_array_copy_sized (void)
   g_array_unref (array1);
 }
 
+static void
+array_overflow_append_vals (void)
+{
+  if (!g_test_undefined ())
+      return;
+
+  if (g_test_subprocess ())
+    {
+      GArray *array = g_array_new (TRUE, FALSE, 1);
+      /* Check for overflow should happen before data is accessed. */
+      g_array_append_vals (array, NULL, G_MAXUINT);
+    }
+  else
+    {
+      g_test_trap_subprocess (NULL, 0, 0);
+      g_test_trap_assert_failed ();
+      g_test_trap_assert_stderr ("*adding 4294967295 to array would overflow*");
+    }
+}
+
+static void
+array_overflow_set_size (void)
+{
+  if (!g_test_undefined ())
+      return;
+
+  if (g_test_subprocess ())
+    {
+      GArray *array = g_array_new (TRUE, FALSE, 1);
+      g_array_set_size (array, G_MAXUINT);
+    }
+  else
+    {
+      g_test_trap_subprocess (NULL, 0, 0);
+      g_test_trap_assert_failed ();
+      g_test_trap_assert_stderr ("*adding 4294967295 to array would overflow*");
+    }
+}
+
 /* Check g_ptr_array_steal() function */
 static void
 pointer_array_steal (void)
@@ -1643,6 +1682,26 @@ pointer_array_steal_index (void)
   g_assert_cmpuint (i4, ==, 1);
 }
 
+static void
+byte_array_new_take_overflow (void)
+{
+#if G_MAXSIZE <= G_MAXUINT
+  g_test_skip ("Overflow test requires G_MAXSIZE > G_MAXUINT.");
+#else
+  GByteArray* arr;
+
+  if (!g_test_undefined ())
+      return;
+
+  /* Check for overflow should happen before data is accessed. */
+  g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_CRITICAL,
+                          "*assertion 'len <= G_MAXUINT' failed");
+  arr = g_byte_array_new_take (NULL, (gsize)G_MAXUINT + 1);
+  g_assert_null (arr);
+  g_test_assert_expected_messages ();
+#endif
+}
+
 static void
 byte_array_steal (void)
 {
@@ -1998,6 +2057,8 @@ main (int argc, char *argv[])
   g_test_add_func ("/array/clear-func", array_clear_func);
   g_test_add_func ("/array/binary-search", test_array_binary_search);
   g_test_add_func ("/array/copy-sized", test_array_copy_sized);
+  g_test_add_func ("/array/overflow-append-vals", array_overflow_append_vals);
+  g_test_add_func ("/array/overflow-set-size", array_overflow_set_size);
 
   for (i = 0; i < G_N_ELEMENTS (array_configurations); i++)
     {
@@ -2043,6 +2104,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/bytearray/sort", byte_array_sort);
   g_test_add_func ("/bytearray/sort-with-data", byte_array_sort_with_data);
   g_test_add_func ("/bytearray/new-take", byte_array_new_take);
+  g_test_add_func ("/bytearray/new-take-overflow", byte_array_new_take_overflow);
   g_test_add_func ("/bytearray/free-to-bytes", byte_array_free_to_bytes);
 
   return g_test_run ();