bug#19208: replace-match unhelpful error message

bug#19208: replace-match unhelpful error message

[Phillip Lord]

Occurs in Emacs 24.3 and trunk

Running this code in *scratch* buffer

(progn
  (goto-char (point-min))
  (re-search-forward "This")
  (replace-match "That" nil nil nil))

Now undo and run this code

(progn
  (goto-char (point-min))
  (re-search-forward "This")
  (replace-match "That" nil nil nil 1))

This causes the following error.

progn: Args out of range: -1, -1


The error is caused because I have asked for a subexp that doesn't exist in
the regexp. A programmer error for sure, but the Args out of range error seems
rather unfriendly to me, especially as I have no idea where the -1, or
-1 has come from.

"Attempt to replace regexp subexpression that doesn't exist", for
example, would be nicer.

bug#19208: replace-match unhelpful error message

[Lars Ingebrigtsen]

phillip.lord@newcastle.ac.uk (Phillip Lord) writes:

> (progn
>   (goto-char (point-min))
>   (re-search-forward "This")
>   (replace-match "That" nil nil nil 1))
>
> This causes the following error.
>
> progn: Args out of range: -1, -1
>
> The error is caused because I have asked for a subexp that doesn't exist in
> the regexp. A programmer error for sure, but the Args out of range error seems
> rather unfriendly to me, especially as I have no idea where the -1, or
> -1 has come from.
>
> "Attempt to replace regexp subexpression that doesn't exist", for
> example, would be nicer.

Yup.

This is the code that signals the error:

      if (search_regs.start[sub] < BEGV
	  || search_regs.start[sub] > search_regs.end[sub]
	  || search_regs.end[sub] > ZV)
	args_out_of_range (make_fixnum (search_regs.start[sub]),
			   make_fixnum (search_regs.end[sub]));

Which is a weird way to check for this, surely?  It looks more like a
sanity checks that checks whether start[sub] and end[sub] is inside the
buffer, and that the start is before the end.

This happens to actually work here because when there is no match,
start[sub] is -1, which is always less than BEGV.

And this explains where the -1 in the args out of range comes from.

The sanity check seems sensible (because the buffer may have been
narrowed before calling replace-match), so I propose to add an
additional check that actually explicitly checks whether the subexp is
valid.

Hm...  or is this what the check right above it tried to do?

  if (NILP (subexp))
    sub = 0;
  else
    {
      CHECK_FIXNUM (subexp);
      if (! (0 <= XFIXNUM (subexp) && XFIXNUM (subexp) < search_regs.num_regs))
	args_out_of_range (subexp, make_fixnum (search_regs.num_regs));
      sub = XFIXNUM (subexp);
    }

Yoda conditionals and a !...  Let's see...

No, that's just another sanity check -- num_regs is the max allowed
number of sub-matches.  (I've now added some comments to clarify.)

I've now added a new check to check for this situation, and I've used
your suggested error string.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#19208: replace-match unhelpful error message

[Paul Eggert]

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

> No, that's just another sanity check -- num_regs is the max allowed
> number of sub-matches.  (I've now added some comments to clarify.)

Unfortunately that patch messes up the sanity check, as the patched code allows 
'sub' to be negative, or to be equal to search_regs.num_regs, and in either case 
this results in a bad pointer.

> Yoda conditionals and a !...

Actually those were Leibniz conditionals, which are comparisons involving "<" or 
"<=". The idea is that the conditionals' textual order reflects numeric order. 
This is a common style in math when doing range checking, e.g., "0 <= i < n". 
Yoda conditionals are expressions like "0 != x" which I agree are confusing.

I learned Leibniz conditionals from the late Val Schorre, who was *really* good 
at programming and programming style: Knuth credits Schorre with the invention 
of goto-less programming in the early 1960s. When I redid your patch as a 
Leibniz conditional, I instantly spotted the bug that it introduced. You might 
give Leibniz conditionals a try, as they help make code more readable and 
reliable when checking for range errors or overflow.

While in the neighborhood I spotted some other rare possibilities for bad 
behavior due to out-of-range indexes. I fixed the bugs I found by installing the 
attached.

[-- Attachment #2: 0001-Fix-rare-undefined-behaviors-in-replace-match.patch --]
[-- Type: text/x-patch, Size: 8020 bytes --]

From 13fe8a27042b1539d43727e6df97c386c61c3053 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 3 Aug 2019 12:45:19 -0700
Subject: [PATCH] Fix rare undefined behaviors in replace-match

* src/search.c (Freplace_match): Simplify by caching search_regs
components.  Fix sanity check for out-of-range subscripts;
it incorrectly allowed negative subscripts, subscripts
equal to search_regs.num_regs, and it had undefined
behavior for subscripts outside ptrdiff_t range.
Improve wording of newly-introduced replace-match diagnostic.
Rework use of opoint, to avoid setting point to an out-of-range value
in rare cases involving modification hooks.
---
 src/search.c | 107 ++++++++++++++++++---------------------------------
 1 file changed, 38 insertions(+), 69 deletions(-)

diff --git a/src/search.c b/src/search.c
index 0e2ae059e8..9b674a5810 100644
--- a/src/search.c
+++ b/src/search.c
@@ -2389,44 +2389,32 @@ since only regular expressions have distinguished subexpressions.  */)
   case_action = nochange;	/* We tried an initialization */
 				/* but some C compilers blew it */
 
-  if (search_regs.num_regs <= 0)
+  ptrdiff_t num_regs = search_regs.num_regs;
+  if (num_regs <= 0)
     error ("`replace-match' called before any match found");
 
   if (NILP (subexp))
     sub = 0;
   else
     {
-      CHECK_FIXNUM (subexp);
+      CHECK_RANGED_INTEGER (subexp, 0, num_regs - 1);
       sub = XFIXNUM (subexp);
-      /* Sanity check to see whether the subexp is larger than the
-	 allowed number of sub-regexps. */
-      if (sub >= 0 && sub > search_regs.num_regs)
-	args_out_of_range (subexp, make_fixnum (search_regs.num_regs));
     }
 
-  /* Check whether the subexpression to replace is greater than the
-     number of subexpressions in the regexp. */
-  if (sub > 0 && search_regs.start[sub] == -1)
-    args_out_of_range (build_string ("Attempt to replace regexp subexpression that doesn't exist"),
-		       subexp);
+  ptrdiff_t sub_start = search_regs.start[sub];
+  ptrdiff_t sub_end = search_regs.end[sub];
+  eassert (sub_start <= sub_end);
 
-  /* Sanity check to see whether the text to replace is present in the
-     buffer/string. */
-  if (NILP (string))
+  /* Check whether the text to replace is present in the buffer/string.  */
+  if (! (NILP (string)
+	 ? BEGV <= sub_start && sub_end <= ZV
+	 : 0 <= sub_start && sub_end <= SCHARS (string)))
     {
-      if (search_regs.start[sub] < BEGV
-	  || search_regs.start[sub] > search_regs.end[sub]
-	  || search_regs.end[sub] > ZV)
-	args_out_of_range (make_fixnum (search_regs.start[sub]),
-			   make_fixnum (search_regs.end[sub]));
-    }
-  else
-    {
-      if (search_regs.start[sub] < 0
-	  || search_regs.start[sub] > search_regs.end[sub]
-	  || search_regs.end[sub] > SCHARS (string))
-	args_out_of_range (make_fixnum (search_regs.start[sub]),
-			   make_fixnum (search_regs.end[sub]));
+      if (sub_start < 0)
+	xsignal2 (Qerror,
+		  build_string ("replace-match subexpression does not exist"),
+		  subexp);
+      args_out_of_range (make_fixnum (sub_start), make_fixnum (sub_end));
     }
 
   if (NILP (fixedcase))
@@ -2434,8 +2422,8 @@ since only regular expressions have distinguished subexpressions.  */)
       /* Decide how to casify by examining the matched text. */
       ptrdiff_t last;
 
-      pos = search_regs.start[sub];
-      last = search_regs.end[sub];
+      pos = sub_start;
+      last = sub_end;
 
       if (NILP (string))
 	pos_byte = CHAR_TO_BYTE (pos);
@@ -2511,9 +2499,8 @@ since only regular expressions have distinguished subexpressions.  */)
     {
       Lisp_Object before, after;
 
-      before = Fsubstring (string, make_fixnum (0),
-			   make_fixnum (search_regs.start[sub]));
-      after = Fsubstring (string, make_fixnum (search_regs.end[sub]), Qnil);
+      before = Fsubstring (string, make_fixnum (0), make_fixnum (sub_start));
+      after = Fsubstring (string, make_fixnum (sub_end), Qnil);
 
       /* Substitute parts of the match into NEWTEXT
 	 if desired.  */
@@ -2542,12 +2529,12 @@ since only regular expressions have distinguished subexpressions.  */)
 
 		  if (c == '&')
 		    {
-		      substart = search_regs.start[sub];
-		      subend = search_regs.end[sub];
+		      substart = sub_start;
+		      subend = sub_end;
 		    }
 		  else if (c >= '1' && c <= '9')
 		    {
-		      if (c - '0' < search_regs.num_regs
+		      if (c - '0' < num_regs
 			  && search_regs.start[c - '0'] >= 0)
 			{
 			  substart = search_regs.start[c - '0'];
@@ -2612,13 +2599,8 @@ since only regular expressions have distinguished subexpressions.  */)
       return concat3 (before, newtext, after);
     }
 
-  /* Record point, then move (quietly) to the start of the match.  */
-  if (PT >= search_regs.end[sub])
-    opoint = PT - ZV;
-  else if (PT > search_regs.start[sub])
-    opoint = search_regs.end[sub] - ZV;
-  else
-    opoint = PT;
+  /* Record point.  A nonpositive OPOINT is actually an offset from ZV.  */
+  opoint = PT <= sub_start ? PT : max (PT, sub_end) - ZV;
 
   /* If we want non-literal replacement,
      perform substitution on the replacement string.  */
@@ -2687,7 +2669,7 @@ since only regular expressions have distinguished subexpressions.  */)
 
 	      if (c == '&')
 		idx = sub;
-	      else if (c >= '1' && c <= '9' && c - '0' < search_regs.num_regs)
+	      else if ('1' <= c && c <= '9' && c - '0' < num_regs)
 		{
 		  if (search_regs.start[c - '0'] >= 1)
 		    idx = c - '0';
@@ -2745,25 +2727,11 @@ since only regular expressions have distinguished subexpressions.  */)
       xfree (substed);
     }
 
-  /* The functions below modify the buffer, so they could trigger
-     various modification hooks (see signal_before_change and
-     signal_after_change).  If these hooks clobber the match data we
-     error out since otherwise this will result in confusing bugs.  */
-  ptrdiff_t sub_start = search_regs.start[sub];
-  ptrdiff_t sub_end = search_regs.end[sub];
-  ptrdiff_t num_regs = search_regs.num_regs;
-  newpoint = search_regs.start[sub] + SCHARS (newtext);
+  newpoint = sub_start + SCHARS (newtext);
+  ptrdiff_t newstart = sub_start == sub_end ? newpoint : sub_start;
 
   /* 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, 1);
-  /* Update saved data to match adjustment made by replace_range.  */
-  {
-    ptrdiff_t change = newpoint - sub_end;
-    if (sub_start >= sub_end)
-      sub_start += change;
-    sub_end += change;
-  }
+  replace_range (sub_start, sub_end, newtext, 1, 0, 1, true);
 
   if (case_action == all_caps)
     Fupcase_region (make_fixnum (search_regs.start[sub]),
@@ -2773,17 +2741,18 @@ since only regular expressions have distinguished subexpressions.  */)
     Fupcase_initials_region (make_fixnum (search_regs.start[sub]),
 			     make_fixnum (newpoint));
 
-  if (search_regs.start[sub] != sub_start
-      || search_regs.end[sub] != sub_end
-      || search_regs.num_regs != num_regs)
+  /* 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))
     error ("Match data clobbered by buffer modification hooks");
 
-  /* Put point back where it was in the text.  */
-  if (opoint <= 0)
-    TEMP_SET_PT (opoint + ZV);
-  else
-    TEMP_SET_PT (opoint);
-
+  /* Put point back where it was in the text, if possible.  */
+  TEMP_SET_PT (clip_to_bounds (BEGV, opoint + (opoint <= 0 ? ZV : 0), ZV));
   /* Now move point "officially" to the start of the inserted replacement.  */
   move_if_not_intangible (newpoint);
 
-- 
2.17.1

bug#19208: replace-match unhelpful error message

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

>> No, that's just another sanity check -- num_regs is the max allowed
>> number of sub-matches.  (I've now added some comments to clarify.)
>
> Unfortunately that patch messes up the sanity check, as the patched
> code allows 'sub' to be negative, or to be equal to
> search_regs.num_regs, and in either case this results in a bad
> pointer.

Sorry; thanks for fixing.

>> Yoda conditionals and a !...
>
> Actually those were Leibniz conditionals, which are comparisons
> involving "<" or "<=". The idea is that the conditionals' textual
> order reflects numeric order. This is a common style in math when
> doing range checking, e.g., "0 <= i < n". Yoda conditionals are
> expressions like "0 != x" which I agree are confusing.

Well, I think both are called Yoda conditionals, and they are
demonstrably confusing for people not used to seeing conditionals
written that way, which is I think pretty much everybody, except four
people working on Emacs.  :-)

In real life, nobody says "if 62 is less than your age, you're eligible
for Medicare".  I think one should write code for legibility -- but
opinions on what's legible various, of course.  Reading "if (62 < age)",
for me, requires mental gymnastics.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#19208: replace-match unhelpful error message

[Paul Eggert]

Lars Ingebrigtsen wrote:
> they are
> demonstrably confusing for people not used to seeing conditionals
> written that way, which is I think pretty much everybody, except four
> people working on Emacs.  :-)

Just four?  :-)  (Actually the style is also used in other GNU projects.)

In this particular case the Leibniz style caught a potentially-serious bug. 
Although I understand that it can be a minor irritation for newcomers, it is 
quite useful for range checking and in practice I find that its reliability 
outweighs any irritation. (I didn't like Val Schorre's advice either, when I 
first heard it - but he was right.)

bug#19208: replace-match unhelpful error message

[Noam Postavsky]

>> Actually those were Leibniz conditionals, which are comparisons
>> involving "<" or "<=". The idea is that the conditionals' textual
>> order reflects numeric order. This is a common style in math when
>> doing range checking, e.g., "0 <= i < n". Yoda conditionals are
>> expressions like "0 != x" which I agree are confusing.
>
> Well, I think both are called Yoda conditionals, and they are
> demonstrably confusing for people not used to seeing conditionals
> written that way, which is I think pretty much everybody, except four
> people working on Emacs.  :-)
>
> In real life, nobody says "if 62 is less than your age, you're eligible
> for Medicare".

People might say things like "your height must be between 120 and 200 cm
to ride this roller coaster" as opposed to "your height must be more
than 120 cm and your height must be less than 200 cm".

> I think one should write code for legibility -- but opinions on what's
> legible various, of course.  Reading "if (62 < age)", for me, requires
> mental gymnastics.

"if (height > 120 && height < 200)" require mental gymnastics, for me,
because each comparison is a different direction.

bug#19208: replace-match unhelpful error message

[Andreas Schwab]

On Aug 03 2019, Noam Postavsky <npostavs@gmail.com> wrote:

> People might say things like "your height must be between 120 and 200 cm
> to ride this roller coaster"

This still puts "height" (the variable) first.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#19208: replace-match unhelpful error message

[Andreas Schwab]

On Aug 03 2019, Paul Eggert <eggert@cs.ucla.edu> wrote:

> Lars Ingebrigtsen wrote:
>> they are
>> demonstrably confusing for people not used to seeing conditionals
>> written that way, which is I think pretty much everybody, except four
>> people working on Emacs.  :-)
>
> Just four?  :-)  (Actually the style is also used in other GNU projects.)

And always the same four people.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#19208: replace-match unhelpful error message

[Andreas Schwab]

On Aug 03 2019, Paul Eggert <eggert@cs.ucla.edu> wrote:

> I learned Leibniz conditionals from the late Val Schorre, who was *really*
> good at programming and programming style: Knuth credits Schorre with the
> invention of goto-less programming in the early 1960s. When I redid your
> patch as a Leibniz conditional, I instantly spotted the bug that it
> introduced. You might give Leibniz conditionals a try, as they help make
> code more readable and reliable when checking for range errors or
> overflow.

Perhaps those that need that construct are not good enough to cope with
natural languages.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#19208: replace-match unhelpful error message

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

> People might say things like "your height must be between 120 and 200 cm
> to ride this roller coaster" as opposed to "your height must be more
> than 120 cm and your height must be less than 200 cm".

And they don't say "120 cm must be less than your height, which must also
be less than 200 cm".  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#19208: replace-match unhelpful error message

[Noam Postavsky]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Aug 03 2019, Noam Postavsky <npostavs@gmail.com> wrote:
>
>> People might say things like "your height must be between 120 and 200 cm
>> to ride this roller coaster"
>
> This still puts "height" (the variable) first.

Lars Ingebrigtsen <larsi@gnus.org> writes:
>
> And they don't say "120 cm must be less than your height, which must also
> be less than 200 cm".  :-)

Right, but unless we want to use a macro like

    IN_RANGE_P (height, 120, 200)

I think ((120 < height) && (height < 200)) is the closest we can get to
the natural phrasing.  In Elisp we can do (< 120 height 200) which is
nicer, but we're still stuck with `and' for semi-closed intervals.

bug#19208: replace-match unhelpful error message

[Andreas Schwab]

On Aug 04 2019, Noam Postavsky <npostavs@gmail.com> wrote:

> I think ((120 < height) && (height < 200)) is the closest we can get to
> the natural phrasing.

"X is between A and B" means "X is bigger than A and [X is] smaller than
B".  You would never say "A is smaller than X and [X is] smaller than
B".

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#19208: replace-match unhelpful error message

[Paul Eggert]

This is not strictly a question of what one would say in English, as 
mathematical notation is typically a better choice for mathematical notions. For 
example, in C it's typically better to write sixteen as "16" rather than as "6 + 
10" even though the latter is closer to the original English.

If we wanted code to mimic English better, COBOL would be a good choice. (Or 
maybe Perl. :-) But the goal is to write readable code, not to imitate how 
Anglo-Saxons talked.

bug#19208: replace-match unhelpful error message

[Andreas Schwab]

On Aug 04 2019, Paul Eggert <eggert@cs.ucla.edu> wrote:

> readable code

Exactly.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#19208: replace-match unhelpful error message

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I think ((120 < height) && (height < 200)) is the closest we can get to
  > the natural phrasing.

That parallels the mathematical expression 120 < h < 200, which is a
plus, but doesn't parallel the textual form "height must be between
120 and 200", which is a minus.

The way to parallel the latter is (height > 120 && height < 200).

I think both of those are ok, but let's avoid other orderings
-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)