Re: [Emacs-diffs] master 5b5f441: read_key_sequence: correct the handling of raw_keybuf in recursive calls

[Alan Mackenzie <acm@muc.de>]

Hello, Eli.

On Thu, Nov 23, 2017 at 13:46:44 -0500, Stefan Monnier wrote:
> >> Hmm... that might work, but I still can't imagine what comment I could
> >> write next to the code to explain/prove that it's safe.
> > It's as safe as the original, since the only buffer space used is the
> > original raw_keybuf, indexed by raw_keybuf_count.  The only change is to
> > initialise raw_keybuf_count in different places, to allow the recursive
> > call to read_key_sequence not to overwrite the current buffer contents.

> I had misunderstood.  Yes, that does look safe.  No idea if it's correct
> enough in practice, but it looks much better than what we have.
> Thanks.

What do you say?  I would like to revert the previous commit that fixed
this and install the one below.

I don't know if you've been following this conversation, but Stefan was
worried (and I agree with him) that the existing fix is dangerous,
because read_key_sequence sets global variables to point into a buffer
on the stack.  If there's an unfortunate longjmp or unwind_protect, or
something similar, the global variables might be left pointing at stale
stack space.  Neither of us can say for sure whether the actual code is
safe or dangerous.

My newer approach is to use only the static raw_keybuf as the buffer,
but to be more discerning about where raw_keybuf_count gets initialised
to zero.  Instead of doing this at the start of read_key_sequence, I
would do it in:
    command_loop_1
    read_key_sequence_vs
    Fx_popup_menu
.  Fx_popup_menu will be split into the function itself (which
initialises raw_keybuf_count) and x_popup_menu_1, this latter function
not touching raw_keybuf_count, but otherwise containing the bulk of what
was Fx_popup_menu.

Stefan is happy enough about this new patch.  What do you think?

For some reason, I get lots of <mouse-movement>s in the listed
"translation" strings, which I don't get with the current fix.  This may
have to be fixed somehow.

Here's the patch (generated by $ git diff 5b5f441ff^ \
src/{keyboard,menu}.[ch]).



diff --git a/src/keyboard.c b/src/keyboard.c
index 6b7a6bfa74..a751861081 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -43,6 +43,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include "systime.h"
 #include "atimer.h"
 #include "process.h"
+#include "menu.h"
 #include <errno.h>
 
 #ifdef HAVE_PTHREAD
@@ -1365,6 +1366,7 @@ command_loop_1 (void)
       Vthis_command_keys_shift_translated = Qnil;
 
       /* Read next key sequence; i gets its length.  */
+      raw_keybuf_count = 0;
       i = read_key_sequence (keybuf, ARRAYELTS (keybuf),
 			     Qnil, 0, 1, 1, 0);
 
@@ -8453,7 +8455,8 @@ read_char_x_menu_prompt (Lisp_Object map,
       /* Display the menu and get the selection.  */
       Lisp_Object value;
 
-      value = Fx_popup_menu (prev_event, get_keymap (map, 0, 1));
+      /* value = Fx_popup_menu (prev_event, get_keymap (map, 0, 1)); */
+      value = x_popup_menu_1 (prev_event, get_keymap (map, 0, 1));
       if (CONSP (value))
 	{
 	  Lisp_Object tem;
@@ -8863,6 +8866,11 @@ test_undefined (Lisp_Object binding)
 	      && EQ (Fcommand_remapping (binding, Qnil, Qnil), Qundefined)));
 }
 
+void init_raw_keybuf_count (void)
+{
+  raw_keybuf_count = 0;
+}
+
 /* Read a sequence of keys that ends with a non prefix character,
    storing it in KEYBUF, a buffer of size BUFSIZE.
    Prompt with PROMPT.
@@ -8973,7 +8981,11 @@ 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 is now initialized in (most of) the callers of
+     read_key_sequence.  This is so that in a recursive call (for
+     mouse menus) a spurious initialization doesn't erase the contents
+     of raw_keybuf created by the outer call.  */
+  /* raw_keybuf_count = 0; */
 
   last_nonmenu_event = Qnil;
 
@@ -9346,7 +9358,8 @@ 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);
+		      GROW_RAW_KEYBUF;
+                      ASET (raw_keybuf, raw_keybuf_count, key);
 		      raw_keybuf_count++;
 		      keybuf[t] = key;
 		      mock_input = t + 1;
@@ -9840,6 +9853,7 @@ read_key_sequence_vs (Lisp_Object prompt, Lisp_Object continue_echo,
     cancel_hourglass ();
 #endif
 
+  raw_keybuf_count = 0;
   i = read_key_sequence (keybuf, ARRAYELTS (keybuf),
 			 prompt, ! NILP (dont_downcase_last),
 			 ! NILP (can_return_switch_frame), 0, 0);
diff --git a/src/keyboard.h b/src/keyboard.h
index 662d8e4a4f..c232e778e2 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -438,6 +438,7 @@ extern unsigned int timers_run;
 extern bool menu_separator_name_p (const char *);
 extern bool parse_menu_item (Lisp_Object, int);
 
+extern void init_raw_keybuf_count (void);
 extern KBOARD *allocate_kboard (Lisp_Object);
 extern void delete_kboard (KBOARD *);
 extern void not_single_kboard_state (KBOARD *);
diff --git a/src/menu.c b/src/menu.c
index d569b4b29b..b40c2c04ce 100644
--- a/src/menu.c
+++ b/src/menu.c
@@ -1112,51 +1112,8 @@ into menu items.  */)
   return Qnil;
 }
 
-
-DEFUN ("x-popup-menu", Fx_popup_menu, Sx_popup_menu, 2, 2, 0,
-       doc: /* Pop up a deck-of-cards menu and return user's selection.
-POSITION is a position specification.  This is either a mouse button event
-or a list ((XOFFSET YOFFSET) WINDOW)
-where XOFFSET and YOFFSET are positions in pixels from the top left
-corner of WINDOW.  (WINDOW may be a window or a frame object.)
-This controls the position of the top left of the menu as a whole.
-If POSITION is t, it means to use the current mouse position.
-
-MENU is a specifier for a menu.  For the simplest case, MENU is a keymap.
-The menu items come from key bindings that have a menu string as well as
-a definition; actually, the "definition" in such a key binding looks like
-\(STRING . REAL-DEFINITION).  To give the menu a title, put a string into
-the keymap as a top-level element.
-
-If REAL-DEFINITION is nil, that puts a nonselectable string in the menu.
-Otherwise, REAL-DEFINITION should be a valid key binding definition.
-
-You can also use a list of keymaps as MENU.
-  Then each keymap makes a separate pane.
-
-When MENU is a keymap or a list of keymaps, the return value is the
-list of events corresponding to the user's choice. Note that
-`x-popup-menu' does not actually execute the command bound to that
-sequence of events.
-
-Alternatively, you can specify a menu of multiple panes
-  with a list of the form (TITLE PANE1 PANE2...),
-where each pane is a list of form (TITLE ITEM1 ITEM2...).
-Each ITEM is normally a cons cell (STRING . VALUE);
-but a string can appear as an item--that makes a nonselectable line
-in the menu.
-With this form of menu, the return value is VALUE from the chosen item.
-
-If POSITION is nil, don't display the menu at all, just precalculate the
-cached information about equivalent key sequences.
-
-If the user gets rid of the menu without making a valid choice, for
-instance by clicking the mouse away from a valid choice or by typing
-keyboard input, then this normally results in a quit and
-`x-popup-menu' does not return.  But if POSITION is a mouse button
-event (indicating that the user invoked the menu with the mouse) then
-no quit occurs and `x-popup-menu' returns nil.  */)
-  (Lisp_Object position, Lisp_Object menu)
+Lisp_Object
+x_popup_menu_1 (Lisp_Object position, Lisp_Object menu)
 {
   Lisp_Object keymap, tem, tem2;
   int xpos = 0, ypos = 0;
@@ -1443,6 +1400,55 @@ no quit occurs and `x-popup-menu' returns nil.  */)
   return selection;
 }
 
+DEFUN ("x-popup-menu", Fx_popup_menu, Sx_popup_menu, 2, 2, 0,
+       doc: /* Pop up a deck-of-cards menu and return user's selection.
+POSITION is a position specification.  This is either a mouse button event
+or a list ((XOFFSET YOFFSET) WINDOW)
+where XOFFSET and YOFFSET are positions in pixels from the top left
+corner of WINDOW.  (WINDOW may be a window or a frame object.)
+This controls the position of the top left of the menu as a whole.
+If POSITION is t, it means to use the current mouse position.
+
+MENU is a specifier for a menu.  For the simplest case, MENU is a keymap.
+The menu items come from key bindings that have a menu string as well as
+a definition; actually, the "definition" in such a key binding looks like
+\(STRING . REAL-DEFINITION).  To give the menu a title, put a string into
+the keymap as a top-level element.
+
+If REAL-DEFINITION is nil, that puts a nonselectable string in the menu.
+Otherwise, REAL-DEFINITION should be a valid key binding definition.
+
+You can also use a list of keymaps as MENU.
+  Then each keymap makes a separate pane.
+
+When MENU is a keymap or a list of keymaps, the return value is the
+list of events corresponding to the user's choice. Note that
+`x-popup-menu' does not actually execute the command bound to that
+sequence of events.
+
+Alternatively, you can specify a menu of multiple panes
+  with a list of the form (TITLE PANE1 PANE2...),
+where each pane is a list of form (TITLE ITEM1 ITEM2...).
+Each ITEM is normally a cons cell (STRING . VALUE);
+but a string can appear as an item--that makes a nonselectable line
+in the menu.
+With this form of menu, the return value is VALUE from the chosen item.
+
+If POSITION is nil, don't display the menu at all, just precalculate the
+cached information about equivalent key sequences.
+
+If the user gets rid of the menu without making a valid choice, for
+instance by clicking the mouse away from a valid choice or by typing
+keyboard input, then this normally results in a quit and
+`x-popup-menu' does not return.  But if POSITION is a mouse button
+event (indicating that the user invoked the menu with the mouse) then
+no quit occurs and `x-popup-menu' returns nil.  */)
+  (Lisp_Object position, Lisp_Object menu)
+{
+  init_raw_keybuf_count ();
+  return x_popup_menu_1 (position, menu);
+}
+
 /* If F's terminal is not capable of displaying a popup dialog,
    emulate it with a menu.  */
 
diff --git a/src/menu.h b/src/menu.h
index 1469cc87d9..3335616338 100644
--- a/src/menu.h
+++ b/src/menu.h
@@ -60,4 +60,5 @@ extern Lisp_Object ns_menu_show (struct frame *, int, int, int,
 extern Lisp_Object tty_menu_show (struct frame *, int, int, int,
 				  Lisp_Object, const char **);
 extern ptrdiff_t menu_item_width (const unsigned char *);
+extern Lisp_Object x_popup_menu_1 (Lisp_Object position, Lisp_Object menu);
 #endif /* MENU_H */


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).