From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#73018: 31.0.50; wdired + replace-regexp only modifies the visible portion of the buffer Date: Tue, 17 Sep 2024 14:52:57 -0400 Message-ID: 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> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="11821"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: michael_heerdegen@web.de, enometh@meer.net, 73018@debbugs.gnu.org, Ihor Radchenko , juri@linkov.net To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Sep 17 20:54:19 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 1sqdL8-0002xn-2l for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 17 Sep 2024 20:54:18 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sqdKp-0007NT-1y; Tue, 17 Sep 2024 14:53:59 -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 1sqdKm-0007Mw-PQ for bug-gnu-emacs@gnu.org; Tue, 17 Sep 2024 14:53:56 -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 1sqdKd-0007Sd-Pr for bug-gnu-emacs@gnu.org; Tue, 17 Sep 2024 14:53:56 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=MIME-Version:Date:References:In-Reply-To:From:To:Subject; bh=CDFlKEEZqr6xT26He2oBabMFnKPmpxEoTvUm1677N3w=; b=rirIB+n3z5HkyKjUV5m4T6k2vmkTQmJRzFtIE3hSHnAdTHbN5tPv2rohWE6xn2hY1T3eUVfrq6DrLfatQdZVH+JhILzefa6KXxcqm1UM4635vJdX4RP4TPpwGI9DUM8L3MTqSKHFL1J3oYNfoYVL82GyuhldsxZ7NWecaUG+cPU1ouT5A3F0WL0Yawl6XkaazKRFJRBdjTBmUYK3vYKSOsYqsejU6vPTfkyVdiFKleMd02mto1UYSHSHIrx44wW1ClOwio75i9flwJqq2DotvlJK2xNNHXE6qshDM9px1XhrU4RtMWRV4vrGtrnHRGNCswBIeSk7AppRtamZD15BQw==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1sqdKs-0006Hn-A0 for bug-gnu-emacs@gnu.org; Tue, 17 Sep 2024 14:54:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 17 Sep 2024 18:54:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 73018 X-GNU-PR-Package: emacs Original-Received: via spool by 73018-submit@debbugs.gnu.org id=B73018.172659920824109 (code B ref 73018); Tue, 17 Sep 2024 18:54:02 +0000 Original-Received: (at 73018) by debbugs.gnu.org; 17 Sep 2024 18:53:28 +0000 Original-Received: from localhost ([127.0.0.1]:55953 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sqdKJ-0006Gl-GP for submit@debbugs.gnu.org; Tue, 17 Sep 2024 14:53:28 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:28389) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1sqdKH-0006GW-7b for 73018@debbugs.gnu.org; Tue, 17 Sep 2024 14:53:25 -0400 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 435594412FA; Tue, 17 Sep 2024 14:53:04 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1726599177; bh=LN82O9i7UJGJvZ+pZ1HxL/sxebqYvpYAtKj+8LY5Z+8=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=Oez37LanhguvjvJCnhR6YaSuZ9GJU0WhOKLkPZQXXKN7mArfyk3qObvCbNtSHSo33 Lr1/neoji8TnszFv2hBGXhJzSphXWPkPQyAjHDL24HVzLtqWS1gLdSlovybOIh9Khs VJ5PpnQZn9XShfEeZs7gBKcko32EJHFIf6jQs4tV3w+04VOWsXSAybINIXCxamncfn +uZ+Blh65dKuO/bY3WoyWP7GW7gc1LN5hPXSNiEyWe94kMOSBjSIZolXdAT3ktoX/d rF9Uf4rx9ekAXzPm7YWqw3PS3xxM1yLoKKud3WvRNrWRsHZTj+s3u5XyUFlaZWRc9w b8dZij9N0/hDw== Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id BC5434412F0; Tue, 17 Sep 2024 14:52:57 -0400 (EDT) Original-Received: from lechazo (lechon.iro.umontreal.ca [132.204.27.242]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id A2B801204E2; Tue, 17 Sep 2024 14:52:57 -0400 (EDT) In-Reply-To: <86zfo9c64h.fsf@gnu.org> (Eli Zaretskii's message of "Sun, 15 Sep 2024 18:02:22 +0300") 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:291958 Archived-At: >> > 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). Stefan