unix: sanity-check fds before closing

bnoordhuis · bnoordhuis · commit 359d6678930f · 2013-10-01T03:55:54.000+02:00
Ensure that close() system calls don't close stdio file descriptors because that is almost never the intention. This is also a partial workaround for a kernel bug that seems to affect all Linux kernels when stdin is a pipe that gets closed: fd 0 keeps signalling EPOLLHUP but a subsequent call to epoll_ctl(EPOLL_CTL_DEL) fails with EBADF. See nodejs/node-v0.x-archive#6271 for details and a test case.
diff --git a/src/unix/aix.c b/src/unix/aix.c
@@ -85,7 +85,7 @@ int uv_exepath(char* buffer, size_t* size) {
     return fd;
 
   res = read(fd, &ps, sizeof(ps));
-  close(fd);
+  uv__close(fd);
   if (res < 0)
     return res;
 
@@ -179,7 +179,7 @@ int uv_resident_set_memory(size_t* rss) {
     *rss = (size_t)psinfo.pr_rssize * 1024;
     err = 0;
   }
-  close(fd);
+  uv__close(fd);
 
   return err;
 }
@@ -291,14 +291,14 @@ int uv_interface_addresses(uv_interface_address_t** addresses,
   }
 
   if (ioctl(sockfd, SIOCGSIZIFCONF, &size) == -1) {
-    close(sockfd);
+    uv__close(sockfd);
     return -ENOSYS;
   }
 
   ifc.ifc_req = (struct ifreq*)malloc(size);
   ifc.ifc_len = size;
   if (ioctl(sockfd, SIOCGIFCONF, &ifc) == -1) {
-    close(sockfd);
+    uv__close(sockfd);
     return -ENOSYS;
   }
 
@@ -317,7 +317,7 @@ int uv_interface_addresses(uv_interface_address_t** addresses,
 
     memcpy(flg.ifr_name, p->ifr_name, sizeof(flg.ifr_name));
     if (ioctl(sockfd, SIOCGIFFLAGS, &flg) == -1) {
-      close(sockfd);
+      uv__close(sockfd);
       return -ENOSYS;
     }
 
@@ -331,7 +331,7 @@ int uv_interface_addresses(uv_interface_address_t** addresses,
   *addresses = (uv_interface_address_t*)
     malloc(*count * sizeof(uv_interface_address_t));
   if (!(*addresses)) {
-    close(sockfd);
+    uv__close(sockfd);
     return -ENOMEM;
   }
   address = *addresses;
@@ -348,7 +348,7 @@ int uv_interface_addresses(uv_interface_address_t** addresses,
 
     memcpy(flg.ifr_name, p->ifr_name, sizeof(flg.ifr_name));
     if (ioctl(sockfd, SIOCGIFFLAGS, &flg) == -1) {
-      close(sockfd);
+      uv__close(sockfd);
       return -ENOSYS;
     }
 
@@ -374,7 +374,7 @@ int uv_interface_addresses(uv_interface_address_t** addresses,
 
 #undef ADDR_SIZE
 
-  close(sockfd);
+  uv__close(sockfd);
   return 0;
 }
 
diff --git a/src/unix/async.c b/src/unix/async.c
@@ -237,11 +237,11 @@ void uv__async_stop(uv_loop_t* loop, struct uv__async* wa) {
     return;
 
   uv__io_stop(loop, &wa->io_watcher, UV__POLLIN);
-  close(wa->io_watcher.fd);
+  uv__close(wa->io_watcher.fd);
   wa->io_watcher.fd = -1;
 
   if (wa->wfd != -1) {
-    close(wa->wfd);
+    uv__close(wa->wfd);
     wa->wfd = -1;
   }
 }
diff --git a/src/unix/core.c b/src/unix/core.c
@@ -340,7 +340,7 @@ int uv__socket(int domain, int type, int protocol) {
     err = uv__cloexec(sockfd, 1);
 
   if (err) {
-    close(sockfd);
+    uv__close(sockfd);
     return err;
   }
 
@@ -397,7 +397,7 @@ int uv__accept(int sockfd) {
       err = uv__nonblock(peerfd, 1);
 
     if (err) {
-      close(peerfd);
+      uv__close(peerfd);
       return err;
     }
 
@@ -406,6 +406,26 @@ int uv__accept(int sockfd) {
 }
 
 
+int uv__close(int fd) {
+  int saved_errno;
+  int rc;
+
+  assert(fd > -1);  /* Catch uninitialized io_watcher.fd bugs. */
+  assert(fd > STDERR_FILENO);  /* Catch stdio close bugs. */
+
+  saved_errno = errno;
+  rc = close(fd);
+  if (rc == -1) {
+    rc = -errno;
+    if (rc == -EINTR)
+      rc = -EINPROGRESS;  /* For platform/libc consistency. */
+    errno = saved_errno;
+  }
+
+  return rc;
+}
+
+
 #if defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__)
 
 int uv__nonblock(int fd, int set) {
@@ -514,7 +534,7 @@ int uv__dup(int fd) {
 
   err = uv__cloexec(fd, 1);
   if (err) {
-    close(fd);
+    uv__close(fd);
     return err;
   }
 
diff --git a/src/unix/internal.h b/src/unix/internal.h
@@ -130,6 +130,7 @@ enum {
 
 /* core */
 int uv__nonblock(int fd, int set);
+int uv__close(int fd);
 int uv__cloexec(int fd, int set);
 int uv__socket(int domain, int type, int protocol);
 int uv__dup(int fd);
diff --git a/src/unix/kqueue.c b/src/unix/kqueue.c
@@ -354,6 +354,6 @@ void uv__fs_event_close(uv_fs_event_t* handle) {
   free(handle->filename);
   handle->filename = NULL;
 
-  close(handle->event_watcher.fd);
+  uv__close(handle->event_watcher.fd);
   handle->event_watcher.fd = -1;
 }
diff --git a/src/unix/linux-core.c b/src/unix/linux-core.c
@@ -98,7 +98,7 @@ int uv__platform_loop_init(uv_loop_t* loop, int default_loop) {
 void uv__platform_loop_delete(uv_loop_t* loop) {
   if (loop->inotify_fd == -1) return;
   uv__io_stop(loop, &loop->inotify_read_watcher, UV__POLLIN);
-  close(loop->inotify_fd);
+  uv__close(loop->inotify_fd);
   loop->inotify_fd = -1;
 }
 
@@ -309,7 +309,7 @@ int uv_resident_set_memory(size_t* rss) {
     n = read(fd, buf, sizeof(buf) - 1);
   while (n == -1 && errno == EINTR);
 
-  SAVE_ERRNO(close(fd));
+  uv__close(fd);
   if (n == -1)
     return -errno;
   buf[n] = '\0';
diff --git a/src/unix/linux-inotify.c b/src/unix/linux-inotify.c
@@ -81,7 +81,7 @@ static int new_inotify_fd(void) {
     err = uv__nonblock(fd, 1);
 
   if (err) {
-    close(fd);
+    uv__close(fd);
     return err;
   }
 
diff --git a/src/unix/loop.c b/src/unix/loop.c
@@ -136,12 +136,12 @@ static void uv__loop_delete(uv_loop_t* loop) {
   uv__async_stop(loop, &loop->async_watcher);
 
   if (loop->emfile_fd != -1) {
-    close(loop->emfile_fd);
+    uv__close(loop->emfile_fd);
     loop->emfile_fd = -1;
   }
 
   if (loop->backend_fd != -1) {
-    close(loop->backend_fd);
+    uv__close(loop->backend_fd);
     loop->backend_fd = -1;
   }
 
diff --git a/src/unix/pipe.c b/src/unix/pipe.c
@@ -93,11 +93,11 @@ int uv_pipe_bind(uv_pipe_t* handle, const char* name) {
 
 out:
   if (bound) {
-    /* unlink() before close() to avoid races. */
+    /* unlink() before uv__close() to avoid races. */
     assert(pipe_fname != NULL);
     unlink(pipe_fname);
   }
-  close(sockfd);
+  uv__close(sockfd);
   free((void*)pipe_fname);
   return err;
 }
diff --git a/src/unix/process.c b/src/unix/process.c
@@ -231,7 +231,7 @@ static int uv__process_open_stream(uv_stdio_container_t* container,
   if (!(container->flags & UV_CREATE_PIPE) || pipefds[0] < 0)
     return 0;
 
-  if (close(pipefds[1]))
+  if (uv__close(pipefds[1]))
     if (errno != EINTR && errno != EINPROGRESS)
       abort();
 
@@ -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)
+        uv__close(close_fd);
+    }
     else if (fd >= 3)
       continue;
     else {
@@ -306,7 +308,7 @@ static void uv__process_child_init(const uv_process_options_t* options,
       uv__cloexec(use_fd, 0);
     else {
       dup2(use_fd, fd);
-      close(use_fd);
+      uv__close(use_fd);
     }
 
     if (fd <= 2)
@@ -414,8 +416,8 @@ int uv_spawn(uv_loop_t* loop,
 
   if (pid == -1) {
     err = -errno;
-    close(signal_pipe[0]);
-    close(signal_pipe[1]);
+    uv__close(signal_pipe[0]);
+    uv__close(signal_pipe[1]);
     goto error;
   }
 
@@ -424,7 +426,7 @@ int uv_spawn(uv_loop_t* loop,
     abort();
   }
 
-  close(signal_pipe[1]);
+  uv__close(signal_pipe[1]);
 
   process->errorno = 0;
   do
@@ -440,7 +442,7 @@ int uv_spawn(uv_loop_t* loop,
   else
     abort();
 
-  close(signal_pipe[0]);
+  uv__close(signal_pipe[0]);
 
   for (i = 0; i < options->stdio_count; i++) {
     err = uv__process_open_stream(options->stdio + i, pipes[i], i == 0);
@@ -465,8 +467,8 @@ int uv_spawn(uv_loop_t* loop,
 
 error:
   for (i = 0; i < stdio_count; i++) {
-    close(pipes[i][0]);
-    close(pipes[i][1]);
+    uv__close(pipes[i][0]);
+    uv__close(pipes[i][1]);
   }
   free(pipes);
 
diff --git a/src/unix/signal.c b/src/unix/signal.c
@@ -240,12 +240,12 @@ void uv__signal_loop_cleanup(uv_loop_t* loop) {
   }
 
   if (loop->signal_pipefd[0] != -1) {
-    close(loop->signal_pipefd[0]);
+    uv__close(loop->signal_pipefd[0]);
     loop->signal_pipefd[0] = -1;
   }
 
   if (loop->signal_pipefd[1] != -1) {
-    close(loop->signal_pipefd[1]);
+    uv__close(loop->signal_pipefd[1]);
     loop->signal_pipefd[1] = -1;
   }
 }
diff --git a/src/unix/stream.c b/src/unix/stream.c
@@ -85,7 +85,7 @@ static int uv__open_cloexec(const char* path, int flags) {
 
   err = uv__cloexec(fd, 1);
   if (err) {
-    close(fd);
+    uv__close(fd);
     return err;
   }
 
@@ -309,7 +309,7 @@ int uv__stream_try_select(uv_stream_t* stream, int* fd) {
   timeout.tv_nsec = 1;
 
   ret = kevent(kq, filter, 1, events, 1, &timeout);
-  SAVE_ERRNO(close(kq));
+  uv__close(kq);
 
   if (ret == -1)
     return -errno;
@@ -357,8 +357,8 @@ int uv__stream_try_select(uv_stream_t* stream, int* fd) {
   return 0;
 
 fatal4:
-  close(s->fake_fd);
-  close(s->int_fd);
+  uv__close(s->fake_fd);
+  uv__close(s->int_fd);
   s->fake_fd = -1;
   s->int_fd = -1;
 fatal3:
@@ -467,13 +467,13 @@ static int uv__emfile_trick(uv_loop_t* loop, int accept_fd) {
   if (loop->emfile_fd == -1)
     return -EMFILE;
 
-  close(loop->emfile_fd);
+  uv__close(loop->emfile_fd);
 
   for (;;) {
     fd = uv__accept(accept_fd);
 
     if (fd != -1) {
-      close(fd);
+      uv__close(fd);
       continue;
     }
 
@@ -572,7 +572,7 @@ int uv_accept(uv_stream_t* server, uv_stream_t* client) {
                             UV_STREAM_READABLE | UV_STREAM_WRITABLE);
       if (err) {
         /* TODO handle error */
-        close(server->accepted_fd);
+        uv__close(server->accepted_fd);
         server->accepted_fd = -1;
         return err;
       }
@@ -581,7 +581,7 @@ int uv_accept(uv_stream_t* server, uv_stream_t* client) {
     case UV_UDP:
       err = uv_udp_open((uv_udp_t*) client, server->accepted_fd);
       if (err) {
-        close(server->accepted_fd);
+        uv__close(server->accepted_fd);
         server->accepted_fd = -1;
         return err;
       }
@@ -1424,8 +1424,8 @@ void uv__stream_close(uv_stream_t* handle) {
     uv_thread_join(&s->thread);
     uv_sem_destroy(&s->close_sem);
     uv_sem_destroy(&s->async_sem);
-    close(s->fake_fd);
-    close(s->int_fd);
+    uv__close(s->fake_fd);
+    uv__close(s->int_fd);
     uv_close((uv_handle_t*) &s->async, uv__stream_osx_cb_close);
 
     handle->select = NULL;
@@ -1436,11 +1436,15 @@ void uv__stream_close(uv_stream_t* handle) {
   uv_read_stop(handle);
   uv__handle_stop(handle);
 
-  close(handle->io_watcher.fd);
-  handle->io_watcher.fd = -1;
+  if (handle->io_watcher.fd != -1) {
+    /* Don't close stdio file descriptors.  Nothing good comes from it. */
+    if (handle->io_watcher.fd > STDERR_FILENO)
+      uv__close(handle->io_watcher.fd);
+    handle->io_watcher.fd = -1;
+  }
 
-  if (handle->accepted_fd >= 0) {
-    close(handle->accepted_fd);
+  if (handle->accepted_fd != -1) {
+    uv__close(handle->accepted_fd);
     handle->accepted_fd = -1;
   }
 
diff --git a/src/unix/sunos.c b/src/unix/sunos.c
diff --git a/src/unix/tcp.c b/src/unix/tcp.c
diff --git a/src/unix/udp.c b/src/unix/udp.c
diff --git a/test/test-stdio-over-pipes.c b/test/test-stdio-over-pipes.c

Original file line number	Diff line number	Diff line change
`@@ -237,11 +237,11 @@ void uv__async_stop(uv_loop_t* loop, struct uv__async* wa) {`
`237`	`237`	`return;`
`238`	`238`
`239`	`239`	`uv__io_stop(loop, &wa->io_watcher, UV__POLLIN);`
`240`		`- close(wa->io_watcher.fd);`
	`240`	`+ uv__close(wa->io_watcher.fd);`
`241`	`241`	`wa->io_watcher.fd = -1;`
`242`	`242`
`243`	`243`	`if (wa->wfd != -1) {`
`244`		`- close(wa->wfd);`
	`244`	`+ uv__close(wa->wfd);`
`245`	`245`	`wa->wfd = -1;`
`246`	`246`	`}`
`247`	`247`	`}`
Original file line number	Diff line number	Diff line change
`@@ -354,6 +354,6 @@ void uv__fs_event_close(uv_fs_event_t* handle) {`
`354`	`354`	`free(handle->filename);`
`355`	`355`	`handle->filename = NULL;`
`356`	`356`
`357`		`- close(handle->event_watcher.fd);`
	`357`	`+ uv__close(handle->event_watcher.fd);`
`358`	`358`	`handle->event_watcher.fd = -1;`
`359`	`359`	`}`
Original file line number	Diff line number	Diff line change
`@@ -81,7 +81,7 @@ static int new_inotify_fd(void) {`
`81`	`81`	`err = uv__nonblock(fd, 1);`
`82`	`82`
`83`	`83`	`if (err) {`
`84`		`- close(fd);`
	`84`	`+ uv__close(fd);`
`85`	`85`	`return err;`
`86`	`86`	`}`
`87`	`87`
Original file line number	Diff line number	Diff line change
`@@ -136,12 +136,12 @@ static void uv__loop_delete(uv_loop_t* loop) {`
`136`	`136`	`uv__async_stop(loop, &loop->async_watcher);`
`137`	`137`
`138`	`138`	`if (loop->emfile_fd != -1) {`
`139`		`- close(loop->emfile_fd);`
	`139`	`+ uv__close(loop->emfile_fd);`
`140`	`140`	`loop->emfile_fd = -1;`
`141`	`141`	`}`
`142`	`142`
`143`	`143`	`if (loop->backend_fd != -1) {`
`144`		`- close(loop->backend_fd);`
	`144`	`+ uv__close(loop->backend_fd);`
`145`	`145`	`loop->backend_fd = -1;`
`146`	`146`	`}`
`147`	`147`
Original file line number	Diff line number	Diff line change
`@@ -93,11 +93,11 @@ int uv_pipe_bind(uv_pipe_t* handle, const char* name) {`
`93`	`93`
`94`	`94`	`out:`
`95`	`95`	`if (bound) {`
`96`		`- /* unlink() before close() to avoid races. */`
	`96`	`+ /* unlink() before uv__close() to avoid races. */`
`97`	`97`	`assert(pipe_fname != NULL);`
`98`	`98`	`unlink(pipe_fname);`
`99`	`99`	`}`
`100`		`- close(sockfd);`
	`100`	`+ uv__close(sockfd);`
`101`	`101`	`free((void*)pipe_fname);`
`102`	`102`	`return err;`
`103`	`103`	`}`
Original file line number	Diff line number	Diff line change
`@@ -240,12 +240,12 @@ void uv__signal_loop_cleanup(uv_loop_t* loop) {`
`240`	`240`	`}`
`241`	`241`
`242`	`242`	`if (loop->signal_pipefd[0] != -1) {`
`243`		`- close(loop->signal_pipefd[0]);`
	`243`	`+ uv__close(loop->signal_pipefd[0]);`
`244`	`244`	`loop->signal_pipefd[0] = -1;`
`245`	`245`	`}`
`246`	`246`
`247`	`247`	`if (loop->signal_pipefd[1] != -1) {`
`248`		`- close(loop->signal_pipefd[1]);`
	`248`	`+ uv__close(loop->signal_pipefd[1]);`
`249`	`249`	`loop->signal_pipefd[1] = -1;`
`250`	`250`	`}`
`251`	`251`	`}`