bug#33547: 27.0.50; Emacs stuck in infinite loop in garbage collection

[Paul Eggert <eggert@cs.ucla.edu>]

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

Thanks for reporting that. I installed the first attached patch into 
Emacs master and am marking this bug as done. I am not sure whether a 
simpler patch should be installed into the emacs-26 branch, but the 
second attached patch should do the trick for that if needed, not that 
it's easy to test this sort of thing.


[-- Attachment #2: 0001-Fix-infloop-in-GC-mark_kboards-master.patch --]
[-- Type: text/x-patch, Size: 13130 bytes --]

From 7d9fa89fb3f6db0bdc3960bbbf6c0cf34c98d1ca Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 30 Nov 2018 14:22:54 -0800
Subject: [PATCH] Fix infloop in GC mark_kboards

* src/keyboard.c (KBD_BUFFER_SIZE): Now a constant, not a macro.
(kbd_fetch_ptr, kbd_store_ptr): These now always point somewhere
into kbd_buffer, instead of sometimes pointing just past the
end which led to serious bugs (Bug#33547).  All uses changed.
(kbd_store_ptr): No longer volatile.  This variable has not been
accessed by a signal handler for some time, it seems.
(next_kbd_event, prev_kbd_event): New functions.
(kbd_buffer_nr_stored, process_special_events): Simplify.
---
 src/keyboard.c | 181 ++++++++++++++++++++-----------------------------
 1 file changed, 72 insertions(+), 109 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index be727a6549..59acb2dd08 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -92,7 +92,7 @@ volatile int interrupt_input_blocked;
    The maybe_quit function checks this.  */
 volatile bool pending_signals;
 
-#define KBD_BUFFER_SIZE 4096
+enum { KBD_BUFFER_SIZE = 4096 };
 
 KBOARD *initial_kboard;
 KBOARD *current_kboard;
@@ -286,15 +286,11 @@ static bool input_was_pending;
 static union buffered_input_event kbd_buffer[KBD_BUFFER_SIZE];
 
 /* Pointer to next available character in kbd_buffer.
-   If kbd_fetch_ptr == kbd_store_ptr, the buffer is empty.
-   This may be kbd_buffer + KBD_BUFFER_SIZE, meaning that the
-   next available char is in kbd_buffer[0].  */
+   If kbd_fetch_ptr == kbd_store_ptr, the buffer is empty.  */
 static union buffered_input_event *kbd_fetch_ptr;
 
-/* Pointer to next place to store character in kbd_buffer.  This
-   may be kbd_buffer + KBD_BUFFER_SIZE, meaning that the next
-   character should go in kbd_buffer[0].  */
-static union buffered_input_event *volatile kbd_store_ptr;
+/* Pointer to next place to store character in kbd_buffer.  */
+static union buffered_input_event *kbd_store_ptr;
 
 /* The above pair of variables forms a "queue empty" flag.  When we
    enqueue a non-hook event, we increment kbd_store_ptr.  When we
@@ -302,8 +298,7 @@ static union buffered_input_event *volatile kbd_store_ptr;
    there is input available if the two pointers are not equal.
 
    Why not just have a flag set and cleared by the enqueuing and
-   dequeuing functions?  Such a flag could be screwed up by interrupts
-   at inopportune times.  */
+   dequeuing functions?  The code is a bit simpler this way.  */
 
 static void recursive_edit_unwind (Lisp_Object buffer);
 static Lisp_Object command_loop (void);
@@ -375,6 +370,20 @@ static void deliver_user_signal (int);
 static char *find_user_signal_name (int);
 static void store_user_signal_events (void);
 
+/* Advance or retreat a buffered input event pointer.  */
+
+static union buffered_input_event *
+next_kbd_event (union buffered_input_event *ptr)
+{
+  return ptr == kbd_buffer + KBD_BUFFER_SIZE - 1 ? kbd_buffer : ptr + 1;
+}
+
+static union buffered_input_event *
+prev_kbd_event (union buffered_input_event *ptr)
+{
+  return ptr == kbd_buffer ? kbd_buffer + KBD_BUFFER_SIZE - 1 : ptr - 1;
+}
+
 /* Like EVENT_START, but assume EVENT is an event.
    This pacifies gcc -Wnull-dereference, which might otherwise
    complain about earlier checks that EVENT is indeed an event.  */
@@ -3338,8 +3347,6 @@ readable_events (int flags)
 
 	  do
 	    {
-              if (event == kbd_buffer + KBD_BUFFER_SIZE)
-                event = kbd_buffer;
 	      if (!(
 #ifdef USE_TOOLKIT_SCROLL_BARS
 		    (flags & READABLE_EVENTS_FILTER_EVENTS) &&
@@ -3356,7 +3363,7 @@ readable_events (int flags)
 		  && !((flags & READABLE_EVENTS_FILTER_EVENTS)
 		       && event->kind == BUFFER_SWITCH_EVENT))
 		return 1;
-	      event++;
+	      event = next_kbd_event (event);
 	    }
 	  while (event != kbd_store_ptr);
         }
@@ -3410,12 +3417,8 @@ event_to_kboard (struct input_event *event)
 static int
 kbd_buffer_nr_stored (void)
 {
-  return kbd_fetch_ptr == kbd_store_ptr
-    ? 0
-    : (kbd_fetch_ptr < kbd_store_ptr
-       ? kbd_store_ptr - kbd_fetch_ptr
-       : ((kbd_buffer + KBD_BUFFER_SIZE) - kbd_fetch_ptr
-          + (kbd_store_ptr - kbd_buffer)));
+  int n = kbd_store_ptr - kbd_fetch_ptr;
+  return n + (n < 0 ? KBD_BUFFER_SIZE : 0);
 }
 #endif	/* Store an event obtained at interrupt level into kbd_buffer, fifo */
 
@@ -3466,12 +3469,10 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
 		(kb, list2 (make_lispy_switch_frame (event->ie.frame_or_window),
 			    make_fixnum (c)));
 	      kb->kbd_queue_has_data = true;
-	      union buffered_input_event *sp;
-	      for (sp = kbd_fetch_ptr; sp != kbd_store_ptr; sp++)
-		{
-		  if (sp == kbd_buffer + KBD_BUFFER_SIZE)
-		    sp = kbd_buffer;
 
+	      for (union buffered_input_event *sp = kbd_fetch_ptr;
+		   sp != kbd_store_ptr; sp = next_kbd_event (sp))
+		{
 		  if (event_to_kboard (&sp->ie) == kb)
 		    {
 		      sp->ie.kind = NO_EVENT;
@@ -3516,22 +3517,18 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
      Just ignore the second one.  */
   else if (event->kind == BUFFER_SWITCH_EVENT
 	   && kbd_fetch_ptr != kbd_store_ptr
-	   && ((kbd_store_ptr == kbd_buffer
-		? kbd_buffer + KBD_BUFFER_SIZE - 1
-		: kbd_store_ptr - 1)->kind) == BUFFER_SWITCH_EVENT)
+	   && prev_kbd_event (kbd_store_ptr)->kind == BUFFER_SWITCH_EVENT)
     return;
 
-  if (kbd_store_ptr - kbd_buffer == KBD_BUFFER_SIZE)
-    kbd_store_ptr = kbd_buffer;
-
   /* Don't let the very last slot in the buffer become full,
      since that would make the two pointers equal,
      and that is indistinguishable from an empty buffer.
      Discard the event if it would fill the last slot.  */
-  if (kbd_fetch_ptr - 1 != kbd_store_ptr)
+  union buffered_input_event *next_slot = next_kbd_event (kbd_store_ptr);
+  if (kbd_fetch_ptr != next_slot)
     {
       *kbd_store_ptr = *event;
-      ++kbd_store_ptr;
+      kbd_store_ptr = next_slot;
 #ifdef subprocesses
       if (kbd_buffer_nr_stored () > KBD_BUFFER_SIZE / 2
 	  && ! kbd_on_hold_p ())
@@ -3574,11 +3571,8 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
 void
 kbd_buffer_unget_event (struct selection_input_event *event)
 {
-  if (kbd_fetch_ptr == kbd_buffer)
-    kbd_fetch_ptr = kbd_buffer + KBD_BUFFER_SIZE;
-
   /* Don't let the very last slot in the buffer become full,  */
-  union buffered_input_event *kp = kbd_fetch_ptr - 1;
+  union buffered_input_event *kp = prev_kbd_event (kbd_fetch_ptr);
   if (kp != kbd_store_ptr)
     {
       kp->sie = *event;
@@ -3666,12 +3660,9 @@ kbd_buffer_store_help_event (Lisp_Object frame, Lisp_Object help)
 void
 discard_mouse_events (void)
 {
-  union buffered_input_event *sp;
-  for (sp = kbd_fetch_ptr; sp != kbd_store_ptr; sp++)
+  for (union buffered_input_event *sp = kbd_fetch_ptr;
+       sp != kbd_store_ptr; sp = next_kbd_event (sp))
     {
-      if (sp == kbd_buffer + KBD_BUFFER_SIZE)
-	sp = kbd_buffer;
-
       if (sp->kind == MOUSE_CLICK_EVENT
 	  || sp->kind == WHEEL_EVENT
           || sp->kind == HORIZ_WHEEL_EVENT
@@ -3696,18 +3687,13 @@ discard_mouse_events (void)
 bool
 kbd_buffer_events_waiting (void)
 {
-  union buffered_input_event *sp;
-
-  for (sp = kbd_fetch_ptr;
-       sp != kbd_store_ptr && sp->kind == NO_EVENT;
-       ++sp)
-    {
-      if (sp == kbd_buffer + KBD_BUFFER_SIZE)
-	sp = kbd_buffer;
-    }
-
-  kbd_fetch_ptr = sp;
-  return sp != kbd_store_ptr && sp->kind != NO_EVENT;
+  for (union buffered_input_event *sp = kbd_fetch_ptr;
+       ; sp = next_kbd_event (sp))
+    if (sp == kbd_store_ptr || sp->kind != NO_EVENT)
+      {
+	kbd_fetch_ptr = sp;
+	return sp != kbd_store_ptr && sp->kind != NO_EVENT;
+      }
 }
 
 \f
@@ -3836,11 +3822,7 @@ kbd_buffer_get_event (KBOARD **kbp,
      mouse movement enabled and available.  */
   if (kbd_fetch_ptr != kbd_store_ptr)
     {
-      union buffered_input_event *event;
-
-      event = ((kbd_fetch_ptr < kbd_buffer + KBD_BUFFER_SIZE)
-	       ? kbd_fetch_ptr
-	       : kbd_buffer);
+      union buffered_input_event *event = kbd_fetch_ptr;
 
       *kbp = event_to_kboard (&event->ie);
       if (*kbp == 0)
@@ -3861,7 +3843,7 @@ kbd_buffer_get_event (KBOARD **kbp,
 	     since otherwise swallow_events will see it
 	     and process it again.  */
 	  struct selection_input_event copy = event->sie;
-	  kbd_fetch_ptr = event + 1;
+	  kbd_fetch_ptr = next_kbd_event (event);
 	  input_pending = readable_events (0);
 	  x_handle_selection_event (&copy);
 #else
@@ -3876,7 +3858,7 @@ kbd_buffer_get_event (KBOARD **kbp,
     || defined (HAVE_NS) || defined (USE_GTK)
       case MENU_BAR_ACTIVATE_EVENT:
 	{
-	  kbd_fetch_ptr = event + 1;
+	  kbd_fetch_ptr = next_kbd_event (event);
 	  input_pending = readable_events (0);
 	  if (FRAME_LIVE_P (XFRAME (event->ie.frame_or_window)))
 	    x_activate_menubar (XFRAME (event->ie.frame_or_window));
@@ -3921,7 +3903,7 @@ kbd_buffer_get_event (KBOARD **kbp,
       case SELECT_WINDOW_EVENT:
         {
           obj = make_lispy_event (&event->ie);
-          kbd_fetch_ptr = event + 1;
+          kbd_fetch_ptr = next_kbd_event (event);
         }
         break;
       default:
@@ -3975,7 +3957,7 @@ kbd_buffer_get_event (KBOARD **kbp,
 
 	      /* Wipe out this event, to catch bugs.  */
 	      clear_event (&event->ie);
-	      kbd_fetch_ptr = event + 1;
+	      kbd_fetch_ptr = next_kbd_event (event);
 	    }
 	}
       }
@@ -4042,17 +4024,9 @@ kbd_buffer_get_event (KBOARD **kbp,
 static void
 process_special_events (void)
 {
-  union buffered_input_event *event;
-
-  for (event = kbd_fetch_ptr; event != kbd_store_ptr; ++event)
+  for (union buffered_input_event *event = kbd_fetch_ptr;
+       event != kbd_store_ptr; event = next_kbd_event (event))
     {
-      if (event == kbd_buffer + KBD_BUFFER_SIZE)
-	{
-	  event = kbd_buffer;
-	  if (event == kbd_store_ptr)
-	    break;
-	}
-
       /* If we find a stored X selection request, handle it now.  */
       if (event->kind == SELECTION_REQUEST_EVENT
 	  || event->kind == SELECTION_CLEAR_EVENT)
@@ -4066,28 +4040,21 @@ process_special_events (void)
 	     cyclically.  */
 
 	  struct selection_input_event copy = event->sie;
-	  union buffered_input_event *beg
-	    = (kbd_fetch_ptr == kbd_buffer + KBD_BUFFER_SIZE)
-	    ? kbd_buffer : kbd_fetch_ptr;
+	  int moved_events;
 
-	  if (event > beg)
-	    memmove (beg + 1, beg, (event - beg) * sizeof *beg);
-	  else if (event < beg)
+	  if (event < kbd_fetch_ptr)
 	    {
-	      if (event > kbd_buffer)
-		memmove (kbd_buffer + 1, kbd_buffer,
-			 (event - kbd_buffer) * sizeof *kbd_buffer);
-	      *kbd_buffer = *(kbd_buffer + KBD_BUFFER_SIZE - 1);
-	      if (beg < kbd_buffer + KBD_BUFFER_SIZE - 1)
-		memmove (beg + 1, beg,
-			 (kbd_buffer + KBD_BUFFER_SIZE - 1 - beg) * sizeof *beg);
+	      memmove (kbd_buffer + 1, kbd_buffer,
+		       (event - kbd_buffer) * sizeof *kbd_buffer);
+	      kbd_buffer[0] = kbd_buffer[KBD_BUFFER_SIZE - 1];
+	      moved_events = kbd_buffer + KBD_BUFFER_SIZE - 1 - kbd_fetch_ptr;
 	    }
-
-	  if (kbd_fetch_ptr == kbd_buffer + KBD_BUFFER_SIZE)
-	    kbd_fetch_ptr = kbd_buffer + 1;
 	  else
-	    kbd_fetch_ptr++;
+	    moved_events = event - kbd_fetch_ptr;
 
+	  memmove (kbd_fetch_ptr + 1, kbd_fetch_ptr,
+		   moved_events * sizeof *kbd_fetch_ptr);
+	  kbd_fetch_ptr = next_kbd_event (kbd_fetch_ptr);
 	  input_pending = readable_events (0);
 	  x_handle_selection_event (&copy);
 #else
@@ -10261,11 +10228,10 @@ stuff_buffered_input (Lisp_Object stuffstring)
 
      rms: we should stuff everything back into the kboard
      it came from.  */
-  for (; kbd_fetch_ptr != kbd_store_ptr; kbd_fetch_ptr++)
+  for (; kbd_fetch_ptr != kbd_store_ptr;
+       kbd_fetch_ptr = next_kbd_event (kbd_fetch_ptr))
     {
 
-      if (kbd_fetch_ptr == kbd_buffer + KBD_BUFFER_SIZE)
-	kbd_fetch_ptr = kbd_buffer;
       if (kbd_fetch_ptr->kind == ASCII_KEYSTROKE_EVENT)
 	stuff_char (kbd_fetch_ptr->ie.code);
 
@@ -12003,21 +11969,18 @@ mark_kboards (void)
       mark_object (KVAR (kb, echo_string));
       mark_object (KVAR (kb, echo_prompt));
     }
-  {
-    union buffered_input_event *event;
-    for (event = kbd_fetch_ptr; event != kbd_store_ptr; event++)
-      {
-	if (event == kbd_buffer + KBD_BUFFER_SIZE)
-	  event = kbd_buffer;
-	/* These two special event types has no Lisp_Objects to mark.  */
-	if (event->kind != SELECTION_REQUEST_EVENT
-	    && event->kind != SELECTION_CLEAR_EVENT)
-	  {
-	    mark_object (event->ie.x);
-	    mark_object (event->ie.y);
-	    mark_object (event->ie.frame_or_window);
-	    mark_object (event->ie.arg);
-	  }
-      }
-  }
+
+  for (union buffered_input_event *event = kbd_fetch_ptr;
+       event != kbd_store_ptr; event = next_kbd_event (event))
+    {
+      /* These two special event types have no Lisp_Objects to mark.  */
+      if (event->kind != SELECTION_REQUEST_EVENT
+	  && event->kind != SELECTION_CLEAR_EVENT)
+	{
+	  mark_object (event->ie.x);
+	  mark_object (event->ie.y);
+	  mark_object (event->ie.frame_or_window);
+	  mark_object (event->ie.arg);
+	}
+    }
 }
-- 
2.19.2


[-- Attachment #3: 0001-Fix-infloop-in-GC-mark_kboards-emacs-26.patch --]
[-- Type: text/x-patch, Size: 972 bytes --]

From 7df217da0c97702988aadb6b231067a8bcda1703 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 30 Nov 2018 14:14:18 -0800
Subject: [PATCH] Fix infloop in GC mark_kboards

Do not merge to master, as I have a more systematic fix there.
* src/keyboard.c (mark_kboards): Fix infloop (Bug#33547).
---
 src/keyboard.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index 0d56ea3f7a..dccc6b7f12 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -12011,7 +12011,12 @@ mark_kboards (void)
     for (event = kbd_fetch_ptr; event != kbd_store_ptr; event++)
       {
 	if (event == kbd_buffer + KBD_BUFFER_SIZE)
-	  event = kbd_buffer;
+	  {
+	    event = kbd_buffer;
+	    if (event == kbd_store_ptr)
+	      break;
+	  }
+
 	/* These two special event types has no Lisp_Objects to mark.  */
 	if (event->kind != SELECTION_REQUEST_EVENT
 	    && event->kind != SELECTION_CLEAR_EVENT)
-- 
2.19.2