diff mbox

posix: Fix open file action for posix_spawn on Linux

Message ID 82eaa88f-ee29-4b78-33e9-61b27dfba9e6@linaro.org
State Accepted
Commit e83be730910c341f2f02ccc207b0586bb04fc21a
Headers show

Commit Message

Adhemerval Zanella Netto 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):

-- 
2.7.4

Comments

Adhemerval Zanella Netto Sept. 19, 2016, 2:39 p.m. UTC | #1
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?
Adhemerval Zanella Netto Sept. 19, 2016, 5:14 p.m. UTC | #2
On 19/09/2016 12:25, Florian Weimer wrote:
> On 09/19/2016 04:39 PM, Adhemerval Zanella wrote:

>>

>>

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

> 

> I'd rather suggest something like this, avoiding the close in the common case when the open call succeeds.


This approach has the issue that to 1. potentially trigger the dup + close
case more often because the new fd won't be equal to the expected one,
while closing the file descriptor first might avoid this issue; and 2.
to always trigger a failure in the behaviour described in the patch
(EMFILE).

> 

> The sysdeps/posix implementation would need a similar change.


Yes, that's why I state this patch only fixes the Linux part.
 
> However, the new test case consistently fails for me because of the pipe2 call in the Linux implementation.


Sorry, I should have stated that it requires the pipe removal
patch [1] on posix_spawn.

[1] https://sourceware.org/ml/libc-alpha/2016-09/msg00006.html
Adhemerval Zanella Netto Sept. 20, 2016, 8:04 p.m. UTC | #3
On 20/09/2016 16:57, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

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

>>>

>>> I'd rather suggest something like this, avoiding the close in the

>>> common case when the open call succeeds.

>>

>> This approach has the issue that to 1. potentially trigger the dup + close

>> case more often because the new fd won't be equal to the expected one,

>> while closing the file descriptor first might avoid this issue; and 2.

>> to always trigger a failure in the behaviour described in the patch

>> (EMFILE).

> 

> 1. seems a valid argument to me.  It may pollute the strace output in

> some cases, but it's true that it's a net savings in the most common

> cases. Suggestion withdrawn.

> 

> 2. I do not understand.

> 


Issuing open and check its result in a program that reached its maximum
file descriptor count (EMFILE situation such as the testcase aims to 
reproduce) will always trigger the exception case where it will require 
to close and retry the open (so instead of just close plus open, it will
be open failure, close, open).


>>> However, the new test case consistently fails for me because of the

>>> pipe2 call in the Linux implementation.

>>

>> Sorry, I should have stated that it requires the pipe removal

>> patch [1] on posix_spawn.

>>

>> [1] https://sourceware.org/ml/libc-alpha/2016-09/msg00006.html

> 

> Okay, we'll have to get that patch in first obviously.

> 


I think we just need to align which would be best error option for
unknown issues that do not set the errno (because the error code
relies on errno value).  Original patch suggested returning a
bogus value (EHOSTDOWN), where I would prefer just -1 to avoid
implement specific behaviour.
diff mbox

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,