dash repeats first line in history entry forever with fc -s

dash repeats first line in history entry forever with fc -s

[наб]

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

Hi!

Fun test for ya (--with-libedit):
  $ printf '%s\n' id 'fc 1' a ls . w q 'fc -s 1' | src/dash -i
  $ uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  $ 3
  6
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  002         autogen.sh      compile       config.status  install-sh   missing        q.cpp           q-ksh-stderr   q-zsh-stdout         stamp-h1
  003         autom4te.cache  config.h      configure      ll           q              q-dash2-stderr  q-ksh-stdout   reject_filtered_cmd  test.sh
  004         back            config.h.in   configure.ac   Makefile     q-bash-stderr  q-dash2-stdout  q-mksh-stderr  sh-stdin.sh          traptest
  005         ChangeLog       config.h.in~  COPYING        Makefile.am  q-bash-stdout  q-dash-stderr   q-mksh-stdout  sh-stdin.sh-f        tst.sh
  aclocal.m4  ChangeLog.O     config.log    depcomp        Makefile.in  q.c            q-dash-stdout   q-zsh-stderr   src                  x1.sh
  $ id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  fc -s 1
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  id
  ...
  ^C
  $
  src/dash: 1: Cannot set tty process group (No such process)

Interactively, if you ^C you can then
  $ fc -l
    426 id
    427 id
    428 id
    429 id
    430 id
    431 id
    432 id
    433 id
    434 id
    435 id
    436 id
    437 id
    438 id
    439 id
    440 id
    441 id
(or, as the case may be, something in the 30000 range)
of the first command in the history entry.

I've never used the POSIX history mechanism so idk what the correct
thing it should do there is, but it's probably not this.

libedit 3.1-20191231-2+b1, dash trunk (f96ec8765cf37eb0c222a563de2f767ebfbf56db).

Best,
наб

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: dash repeats first line in history entry forever with fc -s

[Harald van Dijk]

On 08/01/2023 16:10, наб wrote:
> Hi!
> 
> Fun test for ya (--with-libedit):
>    $ printf '%s\n' id 'fc 1' a ls . w q 'fc -s 1' | src/dash -i

Indeed. This was reported also by Martijn Dekker, 
<https://marc.info/?l=dash&m=154635285309187>, that thread lists some 
more related problems in the implementation of the fc command.

Cheers,
Harald van Dijk

Re: dash repeats first line in history entry forever with fc -s

[Christoph Anton Mitterer]

On Sun, 2023-01-08 at 16:31 +0000, Harald van Dijk wrote:
> Indeed. This was reported also by Martijn Dekker, 
> <https://marc.info/?l=dash&m=154635285309187>, that thread lists some
> more related problems in the implementation of the fc command.

A bit off-topic, but was it ever considered to move dash development
into some gitforge (e.g. git lab) or at least give it a proper bug
tracker so that such things could be... well... properly tracked?

Cheers,
Chris.

[PATCH 1/6] fc -s: refuse multiple events instead of ignoring

[наб]

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

The POSIX SYNOPSIS (and our manual which steals it verbatim) says:
  fc -s [old=new] [first]
and, indeed, we only use the first non-= argument
instead of enforcing the usage, which is confusing.

bash:
	2025     ls
	2026     id
	$ fc -s ls=who 2025 2026
	who
	nabijaczleweli pts/2 2023-02-07 17:36 (192.168.1.109)
	nabijaczleweli pts/3 2023-02-07 17:38 (192.168.1.109)
	nabijaczleweli pts/4 2023-02-07 16:58 (192.168.1.109)
	nabijaczleweli pts/5 2023-02-07 17:45 (192.168.1.109)
ksh93:
	240     id
	241     ls
	$ fc -s ls=who 241 240
	ksh: hist: -e - requires single argument
yash:
	2       ls
	3       id
	$ fc -s ls=who 2 3
	fc: too many operands are specified
zsh:
	    2  id
	    3  ls
	tarta% fc -s ls=who 3 2
	fc: bad option: -s
dash (before):
	    1 ls
	    2 id
	$ fc -s ls=who 1 2
	who
	nabijaczleweli pts/2 2023-02-07 17:36 (192.168.1.109)
	nabijaczleweli pts/3 2023-02-07 17:38 (192.168.1.109)
	nabijaczleweli pts/4 2023-02-07 16:58 (192.168.1.109)
	nabijaczleweli pts/5 2023-02-07 17:45 (192.168.1.109)
dash (after):
	    1 ls
	    2 id
	$ fc -s ls=who 1 2
	src/dash: 3: fc: -s takes one history argument

Adapted-from: NetBSD src bin/sh/histedit.c rev 1.38 by aymeric@
---
Here's a quick few that fix my report and a couple of the ones you list
at the end of that thread.

 src/histedit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/histedit.c b/src/histedit.c
index f5c90ab..fc87283 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -296,6 +296,13 @@ histcmd(int argc, char **argv)
 		*repl++ = '\0';
 		argc--, argv++;
 	}
+
+	/*
+	 * If -s is specified, accept only one operand
+	 */
+	if (sflg && argc >= 2)
+		sh_error("too many args");
+
 	/*
 	 * determine [first] and [last]
 	 */
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/6] fc -s: don't loop forever when executing the latest entry

[наб]

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

Quoting CVS comment:
  Ensure [...] that we break out of the loop after executing it.
  With the previous code, because the re-executed command was added to
  the history, it would think that it still had to execute it,
  leading to an infinite loop.

  The last thing which differs from ksh is that we get the "fc -s"
  command into the history, but this is actually rather a feature
  in my humble opinion.

POSIX disagrees:
  [W]hen the -l option is not specified, the resulting lines shall be
  entered at the end of the history list and then re-executed by sh.
  The fc command that caused the editing shall not be entered into the
  history list.

But this is a separate issue, since even without -s dash puts fc into
the history (i.e. id; ls; fc 1 2; fc -l; yields 1=id, 2=ls, 3=fc -l).

Test cases:
  $ id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  $ fc -s 1
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  $ fc -l
      1 id
      2 fc -s 1
      3 id
-- >8 --
  $ ls
  autogen.sh  ChangeLog  ...
  $ id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  $ fc -s -1
  id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
-- >8 --
  $ ls
  autogen.sh  ChangeLog  ...
  $ id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  $ fc -s -2
  ls
  autogen.sh  ChangeLog  ...
-- >8 --
  $ ls
  autogen.sh  ChangeLog  ...
  $ id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  $ fc -s -2
  ls
  autogen.sh  ChangeLog  ...
  $ id 1
  uid=1(daemon) gid=1(daemon) groups=1(daemon)
  $ id 2
  uid=2(bin) gid=2(bin) groups=2(bin)
  $ fc -s 3
  fc -s -2
  id 1
  uid=1(daemon) gid=1(daemon) groups=1(daemon)
  $ fc -l
      1 ls
      2 id
      3 fc -s -2
      4 ls
      5 id 1
      6 id 2
      7 fc -s 3
      8 id 1
-- >8 --
  $ id 1
  uid=1(daemon) gid=1(daemon) groups=1(daemon)
  $ fc -s 2
  fc -s 2
  fc -s 2
  fc -s 2
  fc -s 2
  src/dash: 1: fc: called recursively too many times
and I'm happy to call this "behaving exactly as I expected when I was
typing it in", so removing the XXX.

Adapted-from: NetBSD src bin/sh/histedit.c rev 1.38 by aymeric@
---
 src/histedit.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/histedit.c b/src/histedit.c
index fc87283..28956ec 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -382,12 +382,10 @@ histcmd(int argc, char **argv)
 
 				evalstring(s, 0);
 				if (displayhist && hist) {
-					/*
-					 *  XXX what about recursive and
-					 *  relative histnums.
-					 */
 					history(hist, &he, H_ENTER, s);
 				}
+
+				break;
 			} else
 				fputs(s, efp);
 		}
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/6] fc: only parse old=new if -s, per POSIX

[наб]

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

Before (erroneously replaced):
	$ a=b set
	...
	$ fc a=b
	8
	,
	b=b set
After (used as search string):
	$ fc a=b
	8
	,
	a=b set

Reported-by: Harald van Dijk <harald@gigawatt.nl>
Reported-in: https://marc.info/?l=dash&m=154707728009743&w=2
---
 src/histedit.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/src/histedit.c b/src/histedit.c
index 28956ec..24631ca 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -288,21 +288,18 @@ histcmd(int argc, char **argv)
 	}
 
 	/*
-	 * If executing, parse [old=new] now
+	 * If -s is specified, accept [old=new] first only
 	 */
-	if (lflg == 0 && argc > 0 &&
-	     ((repl = strchr(argv[0], '=')) != NULL)) {
-		pat = argv[0];
-		*repl++ = '\0';
-		argc--, argv++;
+	if (sflg) {
+		if (argc > 0 && ((repl = strchr(argv[0], '=')) != NULL)) {
+			pat = argv[0];
+			*repl++ = '\0';
+			argc--, argv++;
+		}
+		if (argc >= 2)
+			sh_error("too many args");
 	}
 
-	/*
-	 * If -s is specified, accept only one operand
-	 */
-	if (sflg && argc >= 2)
-		sh_error("too many args");
-
 	/*
 	 * determine [first] and [last]
 	 */
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/6] fc: don't require an argument

[наб]

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

This is already handled correctly (per POSIX) below:
  When the synopsis form with -s is used:
    If first is omitted, the previous command shall be used.
  For the synopsis forms without -s:
    If first and last are both omitted, the previous 16 commands
    shall be listed or the previous single command shall be edited
    (based on the -l option).

Test log:
  $ ls
  autogen.sh  ChangeLog  ...
  $ id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  $ who
  nabijaczleweli pts/2 2023-02-07 18:36 (192.168.1.109)
  $ fc
  4
  ,
  who
  q
  nabijaczleweli pts/2 2023-02-07 18:36 (192.168.1.109)
  $ fc -l
      1 ls
      2 id
      3 who
      4 fc
  $ fc -s
  fc -l
      1 ls
      2 id
      3 who
      4 fc
      5 fc -l

Reported-by: Harald van Dijk <harald@gigawatt.nl>
Reported-in: https://marc.info/?l=dash&m=154707728009743&w=2
---
 src/histedit.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/histedit.c b/src/histedit.c
index 24631ca..f10e5ab 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -212,9 +212,6 @@ histcmd(int argc, char **argv)
 	if (hist == NULL)
 		sh_error("history not active");
 
-	if (argc == 1)
-		sh_error("missing history argument");
-
 #ifdef __GLIBC__
 	optind = 0;
 #else
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/6] fc: fix "fc -3" extension on glibc

[наб]

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

Before:
	$ echo a
	a
	$ echo b
	b
	$ fc -2 -1
	src/dash: 3: fc: unknown option: -2
	$ fc -- -2 -1
	16
	,p
	echo b
	fc -2 -1
After:
	$ echo a
	a
	$ echo b
	b
	$ fc -2 -1
	6
	,p
	echo a
	echo b

Reported-by: Harald van Dijk <harald@gigawatt.nl>
Reported-in: https://marc.info/?l=dash&m=154707728009743&w=2
---
 src/histedit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/histedit.c b/src/histedit.c
index f10e5ab..502ad40 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -217,7 +217,7 @@ histcmd(int argc, char **argv)
 #else
 	optreset = 1; optind = 1; /* initialize getopt */
 #endif
-	while (not_fcnumber(argv[optind]) &&
+	while (not_fcnumber(argv[optind ?: 1]) &&
 	      (ch = getopt(argc, argv, ":e:lnrs")) != -1)
 		switch ((char)ch) {
 		case 'e':
@@ -243,6 +243,7 @@ histcmd(int argc, char **argv)
 			sh_error("unknown option: -%c", optopt);
 			/* NOTREACHED */
 		}
+	optind = optind ?: 1;
 	argc -= optind, argv += optind;
 
 	/*
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 6/6] fc: don't include the current fc in out-of-range last

[наб]

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

POSIX states:
  When a range of commands is used, it shall not be an error to specify
  first or last values that are not in the history list; fc shall
  substitute the value representing the oldest or newest command in the
  list, as appropriate. For example, if there are only ten commands in
  the history list, numbered 1 to 10:
    fc -l
    fc 1 99
  shall list and edit, respectively, all ten commands.

Which would seem to imply that the current fc shouldn't be included
(well, in the POSIX model, no non--l fc enters the history,
 so that reinforces that).

zsh, bash, mksh, yash all agree with this; oddly, ksh includes it.

Before:
  $ 1
  src/dash: 1: 1: not found
  $ 2
  src/dash: 2: 2: not found
  $ 3
  src/dash: 3: 3: not found
  $ 4
  src/dash: 4: 4: not found
  $ 5
  src/dash: 5: 5: not found
  $ 6
  src/dash: 6: 6: not found
  $ fc 1 999
  21
  ,p
  1
  2
  3
  4
  5
  6
  fc 1 999
After:
  $ fc 1 9999
  12
  ,p
  1
  2
  3
  4
  5
  6

Reported-by: Harald van Dijk <harald@gigawatt.nl>
Reported-in: https://marc.info/?l=dash&m=154707728009743&w=2
---
 src/histedit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/histedit.c b/src/histedit.c
index 502ad40..7692776 100644
--- a/src/histedit.c
+++ b/src/histedit.c
@@ -476,6 +476,8 @@ str_to_event(const char *str, int last)
 				 */
 				retval = history(hist, &he,
 						last ? H_FIRST : H_LAST);
+				if (retval != -1 && last)
+					retval = history(hist, &he, H_NEXT);
 			}
 		}
 		if (retval == -1)
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] fc -s: refuse multiple events instead of ignoring

[Herbert Xu]

On Tue, Feb 07, 2023 at 08:33:25PM +0100, наб wrote:
> The POSIX SYNOPSIS (and our manual which steals it verbatim) says:
>   fc -s [old=new] [first]
> and, indeed, we only use the first non-= argument
> instead of enforcing the usage, which is confusing.
> 
> bash:
> 	2025     ls
> 	2026     id
> 	$ fc -s ls=who 2025 2026
> 	who
> 	nabijaczleweli pts/2 2023-02-07 17:36 (192.168.1.109)
> 	nabijaczleweli pts/3 2023-02-07 17:38 (192.168.1.109)
> 	nabijaczleweli pts/4 2023-02-07 16:58 (192.168.1.109)
> 	nabijaczleweli pts/5 2023-02-07 17:45 (192.168.1.109)
> ksh93:
> 	240     id
> 	241     ls
> 	$ fc -s ls=who 241 240
> 	ksh: hist: -e - requires single argument
> yash:
> 	2       ls
> 	3       id
> 	$ fc -s ls=who 2 3
> 	fc: too many operands are specified
> zsh:
> 	    2  id
> 	    3  ls
> 	tarta% fc -s ls=who 3 2
> 	fc: bad option: -s
> dash (before):
> 	    1 ls
> 	    2 id
> 	$ fc -s ls=who 1 2
> 	who
> 	nabijaczleweli pts/2 2023-02-07 17:36 (192.168.1.109)
> 	nabijaczleweli pts/3 2023-02-07 17:38 (192.168.1.109)
> 	nabijaczleweli pts/4 2023-02-07 16:58 (192.168.1.109)
> 	nabijaczleweli pts/5 2023-02-07 17:45 (192.168.1.109)
> dash (after):
> 	    1 ls
> 	    2 id
> 	$ fc -s ls=who 1 2
> 	src/dash: 3: fc: -s takes one history argument
> 
> Adapted-from: NetBSD src bin/sh/histedit.c rev 1.38 by aymeric@
> ---
> Here's a quick few that fix my report and a couple of the ones you list
> at the end of that thread.
> 
>  src/histedit.c | 7 +++++++
>  1 file changed, 7 insertions(+)

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt