| Message ID | 20230901122651.59253-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] linux: Make fdopendir fail with O_PATH (BZ 30373) | expand |
On 2023-09-01 05:26, Adhemerval Zanella wrote: > + /* Fail early for descriptors opened with O_PATH. */ > + if (__glibc_unlikely (flags & O_PATH)) > + { > + __set_errno (EBADF); > + return NULL; > + } > + /* Make sure the descriptor allows for reading. */ > if (__glibc_unlikely ((flags & O_ACCMODE) == O_WRONLY)) > { > __set_errno (EINVAL); Why use EBADF for one situation but EINVAL in the other? POSIX says it should be EBADF for both situations. Arguably this is a separate bug, but we should fix both bugs (and fix the documentation too, to match POSIX).
On 01/09/23 14:57, Paul Eggert wrote: > On 2023-09-01 05:26, Adhemerval Zanella wrote: >> + /* Fail early for descriptors opened with O_PATH. */ >> + if (__glibc_unlikely (flags & O_PATH)) >> + { >> + __set_errno (EBADF); >> + return NULL; >> + } >> + /* Make sure the descriptor allows for reading. */ >> if (__glibc_unlikely ((flags & O_ACCMODE) == O_WRONLY)) >> { >> __set_errno (EINVAL); > > Why use EBADF for one situation but EINVAL in the other? > > POSIX says it should be EBADF for both situations. Arguably this is a separate bug, but we should fix both bugs (and fix the documentation too, to match POSIX). Right, but I think it should a different patch. I can send the fix once this get installed.
On 01/09/23 09:26, Adhemerval Zanella wrote: > It is not strictly required by the POSIX, since O_PATH is a Linux > extension, but it is QoI to fail early instead of at readdir. Also > the check is free, since fdopendir already checks if the file > descriptor is opened for read. > > Checked on x86_64-linux-gnu. Ping. > --- > sysdeps/unix/sysv/linux/Makefile | 1 + > sysdeps/unix/sysv/linux/fdopendir.c | 8 +++- > .../unix/sysv/linux/tst-fdopendir-o_path.c | 48 +++++++++++++++++++ > 3 files changed, 56 insertions(+), 1 deletion(-) > create mode 100644 sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c > > diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile > index be801e3be4..ec0ba3eb05 100644 > --- a/sysdeps/unix/sysv/linux/Makefile > +++ b/sysdeps/unix/sysv/linux/Makefile > @@ -199,6 +199,7 @@ tests += \ > tst-clone3 \ > tst-epoll \ > tst-fanotify \ > + tst-fdopendir-o_path \ > tst-getauxval \ > tst-gettid \ > tst-gettid-kill \ > diff --git a/sysdeps/unix/sysv/linux/fdopendir.c b/sysdeps/unix/sysv/linux/fdopendir.c > index 861345396c..707fb5960e 100644 > --- a/sysdeps/unix/sysv/linux/fdopendir.c > +++ b/sysdeps/unix/sysv/linux/fdopendir.c > @@ -37,10 +37,16 @@ __fdopendir (int fd) > return NULL; > } > > - /* Make sure the descriptor allows for reading. */ > int flags = __fcntl64_nocancel (fd, F_GETFL); > if (__glibc_unlikely (flags == -1)) > return NULL; > + /* Fail early for descriptors opened with O_PATH. */ > + if (__glibc_unlikely (flags & O_PATH)) > + { > + __set_errno (EBADF); > + return NULL; > + } > + /* Make sure the descriptor allows for reading. */ > if (__glibc_unlikely ((flags & O_ACCMODE) == O_WRONLY)) > { > __set_errno (EINVAL); > diff --git a/sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c b/sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c > new file mode 100644 > index 0000000000..2531cf8ddb > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c > @@ -0,0 +1,48 @@ > +/* Check if fdopendir fails with file descriptor opened with O_PATH (BZ 30737) > + Copyright (C) 2023 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 > + <https://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <fcntl.h> > +#include <dirent.h> > +#include <support/check.h> > +#include <support/temp_file.h> > +#include <support/xunistd.h> > + > +static int > +do_test (void) > +{ > + char *dirname = support_create_temp_directory ("tst-fdopendir-o_path"); > + > + { > + int fd = xopen (dirname, O_RDONLY | O_DIRECTORY, 0600); > + DIR *dir = fdopendir (fd); > + TEST_VERIFY_EXIT (dir != NULL); > + closedir (dir); > + } > + > + { > + int fd = xopen (dirname, O_RDONLY | O_PATH | O_DIRECTORY, 0600); > + TEST_VERIFY (fdopendir (fd) == NULL); > + TEST_COMPARE (errno, EBADF); > + xclose (fd); > + } > + > + return 0; > +} > + > +#include <support/test-driver.c>
On 02/10/23 08:09, Adhemerval Zanella Netto wrote: > > > On 01/09/23 09:26, Adhemerval Zanella wrote: >> It is not strictly required by the POSIX, since O_PATH is a Linux >> extension, but it is QoI to fail early instead of at readdir. Also >> the check is free, since fdopendir already checks if the file >> descriptor is opened for read. >> >> Checked on x86_64-linux-gnu. > > Ping. Ping (x2). > >> --- >> sysdeps/unix/sysv/linux/Makefile | 1 + >> sysdeps/unix/sysv/linux/fdopendir.c | 8 +++- >> .../unix/sysv/linux/tst-fdopendir-o_path.c | 48 +++++++++++++++++++ >> 3 files changed, 56 insertions(+), 1 deletion(-) >> create mode 100644 sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c >> >> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile >> index be801e3be4..ec0ba3eb05 100644 >> --- a/sysdeps/unix/sysv/linux/Makefile >> +++ b/sysdeps/unix/sysv/linux/Makefile >> @@ -199,6 +199,7 @@ tests += \ >> tst-clone3 \ >> tst-epoll \ >> tst-fanotify \ >> + tst-fdopendir-o_path \ >> tst-getauxval \ >> tst-gettid \ >> tst-gettid-kill \ >> diff --git a/sysdeps/unix/sysv/linux/fdopendir.c b/sysdeps/unix/sysv/linux/fdopendir.c >> index 861345396c..707fb5960e 100644 >> --- a/sysdeps/unix/sysv/linux/fdopendir.c >> +++ b/sysdeps/unix/sysv/linux/fdopendir.c >> @@ -37,10 +37,16 @@ __fdopendir (int fd) >> return NULL; >> } >> >> - /* Make sure the descriptor allows for reading. */ >> int flags = __fcntl64_nocancel (fd, F_GETFL); >> if (__glibc_unlikely (flags == -1)) >> return NULL; >> + /* Fail early for descriptors opened with O_PATH. */ >> + if (__glibc_unlikely (flags & O_PATH)) >> + { >> + __set_errno (EBADF); >> + return NULL; >> + } >> + /* Make sure the descriptor allows for reading. */ >> if (__glibc_unlikely ((flags & O_ACCMODE) == O_WRONLY)) >> { >> __set_errno (EINVAL); >> diff --git a/sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c b/sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c >> new file mode 100644 >> index 0000000000..2531cf8ddb >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c >> @@ -0,0 +1,48 @@ >> +/* Check if fdopendir fails with file descriptor opened with O_PATH (BZ 30737) >> + Copyright (C) 2023 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 >> + <https://www.gnu.org/licenses/>. */ >> + >> +#include <errno.h> >> +#include <fcntl.h> >> +#include <dirent.h> >> +#include <support/check.h> >> +#include <support/temp_file.h> >> +#include <support/xunistd.h> >> + >> +static int >> +do_test (void) >> +{ >> + char *dirname = support_create_temp_directory ("tst-fdopendir-o_path"); >> + >> + { >> + int fd = xopen (dirname, O_RDONLY | O_DIRECTORY, 0600); >> + DIR *dir = fdopendir (fd); >> + TEST_VERIFY_EXIT (dir != NULL); >> + closedir (dir); >> + } >> + >> + { >> + int fd = xopen (dirname, O_RDONLY | O_PATH | O_DIRECTORY, 0600); >> + TEST_VERIFY (fdopendir (fd) == NULL); >> + TEST_COMPARE (errno, EBADF); >> + xclose (fd); >> + } >> + >> + return 0; >> +} >> + >> +#include <support/test-driver.c>
* Adhemerval Zanella via Libc-alpha: > diff --git a/sysdeps/unix/sysv/linux/fdopendir.c b/sysdeps/unix/sysv/linux/fdopendir.c > index 861345396c..707fb5960e 100644 > --- a/sysdeps/unix/sysv/linux/fdopendir.c > +++ b/sysdeps/unix/sysv/linux/fdopendir.c > @@ -37,10 +37,16 @@ __fdopendir (int fd) > return NULL; > } > > - /* Make sure the descriptor allows for reading. */ > int flags = __fcntl64_nocancel (fd, F_GETFL); > if (__glibc_unlikely (flags == -1)) > return NULL; > + /* Fail early for descriptors opened with O_PATH. */ > + if (__glibc_unlikely (flags & O_PATH)) > + { > + __set_errno (EBADF); > + return NULL; > + } Should we fail with EINVAL instead of EBADF? Maybe add a comment that EBADF matches what readdir will fail with? Otherwise: Reviewed-by: Florian Weimer <fweimer@redhat.com> Thanks, Florian
On 30/11/23 06:54, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> diff --git a/sysdeps/unix/sysv/linux/fdopendir.c b/sysdeps/unix/sysv/linux/fdopendir.c >> index 861345396c..707fb5960e 100644 >> --- a/sysdeps/unix/sysv/linux/fdopendir.c >> +++ b/sysdeps/unix/sysv/linux/fdopendir.c >> @@ -37,10 +37,16 @@ __fdopendir (int fd) >> return NULL; >> } >> >> - /* Make sure the descriptor allows for reading. */ >> int flags = __fcntl64_nocancel (fd, F_GETFL); >> if (__glibc_unlikely (flags == -1)) >> return NULL; >> + /* Fail early for descriptors opened with O_PATH. */ >> + if (__glibc_unlikely (flags & O_PATH)) >> + { >> + __set_errno (EBADF); >> + return NULL; >> + } > > Should we fail with EINVAL instead of EBADF? POSIX states it should be EBADF indeed, as Paul has noted [1]. The EINVAL returns is a different issue. > > Maybe add a comment that EBADF matches what readdir will fail with? > > Otherwise: > > Reviewed-by: Florian Weimer <fweimer@redhat.com> Thanks. > > Thanks, > Florian > [1] https://sourceware.org/pipermail/libc-alpha/2023-September/151302.html
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index be801e3be4..ec0ba3eb05 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -199,6 +199,7 @@ tests += \ tst-clone3 \ tst-epoll \ tst-fanotify \ + tst-fdopendir-o_path \ tst-getauxval \ tst-gettid \ tst-gettid-kill \ diff --git a/sysdeps/unix/sysv/linux/fdopendir.c b/sysdeps/unix/sysv/linux/fdopendir.c index 861345396c..707fb5960e 100644 --- a/sysdeps/unix/sysv/linux/fdopendir.c +++ b/sysdeps/unix/sysv/linux/fdopendir.c @@ -37,10 +37,16 @@ __fdopendir (int fd) return NULL; } - /* Make sure the descriptor allows for reading. */ int flags = __fcntl64_nocancel (fd, F_GETFL); if (__glibc_unlikely (flags == -1)) return NULL; + /* Fail early for descriptors opened with O_PATH. */ + if (__glibc_unlikely (flags & O_PATH)) + { + __set_errno (EBADF); + return NULL; + } + /* Make sure the descriptor allows for reading. */ if (__glibc_unlikely ((flags & O_ACCMODE) == O_WRONLY)) { __set_errno (EINVAL); diff --git a/sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c b/sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c new file mode 100644 index 0000000000..2531cf8ddb --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-fdopendir-o_path.c @@ -0,0 +1,48 @@ +/* Check if fdopendir fails with file descriptor opened with O_PATH (BZ 30737) + Copyright (C) 2023 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 + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <fcntl.h> +#include <dirent.h> +#include <support/check.h> +#include <support/temp_file.h> +#include <support/xunistd.h> + +static int +do_test (void) +{ + char *dirname = support_create_temp_directory ("tst-fdopendir-o_path"); + + { + int fd = xopen (dirname, O_RDONLY | O_DIRECTORY, 0600); + DIR *dir = fdopendir (fd); + TEST_VERIFY_EXIT (dir != NULL); + closedir (dir); + } + + { + int fd = xopen (dirname, O_RDONLY | O_PATH | O_DIRECTORY, 0600); + TEST_VERIFY (fdopendir (fd) == NULL); + TEST_COMPARE (errno, EBADF); + xclose (fd); + } + + return 0; +} + +#include <support/test-driver.c>