bug#40000: 27.0.60; next-single-char-property-change hangs on bad argument

[Federico Tedin <federicotedin@gmail.com>]

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

Hey Eli, thanks for the detailed feedback.

> What you used as the description of the change is actually the
> explanation why the change was made.  The description of the change
> would be something like this:
>
>   * src/textprop.c (Fnext_single_char_property_change): Clarify in
>   the doc string the behavior when LIMIT is past the end of OBJECT.
>   Stop the search when position gets to end of buffer, for when LIMIT
>   is beyond that.  (Bug#40000)
>
> The explanation should ideally be in the comments to the code.  In
> this case, I'm not even sure we need an explanation, since there's a
> reference to the bug report, and the explanation and the fix are quite
> simple.

Aah OK, that makes sense. I agree that the bug number should be enough
if someone wants to find an explanation for the changes; I've changed
the commit message to the one you proposed.

>> -In a string, scan runs to the end of the string, unless LIMIT is non-nil.
>> -In a buffer, if LIMIT is nil or omitted, it runs to (point-max), and the
>> -value cannot exceed that.
>> -If the optional fourth argument LIMIT is non-nil, don't search
>> -past position LIMIT; return LIMIT if nothing is found before LIMIT.
>> +In a string, scan runs to the end of the string, unless LIMIT is
>> +non-nil, in which case its value is returned if nothing is found
>> +before it.
>> 
>> -The property values are compared with `eq'.
>> -If the property is constant all the way to the end of OBJECT, return the
>> -last valid position in OBJECT.  */)
>> +In a buffer, if LIMIT is nil or omitted, it runs to (point-max).  If
>> +LIMIT is non-nil, scan does not go past it, and the smallest of
>> +(point-max) and LIMIT is returned.
>> +
>> +The property values are compared with `eq'.  */)
>
> Try to avoid passive tense in documentation, it makes the text longer
> and sometimes harder to understand.  Here's how I'd rephrase this:
>
>   In a string, scan runs to the end of the string, unless LIMIT is non-nil.
>   In a buffer, scan runs to end of buffer, unless LIMIT is non-nil.
>   If the optional fourth argument LIMIT is non-nil, don't search
>   past position LIMIT; return LIMIT if nothing is found before LIMIT.
>   However, if OBJECT is a buffer and LIMIT is beyond the end of the
>   buffer, this function returns `point-max', not LIMIT.

Good point, for some reason I am biased towards using the passive
tense. I've changed the documentation string.

>   The property values are compared with `eq'.
>
>> +	    if (XFIXNAT (position) >= ZV)
>> +	      {
>> +		XSETFASTINT (position, ZV);
>> +		break;
>> +	      }
>
> Here we expect 'position' to have the value of ZV already, right.
> Then there's no need to use XSETFASTINT; if you want to make sure we
> never get here with value >= ZV, you can add an assertion.

Aah right, because `next-char-property-change' never returns
values larger than `point-max'. So in the last iteration, `position'
will be `ZV'. I don't feel like it's necessary to add an assertion
though, unless the code inside the loop somehow was modified to be
longer/more complex. I'm attaching an updated patch.

- Fede


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Prevent-hanging-in-next-single-char-property-change.patch --]
[-- Type: text/x-diff, Size: 1974 bytes --]

From 6adaeec2a5681bb290a3a70968952780146abf66 Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Sun, 3 May 2020 15:47:56 +0200
Subject: [PATCH] Prevent hanging in next-single-char-property-change

* src/textprop.c (Fnext_single_char_property_change): Clarify in
the doc string the behavior when LIMIT is past the end of OBJECT.
Stop the search when position gets to end of buffer, for when LIMIT
is beyond that.  (Bug#40000)
---
 src/textprop.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/textprop.c b/src/textprop.c
index 960dba3f8d..0876badc87 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -766,14 +766,13 @@ DEFUN ("next-single-char-property-change", Fnext_single_char_property_change,
 If OBJECT is a string, POSITION is a 0-based index into it.
 
 In a string, scan runs to the end of the string, unless LIMIT is non-nil.
-In a buffer, if LIMIT is nil or omitted, it runs to (point-max), and the
-value cannot exceed that.
+In a buffer, scan runs to end of buffer, unless LIMIT is non-nil.
 If the optional fourth argument LIMIT is non-nil, don't search
 past position LIMIT; return LIMIT if nothing is found before LIMIT.
+However, if OBJECT is a buffer and LIMIT is beyond the end of the
+buffer, this function returns `point-max', not LIMIT.
 
-The property values are compared with `eq'.
-If the property is constant all the way to the end of OBJECT, return the
-last valid position in OBJECT.  */)
+The property values are compared with `eq'.  */)
   (Lisp_Object position, Lisp_Object prop, Lisp_Object object, Lisp_Object limit)
 {
   if (STRINGP (object))
@@ -832,6 +831,9 @@ DEFUN ("next-single-char-property-change", Fnext_single_char_property_change,
 	    value = Fget_char_property (position, prop, object);
 	    if (!EQ (value, initial_value))
 	      break;
+
+	    if (XFIXNAT (position) >= ZV)
+	      break;
 	  }
 
       position = unbind_to (count, position);
-- 
2.17.1