bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc.

[Eli Zaretskii <eliz@gnu.org>]

> From: Tim Ruffing <crypto@timruffing.de>
> Date: Fri, 05 Jan 2024 22:19:10 +0100
> 
> 'read_char' in src/keyboard.c returns -1 to indicate the end of a
> keyboard macro. This case is supposed to be propagated via 
> 'read_key_sequence' and 'command_loop_2' to 'Fexecute_kbd_macro'.
> 
> But 'read_key_sequence' is not the only caller of 'read_char'. It is
> also called by 'read-event' and similar lisp functions, and in that
> case, the magic C return value -1 is wrongly propagated to the lisp
> caller. 
> 
> There are at least workarounds for this bug in at least three lisp
> modules in the code base, in subr.el, in calc and most recently added
> in dbus.el (bug #62018), see the attached patches. These patches are
> supposed to resolve the underlying issue, and then remove the
> workarounds.

"There be dragons."  AFAIU, this is basically a cleanup: a non-elegant
solution already exists, but we'd want to do it more elegantly.  In
such cases, and in a maze such as keyboard.c's processing of input and
related code, the danger is to introduce regressions because some code
somewhere expects something to happen, and the changes disrupt that.
With that in mind, couldn't we solve this in a more localized manner,
such that we are sure the changes could not affect unrelated code
paths and use cases?  For example, your patches modify
requeued_events_pending_p, but that function is called in several
places, including outside of keyboard.c -- how can we be sure we are
not introducing regressions this way?  And read_char is called in
several places, including by lread.c and read_char itself -- did you
figure out how will this change affect those?

Given your description above, that read_char is called by read-event
and other Lisp functions, I would expect a fix to be localized to
read_char and those functions calling read_char (to limit special
handling of -1 to those functions), but that is not what I see in the
patch.  Moreover, the changes in read_char modify code more than is
necessary to handle the "at end of macro" situation, so we are risking
changes that will break something else.  Here's an example:

> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -2610,7 +2610,8 @@ read_char (int commandflag, Lisp_Object map,
>        goto reread_for_input_method;
>      }
>  
> -  if (!NILP (Vexecuting_kbd_macro))
> +  /* If we're executing a macro, process it unless we are at its end. */
> +  if (!NILP (Vexecuting_kbd_macro) && !at_end_of_macro_p ())
>      {
>        /* We set this to Qmacro; since that's not a frame, nobody will
>  	 try to switch frames on us, and the selected window will
> @@ -2624,15 +2625,6 @@ read_char (int commandflag, Lisp_Object map,
>  	 selected.  */
>        Vlast_event_frame = internal_last_event_frame = Qmacro;
>  
> -      /* Exit the macro if we are at the end.
> -	 Also, some things replace the macro with t
> -	 to force an early exit.  */
> -      if (at_end_of_macro_p ())
> -	{
> -	  XSETINT (c, -1);
> -	  goto exit;
> -	}
> -

This hunk moves the at_end_of_macro_p test to a higher level, which
has a side effect of not executing the code before the original
at_end_of_macro_p test -- how do we know this won't break something
that happens to depend on that code which will now not execute in the
at-end-of-macro case?

Also note that at least in subr.el we don't only test the -1 value, we
also test that a keyboard macro is being executed, and in this and
other callers that handle -1, each application behaves in
application-specific way when it receives -1.  It is not clear to me
how your changes modify the behavior in those cases, and your
explanations and the log message doesn't seem to answer this question.
For example, this code in calc-prog:

> --- a/lisp/calc/calc-prog.el
> +++ b/lisp/calc/calc-prog.el
> @@ -1230,8 +1230,6 @@ calc-kbd-skip-to-else-if
>  	ch)
>      (while (>= count 0)
>        (setq ch (read-char))
> -      (if (= ch -1)
> -	  (error "Unterminated Z[ in keyboard macro"))
>        (if (= ch ?Z)
>  	  (progn
>  	    (setq ch (read-char))

now signals a specific error in the case of an unterminated keyboard
macro: what will the behavior in that case be after the changes?

The questions I ask above are not theoretical -- we have been bitten
before, more than once or twice, by making seemingly innocuous changes
like this in keyboard.c, only to discover later, sometimes much later,
that some important use case became broken due to the change.  My take
from those incidents was that read_char and related code in keyboard.c
is a complex maze of code that should be touched only if we have a
very good reason, and then in a way that makes changes as localized as
possible to the very fragments we want to change.  In this case, given
that we want a more elegant solution for a situation that we already
handle, I tend to avoid any such changes to begin with, for the
reasons I explained above, and instead perhaps document the special
meaning of -1 in these cases.  And if we want to consider such changes
these dangers notwithstanding, I would like to see them affecting as
little other code as possible.  Can you suggest a safer changeset?

Adding Stefan to the discussion, in case he has comments.

Thanks.