Re: CVS commit: src/bin/sh

[christos%zoulas.com@localhost (Christos Zoulas)]

On Jan 5,  2:07am, kre%munnari.OZ.AU@localhost (Robert Elz) wrote:
-- Subject: Re: CVS commit: src/bin/sh

| It isn't a matter of whether it can be made to work or not for
| NetBSD's build system, or whether you have hodden a bug in the
| rc scripts (I'll explain why "hidden" rather than "fixed" below)
| but of what is right.
| 
| In the test case you included in the commit log, I expect the
| echo in test2 to work, and write to file "out" the way NetBSD's
| sh (and all Bourne shell variants, since the first one) have
| always done.
| 
| That is, the shell code
| 
| 	exec 3>&1
| 	cmd
| 
| is (and always was intended to be) the same as the (abbreviated) C code
| 
| 	dup2(1, 3);
| 	if (fork() == 0)
| 		execlp("cmd", "cmd", (char *)0);
| 	else
| 		wait(&status);
| 
| The case you use "exec 6>out" is similar, but messier to write directly
| in C because there's no "open_giving_fd_n()" function, so it needs an
| open/dup2/close sequence, so I'll omit that one (I'd probably
| get it wrong...).  The effect should be the same however.
| 
| If I wanted fd 6 closed when test2 was run in your example, I'd write
| the line that runs it as ...
| 
| 	sh ./test2 6>&-
| 
| We know fd 6 is open at that point, the "exec >&6" is there explicitly,
| and the close hasn't happened yet.
| 
| Now I know that POSIX says you're allowed to do it the way you have changed
| it to, but that's only because ksh (for whatever reason) decided to change
| the semantics, and what ksh does seems to have inordinate sway with the
| posix shell people...
| 
| But, they say ...
| 
| 	it is unspecified whether those file descriptors remain open
| 	when the shell invokes another utility
| 
| so the way that NetBSD's shell (and Bourne shells in general) have always
| done it is just as valid.

Yes, the issues are:

    - Is leaving descriptors open really needed? I can write the above
      example in a way it works...
    - Should portable code rely on open file descriptors across exec, since
      it is unspecified behavior?
    - Should there be away to control this from the shell? And if there
      is what should the default be? What should the syntax be?

| That's why you haven't "fixed" anything in the rc system by the change.
| What you have done is made it rely upon (your version of) NetBSD's
| implementation of that unspecified operation.   Take the scripts to any
| other (rational) system and the bug (if there is one) would reappear.
| On NetBSD the bug will be hidden, which makes it one of more insidious type.

Yes, but fixing this is not easy I think, without relying on a non-portable
feature of the shell to do it (or forking another subshell). What we want is:

	- Setup redirections so that stdout and stderr are captured
	- Execute command, where if successful the file descriptors
	  are closed, and if it failed, they are kept open for further
	  processing.

| If there is a bug there, it should be fixed in the scripts (by explicitly
| closing fd's that have been opened using exec when they are not needed).

If possible yes, we should definitely fix them.

| The "if" is not because I believe the daemons you showed need, or want,
| to have those fds open, but because the rc scripts are complex, and I
| have not analysed them to determine whether they are relying upon the
| "keep open through exec" behaviour for some reason.  And yes, that would
| also be relying on unspecified behaviour, but it is at least the ancient
| historic behaviour, where it is very hard to code correctly in any sane way
| if close_on_exec is forced on when it is not wanted (whereas it is trivial
| to close fds that have been opened but are not wanted).

Yes, I think that the scripts could be fixed by doubling the amount of forking..
I could be wrong though.

| kre
| 
| ps: have you tested the shell with your change to make sure that
| 
| 	exec >outfile
| 
| followed by
| 
| 	cmd1; cmd2; cmd3
| 
| doesn't run those commands with stdout closed?

Yup, it works. Nothing would work if it did not. The closeon-exec feature
is only for specific redirections (redirections to specific file numbers).
I.e. should I expect the following to work?

$ cat test1
#!/bin/sh
exec 6> out
echo "test" >&6
sh ./test2
exec 6>&-

$ cat test2
cat cloexec/test2
echo "test2" >&6

$ ./test1

I am open to suggestions, but I think that closing on exec is a nice
feature to have from the security POV, and it should be the default,
unless otherwise specified. I view the ksh behavior a correction
to the loose standard that was just there before close on exec did not
even exist when the shell file-descriptor redirection was invented IIRC.

christos