unix: spawn should protect its input fds #226
Conversation
spawn allows copying descriptor numbers during exec, but it wasn't protecting them. Under some conditions, such as swapping stdout and stderr, it would do this: dup 2 -> 1 dup 1 -> 2 With end result that both 1 and 2 would be stderr, rather than being swapped. With this change, the above sequence gets rewritten to: dup 1 -> 3 dup 2 -> 1 dup 3 -> 2
|
I'm not sure it works in all Unices, but how about using this to get duplicated fds beyond stdio_count? It's simpler, assuming it works in all platforms we need it. |
|
Also, a test! :-) |
|
F_DUPFD fcntl: I wasn't aware of that fcntl. It looks perfect. I see it on Linux and OS X's man pages, but no idea how wide-spread it is. I could do an #ifdef F_DUPFD... but then I would be maintaining two copies of the same code. I considered changing the dup2() to be something like: Basically, repeatedly calling dup() until the result is > use_fd. That loop would normally never run more than once, so have zero-overhead in the normal case. It might be the best "portable" alternative to F_DUPFD. What to do, rewrite to the "portable" F_DUPFD, or just use F_DUPFD, and see if builds fail anywhere? As for tests, I'm sorry, but I can find no tests for spawn-with-stdio at all, anywhere in uv, that I can use as a base to add this case to. Did I fail to find the right code? Writing spawn() tests from scratch seems like it would take days (that I won't have in the next weeks). Can you point me to a test I can use as a base? Or do you have any ideas how to write the test? |
|
AFAIS, F_DUPFD is part of POSIX (http://pubs.opengroup.org/onlinepubs/009695399/functions/fcntl.html) so we should be fine going that road. If any of the supported platforms doesn't have it (which I doubt) we can reconsider. As for tests, spawn_stdout uses a spawn_helper2 process which just prints something. You could use it as a base and make it print to both stdout and stderr, which are redirected to 2 files, then synchronously read and compare the content. |
|
OK, @saghul I'll rewrite to use F_DUPFD, and take a shot at a test. |
|
@sam-github will you have time to work on this? If possible it would be nice to include it on the next release. The test can probably be written similar to #224. I can try to finish it if you're short on time. |
Alternative implementation (and test) to libuv#226 Fixes: joyent/libuv#1084
Alternative implementation (and test) to libuv#226 Fixes: joyent/libuv#1084 PR-URL: libuv#309 Reviewed-By: Ben Noordhuis <[email protected]>
|
#309 landed, closing! |
spawn allows copying descriptor numbers during exec, but it wasn't
protecting them. Under some conditions, such as swapping stdout and
stderr, it would do this:
dup 2 -> 1
dup 1 -> 2
With end result that both 1 and 2 would be stderr, rather than being
swapped.
With this change, the above sequence gets rewritten to:
dup 1 -> 3
dup 2 -> 1
dup 3 -> 2
This fix can be verified against io.js. Using fd-dup.js:
Without this patch both fd 1 and 2 in child end up in parent's stderr:
(Above assumes an io.js that has libuv bug #224 fixed, otherwise it will output nothing, because fd 2 will be closed).
After this patch, they are swapped, as intended:
/to @indutny, this fixes joyent/libuv#1084