Package: emacs;
Reported by: Tim Ruffing <crypto <at> timruffing.de>
Date: Fri, 5 Jan 2024 21:20:01 UTC
Severity: normal
Tags: patch
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Eli Zaretskii <eliz <at> gnu.org> To: Tim Ruffing <crypto <at> timruffing.de> Cc: 68272 <at> debbugs.gnu.org Subject: bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc. Date: Sat, 06 Jan 2024 09:42:39 +0200
> From: Tim Ruffing <crypto <at> 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.