bug#29349: [Patch] Bug 29349: read_key_sequence is only partially recursive. This is a bug.

[Alan Mackenzie]

Hello, Emacs.

On Sun, Nov 19, 2017 at 12:34:56 +0000, Alan Mackenzie wrote:
> On Sat, Nov 18, 2017 at 09:38:43 +0000, Alan Mackenzie wrote:
> > In branch emacs-26.

> > I came across this bug whilst working on bug #29272 ("C-h k C-mouse-3"
> > followed by menu selection asks for more keys).

> > In a Linux tty using the GPM mouse package, doing read_key_sequence (the
> > C function in keyboard.c), when a menu action is activated,
> > read_key_sequence calls itself recursively to handle all the mouse
> > manipulation.

> > Unfortunately, the variable raw_keybuf_count is initialised to 0 in
> > r_k_s.  This includes in the recursive call.  This variable indexes the
> > global buffer raw_keybuf, which accumulates the raw events which make up
> > the key sequence.

> > The result of this is that the events in the recursive call overwrite
> > the stored events of the outer r_k_s call, leaving a mess.

> > r_k_s is static in keyboard.c and is called from three functions:
> > command_loop_1, read_menu_command (the one that gives the trouble), and
> > read_key_sequence_vs.

> > So I propose as a solution that raw_keybuf_count be initialised to zero
> > in two of these three functions, but not in read_menu_command (and no
> > longer in read_key_sequence).  I've tried this, and it seems to work.
> > It has the disadvantage of being ugly, and it makes read_menu_command
> > only callable as a subfunction of r_k_s.

> > Has anybody any thoughts on this?

> Here is how I propose to solve this:

> (i) In keyboard.c, the static variables raw_keybuf and raw_keybuf_count
> will become pointers.  They will initially point to a static buffer and
> a static integer.  For safety's sake, they will be reinitialised to
> those static variables in command_loop_1(), just before the invocation
> of read_key_sequence().

> (ii) read_key_sequence() will get a Lisp_Object buffer and a count
> variable as local variables.  Around the call to read_char(),
> raw_keybuf{,_count} will be set to point to these locals, so that should
> read_char encounter a menu, its events will be stored in the local copy
> of the Lisp_Object buffer.

> (iii) On return from read_char, if any events are in the local buffer,
> they will be appended to the buffer in the enclosing scope.  The global
> pointers raw_keybuf{,_count} will be restored to their previous values.

> In short, raw_keybuf and raw_keybuf_count will be "bound" to local
> variables around the call to read_char().

> Comments?

Here's a patch which implements the above:



diff --git a/src/keyboard.c b/src/keyboard.c
index 57757cf211..ae687aad89 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -121,12 +121,17 @@ ptrdiff_t this_command_key_count;
 
 /* This vector is used as a buffer to record the events that were actually read
    by read_key_sequence.  */
-static Lisp_Object raw_keybuf;
-static int raw_keybuf_count;
-
-#define GROW_RAW_KEYBUF                                                        
\
- if (raw_keybuf_count == ASIZE (raw_keybuf))                           \
-   raw_keybuf = larger_vector (raw_keybuf, 1, -1)
+static Lisp_Object raw_keybuf_buffer;
+static int raw_keybuf_count_buffer;
+static Lisp_Object *raw_keybuf = &raw_keybuf_buffer;
+static int *raw_keybuf_count = &raw_keybuf_count_buffer;
+
+#define GROW_RAW_KEYBUF(inc)                                            \
+  if (*raw_keybuf_count > ASIZE (*raw_keybuf) - (inc))                  \
+    *raw_keybuf =                                                       \
+      larger_vector (*raw_keybuf,                                       \
+                     (inc) + *raw_keybuf_count - ASIZE (*raw_keybuf),   \
+                     -1)
 
 /* Number of elements of this_command_keys
    that precede this key sequence.  */
@@ -1365,6 +1370,8 @@ command_loop_1 (void)
       Vthis_command_keys_shift_translated = Qnil;
 
       /* Read next key sequence; i gets its length.  */
+      raw_keybuf_count = &raw_keybuf_count_buffer; /* For safety */
+      raw_keybuf = &raw_keybuf_buffer;             /* Ditto */
       i = read_key_sequence (keybuf, ARRAYELTS (keybuf),
                             Qnil, 0, 1, 1, 0);
 
@@ -8910,6 +8917,11 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, 
Lisp_Object prompt,
   /* How many keys there are in the current key sequence.  */
   int t;
 
+  int *outer_raw_keybuf_count;
+  Lisp_Object *outer_raw_keybuf;
+  int inner_raw_keybuf_count_buffer;
+  Lisp_Object inner_raw_keybuf_buffer = Fmake_vector (make_number (30), Qnil);
+
   /* The length of the echo buffer when we started reading, and
      the length of this_command_keys when we started reading.  */
   ptrdiff_t echo_start UNINIT;
@@ -8971,7 +8983,7 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, 
Lisp_Object prompt,
   /* List of events for which a fake prefix key has been generated.  */
   Lisp_Object fake_prefixed_keys = Qnil;
 
-  raw_keybuf_count = 0;
+  *raw_keybuf_count = 0;
 
   last_nonmenu_event = Qnil;
 
@@ -9142,11 +9154,23 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, 
Lisp_Object prompt,
          {
            KBOARD *interrupted_kboard = current_kboard;
            struct frame *interrupted_frame = SELECTED_FRAME ();
+            int i;
+            outer_raw_keybuf_count = raw_keybuf_count;
+            outer_raw_keybuf = raw_keybuf;
+            inner_raw_keybuf_count_buffer = 0;
+            raw_keybuf_count = &inner_raw_keybuf_count_buffer;
+            raw_keybuf = &inner_raw_keybuf_buffer;
            /* Calling read_char with COMMANDFLAG = -2 avoids
               redisplay in read_char and its subroutines.  */
            key = read_char (prevent_redisplay ? -2 : NILP (prompt),
                             current_binding, last_nonmenu_event,
                              &used_mouse_menu, NULL);
+            raw_keybuf_count = outer_raw_keybuf_count;
+            raw_keybuf = outer_raw_keybuf;
+            GROW_RAW_KEYBUF (inner_raw_keybuf_count_buffer);
+            for (i = 0; i < inner_raw_keybuf_count_buffer; i++)
+              ASET (*raw_keybuf, (*raw_keybuf_count)++,
+                    AREF (inner_raw_keybuf_buffer, i));
            if ((INTEGERP (key) && XINT (key) == -2) /* wrong_kboard_jmpbuf */
                /* When switching to a new tty (with a new keyboard),
                   read_char returns the new buffer, rather than -2
@@ -9254,9 +9278,9 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, 
Lisp_Object prompt,
              && XINT (key) == quit_char
              && current_buffer != starting_buffer)
            {
-             GROW_RAW_KEYBUF;
-             ASET (raw_keybuf, raw_keybuf_count, key);
-             raw_keybuf_count++;
+             GROW_RAW_KEYBUF (1);
+             ASET (*raw_keybuf, *raw_keybuf_count, key);
+             (*raw_keybuf_count)++;
              keybuf[t++] = key;
              mock_input = t;
              Vquit_flag = Qnil;
@@ -9295,9 +9319,9 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, 
Lisp_Object prompt,
              current_binding = active_maps (first_event);
            }
 
-         GROW_RAW_KEYBUF;
-         ASET (raw_keybuf, raw_keybuf_count, key);
-         raw_keybuf_count++;
+         GROW_RAW_KEYBUF (1);
+         ASET (*raw_keybuf, *raw_keybuf_count, key);
+         (*raw_keybuf_count)++;
        }
 
       /* Clicks in non-text areas get prefixed by the symbol
@@ -9343,8 +9367,9 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, 
Lisp_Object prompt,
                      && BUFFERP (XWINDOW (window)->contents)
                      && XBUFFER (XWINDOW (window)->contents) != current_buffer)
                    {
-                     ASET (raw_keybuf, raw_keybuf_count, key);
-                     raw_keybuf_count++;
+                     GROW_RAW_KEYBUF (1);
+                      ASET (*raw_keybuf, *raw_keybuf_count, key);
+                     (*raw_keybuf_count)++;
                      keybuf[t] = key;
                      mock_input = t + 1;
 
@@ -10117,7 +10142,7 @@ shows the events before all translations (except for 
input methods).
 The value is always a vector.  */)
   (void)
 {
-  return Fvector (raw_keybuf_count, XVECTOR (raw_keybuf)->contents);
+  return Fvector (*raw_keybuf_count, XVECTOR (*raw_keybuf)->contents);
 }
 
 DEFUN ("clear-this-command-keys", Fclear_this_command_keys,
@@ -11262,8 +11287,8 @@ syms_of_keyboard (void)
   this_command_keys = Fmake_vector (make_number (40), Qnil);
   staticpro (&this_command_keys);
 
-  raw_keybuf = Fmake_vector (make_number (30), Qnil);
-  staticpro (&raw_keybuf);
+  raw_keybuf_buffer = Fmake_vector (make_number (30), Qnil);
+  staticpro (raw_keybuf);
 
   DEFSYM (Qcommand_execute, "command-execute");
   DEFSYM (Qinternal_echo_keystrokes_prefix, "internal-echo-keystrokes-prefix");


-- 
Alan Mackenzie (Nuremberg, Germany).