bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Johan =?UTF-8?Q?Bockg=C3=A5rd]

emacs -Q /tmp/empty.pl

@x
C-M-b

  => error "Point before start of properties"

The syntax of "@" in perl-mode is `. p' (punctuation/prefix char).

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Noam Postavsky]

found 3552 24.5
found 3552 25.0.94
tag 3552 + confirmed
quit

Still a problem with latest Emacs 25 pretest, and on Windows 8, Emacs
25.0.94 this actually crashes Emacs too.

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Noam Postavsky]

# bumping severity due to crash potential
severity 3352 important
tag 3352 + patch
quit

On Thu, Jun 2, 2016 at 11:34 PM, Noam Postavsky
<npostavs@users.sourceforge.net> wrote:
> Still a problem with latest Emacs 25 pretest, and on Windows 8, Emacs
> 25.0.94 this actually crashes Emacs too.

Running under valgrind I get "invalid read of size 1" in
Fbackward_prefix_chars on GNU/Linux as well (see below). I think this
is a long standing bug that allows reading from before beginning of
the buffer. It was introduced way back in 1998, 1fd3172dd4819
"(Fbackward_prefix_chars): Set point properly while scanning."

diff --git a/src/syntax.c b/src/syntax.c
index 4ac1c8d..0235767 100644
--- a/src/syntax.c
+++ b/src/syntax.c
@@ -2174,12 +2174,16 @@ DEFUN ("backward-prefix-chars",
Fbackward_prefix_chars, Sbackward_prefix_chars,

   DEC_BOTH (pos, pos_byte);

-  while (pos + 1 > beg && !char_quoted (pos, pos_byte)
+  while (!char_quoted (pos, pos_byte)
      /* Previous statement updates syntax table.  */
      && ((c = FETCH_CHAR (pos_byte), SYNTAX (c) == Squote)
          || SYNTAX_PREFIX (c)))
     {
-      DEC_BOTH (pos, pos_byte);
+      opoint = pos;
+      opoint_byte = pos_byte;
+
+      if (pos + 1 > beg)
+    DEC_BOTH (pos, pos_byte);
     }

   SET_PT_BOTH (opoint, opoint_byte);


The (pos + 1 > beg) check originally followed the decrementing of pos,
but after that commit the check came before (and also doesn't end the
loop anymore). Therefore, if (pos == beg), we decrement and then try
to look at the syntax of the character at position (beg-1). This may
segfault, or trigger the "point before start of properties" error in
update_interval (eventually called from char_quoted).

I propose the following patch be applied to the emacs-25 branch:

@@ -3109,8 +3109,9 @@ DEFUN ("backward-prefix-chars",
Fbackward_prefix_chars, Sbackward_prefix_chars,
       opoint = pos;
       opoint_byte = pos_byte;

-      if (pos + 1 > beg)
-    DEC_BOTH (pos, pos_byte);
+      DEC_BOTH (pos, pos_byte);
+      if (pos < beg)
+        break;
     }

   SET_PT_BOTH (opoint, opoint_byte);


This fixes the originally reported error, and the invalid read, cf the
valgrind output mentioned above:

==2557== Invalid read of size 1
==2557==    at 0x56691D: Fbackward_prefix_chars (syntax.c:3113)
==2557==    by 0x541543: Ffuncall (eval.c:2690)
==2557==    by 0x5704D9: exec_byte_code (bytecode.c:880)
==2557==    by 0x541151: funcall_lambda (eval.c:2855)
==2557==    by 0x54167E: Ffuncall (eval.c:2742)
==2557==    by 0x5704D9: exec_byte_code (bytecode.c:880)
==2557==    by 0x541151: funcall_lambda (eval.c:2855)
==2557==    by 0x54167E: Ffuncall (eval.c:2742)
==2557==    by 0x53D941: Ffuncall_interactively (callint.c:252)
==2557==    by 0x5414E2: Ffuncall (eval.c:2673)
==2557==    by 0x53F07D: Fcall_interactively (callint.c:840)
==2557==    by 0x54157F: Ffuncall (eval.c:2700)
==2557==  Address 0x146aab9f is 1 bytes before a block of size 2,146 alloc'd
==2557==    at 0x4C2CB1D: realloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2557==    by 0x527F90: lrealloc (alloc.c:1427)
==2557==    by 0x529628: xrealloc (alloc.c:856)
==2557==    by 0x4F837F: enlarge_buffer_text (buffer.c:4974)
==2557==    by 0x4FB610: make_gap_larger (insdel.c:393)
==2557==    by 0x4FB6D7: make_gap (insdel.c:491)
==2557==    by 0x4FC5D7: insert_from_string_1 (insdel.c:926)
==2557==    by 0x4FD157: insert_from_string (insdel.c:872)
==2557==    by 0x535103: general_insert_function (editfns.c:2468)
==2557==    by 0x53514C: Finsert (editfns.c:2504)
==2557==    by 0x571D28: exec_byte_code (bytecode.c:1509)
==2557==    by 0x541151: funcall_lambda (eval.c:2855)

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Noam Postavsky]

On Sat, Jun 4, 2016 at 9:35 AM, Noam Postavsky
<npostavs@users.sourceforge.net> wrote:
> I propose the following patch be applied to the emacs-25 branch:

Sorry, that's not quite right, I didn't realize DEC_BOTH also reads
from the buffer, here is a patch that actually fixes the invalid read:

@@ -3109,8 +3109,10 @@ DEFUN ("backward-prefix-chars",
Fbackward_prefix_chars, Sbackward_prefix_chars,
       opoint = pos;
       opoint_byte = pos_byte;

-      if (pos + 1 > beg)
+      if (pos > beg)
     DEC_BOTH (pos, pos_byte);
+      else
+        break;
     }

   SET_PT_BOTH (opoint, opoint_byte);

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sat, 4 Jun 2016 11:22:23 -0400
> 
> -      if (pos + 1 > beg)
> +      if (pos > beg)
>      DEC_BOTH (pos, pos_byte);
> +      else
> +        break;

I would use

  if (pos <= beg)
    break;
  DEC_BOTH (pos, pos_byte);

But I don't insist.

Thanks.

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Noam Postavsky]

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

On Sat, Jun 4, 2016 at 1:55 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> I would use
>
>   if (pos <= beg)
>     break;
>   DEC_BOTH (pos, pos_byte);

Oh yeah, that makes sense; parallels with the same check at the
beginning of the function. Full patch attached.

[-- Attachment #2: 0001-Fbackward_prefix_chars-stay-within-buffer-bounds.patch --]
[-- Type: text/x-diff, Size: 1273 bytes --]

From fb739ee2f83df58266c8bfc6a0e4426fed5b5890 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 4 Jun 2016 09:02:20 -0400
Subject: [PATCH] Fbackward_prefix_chars: stay within buffer bounds

The commit 1fd3172d "(Fbackward_prefix_chars): Set point properly while
scanning" (1998-03-18), moved the check against of the position against the
buffer beginning out the loop condition so that we might end up checking
the syntax of characters before the beginning of the buffer.  This can
cause segfaults or trigger a "Point before start of properties" error in
`update_interval' (called indirectly from `char_quoted').

* src/syntax.c (Fbackward_prefix_chars): Stop the loop when beginning of
buffer is reached (Bug #3552).
---
 src/syntax.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/syntax.c b/src/syntax.c
index 8e14bf3..b1ba5c6 100644
--- a/src/syntax.c
+++ b/src/syntax.c
@@ -3109,8 +3109,9 @@ DEFUN ("backward-prefix-chars", Fbackward_prefix_chars, Sbackward_prefix_chars,
       opoint = pos;
       opoint_byte = pos_byte;
 
-      if (pos + 1 > beg)
-	DEC_BOTH (pos, pos_byte);
+      if (pos <= beg)
+        break;
+      DEC_BOTH (pos, pos_byte);
     }
 
   SET_PT_BOTH (opoint, opoint_byte);
-- 
2.8.0

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[martin rudalics]

 > * src/syntax.c (Fbackward_prefix_chars): Stop the loop when beginning of
 > buffer is reached (Bug #3552).

Make it

* src/syntax.c (Fbackward_prefix_chars): Stop the loop when beginning of
buffer is reached (Bug#3552, Bug#19379).

martin

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Noam Postavsky]

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

forcemerge 3552 17132 19379
quit


On Sun, Jun 5, 2016 at 3:36 AM, martin rudalics <rudalics@gmx.at> wrote:
> Make it
>
> * src/syntax.c (Fbackward_prefix_chars): Stop the loop when beginning of
> buffer is reached (Bug#3552, Bug#19379).

Heh, seeing that I decided to search the bug database for
backwards-prefix-chars and found also Bug #17132. Updated patch
attached.

[-- Attachment #2: 0001-Fbackward_prefix_chars-stay-within-buffer-bounds.patch --]
[-- Type: text/x-diff, Size: 1297 bytes --]

From 985874ebcfae96983857e819f570cac3551052c7 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 4 Jun 2016 09:02:20 -0400
Subject: [PATCH] Fbackward_prefix_chars: stay within buffer bounds

The commit 1fd3172d "(Fbackward_prefix_chars): Set point properly while
scanning" (1998-03-18), moved the check against of the position against the
buffer beginning out the loop condition so that we might end up checking
the syntax of characters before the beginning of the buffer.  This can
cause segfaults or trigger a "Point before start of properties" error in
`update_interval' (called indirectly from `char_quoted').

* src/syntax.c (Fbackward_prefix_chars): Stop the loop when beginning of
buffer is reached (Bug #3552, Bug #17132, Bug #19379).
---
 src/syntax.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/syntax.c b/src/syntax.c
index 8e14bf3..b1ba5c6 100644
--- a/src/syntax.c
+++ b/src/syntax.c
@@ -3109,8 +3109,9 @@ DEFUN ("backward-prefix-chars", Fbackward_prefix_chars, Sbackward_prefix_chars,
       opoint = pos;
       opoint_byte = pos_byte;
 
-      if (pos + 1 > beg)
-	DEC_BOTH (pos, pos_byte);
+      if (pos <= beg)
+        break;
+      DEC_BOTH (pos, pos_byte);
     }
 
   SET_PT_BOTH (opoint, opoint_byte);
-- 
2.8.0

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Noam Postavsky]

Is it okay to install this to emacs-25? While the bug is
long-standing, I think it's important enough to go in the release
since it can crash Emacs.

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Wed, 15 Jun 2016 22:07:43 -0400
> Cc: Eli Zaretskii <eliz@gnu.org>, 3552@debbugs.gnu.org
> 
> Is it okay to install this to emacs-25?

I was sure you already did.

Yes, please.

bug#3552: 23.0.94; backward-prefix-chars: Point before start of properties

[Noam Postavsky]

Version: 25.1

On Thu, Jun 16, 2016 at 11:05 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Noam Postavsky <npostavs@users.sourceforge.net>
>> Date: Wed, 15 Jun 2016 22:07:43 -0400
>> Cc: Eli Zaretskii <eliz@gnu.org>, 3552@debbugs.gnu.org
>>
>> Is it okay to install this to emacs-25?
>
> I was sure you already did.
>
> Yes, please.

Now pushed as b49cb0ab