Re: Fix to long-standing crashes in GC

[storm@cua.dk (Kim F. Storm)]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> > I have now installed some further changes to the GC code to
> > fix problems with invalid Lisp_Misc objects.
> 
> > The main problem was that marker blocks could be freed while
> > still being pointed to by buffer undo lists, but I made other
> > minor changes to make the code more robust.
> 
> I still don't understand how this could happen.

In retrospect, the problem was/is caused by the method used to mark
the undo-list (the step that your patch removes).

The problem with the current undo-list marking is that we still mark
the cons cells which points to the markers that we might later free
and remove from the undo-list.

This meant that we would not free those cons cells which thus ends up
pointing to invalid marker objects.

Now, when we on next gc calls mark_stack, and thus mark_maybe_object /
mark_maybe_pointer, we may accidentally have a pointer address that
leads to a (valid) cons, which is checked with mark_maybe_object...
that's ok, you see that it points to a valid cons block, and it is not
marked Vdead.

So you do mark_object on that cons, meaning that you unconditionally
does mark_object on the car and cdr.

The cdr of that cons is another cons, so you do mark_object (unconditionally)
on the car of that cons -- but it happened to be a cons cell which used to
be on an undo-list, so its car points to a lisp_free_marker -- or worse
into a freed marker block.


My changes focused on fixing the effects of the erroneous marking of
those cons cells, so part of my fix was to expclicitly clear (Qnil)
the car and cdr of the two cons cells that are removed from the
undo-list; that was necessary as the removal was done AFTER gc_sweep.


Now, looking at your alternative patch, it elegantly eliminates the
bogus marking of the cons cells that are removed from the undo-list,
so I think your patch will also fix the problem.

Your change is indeed cleaner than my fixes, so let's install your
patch instead.


BTW, I think that clearing the cons cells before strings is still a
proper sequence in gc_sweep, as the old sequence (that your patch
reinstalls)  implies that after cleaning the strings, there are
cons cells (unmarked though) that points into freed memory.

I.e. I suggest that the following part of the patch is omitted:

@@ -5202,6 +5152,16 @@
 static void
 gc_sweep ()
 {
+  /* Remove or mark entries in weak hash tables.
+     This must be done before any object is unmarked.  */
+  sweep_weak_hash_tables ();
+
+  sweep_strings ();
+#ifdef GC_CHECK_STRING_BYTES
+  if (!noninteractive)
+    check_string_bytes (1);
+#endif
+
   /* Put all unmarked conses on free list */
   {
     register struct cons_block *cblk;
@@ -5252,16 +5212,6 @@
     total_free_conses = num_free;
   }
 
-  /* Remove or mark entries in weak hash tables.
-     This must be done before any object is unmarked.  */
-  sweep_weak_hash_tables ();
-
-  sweep_strings ();
-#ifdef GC_CHECK_STRING_BYTES
-  if (!noninteractive)
-    check_string_bytes (1);
-#endif
-
   /* Put all unmarked floats on free list */
   {
     register struct float_block *fblk;

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk