Message ID | 56B9E2B0.7030206@redhat.com |
---|---|
State | New |
Headers | show |
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)); > +}
On 02/11/2016 02:14 PM, Adhemerval Zanella wrote: > > > 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. Thanks. I renamed the test to tst-posix_spawn-fd. Florian
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)); +}