bug#30931: abort() due to CHECK_ALLOCATED_AND_LIVE failure during GC

[Noam Postavsky]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 03/30/2018 05:56 AM, Noam Postavsky wrote:
>> Shouldn't we at least do set-marker?
>>
>>      Fset_marker (XCAR (data), Qnil, Qnil)
>>      Fset_marker (XCDR (data), Qnil, Qnil)
>
> Possibly. And perhaps we should making similar changes to the other
> two calls to free_marker (in insdel.c's signal_before_change),

Oh, good catch.  Here's a test-case for that one:

  (with-temp-buffer
    (insert "1234567890")
    (setq buffer-undo-list nil)
    (let ((before-change-functions
           (list (lambda (beg end)
                   (delete-region (1- beg) (1+ end))))))
      (delete-region 2 5))
    (princ (format "%S" buffer-undo-list) #'external-debugging-output)
    (type-of (car (nth 3 buffer-undo-list))))

This prints

    (("678" . 1) ("12345" . 1) (#<marker in no buffer> . -1) (#<misc free cell> 
. -1) (#<misc free cell> . -4))

and then aborts.

> and removing the definition of free_marker while we're at it.

I think that is sensible, see following patch.

>From cf327524c9d66969051f429da6f004003850ffef Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 30 Mar 2018 16:44:24 -0400
Subject: [PATCH] Fix another case of freed markers in the undo-list
 (Bug#30931)

* src/alloc.c (free_marker): Remove.
* src/editfns.c (save_restriction_restore):
* src/insdel.c (signal_before_change): Detach the markers from the
buffer when we're done with them instead of calling free_marker on
them.
* test/src/editfns-tests.el (delete-region-undo-markers-1)
(delete-region-undo-markers-2): New tests.
---
 src/alloc.c               |  9 ---------
 src/editfns.c             | 10 ++++++----
 src/insdel.c              |  7 +++++--
 src/lisp.h                |  1 -
 test/src/editfns-tests.el | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 369592d70e..9fdcc7306a 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -3878,15 +3878,6 @@ build_marker (struct buffer *buf, ptrdiff_t charpos, 
ptrdiff_t bytepos)
   return obj;
 }
 
-/* Put MARKER back on the free list after using it temporarily.  */
-
-void
-free_marker (Lisp_Object marker)
-{
-  unchain_marker (XMARKER (marker));
-  free_misc (marker);
-}
-

 /* Return a newly created vector or string with specified arguments as
    elements.  If all the arguments are characters that can fit
diff --git a/src/editfns.c b/src/editfns.c
index 727f2d0080..84de679273 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -3899,10 +3899,12 @@ save_restriction_restore (Lisp_Object data)
 
          buf->clip_changed = 1; /* Remember that the narrowing changed. */
        }
-      /* This isn’t needed anymore, so don’t wait for GC.
-         Do not call free_marker on XCAR (data) or XCDR (data),
-         though, since record_marker_adjustments may have put
-         them on the buffer’s undo list (Bug#30931).  */
+      /* This isn’t needed anymore, so don’t wait for GC.  Do not call
+         free_marker on XCAR (data) or XCDR (data), though, since
+         record_marker_adjustments may have put them on the buffer’s
+         undo list (Bug#30931).  Just detach them instead.  */
+      Fset_marker (XCAR (data), Qnil, Qnil);
+      Fset_marker (XCDR (data), Qnil, Qnil);
       free_cons (XCONS (data));
     }
   else
diff --git a/src/insdel.c b/src/insdel.c
index 02e3f41bc9..697395c507 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -2148,10 +2148,13 @@ signal_before_change (ptrdiff_t start_int, ptrdiff_t 
end_int,
                                   FETCH_START, FETCH_END, Qnil);
     }
 
+  /* Detach the markers now that we're done with them.  Don't directly
+     free them, since the change functions could have caused them to
+     be inserted into the undo list (Bug#30931).  */
   if (! NILP (start_marker))
-    free_marker (start_marker);
+    Fset_marker (start_marker, Qnil, Qnil);
   if (! NILP (end_marker))
-    free_marker (end_marker);
+    Fset_marker (end_marker, Qnil, Qnil);
   RESTORE_VALUE;
 
   unbind_to (count, Qnil);
diff --git a/src/lisp.h b/src/lisp.h
index b931d23bf3..f471b21658 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3812,7 +3812,6 @@ make_uninit_sub_char_table (int depth, int min_char)
 extern Lisp_Object make_save_memory (Lisp_Object *, ptrdiff_t);
 extern void free_save_value (Lisp_Object);
 extern Lisp_Object build_overlay (Lisp_Object, Lisp_Object, Lisp_Object);
-extern void free_marker (Lisp_Object);
 extern void free_cons (struct Lisp_Cons *);
 extern void init_alloc_once (void);
 extern void init_alloc (void);
diff --git a/test/src/editfns-tests.el b/test/src/editfns-tests.el
index 442ad08937..2e20c9dd12 100644
--- a/test/src/editfns-tests.el
+++ b/test/src/editfns-tests.el
@@ -288,4 +288,55 @@ transpose-test-get-byte-positions
                  (buffer-string)
                  "foo bar baz qux"))))))
 
+(ert-deftest delete-region-undo-markers-1 ()
+  "Make sure we don't end up with freed markers reachable from Lisp."
+  ;; https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30931#40
+  (with-temp-buffer
+    (insert "1234567890")
+    (setq buffer-undo-list nil)
+    (narrow-to-region 2 5)
+    ;; `save-restriction' in a narrowed buffer creates two markers
+    ;; representing the current restriction.
+    (save-restriction
+      (widen)
+      ;; Any markers *within* the deleted region are put onto the undo
+      ;; list.
+      (delete-region 1 6))
+    ;; (princ (format "%S" buffer-undo-list) #'external-debugging-output)
+    ;; `buffer-undo-list' is now
+    ;; (("12345" . 1) (#<temp-marker1> . -1) (#<temp-marker2> . 1))
+    ;;
+    ;; If temp-marker1 or temp-marker2 are freed prematurely, calling
+    ;; `type-of' on them will cause Emacs to abort.  Calling
+    ;; `garbage-collect' will also abort if it finds any reachable
+    ;; freed objects.
+    (should (eq (type-of (car (nth 1 buffer-undo-list))) 'marker))
+    (should (eq (type-of (car (nth 2 buffer-undo-list))) 'marker))
+    (garbage-collect)))
+
+(ert-deftest delete-region-undo-markers-2 ()
+  "Make sure we don't end up with freed markers reachable from Lisp."
+  ;; https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30931#55
+  (with-temp-buffer
+    (insert "1234567890")
+    (setq buffer-undo-list nil)
+    ;; signal_before_change creates markers delimiting a change
+    ;; region.
+    (let ((before-change-functions
+           (list (lambda (beg end)
+                   (delete-region (1- beg) (1+ end))))))
+      (delete-region 2 5))
+    ;; (princ (format "%S" buffer-undo-list) #'external-debugging-output)
+    ;; `buffer-undo-list' is now
+    ;; (("678" . 1) ("12345" . 1) (#<marker in no buffer> . -1)
+    ;;  (#<temp-marker1> . -1) (#<temp-marker2> . -4))
+    ;;
+    ;; If temp-marker1 or temp-marker2 are freed prematurely, calling
+    ;; `type-of' on them will cause Emacs to abort.  Calling
+    ;; `garbage-collect' will also abort if it finds any reachable
+    ;; freed objects.
+    (should (eq (type-of (car (nth 3 buffer-undo-list))) 'marker))
+    (should (eq (type-of (car (nth 4 buffer-undo-list))) 'marker))
+    (garbage-collect)))
+
 ;;; editfns-tests.el ends here
-- 
2.11.0