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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 24/10/2016 12:42, Siddhesh Poyarekar wrote:
> 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.
> 
> 	* include/unistd.h [IS_IN (rtld) || !defined SHARED]: Declare
> 	__access_noerrno.
> 	* io/Makefile (routines): Add access_noerrno.
> 	* io/access.c (__ACCESS)[!__ACCESS]: Define as __access.
> 	(__access): Rename to __ACCESS.
> 	[!NOERRNO]: Retain default __access logic.
> 	* io/access_noerrno.c: New file.
> 	* sysdeps/mach/hurd/access.c (__ACCESS)[!__ACCESS]: Define as
> 	__access.
> 	(__HURD_FAIL): New macro.
> 	(__access): Rename to __ACCESS.  Use __HURD_FAIL instead of
> 	__hurd_fail.
> 	[!NOERRNO]: Set weak alias to access.
> 	* sysdeps/nacl/access.c (__ACCESS)[!__ACCESS]: Define as
> 	__access.
> 	(DO_NACL_CALL): New macro.
> 	(__access): Rename to __ACCESS.  Use DO_NACL_CALL instead of
> 	NACL_CALL.
> 	[!NOERRNO]: Set weak alias to access.
> 	* sysdeps/nacl/nacl-interfaces.h (NACL_CALL_NOERRNO): New
> 	macro.
> 	* sysdeps/unix/access_noerrno.c: New file.
> 	* sysdeps/unix/sysv/linux/generic/access.c: Include sysdep.h.
> 	(__ACCESS)[!__ACCESS]: Define as __access.
> 	(__access): Rename to __ACCESS.
> 	[NOERRNO]: Call faccessat syscall without setting errno.
> ---
>  include/unistd.h                         |  6 +++++
>  io/Makefile                              |  1 +
>  io/access.c                              | 10 ++++++++-
>  io/access_noerrno.c                      | 21 ++++++++++++++++++
>  sysdeps/mach/hurd/access.c               | 20 +++++++++++++----
>  sysdeps/nacl/access.c                    | 16 ++++++++++++--
>  sysdeps/nacl/nacl-interfaces.h           |  4 ++++
>  sysdeps/unix/access_noerrno.c            | 38 ++++++++++++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/generic/access.c | 19 +++++++++++++++-
>  9 files changed, 127 insertions(+), 8 deletions(-)
>  create mode 100644 io/access_noerrno.c
>  create mode 100644 sysdeps/unix/access_noerrno.c
> 
> 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.  */

Is this comment correct? Checking the patch I am seeing it builds
only for libc and there is no resulting __access_noerrno on ld.so.

> diff --git a/sysdeps/unix/access_noerrno.c b/sysdeps/unix/access_noerrno.c
> new file mode 100644
> index 0000000..1ff90a2
> --- /dev/null
> +++ b/sysdeps/unix/access_noerrno.c
> @@ -0,0 +1,38 @@
> +/* Test for access to a file but do not set errno on error.
> +   Copyright (C) 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>
> +#include <sysdep.h>
> +
> +/* Test for access to FILE.  */
> +int
> +__access_noerrno (const char *file, int type)
> +{
> +  INTERNAL_SYSCALL_DECL (err);
> +  int res;
> +  res = INTERNAL_SYSCALL (access, err, 2, file, type);
> +  if (INTERNAL_SYSCALL_ERROR_P (res, err))
> +    return INTERNAL_SYSCALL_ERRNO (res, err);
> +  else
> +    return 0;
> +}

I think it would be simpler to just 1. consolidation Linux access
implementation and 2. add the '_noerrno' on same file.

The 1. would be simpler to just:

   1.1. Remove access from sysdeps/unix/syscalls.list
   1.2. Move sysdeps/unix/sysv/linux/generic/access.c to
	sysdeps/unix/sysv/linux/access.c
   1.3. And implement access checking for __NR_access and
	using __NR_facessat if is not defined.  Something
	like:

[...]
/* Test for access to FILE.  */
int
__access (const char *file, int type)
{
#if __NR_access
  return INLINE_SYSCALL_CALL (access, file, type);
#else
  return INLINE_SYSCALL_CALL (faccessat, AT_FDCWD, file, type);
#endif
}
[...]

Then __access_noerro could be just implemented on same file.
I think it has the advantage of not splitting the access
on multiple files.

I think for hurd and nacl it would also result in simplify
code.  What do you think?


> diff --git a/sysdeps/unix/sysv/linux/generic/access.c b/sysdeps/unix/sysv/linux/generic/access.c
> index 586aa93..5bafc06 100644
> --- a/sysdeps/unix/sysv/linux/generic/access.c
> +++ b/sysdeps/unix/sysv/linux/generic/access.c
> @@ -21,11 +21,28 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <sysdep-cancel.h>
> +#include <sysdep.h>
> +
> +#ifndef __ACCESS
> +# define __ACCESS __access
> +#endif
>  
>  /* Test for access to FILE.  */
>  int
> -__access (const char *file, int type)
> +__ACCESS (const char *file, int type)
>  {
> +#ifndef NOERRNO
>    return INLINE_SYSCALL (faccessat, 3, AT_FDCWD, file, type);
> +#else
> +  INTERNAL_SYSCALL_DECL (err);
> +  int res;
> +  res = INTERNAL_SYSCALL (faccessat, err, 3, AT_FDCWD, file, type);
> +  if (INTERNAL_SYSCALL_ERROR_P (res, err))
> +    return INTERNAL_SYSCALL_ERRNO (res, err);
> +  else
> +    return 0;
> +#endif
>  }
> +#ifndef NOERRNO
>  weak_alias (__access, access)
> +#endif
>