From: Noam Postavsky <npostavs@gmail.com>
To: Paul Eggert <eggert@cs.ucla.edu>
Cc: 30931@debbugs.gnu.org, "Michał Kondraciuk" <k.michal@zoho.com>
Subject: bug#30931: abort() due to CHECK_ALLOCATED_AND_LIVE failure during GC
Date: Fri, 30 Mar 2018 16:50:53 -0400 [thread overview]
Message-ID: <87tvsxp9si.fsf@gmail.com> (raw)
In-Reply-To: <d8cca0e5-687f-5aea-3a33-5ed1a3b6d1dc@cs.ucla.edu> (Paul Eggert's message of "Fri, 30 Mar 2018 09:23:03 -0700")
[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]
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.
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 6305 bytes --]
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);
-}
-
\f
/* 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
next prev parent reply other threads:[~2018-03-30 20:50 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-24 20:30 bug#30931: 27.0.50; Crash in "Automatic GC" Michał Kondraciuk
2018-03-25 2:33 ` Eli Zaretskii
2018-03-28 21:02 ` Michał Kondraciuk
2018-03-29 23:47 ` Noam Postavsky
2018-03-30 5:39 ` Noam Postavsky
2018-03-30 8:16 ` Eli Zaretskii
2018-03-29 15:52 ` Michał Kondraciuk
2018-03-30 6:19 ` bug#30931: abort() due to CHECK_ALLOCATED_AND_LIVE failure during GC Paul Eggert
2018-03-30 12:56 ` Noam Postavsky
2018-03-30 16:23 ` Paul Eggert
2018-03-30 20:50 ` Noam Postavsky [this message]
2018-03-30 21:29 ` Paul Eggert
2018-03-31 7:42 ` Eli Zaretskii
2018-04-19 23:42 ` bug#30931: 27.0.50; Crash in "Automatic GC" Noam Postavsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87tvsxp9si.fsf@gmail.com \
--to=npostavs@gmail.com \
--cc=30931@debbugs.gnu.org \
--cc=eggert@cs.ucla.edu \
--cc=k.michal@zoho.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.