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