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

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

[Stefan Monnier]

> +            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;

Looks good, thanks (tho I think I would have bundled the buf and the
count into a struct, so it's clear that they're switched together).

>  	    /* 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;

But here I worry: what if `read_char` exits non-locally because of
a signal or a throw?


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Mon, Nov 20, 2017 at 13:41:54 -0500, Stefan Monnier wrote:
> > +            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;

> Looks good, thanks (tho I think I would have bundled the buf and the
> count into a struct, so it's clear that they're switched together).

> >  	    /* 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;

> But here I worry: what if `read_char` exits non-locally because of
> a signal or a throw?

raw_keybuf{,_count} should then be re-initialised in command_loop_1 to
the static buffer variables, just before the call to read_key_sequence.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier]

>> >  	    /* 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;
>> But here I worry: what if `read_char` exits non-locally because of
>> a signal or a throw?
> raw_keybuf{,_count} should then be re-initialised in command_loop_1 to
> the static buffer variables, just before the call to read_key_sequence.

But this `read_char` is within read_key_sequence: this read_key_sequence
may have been called from anywhere, so after we exit it (non-locally),
we may end up running arbitrary Elisp code before we return to
command_loop_1, can't we?  And during this time we'll have raw_keybuf
pointing to an out-of-date stack location, which seems
eminently dangerous.

I think we need to setup an unwind protection of some kind.


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Mon, Nov 20, 2017 at 15:09:45 -0500, Stefan Monnier wrote:
> >> >  	    /* 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;
> >> But here I worry: what if `read_char` exits non-locally because of
> >> a signal or a throw?
> > raw_keybuf{,_count} should then be re-initialised in command_loop_1 to
> > the static buffer variables, just before the call to read_key_sequence.

> But this `read_char` is within read_key_sequence: this read_key_sequence
> may have been called from anywhere, ....

read_key_sequence is static in keyboard.c, and is called from precisely
three places: command_loop_1, read_key_sequence_vs, and
read_menu_command.

> .... so after we exit it (non-locally), we may end up running
> arbitrary Elisp code before we return to command_loop_1, can't we?

Is it possible to exit non-locally from read_char (or one of its called
subroutines)?  I don't think read_char calls any lisp, even when it's
dealing with a menu.

> And during this time we'll have raw_keybuf pointing to an out-of-date
> stack location, which seems eminently dangerous.

You have a point, here.  Perhaps it would be better to get storage from
the Emacs heap rather than using the stack.

> I think we need to setup an unwind protection of some kind.

You might be right, but I'm not convinced yet.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier]

>> But this `read_char` is within read_key_sequence: this read_key_sequence
>> may have been called from anywhere, ....
> read_key_sequence is static in keyboard.c, and is called from precisely
> three places: command_loop_1, read_key_sequence_vs, and
> read_menu_command.

Right: the middle one corresponds to `read-key-sequence` which can be
called from "anywhere" (i.e. Elisp).

>> .... so after we exit it (non-locally), we may end up running
>> arbitrary Elisp code before we return to command_loop_1, can't we?
> Is it possible to exit non-locally from read_char (or one of its called
> subroutines)?

`read_char` will run timers and process filters.
See for example `read-key` which calls `read-key-sequence` with a timer
that makes it exit by throwing `read-key` as soon as one key is detected.

> You have a point, here.  Perhaps it would be better to get storage from
> the Emacs heap rather than using the stack.

I like using the stack, here, actually.

Maybe another option is to make raw_keybuf local to read_key_sequence,
and to *copy* it into the global raw_keybuf_buffer just before exiting.


        Stefan


PS: Of course, even better would be to provide another way to get what
`this-single-command-raw-keys` returns, so we don't need to use a global
variable for it.  E.g. have `read-key-sequence` return both the key
sequence and the raw key sequence.  But we'd still have to support
`this-single-command-raw-keys` for the foreseeable future anyway.

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

[Alan Mackenzie]

Hello, Stefan.

On Mon, Nov 20, 2017 at 16:43:52 -0500, Stefan Monnier wrote:
> >> But this `read_char` is within read_key_sequence: this read_key_sequence
> >> may have been called from anywhere, ....
> > read_key_sequence is static in keyboard.c, and is called from precisely
> > three places: command_loop_1, read_key_sequence_vs, and
> > read_menu_command.

> Right: the middle one corresponds to `read-key-sequence` which can be
> called from "anywhere" (i.e. Elisp).

That is surely not the problem.  The problem would be if something
aborted a read_key_sequence, and somehow started another one without
going via command_loop_1.

In the current formulation read_key_sequence_vs could have set the
pertinent pointer variables to the static ones as part of its
initialisation.  I didn't do that because it seemed unneeded.  But it
would be harmless, since there surely cannot be a recursive call to
read_key_sequence_vs.

> >> .... so after we exit it (non-locally), we may end up running
> >> arbitrary Elisp code before we return to command_loop_1, can't we?
> > Is it possible to exit non-locally from read_char (or one of its called
> > subroutines)?

> `read_char` will run timers and process filters.  See for example
> `read-key` which calls `read-key-sequence` with a timer that makes it
> exit by throwing `read-key` as soon as one key is detected.

I've had a closer look.  read_char does indeed call out to lisp.  I
can't yet see any place where a non-local exit could cause a re-entry
into read_key_sequence without re-initialising the static variables, but
it might well be there.

> > You have a point, here.  Perhaps it would be better to get storage from
> > the Emacs heap rather than using the stack.

> I like using the stack, here, actually.

I thought about that.  It would mean passing this buffer as a parameter
through lots of functions to get from where it's defined to where it
gets filled.  Here "lots" means a stack of, perhaps, 7 or 8 function
calls.  It would certainly be safer, though.

> Maybe another option is to make raw_keybuf local to read_key_sequence,
> and to *copy* it into the global raw_keybuf_buffer just before exiting.

That's quite close to what I've done.  The difficulty is in getting a
pointer to that local buffer for use by the menu handling code.

>         Stefan


> PS: Of course, even better would be to provide another way to get what
> `this-single-command-raw-keys` returns, so we don't need to use a global
> variable for it.  E.g. have `read-key-sequence` return both the key
> sequence and the raw key sequence.  But we'd still have to support
> `this-single-command-raw-keys` for the foreseeable future anyway.

I don't think that idea would fly.  Most uses of `read-key-sequence'
aren't going to be interested in the raw events.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier]

>> Right: the middle one corresponds to `read-key-sequence` which can be
>> called from "anywhere" (i.e. Elisp).
> That is surely not the problem.  The problem would be if something
> aborted a read_key_sequence, and somehow started another one without
> going via command_loop_1.

I was thinking more of:

  (progn (catch 'foo (read-key-sequence ...))
    ...
    (this-single-command-raw-keys))

where a timer throws `foo` during read-key-sequence.
Since this-single-command-raw-keys uses the raw_keybuf, it ends up
dereferencing the "stale" pointer to the var that was local in
read_key_sequence.

>> > You have a point, here.  Perhaps it would be better to get storage from
>> > the Emacs heap rather than using the stack.
>> I like using the stack, here, actually.
> I thought about that.

No, I mean, I like the fact that your code uses the stack.

>> Maybe another option is to make raw_keybuf local to read_key_sequence,
>> and to *copy* it into the global raw_keybuf_buffer just before exiting.
> That's quite close to what I've done.  The difficulty is in getting a
> pointer to that local buffer for use by the menu handling code.

Hmm... could you show me that menu handling code you're referring to?
The only user I can see of raw_keybuf (outside of local uses within
read_key_sequence) is this-single-command-raw-keys, so maybe I'm
missing something.

>> PS: Of course, even better would be to provide another way to get what
>> `this-single-command-raw-keys` returns, so we don't need to use a global
>> variable for it.  E.g. have `read-key-sequence` return both the key
>> sequence and the raw key sequence.  But we'd still have to support
>> `this-single-command-raw-keys` for the foreseeable future anyway.
> I don't think that idea would fly.  Most uses of `read-key-sequence'
> aren't going to be interested in the raw events.

[ This is really a side-discussion about a better solution for the
  long-term, but we need to solve the current problem regardless.  ]
I was thinking of adding a new function read-raw-key-sequence which
returns both the normal and the raw key sequences, with the hope that
all users of this-single-command-raw-keys could eventually be changed to
make use of that new function instead.


        Stefan

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

[Alan Mackenzie]

Hello again, Stefan.

On Wed, Nov 22, 2017 at 15:29:00 -0500, Stefan Monnier wrote:
> >> Right: the middle one corresponds to `read-key-sequence` which can be
> >> called from "anywhere" (i.e. Elisp).
> > That is surely not the problem.  The problem would be if something
> > aborted a read_key_sequence, and somehow started another one without
> > going via command_loop_1.

> I was thinking more of:

>   (progn (catch 'foo (read-key-sequence ...))
>     ...
>     (this-single-command-raw-keys))

> where a timer throws `foo` during read-key-sequence.
> Since this-single-command-raw-keys uses the raw_keybuf, it ends up
> dereferencing the "stale" pointer to the var that was local in
> read_key_sequence.

I'm still not sure there's a problem.  But I think the effort involved
in showing there's no problem would exceed that needed to solve it, so
we might as well just assume there's a problem.  Which I think is what
you're doing anyway.  :-)

> >> > You have a point, here.  Perhaps it would be better to get storage from
> >> > the Emacs heap rather than using the stack.
> >> I like using the stack, here, actually.
> > I thought about that.

> No, I mean, I like the fact that your code uses the stack.

> >> Maybe another option is to make raw_keybuf local to read_key_sequence,
> >> and to *copy* it into the global raw_keybuf_buffer just before exiting.
> > That's quite close to what I've done.  The difficulty is in getting a
> > pointer to that local buffer for use by the menu handling code.

> Hmm... could you show me that menu handling code you're referring to?
> The only user I can see of raw_keybuf (outside of local uses within
> read_key_sequence) is this-single-command-raw-keys, so maybe I'm
> missing something.

read_menu_command calls read_key_sequence.  It is called by this stack
of functions:
- read_key_sequence is called by
- read_menu_command .. ...... ..
- read_menu_input
- tty_menu_activate
- tty_menu_show, which is one value of terminal->menu_show_hook.  This
  is called in its turn by:
- Fx_popup_menu, which besides being called from many places in lisp is
  also called from
- read_char_x_menu_prompt
- read_char
- read_key_sequence

This is the recursion which makes it difficult just to use a global
buffer for the raw key events.

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.

> >> PS: Of course, even better would be to provide another way to get what
> >> `this-single-command-raw-keys` returns, so we don't need to use a global
> >> variable for it.  E.g. have `read-key-sequence` return both the key
> >> sequence and the raw key sequence.  But we'd still have to support
> >> `this-single-command-raw-keys` for the foreseeable future anyway.
> > I don't think that idea would fly.  Most uses of `read-key-sequence'
> > aren't going to be interested in the raw events.

> [ This is really a side-discussion about a better solution for the
>   long-term, but we need to solve the current problem regardless.  ]
> I was thinking of adding a new function read-raw-key-sequence which
> returns both the normal and the raw key sequences, with the hope that
> all users of this-single-command-raw-keys could eventually be changed to
> make use of that new function instead.

Ah, I see.  Yes, that would be a good idea.  There are only a few uses
of this-single-command-raw-keys (six) in the Emacs sources.  Any
external uses will have been written by experts, who would probably be
willing to convert to a more rational interface, such as
read-raw-key-sequence.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Stefan Monnier]

> I'm still not sure there's a problem.  But I think the effort involved
> in showing there's no problem would exceed that needed to solve it, so
> we might as well just assume there's a problem.

Exactly: I can't convince myself that it's safe, even after spending
some effort on it, so I think we need to makes its safety more obvious.

> Which I think is what you're doing anyway.  :-)

Exactly.

>> > That's quite close to what I've done.  The difficulty is in getting a
>> > pointer to that local buffer for use by the menu handling code.
>> Hmm... could you show me that menu handling code you're referring to?
>> The only user I can see of raw_keybuf (outside of local uses within
>> read_key_sequence) is this-single-command-raw-keys, so maybe I'm
>> missing something.
> read_menu_command calls read_key_sequence.  It is called by this stack
> of functions:
> - read_key_sequence is called by
> - read_menu_command .. ...... ..
> - read_menu_input
> - tty_menu_activate
> - tty_menu_show, which is one value of terminal->menu_show_hook.  This
>   is called in its turn by:
> - Fx_popup_menu, which besides being called from many places in lisp is
>   also called from
> - read_char_x_menu_prompt
> - read_char
> - read_key_sequence
> This is the recursion which makes it difficult just to use a global
> buffer for the raw key events.

Ah, I see.  I thought that by "getting a pointer to that local buffer"
you meant that the menu handling code actually needs to get its hands at
the raw_keybuf variable.  Yes, I don't doubt that read-key-sequence can
be called recursively, although I indeed hadn't thought about the above
case.

Here's another case where read-key-sequence can be invoked recursively:
read_key_sequence calls keyremap_step which calls
access_keymap_keyremap, which uses `call1` to run the Elisp code
specified in the remapping keymap (e.g. input-decode-map for
xterm-mouse-mode), which internally may decide to call read-key-sequence
(currently at least xterm-mouse-mode and things like
event-apply-control-modifier use read-event rather than
read-key(-sequence), but there could be very valid reasons to use
read-key(-sequence) in there).

> 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.

I think a record_unwind_protect is the easiest way to make it safer.

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.


        Stefan

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).

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

[Stefan Monnier]

>> 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.

>> 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.

I think in this case eliminating this-single-command-raw-keys is
a better solution, since we can then eliminate the global var.


        Stefan

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

[Alan Mackenzie]

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).

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

[Eli Zaretskii]

> Date: Tue, 28 Nov 2017 18:03:04 +0000
> Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> What do you say?  I would like to revert the previous commit that fixed
> this and install the one below.

If you have tested TTY menus well enough, including popup menus
(C-mouse3 after "emacs -Q" etc.), and also the simulated dialog boxes
(yes-or-no-p when triggered by a mouse gesture), then I'm okay with
this.

But please clean the patch up, it seems to have some WIP fragments
that need to be deleted.

Thanks.

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

[Alan Mackenzie]

Hello, Eli.

On Tue, Nov 28, 2017 at 22:30:27 +0200, Eli Zaretskii wrote:
> > Date: Tue, 28 Nov 2017 18:03:04 +0000
> > Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

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

> If you have tested TTY menus well enough, including popup menus
> (C-mouse3 after "emacs -Q" etc.), and also the simulated dialog boxes
> (yes-or-no-p when triggered by a mouse gesture), then I'm okay with
> this.

Sorry for being stupid, but I can't seem to find any way of triggering a
dialog box on the Linux tty.  I've got `use-dialog-box' at t, the
default setting.  I've manipulated the configuration GUI without any
problems, though.  Would that do instead?

> But please clean the patch up, it seems to have some WIP fragments
> that need to be deleted.

I've done that.

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Eli Zaretskii]

> Date: Fri, 1 Dec 2017 16:48:00 +0000
> Cc: monnier@IRO.UMontreal.CA, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > If you have tested TTY menus well enough, including popup menus
> > (C-mouse3 after "emacs -Q" etc.), and also the simulated dialog boxes
> > (yes-or-no-p when triggered by a mouse gesture), then I'm okay with
> > this.
> 
> Sorry for being stupid, but I can't seem to find any way of triggering a
> dialog box on the Linux tty.

Visit a file, modify it, then try to exit by clicking File->Quit from
the menu bar.