bug#43499: 27.1; It is possible for (forward-comment -1) to crash emacs

bug#43499: 27.1; It is possible for (forward-comment -1) to crash emacs

[Jeff Norden]

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

In an unusual circumstance, (forward-comment -1) can move the point before the
accessible buffer text.  This can even result in the point becoming negative.
In the worst-case scenario, emacs becomes completely unresponsive, and it
might even be necessary to reboot the computer.

Instructions for those who want to verify this bug are below.  But the
explanation and fix are fairly simple, so I'll start with that.

The problem is the following code for forward-comment, from syntax.c starting
at line 2542 (emacs-27.1).  This is in the 2nd part of the function, which is
the code that runs when forward-comment is called with a negative arg to move
backwards.  I've marked two relevant lines with * and **.

     if (code == Scomment_fence)
       {
         /* Skip until first preceding unquoted comment_fence.  */
         bool fence_found = 0;
         ptrdiff_t ini = from, ini_byte = from_byte;

         while (1)
           {
*            DEC_BOTH (from, from_byte);
             UPDATE_SYNTAX_TABLE_BACKWARD (from);
             c = FETCH_CHAR_AS_MULTIBYTE (from_byte);
             if (SYNTAX (c) == Scomment_fence
                 && !char_quoted (from, from_byte))
               {
                 fence_found = 1;
                 break;
               }
**            else if (from == stop)
               break;
             rarely_quit (++quit_count);
           }

The loop should, I think, be changed to the following.  The only change is how
from and stop are compared.

              while (from > stop)
                {
                  DEC_BOTH (from, from_byte);
                  UPDATE_SYNTAX_TABLE_BACKWARD (from);
                  c = FETCH_CHAR_AS_MULTIBYTE (from_byte);
                  if (SYNTAX (c) == Scomment_fence
                      && !char_quoted (from, from_byte))
                    {
                      fence_found = 1;
                      break;
                    }
                  rarely_quit (++quit_count);
                }

Analysis: 'stop' is equal to BEGV.  'from' started out at PT, but will already
have been decremented once before the above is reached (to check the syntax of
the char before the point).  This makes it possible for (from == stop) to
already be true, and thus (*) can make (from < stop) true the first time it
runs.  If that happens, the test at (**) will never succeed.  This can occur
if the char at point-min has comment-fence syntax, and the point is at
point-min+1 when (forward-comment -1) is called.

---

If this bug is triggerd in a narrowed buffer, and a there is another fence
before point-min, then forward-comment will currently return with the point
set before BEGV.  This is likely to lead to an args-out-of-range error.

However, if no fence if found, or if the buffer wasn't narrowed, then 'from'
becomes negative.  This may result in a segfault, but it can also happen that
another fence-comment char will be found in the memory being scanned, and
forward-comment returns with the point set to a basically random negative
number.  It seems that this can cause all hell to break loose, but I didn't
figure out the exact mechanism.  Emacs can become unresponsive to C-g or any
keyboard input.  It seems to be constantly updating the display, which may
cause the entire window system to hang.

---

The first thing I tried was to just change (**) to 'else if (from <= stop)'.
The suggested fix above is better, though.  It will never try to access any
characters before BEGV.  It will also put the point at BEGV if a matching
fence is found there, which is probably why the original test is placed after
the syntax check.  An alternative would be to check for (from == stop) before
the loop, which is equivalent.  I've been using a version of 27.1 with the
above change for a few days with no problems.  A simple git-patch for this
change is also attached.

------------------------------
Here are instructions for verifying this bug.  The behavior below is what I've
observed under linux with the mate and gnome3 desktops.  I don't know what
will happen under ms-windows or macos.

1) Please be sure that there are no open applications with unsaved data.
Obviously, don't try this on a mission-critical server.

2) The safest thing is to run 'emacs -nw -Q' from a terminal window.  Or, use
a linux console, as long as you will be able to switch to another console to
kill emacs.

3) Open a plain fundamental-mode buffer.  Do "M-x modify-syntax-entry @ !" to
make the at-sign into a generic fence comment character.  Then put

@This is a fenced comment@

at the start of the buffer. The first at-sign should be the first character of
the buffer.

4) Try 'M-: (forward-comment -1)' with the cursor at the start of the second line.
The cursor should move to the beginning of the buffer, verifying that the
first line is a comment.

5) Now place the cursor on the 'T' after the first at-sign, so the point is
between them, at the 2nd buffer position.  Do 'M-: (forward-comment -1)'
again, and emacs should be dead.
------------------------------

When I trip this bug, emacs won't respond to C-g or any other key.  I can
close the terminal window, which kills emacs.  If I do the above from a
graphical emacs, then my entire desktop freezes, including the mouse.  I can
recover with ctrl-alt-backspace, but that is often disabled by default.  If
I switch to a linux console, top shows that both emacs and the
window-manager are each using about 100% of a cpu.  I can recover by
re-starting X from here.  Someone unable to get to a console might need to
power-cycle the computer.

If I do 'M-x toggle-debug-on-error' between steps 4 and 5, then emacs will
quickly die with a segfault.  If, in step 5, I instead do

 M-: (cons (forward-comment -1) (point))

I can see in the frozen window that forward-comment did return 't with PT<0.
So, except when emacs segfaults, the problem isn't during forward-comment,
it's what happens after the negative point-value is returned.

AFAICT, there doesn't seem to be a similar problem with (forward-comment +1).

==============================
In case you are wondering how I stumbled onto this, in CWEB (the Knuth/Levy
literate programming system) sections are defined and referenced with the
following syntax:

@<Description of a section of code@>

One way to highlight these is to set the syntax-table property of the initial
'@' and the final '>' to comment-fence, which also prevents the description
itself from being interpreted as code.  A CWEB file won't ever start with this
construct, but the definition of a code section does, and it is useful to
temporarily narrow the buffer to a section of code, including its name.

When I traced the source of args-out-of-range errors to forward-comment, I
realized that narrowing the buffer wasn't even necessary.  When I tested that
hypothesis, emacs froze up my desktop.

-Jeff

============================================================
In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.22, cairo version 1.17.3)
 of 2020-08-28 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Manjaro Linux

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft --with-wide-int
 --with-modules --with-cairo --with-harfbuzz 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe -fno-plt' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON
PDUMPER LCMS2 GMP

Important settings:
  value of $LC_COLLATE: C
  value of $LC_MONETARY: en_US.UTF-8
  value of $LC_NUMERIC: en_US.UTF-8
  value of $LC_TIME: en_US.UTF-8
  value of $LANG: en_US.utf8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t

Load-path shadows:
None found.

Features:
(shadow sort flyspell ispell mail-extr emacsbug message rmc puny dired
dired-loaddefs format-spec rfc822 mml easymenu mml-sec password-cache
epa derived epg epg-config gnus-util rmail rmail-loaddefs
text-property-search time-date subr-x seq byte-opt gv bytecomp
byte-compile cconv mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader cl-loaddefs cl-lib sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils jeff-tex jeff-commands
jeff-keys win-move jeff-custom tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting cairo move-toolbar gtk x-toolkit
x multi-tty make-network-process emacs)

Memory information:
((conses 16 52204 5081)
 (symbols 48 18686 1)
 (strings 32 74127 3714)
 (string-bytes 1 1711817)
 (vectors 16 11238)
 (vector-slots 8 537394 9004)
 (floats 8 29 30)
 (intervals 56 203 0)
 (buffers 1000 11))



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-bug-in-forward-comment-that-allowed-the-point-to.patch --]
[-- Type: text/x-diff, Size: 1013 bytes --]

From 5792fcead7bc379e4d855746b59ae0ade51e1303 Mon Sep 17 00:00:00 2001
From: Jeff Norden <jnoden@tntech.edu>
Date: Fri, 18 Sep 2020 19:34:18 -0500
Subject: [PATCH] Fix bug in forward-comment that allowed the point to move
 before BEGV, and even become negative.

---
 src/syntax.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/syntax.c b/src/syntax.c
index 7f0fc34..3dcc7ec 100644
--- a/src/syntax.c
+++ b/src/syntax.c
@@ -2542,7 +2542,7 @@ between them, return t; otherwise return nil.  */)
 	      bool fence_found = 0;
 	      ptrdiff_t ini = from, ini_byte = from_byte;
 
-	      while (1)
+	      while (from > stop)
 		{
 		  dec_both (&from, &from_byte);
 		  UPDATE_SYNTAX_TABLE_BACKWARD (from);
@@ -2553,8 +2553,6 @@ between them, return t; otherwise return nil.  */)
 		      fence_found = 1;
 		      break;
 		    }
-		  else if (from == stop)
-		    break;
 		  rarely_quit (++quit_count);
 		}
 	      if (fence_found == 0)
-- 
2.7.4

bug#43499: 27.1; It is possible for (forward-comment -1) to crash emacs

[Eli Zaretskii]

> From: Jeff Norden <jnorden@tntech.edu>
> Date: Fri, 18 Sep 2020 20:25:33 -0500
> 
> In an unusual circumstance, (forward-comment -1) can move the point before the
> accessible buffer text.  This can even result in the point becoming negative.
> In the worst-case scenario, emacs becomes completely unresponsive, and it
> might even be necessary to reboot the computer.

Thanks.  In my case, I get a segfault in DEC_BOTH (because it attempts
to dereference a pointer outside of buffer text).

> The loop should, I think, be changed to the following.  The only change is how
> from and stop are compared.
> 
>               while (from > stop)
>                 {
>                   DEC_BOTH (from, from_byte);
>                   UPDATE_SYNTAX_TABLE_BACKWARD (from);
>                   c = FETCH_CHAR_AS_MULTIBYTE (from_byte);
>                   if (SYNTAX (c) == Scomment_fence
>                       && !char_quoted (from, from_byte))
>                     {
>                       fence_found = 1;
>                       break;
>                     }
>                   rarely_quit (++quit_count);
>                 }

Thanks.  I propose a slightly different change below.  I think it's
somewhat better, because it does the comparison only once, and the
while loop can then run at full speed without testing on each
iteration.  (It looks like a large change, but almost all of it is
just whitespace changes due to re-indentation of the loop.)  Do you
agree?

diff --git a/src/syntax.c b/src/syntax.c
index a79ab86..e8b32f5 100644
--- a/src/syntax.c
+++ b/src/syntax.c
@@ -2545,20 +2545,23 @@ DEFUN ("forward-comment", Fforward_comment, Sforward_comment, 1, 1, 0,
 	      bool fence_found = 0;
 	      ptrdiff_t ini = from, ini_byte = from_byte;
 
-	      while (1)
+	      if (from > stop)
 		{
-		  DEC_BOTH (from, from_byte);
-		  UPDATE_SYNTAX_TABLE_BACKWARD (from);
-		  c = FETCH_CHAR_AS_MULTIBYTE (from_byte);
-		  if (SYNTAX (c) == Scomment_fence
-		      && !char_quoted (from, from_byte))
+		  while (1)
 		    {
-		      fence_found = 1;
-		      break;
+		      DEC_BOTH (from, from_byte);
+		      UPDATE_SYNTAX_TABLE_BACKWARD (from);
+		      c = FETCH_CHAR_AS_MULTIBYTE (from_byte);
+		      if (SYNTAX (c) == Scomment_fence
+			  && !char_quoted (from, from_byte))
+			{
+			  fence_found = 1;
+			  break;
+			}
+		      else if (from == stop)
+			break;
+		      rarely_quit (++quit_count);
 		    }
-		  else if (from == stop)
-		    break;
-		  rarely_quit (++quit_count);
 		}
 	      if (fence_found == 0)
 		{

bug#43499: 27.1; It is possible for (forward-comment -1) to crash emacs

[Alan Mackenzie]

Hello, Jeff.

Thanks for taking the trouble to report this bug, and thanks also for
analysing it and proposing a patch to fix it.

On Fri, Sep 18, 2020 at 20:25:33 -0500, Jeff Norden wrote:
> In an unusual circumstance, (forward-comment -1) can move the point before the
> accessible buffer text.  This can even result in the point becoming negative.
> In the worst-case scenario, emacs becomes completely unresponsive, and it
> might even be necessary to reboot the computer.

> Instructions for those who want to verify this bug are below.  But the
> explanation and fix are fairly simple, so I'll start with that.

> The problem is the following code for forward-comment, from syntax.c starting
> at line 2542 (emacs-27.1).  This is in the 2nd part of the function, which is
> the code that runs when forward-comment is called with a negative arg to move
> backwards.  I've marked two relevant lines with * and **.

[ Analysis and fix snipped for now. ]

> ------------------------------
> Here are instructions for verifying this bug.  The behavior below is what I've
> observed under linux with the mate and gnome3 desktops.  I don't know what
> will happen under ms-windows or macos.

> 1) Please be sure that there are no open applications with unsaved data.
> Obviously, don't try this on a mission-critical server.

> 2) The safest thing is to run 'emacs -nw -Q' from a terminal window.  Or, use
> a linux console, as long as you will be able to switch to another console to
> kill emacs.

> 3) Open a plain fundamental-mode buffer.  Do "M-x modify-syntax-entry @ !" to
> make the at-sign into a generic fence comment character.  Then put

> @This is a fenced comment@

> at the start of the buffer. The first at-sign should be the first character of
> the buffer.

> 4) Try 'M-: (forward-comment -1)' with the cursor at the start of the second line.
> The cursor should move to the beginning of the buffer, verifying that the
> first line is a comment.

> 5) Now place the cursor on the 'T' after the first at-sign, so the point is
> between them, at the 2nd buffer position.  Do 'M-: (forward-comment -1)'
> again, and emacs should be dead.
> ------------------------------

I can confirm that there is a bug, here.  When I do the above on Emacs
28 master in a Linux TTY, I get a segfault.

I agree with the OP that this needs fixing, and his fix [snipped] is
likely a good one.

[ .... ]

> AFAICT, there doesn't seem to be a similar problem with (forward-comment +1).

No.  At this level, forward and backwards movement over comments use
different code.

> ==============================
> In case you are wondering how I stumbled onto this, in CWEB (the Knuth/Levy
> literate programming system) sections are defined and referenced with the
> following syntax:

> @<Description of a section of code@>

> One way to highlight these is to set the syntax-table property of the initial
> '@' and the final '>' to comment-fence, which also prevents the description
> itself from being interpreted as code.  A CWEB file won't ever start with this
> construct, but the definition of a code section does, and it is useful to
> temporarily narrow the buffer to a section of code, including its name.

> When I traced the source of args-out-of-range errors to forward-comment, I
> realized that narrowing the buffer wasn't even necessary.  When I tested that
> hypothesis, emacs froze up my desktop.

Interesting!

> -Jeff

> ============================================================
> In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.22, cairo version 1.17.3)
>  of 2020-08-28 built on juergen
> Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
> System Description: Manjaro Linux

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#43499: 27.1; It is possible for (forward-comment -1) to crash emacs

[Jeff Norden]

> Thanks.  I propose a slightly different change below.  I think it's
> somewhat better, because it does the comparison only once, and the
> while loop can then run at full speed without testing on each
> iteration.  (It looks like a large change, but almost all of it is
> just whitespace changes due to re-indentation of the loop.)  Do you
> agree?

I think either change will work fine.  It doesn't seem to me that either
one would be faster, unless I'm missing something.  My suggestion was to
move the test from the body of the loop (where from == stop is checked
each iteration) to the clause of the while statement (as from > stop
instead).  But, maybe a test before the loop starts makes the code more
clear - that is entirely your call.

Perhaps I should have included my patch in the body of the email,
instead of as an attachment, which might have made my suggestion more
clear.

Also, it's good that you and Alan are getting segfaults instead of the
really horrible behavior that I found.  Maybe some change since 27.1 has
helped with that.

Regards,
-Jeff

bug#43499: 27.1; It is possible for (forward-comment -1) to crash emacs

[Eli Zaretskii]

> From: Jeff Norden <jnorden@tntech.edu>
> Cc: 43499@debbugs.gnu.org
> Date: Sat, 19 Sep 2020 11:24:23 -0500
> 
> > Thanks.  I propose a slightly different change below.  I think it's
> > somewhat better, because it does the comparison only once, and the
> > while loop can then run at full speed without testing on each
> > iteration.  (It looks like a large change, but almost all of it is
> > just whitespace changes due to re-indentation of the loop.)  Do you
> > agree?
> 
> I think either change will work fine.  It doesn't seem to me that either
> one would be faster, unless I'm missing something.  My suggestion was to
> move the test from the body of the loop (where from == stop is checked
> each iteration) to the clause of the while statement (as from > stop
> instead).  But, maybe a test before the loop starts makes the code more
> clear - that is entirely your call.

Thanks, I installed my changes.

> Perhaps I should have included my patch in the body of the email,
> instead of as an attachment, which might have made my suggestion more
> clear.

It was clear to me, FWIW.

bug#43499: 27.1; It is possible for (forward-comment -1) to crash emacs

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jeff Norden <jnorden@tntech.edu>
>> Cc: 43499@debbugs.gnu.org
>> Date: Sat, 19 Sep 2020 11:24:23 -0500
>>
>> > Thanks.  I propose a slightly different change below.  I think it's
>> > somewhat better, because it does the comparison only once, and the
>> > while loop can then run at full speed without testing on each
>> > iteration.  (It looks like a large change, but almost all of it is
>> > just whitespace changes due to re-indentation of the loop.)  Do you
>> > agree?
>>
>> I think either change will work fine.  It doesn't seem to me that either
>> one would be faster, unless I'm missing something.  My suggestion was to
>> move the test from the body of the loop (where from == stop is checked
>> each iteration) to the clause of the while statement (as from > stop
>> instead).  But, maybe a test before the loop starts makes the code more
>> clear - that is entirely your call.
>
> Thanks, I installed my changes.

Did that fix the issue?  Does that mean that this bug can be closed?

bug#43499: 27.1; It is possible for (forward-comment -1) to crash emacs

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 12 Nov 2020 22:40:17 -0500
> Cc: Jeff Norden <jnorden@tntech.edu>, 43499@debbugs.gnu.org
> 
> > Thanks, I installed my changes.
> 
> Did that fix the issue?  Does that mean that this bug can be closed?

Yes, done now.