diff mbox

[2/2] New internal function __access_noerrno

Message ID eecf08db-aca1-d824-7366-097d8a44a9ac@linaro.org
State Accepted
Commit afcf3cd8ebff8fed79238a2d1b95338c4606b1ee
Headers show

Commit Message

Adhemerval Zanella Netto Nov. 16, 2016, 1:44 p.m. UTC
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


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

-- 
2.7.4

Comments

Carlos O'Donell Nov. 16, 2016, 4:26 p.m. UTC | #1
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

> 



-- 
Cheers,
Carlos.
Joseph Myers Nov. 16, 2016, 6:28 p.m. UTC | #2
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.)

-- 
Joseph S. Myers
joseph@codesourcery.com
Kalle Olavi Niemitalo Nov. 17, 2016, 8:54 p.m. UTC | #3
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 mbox

Patch

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