[PATCH 1/2] Consolidate Linux access implementation

[PATCH 1/2] Consolidate Linux access implementation

[Adhemerval Zanella]

This patch consolidates the Linux access implementation on
sysdeps/unix/sysv/linux/access.c.  Similar to auto-generation through
syscalls.list, __NR_access is check and __NR_faccessat is used only
for newer architectures (where __NR_access is not defined).

Checked on x86_64.

	* sysdeps/unix/sysv/linux/access.c: New file.
	* sysdeps/unix/sysv/linux/generic/access.c: Remove file.
---
 ChangeLog                                |  5 +++++
 sysdeps/unix/sysv/linux/access.c         | 32 ++++++++++++++++++++++++++++++++
 sysdeps/unix/sysv/linux/generic/access.c | 31 -------------------------------
 3 files changed, 37 insertions(+), 31 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/access.c
 delete mode 100644 sysdeps/unix/sysv/linux/generic/access.c

diff --git a/sysdeps/unix/sysv/linux/access.c b/sysdeps/unix/sysv/linux/access.c
new file mode 100644
index 0000000..cdb7908
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/access.c
@@ -0,0 +1,32 @@
+/* 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/>.  */
+
+#include <fcntl.h>
+#include <unistd.h>
+#include <sysdep-cancel.h>
+
+/* Test for access to FILE.  */
+int
+__access (const char *file, int type)
+{
+#ifdef __NR_access
+  return INLINE_SYSCALL_CALL (access, file, type);
+#else
+  return INLINE_SYSCALL_CALL (faccessat, AT_FDCWD, file, type);
+#endif
+}
+weak_alias (__access, access)
diff --git a/sysdeps/unix/sysv/linux/generic/access.c b/sysdeps/unix/sysv/linux/generic/access.c
deleted file mode 100644
index 586aa93..0000000
--- a/sysdeps/unix/sysv/linux/generic/access.c
+++ /dev/null
@@ -1,31 +0,0 @@
-/* Copyright (C) 2011-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>
-
-/* Test for access to FILE.  */
-int
-__access (const char *file, int type)
-{
-  return INLINE_SYSCALL (faccessat, 3, AT_FDCWD, file, type);
-}
-weak_alias (__access, access)
-- 
2.7.4

[PATCH 2/2] New internal function __access_noerrno

[Adhemerval Zanella]

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

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;
+}
 \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)
+{
+  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)
-- 
2.7.4

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

[Andreas Schwab]

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

Re: [PATCH 1/2] Consolidate Linux access implementation

[Siddhesh Poyarekar]

On Thursday 10 November 2016 10:34 PM, Adhemerval Zanella wrote:
> This patch consolidates the Linux access implementation on
> sysdeps/unix/sysv/linux/access.c.  Similar to auto-generation through
> syscalls.list, __NR_access is check and __NR_faccessat is used only
> for newer architectures (where __NR_access is not defined).
> 
> Checked on x86_64.
> 
> 	* sysdeps/unix/sysv/linux/access.c: New file.
> 	* sysdeps/unix/sysv/linux/generic/access.c: Remove file.
> ---
>  ChangeLog                                |  5 +++++
>  sysdeps/unix/sysv/linux/access.c         | 32 ++++++++++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/generic/access.c | 31 -------------------------------
>  3 files changed, 37 insertions(+), 31 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/access.c
>  delete mode 100644 sysdeps/unix/sysv/linux/generic/access.c
> 
> diff --git a/sysdeps/unix/sysv/linux/access.c b/sysdeps/unix/sysv/linux/access.c
> new file mode 100644
> index 0000000..cdb7908
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/access.c
> @@ -0,0 +1,32 @@

Top level comment for the file, you could just move the one right before
the function over here.  Looks good to me with that change.

> +/* 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/>.  */
> +
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sysdep-cancel.h>
> +
> +/* Test for access to FILE.  */
> +int
> +__access (const char *file, int type)
> +{
> +#ifdef __NR_access
> +  return INLINE_SYSCALL_CALL (access, file, type);
> +#else
> +  return INLINE_SYSCALL_CALL (faccessat, AT_FDCWD, file, type);
> +#endif
> +}
> +weak_alias (__access, access)
> diff --git a/sysdeps/unix/sysv/linux/generic/access.c b/sysdeps/unix/sysv/linux/generic/access.c
> deleted file mode 100644
> index 586aa93..0000000
> --- a/sysdeps/unix/sysv/linux/generic/access.c
> +++ /dev/null
> @@ -1,31 +0,0 @@
> -/* Copyright (C) 2011-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>
> -
> -/* Test for access to FILE.  */
> -int
> -__access (const char *file, int type)
> -{
> -  return INLINE_SYSCALL (faccessat, 3, AT_FDCWD, file, type);
> -}
> -weak_alias (__access, access)
> 

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

[Siddhesh Poyarekar]

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.

> +}
>  \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)
> 

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

[Adhemerval Zanella]

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.

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

[Adhemerval Zanella]

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

Re: [PATCH 1/2] Consolidate Linux access implementation

[Carlos O'Donell]

On 11/10/2016 12:04 PM, Adhemerval Zanella wrote:
> This patch consolidates the Linux access implementation on
> sysdeps/unix/sysv/linux/access.c.  Similar to auto-generation through
> syscalls.list, __NR_access is check and __NR_faccessat is used only
> for newer architectures (where __NR_access is not defined).
> 
> Checked on x86_64.

OK to checkin if you test this on a second arch, preferably aarch64,
and fix the comment below.

> 	* sysdeps/unix/sysv/linux/access.c: New file.
> 	* sysdeps/unix/sysv/linux/generic/access.c: Remove file.
> ---
>  ChangeLog                                |  5 +++++
>  sysdeps/unix/sysv/linux/access.c         | 32 ++++++++++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/generic/access.c | 31 -------------------------------
>  3 files changed, 37 insertions(+), 31 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/access.c
>  delete mode 100644 sysdeps/unix/sysv/linux/generic/access.c
> 
> diff --git a/sysdeps/unix/sysv/linux/access.c b/sysdeps/unix/sysv/linux/access.c
> new file mode 100644
> index 0000000..cdb7908
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/access.c
> @@ -0,0 +1,32 @@

As Siddhesh notes this needs first line comment.

> +/* 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/>.  */
> +
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <sysdep-cancel.h>
> +
> +/* Test for access to FILE.  */
> +int
> +__access (const char *file, int type)
> +{
> +#ifdef __NR_access
> +  return INLINE_SYSCALL_CALL (access, file, type);
> +#else
> +  return INLINE_SYSCALL_CALL (faccessat, AT_FDCWD, file, type);
> +#endif
> +}
> +weak_alias (__access, access)
> diff --git a/sysdeps/unix/sysv/linux/generic/access.c b/sysdeps/unix/sysv/linux/generic/access.c
> deleted file mode 100644
> index 586aa93..0000000
> --- a/sysdeps/unix/sysv/linux/generic/access.c
> +++ /dev/null
> @@ -1,31 +0,0 @@
> -/* Copyright (C) 2011-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>
> -
> -/* Test for access to FILE.  */
> -int
> -__access (const char *file, int type)
> -{
> -  return INLINE_SYSCALL (faccessat, 3, AT_FDCWD, file, type);
> -}
> -weak_alias (__access, access)
 


-- 
Cheers,
Carlos.

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

[Adhemerval Zanella]

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

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

[Adhemerval Zanella]

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

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;
+}
 \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..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
-- 
2.7.4

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

[Carlos O'Donell]

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.

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

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.

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

[Joseph Myers]

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

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

[Kalle Olavi Niemitalo]

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.

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

[Adhemerval Zanella]

Thanks for the review.

On 17/11/2016 18:54, Kalle Olavi Niemitalo wrote:
> 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.

My understanding of the tunables usage is it should returns what is
current described by access minus without setting errno, i.e, on
success zero is returned otherwise a value different than 0.

> 
>> +__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.)

Alright, I removed __hurd_fail_noerrno and made hurd_fail_noerrno
just return -1.

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

Below is all the fixes you proposed, if you think it is ok I will
prepare a patch and commit.

--

diff --git a/hurd/hurd.h b/hurd/hurd.h
index c089d4c..ec07827 100644
--- a/hurd/hurd.h
+++ b/hurd/hurd.h
@@ -75,35 +75,6 @@ __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;
-}
 \f
 /* Basic ports and info, initialized by startup.  */
 
diff --git a/include/unistd.h b/include/unistd.h
index 6144f41..16d68a1 100644
--- a/include/unistd.h
+++ b/include/unistd.h
@@ -183,7 +183,8 @@ extern int __getlogin_r_loginuid (char *name, size_t namesize)
 
 #  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.  */
+   code in libc.a and ld.so.  It follows access return semantics (zero for
+   sucess otherwise a value different than 0).  */
 extern __typeof (__access) __access_noerrno attribute_hidden;
 #  endif
 
diff --git a/sysdeps/mach/hurd/access.c b/sysdeps/mach/hurd/access.c
index 93e2166..66bf7d5 100644
--- a/sysdeps/mach/hurd/access.c
+++ b/sysdeps/mach/hurd/access.c
@@ -31,7 +31,7 @@ hurd_fail_seterrno (error_t err)
 static int
 hurd_fail_noerrno (error_t err)
 {
-  return __hurd_fail_noerrno (err);
+  return -1;
 }
 
 static int
@@ -149,7 +149,7 @@ access_common (const char *file, int type, int (*errfunc) (error_t))
 
   if (flags & ~allowed)
     /* We are not allowed all the requested types of access.  */
-    return errfunc (EACESS);
+    return errfunc (EACCES);
 
   return 0;
 }

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

[Kalle Olavi Niemitalo]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> Below is all the fixes you proposed, if you think it is ok I will
> prepare a patch and commit.

Thanks.  It looks like an improvement.
I'd like to do some testing during the weekend.

However, I suspect that this __access_noerrno may be unsafe to run
during initialization of tunables on the Hurd.  access_common calls
__hurd_file_name_lookup, which calls __hurd_file_name_lookup_retry,
which can use errno, __strtoul_internal, _itoa, strcpy, and memcmp.
I'm not yet sure whether this is a problem, and I'm not asking you
to spend time on it.

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

[Adhemerval Zanella]

On 17/11/2016 22:21, Kalle Olavi Niemitalo wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> Below is all the fixes you proposed, if you think it is ok I will
>> prepare a patch and commit.
> 
> Thanks.  It looks like an improvement.
> I'd like to do some testing during the weekend.

I pushed a patch based on the RFC I sent earlier.

> 
> However, I suspect that this __access_noerrno may be unsafe to run
> during initialization of tunables on the Hurd.  access_common calls
> __hurd_file_name_lookup, which calls __hurd_file_name_lookup_retry,
> which can use errno, __strtoul_internal, _itoa, strcpy, and memcmp.
> I'm not yet sure whether this is a problem, and I'm not asking you
> to spend time on it.
> 

I think it might be an issue depending of whether errno is accessible
on tunables initialization (it might be the case where is not yet
relocated/allocated).  Anyway I do not have a working system to 
actually even test a build on Hurd (the instruction at [1] points
to a non bootable VM).

If you could provide us with a working toolchain or a working VM
to actually check hurd builds it would be valuable.  Also it could
be case to add a hurd configuration to build-many-glibcs.py.

[1] https://www.gnu.org/software/hurd/hurd/running/qemu.html

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

[Siddhesh Poyarekar]

On Saturday 19 November 2016 12:23 AM, Adhemerval Zanella wrote:
> I think it might be an issue depending of whether errno is accessible
> on tunables initialization (it might be the case where is not yet

That is correct.  Tunables initialization happens very early, earlier
than TLS setup and errno is a TLS variable.

Siddhesh

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

[Kalle Olavi Niemitalo]

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

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

[Adhemerval Zanella]

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.