Re: [PATCH] Linux: Add execveat system call wrapper

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

On 12/04/2021 16:26, Alexandra Hájková via Libc-alpha wrote:
> From: Alexandra Hájková <ahajkova@redhat.com>
> 
>  Also add the test for the new wrapper.
> ---
> This version:
> * fixes some indentation
> * nothing is missing in NEWS
> * returns the errno after calling openat_nocancel
> * fixes copy paste comment error


> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 303fa297bc..36bc188b55 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -117,6 +117,7 @@ tests += tst-ofdlocks-compat
>  endif
>  
>  tests-internal += tst-sigcontext-get_pc
> +tests-static-internal += tst-execveat-compat
>  
>  CFLAGS-tst-sigcontext-get_pc.c = -fasynchronous-unwind-tables
>  

This not build the tst-execveat-compat since 'tests-static-internal' is
not a generic rule.  The elf/Makefile does define it, but it ended adding
is on test-internal and test-static to actually enable the test.

You need to mimic it:

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile                                                                                                                                    index 36bc188b55..49df102257 100644                                                                                                                                                                                 --- a/sysdeps/unix/sysv/linux/Makefile                                                                                                                                                                              +++ b/sysdeps/unix/sysv/linux/Makefile                                                                                                                                                                              @@ -117,7 +117,9 @@ tests += tst-ofdlocks-compat                                                                                                                                                                     endif                                                                                                                                                                                                                                                                                                                                                                                                                                   tests-internal += tst-sigcontext-get_pc                                                                                                                                                                            -tests-static-internal += tst-execveat-compat                                                                                                                                                                       +                                                                                                                                                                                                                   +tests-static += tst-execveat-compat                                                                                                                                                                                +tests-internal += tst-execveat-compat                                                                                                                                                                                                                                                                                                                                                                                                   CFLAGS-tst-sigcontext-get_pc.c = -fasynchronous-unwind-tables                                                                                                                                                                                                                                                    

> diff --git a/sysdeps/unix/sysv/linux/execveat_fallback.c b/sysdeps/unix/sysv/linux/execveat_fallback.c
> new file mode 100644
> index 0000000000..db67dd0321
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/execveat_fallback.c
> @@ -0,0 +1,69 @@
> +/* Execute program relative to a directory file descriptor.
> +   Copyright (C) 2021 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <sys/stat.h>
> +
> +#include <sysdep.h>
> +#include <sys/syscall.h>
> +#include <kernel-features.h>
> +#include <fd_to_filename.h>
> +#include <not-cancel.h>
> +
> +int
> +__execveat_fallback (int dirfd, const char *path, char *const argv[],
> +                     char *const envp[], int flags)
> +{
> +  int fd;
> +
> +  if (path[0] == '\0' && (flags & AT_EMPTY_PATH) && dirfd >= 0)
> +    fd = dirfd;
> +  else
> +    {
> +      int oflags = O_CLOEXEC;

The default O_CLOEXEC leads the same issue described by fexecve man 
page [1]: if path[0] refers to an executable script with a shebang 
execveat will fail with ENOENT:

$ cat script
#!/bin/sh
exit $*
$ cat test.c 
#include <unistd.h>
#include <assert.h>
#include <fcntl.h>
#include <stdlib.h>
#include <sys/wait.h>

extern int __attribute__((weak)) execveat (int __fd, const char *__path,
                                           char *const __argv[],
                                           char *const __envp[], int __flags);

int main (int argc, char *argv[])
{
  char *pathname = "script";

  pid_t pid = fork ();
  assert (pid >= 0);
  if (pid == 0)
    {
      char *args[] = { pathname, "99", NULL };
      char *envp[] = { NULL };
      execveat (AT_FDCWD, pathname, args, envp, 0);
      _exit (EXIT_FAILURE);
    }

  int status;
  int rc = waitpid (pid, &status, 0);
  assert (rc == pid);
  assert (WIFEXITED(status));
  assert (WEXITSTATUS(status) == 99);

  return 0;
}
$ gcc test.c -o test
$ ./testrun.sh ./test
/bin/sh: 0: Can't open /proc/self/fd/3
test: test.c:34: main: Assertion `WEXITSTATUS(status) == 99' failed.
Aborted (core dumped)

The fexecve call could be mitigated since it moves the responsability
of adding the O_CLOEXEC to caller.  I still think it is not ideal,
but since fexecve is POSIX interface it should be better to always
fail with ENOSYS on older kernels.

There is another corner cases that the fallback do not mimic exactly
the syscall:

  execveat (99 /* invalid fd  */, "", AT_EMPTY_PATH)

which returns ENOENT instead of EBADF, but I do not considere this
a deal break.

So we have some options here:

  1. Leave as is and document properly that using with an executable
     script with shebang might fail.

  2. Remove the O_CLOEXEC from fallback, which leads to file leakage.

  3. Remove the fallback and return ENOSYS on kernel without execveat
     support.

I personally prefer to go for 3. instead of providing a broken fallback.


[1] https://man7.org/linux/man-pages/man3/fexecve.3.html

> +      if (flags & AT_SYMLINK_NOFOLLOW)
> +        oflags |= O_NOFOLLOW;
> +      fd = __openat_nocancel (dirfd, path, oflags);
> +    }
> +  if (fd < 0)
> +    return errno;

This returns the wrong value, it should return 'fd' or just '-1'

> +
> +  struct fd_to_filename fdfilename;
> +  const char *gfilename = __fd_to_filename (fd, &fdfilename);
> + 
> +  /* We do not need the return value.  */
> +  __execve (gfilename, argv, envp);
> +
> +  int save = errno;
> +
> +  /* We come here only if the 'execve' call fails.  Determine whether
> +     /proc is mounted.  If not we return ENOSYS.  */
> +  struct stat64 st;
> +  if (__stat64 (FD_TO_FILENAME_PREFIX, &st) != 0 && errno == ENOENT)
> +    save = ENOSYS;
> +
> +  if (fd != dirfd)
> +    __close_nocancel_nostatus (fd);
> +  __set_errno (save);
> +
> +  return -1;
> +}

> diff --git a/sysdeps/unix/sysv/linux/tst-execveat-compat.c b/sysdeps/unix/sysv/linux/tst-execveat-compat.c
> new file mode 100644
> index 0000000000..a37e2329a5
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-execveat-compat.c
> @@ -0,0 +1,28 @@
> +/* Test the fallback implementation of execveat.
> +   Copyright (C) 2021 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/>.  */
> +
> +/* Get the declaration of the official execveat function.  */
> +#include <unistd.h>
> +
> +/* Compile a local version of execveat.  */
> +#include <sysdeps/unix/sysv/linux/execveat_fallback.c>
> +
> +/* Re-use the test, but run it against execveat_fallback defined
> +   above.  */
> +#define execveat execveat_fallback

Because you weren't building the tests the failure was not showing itself:
execveat_fallback.c defined __execveat_fallback not execveat_fallback.
You need to fix it with:

diff --git a/sysdeps/unix/sysv/linux/tst-execveat-compat.c b/sysdeps/unix/sysv/linux/tst-execveat-compat.c
index a37e2329a5..9374018627 100644
--- a/sysdeps/unix/sysv/linux/tst-execveat-compat.c
+++ b/sysdeps/unix/sysv/linux/tst-execveat-compat.c
@@ -24,5 +24,5 @@
 
 /* Re-use the test, but run it against execveat_fallback defined
    above.  */
-#define execveat execveat_fallback
+#define execveat __execveat_fallback
 #include <posix/tst-execveat.c>