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

[Alan Mackenzie]

Hello, Stefan.

On Thu, Nov 23, 2017 at 09:35:03 -0500, Stefan Monnier wrote:

[ .... ]

> > How about this idea?  Split Fx_popup_menu into two pieces, Fx_popup_menu
> > which just initialises the global raw key buffer then calls
> > sub_x_popup_menu.  read_char_x_menu_prompt (see above list) would then
> > call sub_x_popup_menu to avoid emptying the global buffer.

> > Other than that, the global raw key buffer would be initialised in
> > command_loop_1 and (possibly) read_key_sequence_vs.

> > This way, everything which appends to the raw key buffer could just use
> > the global variables, without any dangerous shenanigans on the stack.

> 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 think a record_unwind_protect is the easiest way to make it safer.

Please try the patch below (which still needs some commenting added).

> Yet, with the new concurrency feature, the unwind might reset the
> global var to a pointer into a stack area on another thread, which may
> have gotten stale in the mean time.

If we've got two tasks simultaneously accessing that global buffer,
we're in deep trouble anyway.  Obviously some sort of lock would need to
be applied to this and other global things.

>         Stefan

The following patch (which applies to master without 5b5f441) implements
the above idea:



diff --git a/src/keyboard.c b/src/keyboard.c
index 57757cf211..c32e8feb97 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1365,6 +1365,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);
 
@@ -8860,6 +8861,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.
@@ -8971,7 +8977,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;
 
@@ -9837,6 +9843,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..2715f9ce82 100644
--- a/src/menu.c
+++ b/src/menu.c
@@ -1475,30 +1475,9 @@ emulate_dialog_with_menu (struct frame *f, Lisp_Object 
contents)
   return Fx_popup_menu (newpos, list2 (prompt, contents));
 }
 
-DEFUN ("x-popup-dialog", Fx_popup_dialog, Sx_popup_dialog, 2, 3, 0,
-       doc: /* Pop up a dialog box and return user's selection.
-POSITION specifies which frame to use.
-This is normally a mouse button event or a window or frame.
-If POSITION is t, it means to use the frame the mouse is on.
-The dialog box appears in the middle of the specified frame.
-
-CONTENTS specifies the alternatives to display in the dialog box.
-It is a list of the form (DIALOG ITEM1 ITEM2...).
-Each ITEM is a cons cell (STRING . VALUE).
-The return value is VALUE from the chosen item.
-
-An ITEM may also be just a string--that makes a nonselectable item.
-An ITEM may also be nil--that means to put all preceding items
-on the left of the dialog box and all following items on the right.
-\(By default, approximately half appear on each side.)
-
-If HEADER is non-nil, the frame title for the box is "Information",
-otherwise it is "Question".
-
-If the user gets rid of the dialog box without making a valid choice,
-for instance using the window manager, then this produces a quit and
-`x-popup-dialog' does not return.  */)
-  (Lisp_Object position, Lisp_Object contents, Lisp_Object header)
+static Lisp_Object
+x_popup_dialog_1 (Lisp_Object position, Lisp_Object contents,
+                  Lisp_Object header)
 {
   struct frame *f = NULL;
   Lisp_Object window;
@@ -1571,6 +1550,35 @@ for instance using the window manager, then this 
produces a quit and
   return emulate_dialog_with_menu (f, contents);
 }
 
+DEFUN ("x-popup-dialog", Fx_popup_dialog, Sx_popup_dialog, 2, 3, 0,
+       doc: /* Pop up a dialog box and return user's selection.
+POSITION specifies which frame to use.
+This is normally a mouse button event or a window or frame.
+If POSITION is t, it means to use the frame the mouse is on.
+The dialog box appears in the middle of the specified frame.
+
+CONTENTS specifies the alternatives to display in the dialog box.
+It is a list of the form (DIALOG ITEM1 ITEM2...).
+Each ITEM is a cons cell (STRING . VALUE).
+The return value is VALUE from the chosen item.
+
+An ITEM may also be just a string--that makes a nonselectable item.
+An ITEM may also be nil--that means to put all preceding items
+on the left of the dialog box and all following items on the right.
+\(By default, approximately half appear on each side.)
+
+If HEADER is non-nil, the frame title for the box is "Information",
+otherwise it is "Question".
+
+If the user gets rid of the dialog box without making a valid choice,
+for instance using the window manager, then this produces a quit and
+`x-popup-dialog' does not return.  */)
+  (Lisp_Object position, Lisp_Object contents, Lisp_Object header)
+{
+  init_raw_keybuf_count ();
+  return x_popup_dialog_1 (position, contents, header);
+}
+
 void
 syms_of_menu (void)
 {


-- 
Alan Mackenzie (Nuremberg, Germany).