bug#32951: emacs Lock-Up-Crash Bug report and patch

bug#32951: emacs Lock-Up-Crash Bug report and patch

[Scott Corley]

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

If an emacs window exceeds 255 lines, it causes an unsigned char overflow in
scroll.c:do_direct_scrolling

The effect of this overflow is that do_direct_scrolling enters
an infinite loop.
This causes emacs to lock up in an unrecoverable way, and the process has to
be killed.

This bug can be reproduced in a number of ways:

1. Open up emacs on a very large display and expand the emacs window so that
   it is larger than 255 lines. Do some editing, add text to the window,
   enough that do_direct_scrolling is invoked.

   For example, opening and editing emacs/INSTALL will eventually cause
   the lockup. You may need to navigate through the file and/or delete and
   edit some lines.
   Once do_direct_scrolling function is invoked, it will enter an infinite
   loop and lock up emacs.

   (new 43" monitor in portrait mode is how I first encountered this bug).
   This happens in "-nw" mode and in X display mode.

2. If you don't have a large display, open up an X windows instance of
   emacs and decrease the font size until the window has more than 255
   lines. Follow the editing example in (1) above, and emacs will lock up.

3. You can also reproduce in '-nw' mode on a normal size display by opening
   up a terminal window with a very small font size, enough to have more
   than 255 vertical lines.

I can reproduce the bug using all of these methods on
these and similar systems:

   Red Hat Enterprise Linux Server Release 7.3 (Maipo)
   Intel Xeon CPU E5-2667 V3
   GNU Emacs 24.3.1
   GNU Emacs 27.0.50 (built from master)

   macOS Mojave
   2013 MacBook Pro
   Intel Core i7
   GNU Emacs 25.3.1

Cause of unsigned char overflow:

struct matrix_elt, used by do_direct_scrolling, in scroll.c, has three
fields
that are declared as unsigned char. It looks like they were previously
declared as signed char, then changed to unsigned char, perhaps to fix a
previous incarnation of this overflow.

The fields in question are 'insertcount', 'deletecount' and 'writecount'.

The patch below changes these fields to 'int'. The alloction of 'matrix_elt'
is a small memory footprint
(see allocation in scroll.c function 'scrolling_1')
I can't see a justification for keeping these fields 'unsigned char'.

On most systems, it is actually a challenge to create an emacs window larger
than 255 lines, which is why I'm guessing this overflow hasn't come up
before
(I couldn't find bug report mentioning it). However, on a system with a 4k
display in portrait mode, it is pretty easy to accidentally create a window
this large.

For example, if I am using a tmux display with reasonably-sized pane on a
4k monitor in portrait mode, and I happen to zoom the tmux pane to full
screen, the emacs window will now be larger than 255 lines, emacs will enter
an infinite loop, and the process will need to be killed.

The overflow was confirmed by attaching to a crashed/looping emacs
process with gdb, and stepping through the code, on different operating
systems and machines.

Changelog:

2018-10-05   Scott Corley <scott@scorley.com>
* scroll.c (struct matrix_elt): change unsigned char fields to int
  fixes overflow lockup crash with window sizes > 255 lines.

Here is the patch:

---
 src/scroll.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/scroll.c b/src/scroll.c
index a29f2d37f5..240005b4e3 100644
--- a/src/scroll.c
+++ b/src/scroll.c
@@ -41,13 +41,13 @@ struct matrix_elt
     int deletecost;
     /* Number of inserts so far in this run of inserts,
        for the cost in insertcost.  */
-    unsigned char insertcount;
+    int insertcount;
     /* Number of deletes so far in this run of deletes,
        for the cost in deletecost.  */
-    unsigned char deletecount;
+    int deletecount;
     /* Number of writes so far since the last insert
        or delete for the cost in writecost. */
-    unsigned char writecount;
+    int writecount;
   };

 static void do_direct_scrolling (struct frame *,
--

Thanks,
Scott

[-- Attachment #2: Type: text/html, Size: 4638 bytes --]

bug#32951: emacs Lock-Up-Crash Bug report and patch

[Paul Eggert]

Thanks for the bug report and fix. I verified that the patch fixes the lockup, 
installed the patch into the Emacs master branch, and am closing the bug report.

It might be a good idea to backport this to the emacs-26 branch, but that's 
Eli's call.

bug#32951: emacs Lock-Up-Crash Bug report and patch

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 7 Oct 2018 00:14:33 -0700
> Cc: 32951-done@debbugs.gnu.org
> 
> Thanks for the bug report and fix. I verified that the patch fixes the lockup, 
> installed the patch into the Emacs master branch, and am closing the bug report.
> 
> It might be a good idea to backport this to the emacs-26 branch, but that's 
> Eli's call.

It should have been installed on the emacs-26 branch to begin with.

Thanks.