bug#23869: 25.0.95; replace-match can crash emacs

bug#23869: 25.0.95; replace-match can crash emacs

[Leo Liu]

[-- Attachment #1: Type: text/plain, Size: 279 bytes --]


Under some circumstances replace-match can crash emacs. It takes a lot
to reproduce the crash and I haven't been able to find a minimal
example. But I wonder if the backtrace that I captured in LLDB can give
any clue to the clause? I can't reproduce the same crash in emacs24.


[-- Attachment #2: emacs-crash.txt --]
[-- Type: text/plain, Size: 4796 bytes --]

Process 26639 resuming
Process 26639 stopped
* thread #1: tid = 0x16771fc, 0x0000000100280a5e emacs`find_interval(tree=0x0000000000000000, position=-1) + 142 at intervals.c:678, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
    frame #0: 0x0000000100280a5e emacs`find_interval(tree=0x0000000000000000, position=-1) + 142 at intervals.c:678
   675 	  while (1)
   676 	    {
   677 	      eassert (tree);
-> 678 	      if (relative_position < LEFT_TOTAL_LENGTH (tree))
   679 		{
   680 		  tree = tree->left;
   681 		}
(lldb) bt
* thread #1: tid = 0x16771fc, 0x0000000100280a5e emacs`find_interval(tree=0x0000000000000000, position=-1) + 142 at intervals.c:678, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
  * frame #0: 0x0000000100280a5e emacs`find_interval(tree=0x0000000000000000, position=-1) + 142 at intervals.c:678
    frame #1: 0x000000010028356a emacs`set_point_both(charpos=-1, bytepos=-1) + 234 at intervals.c:1921
    frame #2: 0x0000000100283476 emacs`set_point(charpos=-1) + 54 at intervals.c:1816
    frame #3: 0x0000000100284597 emacs`move_if_not_intangible(position=-1) + 855 at intervals.c:2157
    frame #4: 0x00000001001abf19 emacs`Freplace_match(newtext=4298753236, fixedcase=0, literal=0, string=0, subexp=6) + 8505 at search.c:2726
    frame #5: 0x00000001001ff4f2 emacs`Ffuncall(nargs=6, args=0x00007fff5fbfc638) + 1298 at eval.c:2709
    frame #6: 0x000000010025dda1 emacs`exec_byte_code(bytestr=4309967268, vector=4312661053, maxdepth=46, args_template=2054, nargs=2, args=0x00007fff5fbfd058) + 3537 at bytecode.c:880
    frame #7: 0x00000001002005a9 emacs`funcall_lambda(fun=4312661285, nargs=2, arg_vector=0x00007fff5fbfd048) + 521 at eval.c:2855
    frame #8: 0x00000001001ff687 emacs`Ffuncall(nargs=3, args=0x00007fff5fbfd040) + 1703 at eval.c:2742
    frame #9: 0x000000010025dda1 emacs`exec_byte_code(bytestr=4320265620, vector=4312660189, maxdepth=26, args_template=3078, nargs=3, args=0x00007fff5fbfdba8) + 3537 at bytecode.c:880
    frame #10: 0x00000001002005a9 emacs`funcall_lambda(fun=4312660309, nargs=3, arg_vector=0x00007fff5fbfdb90) + 521 at eval.c:2855
    frame #11: 0x00000001001ff687 emacs`Ffuncall(nargs=4, args=0x00007fff5fbfdb88) + 1703 at eval.c:2742
    frame #12: 0x00000001001f571a emacs`Ffuncall_interactively(nargs=4, args=0x00007fff5fbfdb88) + 58 at callint.c:252
    frame #13: 0x00000001001ff2e0 emacs`Ffuncall(nargs=5, args=0x00007fff5fbfdb80) + 768 at eval.c:2673
    frame #14: 0x00000001001fef5c emacs`Fapply(nargs=3, args=0x00007fff5fbfe320) + 1196 at eval.c:2321
    frame #15: 0x00000001001f5bef emacs`Fcall_interactively(function=10062088, record_flag=0, keys=4370513885) + 1215 at callint.c:389
    frame #16: 0x00000001001ff45b emacs`Ffuncall(nargs=4, args=0x00007fff5fbfe4c8) + 1147 at eval.c:2700
    frame #17: 0x000000010025dda1 emacs`exec_byte_code(bytestr=4299648644, vector=4299648677, maxdepth=54, args_template=4102, nargs=1, args=0x00007fff5fbfeed8) + 3537 at bytecode.c:880
    frame #18: 0x00000001002005a9 emacs`funcall_lambda(fun=4299648597, nargs=1, arg_vector=0x00007fff5fbfeed0) + 521 at eval.c:2855
    frame #19: 0x00000001001ff687 emacs`Ffuncall(nargs=2, args=0x00007fff5fbfeec8) + 1703 at eval.c:2742
    frame #20: 0x00000001001fffa4 emacs`call1(fn=15168, arg1=10062088) + 68 at eval.c:2552
    frame #21: 0x000000010013a9c3 emacs`__command_loop_1_block_invoke(.block_descriptor=<unavailable>) + 2147 at keyboard.c:1519
    frame #22: 0x00000001002e6252 emacs`mac_autorelease_loop(body=0x00007fff5fbff268) + 34 at macappkit.m:835
    frame #23: 0x0000000100139ffa emacs`command_loop_1 + 794 at keyboard.c:1332
    frame #24: 0x00000001001fd333 emacs`internal_condition_case(bfun=(emacs`command_loop_1 at keyboard.c:1283), handlers=19680, hfun=(emacs`cmd_error at keyboard.c:935)) + 115 at eval.c:1309
    frame #25: 0x000000010014fa52 emacs`__command_loop_2_block_invoke(.block_descriptor=<unavailable>) + 50 at keyboard.c:1119
    frame #26: 0x00000001002e6252 emacs`mac_autorelease_loop(body=0x00000001003839c0) + 34 at macappkit.m:835
    frame #27: 0x000000010014f8fb emacs`command_loop_2(ignore=0) + 27 at keyboard.c:1118
    frame #28: 0x00000001001fcb08 emacs`internal_catch(tag=47616, func=(emacs`command_loop_2 at keyboard.c:1116), arg=0) + 72 at eval.c:1074
    frame #29: 0x000000010013924b emacs`command_loop + 251 at keyboard.c:1099
    frame #30: 0x00000001001390b0 emacs`recursive_edit_1 + 192 at keyboard.c:692
    frame #31: 0x00000001001393e1 emacs`Frecursive_edit + 305 at keyboard.c:763
    frame #32: 0x00000001001370ac emacs`main(argc=14, argv=0x00007fff5fbff838) + 5740 at emacs.c:1650
    frame #33: 0x00007fff950485ad libdyld.dylib`start + 1
    frame #34: 0x00007fff950485ad libdyld.dylib`start + 1

bug#23869: 25.0.95; replace-match can crash emacs

[Eli Zaretskii]

> From: Leo Liu <sdl.web@gmail.com>
> Date: Wed, 29 Jun 2016 22:17:20 +0800
> 
> Under some circumstances replace-match can crash emacs. It takes a lot
> to reproduce the crash and I haven't been able to find a minimal
> example. But I wonder if the backtrace that I captured in LLDB can give
> any clue to the clause? I can't reproduce the same crash in emacs24.

Do you still have this crashed session in a debugger?  Because some of
the values involved in this crash are not clear or not shown.

>     frame #1: 0x000000010028356a emacs`set_point_both(charpos=-1, bytepos=-1) + 234 at intervals.c:1921
>     frame #2: 0x0000000100283476 emacs`set_point(charpos=-1) + 54 at intervals.c:1816
>     frame #3: 0x0000000100284597 emacs`move_if_not_intangible(position=-1) + 855 at intervals.c:2157

The immediate reason is obviously the POSITION = -1 above, but the
question is: how did replace-match allow that to happen?  It's a large
function, so it's hard to tell how this could happen without seeing
some of the variables there.

Do you perhaps remember or know what text was replaced by what when
this happened?

bug#23869: 25.0.95; replace-match can crash emacs

[Leo Liu]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On 2016-06-29 18:37 +0300, Eli Zaretskii wrote:
> Do you still have this crashed session in a debugger?  Because some of
> the values involved in this crash are not clear or not shown.

Yes.

[snipped 4 lines]
> The immediate reason is obviously the POSITION = -1 above, but the
> question is: how did replace-match allow that to happen?  It's a large
> function, so it's hard to tell how this could happen without seeing
> some of the variables there.
>
> Do you perhaps remember or know what text was replaced by what when
> this happened?

Luckily I am able to find a way to reproduce this.

1. Save the attached bug.el and bug.m to disk
2. start emacs like this:
   emacs -q -l ./bug.el ./bug.m
3. Make sure we are in the bug.m buffer and eval
   (unexport-defun "is_odd/1")

Emacs should crash.


[-- Attachment #2: bug.el --]
[-- Type: application/emacs-lisp, Size: 680 bytes --]

[-- Attachment #3: bug.m --]
[-- Type: text/plain, Size: 95 bytes --]

## -*- Octave -*-
-module(bug).
-export([identity/1, is_odd/1, is_even/1, size/1, reverse/1]).

bug#23869: 25.0.95; replace-match can crash emacs

[Eli Zaretskii]

> From:  Leo Liu <sdl.web@gmail.com>
> Cc: 23869@debbugs.gnu.org
> Date: Thu, 30 Jun 2016 00:00:56 +0800
> 
> > Do you still have this crashed session in a debugger?  Because some of
> > the values involved in this crash are not clear or not shown.
> 
> Yes.

You can kill it now.

> 1. Save the attached bug.el and bug.m to disk
> 2. start emacs like this:
>    emacs -q -l ./bug.el ./bug.m
> 3. Make sure we are in the bug.m buffer and eval
>    (unexport-defun "is_odd/1")
> 
> Emacs should crash.

Thanks, reproduced.  I will take a look if no one beats me to it.

bug#23869: 25.0.95; replace-match can crash emacs

[Leo Liu]

On 2016-06-29 19:07 +0300, Eli Zaretskii wrote:
> Thanks, reproduced.  I will take a look if no one beats me to it.

Thanks, the bug is in good hands.

There may be another bug in replace-match. But it was something I saw
nearly half a year ago. I was in a hurry and chose a quick workaround to
move on.

The bug is this: replace-match may move point to point-min when changed
region doesn't involve point-min. The trigger is similar to this bug
i.e. smie-config-guess in first-change-hook. Unfortunately I forgot how
to reproduce this bug. I thought I mention it in case it is obvious from
the code.

My workaround is to wrap replace-match in save-excursion. I'll remove
the save-excursion and see if it comes up again.

Leo

bug#23869: 25.0.95; replace-match can crash emacs

[Eli Zaretskii]

> From:  Leo Liu <sdl.web@gmail.com>
> Cc: 23869@debbugs.gnu.org
> Date: Thu, 30 Jun 2016 11:27:07 +0800
> 
> 1. Save the attached bug.el and bug.m to disk
> 2. start emacs like this:
>    emacs -q -l ./bug.el ./bug.m
> 3. Make sure we are in the bug.m buffer and eval
>    (unexport-defun "is_odd/1")
>
> Emacs should crash.

The code which triggers the bug is this (from your bug.el):

  (add-hook 'first-change-hook
	    (lambda ()
	      (and (derived-mode-p 'octave-mode)
		   (smie-config-guess))))

When replace-match modifies the buffer, this hook gets run, and
smie-config-guess it calls feels free to call functions that clobber
match-data right under replace-match's feet, something that we don't
expect, because replace-match needs to use the match data after the
replacement, to adjust point for the replacement.

Morale: a buffer-modification hook should always save-match-data.

Of course, Emacs shouldn't crash if a hook doesn't, so I propose the
patch below.

This is a nasty bug, and I'd like to install it on the release
branch.  So I'd like to hear Stefan's opinion on (a) whether the fix
looks correct and safe for the release branch, and (b) whether we
might want to fix this on master in some other way.

> There may be another bug in replace-match. But it was something I saw
> nearly half a year ago. I was in a hurry and chose a quick workaround to
> move on.
> 
> The bug is this: replace-match may move point to point-min when changed
> region doesn't involve point-min. The trigger is similar to this bug
> i.e. smie-config-guess in first-change-hook. Unfortunately I forgot how
> to reproduce this bug. I thought I mention it in case it is obvious from
> the code.

I believe this is a manifestation of the same bug.  Once a
modification hook clobbers match-data, it's sheer luck (or lack
thereof) where point will wind up when replace-match returns, because
replace-match moves point to adjust it after replacement -- this is
what caused the crash in the first place, because the recipe causes
replace-match to try to move point to buffer position of -1.

Here's the proposed patch:

--- src/search.c~0	2016-06-06 10:08:54.000000000 +0300
+++ src/search.c	2016-06-30 17:26:23.678839500 +0300
@@ -2684,19 +2684,35 @@ since only regular expressions have dist
       xfree (substed);
     }
 
+  /* The functions below modify the buffer, so they could trigger
+     various modification hooks (see signal_before_change and
+     signal_after_change), which might clobber the match data we need
+     to adjust after the replacement.  So we save and restore the
+     match data around the calls.  */
+  ptrdiff_t sub_start = search_regs.start[sub];
+  ptrdiff_t sub_end = search_regs.end[sub];
+  save_search_regs ();
+
   /* Replace the old text with the new in the cleanest possible way.  */
-  replace_range (search_regs.start[sub], search_regs.end[sub],
-		 newtext, 1, 0, 1);
-  newpoint = search_regs.start[sub] + SCHARS (newtext);
+  replace_range (sub_start, sub_end, newtext, 1, 0, 1);
+
+  newpoint = sub_start + SCHARS (newtext);
 
   if (case_action == all_caps)
-    Fupcase_region (make_number (search_regs.start[sub]),
+    Fupcase_region (make_number (sub_start),
 		    make_number (newpoint),
 		    Qnil);
   else if (case_action == cap_initial)
-    Fupcase_initials_region (make_number (search_regs.start[sub]),
+    Fupcase_initials_region (make_number (sub_start),
 			     make_number (newpoint));
 
+  restore_search_regs ();
+  /* 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");
+
   /* Adjust search data for this change.  */
   {
     ptrdiff_t oldend = search_regs.end[sub];
@@ -3017,8 +3033,10 @@ static bool search_regs_saved;
 static struct re_registers saved_search_regs;
 static Lisp_Object saved_last_thing_searched;
 
-/* Called from Flooking_at, Fstring_match, search_buffer, Fstore_match_data
-   if asynchronous code (filter or sentinel) is running. */
+/* Called from Flooking_at, Fstring_match, search_buffer,
+   Fstore_match_data if asynchronous code (filter or sentinel) is
+   running.  Also called from Freplace_match, to countermand possible
+   clobbering of match data by buffer-modification hooks.  */
 static void
 save_search_regs (void)
 {
@@ -3037,7 +3055,8 @@ save_search_regs (void)
     }
 }
 
-/* Called upon exit from filters and sentinels. */
+/* Called upon exit from filters and sentinels, and before updating
+   the match data in Freplace_match. */
 void
 restore_search_regs (void)
 {

bug#23869: 25.0.95; replace-match can crash emacs

[Leo Liu]

On 2016-06-30 18:04 +0300, Eli Zaretskii wrote:
> So I'd like to hear Stefan's opinion on (a) whether the fix looks
> correct and safe for the release branch, and (b) whether we might want
> to fix this on master in some other way.

It's independence day let's celebrate it by fixing this bug?

Leo

bug#23869: 25.0.95; replace-match can crash emacs

[Eli Zaretskii]

> From:  Leo Liu <sdl.web@gmail.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  23869@debbugs.gnu.org
> Date: Mon, 04 Jul 2016 02:53:55 +0800
> 
> It's independence day

Not mine.

bug#23869: 25.0.95; replace-match can crash emacs

[Stefan Monnier]

> Morale: a buffer-modification hook should always save-match-data.

Indeed.  If it does modify the match data, it's a bug in that hook function.

> +  /* The functions below modify the buffer, so they could trigger
> +     various modification hooks (see signal_before_change and
> +     signal_after_change), which might clobber the match data we need
> +     to adjust after the replacement.  So we save and restore the
> +     match data around the calls.  */
> +  ptrdiff_t sub_start = search_regs.start[sub];
> +  ptrdiff_t sub_end = search_regs.end[sub];
> +  save_search_regs ();

I think it's important to avoid a crash, but it's not important to
behave correctly even if the change-hooks modifying the match data.
So if we can find a simpler/cheaper way to avoid the crash, it'd
be preferable.


        Stefan

bug#23869: 25.0.95; replace-match can crash emacs

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Leo Liu <sdl.web@gmail.com>,  23869@debbugs.gnu.org
> Date: Sun, 03 Jul 2016 16:08:10 -0400
> 
> > +  /* The functions below modify the buffer, so they could trigger
> > +     various modification hooks (see signal_before_change and
> > +     signal_after_change), which might clobber the match data we need
> > +     to adjust after the replacement.  So we save and restore the
> > +     match data around the calls.  */
> > +  ptrdiff_t sub_start = search_regs.start[sub];
> > +  ptrdiff_t sub_end = search_regs.end[sub];
> > +  save_search_regs ();
> 
> I think it's important to avoid a crash, but it's not important to
> behave correctly even if the change-hooks modifying the match data.
> So if we can find a simpler/cheaper way to avoid the crash, it'd
> be preferable.

The only alternative I'm aware of is to detect the clobbering and
error out, as the patch I proposed does as "last line of defense".  Is
that what you had in mind?

bug#23869: 25.0.95; replace-match can crash emacs

[Stefan Monnier]

> The only alternative I'm aware of is to detect the clobbering and
> error out, as the patch I proposed does as "last line of defense".

Sounds fine, yes.

> Is that what you had in mind?

I didn't have anything specific in mind, but indeed it's typically the
approach I've taken so far in similar circumstances (e.g. detecting
when syntax-propertize modified the buffer and cry foul if so).


        Stefan

bug#23869: 25.0.95; replace-match can crash emacs

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: sdl.web@gmail.com,  23869@debbugs.gnu.org
> Date: Sun, 03 Jul 2016 17:06:18 -0400
> 
> > The only alternative I'm aware of is to detect the clobbering and
> > error out, as the patch I proposed does as "last line of defense".
> 
> Sounds fine, yes.
> 
> > Is that what you had in mind?
> 
> I didn't have anything specific in mind, but indeed it's typically the
> approach I've taken so far in similar circumstances (e.g. detecting
> when syntax-propertize modified the buffer and cry foul if so).

Thanks, I installed on emacs-25 changes to do that, and I'm closing
this bug.

bug#23869: 25.0.95; replace-match can crash emacs

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> I didn't have anything specific in mind, but indeed it's typically the
>> approach I've taken so far in similar circumstances (e.g. detecting
>> when syntax-propertize modified the buffer and cry foul if so).
>
> Thanks, I installed on emacs-25 changes to do that, and I'm closing
> this bug.

This change is causing org-capture to error out for me.  How should I
go about determining what needs to be modified in org to fix this?

This is with Org-mode version 8.3.4 (8.3.4-99-ga8e4a3-elpa @
/home/rpluim/.emacs.d/elpa/org-20160704/), based on a quick test the
Org shipped with emacs-25 does not have this problem.

Regards

Robert