bug#67046: 29.1.50; map-y-or-n-p infinite loops if it's at the end of a kmacro

bug#67046: 29.1.50; map-y-or-n-p infinite loops if it's at the end of a kmacro

[Spencer Baugh]

For example, if you record a kmacro which does C-x s, then when you
repeat the kmacro and there are more buffers which could be saved, the
kmacro will hang instead of either completing or erroring.

Straightforward minimal reproduction:

(execute-kbd-macro (kbd "M-: (map-y-or-n-p SPC \"\" SPC #'ignore SPC '(1)) RET"))

This hangs in a loop around read-event and has to be interrupted with C-g.
The loop in question in map-y-or-n-p:

(while (progn
 	(setq char (read-event))
 	;; If we get -1, from end of keyboard
 	;; macro, try again.
          (equal char -1)))

I'll fix this in a followup patch.


In GNU Emacs 29.1.50 (build 15, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2023-10-25 built on

Repository revision: 5cbca757f620c7b4ca31776711a247b8f266c36e
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.8 (Green Obsidian)

Configured using:
 'configure --config-cache --with-x-toolkit=lucid
 --with-gif=ifavailable'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM
XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  delete-selection-mode: t
  global-so-long-mode: t
  pixel-scroll-precision-mode: t
  jane-fe-minor-mode: t
  jane-fe-jenga-minor-mode: t
  editorconfig-mode: t
  which-function-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  shell-dirtrack-mode: t
  server-mode: t
  windmove-mode: t
  savehist-mode: t
  save-place-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  context-menu-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t


Features:
(shadow mail-extr emacsbug magit-imenu git-rebase face-remap pcmpl-git
vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs log-view cus-start dabbrev
cl-print macros vc-git vc-fe vc-hg vc-dir vc vc-dispatcher cc-mode
cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars
cc-defs mule-util sort display-line-numbers find-dired pulse
bug-reference shortdoc help-fns radix-tree tabify man sh-script treesit
misearch multi-isearch executable org-element org-persist org-id
org-refile avl-tree generator oc-basic ol-eww eww xdg url-queue mm-url
ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect gnus-art mm-uu mml2015
mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill kinsoku
url-file svg dom gnus-group gnus-undo gnus-start gnus-dbus dbus xml
gnus-cloud nnimap nnmail mail-source utf7 nnoo gnus-spec gnus-int
gnus-range gnus-win ol-docview doc-view jka-compr image-mode exif
ol-bibtex bibtex ol-bbdb ol-w3m ol-doi org-link-doi goto-addr delsel
so-long jane-fe-read-feature pixel-scroll cua-base tramp tramp-loaddefs
trampver tramp-integration tramp-compat parse-time iso8601 ffap
jane-merlin merlin-imenu let-alist merlin-xref merlin-cap merlin
jane-async-merlin jane-completion grep jane-common jane-comint
org-protocol jane-fe-project xref files-x jane-fe-menu ecaml_plugin view
gopcaml magit-bookmark bookmark image+ advice image-file image-converter
editorconfig editorconfig-core editorconfig-core-handle
editorconfig-fnmatch whitespace jane-auto-modes vba-mode markdown-mode
color jane jane-yasnippet jane-micro-features ert ewoc debug backtrace
jane-diff unified-test-mode shell-file core core-buffer core-error
jane-sexp jane-python jane-ocaml jane-tuareg-theme tuareg tuareg-compat
tuareg-opam skeleton flymake-proc flymake warnings thingatpt smie
caml-types caml-help caml-emacs find-file compile jane-cr jane-codeium
jane-align jane-deprecated jane-smerge gnu-elpa-keyring-update
jane-ocp-indent ocp-indent jane-eglot yasnippet-autoloads
swiper-autoloads htmlize-autoloads eglot-autoloads
editorconfig-autoloads codeium-autoloads jane-autoloads jane-util
ob-shell page-ext dired-x magit-extras project magit-submodule
magit-obsolete magit-blame magit-stash magit-reflog magit-bisect
magit-push magit-pull magit-fetch magit-clone magit-remote magit-commit
magit-sequence magit-notes magit-worktree magit-tag magit-merge
magit-branch magit-reset magit-files magit-refs magit-status magit
magit-repos magit-apply magit-wip magit-log which-func imenu magit-diff
smerge-mode diff diff-mode git-commit log-edit message sendmail
yank-media puny dired dired-loaddefs rfc822 mml mml-sec epa derived epg
rfc6068 epg-config mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils mailheader pcvs-util
add-log magit-core magit-autorevert autorevert filenotify magit-margin
magit-transient magit-process with-editor shell server magit-mode
transient edmacro kmacro magit-git magit-section magit-utils crm dash
gnus nnheader gnus-util text-property-search mail-utils range mm-util
mail-prsvr cl-extra help-mode windmove org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-src ob-comint org-pcomplete pcomplete
org-list org-footnote org-faces org-entities time-date noutline outline
ob-emacs-lisp ob-core ob-eval org-cycle org-table ol rx org-fold
org-fold-core org-keys oc org-loaddefs find-func cal-menu calendar
cal-loaddefs org-version org-compat org-macs format-spec gdb-mi bindat
gud easy-mmode comint ansi-osc ansi-color ring vundo modus-vivendi-theme
modus-themes pcase savehist saveplace cus-edit pp cus-load icons
wid-edit adaptive-wrap-autoloads csv-mode-autoloads
cyberpunk-theme-autoloads evil-autoloads exwm-autoloads helm-autoloads
helm-core-autoloads async-autoloads ivy-autoloads magit-autoloads
git-commit-autoloads finder-inf magit-section-autoloads dash-autoloads
popup-autoloads url-http-ntlm-autoloads url-auth vc-hgcmd-autoloads
vundo-autoloads info with-editor-autoloads xelb-autoloads package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs
cl-lib rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify dynamic-setting system-font-setting
font-render-setting cairo x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 894359 138172)
 (symbols 48 53718 0)
 (strings 32 244206 27230)
 (string-bytes 1 7923236)
 (vectors 16 139411)
 (vector-slots 8 2276866 106367)
 (floats 8 660 496)
 (intervals 56 29358 1203)
 (buffers 976 85))

bug#67046: 29.1.50; map-y-or-n-p infinite loops if it's at the end of a kmacro

[Spencer Baugh]

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


Patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-infinite-loop-in-map-y-or-n-p-if-at-the-end-of.patch --]
[-- Type: text/x-patch, Size: 1845 bytes --]

From d7aa9644b308a936926b1074d0f6eac3e425b011 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Fri, 10 Nov 2023 11:27:10 -0500
Subject: [PATCH] Don't infinite loop in map-y-or-n-p if at the end of kmacro

Previously, if map-y-or-n-p got -1 from read-event (indicating no
input due to the end of a keyboard macro), it would just infinite
loop.

Now it behaves like other commands which use read-event/read-char/etc,
and just errors when we try to look up -1 in our keymap and find
nothing.

Also, just for the sake of users, print a slightly prettier message
when this happens.

* lisp/emacs-lisp/map-ynp.el (map-y-or-n-p): Don't loop if we reach
the end of a keyboard macro.  (Bug#67046)
---
 lisp/emacs-lisp/map-ynp.el | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/map-ynp.el b/lisp/emacs-lisp/map-ynp.el
index cb1cc88e78f..fffb199e2ea 100644
--- a/lisp/emacs-lisp/map-ynp.el
+++ b/lisp/emacs-lisp/map-ynp.el
@@ -168,16 +168,14 @@ map-y-or-n-p
 				(key-description (vector help-char)))
 		       (if minibuffer-auto-raise
 			   (raise-frame (window-frame (minibuffer-window))))
-		       (while (progn
-				(setq char (read-event))
-				;; If we get -1, from end of keyboard
-				;; macro, try again.
-                                (equal char -1)))
+		       (setq char (read-event))
 		       ;; Show the answer to the question.
 		       (message "%s(y, n, !, ., q, %sor %s) %s"
 				prompt user-keys
 				(key-description (vector help-char))
-				(single-key-description char)))
+				(if (equal char -1)
+                                    "[end-of-keyboard-macro]"
+                                  (single-key-description char))))
 		     (setq def (lookup-key map (vector char))))
 		   (cond ((eq def 'exit)
 			  (setq next (lambda () nil)))
-- 
2.39.3

bug#67046: 29.1.50; map-y-or-n-p infinite loops if it's at the end of a kmacro

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Fri, 10 Nov 2023 11:47:36 -0500
> 
> >From d7aa9644b308a936926b1074d0f6eac3e425b011 Mon Sep 17 00:00:00 2001
> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Fri, 10 Nov 2023 11:27:10 -0500
> Subject: [PATCH] Don't infinite loop in map-y-or-n-p if at the end of kmacro
> 
> Previously, if map-y-or-n-p got -1 from read-event (indicating no
> input due to the end of a keyboard macro), it would just infinite
> loop.
> 
> Now it behaves like other commands which use read-event/read-char/etc,
> and just errors when we try to look up -1 in our keymap and find
> nothing.
> 
> Also, just for the sake of users, print a slightly prettier message
> when this happens.
> 
> * lisp/emacs-lisp/map-ynp.el (map-y-or-n-p): Don't loop if we reach
> the end of a keyboard macro.  (Bug#67046)

You have removed the loop, so now the "try again" part is no longer
working.  How can we be sure this doesn't introduce some regression?
Do you understand why this loop was added, in commit 3f72fac865?  If
so, can you explain why it was needed?

I have added Stefan to the discussion.

bug#67046: 29.1.50; map-y-or-n-p infinite loops if it's at the end of a kmacro

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Date: Fri, 10 Nov 2023 11:47:36 -0500
>> 
>> >From d7aa9644b308a936926b1074d0f6eac3e425b011 Mon Sep 17 00:00:00 2001
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Date: Fri, 10 Nov 2023 11:27:10 -0500
>> Subject: [PATCH] Don't infinite loop in map-y-or-n-p if at the end of kmacro
>> 
>> Previously, if map-y-or-n-p got -1 from read-event (indicating no
>> input due to the end of a keyboard macro), it would just infinite
>> loop.
>> 
>> Now it behaves like other commands which use read-event/read-char/etc,
>> and just errors when we try to look up -1 in our keymap and find
>> nothing.
>> 
>> Also, just for the sake of users, print a slightly prettier message
>> when this happens.
>> 
>> * lisp/emacs-lisp/map-ynp.el (map-y-or-n-p): Don't loop if we reach
>> the end of a keyboard macro.  (Bug#67046)
>
> You have removed the loop, so now the "try again" part is no longer
> working.

Yep.

> How can we be sure this doesn't introduce some regression?

I'm not certain, but the behavior as written is a completely inert
infinite loop, just sitting and spamming read-event over and over
forever and maxing out the CPU.  It seems hard for this to be correct
behavior.

> Do you understand why this loop was added, in commit 3f72fac865?

I do not.

> If so, can you explain why it was needed?

Maybe this was some kind of XEmacs confusion?  I don't know the full
history of keyboard macros, but perhaps in XEmacs read-event would start
returning keyboard input again after starting to return -1.  (In GNU
Emacs, AFAICT, it's always been the case that read-event returns -1
forever after we run out of input in the keyboard macro, but haven't yet
actually returned from the command loop)

Or it was just a bug from the start.

bug#67046: 29.1.50; map-y-or-n-p infinite loops if it's at the end of a kmacro

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> How can we be sure this doesn't introduce some regression?
> I'm not certain, but the behavior as written is a completely inert
> infinite loop, just sitting and spamming read-event over and over
> forever and maxing out the CPU.  It seems hard for this to be correct
> behavior.

Agreed.

>> Do you understand why this loop was added, in commit 3f72fac865?
> I do not.

Neither do I.  The handling of keyboard macro wasn't very different from
what we have now, so by my reading of the code it suffered from the same
inf-loop as the one we're discussing.

I see that Gerd fixed that commit a week later by removing a `not` which
strongly suggests the code wasn't tested very much, if at all.

> Maybe this was some kind of XEmacs confusion?  I don't know the full
> history of keyboard macros, but perhaps in XEmacs read-event would start
> returning keyboard input again after starting to return -1.  (In GNU
> Emacs, AFAICT, it's always been the case that read-event returns -1
> forever after we run out of input in the keyboard macro, but haven't yet
> actually returned from the command loop)

Reading the code I'm wondering how come we don't get into inf-loops
more often when executing macros that stop in the middle of a recursive
edit, or minibuffer input, or ...


        Stefan

bug#67046: 29.1.50; map-y-or-n-p infinite loops if it's at the end of a kmacro

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  67046@debbugs.gnu.org
> Date: Tue, 14 Nov 2023 17:09:45 -0500
> 
> >> How can we be sure this doesn't introduce some regression?
> > I'm not certain, but the behavior as written is a completely inert
> > infinite loop, just sitting and spamming read-event over and over
> > forever and maxing out the CPU.  It seems hard for this to be correct
> > behavior.
> 
> Agreed.
> 
> >> Do you understand why this loop was added, in commit 3f72fac865?
> > I do not.
> 
> Neither do I.  The handling of keyboard macro wasn't very different from
> what we have now, so by my reading of the code it suffered from the same
> inf-loop as the one we're discussing.
> 
> I see that Gerd fixed that commit a week later by removing a `not` which
> strongly suggests the code wasn't tested very much, if at all.
> 
> > Maybe this was some kind of XEmacs confusion?  I don't know the full
> > history of keyboard macros, but perhaps in XEmacs read-event would start
> > returning keyboard input again after starting to return -1.  (In GNU
> > Emacs, AFAICT, it's always been the case that read-event returns -1
> > forever after we run out of input in the keyboard macro, but haven't yet
> > actually returned from the command loop)
> 
> Reading the code I'm wondering how come we don't get into inf-loops
> more often when executing macros that stop in the middle of a recursive
> edit, or minibuffer input, or ...

So what is the path forward?  Should we install the change on master
and cross our fingers?

bug#67046: 29.1.50; map-y-or-n-p infinite loops if it's at the end of a kmacro

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> So what is the path forward?
> Should we install the change on master and cross our fingers?

Yes, the change is safe.


        Stefan

bug#67046: 29.1.50; map-y-or-n-p infinite loops if it's at the end of a kmacro

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: sbaugh@janestreet.com,  67046@debbugs.gnu.org
> Date: Wed, 15 Nov 2023 07:19:34 -0500
> 
> > So what is the path forward?
> > Should we install the change on master and cross our fingers?
> 
> Yes, the change is safe.

Thanks, installed on master, and closing the bug.