process: don't close stdio fds during spawn

[tonistiigi]

Original Node issue: nodejs/node-v0.x-archive#7331
Simpler testcase for the same issue: https://gist.github.com/tonistiigi/9721405

What is going on here(afaik) is that when stdio fd's are already closed during spawn then they are reused for creating pipes between the processes. Spawn doesn't handle this case, duplicates the pipe endpoint back to stdio and closes the old fd(effectively closing its own stdio).

The regression is at #1083.

One way to fix this would be to only close non-stdio fd's. Others will be closed automatically by dup. 4240806

This does not happen in v0.11 because 359d667 already forbids ever closing stdio. So the other way would be to get this under v0.10 as well.

cc @indutny

[indutny]

The problem here is unrelated to close() in process.c AFAIK. The problem is that node v0.10 is closing stdio fds, while it definitely shouldn't. You could try backporting nodejs/node-v0.x-archive@e796e11 to your fork and see if it'll fix the problem.

Closing the issue for now, but please let me know if it doesn't work for you and you are sure that it is a libuv problem.

Thanks for reporting, though!

[tonistiigi]

Hi Fedor

Thanks for looking into it.

nodejs/node-v0.x-archive@e796e11 seems to be already merged under v0.10 so that doesn't help me much.

I added strace output to my testcase gist before and after #1083 that made me think this was the regression point.

If you're just saying that never closing stdio is better solution to this problem then I totally agree. There are probably more problems that may come from this that I'm not aware of.

Regarding your comment that this is a Node-only issue can you clarify what is wrong with the libuv testcase that doesn't pass under v0.10. 42408065#diff-c9dcbe3c43996f12dc7c373f3832f68fR679

[indutny]

Hm... If I get you right, you are talking about following scenario:

1. One of stdio fd's is closed
2. uv_spawn() is called
3. One of socketpair()'s fd is allocated
4. And get's closed in uv__process_child_init, which causes an error?

Something like this or more complicated?

[indutny]

If so - it should be fixed after fixing #1084

[tonistiigi]

Yes, thats pretty much it. https://gist.github.com/tonistiigi/9721405#file-after-1083-L7 is the most likely cause imho.

More complicated fd mapping support may very well solve this but note that in my case I do not care about the main process stdio fd-s at all. I just want a pipe to talk to the child process. Starting main process from a pipe that is already closed forbids me from doing that.

Also just a reminder that this is only the case in node-v0.10.25 and node-v0.10.26.

[indutny]

Ah, I think I finally got what you was talking about :)

Does this patch fix problem for you:

diff --git a/src/unix/process.c b/src/unix/process.c
index 406df5f..0e4f44b 100644
--- a/src/unix/process.c
+++ b/src/unix/process.c
@@ -315,7 +315,7 @@ static void uv__process_child_init(uv_process_options_t options,
     if (fd <= 2)
       uv__nonblock(fd, 0);

-    if (close_fd != -1)
+    if (close_fd != -1 && close_fd >= stdio_count)
       close(close_fd);
   }

[tonistiigi]

Yes, that looks equal to 4240806 that worked for me.

[indutny]

Ok, good. Mind submitting your patch with close_fd >= stdio_count instead of >= 3?

[tonistiigi]

Sure, here it is: #1217

[lucab]

It looks like this is causing a regression in the test suite:

[%  72|+ 140|-   0|T   0|S   2]: spawn_closed_process_io
> `spawn_closed_process_io` failed: exit code 6
> Output from process `spawn_closed_process_io`:
> Assertion failed in test/test-spawn.c on line 129: status == 0
> =============================================================

While I can't reproduce it on any of my machines, it has been observed by Debian builders and your jenkins linux builder:

• https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=750301
• http://jenkins.nodejs.org/job/libuv-v0.10/171/label=linux/

[tonistiigi]

@indutny I think 69f9f6f should be backported to v0.10

[indutny]

Thanks, backported in c38e97e