bug#72220: 31.0.50; [PATCH] Use 'unwind-protect' to ensure that Eshell always closes I/O handles

[Jim Porter <jporterbugs@gmail.com>]

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

Eshell has long had difficult-to-diagnose issues with I/O handles 
closing when they shouldn't, or not closing when they should. This is 
because the handles are refcounted entirely manually, and the actual 
code is very easy to get wrong: the location in the source code that 
increments the refcount is often distant from where it's decremented.

I've fixed many of these issues over the last couple years and added 
tests covering them, but it's time to solve this for once and for all. 
By using 'unwind-protect', we can always ensure that the handles are 
cleaned up at the right time without having to have a perfect 
understanding of every disparate part of Eshell's command evaluation.

Luckily, thanks to the previous work here, we have a thorough suite of 
regression tests, which made developing this patch a lot easier. This 
also fixes some remaining issues with I/O handles (see the new 
regression test).

[-- Attachment #2: 0001-Improve-correctness-of-eshell-do-eval-in-some-edge-c.patch --]
[-- Type: text/plain, Size: 1258 bytes --]

From f6ad218d8fce477b9b97ab247f932577a6d44189 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 19 Jul 2024 18:07:36 -0700
Subject: [PATCH 1/2] ; Improve correctness of 'eshell-do-eval' in some edge
 cases

* lisp/eshell/esh-cmd.el (eshell-do-eval): Make sure that replacing 'if'
forms returns the correct result, and evaluate to 'nil' for 'progn'
forms with no body.
---
 lisp/eshell/esh-cmd.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 2b47962735a..43d536ac463 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1159,7 +1159,7 @@ eshell-do-eval
                   (t
                    (caddr args)))))        ; Zero or one ELSE forms
             (unless (consp new-form)
-              (setq new-form (cons 'progn new-form)))
+              (setq new-form `(progn ,new-form)))
             (setcar form (car new-form))
             (setcdr form (cdr new-form))))
         (eshell-do-eval form synchronous-p))
@@ -1261,7 +1261,7 @@ eshell-do-eval
 		(setq args (cdr args)))))
 	(cond
 	 ((eq (car form) 'progn)
-	  (car (last form)))
+	  (car (last (cdr form))))
 	 ((eq (car form) 'prog1)
 	  (cadr form))
 	 (t
-- 
2.25.1


[-- Attachment #3: 0002-Use-unwind-protect-to-ensure-that-Eshell-always-clos.patch --]
[-- Type: text/plain, Size: 16526 bytes --]

From b21f4a3820be8fc579427a9dae20270bfebcbcc9 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 19 Jul 2024 18:02:16 -0700
Subject: [PATCH 2/2] Use 'unwind-protect' to ensure that Eshell always closes
 I/O handles

* lisp/eshell/esh-cmd.el (eshell-with-handles): New macro...
(eshell-commands): ... use it.
(eshell-with-copied-handles): Remove STEAL-P and allow multiple body
forms (this is an incompatible change, but the macro is currently
internal despite the name).
(eshell-parse-command, eshell-do-pipelines)
(eshell-do-pipelines-synchronously, eshell--invoke-command-directly-p):
Remove handle stealing.
(eshell-structure-basic-command, eshell-do-command)
(eshell-lisp-command): Remove 'eshell-close-handles'.
(eshell-protect): Make obsolete.
(eshell-rewrite-for-command, eshell-rewrite-while-command)
(eshell-rewrite-if-command, (eshell-parse-pipeline): Remove
'eshell-protect'.

* lisp/eshell/esh-io.el (eshell-duplicate-handles): Make STEAL-P
obsolete.

* lisp/eshell/esh-proc.el (eshell-gather-process-output): Call
'eshell-protect-handles' one more time.  Remove 'eshell-close-handles'.

* lisp/eshell/esh-var.el (eshell-parse-variable-ref): Reimplement
$<COMMAND> form using 'eshell-with-handles'.

* test/lisp/eshell/esh-cmd-tests.el
(esh-cmd-test/command-not-found/pipeline): New test.

* test/lisp/eshell/em-tramp-tests.el
(em-tramp-test/should-replace-command): Adjust check for
'eshell-with-copied-handles'.
---
 lisp/eshell/esh-cmd.el             | 92 ++++++++++++++----------------
 lisp/eshell/esh-io.el              |  8 +--
 lisp/eshell/esh-proc.el            |  2 +-
 lisp/eshell/esh-var.el             | 34 ++++++-----
 test/lisp/eshell/em-tramp-tests.el |  3 +-
 test/lisp/eshell/esh-cmd-tests.el  | 11 ++++
 6 files changed, 76 insertions(+), 74 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 43d536ac463..0a68859fc0a 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -415,7 +415,6 @@ eshell-parse-command
      ;; The last command (first in our reversed list) is implicitly
      ;; terminated by ";".
      (sep-terms (cons ";" sep-terms))
-     (steal-handles t)
      (commands
       (nreverse
        (mapcan
@@ -428,11 +427,8 @@ eshell-parse-command
               (unless eshell-in-pipeline-p
                 (setq cmd `(eshell-do-command ,cmd)))
               ;; Copy I/O handles so each full statement can manipulate
-              ;; them if they like.  Steal the handles for the last
-              ;; command (first in our reversed list); we won't use the
-              ;; originals again anyway.
-              (setq cmd `(eshell-with-copied-handles ,cmd ,steal-handles)
-                    steal-handles nil)
+              ;; them if they like.
+              (setq cmd `(eshell-with-copied-handles ,cmd))
               (when (equal sep "&")
                 (setq cmd `(eshell-do-subjob ,cmd)))
               (list cmd))))
@@ -547,10 +543,8 @@ eshell-rewrite-for-command
              (let ((,(intern (cadr terms)) (car ,for-items))
 		   (eshell--local-vars (cons ',(intern (cadr terms))
                                              eshell--local-vars)))
-	       (eshell-protect
-	   	,(eshell-invokify-arg body t)))
-             (setq ,for-items (cdr ,for-items)))
-           (eshell-close-handles)))))
+	       ,(eshell-invokify-arg body t))
+             (setq ,for-items (cdr ,for-items)))))))
 
 (defun eshell-structure-basic-command (func names keyword test body
 					    &optional else)
@@ -577,11 +571,8 @@ eshell-structure-basic-command
 	       (string= keyword (cadr names))))
       (setq test `(not ,test)))
 
-  ;; finally, create the form that represents this structured
-  ;; command
-  `(progn
-     (,func ,test ,body ,else)
-     (eshell-close-handles)))
+  ;; Finally, create the form that represents this structured command.
+  `(,func ,test ,body ,else))
 
 (defun eshell-rewrite-while-command (terms)
   "Rewrite a `while' command into its equivalent Eshell command form.
@@ -593,8 +584,7 @@ eshell-rewrite-while-command
       (eshell-structure-basic-command
        'while '("while" "until") (car terms)
        (eshell-invokify-arg (cadr terms) nil t)
-       `(eshell-protect
-         ,(eshell-invokify-arg (car (last terms)) t)))))
+       (eshell-invokify-arg (car (last terms)) t))))
 
 (defun eshell-rewrite-if-command (terms)
   "Rewrite an `if' command into its equivalent Eshell command form.
@@ -606,12 +596,9 @@ eshell-rewrite-if-command
       (eshell-structure-basic-command
        'if '("if" "unless") (car terms)
        (eshell-invokify-arg (cadr terms) nil t)
-       `(eshell-protect
-         ,(eshell-invokify-arg (car (last terms (if (= (length terms) 4) 2)))
-                               t))
-       (if (= (length terms) 4)
-	   `(eshell-protect
-             ,(eshell-invokify-arg (car (last terms)) t))))))
+       (eshell-invokify-arg (car (last terms (if (= (length terms) 4) 2))) t)
+       (when (= (length terms) 4)
+         (eshell-invokify-arg (car (last terms)) t)))))
 
 (defun eshell-set-exit-info (status &optional result)
   "Set the exit status and result for the last command.
@@ -662,8 +649,7 @@ eshell-parse-pipeline
       (cl-assert (car sep-terms))
       (setq final (eshell-structure-basic-command
                    'if (string= (pop sep-terms) "&&") "if"
-                   `(eshell-protect ,(pop results))
-                   `(eshell-protect ,final))))
+                   (pop results) final)))
     final))
 
 (defun eshell-parse-subcommand-argument ()
@@ -762,6 +748,28 @@ eshell-separate-commands
 ;; that `eshell-do-eval' will evaluated, such as command rewriting
 ;; hooks (see `eshell-rewrite-command-hook' and friends).
 
+(defmacro eshell-with-handles (handle-args &rest body)
+  "Create a new set of I/O handles and evaluate BODY.
+HANDLE-ARGS is a list of arguments to pass to `eshell-create-handles'.
+After evaluating BODY, automatically release the handles, allowing them
+to close."
+  (declare (indent 1))
+  `(let ((eshell-current-handles (eshell-create-handles ,@handle-args)))
+     (unwind-protect
+         ,(if (length= body 1) (car body) `(progn ,@body))
+       (eshell-close-handles))))
+
+(defmacro eshell-with-copied-handles (&rest body)
+  "Copy the current I/O handles and evaluate BODY.
+After evaluating BODY, automatically release the handles, allowing them
+to close."
+  (declare (indent 0))
+  `(let ((eshell-current-handles
+          (eshell-duplicate-handles eshell-current-handles)))
+     (unwind-protect
+         ,(if (length= body 1) (car body) `(progn ,@body))
+       (eshell-close-handles))))
+
 (defmacro eshell-do-subjob (object)
   "Evaluate a command OBJECT as a subjob.
 We indicate that the process was run in the background by
@@ -774,10 +782,9 @@ eshell-do-subjob
 
 (defmacro eshell-commands (object &optional silent)
   "Place a valid set of handles, and context, around command OBJECT."
-  `(let ((eshell-current-handles
-	  (eshell-create-handles ,(not silent) 'append))
-	 eshell-current-subjob-p)
-     ,object))
+  `(let (eshell-current-subjob-p)
+     (eshell-with-handles (,(not silent) 'append)
+       ,object)))
 
 (defvar eshell-this-command-hook nil)
 
@@ -796,8 +803,7 @@ eshell-do-command
            (mapc #'funcall eshell-this-command-hook)))
      (error
       (eshell-errorn (error-message-string err))
-      (eshell-set-exit-info 1)
-      (eshell-close-handles))))
+      (eshell-set-exit-info 1))))
 
 (define-obsolete-function-alias 'eshell-trap-errors #'eshell-do-command "31.1")
 
@@ -806,19 +812,12 @@ 'eshell-deferrable
 If the wrapped form returns a process (or list thereof), Eshell will
 wait for completion in the background for the process(es) to complete.")
 
-(defmacro eshell-with-copied-handles (object &optional steal-p)
-  "Duplicate current I/O handles, so OBJECT works with its own copy.
-If STEAL-P is non-nil, these new handles will be stolen from the
-current ones (see `eshell-duplicate-handles')."
-  `(let ((eshell-current-handles
-          (eshell-duplicate-handles eshell-current-handles ,steal-p)))
-     ,object))
-
 (define-obsolete-function-alias 'eshell-copy-handles
   #'eshell-with-copied-handles "30.1")
 
 (defmacro eshell-protect (object)
   "Protect I/O handles, so they aren't get closed after eval'ing OBJECT."
+  (declare (obsolete nil "31.1"))
   `(progn
      (eshell-protect-handles eshell-current-handles)
      ,object))
@@ -845,9 +844,7 @@ eshell-do-pipelines
            `(eshell-set-output-handle ,eshell-output-handle
                                       'append (car next-procs)))
         (let ((proc ,(car pipeline)))
-          (cons proc next-procs)))
-      ;; Steal handles if this is the last item in the pipeline.
-      ,(null (cdr pipeline)))))
+          (cons proc next-procs))))))
 
 (defmacro eshell-do-pipelines-synchronously (pipeline)
   "Execute the commands in PIPELINE in sequence synchronously.
@@ -869,9 +866,7 @@ eshell-do-pipelines-synchronously
                   ;; meaning for synchronous processes: it's non-nil
                   ;; only when piping *to* a process.
                   (eshell-in-pipeline-p ,(and (cdr pipeline) t)))
-              ,(car pipeline)))
-          ;; Steal handles if this is the last item in the pipeline.
-          ,(null (cdr pipeline)))
+              ,(car pipeline))))
        ,(when (cdr pipeline)
           `(eshell-do-pipelines-synchronously (quote ,(cdr pipeline)))))))
 
@@ -946,7 +941,7 @@ eshell--invoke-command-directly-p
 
 * The command is of the form
   (eshell-with-copied-handles
-   (eshell-do-command (eshell-named-command NAME [ARGS])) _).
+   (eshell-do-command (eshell-named-command NAME [ARGS]))).
 
 * NAME is a string referring to an alias function and isn't a
   complex command (see `eshell-complex-commands').
@@ -954,8 +949,7 @@ eshell--invoke-command-directly-p
 * Any subcommands in ARGS can also be invoked directly."
   (pcase command
     (`(eshell-with-copied-handles
-       (eshell-do-command (eshell-named-command ,name . ,args))
-       ,_)
+       (eshell-do-command (eshell-named-command ,name . ,args)))
      (and name (stringp name)
 	  (not (member name eshell-complex-commands))
 	  (catch 'simple
@@ -1557,7 +1551,7 @@ eshell-lisp-command
                     (not result))
            2)
          result))
-      (eshell-close-handles))))
+      nil)))
 
 (define-obsolete-function-alias 'eshell-lisp-command* #'eshell-lisp-command
   "31.1")
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 5fccc4fe82f..a6df75e86e9 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -353,14 +353,14 @@ eshell-create-handles
 (defun eshell-duplicate-handles (handles &optional steal-p)
   "Create a duplicate of the file handles in HANDLES.
 This uses the targets of each handle in HANDLES, incrementing its
-reference count by one (unless STEAL-P is non-nil).  These
-targets are shared between the original set of handles and the
-new one, so the targets are only closed when the reference count
-drops to 0 (see `eshell-close-handles').
+reference count by one.  These targets are shared between the original
+set of handles and the new one, so the targets are only closed when the
+reference count drops to 0 (see `eshell-close-handles').
 
 This function also sets the DEFAULT field for each handle to
 t (see `eshell-create-handles').  Unlike the targets, this value
 is not shared with the original handles."
+  (declare (advertised-calling-convention (handles) "31.1"))
   (let ((dup-handles (make-vector eshell-number-of-handles nil)))
     (dotimes (idx eshell-number-of-handles)
       (when-let ((handle (aref handles idx)))
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index dc7b497666b..b579a93e14c 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -371,6 +371,7 @@ eshell-gather-process-output
                          #'eshell-insertion-filter)
                :sentinel #'eshell-sentinel))
         (eshell-record-process-properties stderr-proc eshell-error-handle))
+      (eshell-protect-handles eshell-current-handles)
       (setq proc
             (let ((command (file-local-name (expand-file-name command)))
                   (conn-type (pcase (bound-and-true-p eshell-in-pipeline-p)
@@ -468,7 +469,6 @@ eshell-gather-process-output
         (eshell-set-exit-info
          (if (numberp exit-status) exit-status -1)
          (and (numberp exit-status) (= exit-status 0)))
-        (eshell-close-handles)
 	(run-hook-with-args 'eshell-kill-hook command exit-status)
 	(or (bound-and-true-p eshell-in-pipeline-p)
 	    (setq eshell-last-sync-output-start nil))
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index f0270aca92c..0e6c01d2774 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -553,24 +553,22 @@ eshell-parse-variable-ref
              (subcmd (or (eshell-unescape-inner-double-quote end)
                          (cons (point) end))))
         (prog1
-            `(let ((eshell-current-handles
-                    (eshell-create-handles ,temp 'overwrite)))
-               (progn
-                 (eshell-as-subcommand
-                  ,(let ((eshell-current-quoted nil))
-                     (eshell-parse-command subcmd)))
-                 (ignore
-                  (nconc eshell-this-command-hook
-                         ;; Quote this lambda; it will be evaluated by
-                         ;; `eshell-do-eval', which requires very
-                         ;; particular forms in order to work
-                         ;; properly.  See bug#54190.
-                         (list (function
-                                (lambda ()
-                                  (delete-file ,temp)
-                                  (when-let ((buffer (get-file-buffer ,temp)))
-                                    (kill-buffer buffer)))))))
-                 (eshell-apply-indices ,temp indices ,eshell-current-quoted)))
+            `(eshell-with-handles (,temp 'overwrite)
+               (eshell-as-subcommand
+                ,(let ((eshell-current-quoted nil))
+                   (eshell-parse-command subcmd)))
+               (ignore
+                (nconc eshell-this-command-hook
+                       ;; Quote this lambda; it will be evaluated by
+                       ;; `eshell-do-eval', which requires very
+                       ;; particular forms in order to work
+                       ;; properly.  See bug#54190.
+                       (list (function
+                              (lambda ()
+                                (delete-file ,temp)
+                                (when-let ((buffer (get-file-buffer ,temp)))
+                                  (kill-buffer buffer)))))))
+               (eshell-apply-indices ,temp indices ,eshell-current-quoted))
           (goto-char (1+ end))))))
    ((eq (char-after) ?\()
     (condition-case nil
diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el
index 49dd5a78c3d..b85dfc61580 100644
--- a/test/lisp/eshell/em-tramp-tests.el
+++ b/test/lisp/eshell/em-tramp-tests.el
@@ -29,8 +29,7 @@ em-tramp-test/should-replace-command
   `(should (equal
             (catch 'eshell-replace-command ,form)
             (list 'eshell-with-copied-handles
-                  (list 'eshell-do-command ,replacement)
-                  t))))
+                  (list 'eshell-do-command ,replacement)))))
 
 (ert-deftest em-tramp-test/su-default ()
   "Test Eshell `su' command with no arguments."
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index 18ea1f9a9d6..cac349a2616 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -558,6 +558,17 @@ esh-cmd-test/throw
    ;; Make sure we can call another command after throwing.
    (eshell-match-command-output "echo again" "\\`again\n")))
 
+(ert-deftest esh-cmd-test/command-not-found/pipeline ()
+  "Ensure that processes are stopped if a command in a pipeline is not found."
+  (skip-when (or (not (executable-find "cat"))
+                 (executable-find "nonexist")))
+  (with-temp-eshell
+    (let ((starting-process-list (process-list)))
+      (eshell-match-command-output "nonexist | *cat"
+                                   "\\`nonexist: command not found\n")
+      (eshell-wait-for-subprocess t)
+      (should (equal (process-list) starting-process-list)))))
+
 \f
 ;; `which' command
 
-- 
2.25.1