diff mbox

[2/2] New internal function __access_noerrno

Message ID 1478797446-12213-2-git-send-email-adhemerval.zanella@linaro.org
State Superseded
Headers show

Commit Message

Adhemerval Zanella Netto Nov. 10, 2016, 5:04 p.m. UTC
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.

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(-)

-- 
2.7.4

Comments

Andreas Schwab Nov. 10, 2016, 5:12 p.m. UTC | #1
On Nov 10 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> 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;


Isn't that supposed to _not_ set errno?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Siddhesh Poyarekar Nov. 10, 2016, 5:53 p.m. UTC | #2
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.

> 

> 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.

> +}

>  

>  /* 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)

>
Adhemerval Zanella Netto Nov. 10, 2016, 6:26 p.m. UTC | #3
On 10/11/2016 15:12, Andreas Schwab wrote:
> On Nov 10 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> 

>> 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;

> 

> Isn't that supposed to _not_ set errno?

> 

> Andreas.

> 


Oops, fixed it locally.
Adhemerval Zanella Netto Nov. 10, 2016, 6:31 p.m. UTC | #4
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.

> 

>> +}

>>  

>>  /* 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)

>>
Adhemerval Zanella Netto Nov. 16, 2016, 1:27 p.m. UTC | #5
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
Kalle Olavi Niemitalo Nov. 21, 2016, 5:13 a.m. UTC | #6
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> Hurd VM does not boot with a recent qemu (2.7.50)


I tried booting debian-hurd-20160824.img (sha256sum
a17547a6b4ae56d2e1e84d82a7308c12f6301c1b4bc9fc210e239730da4a08f5)
with the following versions of QEMU on Debian 8.6 amd64, and did
not see that problem.  KVM was enabled, i.e. I ran
qemu-system-i386 -enable-kvm under a user account that was a
member of the kvm group.

* qemu-system-x86 1:2.1+dfsg-12+deb8u6 from Debian

* QEMU 2.7.0 built from source

* QEMU 2.8.0-rc0 + "trace: fix generated code build break"
  (commit d4f7ca59017835784c6872dfab0e269d9b41b05a on 2016-11-17)
  built from source
Adhemerval Zanella Netto Nov. 21, 2016, 12:44 p.m. UTC | #7
On 21/11/2016 03:13, Kalle Olavi Niemitalo wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> 

>> Hurd VM does not boot with a recent qemu (2.7.50)

> 

> I tried booting debian-hurd-20160824.img (sha256sum

> a17547a6b4ae56d2e1e84d82a7308c12f6301c1b4bc9fc210e239730da4a08f5)

> with the following versions of QEMU on Debian 8.6 amd64, and did

> not see that problem.  KVM was enabled, i.e. I ran

> qemu-system-i386 -enable-kvm under a user account that was a

> member of the kvm group.

> 

> * qemu-system-x86 1:2.1+dfsg-12+deb8u6 from Debian

> 

> * QEMU 2.7.0 built from source

> 

> * QEMU 2.8.0-rc0 + "trace: fix generated code build break"

>   (commit d4f7ca59017835784c6872dfab0e269d9b41b05a on 2016-11-17)

>   built from source

> 


I tested again on and it seems for some reason '--nographic' does 
not show any output from console.  On a system with X I now see
a bootable system, thanks for the tip.
diff mbox

Patch

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;
+}
 
 /* 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)
+{
+  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)
+{
+  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)