| Message ID | eecf08db-aca1-d824-7366-097d8a44a9ac@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 11/16/2016 08:44 AM, Adhemerval Zanella wrote: > > > On 16/11/2016 11:27, Adhemerval Zanella wrote: >> >> >> On 10/11/2016 16:31, Adhemerval Zanella wrote: >>> >>> >>> On 10/11/2016 15:53, Siddhesh Poyarekar wrote: >>>> On Thursday 10 November 2016 10:34 PM, Adhemerval Zanella wrote: >>>>> This ia follow up patch for tunables requirement [1]. It 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. >>>> >>>> I think someone else should also review and ack this one, but I'll do a >>>> review round anyway. >>> >>> Thanks, I fixes all my mistakes locally. It would be good to have a >>> ack for nacl/hurd before pushing it. >> >> I tried both hurd [1] and nacl [2] environments to check the build but >> without success. Hurd VM does not boot with a recent qemu (2.7.50) and >> NaCL toolchain seems stuck in a ancient gcc version (4.4.7). Since I >> this patch won't break any functionality (since it only adds a new >> symbol), I see it should be safe to push. >> >> [1] https://people.debian.org/~sthibault/hurd-i386/README >> [2] https://developer.chrome.com/native-client/sdk/download Agreed, if we can't get building solutions for nacl and hurd then we can't test them. I've discussed this before without contributors and it's a serious issue for hurd that we really need to fix, but that is not your responsibility. > Updated version below: > > * hurd/hurd.h (__hurd_fail_noerrno): New function. > * include/unistd.h [IS_IN (rtld) || !defined SHARED]: Declare > __access_noerrno. > * io/access.c (__access_noerrno): New function. > * sysdeps/mach/hurd/access.c (hurd_fail_seterrno): New function. > (hurd_fail_seterrno): Likewise. > (access_common): Likewise. > (__access_noerrno): Likewise. > * sysdeps/nacl/access.c (__access_noerrno): Likewise. > * sysdeps/unix/sysv/linux/access.c (__access_noerrno): Likewise. > * sysdeps/nacl/nacl-interfaces.h (NACL_CALL_NOERRNO): New > macro. This patch looks good to me. > --- > ChangeLog | 15 +++++++++++++++ > hurd/hurd.h | 29 +++++++++++++++++++++++++++++ > include/unistd.h | 6 ++++++ > io/access.c | 7 +++++++ > sysdeps/mach/hurd/access.c | 37 +++++++++++++++++++++++++++++++------ > sysdeps/nacl/access.c | 7 +++++++ > sysdeps/nacl/nacl-interfaces.h | 4 ++++ > sysdeps/unix/sysv/linux/access.c | 15 +++++++++++++++ > 8 files changed, 114 insertions(+), 6 deletions(-) > > diff --git a/ChangeLog b/ChangeLog > index dfa48e4..b2e5b68 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,18 @@ > +2016-11-16 Adhemerval Zanella <adhemerval.zanella@linaro.org> > + > + * hurd/hurd.h (__hurd_fail_noerrno): New function. > + * include/unistd.h [IS_IN (rtld) || !defined SHARED]: Declare > + __access_noerrno. > + * io/access.c (__access_noerrno): New function. > + * sysdeps/mach/hurd/access.c (hurd_fail_seterrno): New function. > + (hurd_fail_seterrno): Likewise. > + (access_common): Likewise. > + (__access_noerrno): Likewise. > + * sysdeps/nacl/access.c (__access_noerrno): Likewise. > + * sysdeps/unix/sysv/linux/access.c (__access_noerrno): Likewise. > + * sysdeps/nacl/nacl-interfaces.h (NACL_CALL_NOERRNO): New > + macro. > + > 2016-11-16 Joseph Myers <joseph@codesourcery.com> > > * sysdeps/unix/sysv/linux/sh/sh4/register-dump.h (register_dump): > diff --git a/hurd/hurd.h b/hurd/hurd.h > index ec07827..c089d4c 100644 > --- a/hurd/hurd.h > +++ b/hurd/hurd.h > @@ -75,6 +75,35 @@ __hurd_fail (error_t err) > errno = err; > return -1; > } > + > +_HURD_H_EXTERN_INLINE int > +__hurd_fail_noerrno (error_t err) > +{ > + switch (err) > + { > + case EMACH_SEND_INVALID_DEST: > + case EMIG_SERVER_DIED: > + /* The server has disappeared! */ > + err = EIEIO; > + break; > + > + case KERN_NO_SPACE: > + err = ENOMEM; > + break; > + > + case KERN_INVALID_ARGUMENT: > + err = EINVAL; > + break; > + > + case 0: > + return 0; > + > + default: > + break; > + } > + > + return -1; > +} OK. > > /* Basic ports and info, initialized by startup. */ > > 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 OK. > + > __END_DECLS > # endif > > diff --git a/io/access.c b/io/access.c > index 4534704..859cf75 100644 > --- a/io/access.c > +++ b/io/access.c > @@ -19,6 +19,13 @@ > #include <stddef.h> > #include <unistd.h> > > +/* Test for access to FILE without setting errno. */ > +int > +__access_noerrno (const char *file, int type) > +{ > + return -1; > +} OK. > + > /* Test for access to FILE. */ > int > __access (const char *file, int type) > diff --git a/sysdeps/mach/hurd/access.c b/sysdeps/mach/hurd/access.c > index c308340..620acea 100644 > --- a/sysdeps/mach/hurd/access.c > +++ b/sysdeps/mach/hurd/access.c > @@ -22,9 +22,20 @@ > #include <hurd/lookup.h> > #include <fcntl.h> > > -/* Test for access to FILE by our real user and group IDs. */ > -int > -__access (const char *file, int type) > +static int > +hurd_fail_seterrno (error_t err) > +{ > + return __hurd_fail (err); > +} > + > +static int > +hurd_fail_noerrno (error_t err) > +{ > + return __hurd_fail_noerrno (err); > +} OK. > + > +static int > +access_common (const char *file, int type, int (*errfunc) (error_t)) > { > error_t err; > file_t rcrdir, rcwdir, io; > @@ -120,13 +131,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 errfunc (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 errfunc (err); > > flags = 0; > if (type & R_OK) > @@ -138,9 +149,23 @@ __access (const char *file, int type) > > if (flags & ~allowed) > /* We are not allowed all the requested types of access. */ > - return __hurd_fail (EACCES); > + return errfunc (EACESS); > OK. > return 0; > } > > +/* Test for access to FILE by our real user and group IDs without setting > + errno. */ > +int > +__access_noerrno (const char *file, int type) > +{ > + return access_common (file, type, hurd_fail_noerrno); > +} > + > +/* Test for access to FILE by our real user and group IDs. */ > +int > +__access (const char *file, int type) > +{ > + return access_common (file, type, hurd_fail); > +} OK. > weak_alias (__access, access) > diff --git a/sysdeps/nacl/access.c b/sysdeps/nacl/access.c > index 95a0fb7..e07d558 100644 > --- a/sysdeps/nacl/access.c > +++ b/sysdeps/nacl/access.c > @@ -19,6 +19,13 @@ > #include <unistd.h> > #include <nacl-interfaces.h> > > +/* Test for access to FILE without setting errno. */ > +int > +__access_noerrno (const char *file, int type) > +{ > + return NACL_CALL_NOERRNO (__nacl_irt_dev_filename.access (file, type), 0); > +} > + OK. > /* Test for access to FILE. */ > int > __access (const char *file, int type) > diff --git a/sysdeps/nacl/nacl-interfaces.h b/sysdeps/nacl/nacl-interfaces.h > index b7b45bb..edd3217 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); }) > > +/* Same as NACL_CALL but without setting errno. */ > +#define NACL_CALL_NOERRNO(err, val) \ > + ({ int _err = (err); _err ? _err : (val); }) > + OK. > #endif /* nacl-interfaces.h */ > diff --git a/sysdeps/unix/sysv/linux/access.c b/sysdeps/unix/sysv/linux/access.c > index 8f003df..adefbca 100644 > --- a/sysdeps/unix/sysv/linux/access.c > +++ b/sysdeps/unix/sysv/linux/access.c > @@ -21,6 +21,21 @@ > #include <sysdep-cancel.h> > > int > +__access_noerrno (const char *file, int type) > +{ > + int res; > + INTERNAL_SYSCALL_DECL (err); > +#ifdef __NR_access > + res = INTERNAL_SYSCALL_CALL (access, err, file, type); > +#else > + res = INTERNAL_SYSCALL_CALL (faccessat, err, AT_FDCWD, file, type); > +#endif > + if (INTERNAL_SYSCALL_ERROR_P (res, err)) > + return INTERNAL_SYSCALL_ERRNO (res, err); > + return 0; > +} OK. > + > +int > __access (const char *file, int type) > { > #ifdef __NR_access >
On Wed, 16 Nov 2016, Carlos O'Donell wrote: > Agreed, if we can't get building solutions for nacl and hurd then we can't > test them. I've discussed this before without contributors and it's a serious > issue for hurd that we really need to fix, but that is not your responsibility. Now we have build-many-glibcs.py we could do with Hurd and NaCl people either adding support for them (support for checking out and building anything extra needed as equivalent of kernel headers, appropriate configurations to build the toolchain for those systems) or providing detailed instructions on how to build cross toolchains for them from current upstream sources so someone else can add the support. (In the Hurd case there may also be the issue of getting enough out-of-tree changes into glibc so it actually builds and passes compilation tests with unmodified glibc sources.)
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes: > +/* __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; Please document what __access_noerrno is supposed to return on error. The NaCl and Linux implementations appear to return the error code, but the stub and Hurd implementations return -1 like access does, so I'm not sure what you intended. In "[PATCH 1/4] Add framework for tunables", the caller only checks whether the result is zero or nonzero. > +__hurd_fail_noerrno (error_t err) Your __hurd_fail_noerrno just returns (err == 0 ? 0 : -1). All the assignments to err in the switch statement are dead. If __access_noerrno is not required to return the error code, then I don't think __hurd_fail_noerrno is necessary at all... > +static int > +hurd_fail_noerrno (error_t err) > +{ > + return __hurd_fail_noerrno (err); > +} ... instead, hurd_fail_noerrno could just return -1 unconditionally, because it is not even called if err == 0. (Perhaps then rename it, to avoid suggesting it's similar to __hurd_fail.) > - return __hurd_fail (EACCES); > + return errfunc (EACESS); Typo here. The t/faccessat branch at git://git.sv.gnu.org/hurd/glibc.git changes this code path quite a lot. Until that branch is merged to upstream glibc, I think your function-pointer scheme is OK, apart from the comments above. I didn't yet try building it though.
diff --git a/ChangeLog b/ChangeLog index dfa48e4..b2e5b68 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,18 @@ +2016-11-16 Adhemerval Zanella <adhemerval.zanella@linaro.org> + + * hurd/hurd.h (__hurd_fail_noerrno): New function. + * include/unistd.h [IS_IN (rtld) || !defined SHARED]: Declare + __access_noerrno. + * io/access.c (__access_noerrno): New function. + * sysdeps/mach/hurd/access.c (hurd_fail_seterrno): New function. + (hurd_fail_seterrno): Likewise. + (access_common): Likewise. + (__access_noerrno): Likewise. + * sysdeps/nacl/access.c (__access_noerrno): Likewise. + * sysdeps/unix/sysv/linux/access.c (__access_noerrno): Likewise. + * sysdeps/nacl/nacl-interfaces.h (NACL_CALL_NOERRNO): New + macro. + 2016-11-16 Joseph Myers <joseph@codesourcery.com> * sysdeps/unix/sysv/linux/sh/sh4/register-dump.h (register_dump): diff --git a/hurd/hurd.h b/hurd/hurd.h index ec07827..c089d4c 100644 --- a/hurd/hurd.h +++ b/hurd/hurd.h @@ -75,6 +75,35 @@ __hurd_fail (error_t err) errno = err; return -1; } + +_HURD_H_EXTERN_INLINE int +__hurd_fail_noerrno (error_t err) +{ + switch (err) + { + case EMACH_SEND_INVALID_DEST: + case EMIG_SERVER_DIED: + /* The server has disappeared! */ + err = EIEIO; + break; + + case KERN_NO_SPACE: + err = ENOMEM; + break; + + case KERN_INVALID_ARGUMENT: + err = EINVAL; + break; + + case 0: + return 0; + + default: + break; + } + + return -1; +} /* Basic ports and info, initialized by startup. */ 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/access.c b/io/access.c index 4534704..859cf75 100644 --- a/io/access.c +++ b/io/access.c @@ -19,6 +19,13 @@ #include <stddef.h> #include <unistd.h> +/* Test for access to FILE without setting errno. */ +int +__access_noerrno (const char *file, int type) +{ + return -1; +} + /* Test for access to FILE. */ int __access (const char *file, int type) diff --git a/sysdeps/mach/hurd/access.c b/sysdeps/mach/hurd/access.c index c308340..620acea 100644 --- a/sysdeps/mach/hurd/access.c +++ b/sysdeps/mach/hurd/access.c @@ -22,9 +22,20 @@ #include <hurd/lookup.h> #include <fcntl.h> -/* Test for access to FILE by our real user and group IDs. */ -int -__access (const char *file, int type) +static int +hurd_fail_seterrno (error_t err) +{ + return __hurd_fail (err); +} + +static int +hurd_fail_noerrno (error_t err) +{ + return __hurd_fail_noerrno (err); +} + +static int +access_common (const char *file, int type, int (*errfunc) (error_t)) { error_t err; file_t rcrdir, rcwdir, io; @@ -120,13 +131,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 errfunc (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 errfunc (err); flags = 0; if (type & R_OK) @@ -138,9 +149,23 @@ __access (const char *file, int type) if (flags & ~allowed) /* We are not allowed all the requested types of access. */ - return __hurd_fail (EACCES); + return errfunc (EACESS); return 0; } +/* Test for access to FILE by our real user and group IDs without setting + errno. */ +int +__access_noerrno (const char *file, int type) +{ + return access_common (file, type, hurd_fail_noerrno); +} + +/* Test for access to FILE by our real user and group IDs. */ +int +__access (const char *file, int type) +{ + return access_common (file, type, hurd_fail); +} weak_alias (__access, access) diff --git a/sysdeps/nacl/access.c b/sysdeps/nacl/access.c index 95a0fb7..e07d558 100644 --- a/sysdeps/nacl/access.c +++ b/sysdeps/nacl/access.c @@ -19,6 +19,13 @@ #include <unistd.h> #include <nacl-interfaces.h> +/* Test for access to FILE without setting errno. */ +int +__access_noerrno (const char *file, int type) +{ + return NACL_CALL_NOERRNO (__nacl_irt_dev_filename.access (file, type), 0); +} + /* Test for access to FILE. */ int __access (const char *file, int type) diff --git a/sysdeps/nacl/nacl-interfaces.h b/sysdeps/nacl/nacl-interfaces.h index b7b45bb..edd3217 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); }) +/* Same as NACL_CALL but without setting errno. */ +#define NACL_CALL_NOERRNO(err, val) \ + ({ int _err = (err); _err ? _err : (val); }) + #endif /* nacl-interfaces.h */ diff --git a/sysdeps/unix/sysv/linux/access.c b/sysdeps/unix/sysv/linux/access.c index 8f003df..adefbca 100644 --- a/sysdeps/unix/sysv/linux/access.c +++ b/sysdeps/unix/sysv/linux/access.c @@ -21,6 +21,21 @@ #include <sysdep-cancel.h> int +__access_noerrno (const char *file, int type) +{ + int res; + INTERNAL_SYSCALL_DECL (err); +#ifdef __NR_access + res = INTERNAL_SYSCALL_CALL (access, err, file, type); +#else + res = INTERNAL_SYSCALL_CALL (faccessat, err, AT_FDCWD, file, type); +#endif + if (INTERNAL_SYSCALL_ERROR_P (res, err)) + return INTERNAL_SYSCALL_ERRNO (res, err); + return 0; +} + +int __access (const char *file, int type) { #ifdef __NR_access