GNU bug report logs - #35264
"Match data clobbered by buffer modification hooks" when hooks only shifted match-data's markers

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 35264 in the body.
You can then email your comments to 35264 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: "Match data clobbered by buffer modification hooks" when hooks only
 shifted match-data's markers
Date: Sun, 14 Apr 2019 00:40:13 -0400

[Message part 1 (text/plain, inline)]

Version: 27.0.50 26.2 25.3
X-Debbugs-CC: Stefan Monnier <monnier <at> 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:

[bug-xxxx-match-data-marker-clobber.el (text/plain, inline)]

(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 <at> 11> #<marker <at> 17>)
                    ))))))
      (search-backward "abcdef")
      ;; (match-data) ;=> (#<marker <at> 12> #<marker <at> 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)

Message #8 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: 35264 <at> debbugs.gnu.org
Cc: stefan monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Thu, 30 May 2019 19:03:54 -0400

[Message part 1 (text/plain, inline)]

> 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.

[0001-Remove-unreliable-test-for-match-data-clobbering-Bug.patch (text/x-diff, inline)]

From 6fd6605c63ecc031a3fd6ba8b8e2e754c183b3f2 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> 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

Message #13 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: stefan monnier <monnier <at> iro.umontreal.ca>, 35264 <at> debbugs.gnu.org
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Fri, 02 Oct 2020 06:47:55 +0200

Noam Postavsky <npostavs <at> 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

Message #16 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: stefan monnier <monnier <at> iro.umontreal.ca>, 35264 <at> debbugs.gnu.org
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Wed, 12 May 2021 16:35:38 +0200

Lars Ingebrigtsen <larsi <at> 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

Message #19 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 35264 <at> debbugs.gnu.org, npostavs <at> gmail.com, monnier <at> iro.umontreal.ca
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Wed, 12 May 2021 17:55:58 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Wed, 12 May 2021 16:35:38 +0200
> Cc: stefan monnier <monnier <at> iro.umontreal.ca>, 35264 <at> debbugs.gnu.org
> 
> Lars Ingebrigtsen <larsi <at> 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?

Message #22 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35264 <at> debbugs.gnu.org, npostavs <at> gmail.com, monnier <at> iro.umontreal.ca
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Wed, 12 May 2021 17:43:33 +0200

Eli Zaretskii <eliz <at> 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

Message #25 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 35264 <at> debbugs.gnu.org, npostavs <at> gmail.com, monnier <at> iro.umontreal.ca
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Wed, 12 May 2021 18:48:54 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: npostavs <at> gmail.com,  monnier <at> iro.umontreal.ca,  35264 <at> debbugs.gnu.org
> Date: Wed, 12 May 2021 17:43:33 +0200
> 
> Eli Zaretskii <eliz <at> 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.

Message #28 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35264 <at> debbugs.gnu.org, npostavs <at> gmail.com, monnier <at> iro.umontreal.ca
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Wed, 12 May 2021 17:57:14 +0200

Eli Zaretskii <eliz <at> 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

Message #31 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 35264 <at> debbugs.gnu.org, npostavs <at> gmail.com, monnier <at> iro.umontreal.ca
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Wed, 12 May 2021 19:03:04 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: npostavs <at> gmail.com,  monnier <at> iro.umontreal.ca,  35264 <at> debbugs.gnu.org
> Date: Wed, 12 May 2021 17:57:14 +0200
> 
> Eli Zaretskii <eliz <at> 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.

Message #34 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: npostavs <at> gmail.com, monnier <at> iro.umontreal.ca, 35264 <at> debbugs.gnu.org
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Thu, 13 May 2021 11:11:35 +0200

Eli Zaretskii <eliz <at> 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

Message #37 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: npostavs <at> gmail.com, monnier <at> iro.umontreal.ca, 35264 <at> debbugs.gnu.org
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Thu, 13 May 2021 13:14:19 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: 35264 <at> debbugs.gnu.org,  npostavs <at> gmail.com,  monnier <at> iro.umontreal.ca
> Date: Thu, 13 May 2021 11:11:35 +0200
> 
> Eli Zaretskii <eliz <at> 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.

Message #40 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, npostavs <at> gmail.com,
 35264 <at> debbugs.gnu.org
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
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.


        Stefan

Message #43 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: larsi <at> gnus.org, npostavs <at> gmail.com, 35264 <at> debbugs.gnu.org
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Thu, 13 May 2021 20:09:20 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi <at> gnus.org>,  35264 <at> debbugs.gnu.org,
>   npostavs <at> 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.

Message #46 received at 35264 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: npostavs <at> gmail.com, monnier <at> iro.umontreal.ca, 35264 <at> debbugs.gnu.org
Subject: Re: bug#35264: "Match data clobbered by buffer modification hooks"
 when hooks only shifted match-data's markers
Date: Sun, 16 May 2021 15:20:32 +0200

Eli Zaretskii <eliz <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.