shell: Reset handler when entering a subshell

Commit Message

Herbert Xu Feb. 28, 2019, 6:27 a.m. UTC

On Fri, Jan 11, 2019 at 11:25:48PM +0000, Harald van Dijk wrote:
> On 10/01/2019 14:21, Martijn Dekker wrote:
> > Op 10-01-19 om 11:37 schreef Martijn Dekker:
> > > In a dot script sourced with 'command .' (which is useful to avoid
> > > exiting if the script doesn't exist), triggering a syntax error in an
> > > 'eval' in a subshell causes dash to hang at the end of the main script.
> > 
> > In fact, 'eval' doesn't appear related. I can also trigger the bug by
> > triggering an error in another special builtin:
> > 
> > command . /dev/stdin <<EOF
> > ( set -o foobar ) && echo WOOPS
> > EOF
> > echo end
> 
> I do not see a hang myself, but I definitely see wrong behaviour: the output
> I get is
> 
>   $ command . /dev/stdin <<EOF
>   > ( set -o foobar ) && echo WOOPS
>   > EOF
>   src/dash: 1: set: Illegal option -o foobar
>   $ echo end
>   end
>   $ <Ctrl-D>
>   WOOPS
>   $
> 
> This was introduced by
> 
>   commit 46d5a7fcea81b489819f753451c1ad2fe435f148
>   Author: Herbert Xu <[e-mail]>
>   Date:   Tue Mar 27 00:39:35 2018 +0800
> 
>     eval: Restore input files in evalcommand
> 
>     When evalcommand invokes a command that modifies parsefile and
>     then bails out without popping the file, we need to ensure the
>     input file is restored so that the shell can continue to execute.
> 
> What I think it going on here, although I do not understand it completely
> yet, is:
> 
> evalcommand invokes a command that modifies parsefile: that's the dot
> command. The evaluation of the dotted file involves creating a subshell, and
> because of the invalid option, that subshell exits with EXERROR. Because the
> subshell is invoked from within the command builtin, that EXERROR is eaten,
> and the input file is restored. The subshell then happily continues reading
> commands at the same time as the parent shell.

Thanks for the analysis.  I think the real bug here is the fact
that subshells can "escape" their jail by a longjmp that is caught
by evalcommand.

This is something that both NetBSD/FreeBSD have already fixed.
Here is a similar patch to fix it in dash:

---8<---
As it is a subshell can execute code that is only meant for the
parent shell when it executes a longjmp that is caught by something
like evalcommand.  This patch fixes it by resetting the handler
when entering a subshell.

Reported-by: Martijn Dekker <[e-mail]>
Signed-off-by: Herbert Xu <[e-mail]>

Comments

Harald van Dijk March 2, 2019, 1:28 p.m. UTC | #1

On 28/02/2019 06:27, Herbert Xu wrote:
> Thanks for the analysis.  I think the real bug here is the fact
> that subshells can "escape" their jail by a longjmp that is caught
> by evalcommand.

That is *a* real bug here, not *the* real bug. This leaves the buggy 
behaviour of the "command" built-in intact in the test case I included 
in the message you replied to.

For fixing the one bug, it is a sensible approach, but keep in mind that 
when a fork is omitted because of EV_EXIT, so too will the reset of 
handler. You may be able to get away with that as long as you do not 
propagate EV_EXIT in any cases where a cleanup action might cause problems.

Cheers,
Harald van Dijk

Patch

diff --git a/src/error.h b/src/error.h
index 94e30a2..c5db134 100644
--- a/src/error.h
+++ b/src/error.h
@@ -34,6 +34,9 @@ 
  *	@(#)error.h	8.2 (Berkeley) 5/4/95
  */
 
+#ifndef _SRC_ERROR_H
+#define _SRC_ERROR_H
+
 #include <setjmp.h>
 #include <signal.h>
 
@@ -127,3 +130,5 @@  void exerror(int, const char *, ...) __attribute__((__noreturn__));
 const char *errmsg(int, int);
 
 void sh_warnx(const char *, ...);
+
+#endif	/* _SRC_ERROR_H */
diff --git a/src/jobs.c b/src/jobs.c
index 26a6248..80ae742 100644
--- a/src/jobs.c
+++ b/src/jobs.c
@@ -952,9 +952,10 @@  forkshell(struct job *jp, union node *n, int mode)
 
 	TRACE(("forkshell(%%%d, %p, %d) called\n", jobno(jp), n, mode));
 	pid = fork();
-	if (pid == 0)
+	if (pid == 0) {
+		handler = &main_handler;
 		forkchild(jp, n, mode);
-	else
+	} else
 		forkparent(jp, n, mode, pid);
 
 	return pid;
diff --git a/src/main.c b/src/main.c
index 6b3a090..5346ef6 100644
--- a/src/main.c
+++ b/src/main.c
@@ -71,6 +71,7 @@  int *dash_errno;
 short profile_buf[16384];
 extern int etext();
 #endif
+struct jmploc main_handler;
 
 STATIC void read_profile(const char *);
 STATIC char *find_dot_file(char *);
@@ -90,7 +91,6 @@  main(int argc, char **argv)
 {
 	char *shinit;
 	volatile int state;
-	struct jmploc jmploc;
 	struct stackmark smark;
 	int login;
 
@@ -102,7 +102,7 @@  main(int argc, char **argv)
 	monitor(4, etext, profile_buf, sizeof profile_buf, 50);
 #endif
 	state = 0;
-	if (unlikely(setjmp(jmploc.loc))) {
+	if (unlikely(setjmp(main_handler.loc))) {
 		int e;
 		int s;
 
@@ -137,7 +137,7 @@  main(int argc, char **argv)
 		else
 			goto state4;
 	}
-	handler = &jmploc;
+	handler = &main_handler;
 #ifdef DEBUG
 	opentrace();
 	trputs("Shell args:  ");  trargs(argv);
diff --git a/src/main.h b/src/main.h
index 19e4983..d54e10d 100644
--- a/src/main.h
+++ b/src/main.h
@@ -35,6 +35,7 @@ 
  */
 
 #include <errno.h>
+#include "error.h"
 
 /* pid of main shell */
 extern int rootpid;
@@ -49,6 +50,8 @@  extern int *dash_errno;
 #define errno (*dash_errno)
 #endif
 
+extern struct jmploc main_handler;
+
 void readcmdfile(char *);
 int dotcmd(int, char **);
 int exitcmd(int, char **);