GNU bug report logs - #73018
31.0.50; wdired + replace-regexp only modifies the visible portion of the buffer

Previous Next

Full log

Message #82 received at 73018-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: michael_heerdegen <at> web.de, enometh <at> meer.net, 73018-done <at> debbugs.gnu.org,
 yantar92 <at> posteo.net, juri <at> linkov.net
Subject: Re: bug#73018: 31.0.50; wdired + replace-regexp only modifies the
 visible portion of the buffer
Date: Sat, 21 Sep 2024 12:34:12 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: michael_heerdegen <at> web.de,  enometh <at> meer.net,  Ihor Radchenko
>  <yantar92 <at> posteo.net>,  73018 <at> debbugs.gnu.org,  juri <at> linkov.net
> Date: Tue, 17 Sep 2024 14:52:57 -0400
> 
> >> > Since this is a regression in Emacs 30, I'd like to solve it on the
> >> > release branch.  Can you suggest the safest fix you can come up with
> >> > for that purpose?
> >> 
> >> Oh, yes: just remove the check.
> >
> > Whoa!  We had that check there for 9 years, and it was introduced to
> > avoid crashes (see bug#23869), so removing it now, during a pretest,
> > is scary.
> 
> Here's the story I see:
> In response to that bug, you proposed to add:
> 
>     /* Last line of defense, in case search registers were actually not
>        saved (because someone else already occupied the save slots).  */
>     if (search_regs.start[sub] != sub_start
>         || search_regs.end[sub] != sub_end)
>       error ("Match data clobbered by buffer modification hooks");
> 
> In the end, you added:
> 
>     commit 3a9d6296b35e5317c497674d5725eb52699bd3b8
>     Author: Eli Zaretskii <eliz <at> gnu.org>
>     Date:   Mon Jul 4 18:34:40 2016 +0300
>     
>         Avoid crashes when buffer modification hooks clobber match data
>         
>         * src/search.c (Freplace_match): Error out if buffer modification
>         hooks triggered by buffer changes in replace_range, upcase-region,
>         and upcase-initials-region clobber the match data needed to be
>         adjusted for the replacement.  (Bug#23869)
>     
>     diff --git a/src/search.c b/src/search.c
>     --- a/src/search.c
>     +++ b/src/search.c
>     @@ -2699,0 +2707,5 @@
>     +  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");
> 
> A bit later we dropped the start/end part (for a reason I'm not sure is
> valid, since change hooks that modify the buffer should be disallowed,
> I think):
> 
>     commit 487498e497f8c6b6303bd5feeac83a5bcc2315af
>     Author: Noam Postavsky <npostavs <at> gmail.com>
>     Date:   Sun May 16 15:19:57 2021 +0200
>     
>         Remove unreliable test for match data clobbering
>         
>         * 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 (bug#35264).
>     
>     diff --git a/src/search.c b/src/search.c
>     --- a/src/search.c
>     +++ b/src/search.c
>     @@ -2739,10 +2738,10 @@
>        /* 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");
> 
> So the check that remains is one that wasn't even present originally.
> 
> Also, IIUC the origin of the crash in bug#23869 is that we did:
> 
>        /* Adjust search data for this change.  */
>        {
>          ptrdiff_t oldend = search_regs.end[sub];
> 
> after running the change functions (i.e. at a time where
> `search_regs.end[sub]` might not hold the same match data and hence
> might be -1, leading to the crash).
> 
> This code is different now.  The only place where we use something like
> `search_regs.end[sub]` once it's possibly-clobbered is:
> 
>       if (case_action == all_caps)
>         Fupcase_region (make_fixnum (search_regs.start[sub]),
>     		    make_fixnum (newpoint),
>     		    Qnil);
>       else if (case_action == cap_initial)
>         Fupcase_initials_region (make_fixnum (search_regs.start[sub]),
>     			     make_fixnum (newpoint), Qnil);
> 
> both of whose functions should not crash just because they're called
> with a -1.  So I think the original crash should not happen nowadays,
> and this is because the "Adjust search data" part of the code was
> completely rewritten by:
> 
>     commit 66f95e0dabf750e9d2eff59b2bb6e593618cd48a
>     Author: Noam Postavsky <npostavs <at> gmail.com>
>     Date:   Wed Jul 20 20:15:14 2016 -0400
>     
>         Adjust match data before calling after-change-funs
>         
>         It's important to adjust the match data in between calling
>         before-change-functions and after-change-functions, so that buffer
>         change hooks will always see match-data consistent with buffer content.
>         (Bug #23917)
>         
>         * src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
>         true call update_search_regs.  Update all callers (except
>         Freplace_match) to pass 0 for the new parameter.
>         * src/search.c (update_search_regs): New function, extracted from
>         Freplace_match.
>         (Freplace_match): Remove match data adjustment code, pass 1 for
>         ADJUST_MATCH_DATA to replace_range instead.
> 
> > And I don't think I understand how a single line you moved in
> > 63588775fcb could cause this check to signal an error in the scenario
> > of this bug.  Can you explain?
> 
> The line-move caused the modification hooks to be run at a different
> moment: we used to run them *after* the if+error check whereas now we
> run them before.  The problem can probably be triggered in the old code
> as well if `case_action` is given a different value (in which case the
> `Fupcase_region` may also run the hooks, thus potentially causing the
> same change to the size of the `search_regs.start/end` arrays before the
> if+error check).

Thanks for the analysis.  Call me a coward, but I don't want to make
this change on the release branch.  Instead, I reverted the search.c
part of the 63588775fcb commit there (which will re-introduce
bug#65451, sigh).  I did install the change you suggested on master.

And with that, I'm closing this bug.

Previous Next

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