GNU bug report logs - #23917
25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

Previous Next

Full log

Message #44 received at 23917 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 23917 <at> debbugs.gnu.org, rpluim <at> gmail.com, alex.bennee <at> linaro.org,
 nljlistbox2 <at> gmail.com, jwiegley <at> gmail.com
Subject: Re: Please consider making Bug #23917 a blocker for 25.1 (was Re:
 org-capture: Capture template ‘g’: Match data clobbered
 by buffer modification hooks)
Date: Tue, 19 Jul 2016 00:48:19 -0400

> The more general problem is when there's at least one more
> sub-expression, whose start and/or end are after the new EOB.
> Those sub-expression's data will be completely bogus after the
> adjustment,

If they were after the EOB, they were already bogus to start with.
So there's really not much harm moving them around.  And in any case,
that's what has been happening for ever and has proved safe enough.

>> So I think a safe fix is to try and relax the check we added to
>> replace-match so it doesn't get all worked up when something ≥ EOB gets
>> changed to something else that's also ≥ EOB.
> And lose the other sub-expressions in a more general case?  Really?

I'm not sure what you mean by "losing sub-expressions".  But yes,
I think the behavior of save-match-data you describe is not
a real problem.  Arguably if save-match-data moves positions from
outside BEGV...ZV to inside it it's a problem.  But if it moves them
from outside BEG...Z to inside it, I think it's perfectly fine.

>> Or maybe instead of signaling an error, we could simply skip the "Adjust
>> search data for this change".
> That would still sweep the problem under the carpet, leaving the match
> data bogus, so I don't like doing that.

Maybe I'm not 100% satisfied with the behavior either, but I don't think
it's a significant problem and I don't think it'd cause the crash we saw
in bug#23869.

> The crash in bug#23869 was due to this:
>
>   newpoint = search_regs.start[sub] + SCHARS (newtext);
>   [...]
>   /* Now move point "officially" to the start of the inserted replacement.  */
>   move_if_not_intangible (newpoint);  <<<<<<<<<<<<<<<<<<<<<<<
>
> because due to clobbering, newpoint became -1.

Ah, I see.

Then maybe another fix is to compute newpoint before we call
replace_range, so it uses search_regs.start[sub] before the
*-change-functions can mess it up.  IOW:

    @@ -2726,9 +2726,9 @@ since only regular expressions have distinguished subexpressions.  */)
       unsigned  num_regs = search_regs.num_regs;
     
       /* Replace the old text with the new in the cleanest possible way.  */
    +  newpoint = search_regs.start[sub] + SCHARS (newtext);
       replace_range (search_regs.start[sub], search_regs.end[sub],
     		 newtext, 1, 0, 1);
    -  newpoint = search_regs.start[sub] + SCHARS (newtext);
     
       if (case_action == all_caps)
         Fupcase_region (make_number (search_regs.start[sub]),

Would that be sufficient to avoid the crash?  Why not?


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.