Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

unix: don't close fd immediately after dup2() in uv_spawn() #923

Closed
bnoordhuis opened this issue Sep 7, 2013 · 5 comments
Closed

unix: don't close fd immediately after dup2() in uv_spawn() #923

bnoordhuis opened this issue Sep 7, 2013 · 5 comments
Labels

Comments

@bnoordhuis
Copy link
Contributor

Test case:

var fd = require('fs').openSync('/dev/tty', 'w');
require('child_process').spawn('/bin/false', [], { stdio: ['ignore', fd, fd] });
$ strace node t.js
$ strace -fe open,dup2 out/Release/node tmp/t.js
<snip>
[pid 19418] open("/dev/tty", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 9
<snip>
[pid 19424] dup2(9, 1)                  = 1
[pid 19424] dup2(9, 2)                  = -1 EBADF (Bad file descriptor)
<snip>
@indutny
Copy link
Contributor

indutny commented Sep 7, 2013

Mind if I'll take a look at it?

@ghost ghost assigned indutny Sep 7, 2013
@bnoordhuis
Copy link
Contributor Author

Sure, go ahead.

@bnoordhuis
Copy link
Contributor Author

I'm reasonably sure the patch below fixes it but I'm unsure as to how correct it is. It passes all node and libuv tests but that doesn't mean it doesn't have bugs, of course.

diff --git a/src/unix/process.c b/src/unix/process.c
index 990ea15..4d86089 100644
--- a/src/unix/process.c
+++ b/src/unix/process.c
@@ -285,8 +285,10 @@ static void uv__process_child_init(const uv_process_options_t* options,
     close_fd = pipes[fd][0];
     use_fd = pipes[fd][1];

-    if (use_fd >= 0)
-      close(close_fd);
+    if (use_fd >= 0) {
+      if (close_fd != -1)
+        close(close_fd);
+    }
     else if (fd >= 3)
       continue;
     else {
@@ -304,15 +306,22 @@ static void uv__process_child_init(const uv_process_options_t* options,

     if (fd == use_fd)
       uv__cloexec(use_fd, 0);
-    else {
+    else
       dup2(use_fd, fd);
-      close(use_fd);
-    }

     if (fd <= 2)
       uv__nonblock(fd, 0);
   }

+  for (fd = 0; fd < stdio_count; fd++) {
+    if (fd >= 3)
+      break;
+
+    use_fd = pipes[fd][1];
+    if (fd != use_fd)
+      close(use_fd);
+  }
+
   if (options->cwd != NULL && chdir(options->cwd)) {
     uv__write_int(error_fd, -errno);
     perror("chdir()");

@indutny
Copy link
Contributor

indutny commented Sep 10, 2013

@bnoordhuis LGTM

@bnoordhuis
Copy link
Contributor Author

I found another bug in uv__process_child_init(), let me follow up with a more thorough patch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants