Bug in garray.c

[Sebastian Wilhelmi <wilhelmi ira uka de>]

Hi everyone,

I just noted, that there is a bug in garray.c:

Take a 'zero-terminated' (and/or 'clear'-ed) array. Append two non-null
elements. Set the size to 0. Then set the size to 1. Now element 0 and element
1 are non-zero, thus making this array not zero-terminated nor cleared.

Appended is a patch to solve that. Furthermore I added some macros
(g_array_elt_pos and g_array_elt_len) to make the raw memory accesing code a
bit more readable.

You might wonder, why the zero-initialization of memory is now done in the
g_array_..._vals functions and not in g_array_maybe_expand and
g_array_set_size. The answer is, that all such solutions (at least those, I
found) could lead to lots of unwanted reinitialization in certain
circumstances. Now a cell is set to zero, exacly, when it needs to be done.

The same applies to GPtrArray, which is not supposed to be zero_terminated but
should be cleared. So I removed the final 
   
   array->pdata[array->len - 1] = NULL;

in the g_ptr_array_remove[_remove] functions. This should be done for
G_MEMROF_FRIENDLY (see my other mail about memprof friendlyness) however.

Because I suppose, none will reply to this mail, here's my ultimatum: I'll
check it in on Friday, when no complaints come in. This is not an offense.
It's just, that some mails really stay unanswered and thus I don't know, what
to do. The same applies to my last mail about memprof friendlyness. But I'm
not going to act there. So I simply consider myself as a GLib developer and go
ahead, but I don't want to offend you. So this is a matter of workability, not
of "I just do, what I want anyway".

I'm however not very sure, I want to check that in on glib-1-2. Even though it
is a bug with a fix, there were no bugreports on that issue so far, so I would
like somebody to review the changes, before they go into stable.

Bye,
Sebastian.
-- 
Sebastian Wilhelmi                   |            här ovanför alla molnen
mailto:wilhelmi@ira.uka.de           |     är himmlen så förunderligt blå
http://goethe.ira.uka.de/~wilhelmi   |
Index: garray.c
===================================================================
RCS file: /cvs/gnome/glib/garray.c,v
retrieving revision 1.14
diff -u -b -B -r1.14 garray.c
--- garray.c	1999/02/24 06:13:29	1.14
+++ garray.c	2000/03/20 15:19:46
@@ -47,6 +46,13 @@
   guint   clear : 1;
 };
 
+#define g_array_elt_len(array,i) ((array)->elt_size * (i))
+#define g_array_elt_pos(array,i) ((array)->data + g_array_elt_len((array),(i)))
+#define g_array_zero_terminate(array) G_STMT_START{			\
+  if ((array)->zero_terminated)						\
+    memset (g_array_elt_pos ((array), (array)->len), 0, 		\
+	    g_array_elt_len ((array), 1));				\
+}G_STMT_END
 
 static gint g_nearest_pow        (gint        num);
 static void g_array_maybe_expand (GRealArray *array,
@@ -102,10 +108,13 @@
 
   g_array_maybe_expand (array, len);
 
-  memcpy (array->data + array->elt_size * array->len, data, array->elt_size * len);
+  memcpy (g_array_elt_pos (array, array->len), data, 
+	  g_array_elt_len (array, len));
 
   array->len += len;
 
+  g_array_zero_terminate (array);
+
   return farray;
 }
 
@@ -118,12 +127,15 @@
 
   g_array_maybe_expand (array, len);
 
-  g_memmove (array->data + array->elt_size * len, array->data, array->elt_size * array->len);
+  g_memmove (g_array_elt_pos (array, len), g_array_elt_pos (array, 0), 
+	     g_array_elt_len (array, array->len));
 
-  memcpy (array->data, data, len * array->elt_size);
+  memcpy (g_array_elt_pos (array, 0), data, g_array_elt_len (array, len));
 
   array->len += len;
 
+  g_array_zero_terminate (array);
+
   return farray;
 }
 
@@ -137,14 +149,16 @@
 
   g_array_maybe_expand (array, len);
 
-  g_memmove (array->data + array->elt_size * (len + index), 
-	     array->data + array->elt_size * index, 
-	     array->elt_size * (array->len - index));
+  g_memmove (g_array_elt_pos (array, len + index), 
+	     g_array_elt_pos (array, index), 
+	     g_array_elt_len (array, array->len - index));
 
-  memcpy (array->data + array->elt_size * index, data, len * array->elt_size);
+  memcpy (g_array_elt_pos (array, index), data, g_array_elt_len (array, len));
 
   array->len += len;
 
+  g_array_zero_terminate (array);
+
   return farray;
 }
 
@@ -153,12 +167,22 @@
 		  guint   length)
 {
   GRealArray *array = (GRealArray*) farray;
-
-  if (array->len < length)
-    g_array_maybe_expand (array, length - array->len);
+  guint old_len = array->len;
 
   array->len = length;
 
+  if (old_len < length)
+    {
+      g_array_maybe_expand (array, length - old_len);
+      
+      if (array->clear || array->zero_terminated)
+	memset (g_array_elt_pos (array, old_len), 0,
+		g_array_elt_len (array, length - old_len + 
+				 array->zero_terminated));
+    }
+  else
+    g_array_zero_terminate (array);
+  
   return farray;
 }
 
@@ -173,16 +197,14 @@
   g_return_val_if_fail (index >= 0 && index < array->len, NULL);
 
   if (index != array->len - 1)
-      g_memmove (array->data + array->elt_size * index, 
-		 array->data + array->elt_size * (index + 1), 
-		 array->elt_size * (array->len - index - 1));
-  
-  if (array->zero_terminated)
-    memset (array->data + array->elt_size * (array->len - 1), 0, 
-	    array->elt_size);
+    g_memmove (g_array_elt_pos (array, index),
+	       g_array_elt_pos (array, index + 1),
+	       g_array_elt_len (array, array->len - index - 1));
 
   array->len -= 1;
 
+  g_array_zero_terminate (array);
+
   return farray;
 }
 
@@ -197,16 +219,14 @@
   g_return_val_if_fail (index >= 0 && index < array->len, NULL);
 
   if (index != array->len - 1)
-    g_memmove (array->data + array->elt_size * index, 
-	       array->data + array->elt_size * (array->len - 1), 
-	       array->elt_size);
-  
-  if (array->zero_terminated)
-    memset (array->data + array->elt_size * (array->len - 1), 0, 
-	    array->elt_size);
+    g_memmove (g_array_elt_pos (array, index), 
+	       g_array_elt_pos (array, array->len - 1),
+	       g_array_elt_len (array, 1));
 
   array->len -= 1;
 
+  g_array_zero_terminate (array);
+
   return farray;
 }
 
@@ -225,19 +245,15 @@
 g_array_maybe_expand (GRealArray *array,
 		      gint        len)
 {
-  guint want_alloc = (array->len + len + array->zero_terminated) * array->elt_size;
+  guint want_alloc = g_array_elt_len (array, array->len + len + 
+				      array->zero_terminated);
 
   if (want_alloc > array->alloc)
     {
-      guint old_alloc = array->alloc;
-
       array->alloc = g_nearest_pow (want_alloc);
       array->alloc = MAX (array->alloc, MIN_ARRAY_SIZE);
 
       array->data = g_realloc (array->data, array->alloc);
-
-      if (array->clear || array->zero_terminated)
-	memset (array->data + old_alloc, 0, array->alloc - old_alloc);
     }
 }
 
@@ -299,20 +315,11 @@
 g_ptr_array_maybe_expand (GRealPtrArray *array,
 			  gint        len)
 {
-  guint old_alloc;
-
   if ((array->len + len) > array->alloc)
     {
-      old_alloc = array->alloc;
-
       array->alloc = g_nearest_pow (array->len + len);
       array->alloc = MAX (array->alloc, MIN_ARRAY_SIZE);
-      if (array->pdata)
 	array->pdata = g_realloc (array->pdata, sizeof(gpointer) * array->alloc);
-      else
-	array->pdata = g_new0 (gpointer, array->alloc);
-
-      memset (array->pdata + old_alloc, 0, array->alloc - old_alloc);
     }
 }
 
@@ -325,7 +332,11 @@
   g_return_if_fail (array);
 
   if (length > array->len)
+    {
     g_ptr_array_maybe_expand (array, (length - array->len));
+      memset (array->pdata + array->len, 0,
+	      sizeof (gpointer) * (length - array->len));
+    }
 
   array->len = length;
 }
@@ -347,8 +358,6 @@
     g_memmove (array->pdata + index, array->pdata + index + 1, 
 	       sizeof (gpointer) * (array->len - index - 1));
   
-  array->pdata[array->len - 1] = NULL;
-
   array->len -= 1;
 
   return result;
@@ -369,8 +378,6 @@
   
   if (index != array->len - 1)
     array->pdata[index] = array->pdata[array->len - 1];
-
-  array->pdata[array->len - 1] = NULL;
 
   array->len -= 1;