[Ecls-list] ECL 10.3.1 GC finalization bug and patch

[Ram Krishnan]

Hi,

There's a fairly serious bug in the GC finalization code in
10.3.1 (and 9.12.3). The effect of the bug is that if a program
allocates multiple ffi objects, and calls SET-FINALIZER on them, then
under some circumstances the finalizer may be called with a completely
bogus pointer object, or just crash with a SIGSEGV. The issue is in
the queueing_finalizer() function in src/c/alloc_2.d.

The line:

GC_register_finalizer_no_order(aux,
(GC_finalization_proc*)group_finalizer, NULL, &ofn, &odata);

should really be:

GC_register_finalizer_no_order(l,
(GC_finalization_proc*)group_finalizer, NULL, &ofn, &odata);

i.e., the first arg should be the head of the queue (l) not the tail.

Otherwise, the queued finalization records could end up being total
trash. See attached patch (gc_finalization-patch.txt) for a fix to the
corruption issue.

However, while this fixes the corruption, the user finalization code
is no longer invoked. I believe the effect of queueing_finalizer() is
to that the `queue' (list of finalization records) remains live, and
isn't gc'ed.

After staring at this function for a while I gave up on what the
motivation behind it is. All that queueing_finalizer() does is to
create a list of the objects and their finalizers, which were passed
to it, so that those finalizers may be called at some later point. I
can't figure out what the benefit of deferring these finalization
calls is. Anyway, the simplest fix is to not do any queueing, and just
invoke the finalizer, like so:

static void
basic_finalizer(cl_object o, cl_object finalizer)
{
	if (finalizer != Cnil && finalizer != NULL) {
		if (finalizer == Ct) {
			CL_NEWENV_BEGIN {
				standard_finalizer(o);
			} CL_NEWENV_END;
		} else {
			CL_NEWENV_BEGIN {
				cl_funcall(2, finalizer, o);
			} CL_NEWENV_END;
		}
	}
}

And, change ecl_set_finalizer_unprotected() and si_get_finalizer() to
use this basic_finalizer() instead. I've attached another patch
(gc_finalization-patch-2.txt) which includes this and the first fix as
well.

If there's a reason for the current behavior of queueing_finalizer(),
I look forward to understanding it.

Cheers,

-ram
-------------- next part --------------
Index: src/c/alloc_2.d
===================================================================
RCS file: /cvsroot/ecls/ecl/src/c/alloc_2.d,v
retrieving revision 1.136
diff -u -r1.136 alloc_2.d
--- src/c/alloc_2.d	28 Feb 2010 14:21:21 -0000	1.136
+++ src/c/alloc_2.d	14 Mar 2010 00:36:03 -0000
@@ -1097,7 +1097,7 @@
 				void *odata;
 				cl_core.to_be_finalized = aux;
 				ecl_disable_interrupts_env(the_env);
-				GC_register_finalizer_no_order(aux, (GC_finalization_proc*)group_finalizer, NULL, &ofn, &odata);
+				GC_register_finalizer_no_order(l, (GC_finalization_proc*)group_finalizer, NULL, &ofn, &odata);
 				ecl_enable_interrupts_env(the_env);
 			} else {
 				ECL_RPLACD(l, aux);
@@ -1106,6 +1106,22 @@
 	}
 }
 
+static void
+basic_finalizer(cl_object o, cl_object finalizer)
+{
+	if (finalizer != Cnil && finalizer != NULL) {
+		if (finalizer == Ct) {
+			CL_NEWENV_BEGIN {
+				standard_finalizer(o);
+			} CL_NEWENV_END;
+		} else {
+			CL_NEWENV_BEGIN {
+				cl_funcall(2, finalizer, o);
+			} CL_NEWENV_END;
+		}
+	}
+}
+
 cl_object
 si_get_finalizer(cl_object o)
 {
@@ -1117,7 +1133,7 @@
 	GC_register_finalizer_no_order(o, (GC_finalization_proc)0, 0, &ofn, &odata);
 	if (ofn == 0) {
 		output = Cnil;
-	} else if (ofn == (GC_finalization_proc)queueing_finalizer) {
+	} else if (ofn == (GC_finalization_proc)basic_finalizer) {
 		output = (cl_object)odata;
 	} else {
 		output = Cnil;
@@ -1137,7 +1153,7 @@
 					       0, &ofn, &odata);
 	} else {
 		GC_finalization_proc newfn;
-		newfn = (GC_finalization_proc)queueing_finalizer;
+		newfn = (GC_finalization_proc)basic_finalizer;
 		GC_register_finalizer_no_order(o, newfn, finalizer,
 					       &ofn, &odata);
 	}
-------------- next part --------------
Index: src/c/alloc_2.d
===================================================================
RCS file: /cvsroot/ecls/ecl/src/c/alloc_2.d,v
retrieving revision 1.136
diff -u -r1.136 alloc_2.d
--- src/c/alloc_2.d	28 Feb 2010 14:21:21 -0000	1.136
+++ src/c/alloc_2.d	14 Mar 2010 00:31:32 -0000
@@ -1097,7 +1097,7 @@
 				void *odata;
 				cl_core.to_be_finalized = aux;
 				ecl_disable_interrupts_env(the_env);
-				GC_register_finalizer_no_order(aux, (GC_finalization_proc*)group_finalizer, NULL, &ofn, &odata);
+				GC_register_finalizer_no_order(l, (GC_finalization_proc*)group_finalizer, NULL, &ofn, &odata);
 				ecl_enable_interrupts_env(the_env);
 			} else {
 				ECL_RPLACD(l, aux);