dash.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] parser: Catch errors in expandstr
       [not found] <SJhI_AW9YJmphaap5RzzVqYTGsA529iV7GOEBafT9ilDPGFpMRRJlNt3TrMBwh0uqKQltSvsH2P8x_2in6enHT5DzygjAtrRSvWl5xHofeY=@emersion.fr>
@ 2020-01-21  6:39 ` Herbert Xu
  2020-01-21 17:06   ` Simon Ser
  2020-02-26 21:12   ` Ron Yorston
  0 siblings, 2 replies; 7+ messages in thread
From: Herbert Xu @ 2020-01-21  6:39 UTC (permalink / raw)
  To: Simon Ser; +Cc: dash

On Fri, Dec 13, 2019 at 02:51:34PM +0000, Simon Ser wrote:
> Just noticed another dash bug: when setting invalid PS1 values dash
> enters an infinite loop.
> 
> For instance, setting PS1='$(' makes dash print many of these:
> 
>    dash: 1: Syntax error: end of file unexpected (expecting ")")
> 
> It would be nice to fallback to the default PS1 value on error.

This patch fixes it by using the literal value of PS1 should an
error occur during expansion.

Reported-by: Simon Ser <contact@emersion.fr>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/parser.c b/src/parser.c
index b318b08..201d5bd 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1571,28 +1571,43 @@ setprompt(int which)
 const char *
 expandstr(const char *ps)
 {
+	struct jmploc *volatile savehandler;
+	const char *volatile result;
+	volatile int saveprompt;
+	struct jmploc jmploc;
 	union node n;
-	int saveprompt;
+	int err;
 
 	/* XXX Fix (char *) cast. */
 	setinputstring((char *)ps);
 
 	saveprompt = doprompt;
 	doprompt = 0;
+	result = ps;
+	savehandler = handler;
+	if (unlikely(err = setjmp(jmploc.loc)))
+		goto out;
+	handler = &jmploc;
 
 	readtoken1(pgetc_eatbnl(), DQSYNTAX, FAKEEOFMARK, 0);
 
-	doprompt = saveprompt;
-
-	popfile();
-
 	n.narg.type = NARG;
 	n.narg.next = NULL;
 	n.narg.text = wordtext;
 	n.narg.backquote = backquotelist;
 
 	expandarg(&n, NULL, EXP_QUOTED);
-	return stackblock();
+	result = stackblock();
+
+out:
+	handler = savehandler;
+	if (err && exception != EXERROR)
+		longjmp(handler->loc, 1);
+
+	doprompt = saveprompt;
+	popfile();
+
+	return result;
 }
 
 /*
-- 
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] parser: Catch errors in expandstr
  2020-01-21  6:39 ` [PATCH] parser: Catch errors in expandstr Herbert Xu
@ 2020-01-21 17:06   ` Simon Ser
  2020-02-26 21:12   ` Ron Yorston
  1 sibling, 0 replies; 7+ messages in thread
From: Simon Ser @ 2020-01-21 17:06 UTC (permalink / raw)
  To: Herbert Xu; +Cc: dash

On Tuesday, January 21, 2020 7:39 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Fri, Dec 13, 2019 at 02:51:34PM +0000, Simon Ser wrote:
>
> > Just noticed another dash bug: when setting invalid PS1 values dash
> > enters an infinite loop.
> > For instance, setting PS1='$(' makes dash print many of these:
> > dash: 1: Syntax error: end of file unexpected (expecting ")")
> > It would be nice to fallback to the default PS1 value on error.
>
> This patch fixes it by using the literal value of PS1 should an
> error occur during expansion.
>
> Reported-by: Simon Ser <contact@emersion.fr>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks, works!

Tested-by: Simon Ser <contact@emersion.fr>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] parser: Catch errors in expandstr
  2020-01-21  6:39 ` [PATCH] parser: Catch errors in expandstr Herbert Xu
  2020-01-21 17:06   ` Simon Ser
@ 2020-02-26 21:12   ` Ron Yorston
  2020-02-28  0:40     ` Herbert Xu
  1 sibling, 1 reply; 7+ messages in thread
From: Ron Yorston @ 2020-02-26 21:12 UTC (permalink / raw)
  To: herbert, contact; +Cc: dash

Herbert Xu wrote:
>This patch fixes it by using the literal value of PS1 should an
>error occur during expansion.

There's another case that should be handled.  PS1='`xxx(`' causes the
shell to exit because the old-style backquote leaves an additional file
on the stack.

Signed-off-by: Ron Yorston <rmy@pobox.com>
---
 src/parser.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/parser.c b/src/parser.c
index 201d5bd..d7e717a 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1572,12 +1572,14 @@ const char *
 expandstr(const char *ps)
 {
 	struct jmploc *volatile savehandler;
+	struct parsefile *file_stop;
 	const char *volatile result;
 	volatile int saveprompt;
 	struct jmploc jmploc;
 	union node n;
 	int err;
 
+	file_stop = parsefile;
 	/* XXX Fix (char *) cast. */
 	setinputstring((char *)ps);
 
@@ -1605,7 +1607,7 @@ out:
 		longjmp(handler->loc, 1);
 
 	doprompt = saveprompt;
-	popfile();
+	unwindfiles(file_stop);
 
 	return result;
 }
-- 
2.24.1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] parser: Catch errors in expandstr
  2020-02-26 21:12   ` Ron Yorston
@ 2020-02-28  0:40     ` Herbert Xu
  2020-04-28  6:17       ` [v2 PATCH] " Herbert Xu
  0 siblings, 1 reply; 7+ messages in thread
From: Herbert Xu @ 2020-02-28  0:40 UTC (permalink / raw)
  To: Ron Yorston; +Cc: contact, dash

On Wed, Feb 26, 2020 at 09:12:04PM +0000, Ron Yorston wrote:
> Herbert Xu wrote:
> >This patch fixes it by using the literal value of PS1 should an
> >error occur during expansion.
> 
> There's another case that should be handled.  PS1='`xxx(`' causes the
> shell to exit because the old-style backquote leaves an additional file
> on the stack.
> 
> Signed-off-by: Ron Yorston <rmy@pobox.com>
> ---
>  src/parser.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

When you send a patch to an existing thread could you please change
the Subject line? Otherwise your patch will be silently dropped by
patchwork.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [v2 PATCH] parser: Catch errors in expandstr
  2020-02-28  0:40     ` Herbert Xu
@ 2020-04-28  6:17       ` Herbert Xu
  2020-05-17 12:19         ` Harald van Dijk
  0 siblings, 1 reply; 7+ messages in thread
From: Herbert Xu @ 2020-04-28  6:17 UTC (permalink / raw)
  To: Ron Yorston; +Cc: contact, dash

On Fri, Feb 28, 2020 at 11:40:23AM +1100, Herbert Xu wrote:
> On Wed, Feb 26, 2020 at 09:12:04PM +0000, Ron Yorston wrote:
> > Herbert Xu wrote:
> > >This patch fixes it by using the literal value of PS1 should an
> > >error occur during expansion.
> > 
> > There's another case that should be handled.  PS1='`xxx(`' causes the
> > shell to exit because the old-style backquote leaves an additional file
> > on the stack.
> > 
> > Signed-off-by: Ron Yorston <rmy@pobox.com>
> > ---
> >  src/parser.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> When you send a patch to an existing thread could you please change
> the Subject line? Otherwise your patch will be silently dropped by
> patchwork.

As I haven't received another patch, I'm going to fold yours into
my original patch.

---8<---
On Fri, Dec 13, 2019 at 02:51:34PM +0000, Simon Ser wrote:
> Just noticed another dash bug: when setting invalid PS1 values dash
> enters an infinite loop.
> 
> For instance, setting PS1='$(' makes dash print many of these:
> 
>    dash: 1: Syntax error: end of file unexpected (expecting ")")
> 
> It would be nice to fallback to the default PS1 value on error.

This patch fixes it by using the literal value of PS1 should an
error occur during expansion.

On Wed, Feb 26, 2020 at 09:12:04PM +0000, Ron Yorston wrote:
> 
> There's another case that should be handled.  PS1='`xxx(`' causes the
> shell to exit because the old-style backquote leaves an additional file
> on the stack.

Ron's change has been folded into this patch.

Reported-by: Simon Ser <contact@emersion.fr>
Reported-by: Ron Yorston <rmy@pobox.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/src/parser.c b/src/parser.c
index b318b08..5e36929 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1571,28 +1571,46 @@ setprompt(int which)
 const char *
 expandstr(const char *ps)
 {
+	struct parsefile *volatile file_stop;
+	struct jmploc *volatile savehandler;
+	const char *volatile result;
+	volatile int saveprompt;
+	struct jmploc jmploc;
 	union node n;
-	int saveprompt;
+	int err;
+
+	file_stop = parsefile;
 
 	/* XXX Fix (char *) cast. */
 	setinputstring((char *)ps);
 
 	saveprompt = doprompt;
 	doprompt = 0;
+	result = ps;
+	savehandler = handler;
+	if (unlikely(err = setjmp(jmploc.loc)))
+		goto out;
+	handler = &jmploc;
 
 	readtoken1(pgetc_eatbnl(), DQSYNTAX, FAKEEOFMARK, 0);
 
-	doprompt = saveprompt;
-
-	popfile();
-
 	n.narg.type = NARG;
 	n.narg.next = NULL;
 	n.narg.text = wordtext;
 	n.narg.backquote = backquotelist;
 
 	expandarg(&n, NULL, EXP_QUOTED);
-	return stackblock();
+	result = stackblock();
+
+out:
+	handler = savehandler;
+	if (err && exception != EXERROR)
+		longjmp(handler->loc, 1);
+
+	doprompt = saveprompt;
+	unwindfiles(file_stop);
+
+	return result;
 }
 
 /*
-- 
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [v2 PATCH] parser: Catch errors in expandstr
  2020-04-28  6:17       ` [v2 PATCH] " Herbert Xu
@ 2020-05-17 12:19         ` Harald van Dijk
  2020-05-17 13:36           ` [PATCH] parser: Save and restore heredoclist " Herbert Xu
  0 siblings, 1 reply; 7+ messages in thread
From: Harald van Dijk @ 2020-05-17 12:19 UTC (permalink / raw)
  To: Herbert Xu, Ron Yorston; +Cc: contact, dash

On 28/04/2020 07:17, Herbert Xu wrote:
> ---8<---
> On Fri, Dec 13, 2019 at 02:51:34PM +0000, Simon Ser wrote:
>> Just noticed another dash bug: when setting invalid PS1 values dash
>> enters an infinite loop.
>>
>> For instance, setting PS1='$(' makes dash print many of these:
>>
>>     dash: 1: Syntax error: end of file unexpected (expecting ")")
>>
>> It would be nice to fallback to the default PS1 value on error.
> 
> This patch fixes it by using the literal value of PS1 should an
> error occur during expansion.
> 
> On Wed, Feb 26, 2020 at 09:12:04PM +0000, Ron Yorston wrote:
>>
>> There's another case that should be handled.  PS1='`xxx(`' causes the
>> shell to exit because the old-style backquote leaves an additional file
>> on the stack.
> 
> Ron's change has been folded into this patch.

This still does not restore the state completely. It does not clean up 
any pending heredocs. I see:

   $ PS1='$(<<EOF "'
   src/dash: 1: Syntax error: Unterminated quoted string
   $(<<EOF ":
   >

That is, after entering the ':' command, the shell is still trying to 
read the heredoc from the prompt.

I have not looked in detail to see if anything else is not getting 
cleaned up that should be.

Cheers,
Harald van Dijk

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] parser: Save and restore heredoclist in expandstr
  2020-05-17 12:19         ` Harald van Dijk
@ 2020-05-17 13:36           ` Herbert Xu
  0 siblings, 0 replies; 7+ messages in thread
From: Herbert Xu @ 2020-05-17 13:36 UTC (permalink / raw)
  To: Harald van Dijk; +Cc: Ron Yorston, contact, dash

On Sun, May 17, 2020 at 01:19:28PM +0100, Harald van Dijk wrote:
>
> This still does not restore the state completely. It does not clean up any
> pending heredocs. I see:
> 
>   $ PS1='$(<<EOF "'
>   src/dash: 1: Syntax error: Unterminated quoted string
>   $(<<EOF ":
>   >
> 
> That is, after entering the ':' command, the shell is still trying to read
> the heredoc from the prompt.

This patch saves and restores the heredoclist in expandstr.

It also removes a bunch of unnecessary volatiles as those variables
are only referenced in case of a longjmp other than one started by
a signal like SIGINT.

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

diff --git a/src/parser.c b/src/parser.c
index 3131045..54c2861 100644
--- a/src/parser.c
+++ b/src/parser.c
@@ -1565,10 +1565,11 @@ setprompt(int which)
 const char *
 expandstr(const char *ps)
 {
-	struct parsefile *volatile file_stop;
+	struct parsefile *file_stop;
 	struct jmploc *volatile savehandler;
-	const char *volatile result;
-	volatile int saveprompt;
+	struct heredoc *saveheredoclist;
+	const char *result;
+	int saveprompt;
 	struct jmploc jmploc;
 	union node n;
 	int err;
@@ -1578,6 +1579,8 @@ expandstr(const char *ps)
 	/* XXX Fix (char *) cast. */
 	setinputstring((char *)ps);
 
+	saveheredoclist = heredoclist;
+	heredoclist = NULL;
 	saveprompt = doprompt;
 	doprompt = 0;
 	result = ps;
@@ -1603,6 +1606,7 @@ out:
 
 	doprompt = saveprompt;
 	unwindfiles(file_stop);
+	heredoclist = saveheredoclist;
 
 	return result;
 }
-- 
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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-05-17 13:36 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <SJhI_AW9YJmphaap5RzzVqYTGsA529iV7GOEBafT9ilDPGFpMRRJlNt3TrMBwh0uqKQltSvsH2P8x_2in6enHT5DzygjAtrRSvWl5xHofeY=@emersion.fr>
2020-01-21  6:39 ` [PATCH] parser: Catch errors in expandstr Herbert Xu
2020-01-21 17:06   ` Simon Ser
2020-02-26 21:12   ` Ron Yorston
2020-02-28  0:40     ` Herbert Xu
2020-04-28  6:17       ` [v2 PATCH] " Herbert Xu
2020-05-17 12:19         ` Harald van Dijk
2020-05-17 13:36           ` [PATCH] parser: Save and restore heredoclist " Herbert Xu

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 NNTP newsgroup(s).