posix: Fix open file action for posix_spawn on Linux

Message ID 82eaa88f-ee29-4b78-33e9-61b27dfba9e6@linaro.org
State Dropped
Headers

Commit Message

Adhemerval Zanella Sept. 19, 2016, 12:41 p.m. UTC
  On 16/09/2016 19:58, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On posix_spawn open file action (issued by posix_spawn_file_actions_addopen)
>> POSIX states that if fildes was already an open file descriptor, it shall be
>> closed before the new file is openedi [1].  This avoid pontential issues when
>> posix_spawn plus addopen action is called with the process already at maximum
>> number of file descriptor opened and also for multiple actions on single-open
>> special paths (like /dev/watchdog).
>>
>> This fixes its behavior on Linux posix_spawn implementation and also adds
>> a tests to check for its behavior.
> 
> The patch appears to be incomplete:
> 
> ../sysdeps/unix/sysv/linux/spawni.c: In function ‘__spawni_child’:
> ../sysdeps/unix/sysv/linux/spawni.c:244:3: error: implicit declaration of function ‘fd_is_valid’ [-Werror=implicit-function-declaration]
> 

Thanks for catching it, below it the correct patch (with the function
defined):
  

Comments

Florian Weimer Sept. 19, 2016, 12:53 p.m. UTC | #1
On 09/19/2016 02:41 PM, Adhemerval Zanella wrote:

> +/* Return if file descriptor is opened.  */
> +static inline int
> +fd_is_valid (int fd)
> +{
> +  return fcntl_not_cancel_2 (fd, F_GETFD) != -1 || errno != EBADF;
> +}
> +
>  /* Function used in the clone call to setup the signals mask, posix_spawn
>     attributes, and file actions.  It run on its own stack (provided by the
>     posix_spawn call).  */
> @@ -219,6 +226,15 @@ __spawni_child (void *arguments)
>
>  	    case spawn_do_open:
>  	      {
> +		/* POSIX states that if fildes was already an open file descriptor,
> +		   it shall be closed before the new file is opened.  This avoid
> +		   pontential issues when posix_spawn plus addopen action is called
> +		   with the process already at maximum number of file descriptor
> +		   opened and also for multiple actions on single-open special
> +		   paths (like /dev/watchdog).  */
> +		if (fd_is_valid (action->action.open_action.fd))
> +		  close_not_cancel (action->action.open_action.fd);

It's not clear to me why you can't just close the file descriptor 
unconditionally.  It does not seem to matter whether you perform fcntl 
or close on an invalid file descriptor.

Thanks,
Florian
  
Andreas Schwab Sept. 19, 2016, 12:54 p.m. UTC | #2
On Sep 19 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index 866ad8b..3ad461f 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -114,6 +114,13 @@ maybe_script_execute (struct posix_spawn_args *args)
>      }
>  }
>  
> +/* Return if file descriptor is opened.  */
> +static inline int

FWIW, static already implies inline.

Andreas.
  
Adhemerval Zanella Sept. 19, 2016, 2:39 p.m. UTC | #3
On 19/09/2016 09:53, Florian Weimer wrote:
> On 09/19/2016 02:41 PM, Adhemerval Zanella wrote:
> 
>> +/* Return if file descriptor is opened.  */
>> +static inline int
>> +fd_is_valid (int fd)
>> +{
>> +  return fcntl_not_cancel_2 (fd, F_GETFD) != -1 || errno != EBADF;
>> +}
>> +
>>  /* Function used in the clone call to setup the signals mask, posix_spawn
>>     attributes, and file actions.  It run on its own stack (provided by the
>>     posix_spawn call).  */
>> @@ -219,6 +226,15 @@ __spawni_child (void *arguments)
>>
>>          case spawn_do_open:
>>            {
>> +        /* POSIX states that if fildes was already an open file descriptor,
>> +           it shall be closed before the new file is opened.  This avoid
>> +           pontential issues when posix_spawn plus addopen action is called
>> +           with the process already at maximum number of file descriptor
>> +           opened and also for multiple actions on single-open special
>> +           paths (like /dev/watchdog).  */
>> +        if (fd_is_valid (action->action.open_action.fd))
>> +          close_not_cancel (action->action.open_action.fd);
> 
> It's not clear to me why you can't just close the file descriptor unconditionally.  It does not seem to matter whether you perform fcntl or close on an invalid file descriptor.

Indeed it seems superfluous to check for file descriptor validity before
calling close, I will remove it.  With this changes, do you have any 
pending issues for the patch?
  
Rasmus Villemoes Sept. 20, 2016, 7:53 p.m. UTC | #4
On Mon, Sep 19 2016, Florian Weimer <fweimer@redhat.com> wrote:

> On 09/19/2016 02:41 PM, Adhemerval Zanella wrote:
>
>> +/* Return if file descriptor is opened.  */
>> +static inline int
>> +fd_is_valid (int fd)
>> +{
>> +  return fcntl_not_cancel_2 (fd, F_GETFD) != -1 || errno != EBADF;
>> +}
>> +
>>  /* Function used in the clone call to setup the signals mask, posix_spawn
>>     attributes, and file actions.  It run on its own stack (provided by the
>>     posix_spawn call).  */
>> @@ -219,6 +226,15 @@ __spawni_child (void *arguments)
>>
>>  	    case spawn_do_open:
>>  	      {
>> +		/* POSIX states that if fildes was already an open file descriptor,
>> +		   it shall be closed before the new file is opened.  This avoid
>> +		   pontential issues when posix_spawn plus addopen action is called
>> +		   with the process already at maximum number of file descriptor
>> +		   opened and also for multiple actions on single-open special
>> +		   paths (like /dev/watchdog).  */
>> +		if (fd_is_valid (action->action.open_action.fd))
>> +		  close_not_cancel (action->action.open_action.fd);
>
> It's not clear to me why you can't just close the file descriptor
> unconditionally.  It does not seem to matter whether you perform fcntl
> or close on an invalid file descriptor.

+1. Just do close+open+(if necessary dup2+close). In practice, I suppose
that actually ends up doing the fewest syscalls, assuming this is mostly
used for setting up the stdio fds.

Rasmus
  

Patch

diff --git a/posix/Makefile b/posix/Makefile
index 3a7719e..586d45b 100644
--- a/posix/Makefile
+++ b/posix/Makefile
@@ -94,7 +94,7 @@  tests		:= tstgetopt testfnm runtests runptests	     \
 xtests		:= bug-ga2
 ifeq (yes,$(build-shared))
 test-srcs	:= globtest
-tests           += wordexp-test tst-exec tst-spawn tst-spawn2
+tests           += wordexp-test tst-exec tst-spawn tst-spawn2 tst-spawn3
 endif
 tests-static	= tst-exec-static tst-spawn-static
 tests		+= $(tests-static)
diff --git a/posix/tst-spawn3.c b/posix/tst-spawn3.c
new file mode 100644
index 0000000..e66c491
--- /dev/null
+++ b/posix/tst-spawn3.c
@@ -0,0 +1,189 @@ 
+/* Check if posix add file actions.
+   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 <stdio.h>
+#include <spawn.h>
+#include <error.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/wait.h>
+#include <sys/resource.h>
+
+static int do_test (void);
+#define TEST_FUNCTION           do_test ()
+#include <test-skeleton.c>
+
+static int
+do_test (void)
+{
+  /* The test checks if posix_spawn open file action close the file descriptor
+     before opening a new one in case the input file descriptor is already
+     opened.  It does by exhausting all file descriptors on the process before
+     issue posix_spawn.  It then issues a posix_spawn for '/bin/sh echo $$'
+     and add two rules:
+
+     1. Redirect stdout to a temporary filepath
+     2. Redirect stderr to stdout
+
+     If the implementation does not close the file 1. will fail with
+     EMFILE.  */
+
+  struct rlimit rl;
+  int max_fd = 24;
+
+  /* Set maximum number of file descriptor to a low value to avoid open
+     too many files in environments where RLIMIT_NOFILE is large and to
+     limit the array size to track the opened file descriptors.  */
+
+  if (getrlimit (RLIMIT_NOFILE, &rl) == -1)
+    {
+      printf ("error: getrlimit RLIMIT_NOFILE failed");
+      exit (EXIT_FAILURE);
+    }
+
+  max_fd = (rl.rlim_cur < max_fd ? rl.rlim_cur : max_fd);
+  rl.rlim_cur = max_fd;
+  
+  if (setrlimit (RLIMIT_NOFILE, &rl) == 1)
+    {
+      printf ("error: setrlimit RLIMIT_NOFILE to %u failed", max_fd);
+      exit (EXIT_FAILURE);
+    }
+
+  /* Exhauste the file descriptor limit with temporary files.  */
+  int files[max_fd];
+  int nfiles = 0;
+  for (;;)
+    {
+      int fd = create_temp_file ("tst-spawn3.", NULL);
+      if (fd == -1)
+	{
+	  if (errno != EMFILE)
+	    {
+	      printf ("error: create_temp_file returned -1 with "
+		      "errno != EMFILE\n");
+	      exit (EXIT_FAILURE);
+	    }
+	  break;
+	}
+      files[nfiles++] = fd;
+    }
+
+  posix_spawn_file_actions_t a;
+  if (posix_spawn_file_actions_init (&a) != 0)
+    {
+      puts ("error: spawn_file_actions_init failed");
+      exit (EXIT_FAILURE);
+    }
+
+  /* Executes a /bin/sh echo $$ 2>&1 > /tmp/tst-spawn3.pid .  */
+  const char pidfile[] = "/tmp/tst-spawn3.pid";
+  if (posix_spawn_file_actions_addopen (&a, STDOUT_FILENO, pidfile, O_WRONLY |
+					O_CREAT | O_TRUNC, 0644) != 0)
+    {
+      puts ("error: spawn_file_actions_addopen failed");
+      exit (EXIT_FAILURE);
+    }
+
+  if (posix_spawn_file_actions_adddup2 (&a, STDOUT_FILENO, STDERR_FILENO) != 0)
+    {
+      puts ("error: spawn_file_actions_addclose");
+      exit (EXIT_FAILURE);
+    }
+
+  /* Since execve (called by posix_spawn) might require to open files to
+     actually execute the shell script, setup to close the temporary file
+     descriptors.  */
+  for (int i=0; i<nfiles; i++)
+    {
+      if (posix_spawn_file_actions_addclose (&a, files[i]))
+	{
+          printf ("error: posix_spawn_file_actions_addclose failed");
+	  exit (EXIT_FAILURE);
+	}
+    }
+
+  char *spawn_argv[] = { (char *) _PATH_BSHELL, (char *) "-c",
+			  (char *) "echo $$", NULL };
+  pid_t pid;
+  if (posix_spawn (&pid, _PATH_BSHELL, &a, NULL, spawn_argv, NULL) != 0)
+    {
+      puts ("error: posix_spawn failed");
+      exit (EXIT_FAILURE);
+    }
+
+  int status;
+  int err = waitpid (pid, &status, 0);
+  if (err != pid)
+    {
+      puts ("error: waitpid failed");
+      exit (EXIT_FAILURE);
+    }
+
+  /* Close the temporary files descriptor so it can check posix_spawn
+     output.  */
+  for (int i=0; i<nfiles; i++)
+    {
+      if (close (files[i]))
+	{
+	  printf ("error: close failed\n");
+	  exit (EXIT_FAILURE);
+	}
+    }
+
+  int pidfd = open (pidfile, O_RDONLY);
+  if (pidfd == -1)
+    {
+      printf ("error: open pidfile failed\n");
+      exit (EXIT_FAILURE);
+    }
+
+  char buf[64];
+  ssize_t n;
+  if ((n = read (pidfd, buf, sizeof (buf))) < 0)
+    {
+      printf ("error: read pidfile failed\n");
+      exit (EXIT_FAILURE);
+    }
+
+  unlink (pidfile);
+
+  /* We only expect to read the PID.  */
+  char *endp;
+  long int rpid = strtol (buf, &endp, 10);
+  if (*endp != '\n')
+    {
+      printf ("error: didn't parse whole line: \"%s\"\n", buf);
+      exit (EXIT_FAILURE);
+    }
+  if (endp == buf)
+    {
+      puts ("error: read empty line");
+      exit (EXIT_FAILURE);
+    }
+
+  if (rpid != pid)
+    {
+      printf ("error: found \"%s\", expected PID %ld\n", buf, (long int) pid);
+      exit (EXIT_FAILURE);
+    }
+
+  return 0;
+}
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index 866ad8b..3ad461f 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -114,6 +114,13 @@  maybe_script_execute (struct posix_spawn_args *args)
     }
 }
 
+/* Return if file descriptor is opened.  */
+static inline int
+fd_is_valid (int fd)
+{
+  return fcntl_not_cancel_2 (fd, F_GETFD) != -1 || errno != EBADF;
+}
+
 /* Function used in the clone call to setup the signals mask, posix_spawn
    attributes, and file actions.  It run on its own stack (provided by the
    posix_spawn call).  */
@@ -219,6 +226,15 @@  __spawni_child (void *arguments)
 
 	    case spawn_do_open:
 	      {
+		/* POSIX states that if fildes was already an open file descriptor,
+		   it shall be closed before the new file is opened.  This avoid
+		   pontential issues when posix_spawn plus addopen action is called
+		   with the process already at maximum number of file descriptor
+		   opened and also for multiple actions on single-open special
+		   paths (like /dev/watchdog).  */
+		if (fd_is_valid (action->action.open_action.fd))
+		  close_not_cancel (action->action.open_action.fd);
+
 		ret = open_not_cancel (action->action.open_action.path,
 				       action->action.
 				       open_action.oflag | O_LARGEFILE,