* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell
@ 2022-08-11 2:43 Jim Porter
2022-08-11 2:46 ` Jim Porter
0 siblings, 1 reply; 57+ messages in thread
From: Jim Porter @ 2022-08-11 2:43 UTC (permalink / raw)
To: 57129
In Eshell, you can use conditionals pretty much like you'd expect from
other shells. Starting from 'emacs -Q -f eshell':
~ $ [ foo = foo ] && echo hi
hi
~ $ [ foo = bar ] && echo hi
~ $ [ foo = foo ] || echo hi
~ $ [ foo = bar ] || echo hi
hi
~ $ if {[ foo = foo ]} {echo yes} {echo no}
yes
~ $ if {[ foo = bar ]} {echo yes} {echo no}
no
However, that only works for external commands. If the command is
implemented in Lisp, it doesn't work:
~ $ (zerop 0) && echo hi
t
hi
~ $ (zerop 1) && echo hi
hi ;; Shouldn't say hi.
That's because the exit status is always 0 for Lisp commands. This works
correctly for external commands:
~ $ [ foo = foo ]; echo status $? result $$
("status" 0 "result" nil)
~ $ [ foo = bar ]; echo status $? result $$
("status" 1 "result" nil)
But not for Lisp commands:
~ $ (zerop 0); echo status $? result $$
t
("status" 0 "result" t)
~ $ (zerop 1); echo status $? result $$
("status" 0 "result" nil)
~ $ (zerop "foo"); echo status $? result $$
Wrong type argument: number-or-marker-p, "foo"
("status" 0 "result" nil)
The manual says that the status should be 1 when a Lisp command fails,
but it's 0 no matter what, even if it signaled an error. (Likewise, the
manual says that the "result" variable should be t for successful
external commands, but it's always nil.)
Similarly to the above, you can't use variable expansions for conditionals:
~ $ (setq foo t)
t
~ $ if $foo {echo yes} {echo no}
yes
~ $ (setq foo nil)
~ $ if $foo {echo yes} {echo no}
yes ;; Should say no.
Patch forthcoming. Just splitting it into two messages since it seemed
more readable that way...
^ permalink raw reply [flat|nested] 57+ messages in thread* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-11 2:43 bug#57129: 29.0.50; Improve behavior of conditionals in Eshell Jim Porter @ 2022-08-11 2:46 ` Jim Porter 2022-08-12 15:22 ` Lars Ingebrigtsen 0 siblings, 1 reply; 57+ messages in thread From: Jim Porter @ 2022-08-11 2:46 UTC (permalink / raw) To: 57129 [-- Attachment #1: Type: text/plain, Size: 1686 bytes --] Here are some patches to fix this. The first patch adds tests/documentation for the current state so that the subsequent patches are clearer. It also improves 'eshell-close-handles' so that you can set the exit status elsewhere, which makes some of the code simpler. The second patch fixes the use of variables in conditionals (e.g. if statements, while loops). The only non-test code change here is that 'eshell-structure-basic-command' needed to be taught that forms beginning with 'eshell-escape-arg' should be treated as data, much like 'eshell-convert'. The third patch fixes the behavior of the '$?' and '$$' variables to match the manual: '$$' is t when an external command succeeds, and '$?' is 1 when a Lisp command signals an error. I also added a new feature that '$?' is 2 when an actual Lisp *form* returns nil. This lets you use Lisp forms as conditionals in Eshell much like you would in regular Lisp. However, it doesn't apply to the command syntax, even if it runs Lisp code: ~ $ (zerop 1); echo $? 2 ~ $ zerop 1; echo $? 0 That's because Eshell prints the return value of Lisp commands (unless it's nil), so a Lisp command that wants to silently succeed would return nil. For example, the Lisp version of 'cat' returns nil. We want that to have an exit status of 0. However, when writing it as a Lisp form with parentheses, I think it makes more sense that nil is treated as false for conditionals. The overall goal is so that this prints hi: cat foo.txt && echo hi And that this doesn't: (zerop 1) && echo hi For people who don't like this behavior, they can customize the new 'eshell-lisp-form-nil-is-failure' variable. [-- Attachment #2: 0001-Only-set-Eshell-execution-result-metavariables-when-.patch --] [-- Type: text/plain, Size: 20211 bytes --] From 17d15df2368a2b078e90d39cb8018e274e422602 Mon Sep 17 00:00:00 2001 From: Jim Porter <jporterbugs@gmail.com> Date: Sat, 6 Aug 2022 13:37:28 -0700 Subject: [PATCH 1/3] Only set Eshell execution result metavariables when non-nil This simplifies usage of 'eshell-close-handles' in several places and makes it work more like the docstring indicated it would. * lisp/eshell/esh-io.el (eshell-close-handles): Only store EXIT-CODE and RESULT if they're non-nil. Also, use 'dotimes' and 'dolist' to simplify the implementation. * lisp/eshell/em-alias.el (eshell-write-aliases-list): * lisp/eshell/esh-cmd.el (eshell-rewrite-for-command) (eshell-structure-basic-command): Adapt calls to 'eshell-close-handles'. * test/lisp/eshell/eshell-tests.el (eshell-test/simple-command-result) (eshell-test/lisp-command, eshell-test/lisp-command-with-quote) (eshell-test/for-loop, eshell-test/for-name-loop) (eshell-test/for-name-shadow-loop, eshell-test/lisp-command-args) (eshell-test/subcommand, eshell-test/subcommand-args) (eshell-test/subcommand-lisp): Move from here... * test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/simple-command-result, esh-cmd-test/lisp-command) (esh-cmd-test/lisp-command-with-quote, esh-cmd-test/for-loop) (esh-cmd-test/for-name-loop, esh-cmd-test/for-name-shadow-loop) (esh-cmd-test/lisp-command-args, esh-cmd-test/subcommand) (esh-cmd-test/subcommand-args, esh-cmd-test/subcommand-lisp): ... to here. (esh-cmd-test/and-operator, esh-cmd-test/or-operator) (esh-cmd-test/for-loop-list, esh-cmd-test/for-loop-multiple-args) (esh-cmd-test/while-loop, esh-cmd-test/until-loop) (esh-cmd-test/if-statement, esh-cmd-test/if-else-statement) (esh-cmd-test/unless-statement, esh-cmd-test/unless-else-statement): New tests. * doc/misc/eshell.texi (Invocation): Explain '&&' and '||'. (for loop): Move from here... (Control Flow): ... to here, and add documentation for other control flow forms. --- doc/misc/eshell.texi | 62 +++++++--- lisp/eshell/em-alias.el | 2 +- lisp/eshell/esh-cmd.el | 8 +- lisp/eshell/esh-io.el | 50 ++++---- test/lisp/eshell/esh-cmd-tests.el | 189 ++++++++++++++++++++++++++++++ test/lisp/eshell/eshell-tests.el | 53 --------- 6 files changed, 261 insertions(+), 103 deletions(-) create mode 100644 test/lisp/eshell/esh-cmd-tests.el diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi index 9f9c88582f..d643cb5096 100644 --- a/doc/misc/eshell.texi +++ b/doc/misc/eshell.texi @@ -201,7 +201,7 @@ Commands * Aliases:: * History:: * Completion:: -* for loop:: +* Control Flow:: * Scripts:: @end menu @@ -219,12 +219,18 @@ Invocation external command. The semicolon (@code{;}) can be used to separate multiple command -invocations on a single line. A command invocation followed by an -ampersand (@code{&}) will be run in the background. Eshell has no job -control, so you can not suspend or background the current process, or -bring a background process into the foreground. That said, background -processes invoked from Eshell can be controlled the same way as any -other background process in Emacs. +invocations on a single line. You can also separate commands with +@code{&&} or @code{||}. When using @code{&&}, Eshell will execute the +second command only if the first succeeds (i.e.@: has an exit +status of 0); with @code{||}, Eshell will execute the second command +only if the first fails. + +A command invocation followed by an ampersand (@code{&}) will be run +in the background. Eshell has no job control, so you can not suspend +or background the current process, or bring a background process into +the foreground. That said, background processes invoked from Eshell +can be controlled the same way as any other background process in +Emacs. @node Arguments @section Arguments @@ -1008,19 +1014,41 @@ Completion the function @code{pcomplete/MAJOR-MODE/COMMAND} to define completions for a specific major mode. -@node for loop -@section @code{for} loop +@node Control Flow +@section Control Flow Because Eshell commands can not (easily) be combined with lisp forms, -Eshell provides a command-oriented @command{for}-loop for convenience. -The syntax is as follows: +Eshell provides command-oriented control flow statements for +convenience. -@example -@code{for VAR in TOKENS @{ command invocation(s) @}} -@end example +@table @code + +@item if @{ @var{conditional} @} @{ @var{true-commands} @} +@itemx if @{ @var{conditional} @} @{ @var{true-commands} @} @{ @var{false-commands} @} +Evaluate @var{true-commands} if @var{conditional} returns success +(i.e.@: its exit code is zero); otherwise, evaluate +@var{false-commands}. + +@item unless @{ @var{conditional} @} @{ @var{false-commands} @} +@itemx unless @{ @var{conditional} @} @{ @var{false-commands} @} @{ @var{true-commands} @} +Evaluate @var{false-commands} if @var{conditional} returns failure +(i.e.@: its exit code is non-zero); otherwise, evaluate +@var{true-commands}. -where @samp{TOKENS} is a space-separated sequence of values of -@var{VAR} for each iteration. This can even be the output of a -command if @samp{TOKENS} is replaced with @samp{@{ command invocation @}}. +@item while @{ @var{conditional} @} @{ @var{commands} @} +Repeatedly evaluate @var{commands} so long as @var{conditional} +returns success. + +@item until @{ @var{conditional} @} @{ @var{commands} @} +Repeatedly evaluate @var{commands} so long as @var{conditional} +returns failure. + +@item for @var{var} in @var{list}@dots{} @{ @var{commands} @} +Iterate over each element of of @var{list}, storing the element in +@var{var} and evaluating @var{commands}. If @var{list} is not a list, +treat it as a list of one element. If you specify multiple +@var{lists}, this will iterate over each of them in turn. + +@end table @node Scripts @section Scripts diff --git a/lisp/eshell/em-alias.el b/lisp/eshell/em-alias.el index 5d3aaf7c81..9ad218d598 100644 --- a/lisp/eshell/em-alias.el +++ b/lisp/eshell/em-alias.el @@ -206,7 +206,7 @@ eshell-write-aliases-list (let ((eshell-current-handles (eshell-create-handles eshell-aliases-file 'overwrite))) (eshell/alias) - (eshell-close-handles 0)))) + (eshell-close-handles 0 'nil)))) (defsubst eshell-lookup-alias (name) "Check whether NAME is aliased. Return the alias if there is one." diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 775e4c1057..96272ca1a3 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -541,9 +541,7 @@ eshell-rewrite-for-command ,(eshell-invokify-arg body t))) (setcar for-items (cadr for-items)) (setcdr for-items (cddr for-items))) - (eshell-close-handles - eshell-last-command-status - (list 'quote eshell-last-command-result)))))) + (eshell-close-handles))))) (defun eshell-structure-basic-command (func names keyword test body &optional else) @@ -574,9 +572,7 @@ eshell-structure-basic-command `(let ((eshell-command-body '(nil)) (eshell-test-body '(nil))) (,func ,test ,body ,else) - (eshell-close-handles - eshell-last-command-status - (list 'quote eshell-last-command-result)))) + (eshell-close-handles))) (defun eshell-rewrite-while-command (terms) "Rewrite a `while' command into its equivalent Eshell command form. diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el index 68e52a2c9c..27703976f6 100644 --- a/lisp/eshell/esh-io.el +++ b/lisp/eshell/esh-io.el @@ -254,6 +254,30 @@ eshell-protect-handles (setq idx (1+ idx)))) handles) +(defun eshell-close-handles (&optional exit-code result handles) + "Close all of the current HANDLES, taking refcounts into account. +If HANDLES is nil, use `eshell-current-handles'. + +EXIT-CODE is the process exit code (zero, if the command +completed successfully). If nil, then use the exit code already +set in `eshell-last-command-status'. + +RESULT is the quoted value of the last command. If nil, then use +the value already set in `eshell-last-command-result'." + (when exit-code + (setq eshell-last-command-status exit-code)) + (when result + (cl-assert (eq (car result) 'quote)) + (setq eshell-last-command-result (cadr result))) + (let ((handles (or handles eshell-current-handles))) + (dotimes (idx eshell-number-of-handles) + (when-let ((handle (aref handles idx))) + (setcdr handle (1- (cdr handle))) + (when (= (cdr handle) 0) + (dolist (target (ensure-list (car (aref handles idx)))) + (eshell-close-target target (= eshell-last-command-status 0))) + (setcar handle nil)))))) + (defun eshell-close-target (target status) "Close an output TARGET, passing STATUS as the result. STATUS should be non-nil on successful termination of the output." @@ -305,32 +329,6 @@ eshell-close-target ((consp target) (apply (car target) status (cdr target))))) -(defun eshell-close-handles (exit-code &optional result handles) - "Close all of the current handles, taking refcounts into account. -EXIT-CODE is the process exit code; mainly, it is zero, if the command -completed successfully. RESULT is the quoted value of the last -command. If nil, then the meta variables for keeping track of the -last execution result should not be changed." - (let ((idx 0)) - (cl-assert (or (not result) (eq (car result) 'quote))) - (setq eshell-last-command-status exit-code - eshell-last-command-result (cadr result)) - (while (< idx eshell-number-of-handles) - (let ((handles (or handles eshell-current-handles))) - (when (aref handles idx) - (setcdr (aref handles idx) - (1- (cdr (aref handles idx)))) - (when (= (cdr (aref handles idx)) 0) - (let ((target (car (aref handles idx)))) - (if (not (listp target)) - (eshell-close-target target (= exit-code 0)) - (while target - (eshell-close-target (car target) (= exit-code 0)) - (setq target (cdr target))))) - (setcar (aref handles idx) nil)))) - (setq idx (1+ idx))) - nil)) - (defun eshell-kill-append (string) "Call `kill-append' with STRING, if it is indeed a string." (if (stringp string) diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el new file mode 100644 index 0000000000..1d5cd29d7c --- /dev/null +++ b/test/lisp/eshell/esh-cmd-tests.el @@ -0,0 +1,189 @@ +;;; esh-cmd-tests.el --- esh-cmd test suite -*- lexical-binding:t -*- + +;; Copyright (C) 2022 Free Software Foundation, Inc. + +;; This file is part of GNU Emacs. + +;; GNU Emacs is free software: you can redistribute it and/or modify +;; it under the terms of the GNU General Public License as published by +;; the Free Software Foundation, either version 3 of the License, or +;; (at your option) any later version. + +;; GNU Emacs is distributed in the hope that it will be useful, +;; but WITHOUT ANY WARRANTY; without even the implied warranty of +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;; GNU General Public License for more details. + +;; You should have received a copy of the GNU General Public License +;; along with GNU Emacs. If not, see <https://www.gnu.org/licenses/>. + +;;; Commentary: + +;; Tests for Eshell's command invocation. + +;;; Code: + +(require 'ert) +(require 'esh-mode) +(require 'eshell) + +(require 'eshell-tests-helpers + (expand-file-name "eshell-tests-helpers" + (file-name-directory (or load-file-name + default-directory)))) + +(defvar eshell-test-value nil) + +;;; Tests: + +\f +;; Command invocation + +(ert-deftest esh-cmd-test/simple-command-result () + "Test invocation with a simple command." + (should (equal (eshell-test-command-result "+ 1 2") 3))) + +(ert-deftest esh-cmd-test/lisp-command () + "Test invocation with an elisp command." + (should (equal (eshell-test-command-result "(+ 1 2)") 3))) + +(ert-deftest esh-cmd-test/lisp-command-with-quote () + "Test invocation with an elisp command containing a quote." + (should (equal (eshell-test-command-result "(eq 'foo nil)") nil))) + +(ert-deftest esh-cmd-test/lisp-command-args () + "Test invocation with elisp and trailing args. +Test that trailing arguments outside the S-expression are +ignored. e.g. \"(+ 1 2) 3\" => 3" + (should (equal (eshell-test-command-result "(+ 1 2) 3") 3))) + +(ert-deftest esh-cmd-test/subcommand () + "Test invocation with a simple subcommand." + (should (equal (eshell-test-command-result "{+ 1 2}") 3))) + +(ert-deftest esh-cmd-test/subcommand-args () + "Test invocation with a subcommand and trailing args. +Test that trailing arguments outside the subcommand are ignored. +e.g. \"{+ 1 2} 3\" => 3" + (should (equal (eshell-test-command-result "{+ 1 2} 3") 3))) + +(ert-deftest esh-cmd-test/subcommand-lisp () + "Test invocation with an elisp subcommand and trailing args. +Test that trailing arguments outside the subcommand are ignored. +e.g. \"{(+ 1 2)} 3\" => 3" + (should (equal (eshell-test-command-result "{(+ 1 2)} 3") 3))) + +\f +;; Logical operators + +(ert-deftest esh-cmd-test/and-operator () + "Test logical && operator." + (skip-unless (executable-find "[")) + (with-temp-eshell + (eshell-command-result-p "[ foo = foo ] && echo hi" + "hi\n") + (eshell-command-result-p "[ foo = bar ] && echo hi" + "\\`\\'"))) + +(ert-deftest esh-cmd-test/or-operator () + "Test logical || operator." + (skip-unless (executable-find "[")) + (with-temp-eshell + (eshell-command-result-p "[ foo = foo ] || echo hi" + "\\`\\'") + (eshell-command-result-p "[ foo = bar ] || echo hi" + "hi\n"))) + +\f +;; Control flow statements + +(ert-deftest esh-cmd-test/for-loop () + "Test invocation of a for loop." + (with-temp-eshell + (eshell-command-result-p "for i in 5 { echo $i }" + "5\n"))) + +(ert-deftest esh-cmd-test/for-loop-list () + "Test invocation of a for loop iterating over a list." + (with-temp-eshell + (eshell-command-result-p "for i in (list 1 2 (list 3 4)) { echo $i }" + "1\n2\n(3 4)\n"))) + +(ert-deftest esh-cmd-test/for-loop-multiple-args () + "Test invocation of a for loop iterating over multiple arguments." + (with-temp-eshell + (eshell-command-result-p "for i in 1 2 (list 3 4) { echo $i }" + "1\n2\n3\n4\n"))) + +(ert-deftest esh-cmd-test/for-name-loop () ; bug#15231 + "Test invocation of a for loop using `name'." + (let ((process-environment (cons "name" process-environment))) + (should (equal (eshell-test-command-result + "for name in 3 { echo $name }") + 3)))) + +(ert-deftest esh-cmd-test/for-name-shadow-loop () ; bug#15372 + "Test invocation of a for loop using an env-var." + (let ((process-environment (cons "name=env-value" process-environment))) + (with-temp-eshell + (eshell-command-result-p + "echo $name; for name in 3 { echo $name }; echo $name" + "env-value\n3\nenv-value\n")))) + +(ert-deftest esh-cmd-test/while-loop () + "Test invocation of a while loop." + (skip-unless (executable-find "[")) + (with-temp-eshell + (let ((eshell-test-value 0)) + (eshell-command-result-p + (concat "while {[ $eshell-test-value -ne 3 ]} " + "{ setq eshell-test-value (1+ eshell-test-value) }") + "1\n2\n3\n")))) + +(ert-deftest esh-cmd-test/until-loop () + "Test invocation of an until loop." + (skip-unless (executable-find "[")) + (with-temp-eshell + (let ((eshell-test-value 0)) + (eshell-command-result-p + (concat "until {[ $eshell-test-value -eq 3 ]} " + "{ setq eshell-test-value (1+ eshell-test-value) }") + "1\n2\n3\n")))) + +(ert-deftest esh-cmd-test/if-statement () + "Test invocation of an if statement." + (skip-unless (executable-find "[")) + (with-temp-eshell + (eshell-command-result-p "if {[ foo = foo ]} {echo yes}" + "yes\n") + (eshell-command-result-p "if {[ foo = bar ]} {echo yes}" + "\\`\\'"))) + +(ert-deftest esh-cmd-test/if-else-statement () + "Test invocation of an if/else statement." + (skip-unless (executable-find "[")) + (with-temp-eshell + (eshell-command-result-p "if {[ foo = foo ]} {echo yes} {echo no}" + "yes\n") + (eshell-command-result-p "if {[ foo = bar ]} {echo yes} {echo no}" + "no\n"))) + +(ert-deftest esh-cmd-test/unless-statement () + "Test invocation of an unless statement." + (skip-unless (executable-find "[")) + (with-temp-eshell + (eshell-command-result-p "unless {[ foo = foo ]} {echo no}" + "\\`\\'") + (eshell-command-result-p "unless {[ foo = bar ]} {echo no}" + "no\n"))) + +(ert-deftest esh-cmd-test/unless-else-statement () + "Test invocation of an unless/else statement." + (skip-unless (executable-find "[")) + (with-temp-eshell + (eshell-command-result-p "unless {[ foo = foo ]} {echo no} {echo yes}" + "yes\n") + (eshell-command-result-p "unless {[ foo = bar ]} {echo no} {echo yes}" + "no\n"))) + +;; esh-cmd-tests.el ends here diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el index 5dc1877548..8423500ea7 100644 --- a/test/lisp/eshell/eshell-tests.el +++ b/test/lisp/eshell/eshell-tests.el @@ -36,59 +36,6 @@ ;;; Tests: -(ert-deftest eshell-test/simple-command-result () - "Test `eshell-command-result' with a simple command." - (should (equal (eshell-test-command-result "+ 1 2") 3))) - -(ert-deftest eshell-test/lisp-command () - "Test `eshell-command-result' with an elisp command." - (should (equal (eshell-test-command-result "(+ 1 2)") 3))) - -(ert-deftest eshell-test/lisp-command-with-quote () - "Test `eshell-command-result' with an elisp command containing a quote." - (should (equal (eshell-test-command-result "(eq 'foo nil)") nil))) - -(ert-deftest eshell-test/for-loop () - "Test `eshell-command-result' with a for loop.." - (let ((process-environment (cons "foo" process-environment))) - (should (equal (eshell-test-command-result - "for foo in 5 { echo $foo }") 5)))) - -(ert-deftest eshell-test/for-name-loop () ;Bug#15231 - "Test `eshell-command-result' with a for loop using `name'." - (let ((process-environment (cons "name" process-environment))) - (should (equal (eshell-test-command-result - "for name in 3 { echo $name }") 3)))) - -(ert-deftest eshell-test/for-name-shadow-loop () ; bug#15372 - "Test `eshell-command-result' with a for loop using an env-var." - (let ((process-environment (cons "name=env-value" process-environment))) - (with-temp-eshell - (eshell-command-result-p "echo $name; for name in 3 { echo $name }; echo $name" - "env-value\n3\nenv-value\n")))) - -(ert-deftest eshell-test/lisp-command-args () - "Test `eshell-command-result' with elisp and trailing args. -Test that trailing arguments outside the S-expression are -ignored. e.g. \"(+ 1 2) 3\" => 3" - (should (equal (eshell-test-command-result "(+ 1 2) 3") 3))) - -(ert-deftest eshell-test/subcommand () - "Test `eshell-command-result' with a simple subcommand." - (should (equal (eshell-test-command-result "{+ 1 2}") 3))) - -(ert-deftest eshell-test/subcommand-args () - "Test `eshell-command-result' with a subcommand and trailing args. -Test that trailing arguments outside the subcommand are ignored. -e.g. \"{+ 1 2} 3\" => 3" - (should (equal (eshell-test-command-result "{+ 1 2} 3") 3))) - -(ert-deftest eshell-test/subcommand-lisp () - "Test `eshell-command-result' with an elisp subcommand and trailing args. -Test that trailing arguments outside the subcommand are ignored. -e.g. \"{(+ 1 2)} 3\" => 3" - (should (equal (eshell-test-command-result "{(+ 1 2)} 3") 3))) - (ert-deftest eshell-test/pipe-headproc () "Check that piping a non-process to a process command waits for the process" (skip-unless (executable-find "cat")) -- 2.25.1 [-- Attachment #3: 0002-Allow-using-dollar-expansions-in-Eshell-conditionals.patch --] [-- Type: text/plain, Size: 8535 bytes --] From ffc7688370f852d7425b7c67e6a53cf94a3e4208 Mon Sep 17 00:00:00 2001 From: Jim Porter <jporterbugs@gmail.com> Date: Mon, 8 Aug 2022 21:24:27 -0700 Subject: [PATCH 2/3] Allow using dollar expansions in Eshell conditionals * lisp/eshell/esh-cmd.el (eshell-structure-basic-command): Forms beginning with 'eshell-escape-arg' are "data-wise". * test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/while-loop) (esh-cmd-test/until-loop, esh-cmd-test/if-statement) (esh-cmd-test/if-else-statement, esh-cmd-test/unless-statement) (esh-cmd-test/unless-else-statement): Use variable interpolation. (esh-cmd-test/while-loop-ext-cmd, esh-cmd-test/until-loop-ext-cmd) (esh-cmd-test/if-else-statement-ext-cmd) (esh-cmd-test/unless-else-statement-ext-cmd): New tests, adapted from the existing ones. * doc/misc/eshell.texi (Control Flow): Update documentation for conditionals (bug#57129). --- doc/misc/eshell.texi | 43 ++++++++++++---------- lisp/eshell/esh-cmd.el | 9 ++--- test/lisp/eshell/esh-cmd-tests.el | 60 +++++++++++++++++++++++++------ 3 files changed, 79 insertions(+), 33 deletions(-) diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi index d643cb5096..141c30ae9b 100644 --- a/doc/misc/eshell.texi +++ b/doc/misc/eshell.texi @@ -1020,27 +1020,32 @@ Control Flow Eshell provides command-oriented control flow statements for convenience. +Most of Eshell's control flow statements accept a @var{conditional}. +This can take a few different forms. If @var{conditional} is a dollar +expansion, the condition is satisfied if the result is a +non-@code{nil} value. If @var{conditional} is a @samp{@{ +@var{subcommand} @}}, the condition is satisfied if the +@var{subcommand}'s exit status is 0. + @table @code -@item if @{ @var{conditional} @} @{ @var{true-commands} @} -@itemx if @{ @var{conditional} @} @{ @var{true-commands} @} @{ @var{false-commands} @} -Evaluate @var{true-commands} if @var{conditional} returns success -(i.e.@: its exit code is zero); otherwise, evaluate -@var{false-commands}. - -@item unless @{ @var{conditional} @} @{ @var{false-commands} @} -@itemx unless @{ @var{conditional} @} @{ @var{false-commands} @} @{ @var{true-commands} @} -Evaluate @var{false-commands} if @var{conditional} returns failure -(i.e.@: its exit code is non-zero); otherwise, evaluate -@var{true-commands}. - -@item while @{ @var{conditional} @} @{ @var{commands} @} -Repeatedly evaluate @var{commands} so long as @var{conditional} -returns success. - -@item until @{ @var{conditional} @} @{ @var{commands} @} -Repeatedly evaluate @var{commands} so long as @var{conditional} -returns failure. +@item if @var{conditional} @{ @var{true-commands} @} +@itemx if @var{conditional} @{ @var{true-commands} @} @{ @var{false-commands} @} +Evaluate @var{true-commands} if @var{conditional} is satisfied; +otherwise, evaluate @var{false-commands}. + +@item unless @var{conditional} @{ @var{false-commands} @} +@itemx unless @var{conditional} @{ @var{false-commands} @} @{ @var{true-commands} @} +Evaluate @var{false-commands} if @var{conditional} is not satisfied; +otherwise, evaluate @var{true-commands}. + +@item while @var{conditional} @{ @var{commands} @} +Repeatedly evaluate @var{commands} so long as @var{conditional} is +satisfied. + +@item until @var{conditional} @{ @var{commands} @} +Repeatedly evaluate @var{commands} until @var{conditional} is +satisfied. @item for @var{var} in @var{list}@dots{} @{ @var{commands} @} Iterate over each element of of @var{list}, storing the element in diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 96272ca1a3..454a90e91d 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -549,10 +549,11 @@ eshell-structure-basic-command The first of NAMES should be the positive form, and the second the negative. It's not likely that users should ever need to call this function." - ;; If the test form begins with `eshell-convert', it means - ;; something data-wise will be returned, and we should let - ;; that determine the truth of the statement. - (unless (eq (car test) 'eshell-convert) + ;; If the test form begins with `eshell-convert' or + ;; `eshell-escape-arg', it means something data-wise will be + ;; returned, and we should let that determine the truth of the + ;; statement. + (unless (memq (car test) '(eshell-convert eshell-escape-arg)) (setq test `(progn ,test (eshell-exit-success-p)))) diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el index 1d5cd29d7c..b31159a1a8 100644 --- a/test/lisp/eshell/esh-cmd-tests.el +++ b/test/lisp/eshell/esh-cmd-tests.el @@ -132,6 +132,15 @@ esh-cmd-test/for-name-shadow-loop (ert-deftest esh-cmd-test/while-loop () "Test invocation of a while loop." + (with-temp-eshell + (let ((eshell-test-value '(0 1 2))) + (eshell-command-result-p + (concat "while $eshell-test-value " + "{ setq eshell-test-value (cdr eshell-test-value) }") + "(1 2)\n(2)\n")))) + +(ert-deftest esh-cmd-test/while-loop-ext-cmd () + "Test invocation of a while loop using an external command." (skip-unless (executable-find "[")) (with-temp-eshell (let ((eshell-test-value 0)) @@ -142,6 +151,15 @@ esh-cmd-test/while-loop (ert-deftest esh-cmd-test/until-loop () "Test invocation of an until loop." + (with-temp-eshell + (let ((eshell-test-value nil)) + (eshell-command-result-p + (concat "until $eshell-test-value " + "{ setq eshell-test-value t }") + "t\n")))) + +(ert-deftest esh-cmd-test/until-loop-ext-cmd () + "Test invocation of an until loop using an external command." (skip-unless (executable-find "[")) (with-temp-eshell (let ((eshell-test-value 0)) @@ -152,15 +170,26 @@ esh-cmd-test/until-loop (ert-deftest esh-cmd-test/if-statement () "Test invocation of an if statement." - (skip-unless (executable-find "[")) (with-temp-eshell - (eshell-command-result-p "if {[ foo = foo ]} {echo yes}" - "yes\n") - (eshell-command-result-p "if {[ foo = bar ]} {echo yes}" - "\\`\\'"))) + (let ((eshell-test-value t)) + (eshell-command-result-p "if $eshell-test-value {echo yes}" + "yes\n")) + (let ((eshell-test-value nil)) + (eshell-command-result-p "if $eshell-test-value {echo yes}" + "\\`\\'")))) (ert-deftest esh-cmd-test/if-else-statement () "Test invocation of an if/else statement." + (with-temp-eshell + (let ((eshell-test-value t)) + (eshell-command-result-p "if $eshell-test-value {echo yes} {echo no}" + "yes\n")) + (let ((eshell-test-value nil)) + (eshell-command-result-p "if $eshell-test-value {echo yes} {echo no}" + "no\n")))) + +(ert-deftest esh-cmd-test/if-else-statement-ext-cmd () + "Test invocation of an if/else statement using an external command." (skip-unless (executable-find "[")) (with-temp-eshell (eshell-command-result-p "if {[ foo = foo ]} {echo yes} {echo no}" @@ -170,15 +199,26 @@ esh-cmd-test/if-else-statement (ert-deftest esh-cmd-test/unless-statement () "Test invocation of an unless statement." - (skip-unless (executable-find "[")) (with-temp-eshell - (eshell-command-result-p "unless {[ foo = foo ]} {echo no}" - "\\`\\'") - (eshell-command-result-p "unless {[ foo = bar ]} {echo no}" - "no\n"))) + (let ((eshell-test-value t)) + (eshell-command-result-p "unless $eshell-test-value {echo no}" + "\\`\\'")) + (let ((eshell-test-value nil)) + (eshell-command-result-p "unless $eshell-test-value {echo no}" + "no\n")))) (ert-deftest esh-cmd-test/unless-else-statement () "Test invocation of an unless/else statement." + (with-temp-eshell + (let ((eshell-test-value t)) + (eshell-command-result-p "unless $eshell-test-value {echo no} {echo yes}" + "yes\n")) + (let ((eshell-test-value nil)) + (eshell-command-result-p "unless $eshell-test-value {echo no} {echo yes}" + "no\n")))) + +(ert-deftest esh-cmd-test/unless-else-statement-ext-cmd () + "Test invocation of an unless/else statement using an external command." (skip-unless (executable-find "[")) (with-temp-eshell (eshell-command-result-p "unless {[ foo = foo ]} {echo no} {echo yes}" -- 2.25.1 [-- Attachment #4: 0003-Make-and-variables-more-consistent-in-Eshell.patch --] [-- Type: text/plain, Size: 20218 bytes --] From 8233c6f41415e7b4379c652bd6b6ba26c514dd61 Mon Sep 17 00:00:00 2001 From: Jim Porter <jporterbugs@gmail.com> Date: Tue, 9 Aug 2022 20:09:57 -0700 Subject: [PATCH 3/3] Make '$?' and '$$' variables more consistent in Eshell Previously, '$?' (last exit code) was only useful for external commands, and '$$' (last result) was only useful for Lisp commands. * lisp/eshell/esh-cmd.el (eshell-lisp-form-nil-is-failure): New option. (eshell-lisp-command): Set last exit code to 1 when the command signals an error, and 2 if it returns nil (for Lisp forms only). * lisp/eshell/esh-proc.el (eshell-sentinel): Set last result to t if the command succeeded. * test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/while-loop-lisp-form, esh-cmd-test/until-loop-lisp-form) (esh-cmd-test/if-else-statement-lisp-form) (esh-cmd-test/if-else-statement-lisp-form-2) (esh-cmd-test/unless-else-statement-lisp-form): New tests. * test/lisp/eshell/esh-var-tests.el (esh-var-test/last-status-var-lisp-command) (esh-var-test/last-status-var-lisp-form) (esh-var-test/last-status-var-lisp-form-2) (esh-var-test/last-status-var-ext-cmd) (esh-var-test/last-status-var-ext-cmd): New tests. (esh-var-test/last-result-var2): Rename from this... ( esh-var-test/last-result-var-twice): ... to this. * doc/misc/eshell.texi (Variables): Update documentation about '$?' and '$$'. (Control Flow): Mention that '(lisp forms)' can be used as conditionals. * etc/NEWS: Announce this change (bug#57129). --- doc/misc/eshell.texi | 14 ++++--- etc/NEWS | 7 ++++ lisp/eshell/esh-cmd.el | 68 +++++++++++++++++++------------ lisp/eshell/esh-proc.el | 68 +++++++++++++++---------------- test/lisp/eshell/esh-cmd-tests.el | 54 ++++++++++++++++++++++++ test/lisp/eshell/esh-var-tests.el | 56 ++++++++++++++++++++++++- 6 files changed, 199 insertions(+), 68 deletions(-) diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi index 141c30ae9b..aae779575d 100644 --- a/doc/misc/eshell.texi +++ b/doc/misc/eshell.texi @@ -890,14 +890,18 @@ Variables @vindex $$ @item $$ -This is the result of the last command. In case of an external -command, it is @code{t} or @code{nil}. +This is the result of the last command. For external commands, it is +@code{t} if the exit code was 0 or @code{nil} otherwise. +@vindex eshell-lisp-form-nil-is-failure @vindex $? @item $? This variable contains the exit code of the last command. If the last command was a Lisp function, it is 0 for successful completion or 1 -otherwise. +otherwise. If @code{eshell-lisp-form-nil-is-failure} is +non-@code{nil}, then a command with a Lisp form, like +@samp{(@var{command} @var{args}@dots{})}, that returns @code{nil} will +set this variable to 2. @vindex $COLUMNS @vindex $LINES @@ -1024,8 +1028,8 @@ Control Flow This can take a few different forms. If @var{conditional} is a dollar expansion, the condition is satisfied if the result is a non-@code{nil} value. If @var{conditional} is a @samp{@{ -@var{subcommand} @}}, the condition is satisfied if the -@var{subcommand}'s exit status is 0. +@var{subcommand} @}} or @samp{(@var{lisp form})}, the condition is +satisfied if the command's exit status is 0. @table @code diff --git a/etc/NEWS b/etc/NEWS index 8d54ccc0dc..21bf1cc050 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -2149,6 +2149,13 @@ Additionally, globs ending with '**/' or '***/' no longer raise an error, and now expand to all directories recursively (following symlinks in the latter case). ++++ +*** Lisp forms in Eshell now treat a 'nil' result as a failed exit status. +When executing a command that looks like '(lisp form)', Eshell will +set the exit status (available in the '$?' variable) to 2. This +allows commands like that to be used as conditionals. To change this +behavior, customize the new 'eshell-lisp-form-nil-is-failure' option. + ** Shell --- diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 454a90e91d..62c95056fd 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -133,6 +133,10 @@ eshell-lisp-regexp Such arguments will be passed to `read', and then evaluated." :type 'regexp) +(defcustom eshell-lisp-form-nil-is-failure t + "If non-nil, Lisp forms like (COMMAND ARGS) treat a nil result as failure." + :type 'boolean) + (defcustom eshell-pre-command-hook nil "A hook run before each interactive command is invoked." :type 'hook) @@ -1412,43 +1416,53 @@ eshell-last-output-end (defun eshell-lisp-command (object &optional args) "Insert Lisp OBJECT, using ARGS if a function." (catch 'eshell-external ; deferred to an external command + (setq eshell-last-command-status 0 + eshell-last-arguments args) (let* ((eshell-ensure-newline-p (eshell-interactive-output-p)) + (command-form-p (functionp object)) (result - (if (functionp object) - (progn - (setq eshell-last-arguments args - eshell-last-command-name + (if command-form-p + (let ((numeric (not (get object + 'eshell-no-numeric-conversions))) + (fname-args (get object 'eshell-filename-arguments))) + (when (or numeric fname-args) + (while args + (let ((arg (car args))) + (cond + ((and numeric (stringp arg) (> (length arg) 0) + (text-property-any 0 (length arg) + 'number t arg)) + ;; If any of the arguments are flagged as + ;; numbers waiting for conversion, convert + ;; them now. + (setcar args (string-to-number arg))) + ((and fname-args (stringp arg) + (string-equal arg "~")) + ;; If any of the arguments match "~", + ;; prepend "./" to treat it as a regular + ;; file name. + (setcar args (concat "./" arg))))) + (setq args (cdr args)))) + (setq eshell-last-command-name (concat "#<function " (symbol-name object) ">")) - (let ((numeric (not (get object - 'eshell-no-numeric-conversions))) - (fname-args (get object 'eshell-filename-arguments))) - (when (or numeric fname-args) - (while args - (let ((arg (car args))) - (cond ((and numeric (stringp arg) (> (length arg) 0) - (text-property-any 0 (length arg) - 'number t arg)) - ;; If any of the arguments are - ;; flagged as numbers waiting for - ;; conversion, convert them now. - (setcar args (string-to-number arg))) - ((and fname-args (stringp arg) - (string-equal arg "~")) - ;; If any of the arguments match "~", - ;; prepend "./" to treat it as a - ;; regular file name. - (setcar args (concat "./" arg))))) - (setq args (cdr args))))) (eshell-apply object eshell-last-arguments)) - (setq eshell-last-arguments args - eshell-last-command-name "#<Lisp object>") + (setq eshell-last-command-name "#<Lisp object>") (eshell-eval object)))) (if (and eshell-ensure-newline-p (save-excursion (goto-char eshell-last-output-end) (not (bolp)))) (eshell-print "\n")) - (eshell-close-handles 0 (list 'quote result))))) + (eshell-close-handles + ;; If `eshell-lisp-form-nil-is-failure' is non-nil, Lisp forms + ;; that succeeded but have a nil result should have an exit + ;; status of 2. + (when (and eshell-lisp-form-nil-is-failure + (not command-form-p) + (= eshell-last-command-status 0) + (not result)) + 2) + (list 'quote result))))) (defalias 'eshell-lisp-command* #'eshell-lisp-command) diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el index 99b43661f2..c367b5cd64 100644 --- a/lisp/eshell/esh-proc.el +++ b/lisp/eshell/esh-proc.el @@ -346,7 +346,9 @@ eshell-gather-process-output (defvar eshell-last-output-end) ;Defined in esh-mode.el. (eshell-update-markers eshell-last-output-end) ;; Simulate the effect of eshell-sentinel. - (eshell-close-handles (if (numberp exit-status) exit-status -1)) + (eshell-close-handles + (if (numberp exit-status) exit-status -1) + (list 'quote (and (numberp exit-status) (= exit-status 0)))) (eshell-kill-process-function command exit-status) (or (bound-and-true-p eshell-in-pipeline-p) (setq eshell-last-sync-output-start nil)) @@ -398,40 +400,36 @@ eshell-sentinel (when (buffer-live-p (process-buffer proc)) (with-current-buffer (process-buffer proc) (unwind-protect - (let ((entry (assq proc eshell-process-list))) -; (if (not entry) -; (error "Sentinel called for unowned process `%s'" -; (process-name proc)) - (when entry - (unwind-protect - (progn - (unless (string= string "run") - ;; Write the exit message if the status is - ;; abnormal and the process is already writing - ;; to the terminal. - (when (and (eq proc (eshell-tail-process)) - (not (string-match "^\\(finished\\|exited\\)" - string))) - (funcall (process-filter proc) proc string)) - (let ((handles (nth 1 entry)) - (str (prog1 (nth 3 entry) - (setf (nth 3 entry) nil))) - (status (process-exit-status proc))) - ;; If we're in the middle of handling output - ;; from this process then schedule the EOF for - ;; later. - (letrec ((finish-io - (lambda () - (if (nth 4 entry) - (run-at-time 0 nil finish-io) - (when str - (ignore-error 'eshell-pipe-broken - (eshell-output-object - str nil handles))) - (eshell-close-handles - status 'nil handles))))) - (funcall finish-io))))) - (eshell-remove-process-entry entry)))) + (when-let ((entry (assq proc eshell-process-list))) + (unwind-protect + (unless (string= string "run") + ;; Write the exit message if the status is + ;; abnormal and the process is already writing + ;; to the terminal. + (when (and (eq proc (eshell-tail-process)) + (not (string-match "^\\(finished\\|exited\\)" + string))) + (funcall (process-filter proc) proc string)) + (let ((handles (nth 1 entry)) + (str (prog1 (nth 3 entry) + (setf (nth 3 entry) nil))) + (status (process-exit-status proc))) + ;; If we're in the middle of handling output + ;; from this process then schedule the EOF for + ;; later. + (letrec ((finish-io + (lambda () + (if (nth 4 entry) + (run-at-time 0 nil finish-io) + (when str + (ignore-error 'eshell-pipe-broken + (eshell-output-object + str nil handles))) + (eshell-close-handles + status (list 'quote (= status 0)) + handles))))) + (funcall finish-io)))) + (eshell-remove-process-entry entry))) (eshell-kill-process-function proc string))))) (defun eshell-process-interact (func &optional all query) diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el index b31159a1a8..e86985ec71 100644 --- a/test/lisp/eshell/esh-cmd-tests.el +++ b/test/lisp/eshell/esh-cmd-tests.el @@ -139,6 +139,15 @@ esh-cmd-test/while-loop "{ setq eshell-test-value (cdr eshell-test-value) }") "(1 2)\n(2)\n")))) +(ert-deftest esh-cmd-test/while-loop-lisp-form () + "Test invocation of a while loop using a Lisp form." + (with-temp-eshell + (let ((eshell-test-value 0)) + (eshell-command-result-p + (concat "while (/= eshell-test-value 3) " + "{ setq eshell-test-value (1+ eshell-test-value) }") + "1\n2\n3\n")))) + (ert-deftest esh-cmd-test/while-loop-ext-cmd () "Test invocation of a while loop using an external command." (skip-unless (executable-find "[")) @@ -158,6 +167,16 @@ esh-cmd-test/until-loop "{ setq eshell-test-value t }") "t\n")))) +(ert-deftest esh-cmd-test/until-loop-lisp-form () + "Test invocation of an until loop using a Lisp form." + (skip-unless (executable-find "[")) + (with-temp-eshell + (let ((eshell-test-value 0)) + (eshell-command-result-p + (concat "until (= eshell-test-value 3) " + "{ setq eshell-test-value (1+ eshell-test-value) }") + "1\n2\n3\n")))) + (ert-deftest esh-cmd-test/until-loop-ext-cmd () "Test invocation of an until loop using an external command." (skip-unless (executable-find "[")) @@ -188,6 +207,30 @@ esh-cmd-test/if-else-statement (eshell-command-result-p "if $eshell-test-value {echo yes} {echo no}" "no\n")))) +(ert-deftest esh-cmd-test/if-else-statement-lisp-form () + "Test invocation of an if/else statement using a Lisp form." + (with-temp-eshell + (eshell-command-result-p "if (zerop 0) {echo yes} {echo no}" + "yes\n") + (eshell-command-result-p "if (zerop 1) {echo yes} {echo no}" + "no\n") + (let ((debug-on-error nil)) + (eshell-command-result-p "if (zerop \"foo\") {echo yes} {echo no}" + "no\n")))) + +(ert-deftest esh-cmd-test/if-else-statement-lisp-form-2 () + "Test invocation of an if/else statement using a Lisp form. +This tests when `eshell-lisp-form-nil-is-failure' is nil." + (let ((eshell-lisp-form-nil-is-failure nil)) + (with-temp-eshell + (eshell-command-result-p "if (zerop 0) {echo yes} {echo no}" + "yes\n") + (eshell-command-result-p "if (zerop 1) {echo yes} {echo no}" + "yes\n") + (let ((debug-on-error nil)) + (eshell-command-result-p "if (zerop \"foo\") {echo yes} {echo no}" + "no\n"))))) + (ert-deftest esh-cmd-test/if-else-statement-ext-cmd () "Test invocation of an if/else statement using an external command." (skip-unless (executable-find "[")) @@ -217,6 +260,17 @@ esh-cmd-test/unless-else-statement (eshell-command-result-p "unless $eshell-test-value {echo no} {echo yes}" "no\n")))) +(ert-deftest esh-cmd-test/unless-else-statement-lisp-form () + "Test invocation of an unless/else statement using a Lisp form." + (with-temp-eshell + (eshell-command-result-p "unless (zerop 0) {echo no} {echo yes}" + "yes\n") + (eshell-command-result-p "unless (zerop 1) {echo no} {echo yes}" + "no\n") + (let ((debug-on-error nil)) + (eshell-command-result-p "unless (zerop \"foo\") {echo no} {echo yes}" + "no\n")))) + (ert-deftest esh-cmd-test/unless-else-statement-ext-cmd () "Test invocation of an unless/else statement using an external command." (skip-unless (executable-find "[")) diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el index 54e701a6aa..66dabd424b 100644 --- a/test/lisp/eshell/esh-var-tests.el +++ b/test/lisp/eshell/esh-var-tests.el @@ -500,18 +500,72 @@ esh-var-test/inside-emacs-var-split-indices (eshell-command-result-p "echo $INSIDE_EMACS[, 1]" "eshell"))) +(ert-deftest esh-var-test/last-status-var-lisp-command () + "Test using the \"last exit status\" ($?) variable with a Lisp command" + (with-temp-eshell + (eshell-command-result-p "zerop 0; echo $?" + "t\n0\n") + (eshell-command-result-p "zerop 1; echo $?" + "0\n") + (let ((debug-on-error nil)) + (eshell-command-result-p "zerop foo; echo $?" + "1\n")))) + +(ert-deftest esh-var-test/last-status-var-lisp-form () + "Test using the \"last exit status\" ($?) variable with a Lisp form" + (let ((eshell-lisp-form-nil-is-failure t)) + (with-temp-eshell + (eshell-command-result-p "(zerop 0); echo $?" + "t\n0\n") + (eshell-command-result-p "(zerop 1); echo $?" + "2\n") + (let ((debug-on-error nil)) + (eshell-command-result-p "(zerop \"foo\"); echo $?" + "1\n"))))) + +(ert-deftest esh-var-test/last-status-var-lisp-form-2 () + "Test using the \"last exit status\" ($?) variable with a Lisp form. +This tests when `eshell-lisp-form-nil-is-failure' is nil." + (let ((eshell-lisp-form-nil-is-failure nil)) + (with-temp-eshell + (eshell-command-result-p "(zerop 0); echo $?" + "0\n") + (eshell-command-result-p "(zerop 0); echo $?" + "0\n") + (let ((debug-on-error nil)) + (eshell-command-result-p "(zerop \"foo\"); echo $?" + "1\n"))))) + +(ert-deftest esh-var-test/last-status-var-ext-cmd () + "Test using the \"last exit status\" ($?) variable with an external command" + (skip-unless (executable-find "[")) + (with-temp-eshell + (eshell-command-result-p "[ foo = foo ]; echo $?" + "0\n") + (eshell-command-result-p "[ foo = bar ]; echo $?" + "1\n"))) + (ert-deftest esh-var-test/last-result-var () "Test using the \"last result\" ($$) variable" (with-temp-eshell (eshell-command-result-p "+ 1 2; + $$ 2" "3\n5\n"))) -(ert-deftest esh-var-test/last-result-var2 () +(ert-deftest esh-var-test/last-result-var-twice () "Test using the \"last result\" ($$) variable twice" (with-temp-eshell (eshell-command-result-p "+ 1 2; + $$ $$" "3\n6\n"))) +(ert-deftest esh-var-test/last-result-var-ext-cmd () + "Test using the \"last result\" ($$) variable with an external command" + (skip-unless (executable-find "[")) + (with-temp-eshell + (eshell-command-result-p "[ foo = foo ]; format \"%s\" $$" + "t\n") + (eshell-command-result-p "[ foo = bar ]; format \"%s\" $$" + "nil\n"))) + (ert-deftest esh-var-test/last-result-var-split-indices () "Test using the \"last result\" ($$) variable with split indices" (with-temp-eshell -- 2.25.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-11 2:46 ` Jim Porter @ 2022-08-12 15:22 ` Lars Ingebrigtsen 2022-08-13 5:14 ` Jim Porter 0 siblings, 1 reply; 57+ messages in thread From: Lars Ingebrigtsen @ 2022-08-12 15:22 UTC (permalink / raw) To: Jim Porter; +Cc: 57129 Jim Porter <jporterbugs@gmail.com> writes: > Here are some patches to fix this. I haven't tested the patches, but reading them, they make sense to me, so please go ahead and push. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-12 15:22 ` Lars Ingebrigtsen @ 2022-08-13 5:14 ` Jim Porter 2022-08-13 7:01 ` Eli Zaretskii 2022-08-14 5:03 ` Sean Whitton 0 siblings, 2 replies; 57+ messages in thread From: Jim Porter @ 2022-08-13 5:14 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 57129 On 8/12/2022 8:22 AM, Lars Ingebrigtsen wrote: > Jim Porter <jporterbugs@gmail.com> writes: > >> Here are some patches to fix this. > > I haven't tested the patches, but reading them, they make sense to me, > so please go ahead and push. Thanks for taking a look. Merged as f3408af0a3251a744cb0b55b5e153565bfd57ea3. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-13 5:14 ` Jim Porter @ 2022-08-13 7:01 ` Eli Zaretskii 2022-08-13 18:56 ` Jim Porter 2022-08-14 5:03 ` Sean Whitton 1 sibling, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2022-08-13 7:01 UTC (permalink / raw) To: Jim Porter; +Cc: larsi, 57129 > Cc: 57129@debbugs.gnu.org > From: Jim Porter <jporterbugs@gmail.com> > Date: Fri, 12 Aug 2022 22:14:02 -0700 > > On 8/12/2022 8:22 AM, Lars Ingebrigtsen wrote: > > Jim Porter <jporterbugs@gmail.com> writes: > > > >> Here are some patches to fix this. > > > > I haven't tested the patches, but reading them, they make sense to me, > > so please go ahead and push. > > Thanks for taking a look. Merged as > f3408af0a3251a744cb0b55b5e153565bfd57ea3. One of the tests in esh-var-tests.el failed on MS-Windows; I fixed it, although I'm not sure it's a correct fix, because the Eshell manual seems to say that a built-in implementation of a command should be preferred by default? One of the tests in eshell-tests.el also fails on MS-Windows, but I have no idea how to fix it, nor even what are the details of the test. Are the 'echo' commands in that test supposed to be external shell commands or internal Eshell commands? If the former, it could be a problem on MS-Windows. (This is a good example of my gripes about the test suite, which I described in bug#57150 yesterday: debugging a failure is unnecessarily hard and tedious. There's no information about the specific intent of the test and its inner workings and assumptions, except the general idea described in the doc string.) Here's the failure I get: Test eshell-test/subcommand-reset-in-pipeline backtrace: signal(ert-test-failed (((should (equal (eshell-test-command-result ert-fail(((should (equal (eshell-test-command-result (format templat (if (unwind-protect (setq value-38 (apply fn-36 args-37)) (setq form (let (form-description-40) (if (unwind-protect (setq value-38 (apply (let ((value-38 'ert-form-evaluation-aborted-39)) (let (form-descrip (let* ((fn-36 #'equal) (args-37 (condition-case err (let ((signal-ho (let ((template (car --dolist-tail--))) (let* ((fn-26 #'equal) (args (while --dolist-tail-- (let ((template (car --dolist-tail--))) (let* (let ((--dolist-tail-- '("echo {%s} | *cat" "echo ${%s} | *cat" "*ca (closure (t) nil (let* ((fn-21 #'executable-find) (args-22 (conditio ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test ert-run-test(#s(ert-test :name eshell-test/subcommand-reset-in-pipel ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp)))) ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/eshell/eshell-tests. command-line() normal-top-level() Test eshell-test/subcommand-reset-in-pipeline condition: (ert-test-failed ((should (equal (eshell-test-command-result ...) "first")) :form (equal nil "first") :value nil :explanation (different-types nil "first"))) FAILED 18/18 eshell-test/subcommand-reset-in-pipeline (1.873920 sec) at lisp/eshell/eshell-tests.el:80 ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-13 7:01 ` Eli Zaretskii @ 2022-08-13 18:56 ` Jim Porter 2022-08-14 5:07 ` Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Jim Porter @ 2022-08-13 18:56 UTC (permalink / raw) To: Eli Zaretskii; +Cc: larsi, 57129 On 8/13/2022 12:01 AM, Eli Zaretskii wrote: > One of the tests in esh-var-tests.el failed on MS-Windows; I fixed it, > although I'm not sure it's a correct fix, because the Eshell manual > seems to say that a built-in implementation of a command should be > preferred by default? Sorry about that. I think that's the right fix for that case. Maybe it would make sense to set 'eshell-prefer-lisp-functions' to t for most of the Eshell tests to improve reproducibility on various platforms; tests that want an external command can just put a "*" in front of the command name. > One of the tests in eshell-tests.el also fails on MS-Windows, but I > have no idea how to fix it, nor even what are the details of the > test. Are the 'echo' commands in that test supposed to be external > shell commands or internal Eshell commands? If the former, it could > be a problem on MS-Windows. The echo commands should be internal Eshell commands (since there's an 'eshell/echo' Lisp function, that one should always be preferred by Eshell). I'm surprised that test fails on MS Windows, since it *should* be testing internal Eshell logic that's not platform-specific. Based on the failure, it looks like one of the following commands is returning the wrong value: echo {echo $eshell-in-pipeline-p | echo} | *cat echo ${echo $eshell-in-pipeline-p | echo} | *cat *cat $<echo $eshell-in-pipeline-p | echo> | *cat All of these should return 'first'. That test is just checking that, when you're in a subcommand ({...}, ${...}, or $<...>), the value of 'eshell-in-pipeline-p' shouldn't be influenced by the pipeline in the "super-command". Some built-in Eshell commands consult 'eshell-in-pipeline-p', and if it had the wrong value, they might do the wrong thing. If nothing else, it would probably be helpful to set up ERT explainers so that the error messages are easier to understand. As it is now, they're not very explanatory. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-13 18:56 ` Jim Porter @ 2022-08-14 5:07 ` Eli Zaretskii 2022-08-14 5:37 ` Jim Porter 0 siblings, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2022-08-14 5:07 UTC (permalink / raw) To: Jim Porter; +Cc: larsi, 57129 > Cc: larsi@gnus.org, 57129@debbugs.gnu.org > From: Jim Porter <jporterbugs@gmail.com> > Date: Sat, 13 Aug 2022 11:56:20 -0700 > > On 8/13/2022 12:01 AM, Eli Zaretskii wrote: > > One of the tests in esh-var-tests.el failed on MS-Windows; I fixed it, > > although I'm not sure it's a correct fix, because the Eshell manual > > seems to say that a built-in implementation of a command should be > > preferred by default? > > Sorry about that. I think that's the right fix for that case. Maybe it > would make sense to set 'eshell-prefer-lisp-functions' to t for most of > the Eshell tests to improve reproducibility on various platforms; tests > that want an external command can just put a "*" in front of the command > name. But then this fragment from the Eshell manual: The command can be either an Elisp function or an external command. Eshell looks first for an alias (*note Aliases::) with the same name as the command, then a built-in (*note Built-ins::) or a function with the same name; if there is no match, it then tries to execute it as an external command. seems to be inaccurate? Since 'format' exists as a built-in command, why did Eshell in this case invoke the external command instead? > I'm surprised that test fails on MS Windows, since it *should* be > testing internal Eshell logic that's not platform-specific. Based on the > failure, it looks like one of the following commands is returning the > wrong value: > > echo {echo $eshell-in-pipeline-p | echo} | *cat > echo ${echo $eshell-in-pipeline-p | echo} | *cat > *cat $<echo $eshell-in-pipeline-p | echo> | *cat > > All of these should return 'first'. The first two do; the last one returns nothing. > That test is just checking that, > when you're in a subcommand ({...}, ${...}, or $<...>), the value of > 'eshell-in-pipeline-p' shouldn't be influenced by the pipeline in the > "super-command". Some built-in Eshell commands consult > 'eshell-in-pipeline-p', and if it had the wrong value, they might do the > wrong thing. So how to investigate this failure further? > If nothing else, it would probably be helpful to set up ERT explainers > so that the error messages are easier to understand. As it is now, > they're not very explanatory. Indeed, more explanations in this and other tests will be most welcome. Thanks. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-14 5:07 ` Eli Zaretskii @ 2022-08-14 5:37 ` Jim Porter 2022-08-14 7:30 ` Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Jim Porter @ 2022-08-14 5:37 UTC (permalink / raw) To: Eli Zaretskii; +Cc: larsi, 57129 [-- Attachment #1: Type: text/plain, Size: 3225 bytes --] On 8/13/2022 10:07 PM, Eli Zaretskii wrote: >> Cc: larsi@gnus.org, 57129@debbugs.gnu.org >> From: Jim Porter <jporterbugs@gmail.com> >> Date: Sat, 13 Aug 2022 11:56:20 -0700 >> >> Sorry about that. I think that's the right fix for that case. Maybe it >> would make sense to set 'eshell-prefer-lisp-functions' to t for most of >> the Eshell tests to improve reproducibility on various platforms; tests >> that want an external command can just put a "*" in front of the command >> name. > > But then this fragment from the Eshell manual: > > The command can be either an Elisp function or an external command. > Eshell looks first for an alias (*note Aliases::) with the same name as > the command, then a built-in (*note Built-ins::) or a function with the > same name; if there is no match, it then tries to execute it as an > external command. > > seems to be inaccurate? Since 'format' exists as a built-in command, > why did Eshell in this case invoke the external command instead? "Built-in" in that part of the manual refers to a function named 'eshell/FOO'. If you run "FOO" as an Eshell command, it will check for 'eshell/FOO' before an external command on your system. The manual could probably stand to be improved here. >> I'm surprised that test fails on MS Windows, since it *should* be >> testing internal Eshell logic that's not platform-specific. Based on the >> failure, it looks like one of the following commands is returning the >> wrong value: >> >> echo {echo $eshell-in-pipeline-p | echo} | *cat >> echo ${echo $eshell-in-pipeline-p | echo} | *cat >> *cat $<echo $eshell-in-pipeline-p | echo> | *cat >> >> All of these should return 'first'. > > The first two do; the last one returns nothing. [snip] > So how to investigate this failure further? I have access to an MS Windows system (but don't have a build environment set up for native MS Windows builds), so I can try to reproduce this using the Emacs 29 pretest binaries in the next few days. Hopefully that works. If you'd like to dig into this further yourself, you could try running this command in Eshell: eshell-parse-command '*cat $<echo $eshell-in-pipeline-p | echo> | *cat' That will print the Lisp form that the command gets converted to. I've attached the result that I get in a recent Emacs 29 build on GNU/Linux. The two functions that set 'eshell-in-pipeline-p' in there are: * 'eshell-as-subcommand', which resets it to nil so that the outer command's pipeline state doesn't interfere with the subcommand. * 'eshell-execute-pipeline', which calls 'eshell-do-pipelines' (except on MS-DOS, I think) and should set 'eshell-in-pipeline-p' to 'first' in the above case. >> If nothing else, it would probably be helpful to set up ERT explainers >> so that the error messages are easier to understand. As it is now, >> they're not very explanatory. > > Indeed, more explanations in this and other tests will be most > welcome. Yeah, this test in particular is high up on the list of tests that needs more explanation. It's pretty subtle, and the test doesn't really serve as a good example of why someone would care that this behavior works the way the test demands it. [-- Attachment #2: parsed-command.el --] [-- Type: text/plain, Size: 1563 bytes --] (eshell-trap-errors (eshell-execute-pipeline '((eshell-named-command (eshell-concat nil "*" "cat") (list (eshell-escape-arg (let ((indices (eshell-eval-indices 'nil))) (let ((eshell-current-handles (eshell-create-handles "/tmp/QqPFwo" 'overwrite))) (progn (eshell-as-subcommand (eshell-trap-errors (eshell-execute-pipeline '((eshell-named-command "echo" (list (eshell-escape-arg (let ((indices (eshell-eval-indices 'nil))) (eshell-get-variable #("eshell-in-pipeline-p" 0 20 (escaped t)) indices nil))))) (progn (ignore (eshell-set-output-handle 1 'overwrite "/tmp/QqPFwo")) (eshell-named-command "echo")))))) (ignore (nconc eshell-this-command-hook (list #'(lambda nil (delete-file "/tmp/QqPFwo"))))) (eshell-apply-indices "/tmp/QqPFwo" indices nil))))))) (eshell-named-command (eshell-concat nil "*" "cat"))))) ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-14 5:37 ` Jim Porter @ 2022-08-14 7:30 ` Eli Zaretskii 2022-08-14 21:40 ` Jim Porter 0 siblings, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2022-08-14 7:30 UTC (permalink / raw) To: Jim Porter; +Cc: larsi, 57129 > Cc: larsi@gnus.org, 57129@debbugs.gnu.org > From: Jim Porter <jporterbugs@gmail.com> > Date: Sat, 13 Aug 2022 22:37:12 -0700 > > > But then this fragment from the Eshell manual: > > > > The command can be either an Elisp function or an external command. > > Eshell looks first for an alias (*note Aliases::) with the same name as > > the command, then a built-in (*note Built-ins::) or a function with the > > same name; if there is no match, it then tries to execute it as an > > external command. > > > > seems to be inaccurate? Since 'format' exists as a built-in command, > > why did Eshell in this case invoke the external command instead? > > "Built-in" in that part of the manual refers to a function named > 'eshell/FOO'. If you run "FOO" as an Eshell command, it will check for > 'eshell/FOO' before an external command on your system. The manual could > probably stand to be improved here. The manual definitely should be clarified in that matter, because: d:/gnu/git/emacs/trunk/src $ which format format is a built-in function in ‘C source code’. To me this says that 'format' is a built-in command, and the manual says such commands should be invoked in preference to external commands. The "eshell/" part is not mentioned anywhere. > If you'd like to dig into this further yourself, you could try running > this command in Eshell: > > eshell-parse-command '*cat $<echo $eshell-in-pipeline-p | echo> | *cat' > > That will print the Lisp form that the command gets converted to. I've > attached the result that I get in a recent Emacs 29 build on GNU/Linux. I get the same output, with 2 exceptions: . the name of the temporary file is different . instead of a simple string here: (eshell-set-output-handle 1 'overwrite "/tmp/QqPFwo")) I get a file name split into several parts, which are then concatenated by eshell-concat: (eshell-set-output-handle 1 'overwrite (eshell-extended-glob (eshell-concat nil "c:/DOCUME" "~" "1/Zaretzky/LOCALS" "~" "1/Temp/OIi8Wd"))) Any chance that the "~" parts here somehow get in the way? If not, any other thoughts? My main worry is that there's something here specific to how we invoke subprocesses, which you lately changed. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-14 7:30 ` Eli Zaretskii @ 2022-08-14 21:40 ` Jim Porter 2022-08-15 12:13 ` Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Jim Porter @ 2022-08-14 21:40 UTC (permalink / raw) To: Eli Zaretskii; +Cc: larsi, 57129 On 8/14/2022 12:30 AM, Eli Zaretskii wrote: > The manual definitely should be clarified in that matter, because: > > d:/gnu/git/emacs/trunk/src $ which format > format is a built-in function in ‘C source code’. > > To me this says that 'format' is a built-in command, and the manual > says such commands should be invoked in preference to external > commands. The "eshell/" part is not mentioned anywhere. I'll work on a fix for that. This is a little more complex than immediately apparent though, since Eshell also unfortunately uses the term "alias" to refer to two kinds of entities: 1) Command abbreviations created by the user using the 'alias' command, e.g. "alias ll 'ls -l'". These are sometimes called "command aliases", and are implemented in em-alias.el. 2) Lisp functions with names like 'eshell/FOO'. These are sometimes called "alias functions", and are implemented in esh-cmd.el. For example, see 'eshell-find-alias-function', which checks if 'eshell/FOO' exists. It would probably be good to fix all this terminology for once and for all. > I get the same output, with 2 exceptions: > > . the name of the temporary file is different > . instead of a simple string here: > > (eshell-set-output-handle 1 'overwrite "/tmp/QqPFwo")) > > I get a file name split into several parts, which are then > concatenated by eshell-concat: > > (eshell-set-output-handle 1 'overwrite > (eshell-extended-glob > (eshell-concat nil "c:/DOCUME" "~" "1/Zaretzky/LOCALS" "~" "1/Temp/OIi8Wd"))) > > Any chance that the "~" parts here somehow get in the way? It's possible. I think Eshell is trying to ensure that the "~" doesn't get parsed as "home directory" here. But it could be doing something wrong there. I did also touch 'eshell-concat' not too long ago, so I might have broken something there. > If not, any other thoughts? My main worry is that there's something > here specific to how we invoke subprocesses, which you lately changed. You could try some Eshell commands that don't use subprocesses to see if that changes anything. For example, these should only use Lisp functions: ;; Should print "first" in the Eshell buffer: eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo} eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo} | eshell/echo ;; Should open a temp file containing "first" in a new buffer: find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo> find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo> | eshell/echo The "eshell/" prefix probably isn't necessary, but I figured it's worth being extra-careful here while diagnosing the issue. I'm particularly interested in whether the third and fourth commands work. If they work, that would definitely point towards a bug in Eshell's subprocess handling; if not, then that would point to a bug in the "$<subcommand>" handling. I also filed bug#57216 to add ERT explainers that will print better error messages for most of the Eshell tests. That doesn't address the sometimes-vague documentation of the tests, but I'll do that separately. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-14 21:40 ` Jim Porter @ 2022-08-15 12:13 ` Eli Zaretskii 2022-08-15 16:14 ` Jim Porter 0 siblings, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2022-08-15 12:13 UTC (permalink / raw) To: Jim Porter; +Cc: larsi, 57129 > Cc: larsi@gnus.org, 57129@debbugs.gnu.org > From: Jim Porter <jporterbugs@gmail.com> > Date: Sun, 14 Aug 2022 14:40:06 -0700 > > On 8/14/2022 12:30 AM, Eli Zaretskii wrote: > > The manual definitely should be clarified in that matter, because: > > > > d:/gnu/git/emacs/trunk/src $ which format > > format is a built-in function in ‘C source code’. > > > > To me this says that 'format' is a built-in command, and the manual > > says such commands should be invoked in preference to external > > commands. The "eshell/" part is not mentioned anywhere. > > I'll work on a fix for that. This is a little more complex than > immediately apparent though, since Eshell also unfortunately uses the > term "alias" to refer to two kinds of entities: > > 1) Command abbreviations created by the user using the 'alias' command, > e.g. "alias ll 'ls -l'". These are sometimes called "command > aliases", and are implemented in em-alias.el. > > 2) Lisp functions with names like 'eshell/FOO'. These are sometimes > called "alias functions", and are implemented in esh-cmd.el. For > example, see 'eshell-find-alias-function', which checks if > 'eshell/FOO' exists. > > It would probably be good to fix all this terminology for once and for all. Yes, that could well be a source of a lot of confusion. > You could try some Eshell commands that don't use subprocesses to see if > that changes anything. For example, these should only use Lisp functions: > > ;; Should print "first" in the Eshell buffer: > eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo} > eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo} | > eshell/echo > > ;; Should open a temp file containing "first" in a new buffer: > find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo> > find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo> | > eshell/echo > > The "eshell/" prefix probably isn't necessary, but I figured it's worth > being extra-careful here while diagnosing the issue. I'm particularly > interested in whether the third and fourth commands work. If they work, > that would definitely point towards a bug in Eshell's subprocess > handling; if not, then that would point to a bug in the "$<subcommand>" > handling. All the 4 commands do what you say they should do. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-15 12:13 ` Eli Zaretskii @ 2022-08-15 16:14 ` Jim Porter 2022-08-15 16:49 ` Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Jim Porter @ 2022-08-15 16:14 UTC (permalink / raw) To: Eli Zaretskii; +Cc: larsi, 57129 On 8/15/2022 5:13 AM, Eli Zaretskii wrote: >> Cc: larsi@gnus.org, 57129@debbugs.gnu.org >> From: Jim Porter <jporterbugs@gmail.com> >> Date: Sun, 14 Aug 2022 14:40:06 -0700 >> >> You could try some Eshell commands that don't use subprocesses to see if >> that changes anything. For example, these should only use Lisp functions: >> >> ;; Should print "first" in the Eshell buffer: >> eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo} >> eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo} | >> eshell/echo >> >> ;; Should open a temp file containing "first" in a new buffer: >> find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo> >> find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo> | >> eshell/echo >> >> The "eshell/" prefix probably isn't necessary, but I figured it's worth >> being extra-careful here while diagnosing the issue. I'm particularly >> interested in whether the third and fourth commands work. If they work, >> that would definitely point towards a bug in Eshell's subprocess >> handling; if not, then that would point to a bug in the "$<subcommand>" >> handling. > > All the 4 commands do what you say they should do. Hm, and I can't reproduce this on the prebuilt Emacs 29 snapshot from July 18, which is from just before I merged the changes to 'make-process', so it's definitely possible that this is due to those commits. Does reverting 4e59830bc0ab17cdbd85748b133c97837bed99e3 and d7b89ea4077d4fe677ba0577245328819ee79cdc fix the issue? I checked locally, and they should revert cleanly at least. It might also be interesting to revert just the changes from d7b89ea in lisp/eshell/esh-proc.el. I don't think any of the changes I made to the C sources should impact this, since a) I was careful to keep the logic as close to before as I could, and b) it just changes the logic for creating a PTY, which doesn't work on MS Windows anyway. I did notice one weird issue though, and maybe this has something to do with the problem: on MS Windows, if I run any command with a $<subcommand>, the second and subsequent times, it warns me about changing the temp file, e.g.: 3Zz80A has changed since visited or saved. Save anyway? (yes or no) This probably shouldn't do that, and it could definitely cause issues in the tests, which can't prompt the user for things like that. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-15 16:14 ` Jim Porter @ 2022-08-15 16:49 ` Eli Zaretskii 2022-08-15 18:08 ` Jim Porter 0 siblings, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2022-08-15 16:49 UTC (permalink / raw) To: Jim Porter; +Cc: larsi, 57129 > Cc: larsi@gnus.org, 57129@debbugs.gnu.org > From: Jim Porter <jporterbugs@gmail.com> > Date: Mon, 15 Aug 2022 09:14:04 -0700 > > > All the 4 commands do what you say they should do. > > Hm, and I can't reproduce this on the prebuilt Emacs 29 snapshot from > July 18, which is from just before I merged the changes to > 'make-process', so it's definitely possible that this is due to those > commits. > > Does reverting 4e59830bc0ab17cdbd85748b133c97837bed99e3 and > d7b89ea4077d4fe677ba0577245328819ee79cdc fix the issue? I checked > locally, and they should revert cleanly at least. > > It might also be interesting to revert just the changes from d7b89ea in > lisp/eshell/esh-proc.el. I don't think any of the changes I made to the > C sources should impact this, since a) I was careful to keep the logic > as close to before as I could, and b) it just changes the logic for > creating a PTY, which doesn't work on MS Windows anyway. None of these reverts fixes the issue. I tried both the tests in eshell-tests.el and the problematic command: *cat $<echo $eshell-in-pipeline-p | echo> | *cat > I did notice one weird issue though, and maybe this has something to do > with the problem: on MS Windows, if I run any command with a > $<subcommand>, the second and subsequent times, it warns me about > changing the temp file, e.g.: > > 3Zz80A has changed since visited or saved. Save anyway? (yes or no) > > This probably shouldn't do that, and it could definitely cause issues in > the tests, which can't prompt the user for things like that. I guess this means you somehow reuse the same temporary file on MS-Windows? Maybe change the test a bit so that the file name definitely differs? ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-15 16:49 ` Eli Zaretskii @ 2022-08-15 18:08 ` Jim Porter 2022-08-15 18:21 ` Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Jim Porter @ 2022-08-15 18:08 UTC (permalink / raw) To: Eli Zaretskii; +Cc: larsi, 57129 On 8/15/2022 9:49 AM, Eli Zaretskii wrote: > None of these reverts fixes the issue. I tried both the tests in > eshell-tests.el and the problematic command: > > *cat $<echo $eshell-in-pipeline-p | echo> | *cat Thanks for testing. I might have to get an MS-Windows dev environment set up to dig into this further. I'm a bit worried that this is related to bug#55590, where I had to jump through some awkward hoops to fix a somewhat-related issue with Eshell subcommands. I think that's due to 'eshell-do-eval' being written before lexical binding existed, and so the variable scoping doesn't work quite as expected. It could be that you only see this failure when using external commands due to some differences in timing. If that's the problem here too, it would be a pain to fix (but it would likely also mean that this bug has been present for a long time, and my tests/fixes have just brought it to the surface). As mentioned in bug#55590, I think it'd be nice to rip out 'eshell-do-eval' and replace it with the generator.el machinery, since they're conceptually pretty similar. That's easier said than done though... >> I did notice one weird issue though, and maybe this has something to do >> with the problem: on MS Windows, if I run any command with a >> $<subcommand>, the second and subsequent times, it warns me about >> changing the temp file, e.g.: >> >> 3Zz80A has changed since visited or saved. Save anyway? (yes or no) >> >> This probably shouldn't do that, and it could definitely cause issues in >> the tests, which can't prompt the user for things like that. > > I guess this means you somehow reuse the same temporary file on > MS-Windows? Maybe change the test a bit so that the file name > definitely differs? I need to investigate this a bit further, since on GNU/Linux, I get a new temp file every time. I think that's the behavior we want, or failing that, we could at least kill the temp file's buffer after we're done with it. I don't think there's much value in leaving it around. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-15 18:08 ` Jim Porter @ 2022-08-15 18:21 ` Eli Zaretskii 2022-08-15 18:30 ` Jim Porter 0 siblings, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2022-08-15 18:21 UTC (permalink / raw) To: Jim Porter; +Cc: larsi, 57129 > Cc: larsi@gnus.org, 57129@debbugs.gnu.org > From: Jim Porter <jporterbugs@gmail.com> > Date: Mon, 15 Aug 2022 11:08:45 -0700 > > >> I did notice one weird issue though, and maybe this has something to do > >> with the problem: on MS Windows, if I run any command with a > >> $<subcommand>, the second and subsequent times, it warns me about > >> changing the temp file, e.g.: > >> > >> 3Zz80A has changed since visited or saved. Save anyway? (yes or no) > >> > >> This probably shouldn't do that, and it could definitely cause issues in > >> the tests, which can't prompt the user for things like that. > > > > I guess this means you somehow reuse the same temporary file on > > MS-Windows? Maybe change the test a bit so that the file name > > definitely differs? > > I need to investigate this a bit further, since on GNU/Linux, I get a > new temp file every time. Can you tell how you create these temporary files? Or point me to the code which does that? ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-15 18:21 ` Eli Zaretskii @ 2022-08-15 18:30 ` Jim Porter 2022-08-15 18:58 ` Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Jim Porter @ 2022-08-15 18:30 UTC (permalink / raw) To: Eli Zaretskii; +Cc: larsi, 57129 On 8/15/2022 11:21 AM, Eli Zaretskii wrote: >> Cc: larsi@gnus.org, 57129@debbugs.gnu.org >> From: Jim Porter <jporterbugs@gmail.com> >> Date: Mon, 15 Aug 2022 11:08:45 -0700 >> >> I need to investigate this a bit further, since on GNU/Linux, I get a >> new temp file every time. > > Can you tell how you create these temporary files? Or point me to the > code which does that? The temp files are created by Eshell in lisp/eshell/esh-var.el in the function 'eshell-parse-variable-ref', specifically in the part starting with: (eq (char-after) ?\<) ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-15 18:30 ` Jim Porter @ 2022-08-15 18:58 ` Eli Zaretskii 2022-08-15 20:55 ` Paul Eggert ` (2 more replies) 0 siblings, 3 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-15 18:58 UTC (permalink / raw) To: Jim Porter, Paul Eggert; +Cc: larsi, 57129 > Cc: larsi@gnus.org, 57129@debbugs.gnu.org > From: Jim Porter <jporterbugs@gmail.com> > Date: Mon, 15 Aug 2022 11:30:07 -0700 > > >> I need to investigate this a bit further, since on GNU/Linux, I get a > >> new temp file every time. > > > > Can you tell how you create these temporary files? Or point me to the > > code which does that? > > The temp files are created by Eshell in lisp/eshell/esh-var.el in the > function 'eshell-parse-variable-ref', specifically in the part starting > with: > > (eq (char-after) ?\<) Ah, okay. It's a (mis)feature of Gnulib's gen_tempname function (which is the guts of make-temp-file) in its implementation for MS-Windows (and maybe other platforms?): it always begins from the same "random" characters in the file name, and only generates other random characters if there's already a file by that name. So if you are careful and delete the temporary file each time after usage, and never need more than one temporary file at the same time, you will get the same name every call. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-15 18:58 ` Eli Zaretskii @ 2022-08-15 20:55 ` Paul Eggert [not found] ` <af0af29b-2362-77db-081e-046158937808@cs.ucla.edu> 2022-08-20 18:03 ` Jim Porter 2 siblings, 0 replies; 57+ messages in thread From: Paul Eggert @ 2022-08-15 20:55 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Jim Porter, larsi, Gnulib bugs, 57129 [-- Attachment #1: Type: text/plain, Size: 1130 bytes --] On 8/15/22 11:58, Eli Zaretskii wrote: > Ah, okay. It's a (mis)feature of Gnulib's gen_tempname function > (which is the guts of make-temp-file) in its implementation for > MS-Windows (and maybe other platforms?): it always begins from the > same "random" characters in the file name, and only generates other > random characters if there's already a file by that name. Not sure I'd call it a misfeature, as gen_tempname is generating a uniquely-named file that is exclusive to Emacs, which is all it's supposed to do. I do see a comment saying that gen_tempname generates "hard-to-predict" names, which as you note is not correct on MS-DOS, nor even strictly speaking on all POSIX platforms. I installed the first attached patch into Gnulib to fix that comment. I'm not sure I'm entirely understanding the Emacs problem, but it appears to be that Emacs has its own set of filenames that it thinks it knows about, and Emacs wants the new temporary file's name to not be a member of that set. If I'm right, does the second attached patch (this patch is to Emacs) address the problem? I haven't tested or installed it. [-- Attachment #2: 0001-tempname-remove-incorrect-comment.patch --] [-- Type: text/x-patch, Size: 1912 bytes --] From a7ef21bf347da5e3a8b5492b849d633ef3252d62 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Mon, 15 Aug 2022 13:04:08 -0700 Subject: [PATCH] tempname: remove incorrect comment * lib/tempname.c, lib/tempname.h: Remove incorrect comment, as the names are not necessarily hard to predict (Bug#57129). --- ChangeLog | 6 ++++++ lib/tempname.c | 2 +- lib/tempname.h | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index ba16b538f5..1e20db7e37 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2022-08-15 Paul Eggert <eggert@cs.ucla.edu> + + tempname: remove incorrect comment + * lib/tempname.c, lib/tempname.h: Remove incorrect comment, + as the names are not necessarily hard to predict (Bug#57129). + 2022-08-14 Simon Josefsson <simon@josefsson.org> bootstrap.conf: Use proper shell marker for Emacs. diff --git a/lib/tempname.c b/lib/tempname.c index 5fc5efe031..75a939e571 100644 --- a/lib/tempname.c +++ b/lib/tempname.c @@ -213,7 +213,7 @@ static const char letters[] = and return a read-write fd. The file is mode 0600. __GT_DIR: create a directory, which will be mode 0700. - We use a clever algorithm to get hard-to-predict names. */ + */ #ifdef _LIBC static #endif diff --git a/lib/tempname.h b/lib/tempname.h index c172820f7f..5e3c5e1550 100644 --- a/lib/tempname.h +++ b/lib/tempname.h @@ -48,7 +48,7 @@ extern "C" { and return a read-write fd. The file is mode 0600. GT_DIR: create a directory, which will be mode 0700. - We use a clever algorithm to get hard-to-predict names. */ + */ extern int gen_tempname (char *tmpl, int suffixlen, int flags, int kind); /* Similar, except X_SUFFIX_LEN gives the number of Xs. */ extern int gen_tempname_len (char *tmpl, int suffixlen, int flags, int kind, -- 2.34.1 [-- Attachment #3: 0001-Don-t-create-temp-file-with-same-name-as-visited.patch --] [-- Type: text/x-patch, Size: 2910 bytes --] From a0776984bc3630c8c0bd57b3fd1acca1f0b8cb5c Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Mon, 15 Aug 2022 12:55:40 -0700 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20create=20temp=20file=20with=20s?= =?UTF-8?q?ame=20name=20as=20visited?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem reported by Jim Porter (Bug#57129#47). * src/fileio.c (create_tempname): New function. (Fmake_temp_file_internal): Use it. --- src/fileio.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/fileio.c b/src/fileio.c index 9697f6c8cf..4f134bbfa9 100644 --- a/src/fileio.c +++ b/src/fileio.c @@ -677,6 +677,27 @@ DEFUN ("directory-file-name", Fdirectory_file_name, Sdirectory_file_name, return val; } +/* Create a temporary file whose encoded name is ENCODED_FILENAME, and + whose decoded name is *(Lisp_Object *) FLAGS. Pretend the file + already exists if a live buffer is visiting it. Return a file + descriptor if successful, -1 (setting errno) otherwise. */ +static int +create_tempname (char *encoded_filename, void *flags) +{ + Lisp_Object *addval = flags; + Lisp_Object filename = *addval; + + if (!NILP (Fget_file_buffer (filename))) + { + errno = EEXIST; + return -1; + } + + return open (encoded_filename, + O_RDWR | O_BINARY | O_CLOEXEC | O_CREAT | O_EXCL, + S_IRUSR | S_IWUSR); +} + DEFUN ("make-temp-file-internal", Fmake_temp_file_internal, Smake_temp_file_internal, 4, 4, 0, doc: /* Generate a new file whose name starts with PREFIX, a string. @@ -707,18 +728,18 @@ DEFUN ("make-temp-file-internal", Fmake_temp_file_internal, memcpy (data, SSDATA (encoded_prefix), prefix_len); memset (data + prefix_len, 'X', nX); memcpy (data + prefix_len + nX, SSDATA (encoded_suffix), suffix_len); + Lisp_Object dval = DECODE_FILE (val), ddval = dval; int kind = (NILP (dir_flag) ? GT_FILE : BASE_EQ (dir_flag, make_fixnum (0)) ? GT_NOCREATE : GT_DIR); - int fd = gen_tempname (data, suffix_len, O_BINARY | O_CLOEXEC, kind); + int fd = try_tempname (data, suffix_len, &ddval, create_tempname); bool failed = fd < 0; if (!failed) { specpdl_ref count = SPECPDL_INDEX (); record_unwind_protect_int (close_file_unwind, fd); - val = DECODE_FILE (val); if (STRINGP (text) && SBYTES (text) != 0) - write_region (text, Qnil, val, Qnil, Qnil, Qnil, Qnil, fd); + write_region (text, Qnil, dval, Qnil, Qnil, Qnil, Qnil, fd); failed = NILP (dir_flag) && emacs_close (fd) != 0; /* Discard the unwind protect. */ specpdl_ptr = specpdl_ref_to_ptr (count); @@ -733,7 +754,7 @@ DEFUN ("make-temp-file-internal", Fmake_temp_file_internal, }; report_file_error (kind_message[kind], prefix); } - return val; + return dval; } -- 2.37.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
[parent not found: <af0af29b-2362-77db-081e-046158937808@cs.ucla.edu>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <af0af29b-2362-77db-081e-046158937808@cs.ucla.edu> @ 2022-08-15 21:22 ` Bruno Haible 2022-08-16 11:04 ` Eli Zaretskii [not found] ` <835yish2l1.fsf@gnu.org> 2 siblings, 0 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-15 21:22 UTC (permalink / raw) To: bug-gnulib, Paul Eggert; +Cc: Jim Porter, Eli Zaretskii, larsi, 57129 Paul Eggert wrote: > I do see a comment saying that gen_tempname generates "hard-to-predict" > names, which as you note is not correct on MS-DOS, nor even strictly > speaking on all POSIX platforms. I installed the first attached patch > into Gnulib to fix that comment. Another comment fix is as below. Note that both comment fixes need to be propagated to glibc. 2022-08-15 Bruno Haible <bruno@clisp.org> tempname: Fix a comment. * lib/tempname.c (try_tempname_len): Use of entropy makes the function more, not less, secure. diff --git a/lib/tempname.c b/lib/tempname.c index 75a939e571..e6520191d7 100644 --- a/lib/tempname.c +++ b/lib/tempname.c @@ -273,7 +273,7 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, /* Whether to consume entropy when acquiring random bits. On the first try it's worth the entropy cost with __GT_NOCREATE, which is inherently insecure and can use the entropy to make it a bit - less secure. On the (rare) second and later attempts it might + more secure. On the (rare) second and later attempts it might help against DoS attacks. */ bool use_getrandom = tryfunc == try_nocreate; ^ permalink raw reply related [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <af0af29b-2362-77db-081e-046158937808@cs.ucla.edu> 2022-08-15 21:22 ` Bruno Haible @ 2022-08-16 11:04 ` Eli Zaretskii [not found] ` <835yish2l1.fsf@gnu.org> 2 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-16 11:04 UTC (permalink / raw) To: Paul Eggert; +Cc: jporterbugs, larsi, bug-gnulib, 57129 > Date: Mon, 15 Aug 2022 13:55:35 -0700 > Cc: larsi@gnus.org, 57129@debbugs.gnu.org, Jim Porter > <jporterbugs@gmail.com>, Gnulib bugs <bug-gnulib@gnu.org> > From: Paul Eggert <eggert@cs.ucla.edu> > > On 8/15/22 11:58, Eli Zaretskii wrote: > > > Ah, okay. It's a (mis)feature of Gnulib's gen_tempname function > > (which is the guts of make-temp-file) in its implementation for > > MS-Windows (and maybe other platforms?): it always begins from the > > same "random" characters in the file name, and only generates other > > random characters if there's already a file by that name. > > Not sure I'd call it a misfeature I didn't say "misfeature". Evidently, for you and perhaps others it's a feature. > as gen_tempname is generating a uniquely-named file that is > exclusive to Emacs, which is all it's supposed to do. Then perhaps calling it from make-temp-file in Emacs is a mistake, because that function's doc string says Create a temporary file. The returned file name (created by appending some random characters at the end of PREFIX, and expanding against ‘temporary-file-directory’ if necessary), is guaranteed to point to a newly created file. When you tell someone that the file name will include "some random characters", they don't expect that they will be the same "random characters" every call, just like saying that some function generates random numbers doesn't imply it will be the same number every call. > I'm not sure I'm entirely understanding the Emacs problem, but it > appears to be that Emacs has its own set of filenames that it thinks it > knows about, and Emacs wants the new temporary file's name to not be a > member of that set. If I'm right, does the second attached patch (this > patch is to Emacs) address the problem? I haven't tested or installed it. I don't think that's the problem, no. The problem is that callers of make-temp-file reasonably expect it to return a new name every call. And evidently, it does that on GNU/Linux (not sure how, given that the tempname.c code is supposed to be in glibc?). So if there's no intention to change gen_tempname on non-glibc platforms so that it doesn't repeat the same "random characters" upon each call, I think Emacs should call a different function to generate "random" names of temporary files, for consistency across platforms if not for other reasons. ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <835yish2l1.fsf@gnu.org>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <835yish2l1.fsf@gnu.org> @ 2022-08-16 13:35 ` Bruno Haible [not found] ` <1871347.6tgchFWduM@nimes> 1 sibling, 0 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-16 13:35 UTC (permalink / raw) To: Paul Eggert, bug-gnulib, Eli Zaretskii; +Cc: jporterbugs, larsi, 57129 Eli Zaretskii wrote: > The problem is that callers of > make-temp-file reasonably expect it to return a new name every call. > And evidently, it does that on GNU/Linux (not sure how, given that the > tempname.c code is supposed to be in glibc?). So if there's no > intention to change gen_tempname on non-glibc platforms so that it > doesn't repeat the same "random characters" upon each call, I think > Emacs should call a different function to generate "random" names of > temporary files, for consistency across platforms if not for other > reasons. You are making incorrect assumptions or hypotheses. I am adding the unit test below. It proves that (at least) on Linux, FreeBSD 11, NetBSD 7, OpenBSD 6.0, macOS, AIX 7.1, Solaris 10, Cygwin, and native Windows, gen_tempname *does* return a new file name on each call, with a very high probability. So, gen_tempname does *not* repeat the same "random characters" upon each call. 2022-08-16 Bruno Haible <bruno@clisp.org> tempname: Add tests. * tests/test-tempname.c: New file. * modules/tempname-tests: New file. =========================== tests/test-tempname.c ========================== /* Test of creating a temporary file. Copyright (C) 2022 Free Software Foundation, Inc. This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later version. This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with this program. If not, see <https://www.gnu.org/licenses/>. */ /* Written by Bruno Haible <bruno@clisp.org>, 2022. */ #include <config.h> #include "tempname.h" #include <string.h> #include <unistd.h> #include "macros.h" int main () { /* Verify that two consecutive calls to gen_tempname return two different file names, with a high probability. */ const char *templ18 = "gl-temp-XXXXXX.xyz"; char filename1[18 + 1]; char filename2[18 + 1]; strcpy (filename1, templ18); int fd1 = gen_tempname (filename1, strlen (".xyz"), 0, GT_FILE); ASSERT (fd1 >= 0); strcpy (filename2, templ18); int fd2 = gen_tempname (filename2, strlen (".xyz"), 0, GT_FILE); ASSERT (fd2 >= 0); /* With 6 'X' and a good pseudo-random number generator behind the scenes, the probability of getting the same file name twice in a row should be 1/62^6 < 1/10^10. */ ASSERT (strcmp (filename1, filename2) != 0); /* Clean up. */ close (fd1); close (fd2); unlink (filename1); unlink (filename2); return 0; } =========================== modules/tempname-tests ========================= Files: tests/test-tempname.c tests/macros.h Depends-on: unlink configure.ac: Makefile.am: TESTS += test-tempname check_PROGRAMS += test-tempname test_tempname_LDADD = $(LDADD) $(LIB_GETRANDOM) $(LIB_CLOCK_GETTIME) ============================================================================ ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <1871347.6tgchFWduM@nimes>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <1871347.6tgchFWduM@nimes> @ 2022-08-16 13:51 ` Eli Zaretskii [not found] ` <838rnofgad.fsf@gnu.org> 1 sibling, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-16 13:51 UTC (permalink / raw) To: Bruno Haible; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 > From: Bruno Haible <bruno@clisp.org> > Cc: larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com > Date: Tue, 16 Aug 2022 15:35:17 +0200 > > Eli Zaretskii wrote: > > The problem is that callers of > > make-temp-file reasonably expect it to return a new name every call. > > And evidently, it does that on GNU/Linux (not sure how, given that the > > tempname.c code is supposed to be in glibc?). So if there's no > > intention to change gen_tempname on non-glibc platforms so that it > > doesn't repeat the same "random characters" upon each call, I think > > Emacs should call a different function to generate "random" names of > > temporary files, for consistency across platforms if not for other > > reasons. > > You are making incorrect assumptions or hypotheses. They are neither assumptions nor hypotheses. They are what I actually saw by stepping with a debugger into the code. > I am adding the unit test below. It proves that (at least) on Linux, > FreeBSD 11, NetBSD 7, OpenBSD 6.0, macOS, AIX 7.1, Solaris 10, > Cygwin, and native Windows, gen_tempname *does* return a new file > name on each call, with a very high probability. > > So, gen_tempname does *not* repeat the same "random characters" upon each call. Well, it does here, when called from Emacs in the particular scenario of the unit test I was looking into. Looking at your test program, I see that you generate the seconds file name without deleting the first one. When a file by the first generated name already exists, gen_tempname will indeed generate a different name. But in the scenario I described, Emacs creates one temporary file, uses it, then deletes it, and only after that creates another file. In terms of your test program, you need to move the first unlink call to before the second call to gen_tempname. Then your test program will faithfully reproduce what Emacs does in the case in point. ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <838rnofgad.fsf@gnu.org>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <838rnofgad.fsf@gnu.org> @ 2022-08-16 14:40 ` Bruno Haible 2022-08-16 16:25 ` Eli Zaretskii [not found] ` <83wnb8dukz.fsf@gnu.org> 2022-08-16 19:59 ` Bruno Haible [not found] ` <2135151.C4sosBPzcN@nimes> 2 siblings, 2 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-16 14:40 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 Eli Zaretskii wrote: > Looking at your test program, I see that you generate the seconds file > name without deleting the first one. When a file by the first > generated name already exists, gen_tempname will indeed generate a > different name. But in the scenario I described, Emacs creates one > temporary file, uses it, then deletes it, and only after that creates > another file. Why would it be important for the second temporary file to bear a different name, once the first one is gone? I mean, process IDs get reused over time; socket numbers get reused over time; what is wrong with reusing a file name, once the older file is gone? Maybe the problem is that the file gets deleted too early, when some parts of Emacs still reference it? Bruno ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-16 14:40 ` Bruno Haible @ 2022-08-16 16:25 ` Eli Zaretskii [not found] ` <83wnb8dukz.fsf@gnu.org> 1 sibling, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-16 16:25 UTC (permalink / raw) To: Bruno Haible; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 > From: Bruno Haible <bruno@clisp.org> > Cc: eggert@cs.ucla.edu, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com > Date: Tue, 16 Aug 2022 16:40:16 +0200 > > Eli Zaretskii wrote: > > Looking at your test program, I see that you generate the seconds file > > name without deleting the first one. When a file by the first > > generated name already exists, gen_tempname will indeed generate a > > different name. But in the scenario I described, Emacs creates one > > temporary file, uses it, then deletes it, and only after that creates > > another file. > > Why would it be important for the second temporary file to bear a different > name, once the first one is gone? I mean, process IDs get reused over time; > socket numbers get reused over time; what is wrong with reusing a file name, > once the older file is gone? Because the Emacs Lisp program expected that, based on what make-temp-file in Emacs promises (see the "random characters" part), and because that's how it behaves on GNU/Linux. Then the same program behaved differently on MS-Windows, and the programmer was stumped. Sure, it's possible to write the same program somewhat differently, carefully working around this issue, but my point is that such functions should ideally present the same behavior traits on all systems, otherwise the portability is hampered. > Maybe the problem is that the file gets deleted too early, when some parts > of Emacs still reference it? The buffer which visited that file still exists, and still records the name of the file, yes. And then, when the program writes to another file created by another call to make-temp-file, it actually writes to a file that some buffer still visits, and in Emacs that triggers a prompt to the user to refresh the buffer. The programmer didn't expect that because it is natural to expect each call to a function that creates a temporary file to create a file under a new name, not reuse the same name. E.g., similar facilities in the Standard C library exhibit that behavior. So the program was written assuming that the second write was to an entirely different and unrelated file. And again, my main point is: why does this work differently on different platforms? If you consider it OK to return the same file name each time, provided that no such file exists, then why doesn't that function do it on GNU/Linux? ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <83wnb8dukz.fsf@gnu.org>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <83wnb8dukz.fsf@gnu.org> @ 2022-08-16 16:54 ` Paul Eggert [not found] ` <206e38df-2db4-a46a-e0ff-952bc8ab939c@cs.ucla.edu> ` (4 subsequent siblings) 5 siblings, 0 replies; 57+ messages in thread From: Paul Eggert @ 2022-08-16 16:54 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, larsi, bug-gnulib, 57129, Bruno Haible On 8/16/22 09:25, Eli Zaretskii wrote: > The programmer didn't > expect that because it is natural to expect each call to a function > that creates a temporary file to create a file under a new name, not > reuse the same name. Regardless of whether the programmer expects a random name or a predictable-but-unique name, there are only a finite number of names to choose from so the programmer must take into account the possibility that the chosen name clashes with names that the programmer would prefer not to be chosen. This means an Emacs patch such as [1] is needed regardless of whether Gnulib's gen_tempname is changed to be more random than it is. Although it's true that the bug fixed by [1] is less likely if gen_tempname generates more-random names, the bug can occur no matter what we do about gen_tempname. [1] https://debbugs.gnu.org/cgi/bugreport.cgi?filename=0001-Don-t-create-temp-file-with-same-name-as-visited.patch;bug=57129;msg=59;att=2 ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <206e38df-2db4-a46a-e0ff-952bc8ab939c@cs.ucla.edu>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <206e38df-2db4-a46a-e0ff-952bc8ab939c@cs.ucla.edu> @ 2022-08-16 17:04 ` Eli Zaretskii [not found] ` <83sflwdsr2.fsf@gnu.org> 1 sibling, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-16 17:04 UTC (permalink / raw) To: Paul Eggert; +Cc: jporterbugs, larsi, bug-gnulib, 57129, bruno > Date: Tue, 16 Aug 2022 09:54:25 -0700 > Cc: bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, > jporterbugs@gmail.com, Bruno Haible <bruno@clisp.org> > From: Paul Eggert <eggert@cs.ucla.edu> > > On 8/16/22 09:25, Eli Zaretskii wrote: > > The programmer didn't > > expect that because it is natural to expect each call to a function > > that creates a temporary file to create a file under a new name, not > > reuse the same name. > > Regardless of whether the programmer expects a random name or a > predictable-but-unique name, there are only a finite number of names to > choose from so the programmer must take into account the possibility > that the chosen name clashes with names that the programmer would prefer > not to be chosen. Then what is this comment and the following assertion in Bruno's code about? /* With 6 'X' and a good pseudo-random number generator behind the scenes, the probability of getting the same file name twice in a row should be 1/62^6 < 1/10^10. */ ASSERT (strcmp (filename1, filename2) != 0); That "finite number" of 62^6 sounds pretty close to infinity to me! > This means an Emacs patch such as [1] is needed regardless of whether > Gnulib's gen_tempname is changed to be more random than it is. Although > it's true that the bug fixed by [1] is less likely if gen_tempname > generates more-random names, the bug can occur no matter what we do > about gen_tempname. No, sorry. I object to this patch, because it hides the problem under the carpet. That there's a file-visiting buffer that records the name of a temporary file is just one case where this issue gets in the way. The general problem is still there. And it isn't an Emacs problem, so Emacs is not the place to solve it. Therefore, if there's no intention to fix this in Gnulib, I'm going to update the documentation of make-temp-file so that Emacs users and programmers will be informed about that and will be careful enough to side-step these issues in all the situations. (Not that I understand why won't Gnulib provide consistent behavior on all platforms, but I guess it's above my pay grade.) ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <83sflwdsr2.fsf@gnu.org>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <83sflwdsr2.fsf@gnu.org> @ 2022-08-16 17:30 ` Paul Eggert 2022-08-16 17:41 ` Eli Zaretskii [not found] ` <ceeeaa86-6199-93b1-ff65-bbd3e531e235@cs.ucla.edu> 2 siblings, 0 replies; 57+ messages in thread From: Paul Eggert @ 2022-08-16 17:30 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, larsi, bug-gnulib, 57129, bruno On 8/16/22 10:04, Eli Zaretskii wrote: > That "finite number" of 62^6 sounds pretty close to infinity to me! It's only 57 billion or so. That's not infinity, considering all the Emacs sessions out there. If Emacs's success grows, almost surely someone will run into this unlikely bug. Since we have the patch to prevent it, why not install it? ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <83sflwdsr2.fsf@gnu.org> 2022-08-16 17:30 ` Paul Eggert @ 2022-08-16 17:41 ` Eli Zaretskii [not found] ` <ceeeaa86-6199-93b1-ff65-bbd3e531e235@cs.ucla.edu> 2 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-16 17:41 UTC (permalink / raw) To: eggert, bruno; +Cc: jporterbugs, larsi, bug-gnulib, 57129 > Cc: jporterbugs@gmail.com, larsi@gnus.org, bug-gnulib@gnu.org, > 57129@debbugs.gnu.org, bruno@clisp.org > Date: Tue, 16 Aug 2022 20:04:49 +0300 > From: Eli Zaretskii <eliz@gnu.org> > > Therefore, if there's no intention to fix this in Gnulib, I'm going to > update the documentation of make-temp-file so that Emacs users and > programmers will be informed about that and will be careful enough to > side-step these issues in all the situations. (Not that I understand > why won't Gnulib provide consistent behavior on all platforms, but I > guess it's above my pay grade.) And btw, looking closer, I see that this is a regression in Emacs 28, which was caused by a change in Gnulib: the versions of tempname.c that we used up to and including Emacs 27.2 used gettimeofday and the PID to generate the file name, so it was more random than the code in current Gnulib, and in particular the sequence of generate file-name1 delete file-name1 generate file-name2 would produce file-name2 that is different from file-name1. With the current code in Gnulib, something similar happens only on glibc systems. So I hope the code in Gnulib's tempname.c will be amended to call clock_gettime or something similar, so that the names on non-glibc platforms could also be random. Or, failing that, at least that gen_tempname in glibc would behave identically to other systems, i.e. start from a fixed "random" value. ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <ceeeaa86-6199-93b1-ff65-bbd3e531e235@cs.ucla.edu>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <ceeeaa86-6199-93b1-ff65-bbd3e531e235@cs.ucla.edu> @ 2022-08-16 17:56 ` Eli Zaretskii 0 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-16 17:56 UTC (permalink / raw) To: Paul Eggert; +Cc: jporterbugs, larsi, bug-gnulib, 57129, bruno > Date: Tue, 16 Aug 2022 10:30:54 -0700 > Cc: bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, > jporterbugs@gmail.com, bruno@clisp.org > From: Paul Eggert <eggert@cs.ucla.edu> > > On 8/16/22 10:04, Eli Zaretskii wrote: > > That "finite number" of 62^6 sounds pretty close to infinity to me! > > It's only 57 billion or so. That's not infinity Yeah, right. > Since we have the patch to prevent it, why not install it? Because it isn't needed, and is a weird thing to do. It also slows down make-temp-file, especially when a session has a lot of buffers and with remote file names. So thanks, but no, thanks. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <83wnb8dukz.fsf@gnu.org> 2022-08-16 16:54 ` Paul Eggert [not found] ` <206e38df-2db4-a46a-e0ff-952bc8ab939c@cs.ucla.edu> @ 2022-08-16 17:25 ` Paul Eggert 2022-08-16 17:29 ` Bruno Haible ` (2 subsequent siblings) 5 siblings, 0 replies; 57+ messages in thread From: Paul Eggert @ 2022-08-16 17:25 UTC (permalink / raw) To: Eli Zaretskii, Bruno Haible; +Cc: jporterbugs, larsi, bug-gnulib, 57129 [-- Attachment #1: Type: text/plain, Size: 726 bytes --] On 8/16/22 09:25, Eli Zaretskii wrote: > why does this work differently on > different platforms? Platforms that lack clock_gettime and CLOCK_MONOTONIC fall back on a deterministic algorithm. Presumably MS-DOS one of the few such platforms, which is why the problem is observed only on MS-DOS. How about something like the attached patch to Gnulib's lib/tempname.c? If I understand things correctly this should make the names random enough on MS-DOS, though Emacs itself still needs a patch as I mentioned a few minutes ago. If this patch isn't good enough for MS-DOS, what sort of random bits are easily available on that platform? We don't need anything cryptographically secure; a higher-res clock should suffice. [-- Attachment #2: rand.diff --] [-- Type: text/x-patch, Size: 889 bytes --] diff --git a/lib/tempname.c b/lib/tempname.c index e6520191d7..e3d09d963d 100644 --- a/lib/tempname.c +++ b/lib/tempname.c @@ -71,7 +71,8 @@ /* Use getrandom if it works, falling back on a 64-bit linear congruential generator that starts with Var's value - mixed in with a clock's low-order bits if available. */ + mixed in with a clock's low-order bits (or with some other + pseudorandom number) if available. */ typedef uint_fast64_t random_value; #define RANDOM_VALUE_MAX UINT_FAST64_MAX #define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */ @@ -89,6 +90,9 @@ random_bits (random_value var, bool use_getrandom) struct __timespec64 tv; __clock_gettime64 (CLOCK_MONOTONIC, &tv); var ^= tv.tv_nsec; +#else + /* Fall back on pseudorandom number <https://bugs.gnu.org/57129>. */ + var ^= rand (); #endif return 2862933555777941757 * var + 3037000493; } ^ permalink raw reply related [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <83wnb8dukz.fsf@gnu.org> ` (2 preceding siblings ...) 2022-08-16 17:25 ` Paul Eggert @ 2022-08-16 17:29 ` Bruno Haible [not found] ` <f329244a-cba7-65cd-2e5d-2630eba3e9e9@cs.ucla.edu> [not found] ` <2594092.Isy0gbHreE@nimes> 5 siblings, 0 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-16 17:29 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 Thanks for the explanations, Eli. Eli Zaretskii wrote: > > Maybe the problem is that the file gets deleted too early, when some parts > > of Emacs still reference it? > > The buffer which visited that file still exists, and still records the > name of the file, yes. And then, when the program writes to another > file created by another call to make-temp-file, it actually writes to > a file that some buffer still visits, and in Emacs that triggers a > prompt to the user to refresh the buffer. That is a reasonable behaviour for a text editor — but only for file names that are explicitly controlled by some program or by the user, not for temporary files. > The programmer didn't > expect that because it is natural to expect each call to a function > that creates a temporary file to create a file under a new name, not > reuse the same name. This is an incorrect assumption. Just like socket numbers are randomly generated in some situations, temp file names are random, and you can't make assumptions about the resulting file name. How about storing, as an attribute of the buffer, a boolean that tells whether the underlying file name is randomly generated (i.e. a temp file), and query this boolean before prompting to the user to refresh the buffer? Bruno ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <f329244a-cba7-65cd-2e5d-2630eba3e9e9@cs.ucla.edu>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <f329244a-cba7-65cd-2e5d-2630eba3e9e9@cs.ucla.edu> @ 2022-08-16 17:47 ` Eli Zaretskii 2022-08-16 19:11 ` Paul Eggert [not found] ` <d95734ab-6bbc-7403-c1f8-fbf742badda4@cs.ucla.edu> 0 siblings, 2 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-16 17:47 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib, larsi, bruno, 57129, jporterbugs > Date: Tue, 16 Aug 2022 10:25:57 -0700 > Cc: bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, > jporterbugs@gmail.com > From: Paul Eggert <eggert@cs.ucla.edu> > > On 8/16/22 09:25, Eli Zaretskii wrote: > > why does this work differently on > > different platforms? > > Platforms that lack clock_gettime and CLOCK_MONOTONIC fall back on a > deterministic algorithm. Presumably MS-DOS one of the few such > platforms, which is why the problem is observed only on MS-DOS. (Why are you talking about MS-DOS?) MinGW does have clock_gettime, but we don't want to use it, because one of the MinGW flavors (MinGW64) implements that function in libwinpthreads, and that makes Emacs depend on libwinpthreads DLL, which we want to avoid. > How about something like the attached patch to Gnulib's lib/tempname.c? Thanks, but why not use 'random' instead? Emacs does have it on all platforms, including MS-Windows. AFAIU, it's better than 'rand'. > If I understand things correctly this should make the names random > enough on MS-DOS, though Emacs itself still needs a patch as I mentioned > a few minutes ago. Why would Emacs need that patch? ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-16 17:47 ` Eli Zaretskii @ 2022-08-16 19:11 ` Paul Eggert [not found] ` <d95734ab-6bbc-7403-c1f8-fbf742badda4@cs.ucla.edu> 1 sibling, 0 replies; 57+ messages in thread From: Paul Eggert @ 2022-08-16 19:11 UTC (permalink / raw) To: Eli Zaretskii; +Cc: bug-gnulib, larsi, bruno, 57129, jporterbugs [-- Attachment #1: Type: text/plain, Size: 1348 bytes --] On 8/16/22 10:47, Eli Zaretskii wrote: > (Why are you talking about MS-DOS?) I mistakenly thought it was an MS-DOS problem because tempname.c ordinarily would use clock_gettime on MinGW. I didn't know Emacs 'configure' deliberately suppresses MinGW's clock_gettime. > Thanks, but why not use 'random' instead? Emacs does have it on all > platforms, including MS-Windows. AFAIU, it's better than 'rand'. If the code used 'random' then the Gnulib 'tempname' module would need to add a dependency on the Gnulib 'random' module, which would in turn add a cascading dependency on Gnulib's 'random_r' module. It's better to avoid this dependency if we can easily do so. Come to think of it, we don't need to use 'rand' either, since tempname.c already has a good-enough pseudorandom generator. I installed into Gnulib the attached patch, which I hope fixes the Emacs problem without changing glibc's generated code (once this gets migrated back into glibc). >> If I understand things correctly this should make the names random >> enough on MS-DOS, though Emacs itself still needs a patch as I mentioned >> a few minutes ago. > > Why would Emacs need that patch? In another part of this thread you rejected that patch, on the grounds that fixing the unlikely Emacs bug is more trouble than it's worth. So I'll drop that suggestion. [-- Attachment #2: 0001-tempname-generate-better-names-for-MinGW-Emacs.patch --] [-- Type: text/x-patch, Size: 3592 bytes --] From 512e44adaebb3096ddd1bf564e679d06e0301616 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Tue, 16 Aug 2022 12:06:48 -0700 Subject: [PATCH] tempname: generate better names for MinGW Emacs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On MinGW, GNU Emacs disables clock_gettime, which reliably breaks some of gen_tempname’s optimistic callers. Work around the problem by making the generated names less predictable. We don’t need cryptographic randomness here, just enough unpredictability to keep Emacs happy most of the time. * lib/tempname.c (HAS_CLOCK_ENTROPY): New macro. (random_bits): Use it. (try_tempname_len): On systems lacking clock entropy, maintain state so that gen_filename generates less-predictable names on successive successful calls. --- ChangeLog | 14 ++++++++++++++ lib/tempname.c | 18 +++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index b639d1709d..eb96281591 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,17 @@ +2022-08-16 Paul Eggert <eggert@cs.ucla.edu> + + tempname: generate better names for MinGW Emacs + On MinGW, GNU Emacs disables clock_gettime, which reliably breaks + some of gen_tempname’s optimistic callers. Work around the + problem by making the generated names less predictable. We don’t + need cryptographic randomness here, just enough unpredictability + to keep Emacs happy most of the time. + * lib/tempname.c (HAS_CLOCK_ENTROPY): New macro. + (random_bits): Use it. + (try_tempname_len): On systems lacking clock entropy, maintain + state so that gen_filename generates less-predictable names on + successive successful calls. + 2022-08-16 Simon Josefsson <simon@josefsson.org> maintainer-makefile: Check for incorrect DISTCHECK_CONFIGURE_FLAGS diff --git a/lib/tempname.c b/lib/tempname.c index e6520191d7..5adfe629a8 100644 --- a/lib/tempname.c +++ b/lib/tempname.c @@ -77,6 +77,12 @@ typedef uint_fast64_t random_value; #define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */ #define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62) +#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME) +# define HAS_CLOCK_ENTROPY true +#else +# define HAS_CLOCK_ENTROPY false +#endif + static random_value random_bits (random_value var, bool use_getrandom) { @@ -84,7 +90,7 @@ random_bits (random_value var, bool use_getrandom) /* Without GRND_NONBLOCK it can be blocked for minutes on some systems. */ if (use_getrandom && __getrandom (&r, sizeof r, GRND_NONBLOCK) == sizeof r) return r; -#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME) +#if HAS_CLOCK_ENTROPY /* Add entropy if getrandom did not work. */ struct __timespec64 tv; __clock_gettime64 (CLOCK_MONOTONIC, &tv); @@ -267,6 +273,13 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, alignment. */ random_value v = ((uintptr_t) &v) / alignof (max_align_t); +#if !HAS_CLOCK_ENTROPY + /* Arrange gen_tempname to return less predictable file names on + systems lacking clock entropy <https://bugs.gnu.org/57129>. */ + static random_value prev_v; + v ^= prev_v; +#endif + /* How many random base-62 digits can currently be extracted from V. */ int vdigits = 0; @@ -318,6 +331,9 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, if (fd >= 0) { __set_errno (save_errno); +#if !HAS_CLOCK_ENTROPY + prev_v = v; +#endif return fd; } else if (errno != EEXIST) -- 2.34.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
[parent not found: <d95734ab-6bbc-7403-c1f8-fbf742badda4@cs.ucla.edu>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <d95734ab-6bbc-7403-c1f8-fbf742badda4@cs.ucla.edu> @ 2022-08-16 20:12 ` Bruno Haible 2022-08-17 11:08 ` Eli Zaretskii [not found] ` <83h72bdt4z.fsf@gnu.org> 2 siblings, 0 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-16 20:12 UTC (permalink / raw) To: Eli Zaretskii, Paul Eggert; +Cc: jporterbugs, larsi, bug-gnulib, 57129 Paul Eggert wrote: > Emacs 'configure' deliberately suppresses MinGW's clock_gettime. Ah, this explains why Eli saw the problem in Emacs in a deterministic manner, whereas I see it only with 0.3% probability in my tests on 64-bit mingw. > I installed > into Gnulib the attached patch, which I hope fixes the Emacs problem > without changing glibc's generated code (once this gets migrated back > into glibc). In 10000 runs on 64-bit mingw, your patch went from 27 test failures to 11 test failures. So, we're still at ca. 0.1% probability (*), far above the 62^-6 or 10^-10 probability that one would have hoped for. (*) I wouldn't consider these measurements reliable. Maybe the probability did not change, and what I'm seeing is merely a measurement fluke. Bruno ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <d95734ab-6bbc-7403-c1f8-fbf742badda4@cs.ucla.edu> 2022-08-16 20:12 ` Bruno Haible @ 2022-08-17 11:08 ` Eli Zaretskii [not found] ` <83h72bdt4z.fsf@gnu.org> 2 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-17 11:08 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib, larsi, bruno, 57129, jporterbugs > Date: Tue, 16 Aug 2022 12:11:24 -0700 > Cc: bruno@clisp.org, bug-gnulib@gnu.org, larsi@gnus.org, > 57129@debbugs.gnu.org, jporterbugs@gmail.com > From: Paul Eggert <eggert@cs.ucla.edu> > > > Thanks, but why not use 'random' instead? Emacs does have it on all > > platforms, including MS-Windows. AFAIU, it's better than 'rand'. > > If the code used 'random' then the Gnulib 'tempname' module would need > to add a dependency on the Gnulib 'random' module, which would in turn > add a cascading dependency on Gnulib's 'random_r' module. It's better to > avoid this dependency if we can easily do so. > > Come to think of it, we don't need to use 'rand' either, since > tempname.c already has a good-enough pseudorandom generator. I installed > into Gnulib the attached patch, which I hope fixes the Emacs problem > without changing glibc's generated code (once this gets migrated back > into glibc). Thanks. Would it be possible to cherry-pick this to the emacs-28 branch, so that Emacs 28.2 would have this fixed? ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <83h72bdt4z.fsf@gnu.org>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <83h72bdt4z.fsf@gnu.org> @ 2022-08-18 6:05 ` Paul Eggert [not found] ` <57b8f10f-8e9b-5951-e5ad-8cba2a8cb569@cs.ucla.edu> 1 sibling, 0 replies; 57+ messages in thread From: Paul Eggert @ 2022-08-18 6:05 UTC (permalink / raw) To: Eli Zaretskii; +Cc: bug-gnulib, larsi, bruno, 57129, jporterbugs On 8/17/22 04:08, Eli Zaretskii wrote: > Would it be possible to cherry-pick this to the emacs-28 branch, so > that Emacs 28.2 would have this fixed? I installed that. ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <57b8f10f-8e9b-5951-e5ad-8cba2a8cb569@cs.ucla.edu>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <57b8f10f-8e9b-5951-e5ad-8cba2a8cb569@cs.ucla.edu> @ 2022-08-18 6:44 ` Eli Zaretskii 0 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-18 6:44 UTC (permalink / raw) To: Paul Eggert; +Cc: bug-gnulib, larsi, bruno, 57129, jporterbugs > Date: Wed, 17 Aug 2022 23:05:33 -0700 > Cc: bruno@clisp.org, bug-gnulib@gnu.org, larsi@gnus.org, > 57129@debbugs.gnu.org, jporterbugs@gmail.com > From: Paul Eggert <eggert@cs.ucla.edu> > > On 8/17/22 04:08, Eli Zaretskii wrote: > > Would it be possible to cherry-pick this to the emacs-28 branch, so > > that Emacs 28.2 would have this fixed? > > I installed that. Thanks! ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <2594092.Isy0gbHreE@nimes>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <2594092.Isy0gbHreE@nimes> @ 2022-08-16 17:52 ` Eli Zaretskii 2022-08-16 20:06 ` Bruno Haible [not found] ` <2606289.q0ZmV6gNhb@nimes> 0 siblings, 2 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-16 17:52 UTC (permalink / raw) To: Bruno Haible; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 > From: Bruno Haible <bruno@clisp.org> > Cc: eggert@cs.ucla.edu, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com > Date: Tue, 16 Aug 2022 19:29:33 +0200 > > > The buffer which visited that file still exists, and still records the > > name of the file, yes. And then, when the program writes to another > > file created by another call to make-temp-file, it actually writes to > > a file that some buffer still visits, and in Emacs that triggers a > > prompt to the user to refresh the buffer. > > That is a reasonable behaviour for a text editor — but only for file > names that are explicitly controlled by some program or by the user, > not for temporary files. Why not for temporary files? Emacs has a with-temp-file macro, which generates a temporary file name, executes the body of the macro with a variable bound to the file name, then deletes the file. Very handy for writing test suites. We don't usually bother deleting the buffers where the file was processed in those tests, because the test suite runs in batch mode, and Emacs exits after running the tests, thus cleaning up. Having to carefully make sure no such buffers were left from the previous test is a nuisance. > > The programmer didn't > > expect that because it is natural to expect each call to a function > > that creates a temporary file to create a file under a new name, not > > reuse the same name. > > This is an incorrect assumption. Just like socket numbers are randomly > generated in some situations, temp file names are random, and you can't > make assumptions about the resulting file name. People get their ideas from other similar APIs, and functions like tmpfile in C do generate a different name each call. > How about storing, as an attribute of the buffer, a boolean that tells > whether the underlying file name is randomly generated (i.e. a temp file), > and query this boolean before prompting to the user to refresh the buffer? That's a terrible complications, especially since the solution is so close at hand, and it makes gen_tempname behave on Windows as it does on GNU/Linux, and as it behaved just a year or two ago. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-16 17:52 ` Eli Zaretskii @ 2022-08-16 20:06 ` Bruno Haible [not found] ` <2606289.q0ZmV6gNhb@nimes> 1 sibling, 0 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-16 20:06 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 Eli Zaretskii wrote: > Emacs has a with-temp-file macro, which generates a temporary file > name, executes the body of the macro with a variable bound to the file > name, then deletes the file. Very handy for writing test suites. Since, even with Paul's newest patch, gen_tempname on 64-bit mingw produces the same file name as on the previous call with a probability of about 0.1%, you still need to be prepared to see the original problem occasionally. Bruno ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <2606289.q0ZmV6gNhb@nimes>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <2606289.q0ZmV6gNhb@nimes> @ 2022-08-17 11:29 ` Eli Zaretskii 0 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-17 11:29 UTC (permalink / raw) To: Bruno Haible; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 > From: Bruno Haible <bruno@clisp.org> > Cc: eggert@cs.ucla.edu, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com > Date: Tue, 16 Aug 2022 22:06:13 +0200 > > Eli Zaretskii wrote: > > Emacs has a with-temp-file macro, which generates a temporary file > > name, executes the body of the macro with a variable bound to the file > > name, then deletes the file. Very handy for writing test suites. > > Since, even with Paul's newest patch, gen_tempname on 64-bit mingw > produces the same file name as on the previous call with a probability > of about 0.1%, you still need to be prepared to see the original problem > occasionally. I'll take that risk, thank you. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <838rnofgad.fsf@gnu.org> 2022-08-16 14:40 ` Bruno Haible @ 2022-08-16 19:59 ` Bruno Haible [not found] ` <2135151.C4sosBPzcN@nimes> 2 siblings, 0 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-16 19:59 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 [-- Attachment #1: Type: text/plain, Size: 887 bytes --] Eli Zaretskii wrote: > Looking at your test program, I see that you generate the seconds file > name without deleting the first one. When a file by the first > generated name already exists, gen_tempname will indeed generate a > different name. But in the scenario I described, Emacs creates one > temporary file, uses it, then deletes it, and only after that creates > another file. I'm adding this scenario to the unit test. Still, the unit test succeeds when run once on: Linux, FreeBSD, NetBSD, OpenBSD, macOS, Solaris, Cygwin, native Windows. Since this contradicts what you debugged on mingw, I ran the test 10000 times on native Windows. Result: - on 32-bit mingw, no failure. - on 64-bit mingw, around 30 failures (or around 10 failures with Paul's newest patch). That is, a probability of ca. 0.3% of getting the same file name as on the previous call. Bruno [-- Attachment #2: 0001-tempname-Add-more-tests.patch --] [-- Type: text/x-patch, Size: 3713 bytes --] From aa52cadc36fb1af0509dc3a4bce4ce73197ece68 Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Tue, 16 Aug 2022 21:50:11 +0200 Subject: [PATCH] tempname: Add more tests. Based on scenario described by Eli Zaretskii in <https://lists.gnu.org/archive/html/bug-gnulib/2022-08/msg00043.html>. * tests/test-tempname.c (main): Add another test. * modules/tempname-tests (Status): Mark the test as unportable. --- ChangeLog | 8 ++++++ modules/tempname-tests | 3 ++ tests/test-tempname.c | 65 ++++++++++++++++++++++++++++++------------ 3 files changed, 58 insertions(+), 18 deletions(-) diff --git a/ChangeLog b/ChangeLog index eb96281591..de113ce081 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2022-08-16 Bruno Haible <bruno@clisp.org> + + tempname: Add more tests. + Based on scenario described by Eli Zaretskii in + <https://lists.gnu.org/archive/html/bug-gnulib/2022-08/msg00043.html>. + * tests/test-tempname.c (main): Add another test. + * modules/tempname-tests (Status): Mark the test as unportable. + 2022-08-16 Paul Eggert <eggert@cs.ucla.edu> tempname: generate better names for MinGW Emacs diff --git a/modules/tempname-tests b/modules/tempname-tests index 384f98707b..adccd0d8e9 100644 --- a/modules/tempname-tests +++ b/modules/tempname-tests @@ -2,6 +2,9 @@ Files: tests/test-tempname.c tests/macros.h +Status: +unportable-test + Depends-on: unlink diff --git a/tests/test-tempname.c b/tests/test-tempname.c index d463eec2b4..4a0ca65f2c 100644 --- a/tests/test-tempname.c +++ b/tests/test-tempname.c @@ -34,24 +34,53 @@ main () char filename1[18 + 1]; char filename2[18 + 1]; - strcpy (filename1, templ18); - int fd1 = gen_tempname (filename1, strlen (".xyz"), 0, GT_FILE); - ASSERT (fd1 >= 0); - - strcpy (filename2, templ18); - int fd2 = gen_tempname (filename2, strlen (".xyz"), 0, GT_FILE); - ASSERT (fd2 >= 0); - - /* With 6 'X' and a good pseudo-random number generator behind the scenes, - the probability of getting the same file name twice in a row should be - 1/62^6 < 1/10^10. */ - ASSERT (strcmp (filename1, filename2) != 0); - - /* Clean up. */ - close (fd1); - close (fd2); - unlink (filename1); - unlink (filename2); + /* Case 1: The first file still exists while gen_tempname is called a second + time. */ + { + strcpy (filename1, templ18); + int fd1 = gen_tempname (filename1, strlen (".xyz"), 0, GT_FILE); + ASSERT (fd1 >= 0); + + strcpy (filename2, templ18); + int fd2 = gen_tempname (filename2, strlen (".xyz"), 0, GT_FILE); + ASSERT (fd2 >= 0); + + /* gen_tempname arranges (via O_EXCL) to not return the name of an existing + file. */ + ASSERT (strcmp (filename1, filename2) != 0); + + /* Clean up. */ + close (fd1); + close (fd2); + unlink (filename1); + unlink (filename2); + } + + /* Case 2: The first file is deleted before gen_tempname is called a second + time. */ + { + strcpy (filename1, templ18); + int fd1 = gen_tempname (filename1, strlen (".xyz"), 0, GT_FILE); + ASSERT (fd1 >= 0); + + /* Clean up. */ + close (fd1); + unlink (filename1); + + strcpy (filename2, templ18); + int fd2 = gen_tempname (filename2, strlen (".xyz"), 0, GT_FILE); + ASSERT (fd2 >= 0); + + /* With 6 'X' and a good pseudo-random number generator behind the scenes, + the probability of getting the same file name twice in a row should be + 1/62^6 < 1/10^10. + But on 64-bit native Windows, this probability is ca. 0.1% to 0.3%. */ + ASSERT (strcmp (filename1, filename2) != 0); + + /* Clean up. */ + close (fd2); + unlink (filename2); + } return 0; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
[parent not found: <2135151.C4sosBPzcN@nimes>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <2135151.C4sosBPzcN@nimes> @ 2022-08-16 20:53 ` Paul Eggert 2022-08-21 16:20 ` Bruno Haible 0 siblings, 1 reply; 57+ messages in thread From: Paul Eggert @ 2022-08-16 20:53 UTC (permalink / raw) To: Bruno Haible, Eli Zaretskii; +Cc: jporterbugs, larsi, bug-gnulib, 57129 On 8/16/22 12:59, Bruno Haible wrote: > Since this contradicts what you debugged on mingw, I ran the test 10000 > times on native Windows. Result: > - on 32-bit mingw, no failure. > - on 64-bit mingw, around 30 failures (or around 10 failures with Paul's > newest patch). That is, a probability of ca. 0.3% of getting the same > file name as on the previous call. That's odd, for two reasons. First, 64-bit and 32-bit mingw shouldn't differ, as they both use uint_fast64_t which should be the same width on both platforms. Second, I could not reproduce the problem on 64-bit Ubuntu x86-64 (after altering tempname.c to always define HAS_CLOCK_ENTROPY to false) test-tempname always succeeded in 10000 tries. Could you investigate further why mingw 64-bit fails? ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-16 20:53 ` Paul Eggert @ 2022-08-21 16:20 ` Bruno Haible 2022-08-21 16:32 ` Eli Zaretskii 2022-08-21 17:17 ` Bruno Haible 0 siblings, 2 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-21 16:20 UTC (permalink / raw) To: Eli Zaretskii, Paul Eggert; +Cc: jporterbugs, larsi, bug-gnulib, 57129 [-- Attachment #1: Type: text/plain, Size: 2604 bytes --] Paul Eggert wrote: > > Since this contradicts what you debugged on mingw, I ran the test 10000 > > times on native Windows. Result: > > - on 32-bit mingw, no failure. > > - on 64-bit mingw, around 30 failures (or around 10 failures with Paul's > > newest patch). That is, a probability of ca. 0.3% of getting the same > > file name as on the previous call. > > That's odd, for two reasons. First, 64-bit and 32-bit mingw shouldn't > differ, as they both use uint_fast64_t which should be the same width on > both platforms. Second, I could not reproduce the problem on 64-bit > Ubuntu x86-64 (after altering tempname.c to always define > HAS_CLOCK_ENTROPY to false) test-tempname always succeeded in 10000 tries. > > Could you investigate further why mingw 64-bit fails? That's odd indeed, and here are more detailed results. I changed test-tempname.c to skip the case 1 and execute only case 2, and added printf statements per the attached tempname.c.diff. Then ran $ for i in `seq 1000`; do ./test-tempname.exe; done 2>&1 | tee out1000 In 32-bit mingw, the result is fully deterministic: each run behaves the same. The first file name is always gl-temp-3FXzHa.xyz; the second file name is always gl-temp-HgtmVy.xyz. Thus, for a single Emacs process, things will look fine, but as soon as someone starts to use temporary files in two different Emacs processes, in the way Eli described, there will be massive collisions. In 64-bit mingw, the 'tempname 1' value is deterministic. This simply shows that Windows 10 does not use address space randomization (ASLR). The 'tempname 2' value is unique 99% of the time: $ grep 'tempname 2' out1000-mingw64 | sort | uniq -d | wc -l 8 The interesting thing is that each of the duplicate values of v is occurring in the same process: $ grep 'tempname 2' out1000-mingw64 | sort | uniq -d tempname 2 v=0x00c1efa91fb60900 tempname 2 v=0x32ccb2946974f2f6 tempname 2 v=0x5cafcc69e359a147 tempname 2 v=0x6d0676018e27d771 tempname 2 v=0x6d95bd6083168079 tempname 2 v=0x7cb95116ffae8ece tempname 2 v=0xe0afc7086808ce33 tempname 2 v=0xe46a60c28fb0ec7f $ grep 'tempname 2' out1000-mingw64 | grep -n 0x00c1efa91fb60900 560:tempname 2 v=0x00c1efa91fb60900 561:tempname 2 v=0x00c1efa91fb60900 $ grep 'tempname 2' out1000-mingw64 | grep -n 0x32ccb2946974f2f6 1129:tempname 2 v=0x32ccb2946974f2f6 1130:tempname 2 v=0x32ccb2946974f2f6 etc. So, in this environment, different Emacs processes will not conflict. But within a single Emacs process, with 1% probability, the two resulting file names are the same. Bruno [-- Attachment #2: tempname.c.diff --] [-- Type: text/x-patch, Size: 817 bytes --] --- lib/tempname.c 2022-08-16 21:27:10.455608233 +0200 +++ tempname.c 2022-08-17 01:55:47.590241280 +0200 @@ -308,6 +308,7 @@ for (count = 0; count < attempts; ++count) { +fprintf(stderr,"tempname 1 v=0x%016llx\n", v); fflush(stderr); for (size_t i = 0; i < x_suffix_len; i++) { if (vdigits == 0) @@ -315,6 +316,7 @@ do { v = random_bits (v, use_getrandom); +fprintf(stderr,"tempname 2 v=0x%016llx\n", v); fflush(stderr); use_getrandom = true; } while (unfair_min <= v); @@ -326,7 +328,7 @@ v /= 62; vdigits--; } - +fprintf(stderr,"tempname filename=%s\n", tmpl); fflush(stderr); fd = tryfunc (tmpl, args); if (fd >= 0) { [-- Attachment #3: out1000-mingw32.gz --] [-- Type: application/gzip, Size: 942 bytes --] [-- Attachment #4: out1000-mingw64.gz --] [-- Type: application/gzip, Size: 38377 bytes --] ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-21 16:20 ` Bruno Haible @ 2022-08-21 16:32 ` Eli Zaretskii 2022-08-21 17:17 ` Bruno Haible 1 sibling, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-21 16:32 UTC (permalink / raw) To: Bruno Haible; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 > From: Bruno Haible <bruno@clisp.org> > Cc: bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com > Date: Sun, 21 Aug 2022 18:20:52 +0200 > > I changed test-tempname.c to skip the case 1 and execute only case 2, and > added printf statements per the attached tempname.c.diff. Then ran > $ for i in `seq 1000`; do ./test-tempname.exe; done 2>&1 | tee out1000 > > In 32-bit mingw, the result is fully deterministic: each run behaves the same. > The first file name is always gl-temp-3FXzHa.xyz; > the second file name is always gl-temp-HgtmVy.xyz. > > Thus, for a single Emacs process, things will look fine, but as soon as someone > starts to use temporary files in two different Emacs processes, in the way Eli > described, there will be massive collisions. > > In 64-bit mingw, the 'tempname 1' value is deterministic. This simply shows > that Windows 10 does not use address space randomization (ASLR). Some of these problems could be alleviated if the initial value of 'v' depended on something non-deterministic, like the program's PID and/or the current wallclock time (no need to use anything as fancy as clock_gettime, a simple call to 'clock' should be enough). ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-21 16:20 ` Bruno Haible 2022-08-21 16:32 ` Eli Zaretskii @ 2022-08-21 17:17 ` Bruno Haible 2022-08-22 20:47 ` Paul Eggert [not found] ` <2dc7ede0-eca7-baf5-f89a-f5d292b80808@cs.ucla.edu> 1 sibling, 2 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-21 17:17 UTC (permalink / raw) To: Paul Eggert; +Cc: jporterbugs, Eli Zaretskii, bug-gnulib, larsi, 57129 Paul Eggert asked: > > Could you investigate further why mingw 64-bit fails? Some words on the "why". You seem to have expectations regarding the distribution quality of the resulting file names, but these expectations are not warranted. Donald E. Knuth wrote in TAOCP vol 2 "Seminumerical algorithms" that an arbitrary sequence of number manipulating operations usually has a bad quality as a pseudo-random number generator, and that in order to get good quality pseudo-random numbers, one needs to use *specific* pseudo-random number generators and then *prove* mathematical properties of it. (The same is true, btw, for crypto algorithms.) Then he started discussing linear congruential generators [1] as the simplest example for which something could be proved. The current source code of tempname.c generates the pseudo-random numbers — assuming no HAS_CLOCK_ENTROPY and no ASLR — using a mix of three operations: (A) A linear congruential generator [2] with m = 2^64, a = 2862933555777941757, c = 3037000493. (B) A floor operation: v ← floor(v / 62^6) (C) A xor operation: v ^= prev_v There are three different questions: (Q1) What is the expected quality inside a single gen_tempname call? (Q2) What is the expected quality of multiple gen_tempname calls in a single process? (Q3) What is the expected quality when considering different processes that call gen_tempname? Answers: (Q1) For just 6 'X'es, there is essentially a single (A) operation. Therefore the quality will be good. If someone uses more than 10 'X'es, for example, 50 'X'es, there will be 5 (A) and 5 (B), interleaved: (A), (B), (A), (B), ... This is *not* a linear congruential generator, therefore the expected quality is BAD. In order to fix this case, what I would do is to get back to a linear congruential generator: (A), (A), ..., (A), (B). In other words, feed into (A) exactly the output from the previous (A). This means, do the statements XXXXXX[i] = letters[v % 62]; v /= 62; not on v itself, but on a copy of v. But wait, there is also the desire to have predictability! This means to not consume all the possible bits the random_bits() call, but only part of it. What I would do here, is to reduce BASE_62_DIGITS from 10 to 6. So that in each round of the loop, 6 base-62 digits are consumed and more than 4 base-62 digits are left in v, for predictability. In the usual calls with 6 'X'es the loop will still end after a single round. (Q2) First of all, the multiple gen_tempname calls can occur in different threads. Since no locking is involved, it is undefined behaviour to access the 'prev_v' variable from different threads. On machines with an IA-64 CPU, the 'prev_v' variable's value may not be propagated from one thread to the other. [3][4][5] The fix is simple, though: Mark 'prev_v' as 'volatile'. Then, what the code does, is a mix of (A), (B), (C). Again, this is *not* a linear congruential generator, therefore the expected quality is BAD. To get good quality, I would suggest to use a linear congruential generator across *all* gen_tempname calls of the entire thread. This means: - Move out the (B) invocations out, like explained above in (Q1). - Remove the (C) code that you added last week. - Store the v value in a per-thread variable. Using '__thread' on glibc systems and a TLS variable (#include "glthread/tls.h") on the other platforms. (Q3) Here, to force differences between different processes, I would suggest to use a fine-grained clock value. In terms of platforms, #if defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME is way too restrictive. How about - using CLOCK_REALTIME when CLOCK_MONOTONIC is not available, - using gettimeofday() as fallback, especially on native Windows. If one does (Q3), then the suggestions for (Q2) (other than the 'volatile') may not be needed. Bruno [1] https://en.wikipedia.org/wiki/Linear_congruential_generator [2] https://en.wikipedia.org/wiki/Linear_congruential_generator#c_%E2%89%A0_0 [3] https://es.cs.uni-kl.de/publications/datarsg/Geor16.pdf [4] https://db.in.tum.de/teaching/ws1718/dataprocessing/chapter3.pdf?lang=de page 18 [5] https://os.inf.tu-dresden.de/Studium/DOS/SS2014/04-Memory-Consistency.pdf ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-21 17:17 ` Bruno Haible @ 2022-08-22 20:47 ` Paul Eggert [not found] ` <2dc7ede0-eca7-baf5-f89a-f5d292b80808@cs.ucla.edu> 1 sibling, 0 replies; 57+ messages in thread From: Paul Eggert @ 2022-08-22 20:47 UTC (permalink / raw) To: Bruno Haible; +Cc: jporterbugs, Eli Zaretskii, bug-gnulib, larsi, 57129 [-- Attachment #1: Type: text/plain, Size: 873 bytes --] Thanks for the detailed diagnosis, Bruno. To try to fix the problems I installed the attached patches into Gnulib. If I understand things correctly, these patches should fix the 0.1% failure rate you observed on 64-bit mingw. They also fix a minor security leak I discovered: in rare cases, ASLR entropy was used to generate publicly visible file names, which is a no-no as that might help an attacker infer the randomized layout of a victim process. These fixes follow some but not all the suggestions you made. The basic problem I saw was that tempname.c was using too much belt-and-suspenders code, so much so that the combination of belts and suspenders misbehaved. I simplified it a bit and this removed the need for some of the suggestions. These fixes should be merged into glibc upstream since they fix glibc bugs; I plan to follow up on that shortly. [-- Attachment #2: 0001-tempname-merge-64-bit-time_t-fix-from-glibc.patch --] [-- Type: text/x-patch, Size: 2463 bytes --] From 8304617684ba7f71c36fcf49786d3b279dfbefc3 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Mon, 22 Aug 2022 12:22:52 -0700 Subject: [PATCH 1/3] tempname: merge 64-bit time_t fix from glibc This merges glibc commit 52a5fe70a2c77935afe807fb6e904e512ddd894e "Use 64 bit time_t stat internally". * lib/tempname.c (struct_stat64) [_LIBC]: Use struct __stat64_t64. (__lstat64_time64) [!_LIBC]: Rename from __lstat64. All uses changed. (direxists): Use __stat64_time64 instead of __stat64. --- ChangeLog | 10 ++++++++++ lib/tempname.c | 8 ++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index de113ce081..adb445ddcf 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2022-08-22 Paul Eggert <eggert@cs.ucla.edu> + + tempname: merge 64-bit time_t fix from glibc + This merges glibc commit 52a5fe70a2c77935afe807fb6e904e512ddd894e + "Use 64 bit time_t stat internally". + * lib/tempname.c (struct_stat64) [_LIBC]: Use struct __stat64_t64. + (__lstat64_time64) [!_LIBC]: Rename from __lstat64. + All uses changed. + (direxists): Use __stat64_time64 instead of __stat64. + 2022-08-16 Bruno Haible <bruno@clisp.org> tempname: Add more tests. diff --git a/lib/tempname.c b/lib/tempname.c index 5adfe629a8..4f615b90b9 100644 --- a/lib/tempname.c +++ b/lib/tempname.c @@ -55,14 +55,14 @@ #include <time.h> #if _LIBC -# define struct_stat64 struct stat64 +# define struct_stat64 struct __stat64_t64 # define __secure_getenv __libc_secure_getenv #else # define struct_stat64 struct stat # define __gen_tempname gen_tempname # define __mkdir mkdir # define __open open -# define __lstat64(file, buf) lstat (file, buf) +# define __lstat64_time64(file, buf) lstat (file, buf) # define __stat64(file, buf) stat (file, buf) # define __getrandom getrandom # define __clock_gettime64 clock_gettime @@ -105,7 +105,7 @@ static int direxists (const char *dir) { struct_stat64 buf; - return __stat64 (dir, &buf) == 0 && S_ISDIR (buf.st_mode); + return __stat64_time64 (dir, &buf) == 0 && S_ISDIR (buf.st_mode); } /* Path search algorithm, for tmpnam, tmpfile, etc. If DIR is @@ -197,7 +197,7 @@ try_nocreate (char *tmpl, _GL_UNUSED void *flags) { struct_stat64 st; - if (__lstat64 (tmpl, &st) == 0 || errno == EOVERFLOW) + if (__lstat64_time64 (tmpl, &st) == 0 || errno == EOVERFLOW) __set_errno (EEXIST); return errno == ENOENT ? 0 : -1; } -- 2.37.2 [-- Attachment #3: 0002-tempname-fix-multithreading-ASLR-leak-etc.patch --] [-- Type: text/x-patch, Size: 8768 bytes --] From 9ce573cde017182a69881241e8565ec04e5bc728 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Mon, 22 Aug 2022 12:07:27 -0700 Subject: [PATCH 2/3] tempname: fix multithreading, ASLR leak etc. Fix problems with tempname and multithreading, entropy loss, and missing clock data (this last on non-GNU platforms). See analysis by Bruno Haible in: https://bugs.gnu.org/57129#149 While looking into this, I noticed that tempname can leak info derived from ASLR into publicly-visible file names, which is a no-no. Fix that too. * lib/tempname.c: Don't include stdalign.h. (HAS_CLOCK_ENTROPY): Remove. (mix_random_values): New function. (random_bits): Use it. Args are now new value address and old value, and this function now returns a success indicator. Omit old USE_GETRANDOM argument: always try getrandom now, as there is no good reason not to now that GRND_NONBLOCK is used. Caller changed. Use CLOCK_REALTIME for for ersatz entropy, as CLOCK_MONOTONIC doesn't work on some platforms. Also, mix in ersatz entropy from tv_sec and from clock (). (try_tempname_len): Do not mix in ASLR-based entropy, as the result is published to the world and ASLR should be private. Do not try to use a static var as that has issues if multithreaded. Instead, simply generate new random bits. Worry about bias only with high-quality random bits. * modules/tempname (Depends-on): Do not depend on stdalign. --- ChangeLog | 25 +++++++++++++ lib/tempname.c | 97 +++++++++++++++++++++++++----------------------- modules/tempname | 1 - 3 files changed, 76 insertions(+), 47 deletions(-) diff --git a/ChangeLog b/ChangeLog index adb445ddcf..ed99c845f7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,30 @@ 2022-08-22 Paul Eggert <eggert@cs.ucla.edu> + tempname: fix multithreading, ASLR leak etc. + Fix problems with tempname and multithreading, entropy loss, + and missing clock data (this last on non-GNU platforms). + See analysis by Bruno Haible in: + https://bugs.gnu.org/57129#149 + While looking into this, I noticed that tempname can leak + info derived from ASLR into publicly-visible file names, + which is a no-no. Fix that too. + * lib/tempname.c: Don't include stdalign.h. + (HAS_CLOCK_ENTROPY): Remove. + (mix_random_values): New function. + (random_bits): Use it. Args are now new value address and + old value, and this function now returns a success indicator. + Omit old USE_GETRANDOM argument: always try getrandom now, as + there is no good reason not to now that GRND_NONBLOCK is used. + Caller changed. Use CLOCK_REALTIME for for ersatz entropy, + as CLOCK_MONOTONIC doesn't work on some platforms. + Also, mix in ersatz entropy from tv_sec and from clock (). + (try_tempname_len): Do not mix in ASLR-based entropy, as + the result is published to the world and ASLR should be private. + Do not try to use a static var as that has issues if multithreaded. + Instead, simply generate new random bits. + Worry about bias only with high-quality random bits. + * modules/tempname (Depends-on): Do not depend on stdalign. + tempname: merge 64-bit time_t fix from glibc This merges glibc commit 52a5fe70a2c77935afe807fb6e904e512ddd894e "Use 64 bit time_t stat internally". diff --git a/lib/tempname.c b/lib/tempname.c index 4f615b90b9..bc1f7c14dd 100644 --- a/lib/tempname.c +++ b/lib/tempname.c @@ -48,7 +48,6 @@ #include <string.h> #include <fcntl.h> -#include <stdalign.h> #include <stdint.h> #include <sys/random.h> #include <sys/stat.h> @@ -77,26 +76,55 @@ typedef uint_fast64_t random_value; #define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */ #define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62) -#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME) -# define HAS_CLOCK_ENTROPY true -#else -# define HAS_CLOCK_ENTROPY false -#endif +/* Return the result of mixing the entropy from R and S. + Assume that R and S are not particularly random, + and that the result should look randomish to an untrained eye. */ static random_value -random_bits (random_value var, bool use_getrandom) +mix_random_values (random_value r, random_value s) +{ + /* As this code is used only when high-quality randomness is neither + available nor necessary, there is no need for fancier polynomials + such as those in the Linux kernel's 'random' driver. */ + return (2862933555777941757 * r + 3037000493) ^ s; +} + +/* Set *R to a random value. + Return true if *R is set to high-quality value taken from getrandom. + Otherwise return false, falling back to a low-quality *R that might + depend on S. + + This function returns false only when getrandom fails. + On GNU systems this should happen only early in the boot process, + when the fallback should be good enough for programs using tempname + because any attacker likely has root privileges already. */ + +static bool +random_bits (random_value *r, random_value s) { - random_value r; /* Without GRND_NONBLOCK it can be blocked for minutes on some systems. */ - if (use_getrandom && __getrandom (&r, sizeof r, GRND_NONBLOCK) == sizeof r) - return r; -#if HAS_CLOCK_ENTROPY - /* Add entropy if getrandom did not work. */ + if (__getrandom (r, sizeof *r, GRND_NONBLOCK) == sizeof *r) + return true; + + /* If getrandom did not work, use ersatz entropy based on low-order + clock bits. On GNU systems getrandom should fail only + early in booting, when ersatz should be good enough. + Do not use ASLR-based entropy, as that would leak ASLR info into + the resulting file name which is typically public. + + Of course we are in a state of sin here. */ + + random_value v = 0; + +#if _LIBC || (defined CLOCK_REALTIME && HAVE_CLOCK_GETTIME) struct __timespec64 tv; - __clock_gettime64 (CLOCK_MONOTONIC, &tv); - var ^= tv.tv_nsec; + __clock_gettime64 (CLOCK_REALTIME, &tv); + v = mix_random_values (v, tv.tv_sec); + v = mix_random_values (v, tv.tv_nsec); #endif - return 2862933555777941757 * var + 3037000493; + + *r = mix_random_values (v, clock ()); + return false; } #if _LIBC @@ -267,32 +295,15 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, unsigned int attempts = ATTEMPTS_MIN; #endif - /* A random variable. The initial value is used only the for fallback path - on 'random_bits' on 'getrandom' failure. Its initial value tries to use - some entropy from the ASLR and ignore possible bits from the stack - alignment. */ - random_value v = ((uintptr_t) &v) / alignof (max_align_t); - -#if !HAS_CLOCK_ENTROPY - /* Arrange gen_tempname to return less predictable file names on - systems lacking clock entropy <https://bugs.gnu.org/57129>. */ - static random_value prev_v; - v ^= prev_v; -#endif + /* A random variable. */ + random_value v = 0; /* How many random base-62 digits can currently be extracted from V. */ int vdigits = 0; - /* Whether to consume entropy when acquiring random bits. On the - first try it's worth the entropy cost with __GT_NOCREATE, which - is inherently insecure and can use the entropy to make it a bit - more secure. On the (rare) second and later attempts it might - help against DoS attacks. */ - bool use_getrandom = tryfunc == try_nocreate; - - /* Least unfair value for V. If V is less than this, V can generate - BASE_62_DIGITS digits fairly. Otherwise it might be biased. */ - random_value const unfair_min + /* Least biased value for V. If V is less than this, V can generate + BASE_62_DIGITS unbiased digits. Otherwise the digits are biased. */ + random_value const biased_min = RANDOM_VALUE_MAX - RANDOM_VALUE_MAX % BASE_62_POWER; len = strlen (tmpl); @@ -312,12 +323,9 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, { if (vdigits == 0) { - do - { - v = random_bits (v, use_getrandom); - use_getrandom = true; - } - while (unfair_min <= v); + /* Worry about bias only if the bits are high quality. */ + while (random_bits (&v, v) && biased_min <= v) + continue; vdigits = BASE_62_DIGITS; } @@ -331,9 +339,6 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, if (fd >= 0) { __set_errno (save_errno); -#if !HAS_CLOCK_ENTROPY - prev_v = v; -#endif return fd; } else if (errno != EEXIST) diff --git a/modules/tempname b/modules/tempname index 4779735d9d..f1fb78e8ff 100644 --- a/modules/tempname +++ b/modules/tempname @@ -16,7 +16,6 @@ getrandom libc-config lstat mkdir -stdalign stdbool stdint sys_stat -- 2.37.2 [-- Attachment #4: 0003-tempname-don-t-lose-entropy-in-seed.patch --] [-- Type: text/x-patch, Size: 2663 bytes --] From d27c820595175ed563b4d4ee5d0f421011891572 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Mon, 22 Aug 2022 15:43:18 -0500 Subject: [PATCH 3/3] tempname: don't lose entropy in seed * lib/tempname.c (random_bits): Don't lose entropy in S in the rare case where where the template has more than 10 Xs. From a suggestion by Bruno Haible in: https://bugs.gnu.org/57129#149 --- ChangeLog | 7 +++++++ lib/tempname.c | 11 +++++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index ed99c845f7..4d1c92cd81 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2022-08-22 Paul Eggert <eggert@cs.ucla.edu> + tempname: don't lose entropy in seed + * lib/tempname.c (random_bits): Don't lose entropy in S + in the rare case where where the template has more than 10 Xs. + From a suggestion by Bruno Haible in: + https://bugs.gnu.org/57129#149 + tempname: fix multithreading, ASLR leak etc. Fix problems with tempname and multithreading, entropy loss, and missing clock data (this last on non-GNU platforms). @@ -23,6 +29,7 @@ Do not try to use a static var as that has issues if multithreaded. Instead, simply generate new random bits. Worry about bias only with high-quality random bits. + * modules/tempname (Depends-on): Do not depend on stdalign. tempname: merge 64-bit time_t fix from glibc diff --git a/lib/tempname.c b/lib/tempname.c index bc1f7c14dd..0e2f29f5de 100644 --- a/lib/tempname.c +++ b/lib/tempname.c @@ -114,7 +114,7 @@ random_bits (random_value *r, random_value s) Of course we are in a state of sin here. */ - random_value v = 0; + random_value v = s; #if _LIBC || (defined CLOCK_REALTIME && HAVE_CLOCK_GETTIME) struct __timespec64 tv; @@ -298,7 +298,9 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, /* A random variable. */ random_value v = 0; - /* How many random base-62 digits can currently be extracted from V. */ + /* A value derived from the random variable, and how many random + base-62 digits can currently be extracted from VDIGBUF. */ + random_value vdigbuf; int vdigits = 0; /* Least biased value for V. If V is less than this, V can generate @@ -327,11 +329,12 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, while (random_bits (&v, v) && biased_min <= v) continue; + vdigbuf = v; vdigits = BASE_62_DIGITS; } - XXXXXX[i] = letters[v % 62]; - v /= 62; + XXXXXX[i] = letters[vdigbuf % 62]; + vdigbuf /= 62; vdigits--; } -- 2.37.2 ^ permalink raw reply related [flat|nested] 57+ messages in thread
[parent not found: <2dc7ede0-eca7-baf5-f89a-f5d292b80808@cs.ucla.edu>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <2dc7ede0-eca7-baf5-f89a-f5d292b80808@cs.ucla.edu> @ 2022-08-23 0:13 ` Bruno Haible [not found] ` <3893771.2iPT33SAM4@nimes> 1 sibling, 0 replies; 57+ messages in thread From: Bruno Haible @ 2022-08-23 0:13 UTC (permalink / raw) To: Paul Eggert; +Cc: jporterbugs, Eli Zaretskii, bug-gnulib, larsi, 57129 Paul Eggert wrote: > Thanks for the detailed diagnosis, Bruno. To try to fix the problems I > installed the attached patches into Gnulib. These look all good. Here's again my evaluation of the three scenarios: (Q1) What is the expected quality inside a single gen_tempname call? It depends on quality of random_bits(). Usually on the quality of getrandom(), which is good on most systems. And for the other systems, the "ersatz" in random_bits() gives reasonable quality. (Q2) What is the expected quality of multiple gen_tempname calls in a single process? (Q3) What is the expected quality when considering different processes that call gen_tempname? Both have the same answer: The file name essentially depends on the first call to random_bits(). Two different calls to random_bits() can be correlated only if - getrandom() is not well supported, and - CLOCK_REALTIME is not defined, and - we're in the same process, less than 0.01 sec apart. IMO, that's good enough. > If I understand things > correctly, these patches should fix the 0.1% failure rate you observed > on 64-bit mingw. Yes. Running the "case 2" part 1000 times again, among the 2000 generated file names, there are no duplicates — neither on 32-bit mingw, nor on 64-bit mingw. > They also fix a minor security leak I discovered: in > rare cases, ASLR entropy was used to generate publicly visible file > names, which is a no-no as that might help an attacker infer the > randomized layout of a victim process. Excellent observation :-) > These fixes follow some but not all the suggestions you made. The basic > problem I saw was that tempname.c was using too much belt-and-suspenders > code, so much so that the combination of belts and suspenders > misbehaved. I simplified it a bit and this removed the need for some of > the suggestions. Thanks. The major quality boost comes from invoking getrandom() always. Bruno ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <3893771.2iPT33SAM4@nimes>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <3893771.2iPT33SAM4@nimes> @ 2022-08-23 11:18 ` Eli Zaretskii [not found] ` <831qt79pjj.fsf@gnu.org> 1 sibling, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-23 11:18 UTC (permalink / raw) To: Bruno Haible; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 > From: Bruno Haible <bruno@clisp.org> > Cc: Eli Zaretskii <eliz@gnu.org>, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com > Date: Tue, 23 Aug 2022 02:13:02 +0200 > > > If I understand things > > correctly, these patches should fix the 0.1% failure rate you observed > > on 64-bit mingw. > > Yes. Running the "case 2" part 1000 times again, among the 2000 generated > file names, there are no duplicates — neither on 32-bit mingw, nor on 64-bit > mingw. Was that with or without using clock_gettime? If it was with clock_gettime, what happens without it (i.e. when 'clock' is used instead)? Thanks. ^ permalink raw reply [flat|nested] 57+ messages in thread
[parent not found: <831qt79pjj.fsf@gnu.org>]
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell [not found] ` <831qt79pjj.fsf@gnu.org> @ 2022-08-23 14:49 ` Bruno Haible 2022-08-23 16:07 ` Eli Zaretskii 0 siblings, 1 reply; 57+ messages in thread From: Bruno Haible @ 2022-08-23 14:49 UTC (permalink / raw) To: Eli Zaretskii; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 Eli Zaretskii asked: > > Yes. Running the "case 2" part 1000 times again, among the 2000 generated > > file names, there are no duplicates — neither on 32-bit mingw, nor on 64-bit > > mingw. > > Was that with or without using clock_gettime? If it was with > clock_gettime, what happens without it (i.e. when 'clock' is used > instead)? That was with clock_gettime. Without clock_gettime, i.e. without the dependency to libwinpthread, the result is the same: no duplicates — neither on 32-bit mingw, nor on 64-bit mingw. Bruno ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-23 14:49 ` Bruno Haible @ 2022-08-23 16:07 ` Eli Zaretskii 0 siblings, 0 replies; 57+ messages in thread From: Eli Zaretskii @ 2022-08-23 16:07 UTC (permalink / raw) To: Bruno Haible; +Cc: jporterbugs, larsi, eggert, bug-gnulib, 57129 > From: Bruno Haible <bruno@clisp.org> > Cc: eggert@cs.ucla.edu, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com > Date: Tue, 23 Aug 2022 16:49:36 +0200 > > Eli Zaretskii asked: > > > Yes. Running the "case 2" part 1000 times again, among the 2000 generated > > > file names, there are no duplicates — neither on 32-bit mingw, nor on 64-bit > > > mingw. > > > > Was that with or without using clock_gettime? If it was with > > clock_gettime, what happens without it (i.e. when 'clock' is used > > instead)? > > That was with clock_gettime. Without clock_gettime, i.e. without the > dependency to libwinpthread, the result is the same: no duplicates — neither > on 32-bit mingw, nor on 64-bit mingw. Great, thanks. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-15 18:58 ` Eli Zaretskii 2022-08-15 20:55 ` Paul Eggert [not found] ` <af0af29b-2362-77db-081e-046158937808@cs.ucla.edu> @ 2022-08-20 18:03 ` Jim Porter 2022-08-20 18:14 ` Eli Zaretskii 2022-08-24 21:41 ` Jim Porter 2 siblings, 2 replies; 57+ messages in thread From: Jim Porter @ 2022-08-20 18:03 UTC (permalink / raw) To: Eli Zaretskii, Paul Eggert; +Cc: larsi, 57129 [-- Attachment #1: Type: text/plain, Size: 1294 bytes --] On 8/15/2022 11:58 AM, Eli Zaretskii wrote: >> Cc: larsi@gnus.org, 57129@debbugs.gnu.org >> From: Jim Porter <jporterbugs@gmail.com> >> Date: Mon, 15 Aug 2022 11:30:07 -0700 >> >> The temp files are created by Eshell in lisp/eshell/esh-var.el in the >> function 'eshell-parse-variable-ref', specifically in the part starting >> with: >> >> (eq (char-after) ?\<) > > Ah, okay. It's a (mis)feature of Gnulib's gen_tempname function > (which is the guts of make-temp-file) in its implementation for > MS-Windows (and maybe other platforms?): it always begins from the > same "random" characters in the file name, and only generates other > random characters if there's already a file by that name. So if you > are careful and delete the temporary file each time after usage, and > never need more than one temporary file at the same time, you will get > the same name every call. In addition to the changes to temporary file name generation, I think it would be useful for Eshell to kill the temporary buffer too. If you use this feature in Eshell a lot, the temporary buffers could pile up, consuming memory for no real benefit. (A user who wanted the buffer to stick around would probably redirect to a non-temporary file, or even just to a buffer.) Attached is a patch to do this. [-- Attachment #2: 0001-Kill-the-buffer-associated-with-a-temp-file-when-usi.patch --] [-- Type: text/plain, Size: 1403 bytes --] From 62e4316bad6645ff12ef7587735b9e1c5e74fe21 Mon Sep 17 00:00:00 2001 From: Jim Porter <jporterbugs@gmail.com> Date: Sat, 20 Aug 2022 10:48:32 -0700 Subject: [PATCH] Kill the buffer associated with a temp file when using '$<command>' in Eshell * lisp/eshell/esh-var.el (eshell-parse-variable-ref): Kill the temp file's buffer when we're done. --- lisp/eshell/esh-var.el | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el index 2f6614b5d7..a9df172e88 100644 --- a/lisp/eshell/esh-var.el +++ b/lisp/eshell/esh-var.el @@ -490,8 +490,11 @@ eshell-parse-variable-ref ;; by `eshell-do-eval', which requires very ;; particular forms in order to work ;; properly. See bug#54190. - (list (function (lambda () - (delete-file ,temp)))))) + (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) ?\() -- 2.25.1 ^ permalink raw reply related [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-20 18:03 ` Jim Porter @ 2022-08-20 18:14 ` Eli Zaretskii 2022-08-20 18:49 ` Jim Porter 2022-08-24 21:41 ` Jim Porter 1 sibling, 1 reply; 57+ messages in thread From: Eli Zaretskii @ 2022-08-20 18:14 UTC (permalink / raw) To: Jim Porter; +Cc: larsi, eggert, 57129 > Cc: larsi@gnus.org, 57129@debbugs.gnu.org > From: Jim Porter <jporterbugs@gmail.com> > Date: Sat, 20 Aug 2022 11:03:27 -0700 > > In addition to the changes to temporary file name generation, I think it > would be useful for Eshell to kill the temporary buffer too. If you use > this feature in Eshell a lot, the temporary buffers could pile up, > consuming memory for no real benefit. (A user who wanted the buffer to > stick around would probably redirect to a non-temporary file, or even > just to a buffer.) Attached is a patch to do this. That makes sense, of course, but why not use with-temp-buffer in the first place? ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-20 18:14 ` Eli Zaretskii @ 2022-08-20 18:49 ` Jim Porter 0 siblings, 0 replies; 57+ messages in thread From: Jim Porter @ 2022-08-20 18:49 UTC (permalink / raw) To: Eli Zaretskii; +Cc: larsi, eggert, 57129 On 8/20/2022 11:14 AM, Eli Zaretskii wrote: >> Cc: larsi@gnus.org, 57129@debbugs.gnu.org >> From: Jim Porter <jporterbugs@gmail.com> >> Date: Sat, 20 Aug 2022 11:03:27 -0700 >> >> In addition to the changes to temporary file name generation, I think it >> would be useful for Eshell to kill the temporary buffer too. If you use >> this feature in Eshell a lot, the temporary buffers could pile up, >> consuming memory for no real benefit. (A user who wanted the buffer to >> stick around would probably redirect to a non-temporary file, or even >> just to a buffer.) Attached is a patch to do this. > > That makes sense, of course, but why not use with-temp-buffer in the > first place? I'm not sure that would work in this case. 'with-temp-buffer' (or probably 'with-temp-file', since we want a real file) would clean everything up once BODY has executed, but we want to perform cleanup in a hook, which might execute quite a while later (e.g. if passing this temp file to an external process). If there's a way to use that though, I'm all for it. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-20 18:03 ` Jim Porter 2022-08-20 18:14 ` Eli Zaretskii @ 2022-08-24 21:41 ` Jim Porter 2022-08-26 5:10 ` Jim Porter 1 sibling, 1 reply; 57+ messages in thread From: Jim Porter @ 2022-08-24 21:41 UTC (permalink / raw) To: Eli Zaretskii, Paul Eggert; +Cc: larsi, 57129 On 8/20/2022 11:03 AM, Jim Porter wrote: > In addition to the changes to temporary file name generation, I think it > would be useful for Eshell to kill the temporary buffer too. [snip] > Attached is a patch to do this. Assuming there are no remaining objections, I'll merge this in a day or two. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-24 21:41 ` Jim Porter @ 2022-08-26 5:10 ` Jim Porter 2023-03-16 5:35 ` Jim Porter 0 siblings, 1 reply; 57+ messages in thread From: Jim Porter @ 2022-08-26 5:10 UTC (permalink / raw) To: Eli Zaretskii, Paul Eggert; +Cc: larsi, 57129 On 8/24/2022 2:41 PM, Jim Porter wrote: > On 8/20/2022 11:03 AM, Jim Porter wrote: >> In addition to the changes to temporary file name generation, I think >> it would be useful for Eshell to kill the temporary buffer too. > [snip] >> Attached is a patch to do this. > > Assuming there are no remaining objections, I'll merge this in a day or > two. Merged as a457aa62577284333c7d25d48a49704788b25a04. ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-26 5:10 ` Jim Porter @ 2023-03-16 5:35 ` Jim Porter 0 siblings, 0 replies; 57+ messages in thread From: Jim Porter @ 2023-03-16 5:35 UTC (permalink / raw) To: Eli Zaretskii, Paul Eggert; +Cc: larsi, 57129-done On 8/25/2022 10:10 PM, Jim Porter wrote: > On 8/24/2022 2:41 PM, Jim Porter wrote: >> On 8/20/2022 11:03 AM, Jim Porter wrote: >>> In addition to the changes to temporary file name generation, I think >>> it would be useful for Eshell to kill the temporary buffer too. >> [snip] >>> Attached is a patch to do this. >> >> Assuming there are no remaining objections, I'll merge this in a day >> or two. > > Merged as a457aa62577284333c7d25d48a49704788b25a04. I know there was a quite-lengthy discussion about the tempname function, but I think those patches were merged, along with my patch for the original bug topic. Therefore, I'm going to close this. Of course, if there's still anything to do with the tempname stuff, let's do it (though it might help to give it a separate bug for tracking purposes). ^ permalink raw reply [flat|nested] 57+ messages in thread
* bug#57129: 29.0.50; Improve behavior of conditionals in Eshell 2022-08-13 5:14 ` Jim Porter 2022-08-13 7:01 ` Eli Zaretskii @ 2022-08-14 5:03 ` Sean Whitton 1 sibling, 0 replies; 57+ messages in thread From: Sean Whitton @ 2022-08-14 5:03 UTC (permalink / raw) To: Jim Porter; +Cc: 57129 Hello, On Fri 12 Aug 2022 at 10:14PM -07, Jim Porter wrote: > On 8/12/2022 8:22 AM, Lars Ingebrigtsen wrote: >> Jim Porter <jporterbugs@gmail.com> writes: >> >>> Here are some patches to fix this. >> I haven't tested the patches, but reading them, they make sense to me, >> so please go ahead and push. > > Thanks for taking a look. Merged as f3408af0a3251a744cb0b55b5e153565bfd57ea3. Thanks for working on this, pretty cool. -- Sean Whitton ^ permalink raw reply [flat|nested] 57+ messages in thread
end of thread, other threads:[~2023-03-16 5:35 UTC | newest]
Thread overview: 57+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-11 2:43 bug#57129: 29.0.50; Improve behavior of conditionals in Eshell Jim Porter
2022-08-11 2:46 ` Jim Porter
2022-08-12 15:22 ` Lars Ingebrigtsen
2022-08-13 5:14 ` Jim Porter
2022-08-13 7:01 ` Eli Zaretskii
2022-08-13 18:56 ` Jim Porter
2022-08-14 5:07 ` Eli Zaretskii
2022-08-14 5:37 ` Jim Porter
2022-08-14 7:30 ` Eli Zaretskii
2022-08-14 21:40 ` Jim Porter
2022-08-15 12:13 ` Eli Zaretskii
2022-08-15 16:14 ` Jim Porter
2022-08-15 16:49 ` Eli Zaretskii
2022-08-15 18:08 ` Jim Porter
2022-08-15 18:21 ` Eli Zaretskii
2022-08-15 18:30 ` Jim Porter
2022-08-15 18:58 ` Eli Zaretskii
2022-08-15 20:55 ` Paul Eggert
[not found] ` <af0af29b-2362-77db-081e-046158937808@cs.ucla.edu>
2022-08-15 21:22 ` Bruno Haible
2022-08-16 11:04 ` Eli Zaretskii
[not found] ` <835yish2l1.fsf@gnu.org>
2022-08-16 13:35 ` Bruno Haible
[not found] ` <1871347.6tgchFWduM@nimes>
2022-08-16 13:51 ` Eli Zaretskii
[not found] ` <838rnofgad.fsf@gnu.org>
2022-08-16 14:40 ` Bruno Haible
2022-08-16 16:25 ` Eli Zaretskii
[not found] ` <83wnb8dukz.fsf@gnu.org>
2022-08-16 16:54 ` Paul Eggert
[not found] ` <206e38df-2db4-a46a-e0ff-952bc8ab939c@cs.ucla.edu>
2022-08-16 17:04 ` Eli Zaretskii
[not found] ` <83sflwdsr2.fsf@gnu.org>
2022-08-16 17:30 ` Paul Eggert
2022-08-16 17:41 ` Eli Zaretskii
[not found] ` <ceeeaa86-6199-93b1-ff65-bbd3e531e235@cs.ucla.edu>
2022-08-16 17:56 ` Eli Zaretskii
2022-08-16 17:25 ` Paul Eggert
2022-08-16 17:29 ` Bruno Haible
[not found] ` <f329244a-cba7-65cd-2e5d-2630eba3e9e9@cs.ucla.edu>
2022-08-16 17:47 ` Eli Zaretskii
2022-08-16 19:11 ` Paul Eggert
[not found] ` <d95734ab-6bbc-7403-c1f8-fbf742badda4@cs.ucla.edu>
2022-08-16 20:12 ` Bruno Haible
2022-08-17 11:08 ` Eli Zaretskii
[not found] ` <83h72bdt4z.fsf@gnu.org>
2022-08-18 6:05 ` Paul Eggert
[not found] ` <57b8f10f-8e9b-5951-e5ad-8cba2a8cb569@cs.ucla.edu>
2022-08-18 6:44 ` Eli Zaretskii
[not found] ` <2594092.Isy0gbHreE@nimes>
2022-08-16 17:52 ` Eli Zaretskii
2022-08-16 20:06 ` Bruno Haible
[not found] ` <2606289.q0ZmV6gNhb@nimes>
2022-08-17 11:29 ` Eli Zaretskii
2022-08-16 19:59 ` Bruno Haible
[not found] ` <2135151.C4sosBPzcN@nimes>
2022-08-16 20:53 ` Paul Eggert
2022-08-21 16:20 ` Bruno Haible
2022-08-21 16:32 ` Eli Zaretskii
2022-08-21 17:17 ` Bruno Haible
2022-08-22 20:47 ` Paul Eggert
[not found] ` <2dc7ede0-eca7-baf5-f89a-f5d292b80808@cs.ucla.edu>
2022-08-23 0:13 ` Bruno Haible
[not found] ` <3893771.2iPT33SAM4@nimes>
2022-08-23 11:18 ` Eli Zaretskii
[not found] ` <831qt79pjj.fsf@gnu.org>
2022-08-23 14:49 ` Bruno Haible
2022-08-23 16:07 ` Eli Zaretskii
2022-08-20 18:03 ` Jim Porter
2022-08-20 18:14 ` Eli Zaretskii
2022-08-20 18:49 ` Jim Porter
2022-08-24 21:41 ` Jim Porter
2022-08-26 5:10 ` Jim Porter
2023-03-16 5:35 ` Jim Porter
2022-08-14 5:03 ` Sean Whitton
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).