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

From: Tim Ruffing <[email-masked]>
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.

From f3bd0b346e7aa435d778e56b9a7c6a123315df18 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <[email-masked]>
Date: Wed, 27 Dec 2023 14:26:26 +0100
Subject: [PATCH 1/4] Extract check for end of macro to function

* src/macros.h (at_end_of_macro_p):
* src/macros.c (at_end_of_macro_p):
New function.
* src/keyboard.c (read_char): Use the new function.
---
 src/keyboard.c |  3 +--
 src/macros.c   | 12 ++++++++++++
 src/macros.h   |  5 +++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index e1d738dd6ef..d47363bc39f 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2627,8 +2627,7 @@ read_char (int commandflag, Lisp_Object map,
       /* Exit the macro if we are at the end.
 	 Also, some things replace the macro with t
 	 to force an early exit.  */
-      if (EQ (Vexecuting_kbd_macro, Qt)
-	  || executing_kbd_macro_index >= XFIXNAT (Flength (Vexecuting_kbd_macro)))
+      if (at_end_of_macro_p ())
 	{
 	  XSETINT (c, -1);
 	  goto exit;
diff --git a/src/macros.c b/src/macros.c
index 5f71bcbd361..62129be1629 100644
--- a/src/macros.c
+++ b/src/macros.c
@@ -353,6 +353,18 @@ init_macros (void)
   executing_kbd_macro = Qnil;
 }
 
+/* Whether the execution of a macro has reached its end.
+   It makes only sense to call this when while executing a macro.  */
+
+bool
+at_end_of_macro_p (void)
+{
+  eassume (!NILP (Vexecuting_kbd_macro));
+  /* Some things replace the macro with t to force an early exit.  */
+  return EQ (Vexecuting_kbd_macro, Qt)
+    || executing_kbd_macro_index >= XFIXNAT (Flength (Vexecuting_kbd_macro));
+}
+
 void
 syms_of_macros (void)
 {
diff --git a/src/macros.h b/src/macros.h
index 51599a29bcd..7870aa4de1d 100644
--- a/src/macros.h
+++ b/src/macros.h
@@ -47,4 +47,9 @@ #define EMACS_MACROS_H
 
 extern void store_kbd_macro_char (Lisp_Object);
 
+/* Whether the execution of a macro has reached its end.
+   It makes only sense to call this when while executing a macro.  */
+
+extern bool at_end_of_macro_p (void);
+
 #endif /* EMACS_MACROS_H */
-- 
2.43.0

From 23bcea43d9c56e829bf24877653d59a051e6cd19 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <[email-masked]>
Date: Wed, 27 Dec 2023 14:29:34 +0100
Subject: [PATCH 2/4] * src/keyboard.c (requeued_events_pending_p): Revive

* src/keyboard.c (requeued_events_pending_p):
Fix 'requeued_events_pending_p' function by adding missing
'Vunread_*_event' variables. When 'requeued_events_pending_p'
was added (commit b1878f450868365f27af0c71900237056a44f900),
'Vunread_command_events' was the only variable of this kind.
* (Finput_pending_p): Actually use the changed function.
---
 src/keyboard.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index d47363bc39f..041b616d9aa 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11556,7 +11556,7 @@ clear_input_pending (void)
 }
 
 /* Return true if there are pending requeued events.
-   This isn't used yet.  The hope is to make wait_reading_process_output
+   In the future, the hope is to make wait_reading_process_output
    call it, and return if it runs Lisp code that unreads something.
    The problem is, kbd_buffer_get_event needs to be fixed to know what
    to do in that case.  It isn't trivial.  */
@@ -11564,7 +11564,9 @@ clear_input_pending (void)
 bool
 requeued_events_pending_p (void)
 {
-  return (CONSP (Vunread_command_events));
+  return (CONSP (Vunread_command_events)
+	  || CONSP (Vunread_post_input_method_events)
+	  || CONSP (Vunread_input_method_events));
 }
 
 DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
@@ -11575,9 +11577,7 @@ DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
 If CHECK-TIMERS is non-nil, timers that are ready to run will do so.  */)
   (Lisp_Object check_timers)
 {
-  if (CONSP (Vunread_command_events)
-      || !NILP (Vunread_post_input_method_events)
-      || !NILP (Vunread_input_method_events))
+  if (requeued_events_pending_p ())
     return (Qt);
 
   /* Process non-user-visible events (Bug#10195).  */
-- 
2.43.0

From 27db0625e5348d50bc59674c0a8c8d45ed4db874 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <[email-masked]>
Date: Wed, 27 Dec 2023 14:32:09 +0100
Subject: [PATCH 3/4] Fix -1 leaking from C to lisp in 'read-event' etc.

This fixes a bug that could make 'read-event', 'read-char', and
'read-char-exclusive' erroneously return -1, an internal magic return
value of 'read_char' leaked from C to lisp.
* src/keyboard.c (read_char, read_key_sequence): Move handling
of the end of a keyboard macro from 'read_char' to its caller
'read_key_sequence', which is the only caller that can
meaningfully deal with this case.
---
 src/keyboard.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index 041b616d9aa..9b5e020238c 100644
--- 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;
-	}
-
       c = Faref (Vexecuting_kbd_macro, make_int (executing_kbd_macro_index));
       if (STRINGP (Vexecuting_kbd_macro)
 	  && (XFIXNAT (c) & 0x80) && (XFIXNAT (c) <= 0xff))
@@ -10684,8 +10676,19 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	    }
 	  used_mouse_menu = used_mouse_menu_history[t];
 	}
-
-      /* If not, we should actually read a character.  */
+      /* If we're at the end of a macro, exit it by returning 0,
+	 unless there are unread events pending.  */
+      else if (!NILP (Vexecuting_kbd_macro)
+	  && at_end_of_macro_p ()
+	  && !requeued_events_pending_p ())
+	{
+	  t = 0;
+	  /* The Microsoft C compiler can't handle the goto that
+	     would go here.  */
+	  dummyflag = true;
+	  break;
+	}
+      /* Otherwise, we should actually read a character.  */
       else
 	{
 	  {
@@ -10777,18 +10780,6 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	      return -1;
 	    }
 
-	  /* read_char returns -1 at the end of a macro.
-	     Emacs 18 handles this by returning immediately with a
-	     zero, so that's what we'll do.  */
-	  if (FIXNUMP (key) && XFIXNUM (key) == -1)
-	    {
-	      t = 0;
-	      /* The Microsoft C compiler can't handle the goto that
-		 would go here.  */
-	      dummyflag = true;
-	      break;
-	    }
-
 	  /* If the current buffer has been changed from under us, the
 	     keymap may have changed, so replay the sequence.  */
 	  if (BUFFERP (key))
-- 
2.43.0

From e73e08927792303013a8a9935656365f9f2299b6 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <[email-masked]>
Date: Wed, 27 Dec 2023 14:32:09 +0100
Subject: [PATCH 4/4] Remove workarounds for solved 'read-event' bug

* lisp/subr.el (read-char-choice-with-read-key):
* lisp/net/dbus.el (dbus-call-method):
* lisp/calc/calc-prog.el:
Remove workarounds for the bug fixed in the previous commit
ac82baea1c41ec974ad49f2861ae6c06bda2b4ed, where 'read-event',
'read-char' and 'read-char-exclusively' could return wrongly -1.
In the case of lisp/dbus.el, this reverts commit
7177393826c73c87ffe9b428f0e5edae244d7a98.
---
 lisp/calc/calc-prog.el | 6 ------
 lisp/net/dbus.el       | 6 +-----
 lisp/subr.el           | 5 -----
 3 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/lisp/calc/calc-prog.el b/lisp/calc/calc-prog.el
index 03210995eb3..177c179433d 100644
--- 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))
@@ -1300,8 +1298,6 @@ calc-kbd-loop
 	(message "Reading loop body..."))
     (while (>= count 0)
       (setq ch (read-event))
-      (if (eq ch -1)
-	  (error "Unterminated Z%c in keyboard macro" open))
       (if (eq ch ?Z)
 	  (progn
 	    (setq ch (read-event)
@@ -1428,8 +1424,6 @@ calc-kbd-push
 	       (message "Reading body..."))
 	   (while (>= count 0)
 	     (setq ch (read-char))
-	     (if (= ch -1)
-		 (error "Unterminated Z` in keyboard macro"))
 	     (if (= ch ?Z)
 		 (progn
 		   (setq ch (read-char)
diff --git a/lisp/net/dbus.el b/lisp/net/dbus.el
index 77b334e704e..46f85daba24 100644
--- a/lisp/net/dbus.el
+++ b/lisp/net/dbus.el
@@ -371,11 +371,7 @@ dbus-call-method
 	 (apply
           #'dbus-message-internal dbus-message-type-method-call
           bus service path interface method #'dbus-call-method-handler args))
-        (result (unless executing-kbd-macro (cons :pending nil))))
-
-    ;; While executing a keyboard macro, we run into an infinite loop,
-    ;; receiving the event -1.  So we don't try to get the result.
-    ;; (Bug#62018)
+        (result (cons :pending nil)))
 
     ;; Wait until `dbus-call-method-handler' has put the result into
     ;; `dbus-return-values-table'.  If no timeout is given, use the
diff --git a/lisp/subr.el b/lisp/subr.el
index df28989b399..78e7ffbc979 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3539,11 +3539,6 @@ read-char-choice-with-read-key
 		 (help-form-show)))
 	   ((memq char chars)
 	    (setq done t))
-	   ((and executing-kbd-macro (= char -1))
-	    ;; read-event returns -1 if we are in a kbd macro and
-	    ;; there are no more events in the macro.  Attempt to
-	    ;; get an event interactively.
-	    (setq executing-kbd-macro nil))
 	   ((not inhibit-keyboard-quit)
 	    (cond
 	     ((and (null esc-flag) (eq char ?\e))
-- 
2.43.0