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

[Tim Ruffing <crypto@timruffing.de>]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

Hi, this is an updated patch set.

Changes to the previous revision:
 * Instead of changing requeued_events_pending_p, it's renamed to
   requeued_command_events_pending_p, and I've fixed the outdated
   comment that had misled me.  Then a new requeued_events_pending_p
   with different semantics is added and used in following commits.
   This addresses one of Eli's comments, and should make sure that
   existing callers of requeued_events_pending_p are not affected by
   the patch.
 * I've added a large comment to src/macro.c as an attempt to explain
   how we handle the end of a keyboard macro.
 * I've improved the commit message of (now) 6be0f5f. 

Best,
Tim

[-- Attachment #2: 0001-Extract-check-for-end-of-macro-to-function.patch --]
[-- Type: text/x-patch, Size: 2056 bytes --]

From 27de73af150c458669de29e8662a281baeb0a603 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:26:26 +0100
Subject: [PATCH 1/5] 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 eb0de98bad1..b6fc568cde5 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2637,8 +2637,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.44.0


[-- Attachment #3: 0002-src-keyboard.c-requeued_events_pending_p-Improve-nam.patch --]
[-- Type: text/x-patch, Size: 3421 bytes --]

From 848ede9b578833414e764d9f9dd85b7677a4b110 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:29:34 +0100
Subject: [PATCH 2/5] * src/keyboard.c (requeued_events_pending_p): Improve
 name and fix comment

* src/keyboard.c, src/keyboard.h (requeued_events_pending_p): Rename to
'requeued_command_events_pending_p' to clarify that the function covers
only command events. Fix wrong comment that claimed that the function
was unused.
* src/process.c (wait_reading_process_output): Update caller to use the
new name.
---
 src/keyboard.c | 8 ++------
 src/keyboard.h | 2 +-
 src/process.c  | 8 ++++----
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index b6fc568cde5..e5efde4ef53 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11565,14 +11565,10 @@ clear_input_pending (void)
   input_pending = false;
 }
 
-/* Return true if there are pending requeued events.
-   This isn't used yet.  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.  */
+/* Return true if there are pending requeued command events.  */
 
 bool
-requeued_events_pending_p (void)
+requeued_command_events_pending_p (void)
 {
   return (CONSP (Vunread_command_events));
 }
diff --git a/src/keyboard.h b/src/keyboard.h
index 68e68bc2ae3..600aaf11517 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -483,7 +483,7 @@ #define EVENT_HEAD_KIND(event_head) \
 extern int gobble_input (void);
 extern bool input_polling_used (void);
 extern void clear_input_pending (void);
-extern bool requeued_events_pending_p (void);
+extern bool requeued_command_events_pending_p (void);
 extern void bind_polling_period (int);
 extern int make_ctrl_char (int) ATTRIBUTE_CONST;
 extern void stuff_buffered_input (Lisp_Object);
diff --git a/src/process.c b/src/process.c
index 48a2c0c8e53..6b8b483cdf7 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5439,7 +5439,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
 	  /* If there is unread keyboard input, also return.  */
 	  if (read_kbd != 0
-	      && requeued_events_pending_p ())
+	      && requeued_command_events_pending_p ())
 	    break;
 
           /* This is so a breakpoint can be put here.  */
@@ -5849,7 +5849,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
       /* If there is unread keyboard input, also return.  */
       if (read_kbd != 0
-	  && requeued_events_pending_p ())
+	  && requeued_command_events_pending_p ())
 	break;
 
       /* If we are not checking for keyboard input now,
@@ -8036,7 +8036,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
 	  /* If there is unread keyboard input, also return.  */
 	  if (read_kbd != 0
-	      && requeued_events_pending_p ())
+	      && requeued_command_events_pending_p ())
 	    break;
 
 	  if (timespec_valid_p (timer_delay))
@@ -8109,7 +8109,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 
       /* If there is unread keyboard input, also return.  */
       if (read_kbd
-	  && requeued_events_pending_p ())
+	  && requeued_command_events_pending_p ())
 	break;
 
       /* If wait_for_cell. check for keyboard input
-- 
2.44.0


[-- Attachment #4: 0003-src-keyboard.c-requeued_events_pending_p-New-functio.patch --]
[-- Type: text/x-patch, Size: 2432 bytes --]

From 17593b2c1e7313b156ecc7077aaa019290b8a096 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:29:34 +0100
Subject: [PATCH 3/5] * src/keyboard.c (requeued_events_pending_p): New
 function

* src/keyboard.c, src/keyboard.h (requeued_events_pending_p): Add
function 'requeued_events_pending_p' (whose name was made available in
the previous commit). As opposed to the previous function with the same
name, the new function covers both command and other events.
* src/keyboard.c (Finput_pending_p): Use the new function.
---
 src/keyboard.c | 15 ++++++++++++---
 src/keyboard.h |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index e5efde4ef53..c898988dc5f 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11573,6 +11573,17 @@ requeued_command_events_pending_p (void)
   return (CONSP (Vunread_command_events));
 }
 
+/* Return true if there are any pending requeued events (command events
+   or events to be processed by input methods).  */
+
+bool
+requeued_events_pending_p (void)
+{
+  return (requeued_command_events_pending_p ()
+	  || !NILP (Vunread_post_input_method_events)
+	  || !NILP (Vunread_input_method_events));
+}
+
 DEFUN ("input-pending-p", Finput_pending_p, Sinput_pending_p, 0, 1, 0,
        doc: /* Return t if command input is currently available with no wait.
 Actually, the value is nil only if we can be sure that no input is available;
@@ -11581,9 +11592,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).  */
diff --git a/src/keyboard.h b/src/keyboard.h
index 600aaf11517..2ce003fd444 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -484,6 +484,7 @@ #define EVENT_HEAD_KIND(event_head) \
 extern bool input_polling_used (void);
 extern void clear_input_pending (void);
 extern bool requeued_command_events_pending_p (void);
+extern bool requeued_events_pending_p (void);
 extern void bind_polling_period (int);
 extern int make_ctrl_char (int) ATTRIBUTE_CONST;
 extern void stuff_buffered_input (Lisp_Object);
-- 
2.44.0


[-- Attachment #5: 0004-Continue-reading-in-read-event-etc.-at-the-end-of-a-.patch --]
[-- Type: text/x-patch, Size: 5065 bytes --]

From 6be0f5f6c4253cefd350b23c79e469058d6c589c Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:32:09 +0100
Subject: [PATCH 4/5] Continue reading in 'read-event' etc. at the end of a
 keyboard macro

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. Instead of returning -1, the
aforementioned lisp functions now transparently continue reading
available input (e.g., from the keyboard) when reaching the end of a
keyboard macro.
* 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/macros.c (Fexecute_kbd_macro): Document how the end of keyboard
macro is processed.
---
 src/keyboard.c | 39 +++++++++++++++------------------------
 src/macros.c   | 23 +++++++++++++++++++++++
 2 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index c898988dc5f..e34d1b5491d 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2620,7 +2620,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
@@ -2634,15 +2635,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))
@@ -10694,8 +10686,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
 	{
 	  {
@@ -10787,18 +10790,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))
diff --git a/src/macros.c b/src/macros.c
index 62129be1629..98290e7e276 100644
--- a/src/macros.c
+++ b/src/macros.c
@@ -314,6 +314,29 @@ DEFUN ("execute-kbd-macro", Fexecute_kbd_macro, Sexecute_kbd_macro, 1, 3, 0,
 		      Vreal_this_command));
   record_unwind_protect (pop_kbd_macro, tem);
 
+  /* The following loop starts the execution of the macro.  Individual
+     characters from the macro are read by read_char, which takes care
+     of incrementing executing_kbd_macro_index.  The end of the
+     macro is handled as follows:
+     - read_key_sequence asks at_end_of_macro_p whether the end of
+     (one iteration of the macro) has been reached.  If so, it returns
+     the magic value 0 to command_loop_1.
+     - command_loop_1 returns Qnil to command_loop_2.
+     - command_loop_2 returns Qnil to this function
+       (but only the returning is relevant, not the actual value).
+
+     If read_char happens to be called at the end of the macro, but
+     before read_key_sequence could handle the end (e.g., because lisp
+     code calls 'read-event', 'read-char', and 'read-char-exclusive'),
+     read_char will simply continue reading other available input
+     (Bug#68272).
+
+     Note that this is similar (in observable behavior) to a simpler
+     implementation of keyboard macros in which this function simply
+     pushed all characters of the macro into the incoming event queue
+     and returned immediately.  Maybe this is the implementation that
+     we ideally would like to have, but switching to it will require
+     a larger code change.  */
   do
     {
       Vexecuting_kbd_macro = final;
-- 
2.44.0


[-- Attachment #6: 0005-Remove-workarounds-for-solved-read-event-bug.patch --]
[-- Type: text/x-patch, Size: 3001 bytes --]

From 96684bd0dd5b2ed701e3b440abd210dd9c6fdfd6 Mon Sep 17 00:00:00 2001
From: Tim Ruffing <crypto@timruffing.de>
Date: Wed, 27 Dec 2023 14:32:09 +0100
Subject: [PATCH 5/5] 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 d58f8ba3b27..ce933e3bfdc 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3554,11 +3554,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.44.0