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

[Eli Zaretskii <eliz@gnu.org>]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: michael_heerdegen@web.de,  enometh@meer.net,  Ihor Radchenko
>  <yantar92@posteo.net>,  73018@debbugs.gnu.org,  juri@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@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@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@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.