Improve file descriptor checks for posix_spawn actions [BZ #19505]

8 files changed, 249 insertions, 12 deletions

diff --git a/ChangeLog b/ChangeLog
index bc7c5d374b..8d5406870b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,22 @@
 2016-02-19  Florian Weimer  <fweimer@redhat.com>
 
+	[BZ #19505]
+	* posix/spawn_int.h: Add headers and include guard.
+	(__spawn_valid_fd): New function.
+	* posix/spawn_faction_addopen.c
+	(posix_spawn_file_actions_addopen): Use __spawn_valid_fd.
+	* posix/spawn_faction_addclose.c
+	(posix_spawn_file_actions_addclose): Likewise.
+	* posix/spawn_faction_adddup2.c
+	(posix_spawn_file_actions_adddup2): Likewise.  Add check for
+	second file descriptor.
+	* posix/spawn_valid_fd.c: New file.
+	* posix/tst-posix_spawn-fd.c: New file.
+	* posix/Makefile (routines): Add spawn_valid_fd.
+	(tests): Add tst-posix_spawn-fd.
+
+2016-02-19  Florian Weimer  <fweimer@redhat.com>
+
 	* malloc/tst-malloc-thread-exit.c: Include test-skeleton.c early.
 	(do_test): Limit the number of arenas, so that we can use fewer
 	outer threads.  Limit timeout to 3 seconds, in preparation for a
diff --git a/posix/Makefile b/posix/Makefile
index f94e023c60..4e90a9540a 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -51,7 +51,7 @@ routines :=								      \
 	getaddrinfo gai_strerror wordexp				      \
 	pread pwrite pread64 pwrite64					      \
 	spawn_faction_init spawn_faction_destroy spawn_faction_addclose	      \
-	spawn_faction_addopen spawn_faction_adddup2			      \
+	spawn_faction_addopen spawn_faction_adddup2 spawn_valid_fd	      \
 	spawnattr_init spawnattr_destroy				      \
 	spawnattr_getdefault spawnattr_setdefault			      \
 	spawnattr_getflags spawnattr_setflags				      \
@@ -87,7 +87,8 @@ tests		:= tstgetopt testfnm runtests runptests	     \
 		   bug-getopt1 bug-getopt2 bug-getopt3 bug-getopt4 \
 		   bug-getopt5 tst-getopt_long1 bug-regex34 bug-regex35 \
 		   tst-pathconf tst-getaddrinfo4 tst-rxspencer-no-utf8 \
-		   tst-fnmatch3 bug-regex36 tst-getaddrinfo5
+		   tst-fnmatch3 bug-regex36 tst-getaddrinfo5 \
+		   tst-posix_spawn-fd
 xtests		:= bug-ga2
 ifeq (yes,$(build-shared))
 test-srcs	:= globtest
diff --git a/posix/spawn_faction_addclose.c b/posix/spawn_faction_addclose.c
index 1a4a2f2457..946454e646 100644
--- a/posix/spawn_faction_addclose.c
+++ b/posix/spawn_faction_addclose.c
@@ -27,11 +27,9 @@ int
 posix_spawn_file_actions_addclose (posix_spawn_file_actions_t *file_actions,
 				   int fd)
 {
-  int maxfd = __sysconf (_SC_OPEN_MAX);
   struct __spawn_action *rec;
 
-  /* Test for the validity of the file descriptor.  */
-  if (fd < 0 || fd >= maxfd)
+  if (!__spawn_valid_fd (fd))
     return EBADF;
 
   /* Allocate more memory if needed.  */
diff --git a/posix/spawn_faction_adddup2.c b/posix/spawn_faction_adddup2.c
index 8beee1099b..a04fc521c3 100644
--- a/posix/spawn_faction_adddup2.c
+++ b/posix/spawn_faction_adddup2.c
@@ -27,11 +27,9 @@ int
 posix_spawn_file_actions_adddup2 (posix_spawn_file_actions_t *file_actions,
 				  int fd, int newfd)
 {
-  int maxfd = __sysconf (_SC_OPEN_MAX);
   struct __spawn_action *rec;
 
-  /* Test for the validity of the file descriptor.  */
-  if (fd < 0 || newfd < 0 || fd >= maxfd || newfd >= maxfd)
+  if (!__spawn_valid_fd (fd) || !__spawn_valid_fd (newfd))
     return EBADF;
 
   /* Allocate more memory if needed.  */
diff --git a/posix/spawn_faction_addopen.c b/posix/spawn_faction_addopen.c
index 36cde06fb2..4f37d0b847 100644
--- a/posix/spawn_faction_addopen.c
+++ b/posix/spawn_faction_addopen.c
@@ -16,7 +16,6 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <errno.h>
-#include <spawn.h>
 #include <unistd.h>
 #include <stdlib.h>
 #include <string.h>
@@ -30,11 +29,9 @@ posix_spawn_file_actions_addopen (posix_spawn_file_actions_t *file_actions,
 				  int fd, const char *path, int oflag,
 				  mode_t mode)
 {
-  int maxfd = __sysconf (_SC_OPEN_MAX);
   struct __spawn_action *rec;
 
-  /* Test for the validity of the file descriptor.  */
-  if (fd < 0 || fd >= maxfd)
+  if (!__spawn_valid_fd (fd))
     return EBADF;
 
   char *path_copy = strdup (path);
diff --git a/posix/spawn_int.h b/posix/spawn_int.h
index 861e3b47bb..b7da3b8abe 100644
--- a/posix/spawn_int.h
+++ b/posix/spawn_int.h
@@ -1,3 +1,27 @@
+/* Internal definitions for posix_spawn functionality.
+   Copyright (C) 2000-2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _SPAWN_INT_H
+#define _SPAWN_INT_H
+
+#include <spawn.h>
+#include <stdbool.h>
+
 /* Data structure to contain the action information.  */
 struct __spawn_action
 {
@@ -39,3 +63,9 @@ extern int __spawni (pid_t *pid, const char *path,
 		     const posix_spawn_file_actions_t *file_actions,
 		     const posix_spawnattr_t *attrp, char *const argv[],
 		     char *const envp[], int xflags);
+
+/* Return true if FD falls into the range valid for file descriptors.
+   The check in this form is mandated by POSIX.  */
+bool __spawn_valid_fd (int fd) internal_function attribute_hidden;
+
+#endif /* _SPAWN_INT_H */
diff --git a/posix/spawn_valid_fd.c b/posix/spawn_valid_fd.c
new file mode 100644
index 0000000000..c199750557
--- /dev/null
+++ b/posix/spawn_valid_fd.c
@@ -0,0 +1,31 @@
+/* File descriptor validity check for posix_spawn file actions.
+   Copyright (C) 2000-2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include "spawn_int.h"
+
+#include <unistd.h>
+
+bool
+internal_function
+__spawn_valid_fd (int fd)
+{
+  long maxfd = __sysconf (_SC_OPEN_MAX);
+  return __glibc_likely (fd >= 0)
+    && (__glibc_unlikely (maxfd < 0) /* No limit set.  */
+	|| __glibc_likely (fd < maxfd));
+}
diff --git a/posix/tst-posix_spawn-fd.c b/posix/tst-posix_spawn-fd.c
new file mode 100644
index 0000000000..cde5bdccd5
--- /dev/null
+++ b/posix/tst-posix_spawn-fd.c
@@ -0,0 +1,165 @@
+/* Test that spawn file action functions work without file limit.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <spawn.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <sys/resource.h>
+#include <unistd.h>
+
+/* _SC_OPEN_MAX value.  */
+static long maxfd;
+
+/* A positive but unused file descriptor, used for testing
+   purposes.  */
+static int invalid_fd;
+
+/* Indicate that errors have been encountered.  */
+static bool errors;
+
+static posix_spawn_file_actions_t actions;
+
+static void
+one_test (const char *name, int (*func) (int), int fd,
+          bool expect_success)
+{
+  int ret = func (fd);
+  if (expect_success)
+    {
+      if (ret != 0)
+        {
+          errno = ret;
+          printf ("error: posix_spawn_file_actions_%s (%d): %m\n", name, fd);
+          errors = true;
+        }
+    }
+  else if (ret != EBADF)
+    {
+      if (ret == 0)
+          printf ("error: posix_spawn_file_actions_%s (%d):"
+                  " unexpected success\n", name, fd);
+      else
+        {
+          errno = ret;
+          printf ("error: posix_spawn_file_actions_%s (%d): %m\n", name, fd);
+        }
+      errors = true;
+    }
+}
+
+static void
+all_tests (const char *name, int (*func) (int))
+{
+  one_test (name, func, 0, true);
+  one_test (name, func, invalid_fd, true);
+  one_test (name, func, -1, false);
+  one_test (name, func, -2, false);
+  if (maxfd >= 0)
+    one_test (name, func, maxfd, false);
+}
+
+static int
+addopen (int fd)
+{
+  return posix_spawn_file_actions_addopen
+    (&actions, fd, "/dev/null", O_RDONLY, 0);
+}
+
+static int
+adddup2 (int fd)
+{
+  return posix_spawn_file_actions_adddup2 (&actions, fd, 1);
+}
+
+static int
+adddup2_reverse (int fd)
+{
+  return posix_spawn_file_actions_adddup2 (&actions, 1, fd);
+}
+
+static int
+addclose (int fd)
+{
+  return posix_spawn_file_actions_addclose (&actions, fd);
+}
+
+static void
+all_functions (void)
+{
+  all_tests ("addopen", addopen);
+  all_tests ("adddup2", adddup2);
+  all_tests ("adddup2", adddup2_reverse);
+  all_tests ("adddup2", addclose);
+}
+
+static int
+do_test (void)
+{
+  /* Try to eliminate the file descriptor limit.  */
+  {
+    struct rlimit limit;
+    if (getrlimit (RLIMIT_NOFILE, &limit) < 0)
+      {
+        printf ("error: getrlimit: %m\n");
+        return 1;
+      }
+    limit.rlim_cur = RLIM_INFINITY;
+    if (setrlimit (RLIMIT_NOFILE, &limit) < 0)
+      printf ("warning: setrlimit: %m\n");
+  }
+
+  maxfd = sysconf (_SC_OPEN_MAX);
+  printf ("info: _SC_OPEN_MAX: %ld\n", maxfd);
+
+  invalid_fd = dup (0);
+  if (invalid_fd < 0)
+    {
+      printf ("error: dup: %m\n");
+      return 1;
+    }
+  if (close (invalid_fd) < 0)
+    {
+      printf ("error: close: %m\n");
+      return 1;
+    }
+
+  int ret = posix_spawn_file_actions_init (&actions);
+  if (ret != 0)
+    {
+      errno = ret;
+      printf ("error: posix_spawn_file_actions_init: %m\n");
+      return 1;
+    }
+
+  all_functions ();
+
+  ret = posix_spawn_file_actions_destroy (&actions);
+  if (ret != 0)
+    {
+      errno = ret;
+      printf ("error: posix_spawn_file_actions_destroy: %m\n");
+      return 1;
+    }
+
+  return errors;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"