rakitzis / rc

rc shell -- independent re-implementation for Unix of the Plan 9 shell (from circa 1992)
Other
250 stars 23 forks source link

Redirections cause builtins to have no effect #62

Closed jmi2k closed 2 weeks ago

jmi2k commented 4 years ago

For example, try doing:

; cd /some/path/somewhere > /dev/null
; pwd
/your/pwd

Presumably generalizable to other commands (I just don't know how to test this with another command/builtin)

rakitzis commented 4 years ago

Builtins will execute in a subshell if redirection is used, so that explains the behavior.

jmi2k commented 4 years ago

Is it expected behavior? I hope it isn't, it's really confusing...

rakitzis commented 4 years ago

Well, "expected" in the sense that it's worked this way for 30 years. You may be the first to raise it as a problem! I fear that it would take a significant restructure to change this behavior.

jmi2k commented 4 years ago

I've discovered it trying to (improperly) handle a cd to nowhere, but I've learned better ways and now it's not a problem anymore. However, I've spun up my Plan 9 VM and at least it seems to work correctly there, so it's a quirk of this implementation. I fully understand that the solution can be difficult for very little to no gain, so what do you want me to do with this issue? Leaving it open as some kind of documentation (just in case there is a second "me" out there trying to do things wrong), or closing it?

image

rakitzis commented 4 years ago

Well, I'm not in charge of maintaining rc these days! But I'd say leave it open. It does seem like a hair-splitting issue, as you say, one with little gain. More importantly, I'd be concerned that any fix didn't regress the shell in other, more important ways.

xyb3rt commented 1 year ago

This even effects break, continue and return because they're implemented as builtins!

rakitzis commented 1 year ago

Yes, this explains the other issue on return with a redirection. Do you have an idea of how plan9 rc saves and restores fd state for builtin redirections? I haven't examined the source.

As I said earlier the fix is probably notha point fix, as the strategy rc uses for applying redirections to do it after fork() and before exec(). The kernel gives the subprocess a copy-on-write duplicate of the fd state, while the parent's fd state is never altered.

This strategy does not work for builtins, since they neither fork() nor exec(). The exception here is applying redirections on the exec builtin: you get exactly the effect you want (as in the Bourne shell) of altering the fd set of the controlling shell.

I "solved" this way back by running builtins in a subshell to avoid altering the fd set of the controlling shell. This leads to some of the unintended properties you have found (cd with no effect, etc.) but works for builtins which don't alter shell state.

xyb3rt commented 1 year ago

I think we should either overhaul this completely, which might break existing scripts or not do it at all. The proper way would be to set up redirections like variables and push a frame onto estack so that we can reverse them.

Limiting temporary redirections to simple commands calling a builtin would not solve these cases that bash also does not handle in a subshell:

foo() {
    cd "$@"
    pwd
}
foo / >/dev/null
for f in *; do cd /; echo "$f"; done >/dev/null

Both output nothing and end up in /.

rakitzis commented 1 year ago

I agree. I ended up in the same place, by leaving this behavior alone. I'm not sure if it's worth a fix, even though it breaks the principle of "least surprise".

rakitzis commented 2 weeks ago

Without a solid proposal for a fix, or without a view as to what this would break (as it's a breaking change) I will mark this closed for now.