From 96b8747d5c5d747af13fd84d8fe0308ef2a0ea7a Mon Sep 17 00:00:00 2001 From: Noam Postavsky 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 369592d70ee..9fdcc7306a8 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 727f2d0080c..84de6792738 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 02e3f41bc9f..697395c507b 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 b931d23bf38..f471b216587 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -3812,7 +3812,6 @@ extern Lisp_Object make_save_funcptr_ptr_obj (void (*) (void), void *, 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 442ad089375..2e20c9dd126 100644 --- a/test/src/editfns-tests.el +++ b/test/src/editfns-tests.el @@ -288,4 +288,55 @@ (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) (# . -1) (# . 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) (# . -1) + ;; (# . -1) (# . -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.4.GIT