From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: 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 Message-ID: <86ldzl4agb.fsf@gnu.org> References: <20240904.080321.1100373523429404965.enometh@meer.net> <87seug85hh.fsf@web.de> <86y147gzx0.fsf@mail.linkov.net> <874j6uz4d8.fsf@web.de> <86ed5yujq3.fsf@mail.linkov.net> <87v7z9rnrp.fsf@web.de> <87o750dats.fsf@web.de> <868qw2ruv3.fsf@mail.linkov.net> <87bk0w511c.fsf@web.de> <86a5gg7nrs.fsf@mail.linkov.net> <86tteo3wmh.fsf@mail.linkov.net> <86h6aih8i9.fsf@gnu.org> <86zfo9c64h.fsf@gnu.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="15850"; mail-complaints-to="usenet@ciao.gmane.io" Cc: michael_heerdegen@web.de, enometh@meer.net, 73018-done@debbugs.gnu.org, yantar92@posteo.net, juri@linkov.net To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Sep 21 11:34:59 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1srwW2-0003xy-79 for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 21 Sep 2024 11:34:58 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1srwVp-0001Kj-Aa; Sat, 21 Sep 2024 05:34:45 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1srwVn-0001KL-Au for bug-gnu-emacs@gnu.org; Sat, 21 Sep 2024 05:34:43 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1srwVn-00089D-2H for bug-gnu-emacs@gnu.org; Sat, 21 Sep 2024 05:34:43 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=References:In-Reply-To:From:Date:To:Subject; bh=7c6a3GdoBFKMnmDCujndQl/JzAb+OIAQUdogVlPUJ0U=; b=K+nn61g0DMiesVNEdVrOjlLeC/k/DWmzoSldJ1J9eCPdVj3vDLbNIg5Xs/14JcKcEUN4Y986BqQmBF1Zp1OjBKisBxKqyozAJCOjZtkvlUnbmADc4nc+/0ep7UL5Y8DhdPB10m1uJXxXmXc1LIT9ELF7HY4yHnrsqoQZDMskMccV4gq36OgSg5+21cfFJgObmO3L+iKC6vUScBedbxZRVTbeKFRIEDjZcaaYlBeNCp+ieVZcSf3p681ZR6bt02O4MZUCw+Gdjw2JSiRJFXx0EHU6BMfwUiuUiisqSDUfVHaczmcCyHnLmiMqKplFpJVeipotO2CjbGHtgedOa8BZyg==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1srwW6-00032q-RL for bug-gnu-emacs@gnu.org; Sat, 21 Sep 2024 05:35:02 -0400 Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-To: bug-gnu-emacs@gnu.org Resent-Date: Sat, 21 Sep 2024 09:35:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: cc-closed 73018 X-GNU-PR-Package: emacs Mail-Followup-To: 73018@debbugs.gnu.org, eliz@gnu.org, enometh@meer.net Original-Received: via spool by 73018-done@debbugs.gnu.org id=D73018.172691128811670 (code D ref 73018); Sat, 21 Sep 2024 09:35:02 +0000 Original-Received: (at 73018-done) by debbugs.gnu.org; 21 Sep 2024 09:34:48 +0000 Original-Received: from localhost ([127.0.0.1]:37182 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1srwVr-000328-NY for submit@debbugs.gnu.org; Sat, 21 Sep 2024 05:34:48 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:34006) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1srwVp-00031u-7O for 73018-done@debbugs.gnu.org; Sat, 21 Sep 2024 05:34:46 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1srwVM-00088W-6v; Sat, 21 Sep 2024 05:34:16 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=7c6a3GdoBFKMnmDCujndQl/JzAb+OIAQUdogVlPUJ0U=; b=jaYH2KahaTDr /IDVEbzl3NKoO9LPbZvtdO2m/50GO2Wqqj59C8fW6L92FyF0o//B2OddnpDEhWyqp5tvEAwUzJ9YR N6O9VycL8wWcP/wNl58oZTBMyiKCc4k/0pkSxa3brGdlTaj2VpwgyxLErzPopmPWKWHFpRsh36TkS HQsb7qjPQ/McUB4N7Pl21Kxqc0ouZs6TrtLkOJkhlHzXHtszfNJK486tSMYZg1QWR7Mw5kCRfBOzG MLHF5YMbYg6AGMJh0Zt7ROBxPJPq8DpMG84SQZoLkQWRAixpNz4v25ieK6TAKqRVNGU/Ug5Jg4Er3 /YCZHLd+vf3VVluAI5gYKQ==; In-Reply-To: (message from Stefan Monnier on Tue, 17 Sep 2024 14:52:57 -0400) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:292162 Archived-At: > From: Stefan Monnier > Cc: michael_heerdegen@web.de, enometh@meer.net, Ihor Radchenko > , 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 > 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 > 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 > 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.