Re: [PATCH 2/2] New internal function __access_noerrno

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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.

> 
>>
>> Checked on x86_64.
>>
>> 	Siddhesh Poyarekar  <siddhesh@sourceware.org>
>> 	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.
>>
>> [1] https://sourceware.org/ml/libc-alpha/2016-11/msg00399.html
>> ---
>>  ChangeLog                        | 13 +++++++++++++
>>  hurd/hurd.h                      | 30 ++++++++++++++++++++++++++++++
>>  include/unistd.h                 |  6 ++++++
>>  io/access.c                      |  8 +++++++-
>>  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, 113 insertions(+), 7 deletions(-)
>>
>> diff --git a/hurd/hurd.h b/hurd/hurd.h
>> index ec07827..8bcb1ec 100644
>> --- a/hurd/hurd.h
>> +++ b/hurd/hurd.h
>> @@ -75,6 +75,36 @@ __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;
>> +    }
>> +
>> +  errno = err;
>> +  return -1;
> 
> Should not set errno and return it instead to be consistent with what
> other architectures do.  It might be nicer to have __hurd_fail call
> __hurd_fail_noerrno to reduce code duplication.
> 
>> +}
>>  \f
>>  /* 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..68b49ca 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)
>> @@ -33,5 +40,4 @@ __access (const char *file, int type)
>>    return -1;
>>  }
>>  stub_warning (access)
>> -
>>  weak_alias (__access, access)
>> 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..4266d63 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 (const char *file, int type)
> 
> __access_noerrno
> 
>> +{
>> +  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 cdb7908..004da1b 100644
>> --- a/sysdeps/unix/sysv/linux/access.c
>> +++ b/sysdeps/unix/sysv/linux/access.c
>> @@ -19,6 +19,21 @@
>>  #include <unistd.h>
>>  #include <sysdep-cancel.h>
>>  
>> +int
>> +__access_noerro (const char *file, int type)
> 
> Typo, __access_noerrno.
> 
>> +{
>> +  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;
>> +}
>> +
>>  /* Test for access to FILE.  */
>>  int
>>  __access (const char *file, int type)
>>