* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
@ 2019-04-14 4:40 Noam Postavsky
2019-05-30 23:03 ` Noam Postavsky
0 siblings, 1 reply; 14+ messages in thread
From: Noam Postavsky @ 2019-04-14 4:40 UTC (permalink / raw)
To: 35264; +Cc: stefan monnier
[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]
Version: 27.0.50 26.2 25.3
X-Debbugs-CC: Stefan Monnier <monnier@iro.umontreal.ca>
emacs -Q -l bug-xxxx-match-data-marker-clobber.el
Hit <f12>, gives
Debugger entered--Lisp error: (error "Match data clobbered by buffer modification hooks")
replace-match("ABCDEF" t t)
(let* ((after-change-functions (list (function (lambda (&rest _) (let (... ...) (save-excursion ...))))))) (search-backward "abcdef") (replace-match "ABCDEF" t t))
(save-current-buffer (set-buffer (get-buffer-create "*test*")) (display-buffer (current-buffer)) (erase-buffer) (insert "1234567890\n") (insert "abcdefghilk\n") (make-local-variable (quote after-change-functions)) (let* ((after-change-functions (list (function (lambda (&rest _) (let ... ...)))))) (search-backward "abcdef") (replace-match "ABCDEF" t t)))
bug-match-data-marker-clobber()
funcall-interactively(bug-match-data-marker-clobber)
call-interactively(bug-match-data-marker-clobber nil nil)
command-execute(bug-match-data-marker-clobber)
But the modification hook in question did call save-match-data. As far
as I can tell, the problem is that the match-data consists of markers,
whose position gets shifted by deletion of characters. The check for
this error uses simple integers, so there's no way it can account for
this.
I think this is a variant of Bug#23917, there was some talk there about
removing the check, perhaps that is the right solution.
DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
[...]
/* The functions below modify the buffer, so they could trigger
various modification hooks (see signal_before_change and
signal_after_change). If these hooks clobber the match data we
error out since otherwise this will result in confusing bugs. */
ptrdiff_t sub_start = search_regs.start[sub];
ptrdiff_t sub_end = search_regs.end[sub];
unsigned num_regs = search_regs.num_regs;
newpoint = search_regs.start[sub] + SCHARS (newtext);
/* Replace the old text with the new in the cleanest possible way. */
replace_range (search_regs.start[sub], search_regs.end[sub],
newtext, 1, 0, 1, 1);
[...]
if (search_regs.start[sub] != sub_start
|| search_regs.end[sub] != sub_end
|| search_regs.num_regs != num_regs)
error ("Match data clobbered by buffer modification hooks");
bug-xxxx-match-data-marker-clobber.el:
[-- Attachment #2: bug reproducer --]
[-- Type: text/plain, Size: 1183 bytes --]
(defun bug-match-data-marker-clobber ()
(interactive)
(with-current-buffer (get-buffer-create "*test*")
(display-buffer (current-buffer))
(erase-buffer)
(insert "1234567890\n")
(insert "abcdefghilk\n")
(make-local-variable 'after-change-functions)
(let* ((after-change-functions `
(,(lambda (&rest _)
(let ((inhibit-modification-hooks nil)
(after-change-functions nil))
(save-excursion
(save-match-data
(goto-char (point-min))
(looking-at "[0-9]")
(delete-char 1))
;; match-data is restored, but markers have a
;; different position now, because of the
;; deletion.
;;
;; (match-data) ;=> (#<marker@11> #<marker@17>)
))))))
(search-backward "abcdef")
;; (match-data) ;=> (#<marker@12> #<marker@18>)
(replace-match "ABCDEF" t t) ;; Triggers `after-change-functions'.
)))
(setq debug-on-error t)
(define-key global-map [f12] 'bug-match-data-marker-clobber)
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2019-04-14 4:40 bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers Noam Postavsky
@ 2019-05-30 23:03 ` Noam Postavsky
2020-10-02 4:47 ` Lars Ingebrigtsen
0 siblings, 1 reply; 14+ messages in thread
From: Noam Postavsky @ 2019-05-30 23:03 UTC (permalink / raw)
To: 35264; +Cc: stefan monnier
[-- Attachment #1: Type: text/plain, Size: 154 bytes --]
> I think this is a variant of Bug#23917, there was some talk there about
> removing the check, perhaps that is the right solution.
So, this, I guess.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2381 bytes --]
From 6fd6605c63ecc031a3fd6ba8b8e2e754c183b3f2 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 30 May 2019 19:00:31 -0400
Subject: [PATCH] Remove unreliable test for match data clobbering (Bug#35264)
* src/search.c (Freplace_match): Don't test for change in search_regs
start and end, this is unreliable if change hooks modify text earlier
in the buffer.
---
src/search.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/src/search.c b/src/search.c
index db7fecd9ba..1d4550849e 100644
--- a/src/search.c
+++ b/src/search.c
@@ -2730,22 +2730,16 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
/* The functions below modify the buffer, so they could trigger
various modification hooks (see signal_before_change and
signal_after_change). If these hooks clobber the match data we
- error out since otherwise this will result in confusing bugs. */
- ptrdiff_t sub_start = search_regs.start[sub];
- ptrdiff_t sub_end = search_regs.end[sub];
+ error out since otherwise this will result in confusing bugs. We
+ used to check for changes in search_regs start and end, but that
+ fails if modification hooks remove or add text earlier in the
+ buffer, so just check num_regs now. */
unsigned num_regs = search_regs.num_regs;
newpoint = search_regs.start[sub] + SCHARS (newtext);
/* Replace the old text with the new in the cleanest possible way. */
replace_range (search_regs.start[sub], search_regs.end[sub],
newtext, 1, 0, 1, 1);
- /* Update saved data to match adjustment made by replace_range. */
- {
- ptrdiff_t change = newpoint - sub_end;
- if (sub_start >= sub_end)
- sub_start += change;
- sub_end += change;
- }
if (case_action == all_caps)
Fupcase_region (make_number (search_regs.start[sub]),
@@ -2755,9 +2749,7 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
Fupcase_initials_region (make_number (search_regs.start[sub]),
make_number (newpoint));
- if (search_regs.start[sub] != sub_start
- || search_regs.end[sub] != sub_end
- || search_regs.num_regs != num_regs)
+ if (search_regs.num_regs != num_regs)
error ("Match data clobbered by buffer modification hooks");
/* Put point back where it was in the text. */
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2019-05-30 23:03 ` Noam Postavsky
@ 2020-10-02 4:47 ` Lars Ingebrigtsen
2021-05-12 14:35 ` Lars Ingebrigtsen
0 siblings, 1 reply; 14+ messages in thread
From: Lars Ingebrigtsen @ 2020-10-02 4:47 UTC (permalink / raw)
To: Noam Postavsky; +Cc: stefan monnier, 35264
Noam Postavsky <npostavs@gmail.com> writes:
>> I think this is a variant of Bug#23917, there was some talk there about
>> removing the check, perhaps that is the right solution.
>
> So, this, I guess.
[...]
> * src/search.c (Freplace_match): Don't test for change in search_regs
> start and end, this is unreliable if change hooks modify text earlier
> in the buffer.
[...]
> - if (search_regs.start[sub] != sub_start
> - || search_regs.end[sub] != sub_end
> - || search_regs.num_regs != num_regs)
> + if (search_regs.num_regs != num_regs)
> error ("Match data clobbered by buffer modification hooks");
>
> /* Put point back where it was in the text. */
There were unfortunately no comments on this patch at the time. Noam said:
> But the modification hook in question did call save-match-data. As far
> as I can tell, the problem is that the match-data consists of markers,
> whose position gets shifted by deletion of characters. The check for
> this error uses simple integers, so there's no way it can account for
> this.
That does make sense, but removing this (somewhat buggy) sanity check is
perhaps a bit scary. Any comments on this?
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2020-10-02 4:47 ` Lars Ingebrigtsen
@ 2021-05-12 14:35 ` Lars Ingebrigtsen
2021-05-12 14:55 ` Eli Zaretskii
0 siblings, 1 reply; 14+ messages in thread
From: Lars Ingebrigtsen @ 2021-05-12 14:35 UTC (permalink / raw)
To: Noam Postavsky; +Cc: stefan monnier, 35264
Lars Ingebrigtsen <larsi@gnus.org> writes:
>> But the modification hook in question did call save-match-data. As far
>> as I can tell, the problem is that the match-data consists of markers,
>> whose position gets shifted by deletion of characters. The check for
>> this error uses simple integers, so there's no way it can account for
>> this.
>
> That does make sense, but removing this (somewhat buggy) sanity check is
> perhaps a bit scary. Any comments on this?
There were no comments, and the patch no longer applies. I've respun it
now for Emacs 28, and this fixes the case reported in this bug report
(which still reproduces in Emacs 28).
Any comments?
diff --git a/src/search.c b/src/search.c
index c757bf3d1f..df384e1dcf 100644
--- a/src/search.c
+++ b/src/search.c
@@ -2723,7 +2723,6 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
}
newpoint = sub_start + SCHARS (newtext);
- ptrdiff_t newstart = sub_start == sub_end ? newpoint : sub_start;
/* Replace the old text with the new in the cleanest possible way. */
replace_range (sub_start, sub_end, newtext, 1, 0, 1, true);
@@ -2739,11 +2738,11 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
/* The replace_range etc. functions can trigger modification hooks
(see signal_before_change and signal_after_change). Try to error
out if these hooks clobber the match data since clobbering can
- result in confusing bugs. Although this sanity check does not
- catch all possible clobberings, it should catch many of them. */
- if (! (search_regs.num_regs == num_regs
- && search_regs.start[sub] == newstart
- && search_regs.end[sub] == newpoint))
+ result in confusing bugs. We used to check for changes in
+ search_regs start and end, but that fails if modification hooks
+ remove or add text earlier in the buffer, so just check num_regs
+ now. */
+ if (search_regs.num_regs != num_regs)
error ("Match data clobbered by buffer modification hooks");
/* Put point back where it was in the text, if possible. */
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-12 14:35 ` Lars Ingebrigtsen
@ 2021-05-12 14:55 ` Eli Zaretskii
2021-05-12 15:43 ` Lars Ingebrigtsen
0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2021-05-12 14:55 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: npostavs, monnier, 35264
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 12 May 2021 16:35:38 +0200
> Cc: stefan monnier <monnier@iro.umontreal.ca>, 35264@debbugs.gnu.org
>
> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
> >> But the modification hook in question did call save-match-data. As far
> >> as I can tell, the problem is that the match-data consists of markers,
> >> whose position gets shifted by deletion of characters. The check for
> >> this error uses simple integers, so there's no way it can account for
> >> this.
> >
> > That does make sense, but removing this (somewhat buggy) sanity check is
> > perhaps a bit scary. Any comments on this?
>
> There were no comments, and the patch no longer applies. I've respun it
> now for Emacs 28, and this fixes the case reported in this bug report
> (which still reproduces in Emacs 28).
>
> Any comments?
What about bug#23869, which triggered the addition of that code?
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-12 14:55 ` Eli Zaretskii
@ 2021-05-12 15:43 ` Lars Ingebrigtsen
2021-05-12 15:48 ` Eli Zaretskii
0 siblings, 1 reply; 14+ messages in thread
From: Lars Ingebrigtsen @ 2021-05-12 15:43 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: npostavs, monnier, 35264
Eli Zaretskii <eliz@gnu.org> writes:
> What about bug#23869, which triggered the addition of that code?
I tried the code example in that bug report now (with the patch that
removes the sanity check), and it did not crash Emacs. So that sanity
check no longer seems to be necessary?
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-12 15:43 ` Lars Ingebrigtsen
@ 2021-05-12 15:48 ` Eli Zaretskii
2021-05-12 15:57 ` Lars Ingebrigtsen
0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2021-05-12 15:48 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: npostavs, monnier, 35264
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: npostavs@gmail.com, monnier@iro.umontreal.ca, 35264@debbugs.gnu.org
> Date: Wed, 12 May 2021 17:43:33 +0200
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> > What about bug#23869, which triggered the addition of that code?
>
> I tried the code example in that bug report now (with the patch that
> removes the sanity check), and it did not crash Emacs. So that sanity
> check no longer seems to be necessary?
Probably because some code somewhere uses save-match-data.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-12 15:48 ` Eli Zaretskii
@ 2021-05-12 15:57 ` Lars Ingebrigtsen
2021-05-12 16:03 ` Eli Zaretskii
0 siblings, 1 reply; 14+ messages in thread
From: Lars Ingebrigtsen @ 2021-05-12 15:57 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: npostavs, monnier, 35264
Eli Zaretskii <eliz@gnu.org> writes:
>> I tried the code example in that bug report now (with the patch that
>> removes the sanity check), and it did not crash Emacs. So that sanity
>> check no longer seems to be necessary?
>
> Probably because some code somewhere uses save-match-data.
I guess. But we have a case here where the sanity check is definitely
wrong, and we don't have a reproducing case (any more) for the problem
the check is trying to fix... which seems to indicate to me that we
should apply the patch (i.e., remove the buggy sanity check).
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-12 15:57 ` Lars Ingebrigtsen
@ 2021-05-12 16:03 ` Eli Zaretskii
2021-05-13 9:11 ` Lars Ingebrigtsen
2021-05-16 13:20 ` Lars Ingebrigtsen
0 siblings, 2 replies; 14+ messages in thread
From: Eli Zaretskii @ 2021-05-12 16:03 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: npostavs, monnier, 35264
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: npostavs@gmail.com, monnier@iro.umontreal.ca, 35264@debbugs.gnu.org
> Date: Wed, 12 May 2021 17:57:14 +0200
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> I tried the code example in that bug report now (with the patch that
> >> removes the sanity check), and it did not crash Emacs. So that sanity
> >> check no longer seems to be necessary?
> >
> > Probably because some code somewhere uses save-match-data.
>
> I guess. But we have a case here where the sanity check is definitely
> wrong, and we don't have a reproducing case (any more) for the problem
> the check is trying to fix... which seems to indicate to me that we
> should apply the patch (i.e., remove the buggy sanity check).
I'm not against applying the change, I just wonder why Stefan wasn't
against it back then as he is now.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-12 16:03 ` Eli Zaretskii
@ 2021-05-13 9:11 ` Lars Ingebrigtsen
2021-05-13 10:14 ` Eli Zaretskii
2021-05-16 13:20 ` Lars Ingebrigtsen
1 sibling, 1 reply; 14+ messages in thread
From: Lars Ingebrigtsen @ 2021-05-13 9:11 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 35264, npostavs, monnier
Eli Zaretskii <eliz@gnu.org> writes:
> I'm not against applying the change, I just wonder why Stefan wasn't
> against it back then as he is now.
Hm. Is he against it now?
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-13 9:11 ` Lars Ingebrigtsen
@ 2021-05-13 10:14 ` Eli Zaretskii
2021-05-13 16:03 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2021-05-13 10:14 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 35264, npostavs, monnier
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 35264@debbugs.gnu.org, npostavs@gmail.com, monnier@iro.umontreal.ca
> Date: Thu, 13 May 2021 11:11:35 +0200
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> > I'm not against applying the change, I just wonder why Stefan wasn't
> > against it back then as he is now.
>
> Hm. Is he against it now?
Sorry for sloppy wording: Stefan isn't against applying the change
_you_ want to apply now. What I alluded to is that he seems to think
the change I installed back then was problematic. But he didn't think
so back then.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-13 10:14 ` Eli Zaretskii
@ 2021-05-13 16:03 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-05-13 17:09 ` Eli Zaretskii
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-05-13 16:03 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Lars Ingebrigtsen, npostavs, 35264
>> > I'm not against applying the change, I just wonder why Stefan wasn't
>> > against it back then as he is now.
>> Hm. Is he against it now?
> Sorry for sloppy wording: Stefan isn't against applying the change
> _you_ want to apply now. What I alluded to is that he seems to think
> the change I installed back then was problematic. But he didn't think
> so back then.
Am I the "Stefan" you're referring to?
I can't remember what you're referring to, and I can't find any
intervention of mine in that bug report.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-13 16:03 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-05-13 17:09 ` Eli Zaretskii
0 siblings, 0 replies; 14+ messages in thread
From: Eli Zaretskii @ 2021-05-13 17:09 UTC (permalink / raw)
To: Stefan Monnier; +Cc: larsi, npostavs, 35264
> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 35264@debbugs.gnu.org,
> npostavs@gmail.com
> Date: Thu, 13 May 2021 12:03:48 -0400
>
> >> > I'm not against applying the change, I just wonder why Stefan wasn't
> >> > against it back then as he is now.
> >> Hm. Is he against it now?
> > Sorry for sloppy wording: Stefan isn't against applying the change
> > _you_ want to apply now. What I alluded to is that he seems to think
> > the change I installed back then was problematic. But he didn't think
> > so back then.
>
> Am I the "Stefan" you're referring to?
> I can't remember what you're referring to, and I can't find any
> intervention of mine in that bug report.
Forget it, it isn't worth it to waste so many messages on a minor
remark.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers
2021-05-12 16:03 ` Eli Zaretskii
2021-05-13 9:11 ` Lars Ingebrigtsen
@ 2021-05-16 13:20 ` Lars Ingebrigtsen
1 sibling, 0 replies; 14+ messages in thread
From: Lars Ingebrigtsen @ 2021-05-16 13:20 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 35264, npostavs, monnier
Eli Zaretskii <eliz@gnu.org> writes:
> I'm not against applying the change, I just wonder why Stefan wasn't
> against it back then as he is now.
OK, I've now applied Noam's patch to Emacs 28.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-05-16 13:20 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-14 4:40 bug#35264: "Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers Noam Postavsky
2019-05-30 23:03 ` Noam Postavsky
2020-10-02 4:47 ` Lars Ingebrigtsen
2021-05-12 14:35 ` Lars Ingebrigtsen
2021-05-12 14:55 ` Eli Zaretskii
2021-05-12 15:43 ` Lars Ingebrigtsen
2021-05-12 15:48 ` Eli Zaretskii
2021-05-12 15:57 ` Lars Ingebrigtsen
2021-05-12 16:03 ` Eli Zaretskii
2021-05-13 9:11 ` Lars Ingebrigtsen
2021-05-13 10:14 ` Eli Zaretskii
2021-05-13 16:03 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-05-13 17:09 ` Eli Zaretskii
2021-05-16 13:20 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).