Skip to content

Commit

Permalink
unix: don't close the fds we just setup
Browse files Browse the repository at this point in the history 
If a descriptor was being copied to an fd that *was not its current
value*, it should get closed after being copied. But the existing code
would close the fd, even when it no longer referred to the original
descriptor. Don't do this, only close fds that are not in the desired
range for the child process.

See nodejs/node#862
  • Loading branch information
@sam-github @saghul
sam-github authored and saghul committed Apr 7, 2015
1 parent e2e218b commit 2d4939c
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/unix/process.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ static void uv__process_child_init(const uv_process_options_t* options,
for (fd = 0; fd < stdio_count; fd++) {
use_fd = pipes[fd][1];

if (use_fd >= 0 && fd != use_fd)
if (use_fd >= stdio_count)
close(use_fd);
}

Expand Down

4 comments on commit 2d4939c

@bnoordhuis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me but process.c is such a twisty maze of little if statements all alike that it took me 15 minutes of staring at the code and I'm still not 100% sure.

However, it does look like the close can be done right after the dup2 call a few lines up, allowing this loop to be removed. (Also, close should really be uv__close.)

Apropos that dup2 call, there is a potential for bugs there because it doesn't check the return code.

@saghul
Copy link
Owner

@saghul saghul commented on 2d4939c Apr 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me but process.c is such a twisty maze of little if statements all alike that it took me 15 minutes of staring at the code and I'm still not 100% sure.

Agreed :-S

However, it does look like the close can be done right after the dup2 call a few lines up, allowing this loop to be removed. (Also, close should really be uv__close.)

Hum, But that would break if you want the child's fd 4 and 5 to be the partent's fd 42, right? because after you dup 42 into 4 you'd close it and on the next iteration you'd fail.

Apropos that dup2 call, there is a potential for bugs there because it doesn't check the return code.

Indeed. AFAIS there is not much we can do other than ignoring it though...

@bnoordhuis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that would break if you want the child's fd 4 and 5 to be the partent's fd 42, right? because after you dup 42 into 4 you'd close it and on the next iteration you'd fail.

You're right, good point. Does the same logic apply to the uv__close(close_fd) call?

@saghul
Copy link
Owner

@saghul saghul commented on 2d4939c Apr 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, because close_fd will point to a fd we just opened to /dev/null, in order to redirect stdin, stdout and stderr to it, so in that case use_fd was -1.

Please sign in to comment.