From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc. Date: Sat, 06 Jan 2024 09:42:39 +0200 Message-ID: <83wmsmucuo.fsf@gnu.org> References: <46480759b6d89b5a4864e8ee1b986817366a56e5.camel@timruffing.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="9093"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 68272@debbugs.gnu.org To: Tim Ruffing Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Jan 06 08:44:22 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rM1Ly-0002CP-9w for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 06 Jan 2024 08:44:22 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rM1Lb-0005ey-D6; Sat, 06 Jan 2024 02:43:59 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rM1LZ-0005eU-Dv for bug-gnu-emacs@gnu.org; Sat, 06 Jan 2024 02:43:57 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rM1LZ-0001A5-69 for bug-gnu-emacs@gnu.org; Sat, 06 Jan 2024 02:43:57 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rM1Ld-0006Ry-Me for bug-gnu-emacs@gnu.org; Sat, 06 Jan 2024 02:44:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 06 Jan 2024 07:44:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 68272 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 68272-submit@debbugs.gnu.org id=B68272.170452698824720 (code B ref 68272); Sat, 06 Jan 2024 07:44:01 +0000 Original-Received: (at 68272) by debbugs.gnu.org; 6 Jan 2024 07:43:08 +0000 Original-Received: from localhost ([127.0.0.1]:58489 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rM1Kl-0006Qd-Mj for submit@debbugs.gnu.org; Sat, 06 Jan 2024 02:43:08 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:42184) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rM1Kf-0006Q5-WA for 68272@debbugs.gnu.org; Sat, 06 Jan 2024 02:43:06 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rM1KV-00014C-1l; Sat, 06 Jan 2024 02:42:51 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-version:References:Subject:In-Reply-To:To:From: Date; bh=Bmd7W9he294eibqJoBh5BmxpYXKRUGUSMF66jhhaEa4=; b=JniZypxfQqlXlxTTomW2 8M6DycCrW0bbrDzT3h56yUk2lQ4+oiusoX5OrmIGYgJxa3ITs49ekOtYXaFGP1nf9O32AoKFXOlQJ 62Gr7kQ6652nJJdog0EZ0WLoC9sou/yeCBQ/9/uJ1FgQa+s+27D3t6pNE8t1meYE8rG0Gd2nQkQt7 q6988bSjbtpyWRAJNdV0PLr01l2xU1uF4G2vdiVhLidpJlWSEYJnm54cBO/DZYdwqKzMCcrq6QLMN 2sSR+ZM7kOAG2s54FFm9c8PvOQgLtf9I2orL9ci9Ez50M9bS/HM20gRX0CxtrpJRj0lTqyeYcIvmU O6pGzW6Q2ZkDIg==; In-Reply-To: <46480759b6d89b5a4864e8ee1b986817366a56e5.camel@timruffing.de> (message from Tim Ruffing on Fri, 05 Jan 2024 22:19:10 +0100) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:277432 Archived-At: > From: Tim Ruffing > 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.