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

From: Stefan Monnier <[email-masked]>
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 <[email-masked]>
    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 <[email-masked]>
    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 <[email-masked]>
    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).

        Stefan