Re: [v2 PATCH] eval: Reset handler when entering a subshell
On 03/03/2019 17:39, Jilles Tjoelker wrote: On Sun, Mar 03, 2019 at 04:43:09PM +, Harald van Dijk wrote: The effect of the command built-in is that "if the command_name is the same as the name of one of the special built-in utilities, the special properties in the enumerated list at the beginning of Special Built-In Utilities shall not occur." This is ambiguous as to whether it is just about the special properties associated with the . command, or whether it includes those associated with the set command called indirectly, but it must be one or the other. If it includes the set command, then the shell shall not exit, and the correct output is a followed by b. If it does not include the set command, then the shell shall exit, and the correct output is nothing. dash outputs b, which is wrong in either case. The way I interpret this is that any error while parsing or executing the . or eval command(s) is a "special built-in utility error" which can be "caught" using "command". Therefore, the correct output is an error message about the invalid option (to stderr) followed by b. This is what happens in FreeBSD sh (for quite a few years), ksh93 (20120801_2) and mksh (56c). That's how dash implements it too, but it plainly doesn't make sense based on POSIX's requirements. If the effect of "shall exit" does not occur, the result isn't "shall start exiting and then abort the exit", it's "shall not exit". In order to allow dash/FreeBSD sh's interpretation, POSIX would need to say something like "the special properties [...] shall be suppressed" rather than "[...] shall not occur". Cheers, Harald van Dijk
Re: [v2 PATCH] eval: Reset handler when entering a subshell
On Sun, Mar 03, 2019 at 04:43:09PM +, Harald van Dijk wrote: > On 03/03/2019 13:57, Herbert Xu wrote: > > On Sat, Mar 02, 2019 at 01:28:44PM +, Harald van Dijk wrote: > > > 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. > > I don't quite understand. Could you explain what is still buggy > > after the following patch? > I gave this example in my previous message: > command . /dev/stdin < set -o invalid > echo a > EOF > echo b > Ordinarily, "set -o invalid" is a "special built-in utility error", for > which the consequence is that a non-interactive shell "shall exit". If it > weren't a special built-in utility, it would be an "other utility (not a > special built-in) error", for which a non-interactive shell "shall not > exit". > The effect of the command built-in is that "if the command_name is the same > as the name of one of the special built-in utilities, the special properties > in the enumerated list at the beginning of Special Built-In Utilities shall > not occur." This is ambiguous as to whether it is just about the special > properties associated with the . command, or whether it includes those > associated with the set command called indirectly, but it must be one or the > other. If it includes the set command, then the shell shall not exit, and > the correct output is a followed by b. If it does not include the set > command, then the shell shall exit, and the correct output is nothing. dash > outputs b, which is wrong in either case. The way I interpret this is that any error while parsing or executing the . or eval command(s) is a "special built-in utility error" which can be "caught" using "command". Therefore, the correct output is an error message about the invalid option (to stderr) followed by b. This is what happens in FreeBSD sh (for quite a few years), ksh93 (20120801_2) and mksh (56c). Given that the text is somewhat ambiguous, I think bash's POSIX mode behaviour of exiting may also be valid. I don't like disabling the special properties of the inner set command since this creates a different kind of script for 'command .' versus '.'. Normally (though not in #!/bin/bash scripts), if I write something like 'set -o pipefail', I can safely assume that it will take if the shell continues to the next command; if the shell does not support the option, it exits. The fairly old dash v0.5.9.1 I tried works even differently, writing an error message (to stderr) followed by a. So it first continues after the error and then aborts when '.' ends. This seems clearly broken (but I think things have changed since back then). -- Jilles Tjoelker
Re: [v2 PATCH] eval: Reset handler when entering a subshell
On 03/03/2019 13:57, Herbert Xu wrote: On Sat, Mar 02, 2019 at 01:28:44PM +, Harald van Dijk wrote: 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. I don't quite understand. Could you explain what is still buggy after the following patch? I gave this example in my previous message: command . /dev/stdin
[v2 PATCH] eval: Reset handler when entering a subshell
On Sat, Mar 02, 2019 at 01:28:44PM +, Harald van Dijk wrote:
>
> 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.
I don't quite understand. Could you explain what is still buggy
after the following patch?
> 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.
Good point. Here is a revised patch to fix the nofork case too.
---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
Signed-off-by: Herbert Xu
diff --git a/src/eval.c b/src/eval.c
index 1aad31a..6ee2e1a 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -41,6 +41,7 @@
* Evaluate a command.
*/
+#include "main.h"
#include "shell.h"
#include "nodes.h"
#include "syntax.h"
@@ -492,6 +493,7 @@ evalsubshell(union node *n, int flags)
if (backgnd)
flags &=~ EV_TESTED;
nofork:
+ reset_handler();
redirect(n->nredir.redirect, 0);
evaltreenr(n->nredir.n, flags);
/* never returns */
@@ -574,6 +576,7 @@ evalpipe(union node *n, int flags)
}
}
if (forkshell(jp, lp->n, n->npipe.backgnd) == 0) {
+ reset_handler();
INTON;
if (pip[1] >= 0) {
close(pip[0]);
@@ -630,6 +633,7 @@ evalbackcmd(union node *n, struct backcmd *result)
sh_error("Pipe call failed");
jp = makejob(n, 1);
if (forkshell(jp, n, FORK_NOJOB) == 0) {
+ reset_handler();
FORCEINTON;
close(pip[0]);
if (pip[1] != 1) {
diff --git a/src/main.c b/src/main.c
index 6b3a090..b2712cb 100644
--- a/src/main.c
+++ b/src/main.c
@@ -71,6 +71,7 @@ int *dash_errno;
short profile_buf[16384];
extern int etext();
#endif
+static 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 =
+ handler = _handler;
#ifdef DEBUG
opentrace();
trputs("Shell args: "); trargs(argv);
@@ -353,3 +353,8 @@ exitcmd(int argc, char **argv)
exraise(EXEXIT);
/* NOTREACHED */
}
+
+void reset_handler(void)
+{
+ handler = _handler;
+}
diff --git a/src/main.h b/src/main.h
index 19e4983..51f1604 100644
--- a/src/main.h
+++ b/src/main.h
@@ -52,3 +52,4 @@ extern int *dash_errno;
void readcmdfile(char *);
int dotcmd(int, char **);
int exitcmd(int, char **);
+void reset_handler(void);
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
