[PATCH] avoid GC segfault/hang/etc: make gnulib's hash module use GC

Jim Meyering jim at meyering.net
Mon Aug 1 15:54:48 UTC 2011


Pete Zaitcev wrote:
> On Fri, 29 Jul 2011 19:39:16 +0200
> Jim Meyering <jim at meyering.net> wrote:
>
>> Here's a tentative patch for problems that arise only on rawhide (now F16).
>> I'll test more and give more detail in the log before pushing this.
>
>> +++ b/gl/lib/hash.c.diff
>> @@ -0,0 +1,13 @@
>> +diff --git a/lib/hash.c b/lib/hash.c
>> +index f3de2aa..27f080e 100644
>> +--- a/lib/hash.c
>> ++++ b/lib/hash.c
>> +@@ -43,6 +43,8 @@
>> + # endif
>> + #endif
>> +
>> ++#include "../gc-wrap.h"
>> ++
>> + struct hash_entry
>
> I ack this.

Thanks.

> However, it's not all.
> To help the problem happen more often, I added a periodic call to
> GC_gcollect(). I figured it should be harmless, since it's used for
> leak checks officially. With that in place, iwhd crashes rather
> predictably in the same place... and continues to crash with your
> patch applied.
>
> As it turned out, GC cannot track my_state, because Microhttpd
> stores a pointer to it in a thread-local area. The attached patch
> demonstrates the problem: as soon as I add a tracking list, GC sees
> the structure and does not collect it.
>
> I don't know how sane it is to expend lists just to tell GC that
> we know where something is. Surely there ought to be a better way.

Very nice hack.
At first glance, it does seem like there should be a better way,
but then you wonder... what about when that TLS-stored pointer is
overwritten and its buffer becomes collectible?

I've adjusted my patch and will push it shortly:
- added a decent log, referencing the commit that is being partially reverted.
- also revert the finalizer-related change
- fix a stale comment

As for your additional fix, please retain the GC_gcollect-calling
code one way or another -- at the very least #ifdef'd out.  What do
you think about leaving it enabled?  Does it impose a significant
performance hit?

Re your own list code, please use what you put in that patch,
at least for now.  Simpler is better, especially for a bug-fixing change.

I'll add a NEWS entry separately.


>From fc6da23112019269f798952c365af1a9d5a59938 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Fri, 29 Jul 2011 19:08:48 +0200
Subject: [PATCH] avoid GC segfault/hang/etc: make gnulib's hash module use GC

While commit 0b0c8f73 arranged to free the non-GC'd hash tables, it
was erroneous.  That change led to our storing GC-malloc'd provider
names (and other info) in a non-GC'd hash table, which meant that the
garbage collector would collect/free unref'd (as far as it could see)
but still-in-use buffers.

This change reinstates the gnulib-hash.c-patching hack and removes
the now-unnecessary finalizer-hook code that was added by the above
commit.
* bootstrap.conf (avoided_gnulib_modules): Add --avoid=hash-tests.
* gl/lib/hash.c.diff: New file.
* rest.c (destroy_state_postprocessor): Don't free hash tables
explicitly, now that they're all GC'd.
(gc_register_finalizer_ms): Correct stale comment.
---
 bootstrap.conf     |    2 ++
 gl/lib/hash.c.diff |   13 +++++++++++++
 rest.c             |    5 +----
 3 files changed, 16 insertions(+), 4 deletions(-)
 create mode 100644 gl/lib/hash.c.diff

diff --git a/bootstrap.conf b/bootstrap.conf
index 46bfa52..ddebcbb 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -16,10 +16,12 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.

 # The lock test takes too long for my taste.
+# The hash test would need linking help to get -lgc.
 avoided_gnulib_modules='
   --avoid=lock-tests
   --avoid=dummy
   --avoid=fflush-tests
+  --avoid=hash-tests
 '

 # gnulib modules used by this package.
diff --git a/gl/lib/hash.c.diff b/gl/lib/hash.c.diff
new file mode 100644
index 0000000..517d4e0
--- /dev/null
+++ b/gl/lib/hash.c.diff
@@ -0,0 +1,13 @@
+diff --git a/lib/hash.c b/lib/hash.c
+index f3de2aa..27f080e 100644
+--- a/lib/hash.c
++++ b/lib/hash.c
+@@ -43,6 +43,8 @@
+ # endif
+ #endif
+
++#include "../gc-wrap.h"
++
+ struct hash_entry
+   {
+     void *data;
diff --git a/rest.c b/rest.c
index 11b2971..cd41dd2 100644
--- a/rest.c
+++ b/rest.c
@@ -864,15 +864,12 @@ destroy_state_postprocessor (void *ms_v, void *client_data)
 	my_state *ms = ms_v;
 	if (ms->post)
 		MHD_destroy_post_processor (ms->post);
-	if (ms->dict)
-		hash_free (ms->dict);
 }

 /* Tell the garbage collector that when freeing MS, it must invoke
    destroy_state_postprocessor(MS).  This is required for each ms->post
    since they're allocated via MHD_create_post_processor, which is
-   in a separate library into which the GC has no view.
-   Likewise for ms->dict, ms->query and ms->aquery.  */
+   in a separate library into which the GC has no view.  */
 static void
 gc_register_finalizer_ms(void *ms)
 {
--
1.7.6.347.g4db0d


More information about the iwhd-devel mailing list