Discussion:
[PATCH] avoid closed stdin() in zle widgets
Stephane Chazelas
2017-06-11 18:20:45 UTC
Permalink
Hello,

in zle widgets, stdin currently appears to be closed. That's
generally not a good idea to close fds 0, 1, 2 as many commands
are confused when the files they open suddenly become their
stdin/stdout/stderr because the first free fd is 0/1/2.

See

https://unix.stackexchange.com/q/370475

for instance.

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 81687c7..0e26da3 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -2012,7 +2012,8 @@ enditem()
Attribute flags that transform the final value (tt(-L), tt(-R), tt(-Z),
tt(-l), tt(-u)) are only applied to the expanded value at the point
of a parameter expansion expression using `tt($)'. They are not applied
-when a parameter is retrieved internally by the shell for any purpose.
+when a parameter is retrieved internally by the shell for any purpose.
+They have no effect on array or associative array parameters.

The following attribute flags may be specified:

diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 6c271b5..be2b062 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1485,6 +1485,13 @@ execzlefunc(Thingy func, char **args, int set_bindk)
int inuse = w->flags & WIDGET_INUSE;
w->flags |= WIDGET_INUSE;

+ if (osi > 0) {
+ /*
+ * Many commands don't like having a closed stdin, open on
+ * /dev/null instead
+ */
+ open("/dev/null", O_RDWR | O_NOCTTY); /* ignore failure */
+ }
if (*args) {
largs = newlinklist();
addlinknode(largs, dupstring(w->u.fnnam));
Eric Cook
2017-06-12 04:15:20 UTC
Permalink
Post by Stephane Chazelas
Hello,
in zle widgets, stdin currently appears to be closed. That's
generally not a good idea to close fds 0, 1, 2 as many commands
are confused when the files they open suddenly become their
stdin/stdout/stderr because the first free fd is 0/1/2.
See
https://unix.stackexchange.com/q/370475
It is documented to work like that in zshzle

---
USER-DEFINED WIDGETS
User-defined widgets, being implemented as shell functions, can execute any normal shell command. They can also run other
widgets (whether built-in or user-defined) using the zle builtin command. The standard input of the function is closed to
prevent external commands from unintentionally blocking ZLE by reading from the terminal, but read -k or read -q can be used
to read characters. Finally, they can examine and edit the ZLE buffer being edited by reading and setting the special
parameters described below.
Stephane Chazelas
2017-06-12 06:05:54 UTC
Permalink
2017-06-12 00:15:20 -0400, Eric Cook:
[...]
Post by Eric Cook
Post by Stephane Chazelas
in zle widgets, stdin currently appears to be closed. That's
generally not a good idea to close fds 0, 1, 2 as many commands
are confused when the files they open suddenly become their
stdin/stdout/stderr because the first free fd is 0/1/2.
[...]
Post by Eric Cook
It is documented to work like that in zshzle
[...]

Well spotted. Thanks. (I also left a diff by mistake for an
unrelated issue discussed earlier on. Sorry about that).

New patch:

diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index b65e3be..bd0252f 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -750,12 +750,12 @@ sect(User-Defined Widgets)
cindex(widgets, user-defined)
User-defined widgets, being implemented as shell functions,
can execute any normal shell command. They can also run other widgets
-(whether built-in or user-defined) using the tt(zle) builtin command.
-The standard input of the function is closed to prevent external commands
-from unintentionally blocking ZLE by reading from the terminal, but
-tt(read -k) or tt(read -q) can be used to read characters. Finally,
-they can examine and edit the ZLE buffer being edited by
-reading and setting the special parameters described below.
+(whether built-in or user-defined) using the tt(zle) builtin command. The
+standard input of the function is redirected from /dev/null to prevent
+external commands from unintentionally blocking ZLE by reading from the
+terminal, but tt(read -k) or tt(read -q) can be used to read characters.
+Finally, they can examine and edit the ZLE buffer being edited by reading
+and setting the special parameters described below.

cindex(parameters, editor)
cindex(parameters, zle)
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 6c271b5..be2b062 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1485,6 +1485,13 @@ execzlefunc(Thingy func, char **args, int set_bindk)
int inuse = w->flags & WIDGET_INUSE;
w->flags |= WIDGET_INUSE;

+ if (osi > 0) {
+ /*
+ * Many commands don't like having a closed stdin, open on
+ * /dev/null instead
+ */
+ open("/dev/null", O_RDWR | O_NOCTTY); /* ignore failure */
+ }
if (*args) {
largs = newlinklist();
addlinknode(largs, dupstring(w->u.fnnam));
Daniel Shahaf
2017-06-12 14:34:49 UTC
Permalink
Post by Stephane Chazelas
[...]
Post by Eric Cook
Post by Stephane Chazelas
in zle widgets, stdin currently appears to be closed. That's
generally not a good idea to close fds 0, 1, 2 as many commands
are confused when the files they open suddenly become their
stdin/stdout/stderr because the first free fd is 0/1/2.
[...]
Post by Eric Cook
It is documented to work like that in zshzle
[...]
Well spotted. Thanks. (I also left a diff by mistake for an
unrelated issue discussed earlier on. Sorry about that).
diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index b65e3be..bd0252f 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -750,12 +750,12 @@ sect(User-Defined Widgets)
Incompatible changes should be mentioned in NEWS or README.

I'm not convinced that the change should be made. Devil's advocate
argues that this is a bug in dircolors(1), so (a) as an immediate
workaround, people with a buggy dircolors(1) command should redirect
stdin from /dev/null when invoking it, (b) those people should fix their
dircolors(1) command to not assume fd 0 is open.

Daniel
Stephane Chazelas
2017-06-12 15:10:42 UTC
Permalink
This post might be inappropriate. Click to display it.
Stephane Chazelas
2017-06-12 15:19:02 UTC
Permalink
Even though I can't imagine previous relying on stdin being
closed (and consider myself the previous behaviour a bug),
please find a 3rd version of the patch below with the requested
change:

diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index b65e3be..bd0252f 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -745,22 +745,22 @@ User-defined widgets are defined using `tt(zle -N)', and implemented
as shell functions. When the widget is executed, the corresponding
shell function is executed, and can perform editing (or other) actions.
It is recommended that user-defined widgets should not have names
starting with `tt(.)'.
sect(User-Defined Widgets)
cindex(widgets, user-defined)
User-defined widgets, being implemented as shell functions,
can execute any normal shell command. They can also run other widgets
-(whether built-in or user-defined) using the tt(zle) builtin command.
-The standard input of the function is closed to prevent external commands
-from unintentionally blocking ZLE by reading from the terminal, but
-tt(read -k) or tt(read -q) can be used to read characters. Finally,
-they can examine and edit the ZLE buffer being edited by
-reading and setting the special parameters described below.
+(whether built-in or user-defined) using the tt(zle) builtin command. The
+standard input of the function is redirected from /dev/null to prevent
+external commands from unintentionally blocking ZLE by reading from the
+terminal, but tt(read -k) or tt(read -q) can be used to read characters.
+Finally, they can examine and edit the ZLE buffer being edited by reading
+and setting the special parameters described below.

cindex(parameters, editor)
cindex(parameters, zle)
These special parameters are always available in widget functions, but
are not in any way special outside ZLE. If they have some normal value
outside ZLE, that value is temporarily inaccessible, but will return
when the widget function exits. These special parameters in fact have
local scope, like parameters created in a function using tt(local).
diff --git a/NEWS b/NEWS
index 568b160..e745750 100644
--- a/NEWS
+++ b/NEWS
@@ -9,16 +9,22 @@ Changes from 5.3.1 to 5.4

The 'exec' and 'command' precommand modifiers, and options to them, are
now parsed after parameter expansion. Previously, both the modifier and
any options to it were parsed between alias expansion and parameter
expansion (see zshexpn(1)), so they could neither be quoted nor be the
result of parameter expansion. Examples: 's=command; $s -V ls' and
'\command -V ls' now work as expected.

+Functions executed by ZLE widgets no longer have they standard input
+closed, but is now redirected from /dev/null instead. That still guards
+against user defined widgets inadvertently reading from the tty device,
+and addresses the antisocial behaviour of running a command with its
+stdin closed.
+

Changes from 5.2 to 5.3.1
-------------------------

There are only minor compatibility fixes between 5.3 and 5.3.1.

It is possible to enable character width support for Unicode 9 by
configuring with `--enable-unicode9'; this compiles in some additional
diff --git a/README b/README
index 432a35e..30023da 100644
--- a/README
+++ b/README
@@ -69,16 +69,22 @@ patch-format string, to prevent literal `%' signs in the interpolated
value from being interpreted as prompt escape sequences. If you use
${vcs_info_msg_0_} in a context other than the shell prompt, you may need
to undo the escaping with:

print -v vcs_info_msg_0_ -Pr -- "${vcs_info_msg_0_}"

This is also needed if $vcs_info_msg_0_ is used to set $psvar.

+4) functions executed by ZLE widgets no longer have they standard input
+closed, but is now redirected from /dev/null instead. That still guards
+against user defined widgets inadvertently reading from the tty device,
+and addresses the antisocial behaviour of running a command with its
+stdin closed.
+
Incompatibilities between 5.0.8 and 5.3
----------------------------------------

1) In character classes delimited by "[" and "]" within patterns, whether
used for filename generation (globbing) or other forms of pattern
matching, it used not to be possible to quote "-" when used for a range,
or "^" and "!" when used for negating a character set. The characters can
now be quoted by any of the standard shell means, but note that
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 6c271b5..be2b062 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1480,16 +1480,23 @@ execzlefunc(Thingy func, char **args, int set_bindk)
ret = execimmortal(func, args);
} else {
int osc = sfcontext, osi = movefd(0);
int oxt = isset(XTRACE);
LinkList largs = NULL;
int inuse = w->flags & WIDGET_INUSE;
w->flags |= WIDGET_INUSE;

+ if (osi > 0) {
+ /*
+ * Many commands don't like having a closed stdin, open on
+ * /dev/null instead
+ */
+ open("/dev/null", O_RDWR | O_NOCTTY); /* ignore failure */
+ }
if (*args) {
largs = newlinklist();
addlinknode(largs, dupstring(w->u.fnnam));
while (*args)
addlinknode(largs, dupstring(*args++));
}
startparamscope();
makezleparams(0);
Daniel Shahaf
2017-06-12 15:34:35 UTC
Permalink
Post by Stephane Chazelas
Even though I can't imagine previous relying on stdin being
closed (and consider myself the previous behaviour a bug),
please find a 3rd version of the patch below with the requested
Thanks for adding the README hunk. I assumed the behaviour was
intentional because it was documented (and it didn't seem
"antisocial" to me, however surprised a main() function might be to
find its stdin closed).

Cheers,

Daniel
Bart Schaefer
2017-06-12 16:09:23 UTC
Permalink
Post by Daniel Shahaf
Post by Stephane Chazelas
please find a 3rd version of the patch below with the requested
it didn't seem
"antisocial" to me, however surprised a main() function might be
Yeah, maybe I'm just getting crotchety but that remark seemed a bit
snarky for a NEWS entry.
Stephane Chazelas
2017-06-12 19:02:18 UTC
Permalink
This post might be inappropriate. Click to display it.
Bart Schaefer
2017-06-14 22:44:25 UTC
Permalink
On Jun 12, 8:02pm, Stephane Chazelas wrote:
}
} My point is that a command should be able to reasonably make some
} assumptions, like:
}
} - stdin should be open for at least reading
} - stdout, stderr should be open for at least writing

Although I understand the security implication of accidentally opening
some other file onto fd 0/1/2, I can't agree with the above statements.
Taken to the logical conclusion, the >&- <&- or 2>&- operators would
be required always to fail with an error.

It should not be the shell's job to plug this hole. I will agree that
a valid argument is that the shell should not implicitly *open* this
hole, which one could also argue is what the completion system had
been doing in spite of the behavior being documented. However, with
these two likely exceptions --

} - argv[0] should be set (argc > 0)
} - no dups in the environment

-- there is nothing else on your list where I would agree that the
shell should ignore the user's directives in the name of protecting
an external command from itself.

} $ (limit stacksize 100k; zsh)
} zsh: segmentation fault
} zsh: segmentation fault
}
} (twice!?). Is that a bug?

I believe what's happening is that both the zsh inside the subshell
and the parent handling the subshell exit are reporting the error,
so one failure / two messages.

However, I can't test directly because I can start zsh -f with a hard
limit stack size of *zero*, so I'm quite curious as to why you get a
crash on 100k.
Stephane Chazelas
2017-06-15 08:42:44 UTC
Permalink
Post by Bart Schaefer
}
} My point is that a command should be able to reasonably make some
}
} - stdin should be open for at least reading
} - stdout, stderr should be open for at least writing
Although I understand the security implication of accidentally opening
some other file onto fd 0/1/2, I can't agree with the above statements.
Taken to the logical conclusion, the >&- <&- or 2>&- operators would
be required always to fail with an error.
It should not be the shell's job to plug this hole. I will agree that
a valid argument is that the shell should not implicitly *open* this
hole, which one could also argue is what the completion system had
been doing in spite of the behavior being documented. However, with
these two likely exceptions --
} - argv[0] should be set (argc > 0)
} - no dups in the environment
-- there is nothing else on your list where I would agree that the
shell should ignore the user's directives in the name of protecting
an external command from itself.
I think you misinterpreted what I said, I did not imply that the
shell should take upon itself to prevent users from creating
those pathological conditions, but that it should not take upon
itself to creating those pathological conditions itself.

That's the "Be conservative in what you do" in the "Be
conservative in what you do, be liberal in what you accept from
others" (and yes, it's a case where "dircolors" did not fully
apply the "be liberal in what you accept").

In other words, I would certainly not want zsh to refuse to <&-
just like I would not want close(0) in C to fail.

I would even welcome new options to the "env" utility to execute
a command without arguments or with arbitrary argv[0], or with
duplicate env vars or with env strings without = characters so
one can test applications in those pathological conditions (and
possibly raise awareness on the security implications), but
if we put aside those testing cases, an application like zsh
should not intentionaly (by itself) cause those pathological
conditions.

The case of zle widgets running commands with stdin close was
not a case where the user requested stdin to be closed.
Post by Bart Schaefer
} $ (limit stacksize 100k; zsh)
} zsh: segmentation fault
} zsh: segmentation fault
}
} (twice!?). Is that a bug?
I believe what's happening is that both the zsh inside the subshell
and the parent handling the subshell exit are reporting the error,
so one failure / two messages.
However, I can't test directly because I can start zsh -f with a hard
limit stack size of *zero*, so I'm quite curious as to why you get a
crash on 100k.
I suspect your system won't bring the stack size below some
threshold (or just ignores that limit). With a null stack size,
you woudn't even be able to call execve()

My several segvs were probably down to some child processes
spawned by my ~/.zshrc dying there upon stack overflow.

The point was just an illustration that you can't always deal
with all pathological conditions, you have to put a limit on
the amount of effort you're willing to put into covering for all
possible pathological cases, and IMO a closed stdin is one such
pathological case (even if not as much as a very small stack
size, or random memory bit flips), and one you can blame on the
calling application.
--
Stephane
Bart Schaefer
2017-06-17 03:03:57 UTC
Permalink
On Jun 15, 9:42am, Stephane Chazelas wrote:
}
} In other words, I would certainly not want zsh to refuse to <&-
} just like I would not want close(0) in C to fail.
}
} The case of zle widgets running commands with stdin close was
} not a case where the user requested stdin to be closed.

So, just being contrarian at this point, if _main_complete were to have
done "exec <&-" that would be OK, but because the C code does it we
have to patch it.

The point being that the completion system does LOTS of things that the
user didn't explicitly request, but requested implicitly by choosing to
use it. It traps various signals, throws away error output, for some
commands even runs "su" or "sudo" -- although the latter are probably
the best analogy for this because we added _comp_priv_prefix.

Loading...