GNU bug report logs -
#73018
31.0.50; wdired + replace-regexp only modifies the visible portion of the buffer
Previous Next
Reported by: Madhu <enometh <at> meer.net>
Date: Wed, 4 Sep 2024 02:38:01 UTC
Severity: normal
Found in version 31.0.50
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
Full log
View this message in rfc822 format
>> > 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).
Stefan
This bug report was last modified 296 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.