Message ID | 1477320168-23397-3-git-send-email-siddhesh@sourceware.org |
---|---|
State | New |
Headers | show |
On 24/10/2016 12:42, Siddhesh Poyarekar wrote: > Implement an internal version of __access called __access_noerrno that > avoids setting errno. This is useful to check accessibility of files > very early on in process startup i.e. before TLS setup. This allows > tunables to replace MALLOC_CHECK_ safely (i.e. check existence of > /etc/suid-debug to enable/disable MALLOC_CHECK) and at the same time > initialize very early so that it can override IFUNCs. > > * include/unistd.h [IS_IN (rtld) || !defined SHARED]: Declare > __access_noerrno. > * io/Makefile (routines): Add access_noerrno. > * io/access.c (__ACCESS)[!__ACCESS]: Define as __access. > (__access): Rename to __ACCESS. > [!NOERRNO]: Retain default __access logic. > * io/access_noerrno.c: New file. > * sysdeps/mach/hurd/access.c (__ACCESS)[!__ACCESS]: Define as > __access. > (__HURD_FAIL): New macro. > (__access): Rename to __ACCESS. Use __HURD_FAIL instead of > __hurd_fail. > [!NOERRNO]: Set weak alias to access. > * sysdeps/nacl/access.c (__ACCESS)[!__ACCESS]: Define as > __access. > (DO_NACL_CALL): New macro. > (__access): Rename to __ACCESS. Use DO_NACL_CALL instead of > NACL_CALL. > [!NOERRNO]: Set weak alias to access. > * sysdeps/nacl/nacl-interfaces.h (NACL_CALL_NOERRNO): New > macro. > * sysdeps/unix/access_noerrno.c: New file. > * sysdeps/unix/sysv/linux/generic/access.c: Include sysdep.h. > (__ACCESS)[!__ACCESS]: Define as __access. > (__access): Rename to __ACCESS. > [NOERRNO]: Call faccessat syscall without setting errno. > --- > include/unistd.h | 6 +++++ > io/Makefile | 1 + > io/access.c | 10 ++++++++- > io/access_noerrno.c | 21 ++++++++++++++++++ > sysdeps/mach/hurd/access.c | 20 +++++++++++++---- > sysdeps/nacl/access.c | 16 ++++++++++++-- > sysdeps/nacl/nacl-interfaces.h | 4 ++++ > sysdeps/unix/access_noerrno.c | 38 ++++++++++++++++++++++++++++++++ > sysdeps/unix/sysv/linux/generic/access.c | 19 +++++++++++++++- > 9 files changed, 127 insertions(+), 8 deletions(-) > create mode 100644 io/access_noerrno.c > create mode 100644 sysdeps/unix/access_noerrno.c > > diff --git a/include/unistd.h b/include/unistd.h > index d2802b2..6144f41 100644 > --- a/include/unistd.h > +++ b/include/unistd.h > @@ -181,6 +181,12 @@ extern int __getlogin_r_loginuid (char *name, size_t namesize) > # include <dl-unistd.h> > # endif > > +# if IS_IN (rtld) || !defined SHARED > +/* __access variant that does not set errno. Used in very early initialization > + code in libc.a and ld.so. */ Is this comment correct? Checking the patch I am seeing it builds only for libc and there is no resulting __access_noerrno on ld.so. > diff --git a/sysdeps/unix/access_noerrno.c b/sysdeps/unix/access_noerrno.c > new file mode 100644 > index 0000000..1ff90a2 > --- /dev/null > +++ b/sysdeps/unix/access_noerrno.c > @@ -0,0 +1,38 @@ > +/* Test for access to a file but do not set errno on error. > + Copyright (C) 2016 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011. > + > + 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 <stddef.h> > +#include <unistd.h> > +#include <fcntl.h> > +#include <sysdep-cancel.h> > +#include <sysdep.h> > + > +/* Test for access to FILE. */ > +int > +__access_noerrno (const char *file, int type) > +{ > + INTERNAL_SYSCALL_DECL (err); > + int res; > + res = INTERNAL_SYSCALL (access, err, 2, file, type); > + if (INTERNAL_SYSCALL_ERROR_P (res, err)) > + return INTERNAL_SYSCALL_ERRNO (res, err); > + else > + return 0; > +} I think it would be simpler to just 1. consolidation Linux access implementation and 2. add the '_noerrno' on same file. The 1. would be simpler to just: 1.1. Remove access from sysdeps/unix/syscalls.list 1.2. Move sysdeps/unix/sysv/linux/generic/access.c to sysdeps/unix/sysv/linux/access.c 1.3. And implement access checking for __NR_access and using __NR_facessat if is not defined. Something like: [...] /* Test for access to FILE. */ int __access (const char *file, int type) { #if __NR_access return INLINE_SYSCALL_CALL (access, file, type); #else return INLINE_SYSCALL_CALL (faccessat, AT_FDCWD, file, type); #endif } [...] Then __access_noerro could be just implemented on same file. I think it has the advantage of not splitting the access on multiple files. I think for hurd and nacl it would also result in simplify code. What do you think? > diff --git a/sysdeps/unix/sysv/linux/generic/access.c b/sysdeps/unix/sysv/linux/generic/access.c > index 586aa93..5bafc06 100644 > --- a/sysdeps/unix/sysv/linux/generic/access.c > +++ b/sysdeps/unix/sysv/linux/generic/access.c > @@ -21,11 +21,28 @@ > #include <unistd.h> > #include <fcntl.h> > #include <sysdep-cancel.h> > +#include <sysdep.h> > + > +#ifndef __ACCESS > +# define __ACCESS __access > +#endif > > /* Test for access to FILE. */ > int > -__access (const char *file, int type) > +__ACCESS (const char *file, int type) > { > +#ifndef NOERRNO > return INLINE_SYSCALL (faccessat, 3, AT_FDCWD, file, type); > +#else > + INTERNAL_SYSCALL_DECL (err); > + int res; > + res = INTERNAL_SYSCALL (faccessat, err, 3, AT_FDCWD, file, type); > + if (INTERNAL_SYSCALL_ERROR_P (res, err)) > + return INTERNAL_SYSCALL_ERRNO (res, err); > + else > + return 0; > +#endif > } > +#ifndef NOERRNO > weak_alias (__access, access) > +#endif >
On Tuesday 08 November 2016 10:39 PM, Adhemerval Zanella wrote: > Is this comment correct? Checking the patch I am seeing it builds > only for libc and there is no resulting __access_noerrno on ld.so. It builds for both. rtld-Rules has a rule that looks at all of the libc functions that rtld uses and rebuilds all of them inside ld.so. $ nm elf/ld.so | grep access_noerrno 00000000000199d0 t __access_noerrno > I think it would be simpler to just 1. consolidation Linux access > implementation and 2. add the '_noerrno' on same file. > > The 1. would be simpler to just: > > 1.1. Remove access from sysdeps/unix/syscalls.list > 1.2. Move sysdeps/unix/sysv/linux/generic/access.c to > sysdeps/unix/sysv/linux/access.c > 1.3. And implement access checking for __NR_access and > using __NR_facessat if is not defined. Something > like: > > [...] > /* Test for access to FILE. */ > int > __access (const char *file, int type) > { > #if __NR_access > return INLINE_SYSCALL_CALL (access, file, type); > #else > return INLINE_SYSCALL_CALL (faccessat, AT_FDCWD, file, type); > #endif > } > [...] > > Then __access_noerro could be just implemented on same file. > I think it has the advantage of not splitting the access > on multiple files. > > I think for hurd and nacl it would also result in simplify > code. What do you think? The nacl code can be hacked in there somehow I guess, but the hurd code seems quite different and I am a bit reluctant to hack at it without access to a hurd instance to test on. I can hack at sysdeps/unix/sysv/linux/access.c and drop sysdeps/unix/access_noerrno.c. Siddhesh
On Wednesday 09 November 2016 12:30 AM, Siddhesh Poyarekar wrote: > I can hack at sysdeps/unix/sysv/linux/access.c and drop > sysdeps/unix/access_noerrno.c. Turns out I can't do this because I will be breaking non-Linux configurations that use the SYS_ prefix instead of the __NR_ prefix. I also discovered that I hadn't actually tested the code properly on aarch64 and it is broken, so I'll resend the patchset. Siddhesh
On 10/11/2016 03:44, Siddhesh Poyarekar wrote: > On Wednesday 09 November 2016 12:30 AM, Siddhesh Poyarekar wrote: >> I can hack at sysdeps/unix/sysv/linux/access.c and drop >> sysdeps/unix/access_noerrno.c. > > Turns out I can't do this because I will be breaking non-Linux > configurations that use the SYS_ prefix instead of the __NR_ prefix. I > also discovered that I hadn't actually tested the code properly on > aarch64 and it is broken, so I'll resend the patchset. > > Siddhesh In this branch [1] I have a WIP for this. First patch basically consolidates access Linux implementation and it uses the strategy to check for __NR_access first (and since it on sysdeps/.../linux it should not worry about __SYS_*). Second one is the __access_errno implementation. It basically handles the 3 cases (hurd, nacl, and Linux) by adding the __access_errno on access.c. I think it should a cleaner solution to __access_noerro (less macro handling and build objects). [1] https://github.com/zatrazz/glibc/commits/master-access_noerrno
On Thursday 10 November 2016 05:44 PM, Adhemerval Zanella wrote: > In this branch [1] I have a WIP for this. First patch basically > consolidates access Linux implementation and it uses the strategy > to check for __NR_access first (and since it on sysdeps/.../linux > it should not worry about __SYS_*). Ahh, you meant sysdeps/unix/sysv/linux, I thought you meant sysdeps/unix. Yes, that makes sense. Can you please post that on the list for review? > Second one is the __access_errno implementation. It basically handles > the 3 cases (hurd, nacl, and Linux) by adding the __access_errno on > access.c. > > I think it should a cleaner solution to __access_noerro (less macro > handling and build objects). Sure, I don't mind this kind of implementation either. Do you want to post this one for review too? Thanks, Siddhesh
On 10/11/2016 11:05, Siddhesh Poyarekar wrote: > On Thursday 10 November 2016 05:44 PM, Adhemerval Zanella wrote: >> In this branch [1] I have a WIP for this. First patch basically >> consolidates access Linux implementation and it uses the strategy >> to check for __NR_access first (and since it on sysdeps/.../linux >> it should not worry about __SYS_*). > > Ahh, you meant sysdeps/unix/sysv/linux, I thought you meant > sysdeps/unix. Yes, that makes sense. Can you please post that on the > list for review? > >> Second one is the __access_errno implementation. It basically handles >> the 3 cases (hurd, nacl, and Linux) by adding the __access_errno on >> access.c. >> >> I think it should a cleaner solution to __access_noerro (less macro >> handling and build objects). > > Sure, I don't mind this kind of implementation either. Do you want to > post this one for review too? I will prepare a patch for this.
diff --git a/include/unistd.h b/include/unistd.h index d2802b2..6144f41 100644 --- a/include/unistd.h +++ b/include/unistd.h @@ -181,6 +181,12 @@ extern int __getlogin_r_loginuid (char *name, size_t namesize) # include <dl-unistd.h> # endif +# if IS_IN (rtld) || !defined SHARED +/* __access variant that does not set errno. Used in very early initialization + code in libc.a and ld.so. */ +extern __typeof (__access) __access_noerrno attribute_hidden; +# endif + __END_DECLS # endif diff --git a/io/Makefile b/io/Makefile index e5493b3..1ebb0d0 100644 --- a/io/Makefile +++ b/io/Makefile @@ -40,6 +40,7 @@ routines := \ mkdir mkdirat \ open open_2 open64 open64_2 openat openat_2 openat64 openat64_2 \ read write lseek lseek64 access euidaccess faccessat \ + access_noerrno \ fcntl flock lockf lockf64 \ close dup dup2 dup3 pipe pipe2 \ creat creat64 \ diff --git a/io/access.c b/io/access.c index 4534704..80cfa98 100644 --- a/io/access.c +++ b/io/access.c @@ -19,10 +19,15 @@ #include <stddef.h> #include <unistd.h> +#ifndef __ACCESS +# define __ACCESS __access +#endif + /* Test for access to FILE. */ int -__access (const char *file, int type) +__ACCESS (const char *file, int type) { +#ifndef NOERRNO if (file == NULL || (type & ~(R_OK|W_OK|X_OK|F_OK)) != 0) { __set_errno (EINVAL); @@ -30,8 +35,11 @@ __access (const char *file, int type) } __set_errno (ENOSYS); +#endif return -1; } +#ifndef NOERRNO stub_warning (access) weak_alias (__access, access) +#endif diff --git a/io/access_noerrno.c b/io/access_noerrno.c new file mode 100644 index 0000000..4bd531f --- /dev/null +++ b/io/access_noerrno.c @@ -0,0 +1,21 @@ +/* Test for access to a file but do not set errno on error. + 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/>. */ + +#define NOERRNO 1 +#define __ACCESS __access_noerrno +#include <access.c> diff --git a/sysdeps/mach/hurd/access.c b/sysdeps/mach/hurd/access.c index c308340..189fefe 100644 --- a/sysdeps/mach/hurd/access.c +++ b/sysdeps/mach/hurd/access.c @@ -22,9 +22,19 @@ #include <hurd/lookup.h> #include <fcntl.h> +#ifndef __ACCESS +# define __ACCESS __access +#endif + +#ifndef NOERRNO +# define __HURD_FAIL(e) __hurd_fail (e) +#else +# define __HURD_FAIL(e) (e) +#endif + /* Test for access to FILE by our real user and group IDs. */ int -__access (const char *file, int type) +__ACCESS (const char *file, int type) { error_t err; file_t rcrdir, rcwdir, io; @@ -120,13 +130,13 @@ __access (const char *file, int type) if (rcwdir != MACH_PORT_NULL) __mach_port_deallocate (__mach_task_self (), rcwdir); if (err) - return __hurd_fail (err); + return __HURD_FAIL (err); /* Find out what types of access we are allowed to this file. */ err = __file_check_access (io, &allowed); __mach_port_deallocate (__mach_task_self (), io); if (err) - return __hurd_fail (err); + return __HURD_FAIL (err); flags = 0; if (type & R_OK) @@ -138,9 +148,11 @@ __access (const char *file, int type) if (flags & ~allowed) /* We are not allowed all the requested types of access. */ - return __hurd_fail (EACCES); + return __HURD_FAIL (EACCES); return 0; } +#ifndef NOERRNO weak_alias (__access, access) +#endif diff --git a/sysdeps/nacl/access.c b/sysdeps/nacl/access.c index 95a0fb7..b7339e7 100644 --- a/sysdeps/nacl/access.c +++ b/sysdeps/nacl/access.c @@ -19,10 +19,22 @@ #include <unistd.h> #include <nacl-interfaces.h> +#ifndef __ACCESS +# define __ACCESS __access +#endif + +#ifndef NOERRNO +# define DO_NACL_CALL NACL_CALL +#else +# define DO_NACL_CALL NACL_CALL_NOERRNO +#endif + /* Test for access to FILE. */ int -__access (const char *file, int type) +__ACCESS (const char *file, int type) { - return NACL_CALL (__nacl_irt_dev_filename.access (file, type), 0); + return DO_NACL_CALL (__nacl_irt_dev_filename.access (file, type), 0); } +#ifndef NOERRNO weak_alias (__access, access) +#endif diff --git a/sysdeps/nacl/nacl-interfaces.h b/sysdeps/nacl/nacl-interfaces.h index b7b45bb..fd8d580 100644 --- a/sysdeps/nacl/nacl-interfaces.h +++ b/sysdeps/nacl/nacl-interfaces.h @@ -113,4 +113,8 @@ __nacl_fail (int err) #define NACL_CALL(err, val) \ ({ int _err = (err); _err ? __nacl_fail (_err) : (val); }) +/* Don't set errno. */ +#define NACL_CALL_NOERRNO(err, val) \ + ({ int _err = (err); _err ? _err : (val); }) + #endif /* nacl-interfaces.h */ diff --git a/sysdeps/unix/access_noerrno.c b/sysdeps/unix/access_noerrno.c new file mode 100644 index 0000000..1ff90a2 --- /dev/null +++ b/sysdeps/unix/access_noerrno.c @@ -0,0 +1,38 @@ +/* Test for access to a file but do not set errno on error. + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011. + + 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 <stddef.h> +#include <unistd.h> +#include <fcntl.h> +#include <sysdep-cancel.h> +#include <sysdep.h> + +/* Test for access to FILE. */ +int +__access_noerrno (const char *file, int type) +{ + INTERNAL_SYSCALL_DECL (err); + int res; + res = INTERNAL_SYSCALL (access, err, 2, file, type); + if (INTERNAL_SYSCALL_ERROR_P (res, err)) + return INTERNAL_SYSCALL_ERRNO (res, err); + else + return 0; +} diff --git a/sysdeps/unix/sysv/linux/generic/access.c b/sysdeps/unix/sysv/linux/generic/access.c index 586aa93..5bafc06 100644 --- a/sysdeps/unix/sysv/linux/generic/access.c +++ b/sysdeps/unix/sysv/linux/generic/access.c @@ -21,11 +21,28 @@ #include <unistd.h> #include <fcntl.h> #include <sysdep-cancel.h> +#include <sysdep.h> + +#ifndef __ACCESS +# define __ACCESS __access +#endif /* Test for access to FILE. */ int -__access (const char *file, int type) +__ACCESS (const char *file, int type) { +#ifndef NOERRNO return INLINE_SYSCALL (faccessat, 3, AT_FDCWD, file, type); +#else + INTERNAL_SYSCALL_DECL (err); + int res; + res = INTERNAL_SYSCALL (faccessat, err, 3, AT_FDCWD, file, type); + if (INTERNAL_SYSCALL_ERROR_P (res, err)) + return INTERNAL_SYSCALL_ERRNO (res, err); + else + return 0; +#endif } +#ifndef NOERRNO weak_alias (__access, access) +#endif