diff mbox series

[v2,04/13] Linux: Error handling fixes for fexecve

Message ID 2142860d3f3a6a56fc393f1c0ccc3f859601e06f.1722193092.git.fweimer@redhat.com
State New
Headers show
Series getenv/environ thread safety | expand

Commit Message

Florian Weimer July 28, 2024, 7:02 p.m. UTC 
Do not check argv and envp for null, for consistency with
execveat and execve.  An invalid descriptor should result in
EBADF, not EINVAL.

In the fallback code, if the file descriptor is not valid,
return EBADF, not ENOENT.
---
 posix/tst-fexecve.c               | 14 ++++++++++++++
 sysdeps/unix/sysv/linux/fexecve.c | 20 +++++++++++++++-----
 2 files changed, 29 insertions(+), 5 deletions(-)

Comments

Adhemerval Zanella Netto Oct. 15, 2024, 7:57 p.m. UTC | #1 
On 28/07/24 16:02, Florian Weimer wrote:
> Do not check argv and envp for null, for consistency with
> execveat and execve.  An invalid descriptor should result in
> EBADF, not EINVAL.
> 
> In the fallback code, if the file descriptor is not valid,
> return EBADF, not ENOENT.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  posix/tst-fexecve.c               | 14 ++++++++++++++
>  sysdeps/unix/sysv/linux/fexecve.c | 20 +++++++++++++++-----
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/posix/tst-fexecve.c b/posix/tst-fexecve.c
> index 9c528fe8d9..c8e772afb6 100644
> --- a/posix/tst-fexecve.c
> +++ b/posix/tst-fexecve.c
> @@ -82,6 +82,20 @@ do_test (void)
>    close (fd);
>  #endif
>  
> +  /* Now fd is not open, so fexecve is expected to fail with EBADF.  */
> +  static const char *const argv[] = {
> +    "/bin/sh", "-c", "false", NULL
> +  };
> +  errno = -1;
> +  TEST_COMPARE (fexecve (fd, (char **const) argv, environ), -1);
> +  TEST_COMPARE (errno, EBADF);
> +
> +  /* Likewise for negative descriptors.  */
> +  TEST_VERIFY (AT_FDCWD != -1);
> +  errno = -1;
> +  TEST_COMPARE (fexecve (-1, (char **const) argv, environ), -1);
> +  TEST_COMPARE (errno, EBADF);
> +
>    return ret;
>  }
>  
> diff --git a/sysdeps/unix/sysv/linux/fexecve.c b/sysdeps/unix/sysv/linux/fexecve.c
> index 46c8170092..c2303735de 100644
> --- a/sysdeps/unix/sysv/linux/fexecve.c
> +++ b/sysdeps/unix/sysv/linux/fexecve.c
> @@ -45,9 +45,18 @@ __do_fexecve (int fd, char *const argv[], char *const envp[])
>  
>        /* We come here only if the 'execve' call fails.  Determine whether
>           /proc is mounted.  If not we return ENOSYS.  */
> -      if (INTERNAL_SYSCALL_CALL (faccessat, AT_FDCWD, "/proc/self/fd", 0)
> -          == -ENOENT)
> -        err = ENOSYS;
> +      if (err == ENOENT)
> +        {
> +          if (INTERNAL_SYSCALL_CALL (faccessat, AT_FDCWD, "/proc/self/fd", 0)
> +              == -ENOENT)
> +            err = ENOSYS;
> +          else if (INTERNAL_SYSCALL_CALL (faccessat, AT_FDCWD, path, 0)
> +                   == -ENOENT)
> +            /* The file descriptor does not exist, but /proc is mounted.  */
> +            err = EBADF;
> +          /* Otherwise ENOENT comes from an invalid program
> +             interpreter or some other property of the file.  */
> +        }
>      }
>  #endif /* __ASSUME_EXECVEAT */
>  
> @@ -60,9 +69,10 @@ __do_fexecve (int fd, char *const argv[], char *const envp[])
>  int
>  fexecve (int fd, char *const argv[], char *const envp[])
>  {
> -  if (fd < 0 || argv == NULL || envp == NULL)
> +  /* Avoid unexpected behavior of execveat with AT_FDCWD.  */
> +  if (fd < 0)
>      {
> -      __set_errno (EINVAL);
> +      __set_errno (EBADF);
>        return -1;
>      }
>
diff mbox series

Patch

diff --git a/posix/tst-fexecve.c b/posix/tst-fexecve.c
index 9c528fe8d9..c8e772afb6 100644
--- a/posix/tst-fexecve.c
+++ b/posix/tst-fexecve.c
@@ -82,6 +82,20 @@  do_test (void)
   close (fd);
 #endif
 
+  /* Now fd is not open, so fexecve is expected to fail with EBADF.  */
+  static const char *const argv[] = {
+    "/bin/sh", "-c", "false", NULL
+  };
+  errno = -1;
+  TEST_COMPARE (fexecve (fd, (char **const) argv, environ), -1);
+  TEST_COMPARE (errno, EBADF);
+
+  /* Likewise for negative descriptors.  */
+  TEST_VERIFY (AT_FDCWD != -1);
+  errno = -1;
+  TEST_COMPARE (fexecve (-1, (char **const) argv, environ), -1);
+  TEST_COMPARE (errno, EBADF);
+
   return ret;
 }
 
diff --git a/sysdeps/unix/sysv/linux/fexecve.c b/sysdeps/unix/sysv/linux/fexecve.c
index 46c8170092..c2303735de 100644
--- a/sysdeps/unix/sysv/linux/fexecve.c
+++ b/sysdeps/unix/sysv/linux/fexecve.c
@@ -45,9 +45,18 @@  __do_fexecve (int fd, char *const argv[], char *const envp[])
 
       /* We come here only if the 'execve' call fails.  Determine whether
          /proc is mounted.  If not we return ENOSYS.  */
-      if (INTERNAL_SYSCALL_CALL (faccessat, AT_FDCWD, "/proc/self/fd", 0)
-          == -ENOENT)
-        err = ENOSYS;
+      if (err == ENOENT)
+        {
+          if (INTERNAL_SYSCALL_CALL (faccessat, AT_FDCWD, "/proc/self/fd", 0)
+              == -ENOENT)
+            err = ENOSYS;
+          else if (INTERNAL_SYSCALL_CALL (faccessat, AT_FDCWD, path, 0)
+                   == -ENOENT)
+            /* The file descriptor does not exist, but /proc is mounted.  */
+            err = EBADF;
+          /* Otherwise ENOENT comes from an invalid program
+             interpreter or some other property of the file.  */
+        }
     }
 #endif /* __ASSUME_EXECVEAT */
 
@@ -60,9 +69,10 @@  __do_fexecve (int fd, char *const argv[], char *const envp[])
 int
 fexecve (int fd, char *const argv[], char *const envp[])
 {
-  if (fd < 0 || argv == NULL || envp == NULL)
+  /* Avoid unexpected behavior of execveat with AT_FDCWD.  */
+  if (fd < 0)
     {
-      __set_errno (EINVAL);
+      __set_errno (EBADF);
       return -1;
     }