[PATCH] input: preadfd: read standard input byte-wise

[PATCH] input: preadfd: read standard input byte-wise

[наб]

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

POSIX Issue 7, XCU, sh, STDIN says:
  When the shell is using standard input and it invokes a command that
  also uses standard input, the shell shall ensure that the standard
  input file pointer points directly after the command it has read when
  the command begins execution. It shall not read ahead in such a manner
  that any characters intended to be read by the invoked command are
  consumed by the shell (whether interpreted by the shell or not) or
  that characters that are not read by the invoked command are not seen
  by the shell.

I.e.
  sh <<EOF
  id
  cat
  good!
  EOF
must execute id, then execute cat, then the cat must copy "good!"
to the standard output stream, and similarly
  sh <<"EOF"
  id
  read Q
  good!
  echo Q$Q
  EOF
must execute id, then read "good!" into Q, then echo "Qgood!".

Heretofor the output was as such:
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  ./dash: 3: good!: not found
and as such (with -x):
  + id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  + read Q
  + good!
  sh: 3: good!: not found
  + echo Q
  Q
and a strace confirms:
  read(0, "id\ncat\ngood!\n", 8192)       = 13
  read(0, "id\nread Q\ngood!\necho Q$Q\n", 8192) = 25

Reading the standard input byte-by-byte is the obvious solution to this
issue, Just Works, and is how all other shells do it (we could,
theoretically, read regular files block-wise, then seek within them after
parsing, but the complexity out-weighs the rarity of running
sh < program; we could also do whole-line reads on teletypes in
icanon mode, but, again, the gain here is miniscule for an interactive
session, and the teletype mode can change at any time, so...).
Naturally, we keep reading block-wise for non-standard-input.

With this patch, we observe the correct
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  good!
and
  + id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  + read Q
  + echo Qgood!
  Qgood!

Fixes: https://bugs.debian.org/862907
---
 src/input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/input.c b/src/input.c
index ec075f5..6b6113e 100644
--- a/src/input.c
+++ b/src/input.c
@@ -195,7 +195,7 @@ retry:
 
 	} else
 #endif
-		nr = read(parsefile->fd, buf, IBUFSIZ - 1);
+		nr = read(parsefile->fd, buf, parsefile->fd == 0 ? 1 : IBUFSIZ - 1);
 
 
 	if (nr < 0) {
-- 
2.30.2

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

Re: [PATCH] input: preadfd: read standard input byte-wise

[Harald van Dijk]

On 13/12/2022 22:17, наб wrote:
> Reading the standard input byte-by-byte is the obvious solution to this
> issue, Just Works, and is how all other shells do it (we could,
> theoretically, read regular files block-wise, then seek within them after
> parsing, but the complexity out-weighs the rarity of running
> sh < program; we could also do whole-line reads on teletypes in
> icanon mode, but, again, the gain here is miniscule for an interactive
> session, and the teletype mode can change at any time, so...).
> Naturally, we keep reading block-wise for non-standard-input.

There are a few things to consider here:

- Not all shells do it this way. bash does do the block-wise read,
   followed by a seek, when stdin is seekable. While I agree that it is
   not necessary and not worth it, you specifically say that other shells
   do not do this. That's simply not true.
- This will have no effect when running 'dash /dev/stdin'. I personally
   consider this acceptable, this doesn't work in other shells either.
- This patch breaks internal assumptions in dash that the buffer will
   contain a full line, affecting error recovery. See below.

> With this patch, we observe the correct
>    uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
>    good!
> and
>    + id
>    uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
>    + read Q
>    + echo Qgood!
>    Qgood!
> 
> Fixes: https://bugs.debian.org/862907
> ---
>   src/input.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/input.c b/src/input.c
> index ec075f5..6b6113e 100644
> --- a/src/input.c
> +++ b/src/input.c
> @@ -195,7 +195,7 @@ retry:
>   
>   	} else
>   #endif
> -		nr = read(parsefile->fd, buf, IBUFSIZ - 1);
> +		nr = read(parsefile->fd, buf, parsefile->fd == 0 ? 1 : IBUFSIZ - 1);
>   
>   
>   	if (nr < 0) {

With dash 0.5.12:

   $ | echo bug
   src/dash: 1: Syntax error: "|" unexpected
   $

With dash 0.5.12 + your patch:

   $ | echo bug
   src/dash: 1: Syntax error: "|" unexpected
   $ bug
   $

I had implemented the same change in my fork, see 
<https://github.com/hvdijk/gwsh/commit/d279523041c1c380d64b6dec7760feba20bbf6b5> 
for the additional changes I needed to get everything working.

Cheers,
Harald van Dijk

Re: [PATCH] input: preadfd: read standard input byte-wise

[наб]

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

Hi!

On Tue, Dec 13, 2022 at 10:37:12PM +0000, Harald van Dijk wrote:
> On 13/12/2022 22:17, наб wrote:
> > Reading the standard input byte-by-byte is the obvious solution to this
> > issue, Just Works, and is how all other shells do it (we could,
> > theoretically, read regular files block-wise, then seek within them after
> > parsing, but the complexity out-weighs the rarity of running
> > sh < program; we could also do whole-line reads on teletypes in
> > icanon mode, but, again, the gain here is miniscule for an interactive
> > session, and the teletype mode can change at any time, so...).
> > Naturally, we keep reading block-wise for non-standard-input.
> There are a few things to consider here:
> - Not all shells do it this way. bash does do the block-wise read,
>   followed by a seek, when stdin is seekable. While I agree that it is
>   not necessary and not worth it, you specifically say that other shells
>   do not do this. That's simply not true.

Yeah, I wrote the parenthetical way after the rest and didn't think to
reassess them in consort (it appears that ksh93 seeks, zsh doesn't).

> - This patch breaks internal assumptions in dash that the buffer will
>   contain a full line, affecting error recovery. See below.
> > --- a/src/input.c
> > +++ b/src/input.c
> > @@ -195,7 +195,7 @@ retry:
> >   	} else
> >   #endif
> > -		nr = read(parsefile->fd, buf, IBUFSIZ - 1);
> > +		nr = read(parsefile->fd, buf, parsefile->fd == 0 ? 1 : IBUFSIZ - 1);
> >   	if (nr < 0) {
> 
> With dash 0.5.12:
> 
>   $ | echo bug
>   src/dash: 1: Syntax error: "|" unexpected
>   $
> 
> With dash 0.5.12 + your patch:
> 
>   $ | echo bug
>   src/dash: 1: Syntax error: "|" unexpected
>   $ bug
>   $
> 
> I had implemented the same change in my fork, see <https://github.com/hvdijk/gwsh/commit/d279523041c1c380d64b6dec7760feba20bbf6b5>
> for the additional changes I needed to get everything working.

It's interesting to see that the line error bug appears to blame back to
start-of-git(?), and the partial line consumption is triggerable ‒
with vastly more pathological input, admittedly ‒ on unpatched dash as
well. I've imported the other fixes piece-meal, and the updated
series (in follow-up), passes all cases in your commit message as well.

Best,
наб

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

[PATCH v2 1/3] parser: fixredir: invalid redirections are run-time, not syntax

[наб]

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

This fixes a long-standing bug where
  echo 'echo >&a' | sh
errors out with
  sh: 2: Syntax error: Bad fd number
despite the error being on line 1

This patch makes the error
  sh: 1: Bad fd number: a
as expected

Adapted-from: https://github.com/hvdijk/gwsh/commit/d279523041c1c380d64b6dec7760feba20bbf6b5
---
 src/parser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser.c b/src/parser.c
index a552c47..8a06b9e 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -615,7 +615,7 @@ void fixredir(union node *n, const char *text, int err)
 	else {
 
 		if (err)
-			synerror("Bad fd number");
+			sh_error("Bad fd number: %s", text);
 		else
 			n->ndup.vname = makename();
 	}
-- 
2.30.2


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

[PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[наб]

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

Interactively, sh_error() doesn't terminate, so
  echo "|$(printf %10000s)echo bug" | sh -i
would read the first 8KiB, see that it's invalid, then jump back to the
parser, which would then read and execute the rest of the line as-if
it were the next line.

The fix for this is to explicitly consume the rest of the invalid line,
so that the next line observed is /actually/ the next line.

This is difficult to trigger accidentally right now, since we consume
the entire icanon line buffer at once (provided it's <8k, which it
~always is interactively), so we always observe one line at a time,
but the next patch would make even "| echo bug" blow up.

Imported-from: https://github.com/hvdijk/gwsh/commit/d279523041c1c380d64b6dec7760feba20bbf6b5
---
 src/parser.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/parser.c b/src/parser.c
index 8a06b9e..35fdbc3 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -761,6 +761,13 @@ static void nlnoprompt(void)
 	needprompt = doprompt;
 }
 
+static void
+skipline(void)
+{
+	int c;
+	while ((c = pgetc()) != '\n' && c != PEOF);
+}
+
 
 /*
  * Read the next input token.
@@ -798,7 +805,7 @@ xxreadtoken(void)
 		case ' ': case '\t':
 			continue;
 		case '#':
-			while ((c = pgetc()) != '\n' && c != PEOF);
+			skipline();
 			pungetc();
 			continue;
 		case '\n':
@@ -1526,6 +1533,12 @@ STATIC void
 synerror(const char *msg)
 {
 	errlinno = plinno;
+
+	/* If we see a syntax error in a command, read the rest of the
+	 * line now before reporting the error. This ensures we get error
+	 * reporting that does not depend on buffering details. */
+	skipline();
+
 	sh_error("Syntax error: %s", msg);
 	/* NOTREACHED */
 }
-- 
2.30.2


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

[PATCH v2 3/3] input: preadfd: read standard input byte-wise

[наб]

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

POSIX Issue 7, XCU, sh, STDIN says:
  When the shell is using standard input and it invokes a command that
  also uses standard input, the shell shall ensure that the standard
  input file pointer points directly after the command it has read when
  the command begins execution. It shall not read ahead in such a manner
  that any characters intended to be read by the invoked command are
  consumed by the shell (whether interpreted by the shell or not) or
  that characters that are not read by the invoked command are not seen
  by the shell.

I.e.
  sh <<EOF
  id
  cat
  good!
  EOF
must execute id, then execute cat, then the cat must copy "good!"
to the standard output stream, and similarly
  sh <<"EOF"
  id
  read Q
  good!
  echo Q$Q
  EOF
must execute id, then read "good!" into Q, then echo "Qgood!".

Heretofor the output was as such:
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  ./dash: 3: good!: not found
and as such (with -x):
  + id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  + read Q
  + good!
  sh: 3: good!: not found
  + echo Q
  Q
and a strace confirms:
  read(0, "id\ncat\ngood!\n", 8192)       = 13
  read(0, "id\nread Q\ngood!\necho Q$Q\n", 8192) = 25

Reading the standard input byte-by-byte is the obvious solution to this
issue, Just Works, and is how all of shells do it on non-seekable input
(we could, theoretically, read regular files block-wise,
then seek within them after parsing, but the complexity out-weighs
the rarity of running sh < program; we could also do whole-line reads
on teletypes in icanon mode, but, again, the gain here is miniscule
for an interactive session, and the mode can change at any time, so...).
Naturally, we keep reading block-wise from not-standard-input.

With this patch, we observe the correct
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  good!
and
  + id
  uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
  + read Q
  + echo Qgood!
  Qgood!

Fixes: https://bugs.debian.org/862907
---
 src/input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/input.c b/src/input.c
index ec075f5..6b6113e 100644
--- a/src/input.c
+++ b/src/input.c
@@ -195,7 +195,7 @@ retry:
 
 	} else
 #endif
-		nr = read(parsefile->fd, buf, IBUFSIZ - 1);
+		nr = read(parsefile->fd, buf, parsefile->fd == 0 ? 1 : IBUFSIZ - 1);
 
 
 	if (nr < 0) {
-- 
2.30.2

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

Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[Herbert Xu]

наб <nabijaczleweli@nabijaczleweli.xyz> wrote:
>
> synerror(const char *msg)
> {
>        errlinno = plinno;
> +
> +       /* If we see a syntax error in a command, read the rest of the
> +        * line now before reporting the error. This ensures we get error
> +        * reporting that does not depend on buffering details. */
> +       skipline();

This is broken.  What if we already read a newline just before
the syntax error (e.g., synexpect(TDO))?

This needs to be dealt with in the input layer.

Cheers,
-- 
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

[PATCH] input: Eat rest of line upon reset

[Herbert Xu]

On Tue, Jan 03, 2023 at 09:53:23AM +0800, Herbert Xu wrote:
>
> This is broken.  What if we already read a newline just before
> the syntax error (e.g., synexpect(TDO))?
> 
> This needs to be dealt with in the input layer.

This works for me:

---8<---
Interactively, sh_error() doesn't terminate, so
  echo "|$(printf %10000s)echo bug" | sh -i
would read the first 8KiB, see that it's invalid, then jump back to the
parser, which would then read and execute the rest of the line as-if
it were the next line.

The fix for this is to explicitly consume the rest of the invalid line,
so that the next line observed is /actually/ the next line.

This is difficult to trigger accidentally right now, since we consume
the entire icanon line buffer at once (provided it's <8k, which it
~always is interactively), so we always observe one line at a time,
but the next patch would make even "| echo bug" blow up.

Reported-by: наб <nabijaczleweli@nabijaczleweli.xyz> 
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/input.c b/src/input.c
index dfe2fd6..e72eae7 100644
--- a/src/input.c
+++ b/src/input.c
@@ -77,6 +77,7 @@ INCLUDE <stdio.h>
 INCLUDE <unistd.h>
 INCLUDE "input.h"
 INCLUDE "error.h"
+INCLUDE "syntax.h"
 
 INIT {
 	basepf.nextc = basepf.buf = basebuf;
@@ -85,9 +86,11 @@ INIT {
 
 RESET {
 	/* clear input buffer */
-	basepf.lleft = basepf.nleft = 0;
-	basepf.unget = 0;
 	popallfiles();
+	basepf.unget = 0;
+	while (basepf.lastc[0] != '\n' &&
+	       basepf.lastc[0] != PEOF)
+		pgetc();
 }
 
 FORKRESET {
-- 
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

[v3 PATCH] input: Read standard input byte-wise

[Herbert Xu]

наб <nabijaczleweli@nabijaczleweli.xyz> wrote:
> 
> POSIX Issue 7, XCU, sh, STDIN says:

Your patch breaks history support since that relies on there
being a whole line in the input buffer.

This works for me:

---8<---
POSIX Issue 7, XCU, sh, STDIN says:
 When the shell is using standard input and it invokes a command that
 also uses standard input, the shell shall ensure that the standard
 input file pointer points directly after the command it has read when
 the command begins execution. It shall not read ahead in such a manner
 that any characters intended to be read by the invoked command are
 consumed by the shell (whether interpreted by the shell or not) or
 that characters that are not read by the invoked command are not seen
 by the shell.

I.e.
 sh <<EOF
 id
 cat
 good!
 EOF
must execute id, then execute cat, then the cat must copy "good!"
to the standard output stream, and similarly
 sh <<"EOF"
 id
 read Q
 good!
 echo Q$Q
 EOF
must execute id, then read "good!" into Q, then echo "Qgood!".

Heretofor the output was as such:
 uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
 ./dash: 3: good!: not found
and as such (with -x):
 + id
 uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
 + read Q
 + good!
 sh: 3: good!: not found
 + echo Q
 Q
and a strace confirms:
 read(0, "id\ncat\ngood!\n", 8192)       = 13
 read(0, "id\nread Q\ngood!\necho Q$Q\n", 8192) = 25

Reading the standard input byte-by-byte is the obvious solution to this
issue, Just Works, and is how all of shells do it on non-seekable input
(we could, theoretically, read regular files block-wise,
then seek within them after parsing, but the complexity out-weighs
the rarity of running sh < program; we could also do whole-line reads
on teletypes in icanon mode, but, again, the gain here is miniscule
for an interactive session, and the mode can change at any time, so...).
Naturally, we keep reading block-wise from not-standard-input.

With this patch, we observe the correct
 uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
 good!
and
 + id
 uid=1000(nabijaczleweli) gid=100(users) groups=100(users)
 + read Q
 + echo Qgood!
 Qgood!

Link: https://bugs.debian.org/862907
Reported-by: наб <nabijaczleweli@nabijaczleweli.xyz>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/input.c b/src/input.c
index ec075f5..43c9161 100644
--- a/src/input.c
+++ b/src/input.c
@@ -159,6 +159,17 @@ int pgetc(void)
 	return __pgetc();
 }
 
+static int stdin_clear_nonblock(void)
+{
+	int flags = fcntl(0, F_GETFL, 0);
+
+	if (flags >= 0) {
+		flags &=~ O_NONBLOCK;
+		flags = fcntl(0, F_SETFL, flags);
+	}
+
+	return flags;
+}
 
 static int
 preadfd(void)
@@ -195,22 +206,38 @@ retry:
 
 	} else
 #endif
+	if (parsefile->fd)
 		nr = read(parsefile->fd, buf, IBUFSIZ - 1);
+	else {
+		unsigned len = IBUFSIZ - 1;
+
+		nr = 0;
+
+		do {
+			int err;
 
+			err = read(0, buf, 1);
+			if (err <= 0) {
+				if (nr)
+					break;
+
+				nr = err;
+				if (errno != EWOULDBLOCK)
+					break;
+				if (stdin_clear_nonblock() < 0)
+					break;
+
+				out2str("sh: turning off NDELAY mode\n");
+				goto retry;
+			}
+
+			nr++;
+		} while (!IS_DEFINED_SMALL && *buf++ != '\n' && --len);
+	}
 
 	if (nr < 0) {
 		if (errno == EINTR)
 			goto retry;
-		if (parsefile->fd == 0 && errno == EWOULDBLOCK) {
-			int flags = fcntl(0, F_GETFL, 0);
-			if (flags >= 0 && flags & O_NONBLOCK) {
-				flags &=~ O_NONBLOCK;
-				if (fcntl(0, F_SETFL, flags) >= 0) {
-					out2str("sh: turning off NDELAY mode\n");
-					goto retry;
-				}
-			}
-		}
 	}
 	return nr;
 }
diff --git a/src/input.h b/src/input.h
index 8c39f33..8830b66 100644
--- a/src/input.h
+++ b/src/input.h
@@ -34,6 +34,12 @@
  *	@(#)input.h	8.2 (Berkeley) 5/4/95
  */
 
+#ifdef SMALL
+#define IS_DEFINED_SMALL 1
+#else
+#define IS_DEFINED_SMALL 0
+#endif
+
 /* PEOF (the end of file marker) is defined in syntax.h */
 
 enum {
-- 
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

Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[Harald van Dijk]

On 03/01/2023 01:53, Herbert Xu wrote:
> наб <nabijaczleweli@nabijaczleweli.xyz> wrote:
>>
>> synerror(const char *msg)
>> {
>>         errlinno = plinno;
>> +
>> +       /* If we see a syntax error in a command, read the rest of the
>> +        * line now before reporting the error. This ensures we get error
>> +        * reporting that does not depend on buffering details. */
>> +       skipline();
> 
> This is broken.  What if we already read a newline just before
> the syntax error (e.g., synexpect(TDO))?

In order for this to be a problem, we need something where the newline 
itself triggers a syntax error. For synexpect(TDO), is that possible? In 
all cases where synexpect(TDO) is called, a newline is permitted, and it 
is the token after the newline that the error will be reported on, no? 
In that situation, reading the rest of the line is correct.

I'm not going to rule out that there is a potential for problems, but if 
there is, it's not the situation you say.

> This needs to be dealt with in the input layer.

Handling it in the input layer after the error has already been reported 
means we get inconsistent error handling when running with dash -isv. 
The error may, depending on whether the rest of the line was in the 
buffer already, be reported either in the middle of the input line, or 
on the next line.

Just try changing sh -i to sh -iv in the reproducer and seeing what what 
happens with your patch.

Cheers,
Harald van Dijk

Re: [v3 PATCH] input: Read standard input byte-wise

[Harald van Dijk]

On 03/01/2023 06:15, Herbert Xu wrote:
> Your patch breaks history support since that relies on there
> being a whole line in the input buffer.

Does it? preadbuffer() knows to call history() with the H_APPEND flag to 
append a next chunk of input to what was already added to the buffer.

Cheers,
Harald van Dijk

Re: [v3 PATCH] input: Read standard input byte-wise

[Herbert Xu]

On Tue, Jan 03, 2023 at 11:54:55AM +0000, Harald van Dijk wrote:
>
> Does it? preadbuffer() knows to call history() with the H_APPEND flag to
> append a next chunk of input to what was already added to the buffer.

It only uses H_APPEND once a newline has been detected and
whichprompt switches over to PS2.

Cheers,
-- 
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

Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[Herbert Xu]

On Tue, Jan 03, 2023 at 11:47:05AM +0000, Harald van Dijk wrote:
>
> In order for this to be a problem, we need something where the newline
> itself triggers a syntax error. For synexpect(TDO), is that possible? In all
> cases where synexpect(TDO) is called, a newline is permitted, and it is the
> token after the newline that the error will be reported on, no? In that
> situation, reading the rest of the line is correct.

You're right, it isn't possible with TDO.

> I'm not going to rule out that there is a potential for problems, but if
> there is, it's not the situation you say.

However, the synexpect in parsefname would seem to qualify:

cat <<- <newline>

> Handling it in the input layer after the error has already been reported
> means we get inconsistent error handling when running with dash -isv. The
> error may, depending on whether the rest of the line was in the buffer
> already, be reported either in the middle of the input line, or on the next
> line.
> 
> Just try changing sh -i to sh -iv in the reproducer and seeing what what
> happens with your patch.

I don't think this is such a big deal.  You get the same effect
if you simply do input line buffering but only up to a certain
length.  I tried one million instead of ten thousand with the
reproducer and ksh93 prints out the error after only 65536
characters.

In any case, we could actually fix this by buffering stderr and
then making sh -v bypass the buffer and write directly to 2.

I tried it and it actually seems to work.  But all the extra flush
calls bloat up the code a bit so it needs a bit more work before
I'm happy to run with it.

Cheers,
-- 
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

Re: [v3 PATCH] input: Read standard input byte-wise

[Harald van Dijk]

On 04/01/2023 08:59, Herbert Xu wrote:
> On Tue, Jan 03, 2023 at 11:54:55AM +0000, Harald van Dijk wrote:
>>
>> Does it? preadbuffer() knows to call history() with the H_APPEND flag to
>> append a next chunk of input to what was already added to the buffer.
> 
> It only uses H_APPEND once a newline has been detected and
> whichprompt switches over to PS2.

Oh yeah, that's something I'd changed in my version already in 2019 for 
other reasons. I missed that dash didn't do the same. There are a few 
separate but related bugs in the history handling, and until they are 
fixed, agreed that simply reading one bye at a time will make these more 
prominent.

One is that because of what you say, the history recording for commands 
greater than BUFSIZ is already buggy, but unlikely to pose a problem in 
practice currently because normally, people don't write such long commands.

Another is that the stripping of blank lines from history breaks when 
blank lines are significant, as in

   $ cat <<EOF
   > hello
   >
   > world
   > EOF
   hello

   world
   $ fc 1
   26
   q
   hello
   world

Note how the blank line was lost.

If there was already "something" (as the code puts it), then when more 
data is appended, blanks must be preserved.

There may be more that I am not seeing right now.

Cheers,
Harald van Dijk

Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[Harald van Dijk]

On 04/01/2023 09:51, Herbert Xu wrote:
> On Tue, Jan 03, 2023 at 11:47:05AM +0000, Harald van Dijk wrote:
>>
>> In order for this to be a problem, we need something where the newline
>> itself triggers a syntax error. For synexpect(TDO), is that possible? In all
>> cases where synexpect(TDO) is called, a newline is permitted, and it is the
>> token after the newline that the error will be reported on, no? In that
>> situation, reading the rest of the line is correct.
> 
> You're right, it isn't possible with TDO.
> 
>> I'm not going to rule out that there is a potential for problems, but if
>> there is, it's not the situation you say.
> 
> However, the synexpect in parsefname would seem to qualify:
> 
> cat <<- <newline>

You're right, that does show the problem, thanks.

Since the only case where special care is needed is when the problematic 
token was newline, it can be handled as simply as

   if (lasttoken != TNL)
     skipline();

I will do some testing to see if I am overlooking something, and follow 
up if I am.

>> Handling it in the input layer after the error has already been reported
>> means we get inconsistent error handling when running with dash -isv. The
>> error may, depending on whether the rest of the line was in the buffer
>> already, be reported either in the middle of the input line, or on the next
>> line.
>>
>> Just try changing sh -i to sh -iv in the reproducer and seeing what what
>> happens with your patch.
> 
> I don't think this is such a big deal.  You get the same effect
> if you simply do input line buffering but only up to a certain
> length.  I tried one million instead of ten thousand with the
> reproducer and ksh93 prints out the error after only 65536
> characters.
> 
> In any case, we could actually fix this by buffering stderr and
> then making sh -v bypass the buffer and write directly to 2.
> 
> I tried it and it actually seems to work.  But all the extra flush
> calls bloat up the code a bit so it needs a bit more work before
> I'm happy to run with it.

Interesting approach. Error messages are known to always fit in the 
buffer, so you can probably be sure it won't be forced to be emitted 
early. It could work.

Cheers,
Harald van Dijk

Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[Herbert Xu]

On Wed, Jan 04, 2023 at 11:25:09AM +0000, Harald van Dijk wrote:
> 
> Since the only case where special care is needed is when the problematic
> token was newline, it can be handled as simply as
> 
>   if (lasttoken != TNL)
>     skipline();

You're assuming that the only way of exiting the parser is
through synerror.  That's not the case.  We could also exit
through sh_error, e.g., if ckmalloc runs out of memory.

Cheers,
-- 
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

Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[Harald van Dijk]

On 04/01/2023 14:10, Herbert Xu wrote:
> On Wed, Jan 04, 2023 at 11:25:09AM +0000, Harald van Dijk wrote:
>>
>> Since the only case where special care is needed is when the problematic
>> token was newline, it can be handled as simply as
>>
>>    if (lasttoken != TNL)
>>      skipline();
> 
> You're assuming that the only way of exiting the parser is
> through synerror.  That's not the case.  We could also exit
> through sh_error, e.g., if ckmalloc runs out of memory.

In theory, yes. In practice, because of the default Linux overcommit 
behaviour, that is not going to happen, that's going to cause the OOM 
killer to kick in and forcibly kill the whole process without any way of 
handling it. Either that, or forcibly kill some other process that we 
get no indication of.

Even if we change the overcommit settings, I'm struggling to think of 
any situation where parsing fails because we run out of memory, but 
freeing the memory we've already allocated for parsing that one line of 
input frees enough to allow us to meaningfully continue.

Cheers,
Harald van Dijk

Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[Herbert Xu]

On Wed, Jan 04, 2023 at 02:30:00PM +0000, Harald van Dijk wrote:
>
> In theory, yes. In practice, because of the default Linux overcommit
> behaviour, that is not going to happen, that's going to cause the OOM killer
> to kick in and forcibly kill the whole process without any way of handling
> it. Either that, or forcibly kill some other process that we get no
> indication of.
> 
> Even if we change the overcommit settings, I'm struggling to think of any
> situation where parsing fails because we run out of memory, but freeing the
> memory we've already allocated for parsing that one line of input frees
> enough to allow us to meaningfully continue.

ckmalloc is just an example.  It could really come from anywhere.
For example, expandstr calls expandarg, which could trigger an
exception for SIGINT.  Indeed, getting SIGINT directly while in
the parser would also do the trick.

Cheers,
-- 
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

Re: [PATCH v2 2/3] parser: synerror: explicitly consume the entire invalid line

[Harald van Dijk]

On 04/01/2023 14:41, Herbert Xu wrote:
> On Wed, Jan 04, 2023 at 02:30:00PM +0000, Harald van Dijk wrote:
>>
>> In theory, yes. In practice, because of the default Linux overcommit
>> behaviour, that is not going to happen, that's going to cause the OOM killer
>> to kick in and forcibly kill the whole process without any way of handling
>> it. Either that, or forcibly kill some other process that we get no
>> indication of.
>>
>> Even if we change the overcommit settings, I'm struggling to think of any
>> situation where parsing fails because we run out of memory, but freeing the
>> memory we've already allocated for parsing that one line of input frees
>> enough to allow us to meaningfully continue.
> 
> ckmalloc is just an example.  It could really come from anywhere.
> For example, expandstr calls expandarg, which could trigger an
> exception for SIGINT.  Indeed, getting SIGINT directly while in
> the parser would also do the trick.

The only place expandstr() is called in the parser is in getprompt(), 
which is only called when we're at the start of a line and reading the 
next line of input is probably wrong.

As for a SIGINT during actual parsing, that's true. I'm not sure reading 
the rest of the input line is always the right thing to do in that case 
either. Imagine the user typing

   echo 1 <^C> echo 2 <return>

Normally we want this 'echo 2' to be interpreted as a new command. It 
seems odd to me to say that we interpret the 'echo 2' as a new command, 
*except* if the Ctrl-C is processed early, and the 'echo 2' is entered 
early too, in which case we discard it. Especially if this special 
exception only applies if "early" is so early that it cannot reliably be 
triggered. I would want this to be deterministic.

As long as we're talking about things like that, regardless of how it's 
implemented, we could also get another SIGINT during the skipping of the 
rest of the input line.

Cheers,
Harald van Dijk

[PATCH] input: Check for int_pending while clearing input

[Herbert Xu]

On Wed, Jan 04, 2023 at 02:59:23PM +0000, Harald van Dijk wrote:
>
> As long as we're talking about things like that, regardless of how it's
> implemented, we could also get another SIGINT during the skipping of the
> rest of the input line.

Actually, that's a good point and we should stop clearing the line
if we detect SIGINT.

---8<---
If we receive SIGINT while clearing a partially read line from
stdin we should bail out instead of continuing.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/input.c b/src/input.c
index 8691617..1c83287 100644
--- a/src/input.c
+++ b/src/input.c
@@ -89,7 +89,8 @@ RESET {
 	popallfiles();
 	basepf.unget = 0;
 	while (basepf.lastc[0] != '\n' &&
-	       basepf.lastc[0] != PEOF)
+	       basepf.lastc[0] != PEOF &&
+	       !int_pending())
 		pgetc();
 }
 
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

[PATCH] input: Only skip blank lines on PS1

[Herbert Xu]

On Wed, Jan 04, 2023 at 11:18:49AM +0000, Harald van Dijk wrote:
> 
> One is that because of what you say, the history recording for commands
> greater than BUFSIZ is already buggy, but unlikely to pose a problem in
> practice currently because normally, people don't write such long commands.

It's not just that people don't write long lines, it's the fact
that most people use the shell with a terminal device in canonical
mode and the line length is capped in canonical mode to some value
less than 8K.

> Another is that the stripping of blank lines from history breaks when blank
> lines are significant, as in
> 
>   $ cat <<EOF
>   > hello
>   >
>   > world
>   > EOF
>   hello
> 
>   world
>   $ fc 1
>   26
>   q
>   hello
>   world
> 
> Note how the blank line was lost.
> 
> If there was already "something" (as the code puts it), then when more data
> is appended, blanks must be preserved.

Good point.  This patch should fix it.

---8<---
Blank line should not be skipped if they're found on PS2.

Reported-by: Harald van Dijk <harald@gigawatt.nl>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/input.c b/src/input.c
index 7b37ae2..5dcc8a2 100644
--- a/src/input.c
+++ b/src/input.c
@@ -254,6 +254,7 @@ retry:
 
 static int preadbuffer(void)
 {
+	int first = whichprompt == 1;
 	int something;
 	char savec;
 	int more;
@@ -279,7 +280,7 @@ again:
 	q = parsefile->nextc;
 
 	/* delete nul characters */
-	something = 0;
+	something = !first;
 	for (;;) {
 		int c;
 
@@ -327,7 +328,7 @@ check:
 	if (parsefile->fd == 0 && hist && something) {
 		HistEvent he;
 		INTOFF;
-		history(hist, &he, whichprompt == 1? H_ENTER : H_APPEND,
+		history(hist, &he, first ? H_ENTER : H_APPEND,
 			parsefile->nextc);
 		INTON;
 	}
-- 
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

Re: [PATCH] input: Only skip blank lines on PS1

[Harald van Dijk]

On 05/01/2023 07:26, Herbert Xu wrote:
> On Wed, Jan 04, 2023 at 11:18:49AM +0000, Harald van Dijk wrote:
>>
>> One is that because of what you say, the history recording for commands
>> greater than BUFSIZ is already buggy, but unlikely to pose a problem in
>> practice currently because normally, people don't write such long commands.
> 
> It's not just that people don't write long lines, it's the fact
> that most people use the shell with a terminal device in canonical
> mode and the line length is capped in canonical mode to some value
> less than 8K.

Remember that we're talking about builds of dash here with libedit 
support, as otherwise history is disabled anyway. In builds with libedit 
support, I would imagine the more common use is that libedit is used for 
entry, in which case the terminal is *not* in canonical mode.

Cheers,
Harald van Dijk

Re: [PATCH v2 1/3] parser: fixredir: invalid redirections are run-time, not syntax

[Herbert Xu]

наб <nabijaczleweli@nabijaczleweli.xyz> wrote:
> [-- text/plain, encoding quoted-printable, charset: us-ascii, 33 lines --]
> 
> This fixes a long-standing bug where
>  echo 'echo >&a' | sh
> errors out with
>  sh: 2: Syntax error: Bad fd number
> despite the error being on line 1
> 
> This patch makes the error
>  sh: 1: Bad fd number: a
> as expected
> 
> Adapted-from: https://github.com/hvdijk/gwsh/commit/d279523041c1c380d64b6dec7760feba20bbf6b5
> ---
> src/parser.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Patch 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