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)
 {