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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 09-02-2016 10:59, Florian Weimer wrote:
> The test passes on x86_64-redhat-linux-gnu, but I have not tested this
> on Hurd.
> 
> Thanks,
> Florian
> 

LGTM. THe only nit is I would prefer to name bug19505.c to something related
to what is testing, like posix_spawn-bug19505.c.

> 
> 0001-Improve-file-descriptor-checks-for-posix_spawn-actio.patch
> 
> 
> 2016-02-09  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/bug19505.c: New file.
> 	* posix/Makefile (routines): Add spawn_valid_fd.
> 	(tests): Add bug19505.
> 
> diff --git a/posix/Makefile b/posix/Makefile
> index f94e023..a110ad1 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,7 @@ 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 bug19505
>  xtests		:= bug-ga2
>  ifeq (yes,$(build-shared))
>  test-srcs	:= globtest
> diff --git a/posix/bug19505.c b/posix/bug19505.c
> new file mode 100644
> index 0000000..cde5bdc
> --- /dev/null
> +++ b/posix/bug19505.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"
> diff --git a/posix/spawn_faction_addclose.c b/posix/spawn_faction_addclose.c
> index 1a4a2f2..946454e 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 8beee10..a04fc52 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 36cde06..4f37d0b 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 861e3b4..b7da3b8 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 0000000..c199750
> --- /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));
> +}