Astonishing allocation bug in glib-2.16.4 compiled with gcc 2.96

[Ale2008]

Hi,
I've compiled glib, and make check worked fine. Next, I compiled the
"mono" package, which uses it. It compiled almost ok (few twists
needed on a pre-C99 compiler) but make check freezes. After some
tinkering I found out that freezing occurs because of circular list.

The functions involved only call glib's list find, append, and
eventually free functions. I'm quite astonished at serendipitously
finding such a paramount bug in a frequently used package. However, I
patched g_slist_append like so:

--- glib/gslist.original.c Wed Jul  2 00:30:12 2008
+++ glib/gslist.c Sun Jul 13 11:33:06 2008
@@ -121,7 +121,9 @@
    if (list)
      {
        last = g_slist_last (list);
-      /* g_assert (last != NULL); */
+      if (last == NULL || last->next != NULL || last == new_list)
+        g_print("GOT BUG: last=%p, next=%p, new=%p\n",
+          last, last->next, new_list);
        last->next = new_list;

        return list;

and then while checking "mono", it triggered it

GOT BUG: last=0x82b61a0, next=(nil), new=0x82b61a0

Although I'm inclined to think the bug is due to the old compiler, I
cannot change it because that would require altering libc on the
target system (built on RedHat 7.2) installed by the hardware vendor,
and that's out of my reach.

In facts, 0x82b61a0 was used and freed various times before eventually
being allocated twice on two consecutive loops of the client code. The
client function (visit_bb in mono's liveness.c) starts with an empty
list and appends elements as it recursively walks a tree, in order to
avoid visiting the same elements. The bug reproduces deterministically
every time I launch that specific invocation of "mono". However, the
allocation code in gslice.c is quite intricate and it's not clear to
me how to proceed. I'll try and reconfigure glib with CFLAGS="-O0 -g",
and see if that changes anything. Can anybody suggest anything better?

TIA
Ale
_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Ale2008]

I wrote:
>
> GOT BUG: last=0x82b61a0, next=(nil), new=0x82b61a0
> [...]
> I'll try and reconfigure glib with CFLAGS="-O0 -g", and
> see if that changes anything.

No changes. Even the memory address is the same!
_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Ale2008]

I wrote:
>
> GOT BUG: last=0x82b61a0, next=(nil), new=0x82b61a0
>
> Although I'm inclined to think the bug is due to the old compiler

And I was wrong

> In facts, 0x82b61a0 was used and freed various times before eventually
> being allocated twice on two consecutive loops of the client code. [...]

And it is enough for the client app to free unallocated pointers in
order to get the same chunk of memory allocated as often as desired.
Unfortunately, I discovered that only after reading the sources and
understanding how magazine1 and 2 interact. At that point discovering
how to set G_SLICE=debug-blocks has been a breeze.

So, I *don't* apologize for that false report :-) Both because nobody
saved me a few hours of code reading on this sunny sunday, and because
that debug feature deserves a more thorough advertising: It is
nonsensical that client test suites exist without it.

Thanks for a nice lib, anyway, I'm starting to appreciate it...

Ciao
Ale
--
I'm unsubscribing... please cc any response to me

_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Kalle Vahlman]

2008/7/13 Alessandro Vesely <vesely@...>:
> So, I *don't* apologize for that false report :-) Both because nobody saved
> me a few hours of code reading on this sunny sunday, and because that debug
> feature deserves a more thorough advertising: It is nonsensical that client
> test suites exist without it.

Well, it *is* mentioned in the docs, with explanations:

  http://library.gnome.org/devel/glib/stable/glib-running.html

;)

Also, since it apparently activates different code paths than normal
operation, I'm not sure if it makes sense using that in the test
suite...

--
Kalle Vahlman, zuh@...
Powered by http://movial.fi
Interesting stuff at http://syslog.movial.fi
_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Ale2008]

Kalle Vahlman wrote:
> Well, it *is* mentioned in the docs, with explanations:
>
>   http://library.gnome.org/devel/glib/stable/glib-running.html
>
> ;)

Alas, I missed it. However, I guess mono developers read it. After
googling mono-project.com for G_SLICE, I'd say that page didn't work ;-)

> Also, since it apparently activates different code paths than normal
> operation, I'm not sure if it makes sense using that in the test
> suite...

I didn't mean glib's test suite. What I wanted to say is that any
client program that has its own test suite should enable debug-blocks
in it. The above page should explicitly encourage such practice, IMHO.
Overlapping variables not always result in program crashes, it can be
worse.
_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Sven Herzberg]

Am Dienstag, den 15.07.2008, 14:13 +0200 schrieb Alessandro Vesely:
> I didn't mean glib's test suite. What I wanted to say is that any
> client program that has its own test suite should enable debug-blocks
> in it. The above page should explicitly encourage such practice, IMHO.
> Overlapping variables not always result in program crashes, it can be
> worse.

Feel free to provide a patch for the documentation. It's all in glib's
subversion repository.

Regards,
  Sven
--
Sven Herzberg
Imendio AB - Expert solutions in GTK+
http://www.imendio.com

_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Ale2008]

Sven Herzberg wrote:
> Am Dienstag, den 15.07.2008, 14:13 +0200 schrieb Alessandro Vesely:
>> I didn't mean glib's test suite. What I wanted to say is that any
>> client program that has its own test suite should enable debug-blocks
>> in it. The above page should explicitly encourage such practice, IMHO.
>> Overlapping variables not always result in program crashes, it can be
>> worse.
>
> Feel free to provide a patch for the documentation. It's all in glib's
> subversion repository.

What would you say about that


Index: running.sgml
===================================================================
--- running.sgml (revision 7183)
+++ running.sgml (working copy)
@@ -149,14 +149,16 @@
   which performs sanity checks on the released memory slices.
   Invalid slice adresses or slice sizes will be reported and lead to
   a program halt.
-  This option should only be used in debugging scenarios, because it
-  significantly degrades GSlice performance. Extra per slice memory
-  is requied to do the necessary bookeeping, and multi-thread scalability
-  is given up to perform global slice validation.
-  This option is mostly useful in scenarios where program crashes are encountered
-  while GSlice is in use, but crashes cannot be reproduced with G_SLICE=always-malloc.
-  A potential cause for such a situation that will be caught by G_SLICE=debug-blocks
-  is e.g.:
+  This option should always be used in debugging scenarios.
+  In particular, client packages sporting their own test suite should
+  <emphasis>always enable this option when running tests</emphasis>.
+  Global slice validation is ensured by adding bookeeping information to each
+  allocation and maintaining a global hash table of that data. Although
+  multi-thread scalability is given up, the resulting code usually performs
+  acceptably well, and better than comparable memory checking carried out using
+  external tools. An example of a memory corruption scenario that cannot be
+  reproduced with <literal>G_SLICE=always-malloc</literal>, but will be caught
+  by <literal>G_SLICE=debug-blocks</literal> is as follows:
   <programlisting>
     void *slist = g_slist_alloc(); /* void* gives up type-safety */
     g_list_free (slist);           /* corruption: sizeof (GSList) != sizeof (GList) */

_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Sven Herzberg]

Hi Alessandro,

Am Dienstag, den 15.07.2008, 17:04 +0200 schrieb Alessandro Vesely:
> What would you say about that

It's pretty nice, I only have two comments:

 * you change "only for debugging" to "always for debugging", maybe you
   should simply use "iff" (or the expanded version "if and only if")
 * you drop the section about downsides of the advanced debugging,
   please keep them in there

Regards,
  Sven
--
Sven Herzberg
Imendio AB - Expert solutions in GTK+
http://www.imendio.com

_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Ale2008]

Sven Herzberg wrote:
> Hi Alessandro,
>
> Am Dienstag, den 15.07.2008, 17:04 +0200 schrieb Alessandro Vesely:
>> What would you say about that
>
> It's pretty nice, I only have two comments:
>
>  * you change "only for debugging" to "always for debugging", maybe you
>    should simply use "iff" (or the expanded version "if and only if")

Ehm... well, actually I hope some native English speaker may retouch
it before it is possibly committed. :-)

>  * you drop the section about downsides of the advanced debugging,
>    please keep them in there

I hadn't dropped it. I just reworded it, keeping mentions to added
memory stuff and mutex-acquired global vision.

This discussion reminds me that smc_notify_tree() does not actually
check which thread does a chunk belong to. Could that result in
misbehavior? For example, it looks perfectly legal for a producer
consumer pair of threads to carefully smuggle thread-categorized
chunks of memory from the former to the latter thread; or should that
deserve an optimization warning?

Maybe the doc should be more precise. I attach a second attempt.

Index: running.sgml
===================================================================
--- running.sgml (revision 7183)
+++ running.sgml (working copy)
@@ -149,14 +149,17 @@
   which performs sanity checks on the released memory slices.
   Invalid slice adresses or slice sizes will be reported and lead to
   a program halt.
-  This option should only be used in debugging scenarios, because it
-  significantly degrades GSlice performance. Extra per slice memory
-  is requied to do the necessary bookeeping, and multi-thread scalability
-  is given up to perform global slice validation.
-  This option is mostly useful in scenarios where program crashes are encountered
-  while GSlice is in use, but crashes cannot be reproduced with G_SLICE=always-malloc.
-  A potential cause for such a situation that will be caught by G_SLICE=debug-blocks
-  is e.g.:
+  This option is for debugging scenarios.
+  In particular, client packages sporting their own test suite should
+  <emphasis>always enable this option when running tests</emphasis>.
+  Global slice validation is ensured by storing size and address information
+  for each allocated chunk, and maintaining a global hash table of that data.
+  That way, multi-thread scalability is given up, and memory consumption is
+  increased. However, the resulting code usually performs acceptably well,
+  possibly better than with comparable memory checking carried out using
+  external tools. An example of a memory corruption scenario that cannot be
+  reproduced with <literal>G_SLICE=always-malloc</literal>, but will be caught
+  by <literal>G_SLICE=debug-blocks</literal> is as follows:
   <programlisting>
     void *slist = g_slist_alloc(); /* void* gives up type-safety */
     g_list_free (slist);           /* corruption: sizeof (GSList) != sizeof (GList) */

_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Bastien Nocera]

On Tue, 2008-07-15 at 17:19 +0200, Sven Herzberg wrote:
> Hi Alessandro,
>
> Am Dienstag, den 15.07.2008, 17:04 +0200 schrieb Alessandro Vesely:
> > What would you say about that
>
> It's pretty nice, I only have two comments:
>
>  * you change "only for debugging" to "always for debugging", maybe you
>    should simply use "iff" (or the expanded version "if and only if")

"iff" is maths speak, not English. Don't use maths speak in devel docs
unless you're certain your target users will understand it. In this
case, I'd have taken it for a typo, as I had never seen "iff" used.


_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list

[Tim Janik-2]

On Tue, 15 Jul 2008, Alessandro Vesely wrote:

> This discussion reminds me that smc_notify_tree() does not actually check
> which thread does a chunk belong to. Could that result in misbehavior?

No, chunks may be freely passed back and forth betwen threads without
problems. Except for a few blocks that fit into 2*magazine_size, all
allocations are shared between all threads anyway.

---
ciaoTJ
_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@...
http://mail.gnome.org/mailman/listinfo/gtk-devel-list