[PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[adhemerval.zanella]

From: Adhemerval Zanella <adhemerval.zanella@linaro.org>

Austin Group issue #411 [1] proposes that posix_spawn file action
posix_spawn_file_actions_adddup2 resets the close-on-exec when
source and destination refer to same file descriptor.

It solves the issue on multi-thread applications which uses
close-on-exec as default, and want to hand-chose specifically
file descriptor to purposefully inherited into a child process.
Current approach to achieve this scenario is to use two adddup2 file
actions and a temporary file description which do not conflict with
any other, coupled with a close file action to avoid leaking the
temporary file descriptor.  This approach, besides being complex,
may fail with EMFILE/ENFILE file descriptor exaustion.

This can be more easily accomplished with an in-place removal of
FD_CLOEXEC.  Although the resulting adddup2 semantic is slight
different than dup2 (equal file descriptors should be handled as
no-op), the proposed possible solution are either more complex
(fcntl action which a limited set of operations) or results in
unrequired operations (dup3 which also returns EINVAL for same
file descriptor).

Checked on aarch64-linux-gnu.

	* posix/tst-spawn.c (do_prepare, handle_restart, do_test): Add
	posix_spawn_file_actions_adddup2 test to check O_CLOCEXEC reset.
	* sysdeps/unix/sysv/linux/spawni.c (__spawni_child): Add
	close-on-exec reset for adddup2 file action.
	* sysdeps/posix/spawni.c (__spawni_child): Likewise.

[1] http://austingroupbugs.net/view.php?id=411
---
 ChangeLog                        |  6 ++++
 posix/tst-spawn.c                | 47 +++++++++++++++++++++++++-------
 sysdeps/posix/spawni.c           | 21 ++++++++++++--
 sysdeps/unix/sysv/linux/spawni.c | 21 ++++++++++++--
 4 files changed, 79 insertions(+), 16 deletions(-)

diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
index 638ba1ba55..2354417020 100644
--- a/posix/tst-spawn.c
+++ b/posix/tst-spawn.c
@@ -40,17 +40,19 @@ static int restart;
 static char *name1;
 static char *name2;
 static char *name3;
+static char *name5;
 
 /* Descriptors for the temporary files.  */
 static int temp_fd1 = -1;
 static int temp_fd2 = -1;
 static int temp_fd3 = -1;
+static int temp_fd5 = -1;
 
 /* The contents of our files.  */
 static const char fd1string[] = "This file should get closed";
 static const char fd2string[] = "This file should stay opened";
 static const char fd3string[] = "This file will be opened";
-
+static const char fd5string[] = "This file should stay opened (O_CLOEXEC)";
 
 /* We have a preparation function.  */
 static void
@@ -63,24 +65,31 @@ do_prepare (int argc, char *argv[])
   TEST_VERIFY_EXIT ((temp_fd1 = create_temp_file ("spawn", &name1)) != -1);
   TEST_VERIFY_EXIT ((temp_fd2 = create_temp_file ("spawn", &name2)) != -1);
   TEST_VERIFY_EXIT ((temp_fd3 = create_temp_file ("spawn", &name3)) != -1);
+  TEST_VERIFY_EXIT ((temp_fd5 = create_temp_file ("spawn", &name5)) != -1);
+
+  int flags;
+  TEST_VERIFY_EXIT ((flags = fcntl (temp_fd5, F_GETFD, &flags)) != -1);
+  TEST_VERIFY_EXIT (fcntl (temp_fd5, F_SETFD, flags | FD_CLOEXEC) == 0);
 }
 #define PREPARE do_prepare
 
 static int
 handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
-		const char *fd4s, const char *name)
+		const char *fd4s, const char *name, const char *fd5s)
 {
   char buf[100];
   int fd1;
   int fd2;
   int fd3;
   int fd4;
+  int fd5;
 
   /* First get the descriptors.  */
   fd1 = atol (fd1s);
   fd2 = atol (fd2s);
   fd3 = atol (fd3s);
   fd4 = atol (fd4s);
+  fd5 = atol (fd5s);
 
   /* Sanity check.  */
   TEST_VERIFY_EXIT (fd1 != fd2);
@@ -89,6 +98,7 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
   TEST_VERIFY_EXIT (fd2 != fd3);
   TEST_VERIFY_EXIT (fd2 != fd4);
   TEST_VERIFY_EXIT (fd3 != fd4);
+  TEST_VERIFY_EXIT (fd4 != fd5);
 
   /* First the easy part: read from the file descriptor which is
      supposed to be open.  */
@@ -116,6 +126,10 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
   TEST_VERIFY_EXIT (read (fd1, buf, sizeof buf) == strlen (fd1string));
   TEST_VERIFY_EXIT (memcmp (fd1string, buf, strlen (fd1string)) == 0);
 
+  TEST_VERIFY_EXIT (lseek (fd5, 0, SEEK_SET) == 0);
+  TEST_VERIFY_EXIT (read (fd5, buf, sizeof buf) == strlen (fd5string));
+  TEST_VERIFY_EXIT (memcmp (fd5string, buf, strlen (fd5string)) == 0);
+
   return 0;
 }
 
@@ -131,6 +145,7 @@ do_test (int argc, char *argv[])
   char fd2name[18];
   char fd3name[18];
   char fd4name[18];
+  char fd5name[18];
   char *name3_copy;
   char *spargv[12];
   int i;
@@ -141,21 +156,24 @@ do_test (int argc, char *argv[])
        + "--library-path"	optional
        + the library path	optional
        + the application name
-     - five parameters left if called through re-execution
+     - six parameters left if called through re-execution
        + file descriptor number which is supposed to be closed
        + the open file descriptor
        + the newly opened file descriptor
-       + thhe duped second descriptor
+       + the duped second descriptor
        + the name of the closed descriptor
+       + the duped fourth dile descriptor which O_CLOEXEC should be
+	 remove by adddup2.
   */
-  if (argc != (restart ? 6 : 2) && argc != (restart ? 6 : 5))
+  if (argc != (restart ? 7 : 2) && argc != (restart ? 7 : 5))
     FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
 
   if (restart)
-    return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5]);
+    return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5],
+			   argv[6]);
 
-  /* Prepare the test.  We are creating two files: one which file descriptor
-     will be marked with FD_CLOEXEC, another which is not.  */
+  /* Prepare the test.  We are creating four files: two which file descriptor
+     will be marked with FD_CLOEXEC, another which is not  */
 
   /* Write something in the files.  */
   TEST_VERIFY_EXIT (write (temp_fd1, fd1string, strlen (fd1string))
@@ -164,6 +182,8 @@ do_test (int argc, char *argv[])
 		    == strlen (fd2string));
   TEST_VERIFY_EXIT (write (temp_fd3, fd3string, strlen (fd3string))
 		    == strlen (fd3string));
+  TEST_VERIFY_EXIT (write (temp_fd5, fd5string, strlen (fd5string))
+		    == strlen (fd5string));
 
   /* Close the third file.  It'll be opened by `spawn'.  */
   close (temp_fd3);
@@ -183,17 +203,23 @@ do_test (int argc, char *argv[])
   memset (name3_copy, 'X', strlen (name3_copy));
 
   /* We dup the second descriptor.  */
-  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
+  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, MAX (temp_fd3, temp_fd5)))) + 1;
   TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd2,
 						      fd4) == 0);
 
+  /* We clear the O_CLOEXEC on fourth descriptor, so it should be
+     stay open on child.  */
+  TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd5,
+						      temp_fd5) == 0);
+
   /* Now spawn the process.  */
   snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
   snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
   snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
   snprintf (fd4name, sizeof fd4name, "%d", fd4);
+  snprintf (fd5name, sizeof fd5name, "%d", temp_fd5);
 
-  for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
+  for (i = 0; i < (argc == (restart ? 7 : 5) ? 4 : 1); i++)
     spargv[i] = argv[i + 1];
   spargv[i++] = (char *) "--direct";
   spargv[i++] = (char *) "--restart";
@@ -202,6 +228,7 @@ do_test (int argc, char *argv[])
   spargv[i++] = fd3name;
   spargv[i++] = fd4name;
   spargv[i++] = name1;
+  spargv[i++] = fd5name;
   spargv[i] = NULL;
 
   TEST_VERIFY_EXIT (posix_spawn (&pid, argv[1], &actions, NULL, spargv,
diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
index b138ab4393..d322db5c19 100644
--- a/sysdeps/posix/spawni.c
+++ b/sysdeps/posix/spawni.c
@@ -204,10 +204,25 @@ __spawni_child (void *arguments)
 	      break;
 
 	    case spawn_do_dup2:
-	      if (__dup2 (action->action.dup2_action.fd,
-			  action->action.dup2_action.newfd)
+	      /* Austin Group issue #411 requires adddup2 action with source
+		 and destination being equal to remove close-on-exec flag.  */
+	      if (action->action.dup2_action.fd
 		  != action->action.dup2_action.newfd)
-		goto fail;
+		{
+		  if (__dup2 (action->action.dup2_action.fd,
+			     action->action.dup2_action.newfd)
+		      != action->action.dup2_action.newfd)
+		    goto fail;
+		}
+	      else
+		{
+		  int fd = action->action.dup2_action.newfd;
+		  int flags = __fcntl (fd, F_GETFD, 0);
+		  if (flags == -1)
+		    goto fail;
+		  if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
+		    goto fail;
+		}
 	      break;
 	    }
 	}
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index 85239cedbf..b2304ffe60 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -253,10 +253,25 @@ __spawni_child (void *arguments)
 	      break;
 
 	    case spawn_do_dup2:
-	      if (__dup2 (action->action.dup2_action.fd,
-			  action->action.dup2_action.newfd)
+	      /* Austin Group issue #411 requires adddup2 action with source
+		 and destination being equal to remove close-on-exec flag.  */
+	      if (action->action.dup2_action.fd
 		  != action->action.dup2_action.newfd)
-		goto fail;
+		{
+		  if (__dup2 (action->action.dup2_action.fd,
+			     action->action.dup2_action.newfd)
+		      != action->action.dup2_action.newfd)
+		    goto fail;
+		}
+	      else
+		{
+		  int fd = action->action.dup2_action.newfd;
+		  int flags = __fcntl (fd, F_GETFD, 0);
+		  if (flags == -1)
+		    goto fail;
+		  if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
+		    goto fail;
+		}
 	      break;
 	    }
 	}
-- 
2.17.1

[PATCH 1/2] Use libsupport for tst-spawn.c

[adhemerval.zanella]

From: Adhemerval Zanella <adhemerval.zanella@linaro.org>

No function changes is done.  Checked on x86_64-linux-gnu.

	* posix/tst-spawn.c (do_prepare, handle_restart, do_test):
	Use libsupport.
---
 ChangeLog         |   5 ++
 posix/tst-spawn.c | 212 ++++++++++++++++++++--------------------------
 2 files changed, 95 insertions(+), 122 deletions(-)

diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
index 7c785c430c..638ba1ba55 100644
--- a/posix/tst-spawn.c
+++ b/posix/tst-spawn.c
@@ -17,35 +17,25 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <stdio.h>
+#include <getopt.h>
 #include <errno.h>
 #include <error.h>
 #include <fcntl.h>
 #include <spawn.h>
 #include <stdlib.h>
 #include <string.h>
-#include <wait.h>
 #include <sys/param.h>
 #include <support/check.h>
 #include <support/xunistd.h>
-
+#include <support/temp_file.h>
 
 /* Nonzero if the program gets called via `exec'.  */
 static int restart;
 
-
 #define CMDLINE_OPTIONS \
   { "restart", no_argument, &restart, 1 },
 
-/* Prototype for our test function.  */
-extern void do_prepare (int argc, char *argv[]);
-extern int do_test (int argc, char *argv[]);
-
-/* We have a preparation function.  */
-#define PREPARE do_prepare
-
-#include "../test-skeleton.c"
-
-
 /* Name of the temporary files.  */
 static char *name1;
 static char *name2;
@@ -63,20 +53,18 @@ static const char fd3string[] = "This file will be opened";
 
 
 /* We have a preparation function.  */
-void
+static void
 do_prepare (int argc, char *argv[])
 {
   /* We must not open any files in the restart case.  */
   if (restart)
     return;
 
-  temp_fd1 = create_temp_file ("spawn", &name1);
-  temp_fd2 = create_temp_file ("spawn", &name2);
-  temp_fd3 = create_temp_file ("spawn", &name3);
-  if (temp_fd1 < 0 || temp_fd2 < 0 || temp_fd3 < 0)
-    exit (1);
+  TEST_VERIFY_EXIT ((temp_fd1 = create_temp_file ("spawn", &name1)) != -1);
+  TEST_VERIFY_EXIT ((temp_fd2 = create_temp_file ("spawn", &name2)) != -1);
+  TEST_VERIFY_EXIT ((temp_fd3 = create_temp_file ("spawn", &name3)) != -1);
 }
-
+#define PREPARE do_prepare
 
 static int
 handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
@@ -95,64 +83,44 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
   fd4 = atol (fd4s);
 
   /* Sanity check.  */
-  if (fd1 == fd2)
-    error (EXIT_FAILURE, 0, "value of fd1 and fd2 is the same");
-  if (fd1 == fd3)
-    error (EXIT_FAILURE, 0, "value of fd1 and fd3 is the same");
-  if (fd1 == fd4)
-    error (EXIT_FAILURE, 0, "value of fd1 and fd4 is the same");
-  if (fd2 == fd3)
-    error (EXIT_FAILURE, 0, "value of fd2 and fd3 is the same");
-  if (fd2 == fd4)
-    error (EXIT_FAILURE, 0, "value of fd2 and fd4 is the same");
-  if (fd3 == fd4)
-    error (EXIT_FAILURE, 0, "value of fd3 and fd4 is the same");
+  TEST_VERIFY_EXIT (fd1 != fd2);
+  TEST_VERIFY_EXIT (fd1 != fd3);
+  TEST_VERIFY_EXIT (fd1 != fd4);
+  TEST_VERIFY_EXIT (fd2 != fd3);
+  TEST_VERIFY_EXIT (fd2 != fd4);
+  TEST_VERIFY_EXIT (fd3 != fd4);
 
   /* First the easy part: read from the file descriptor which is
      supposed to be open.  */
-  if (lseek (fd2, 0, SEEK_CUR) != strlen (fd2string))
-    error (EXIT_FAILURE, errno, "file 2 not in right position");
+  TEST_VERIFY_EXIT (lseek (fd2, 0, SEEK_CUR) == strlen (fd2string));
   /* The duped descriptor must have the same position.  */
-  if (lseek (fd4, 0, SEEK_CUR) != strlen (fd2string))
-    error (EXIT_FAILURE, errno, "file 4 not in right position");
-  if (lseek (fd2, 0, SEEK_SET) != 0)
-    error (EXIT_FAILURE, 0, "cannot reset position in file 2");
-  if (lseek (fd4, 0, SEEK_CUR) != 0)
-    error (EXIT_FAILURE, errno, "file 4 not set back, too");
-  if (read (fd2, buf, sizeof buf) != strlen (fd2string))
-    error (EXIT_FAILURE, 0, "cannot read file 2");
-  if (memcmp (fd2string, buf, strlen (fd2string)) != 0)
-    error (EXIT_FAILURE, 0, "file 2 does not match");
+  TEST_VERIFY_EXIT (lseek (fd4, 0, SEEK_CUR) == strlen (fd2string));
+  TEST_VERIFY_EXIT (lseek (fd2, 0, SEEK_SET) == 0);
+  TEST_VERIFY_EXIT (lseek (fd4, 0, SEEK_CUR) == 0);
+  TEST_VERIFY_EXIT (read (fd2, buf, sizeof buf) == strlen (fd2string));
+  TEST_VERIFY_EXIT (memcmp (fd2string, buf, strlen (fd2string)) == 0);
 
   /* Now read from the third file.  */
-  if (read (fd3, buf, sizeof buf) != strlen (fd3string))
-    error (EXIT_FAILURE, 0, "cannot read file 3");
-  if (memcmp (fd3string, buf, strlen (fd3string)) != 0)
-    error (EXIT_FAILURE, 0, "file 3 does not match");
+  TEST_VERIFY_EXIT (read (fd3, buf, sizeof buf) == strlen (fd3string));
+  TEST_VERIFY_EXIT (memcmp (fd3string, buf, strlen (fd3string)) == 0);
   /* Try to write to the file.  This should not be allowed.  */
-  if (write (fd3, "boo!", 4) != -1 || errno != EBADF)
-    error (EXIT_FAILURE, 0, "file 3 is writable");
+  TEST_VERIFY_EXIT (write (fd3, "boo!", 4) == -1 && errno == EBADF);
 
   /* Now try to read the first file.  First make sure it is not opened.  */
-  if (lseek (fd1, 0, SEEK_CUR) != (off_t) -1 || errno != EBADF)
-    error (EXIT_FAILURE, 0, "file 1 (%d) is not closed", fd1);
+  TEST_VERIFY_EXIT (lseek (fd1, 0, SEEK_CUR) == (off_t) -1
+		    && errno == EBADF);
 
   /* Now open the file and read it.  */
-  fd1 = open (name, O_RDONLY);
-  if (fd1 == -1)
-    error (EXIT_FAILURE, errno,
-	   "cannot open first file \"%s\" for verification", name);
+  TEST_VERIFY_EXIT ((fd1 = open (name, O_RDONLY)) != -1);
 
-  if (read (fd1, buf, sizeof buf) != strlen (fd1string))
-    error (EXIT_FAILURE, errno, "cannot read file 1");
-  if (memcmp (fd1string, buf, strlen (fd1string)) != 0)
-    error (EXIT_FAILURE, 0, "file 1 does not match");
+  TEST_VERIFY_EXIT (read (fd1, buf, sizeof buf) == strlen (fd1string));
+  TEST_VERIFY_EXIT (memcmp (fd1string, buf, strlen (fd1string)) == 0);
 
   return 0;
 }
 
 
-int
+static int
 do_test (int argc, char *argv[])
 {
   pid_t pid;
@@ -181,7 +149,7 @@ do_test (int argc, char *argv[])
        + the name of the closed descriptor
   */
   if (argc != (restart ? 6 : 2) && argc != (restart ? 6 : 5))
-    error (EXIT_FAILURE, 0, "wrong number of arguments (%d)", argc);
+    FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
 
   if (restart)
     return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5]);
@@ -189,66 +157,63 @@ do_test (int argc, char *argv[])
   /* Prepare the test.  We are creating two files: one which file descriptor
      will be marked with FD_CLOEXEC, another which is not.  */
 
-   /* Write something in the files.  */
-   if (write (temp_fd1, fd1string, strlen (fd1string)) != strlen (fd1string))
-     error (EXIT_FAILURE, errno, "cannot write to first file");
-   if (write (temp_fd2, fd2string, strlen (fd2string)) != strlen (fd2string))
-     error (EXIT_FAILURE, errno, "cannot write to second file");
-   if (write (temp_fd3, fd3string, strlen (fd3string)) != strlen (fd3string))
-     error (EXIT_FAILURE, errno, "cannot write to third file");
-
-   /* Close the third file.  It'll be opened by `spawn'.  */
-   close (temp_fd3);
-
-   /* Tell `spawn' what to do.  */
-   if (posix_spawn_file_actions_init (&actions) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_init");
-   /* Close `temp_fd1'.  */
-   if (posix_spawn_file_actions_addclose (&actions, temp_fd1) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_addclose");
-   /* We want to open the third file.  */
-   name3_copy = strdup (name3);
-   if (name3_copy == NULL)
-     error (EXIT_FAILURE, errno, "strdup");
-   if (posix_spawn_file_actions_addopen (&actions, temp_fd3, name3_copy,
-					 O_RDONLY, 0666) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_addopen");
-   /* Overwrite the name to check that a copy has been made.  */
-   memset (name3_copy, 'X', strlen (name3_copy));
-
-   /* We dup the second descriptor.  */
-   fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
-   if (posix_spawn_file_actions_adddup2 (&actions, temp_fd2, fd4) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_adddup2");
-
-   /* Now spawn the process.  */
-   snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
-   snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
-   snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
-   snprintf (fd4name, sizeof fd4name, "%d", fd4);
-
-   for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
-     spargv[i] = argv[i + 1];
-   spargv[i++] = (char *) "--direct";
-   spargv[i++] = (char *) "--restart";
-   spargv[i++] = fd1name;
-   spargv[i++] = fd2name;
-   spargv[i++] = fd3name;
-   spargv[i++] = fd4name;
-   spargv[i++] = name1;
-   spargv[i] = NULL;
-
-   if (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn");
-
-   /* Same test but with a NULL pid argument.  */
-   if (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn");
-
-   /* Cleanup.  */
-   if (posix_spawn_file_actions_destroy (&actions) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_destroy");
-   free (name3_copy);
+  /* Write something in the files.  */
+  TEST_VERIFY_EXIT (write (temp_fd1, fd1string, strlen (fd1string))
+		    == strlen (fd1string));
+  TEST_VERIFY_EXIT (write (temp_fd2, fd2string, strlen (fd2string))
+		    == strlen (fd2string));
+  TEST_VERIFY_EXIT (write (temp_fd3, fd3string, strlen (fd3string))
+		    == strlen (fd3string));
+
+  /* Close the third file.  It'll be opened by `spawn'.  */
+  close (temp_fd3);
+
+  /* Tell `spawn' what to do.  */
+  TEST_VERIFY_EXIT (posix_spawn_file_actions_init (&actions) == 0);
+  /* Close `temp_fd1'.  */
+  TEST_VERIFY_EXIT (posix_spawn_file_actions_addclose (&actions, temp_fd1)
+		    == 0);
+  /* We want to open the third file.  */
+  TEST_VERIFY_EXIT ((name3_copy = strdup (name3)) != NULL);
+  TEST_VERIFY_EXIT (posix_spawn_file_actions_addopen (&actions, temp_fd3,
+						      name3_copy,
+						      O_RDONLY, 0666)
+		    == 0);
+  /* Overwrite the name to check that a copy has been made.  */
+  memset (name3_copy, 'X', strlen (name3_copy));
+
+  /* We dup the second descriptor.  */
+  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
+  TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd2,
+						      fd4) == 0);
+
+  /* Now spawn the process.  */
+  snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
+  snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
+  snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
+  snprintf (fd4name, sizeof fd4name, "%d", fd4);
+
+  for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
+    spargv[i] = argv[i + 1];
+  spargv[i++] = (char *) "--direct";
+  spargv[i++] = (char *) "--restart";
+  spargv[i++] = fd1name;
+  spargv[i++] = fd2name;
+  spargv[i++] = fd3name;
+  spargv[i++] = fd4name;
+  spargv[i++] = name1;
+  spargv[i] = NULL;
+
+  TEST_VERIFY_EXIT (posix_spawn (&pid, argv[1], &actions, NULL, spargv,
+				 environ) == 0);
+
+  /* Same test but with a NULL pid argument.  */
+  TEST_VERIFY_EXIT (posix_spawn (NULL, argv[1], &actions, NULL, spargv,
+		    environ) == 0);
+
+  /* Cleanup.  */
+  TEST_VERIFY_EXIT (posix_spawn_file_actions_destroy (&actions) == 0);
+  free (name3_copy);
 
   /* Wait for the children.  */
   TEST_VERIFY (xwaitpid (pid, &status, 0) == pid);
@@ -263,3 +228,6 @@ do_test (int argc, char *argv[])
 
   return 0;
 }
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
-- 
2.17.1

Re: [PATCH 1/2] Use libsupport for tst-spawn.c

[Florian Weimer]

* adhemerval zanella:

>    /* First the easy part: read from the file descriptor which is
>       supposed to be open.  */
> +  TEST_VERIFY_EXIT (lseek (fd2, 0, SEEK_CUR) == strlen (fd2string));
>    /* The duped descriptor must have the same position.  */
> +  TEST_VERIFY_EXIT (lseek (fd4, 0, SEEK_CUR) == strlen (fd2string));
> +  TEST_VERIFY_EXIT (lseek (fd2, 0, SEEK_SET) == 0);
> +  TEST_VERIFY_EXIT (lseek (fd4, 0, SEEK_CUR) == 0);
> +  TEST_VERIFY_EXIT (read (fd2, buf, sizeof buf) == strlen (fd2string));

These can use TEST_COMPARE and xlseek.

> +  TEST_VERIFY_EXIT (memcmp (fd2string, buf, strlen (fd2string)) == 0);

Use TEST_COMPARE_BLOB?

>    /* Now read from the third file.  */
> +  TEST_VERIFY_EXIT (read (fd3, buf, sizeof buf) == strlen (fd3string));
> +  TEST_VERIFY_EXIT (memcmp (fd3string, buf, strlen (fd3string)) == 0);
>    /* Try to write to the file.  This should not be allowed.  */
> +  TEST_VERIFY_EXIT (write (fd3, "boo!", 4) == -1 && errno == EBADF);
>  
>    /* Now try to read the first file.  First make sure it is not opened.  */
> +  TEST_VERIFY_EXIT (lseek (fd1, 0, SEEK_CUR) == (off_t) -1
> +		    && errno == EBADF);

Use too TEST_COMPARE calls?

>  
>    /* Now open the file and read it.  */
> +  TEST_VERIFY_EXIT ((fd1 = open (name, O_RDONLY)) != -1);

You can use xopen.

> +  TEST_VERIFY_EXIT (read (fd1, buf, sizeof buf) == strlen (fd1string));
> +  TEST_VERIFY_EXIT (memcmp (fd1string, buf, strlen (fd1string)) == 0);

Likewise TEST_COMPARE.

More instances below, sorry.

Thanks,
Florian

Re: [PATCH 1/2] Use libsupport for tst-spawn.c

[Adhemerval Zanella]

On 19/09/2018 22:18, Florian Weimer wrote:
> * adhemerval zanella:
> 
>>    /* First the easy part: read from the file descriptor which is
>>       supposed to be open.  */
>> +  TEST_VERIFY_EXIT (lseek (fd2, 0, SEEK_CUR) == strlen (fd2string));
>>    /* The duped descriptor must have the same position.  */
>> +  TEST_VERIFY_EXIT (lseek (fd4, 0, SEEK_CUR) == strlen (fd2string));
>> +  TEST_VERIFY_EXIT (lseek (fd2, 0, SEEK_SET) == 0);
>> +  TEST_VERIFY_EXIT (lseek (fd4, 0, SEEK_CUR) == 0);
>> +  TEST_VERIFY_EXIT (read (fd2, buf, sizeof buf) == strlen (fd2string));
> 
> These can use TEST_COMPARE and xlseek.
> 
>> +  TEST_VERIFY_EXIT (memcmp (fd2string, buf, strlen (fd2string)) == 0);
> 
> Use TEST_COMPARE_BLOB?
> 
>>    /* Now read from the third file.  */
>> +  TEST_VERIFY_EXIT (read (fd3, buf, sizeof buf) == strlen (fd3string));
>> +  TEST_VERIFY_EXIT (memcmp (fd3string, buf, strlen (fd3string)) == 0);
>>    /* Try to write to the file.  This should not be allowed.  */
>> +  TEST_VERIFY_EXIT (write (fd3, "boo!", 4) == -1 && errno == EBADF);
>>  
>>    /* Now try to read the first file.  First make sure it is not opened.  */
>> +  TEST_VERIFY_EXIT (lseek (fd1, 0, SEEK_CUR) == (off_t) -1
>> +		    && errno == EBADF);
> 
> Use too TEST_COMPARE calls?
> 
>>  
>>    /* Now open the file and read it.  */
>> +  TEST_VERIFY_EXIT ((fd1 = open (name, O_RDONLY)) != -1);
> 
> You can use xopen.
> 
>> +  TEST_VERIFY_EXIT (read (fd1, buf, sizeof buf) == strlen (fd1string));
>> +  TEST_VERIFY_EXIT (memcmp (fd1string, buf, strlen (fd1string)) == 0);
> 
> Likewise TEST_COMPARE.
> 
> More instances below, sorry.

No worries, I need to catch up with recent libsupport functionalities.
Below it is an updated patch, which I think it replaces with better
libsupport interfaces:

diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
index 7c785c430c..d3f3c652ff 100644
--- a/posix/tst-spawn.c
+++ b/posix/tst-spawn.c
@@ -17,16 +17,18 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <stdio.h>
+#include <getopt.h>
 #include <errno.h>
 #include <error.h>
 #include <fcntl.h>
 #include <spawn.h>
 #include <stdlib.h>
 #include <string.h>
-#include <wait.h>
 #include <sys/param.h>
 #include <support/check.h>
 #include <support/xunistd.h>
+#include <support/temp_file.h>
 
 
 /* Nonzero if the program gets called via `exec'.  */
@@ -36,16 +38,6 @@ static int restart;
 #define CMDLINE_OPTIONS \
   { "restart", no_argument, &restart, 1 },
 
-/* Prototype for our test function.  */
-extern void do_prepare (int argc, char *argv[]);
-extern int do_test (int argc, char *argv[]);
-
-/* We have a preparation function.  */
-#define PREPARE do_prepare
-
-#include "../test-skeleton.c"
-
-
 /* Name of the temporary files.  */
 static char *name1;
 static char *name2;
@@ -63,19 +55,18 @@ static const char fd3string[] = "This file will be opened";
 
 
 /* We have a preparation function.  */
-void
+static void
 do_prepare (int argc, char *argv[])
 {
   /* We must not open any files in the restart case.  */
   if (restart)
     return;
 
-  temp_fd1 = create_temp_file ("spawn", &name1);
-  temp_fd2 = create_temp_file ("spawn", &name2);
-  temp_fd3 = create_temp_file ("spawn", &name3);
-  if (temp_fd1 < 0 || temp_fd2 < 0 || temp_fd3 < 0)
-    exit (1);
+  TEST_VERIFY_EXIT ((temp_fd1 = create_temp_file ("spawn", &name1)) != -1);
+  TEST_VERIFY_EXIT ((temp_fd2 = create_temp_file ("spawn", &name2)) != -1);
+  TEST_VERIFY_EXIT ((temp_fd3 = create_temp_file ("spawn", &name3)) != -1);
 }
+#define PREPARE do_prepare
 
 
 static int
@@ -95,64 +86,45 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
   fd4 = atol (fd4s);
 
   /* Sanity check.  */
-  if (fd1 == fd2)
-    error (EXIT_FAILURE, 0, "value of fd1 and fd2 is the same");
-  if (fd1 == fd3)
-    error (EXIT_FAILURE, 0, "value of fd1 and fd3 is the same");
-  if (fd1 == fd4)
-    error (EXIT_FAILURE, 0, "value of fd1 and fd4 is the same");
-  if (fd2 == fd3)
-    error (EXIT_FAILURE, 0, "value of fd2 and fd3 is the same");
-  if (fd2 == fd4)
-    error (EXIT_FAILURE, 0, "value of fd2 and fd4 is the same");
-  if (fd3 == fd4)
-    error (EXIT_FAILURE, 0, "value of fd3 and fd4 is the same");
+  TEST_VERIFY_EXIT (fd1 != fd2);
+  TEST_VERIFY_EXIT (fd1 != fd3);
+  TEST_VERIFY_EXIT (fd1 != fd4);
+  TEST_VERIFY_EXIT (fd2 != fd3);
+  TEST_VERIFY_EXIT (fd2 != fd4);
+  TEST_VERIFY_EXIT (fd3 != fd4);
 
   /* First the easy part: read from the file descriptor which is
      supposed to be open.  */
-  if (lseek (fd2, 0, SEEK_CUR) != strlen (fd2string))
-    error (EXIT_FAILURE, errno, "file 2 not in right position");
+  TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), strlen (fd2string));
   /* The duped descriptor must have the same position.  */
-  if (lseek (fd4, 0, SEEK_CUR) != strlen (fd2string))
-    error (EXIT_FAILURE, errno, "file 4 not in right position");
-  if (lseek (fd2, 0, SEEK_SET) != 0)
-    error (EXIT_FAILURE, 0, "cannot reset position in file 2");
-  if (lseek (fd4, 0, SEEK_CUR) != 0)
-    error (EXIT_FAILURE, errno, "file 4 not set back, too");
-  if (read (fd2, buf, sizeof buf) != strlen (fd2string))
-    error (EXIT_FAILURE, 0, "cannot read file 2");
-  if (memcmp (fd2string, buf, strlen (fd2string)) != 0)
-    error (EXIT_FAILURE, 0, "file 2 does not match");
+  TEST_COMPARE (lseek (fd4, 0, SEEK_CUR), strlen (fd2string));
+  TEST_COMPARE (lseek (fd2, 0, SEEK_SET), 0);
+  TEST_COMPARE (lseek (fd4, 0, SEEK_CUR), 0);
+  TEST_COMPARE (read (fd2, buf, sizeof buf), strlen (fd2string));
+  TEST_COMPARE_BLOB (fd2string, strlen (fd2string), buf, strlen (buf));
 
   /* Now read from the third file.  */
-  if (read (fd3, buf, sizeof buf) != strlen (fd3string))
-    error (EXIT_FAILURE, 0, "cannot read file 3");
-  if (memcmp (fd3string, buf, strlen (fd3string)) != 0)
-    error (EXIT_FAILURE, 0, "file 3 does not match");
+  TEST_COMPARE (read (fd3, buf, sizeof buf), strlen (fd3string));
+  TEST_COMPARE_BLOB (fd3string, strlen (fd3string), buf, strlen (buf));
   /* Try to write to the file.  This should not be allowed.  */
-  if (write (fd3, "boo!", 4) != -1 || errno != EBADF)
-    error (EXIT_FAILURE, 0, "file 3 is writable");
+  TEST_COMPARE (write (fd3, "boo!", 4), -1);
+  TEST_COMPARE (errno, EBADF);
 
   /* Now try to read the first file.  First make sure it is not opened.  */
-  if (lseek (fd1, 0, SEEK_CUR) != (off_t) -1 || errno != EBADF)
-    error (EXIT_FAILURE, 0, "file 1 (%d) is not closed", fd1);
+  TEST_COMPARE (xlseek (fd1, 0, SEEK_CUR), (off_t) -1);
+  TEST_COMPARE (errno, EBADF);
 
   /* Now open the file and read it.  */
-  fd1 = open (name, O_RDONLY);
-  if (fd1 == -1)
-    error (EXIT_FAILURE, errno,
-	   "cannot open first file \"%s\" for verification", name);
+  TEST_VERIFY_EXIT ((fd1 = open (name, O_RDONLY)) != -1);
 
-  if (read (fd1, buf, sizeof buf) != strlen (fd1string))
-    error (EXIT_FAILURE, errno, "cannot read file 1");
-  if (memcmp (fd1string, buf, strlen (fd1string)) != 0)
-    error (EXIT_FAILURE, 0, "file 1 does not match");
+  TEST_COMPARE (read (fd1, buf, sizeof buf), strlen (fd1string));
+  TEST_COMPARE_BLOB (fd1string, strlen (fd1string), buf, strlen (buf));
 
   return 0;
 }
 
 
-int
+static int
 do_test (int argc, char *argv[])
 {
   pid_t pid;
@@ -181,7 +153,7 @@ do_test (int argc, char *argv[])
        + the name of the closed descriptor
   */
   if (argc != (restart ? 6 : 2) && argc != (restart ? 6 : 5))
-    error (EXIT_FAILURE, 0, "wrong number of arguments (%d)", argc);
+    FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
 
   if (restart)
     return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5]);
@@ -189,77 +161,76 @@ do_test (int argc, char *argv[])
   /* Prepare the test.  We are creating two files: one which file descriptor
      will be marked with FD_CLOEXEC, another which is not.  */
 
-   /* Write something in the files.  */
-   if (write (temp_fd1, fd1string, strlen (fd1string)) != strlen (fd1string))
-     error (EXIT_FAILURE, errno, "cannot write to first file");
-   if (write (temp_fd2, fd2string, strlen (fd2string)) != strlen (fd2string))
-     error (EXIT_FAILURE, errno, "cannot write to second file");
-   if (write (temp_fd3, fd3string, strlen (fd3string)) != strlen (fd3string))
-     error (EXIT_FAILURE, errno, "cannot write to third file");
-
-   /* Close the third file.  It'll be opened by `spawn'.  */
-   close (temp_fd3);
-
-   /* Tell `spawn' what to do.  */
-   if (posix_spawn_file_actions_init (&actions) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_init");
-   /* Close `temp_fd1'.  */
-   if (posix_spawn_file_actions_addclose (&actions, temp_fd1) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_addclose");
-   /* We want to open the third file.  */
-   name3_copy = strdup (name3);
-   if (name3_copy == NULL)
-     error (EXIT_FAILURE, errno, "strdup");
-   if (posix_spawn_file_actions_addopen (&actions, temp_fd3, name3_copy,
-					 O_RDONLY, 0666) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_addopen");
-   /* Overwrite the name to check that a copy has been made.  */
-   memset (name3_copy, 'X', strlen (name3_copy));
-
-   /* We dup the second descriptor.  */
-   fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
-   if (posix_spawn_file_actions_adddup2 (&actions, temp_fd2, fd4) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_adddup2");
-
-   /* Now spawn the process.  */
-   snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
-   snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
-   snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
-   snprintf (fd4name, sizeof fd4name, "%d", fd4);
-
-   for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
-     spargv[i] = argv[i + 1];
-   spargv[i++] = (char *) "--direct";
-   spargv[i++] = (char *) "--restart";
-   spargv[i++] = fd1name;
-   spargv[i++] = fd2name;
-   spargv[i++] = fd3name;
-   spargv[i++] = fd4name;
-   spargv[i++] = name1;
-   spargv[i] = NULL;
-
-   if (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn");
-
-   /* Same test but with a NULL pid argument.  */
-   if (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn");
-
-   /* Cleanup.  */
-   if (posix_spawn_file_actions_destroy (&actions) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_destroy");
-   free (name3_copy);
+  /* Write something in the files.  */
+  TEST_COMPARE (write (temp_fd1, fd1string, strlen (fd1string)),
+		strlen (fd1string));
+  TEST_COMPARE (write (temp_fd2, fd2string, strlen (fd2string)),
+		strlen (fd2string));
+  TEST_COMPARE (write (temp_fd3, fd3string, strlen (fd3string)),
+		strlen (fd3string));
+
+  /* Close the third file.  It'll be opened by `spawn'.  */
+  close (temp_fd3);
+
+  /* Tell `spawn' what to do.  */
+  TEST_COMPARE (posix_spawn_file_actions_init (&actions), 0);
+  /* Close `temp_fd1'.  */
+  TEST_COMPARE (posix_spawn_file_actions_addclose (&actions, temp_fd1), 0);
+  /* We want to open the third file.  */
+  TEST_VERIFY_EXIT ((name3_copy = strdup (name3)) != NULL);
+  TEST_COMPARE (posix_spawn_file_actions_addopen (&actions, temp_fd3,
+						  name3_copy,
+						  O_RDONLY, 0666),
+		0);
+  /* Overwrite the name to check that a copy has been made.  */
+  memset (name3_copy, 'X', strlen (name3_copy));
+
+  /* We dup the second descriptor.  */
+  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
+  TEST_COMPARE (posix_spawn_file_actions_adddup2 (&actions, temp_fd2, fd4),
+	        0);
+
+  /* Now spawn the process.  */
+  snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
+  snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
+  snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
+  snprintf (fd4name, sizeof fd4name, "%d", fd4);
+
+  for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
+    spargv[i] = argv[i + 1];
+  spargv[i++] = (char *) "--direct";
+  spargv[i++] = (char *) "--restart";
+  spargv[i++] = fd1name;
+  spargv[i++] = fd2name;
+  spargv[i++] = fd3name;
+  spargv[i++] = fd4name;
+  spargv[i++] = name1;
+  spargv[i] = NULL;
+
+  TEST_COMPARE (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ),
+		0);
+
+  /* Same test but with a NULL pid argument.  */
+  TEST_COMPARE (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ),
+		0);
+
+  /* Cleanup.  */
+  TEST_COMPARE (posix_spawn_file_actions_destroy (&actions), 0);
+  free (name3_copy);
 
   /* Wait for the children.  */
-  TEST_VERIFY (xwaitpid (pid, &status, 0) == pid);
+  TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
   TEST_VERIFY (WIFEXITED (status));
   TEST_VERIFY (!WIFSIGNALED (status));
-  TEST_VERIFY (WEXITSTATUS (status) == 0);
+  TEST_COMPARE (WEXITSTATUS (status), 0);
 
   xwaitpid (-1, &status, 0);
   TEST_VERIFY (WIFEXITED (status));
   TEST_VERIFY (!WIFSIGNALED (status));
-  TEST_VERIFY (WEXITSTATUS (status) == 0);
+  TEST_COMPARE (WEXITSTATUS (status), 0);
 
   return 0;
 }
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
-- 
2.17.1

Re: [PATCH 1/2] Use libsupport for tst-spawn.c

[Florian Weimer]

* Adhemerval Zanella:

> No worries, I need to catch up with recent libsupport functionalities.
> Below it is an updated patch, which I think it replaces with better
> libsupport interfaces:

Thanks.

> +  TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), strlen (fd2string));

I assume you could use xlseek here (and in the following code).

> +  TEST_COMPARE_BLOB (fd2string, strlen (fd2string), buf, strlen (buf));

Warning: This should use strlen (fd2string), not strlen (buf).  As far
as I can tell, buf is not necessarily null-terminated.

> +  TEST_COMPARE_BLOB (fd3string, strlen (fd3string), buf, strlen (buf));

> +  TEST_COMPARE_BLOB (fd1string, strlen (fd1string), buf, strlen (buf));

Same warning applies.

> +  TEST_VERIFY_EXIT ((fd1 = open (name, O_RDONLY)) != -1);

This is likely a case for xopen.

> +  /* Write something in the files.  */
> +  TEST_COMPARE (write (temp_fd1, fd1string, strlen (fd1string)),
> +		strlen (fd1string));
> +  TEST_COMPARE (write (temp_fd2, fd2string, strlen (fd2string)),
> +		strlen (fd2string));
> +  TEST_COMPARE (write (temp_fd3, fd3string, strlen (fd3string)),
> +		strlen (fd3string));

You can replace that with xwrite.

> +  /* We want to open the third file.  */
> +  TEST_VERIFY_EXIT ((name3_copy = strdup (name3)) != NULL);

xstrdup.

We are getting close!

Thanks,
Florian

Re: [PATCH 1/2] Use libsupport for tst-spawn.c

[Adhemerval Zanella]

On 21/09/2018 05:56, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> No worries, I need to catch up with recent libsupport functionalities.
>> Below it is an updated patch, which I think it replaces with better
>> libsupport interfaces:
> 
> Thanks.
> 
>> +  TEST_COMPARE (lseek (fd2, 0, SEEK_CUR), strlen (fd2string));
> 
> I assume you could use xlseek here (and in the following code).
> 
>> +  TEST_COMPARE_BLOB (fd2string, strlen (fd2string), buf, strlen (buf));
> 
> Warning: This should use strlen (fd2string), not strlen (buf).  As far
> as I can tell, buf is not necessarily null-terminated.
> 
>> +  TEST_COMPARE_BLOB (fd3string, strlen (fd3string), buf, strlen (buf));
> 
>> +  TEST_COMPARE_BLOB (fd1string, strlen (fd1string), buf, strlen (buf));
> 
> Same warning applies.
> 
>> +  TEST_VERIFY_EXIT ((fd1 = open (name, O_RDONLY)) != -1);
> 
> This is likely a case for xopen.
> 
>> +  /* Write something in the files.  */
>> +  TEST_COMPARE (write (temp_fd1, fd1string, strlen (fd1string)),
>> +		strlen (fd1string));
>> +  TEST_COMPARE (write (temp_fd2, fd2string, strlen (fd2string)),
>> +		strlen (fd2string));
>> +  TEST_COMPARE (write (temp_fd3, fd3string, strlen (fd3string)),
>> +		strlen (fd3string));
> 
> You can replace that with xwrite.
> 
>> +  /* We want to open the third file.  */
>> +  TEST_VERIFY_EXIT ((name3_copy = strdup (name3)) != NULL);
> 
> xstrdup.
> 
> We are getting close!

Thanks again for the review, below a patch with the points you raised
raised:

---

diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
index 7c785c430c..73a8cdc057 100644
--- a/posix/tst-spawn.c
+++ b/posix/tst-spawn.c
@@ -17,16 +17,20 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#include <stdio.h>
+#include <getopt.h>
 #include <errno.h>
 #include <error.h>
 #include <fcntl.h>
 #include <spawn.h>
 #include <stdlib.h>
 #include <string.h>
-#include <wait.h>
 #include <sys/param.h>
+
 #include <support/check.h>
 #include <support/xunistd.h>
+#include <support/temp_file.h>
+#include <support/support.h>
 
 
 /* Nonzero if the program gets called via `exec'.  */
@@ -36,16 +40,6 @@ static int restart;
 #define CMDLINE_OPTIONS \
   { "restart", no_argument, &restart, 1 },
 
-/* Prototype for our test function.  */
-extern void do_prepare (int argc, char *argv[]);
-extern int do_test (int argc, char *argv[]);
-
-/* We have a preparation function.  */
-#define PREPARE do_prepare
-
-#include "../test-skeleton.c"
-
-
 /* Name of the temporary files.  */
 static char *name1;
 static char *name2;
@@ -63,19 +57,18 @@ static const char fd3string[] = "This file will be opened";
 
 
 /* We have a preparation function.  */
-void
+static void
 do_prepare (int argc, char *argv[])
 {
   /* We must not open any files in the restart case.  */
   if (restart)
     return;
 
-  temp_fd1 = create_temp_file ("spawn", &name1);
-  temp_fd2 = create_temp_file ("spawn", &name2);
-  temp_fd3 = create_temp_file ("spawn", &name3);
-  if (temp_fd1 < 0 || temp_fd2 < 0 || temp_fd3 < 0)
-    exit (1);
+  TEST_VERIFY_EXIT ((temp_fd1 = create_temp_file ("spawn", &name1)) != -1);
+  TEST_VERIFY_EXIT ((temp_fd2 = create_temp_file ("spawn", &name2)) != -1);
+  TEST_VERIFY_EXIT ((temp_fd3 = create_temp_file ("spawn", &name3)) != -1);
 }
+#define PREPARE do_prepare
 
 
 static int
@@ -95,64 +88,45 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
   fd4 = atol (fd4s);
 
   /* Sanity check.  */
-  if (fd1 == fd2)
-    error (EXIT_FAILURE, 0, "value of fd1 and fd2 is the same");
-  if (fd1 == fd3)
-    error (EXIT_FAILURE, 0, "value of fd1 and fd3 is the same");
-  if (fd1 == fd4)
-    error (EXIT_FAILURE, 0, "value of fd1 and fd4 is the same");
-  if (fd2 == fd3)
-    error (EXIT_FAILURE, 0, "value of fd2 and fd3 is the same");
-  if (fd2 == fd4)
-    error (EXIT_FAILURE, 0, "value of fd2 and fd4 is the same");
-  if (fd3 == fd4)
-    error (EXIT_FAILURE, 0, "value of fd3 and fd4 is the same");
+  TEST_VERIFY_EXIT (fd1 != fd2);
+  TEST_VERIFY_EXIT (fd1 != fd3);
+  TEST_VERIFY_EXIT (fd1 != fd4);
+  TEST_VERIFY_EXIT (fd2 != fd3);
+  TEST_VERIFY_EXIT (fd2 != fd4);
+  TEST_VERIFY_EXIT (fd3 != fd4);
 
   /* First the easy part: read from the file descriptor which is
      supposed to be open.  */
-  if (lseek (fd2, 0, SEEK_CUR) != strlen (fd2string))
-    error (EXIT_FAILURE, errno, "file 2 not in right position");
+  TEST_COMPARE (xlseek (fd2, 0, SEEK_CUR), strlen (fd2string));
   /* The duped descriptor must have the same position.  */
-  if (lseek (fd4, 0, SEEK_CUR) != strlen (fd2string))
-    error (EXIT_FAILURE, errno, "file 4 not in right position");
-  if (lseek (fd2, 0, SEEK_SET) != 0)
-    error (EXIT_FAILURE, 0, "cannot reset position in file 2");
-  if (lseek (fd4, 0, SEEK_CUR) != 0)
-    error (EXIT_FAILURE, errno, "file 4 not set back, too");
-  if (read (fd2, buf, sizeof buf) != strlen (fd2string))
-    error (EXIT_FAILURE, 0, "cannot read file 2");
-  if (memcmp (fd2string, buf, strlen (fd2string)) != 0)
-    error (EXIT_FAILURE, 0, "file 2 does not match");
+  TEST_COMPARE (xlseek (fd4, 0, SEEK_CUR), strlen (fd2string));
+  TEST_COMPARE (xlseek (fd2, 0, SEEK_SET), 0);
+  TEST_COMPARE (xlseek (fd4, 0, SEEK_CUR), 0);
+  TEST_COMPARE (read (fd2, buf, sizeof buf), strlen (fd2string));
+  TEST_COMPARE_BLOB (fd2string, strlen (fd2string), buf, strlen (fd2string));
 
   /* Now read from the third file.  */
-  if (read (fd3, buf, sizeof buf) != strlen (fd3string))
-    error (EXIT_FAILURE, 0, "cannot read file 3");
-  if (memcmp (fd3string, buf, strlen (fd3string)) != 0)
-    error (EXIT_FAILURE, 0, "file 3 does not match");
+  TEST_COMPARE (read (fd3, buf, sizeof buf), strlen (fd3string));
+  TEST_COMPARE_BLOB (fd3string, strlen (fd3string), buf, strlen (fd3string));
   /* Try to write to the file.  This should not be allowed.  */
-  if (write (fd3, "boo!", 4) != -1 || errno != EBADF)
-    error (EXIT_FAILURE, 0, "file 3 is writable");
+  xwrite (fd3, "boo!", 4);
+  TEST_COMPARE (errno, EBADF);
 
   /* Now try to read the first file.  First make sure it is not opened.  */
-  if (lseek (fd1, 0, SEEK_CUR) != (off_t) -1 || errno != EBADF)
-    error (EXIT_FAILURE, 0, "file 1 (%d) is not closed", fd1);
+  TEST_COMPARE (xlseek (fd1, 0, SEEK_CUR), (off_t) -1);
+  TEST_COMPARE (errno, EBADF);
 
   /* Now open the file and read it.  */
-  fd1 = open (name, O_RDONLY);
-  if (fd1 == -1)
-    error (EXIT_FAILURE, errno,
-	   "cannot open first file \"%s\" for verification", name);
+  fd1 = xopen (name, O_RDONLY, 0600);
 
-  if (read (fd1, buf, sizeof buf) != strlen (fd1string))
-    error (EXIT_FAILURE, errno, "cannot read file 1");
-  if (memcmp (fd1string, buf, strlen (fd1string)) != 0)
-    error (EXIT_FAILURE, 0, "file 1 does not match");
+  TEST_COMPARE (read (fd1, buf, sizeof buf), strlen (fd1string));
+  TEST_COMPARE_BLOB (fd1string, strlen (fd1string), buf, strlen (fd2string));
 
   return 0;
 }
 
 
-int
+static int
 do_test (int argc, char *argv[])
 {
   pid_t pid;
@@ -181,7 +155,7 @@ do_test (int argc, char *argv[])
        + the name of the closed descriptor
   */
   if (argc != (restart ? 6 : 2) && argc != (restart ? 6 : 5))
-    error (EXIT_FAILURE, 0, "wrong number of arguments (%d)", argc);
+    FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
 
   if (restart)
     return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5]);
@@ -189,77 +163,73 @@ do_test (int argc, char *argv[])
   /* Prepare the test.  We are creating two files: one which file descriptor
      will be marked with FD_CLOEXEC, another which is not.  */
 
-   /* Write something in the files.  */
-   if (write (temp_fd1, fd1string, strlen (fd1string)) != strlen (fd1string))
-     error (EXIT_FAILURE, errno, "cannot write to first file");
-   if (write (temp_fd2, fd2string, strlen (fd2string)) != strlen (fd2string))
-     error (EXIT_FAILURE, errno, "cannot write to second file");
-   if (write (temp_fd3, fd3string, strlen (fd3string)) != strlen (fd3string))
-     error (EXIT_FAILURE, errno, "cannot write to third file");
-
-   /* Close the third file.  It'll be opened by `spawn'.  */
-   close (temp_fd3);
-
-   /* Tell `spawn' what to do.  */
-   if (posix_spawn_file_actions_init (&actions) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_init");
-   /* Close `temp_fd1'.  */
-   if (posix_spawn_file_actions_addclose (&actions, temp_fd1) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_addclose");
-   /* We want to open the third file.  */
-   name3_copy = strdup (name3);
-   if (name3_copy == NULL)
-     error (EXIT_FAILURE, errno, "strdup");
-   if (posix_spawn_file_actions_addopen (&actions, temp_fd3, name3_copy,
-					 O_RDONLY, 0666) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_addopen");
-   /* Overwrite the name to check that a copy has been made.  */
-   memset (name3_copy, 'X', strlen (name3_copy));
-
-   /* We dup the second descriptor.  */
-   fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
-   if (posix_spawn_file_actions_adddup2 (&actions, temp_fd2, fd4) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_adddup2");
-
-   /* Now spawn the process.  */
-   snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
-   snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
-   snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
-   snprintf (fd4name, sizeof fd4name, "%d", fd4);
-
-   for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
-     spargv[i] = argv[i + 1];
-   spargv[i++] = (char *) "--direct";
-   spargv[i++] = (char *) "--restart";
-   spargv[i++] = fd1name;
-   spargv[i++] = fd2name;
-   spargv[i++] = fd3name;
-   spargv[i++] = fd4name;
-   spargv[i++] = name1;
-   spargv[i] = NULL;
-
-   if (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn");
-
-   /* Same test but with a NULL pid argument.  */
-   if (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn");
-
-   /* Cleanup.  */
-   if (posix_spawn_file_actions_destroy (&actions) != 0)
-     error (EXIT_FAILURE, errno, "posix_spawn_file_actions_destroy");
-   free (name3_copy);
+  /* Write something in the files.  */
+  xwrite (temp_fd1, fd1string, strlen (fd1string));
+  xwrite (temp_fd2, fd2string, strlen (fd2string));
+  xwrite (temp_fd3, fd3string, strlen (fd3string));
+
+  /* Close the third file.  It'll be opened by `spawn'.  */
+  close (temp_fd3);
+
+  /* Tell `spawn' what to do.  */
+  TEST_COMPARE (posix_spawn_file_actions_init (&actions), 0);
+  /* Close `temp_fd1'.  */
+  TEST_COMPARE (posix_spawn_file_actions_addclose (&actions, temp_fd1), 0);
+  /* We want to open the third file.  */
+  name3_copy = xstrdup (name3);
+  TEST_COMPARE (posix_spawn_file_actions_addopen (&actions, temp_fd3,
+						  name3_copy,
+						  O_RDONLY, 0666),
+		0);
+  /* Overwrite the name to check that a copy has been made.  */
+  memset (name3_copy, 'X', strlen (name3_copy));
+
+  /* We dup the second descriptor.  */
+  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
+  TEST_COMPARE (posix_spawn_file_actions_adddup2 (&actions, temp_fd2, fd4),
+	        0);
+
+  /* Now spawn the process.  */
+  snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
+  snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
+  snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
+  snprintf (fd4name, sizeof fd4name, "%d", fd4);
+
+  for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
+    spargv[i] = argv[i + 1];
+  spargv[i++] = (char *) "--direct";
+  spargv[i++] = (char *) "--restart";
+  spargv[i++] = fd1name;
+  spargv[i++] = fd2name;
+  spargv[i++] = fd3name;
+  spargv[i++] = fd4name;
+  spargv[i++] = name1;
+  spargv[i] = NULL;
+
+  TEST_COMPARE (posix_spawn (&pid, argv[1], &actions, NULL, spargv, environ),
+		0);
+
+  /* Same test but with a NULL pid argument.  */
+  TEST_COMPARE (posix_spawn (NULL, argv[1], &actions, NULL, spargv, environ),
+		0);
+
+  /* Cleanup.  */
+  TEST_COMPARE (posix_spawn_file_actions_destroy (&actions), 0);
+  free (name3_copy);
 
   /* Wait for the children.  */
-  TEST_VERIFY (xwaitpid (pid, &status, 0) == pid);
+  TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
   TEST_VERIFY (WIFEXITED (status));
   TEST_VERIFY (!WIFSIGNALED (status));
-  TEST_VERIFY (WEXITSTATUS (status) == 0);
+  TEST_COMPARE (WEXITSTATUS (status), 0);
 
   xwaitpid (-1, &status, 0);
   TEST_VERIFY (WIFEXITED (status));
   TEST_VERIFY (!WIFSIGNALED (status));
-  TEST_VERIFY (WEXITSTATUS (status) == 0);
+  TEST_COMPARE (WEXITSTATUS (status), 0);
 
   return 0;
 }
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
-- 
2.17.1

Re: [PATCH 1/2] Use libsupport for tst-spawn.c

[Florian Weimer]

* Adhemerval Zanella:

>    /* Try to write to the file.  This should not be allowed.  */
> +  xwrite (fd3, "boo!", 4);
> +  TEST_COMPARE (errno, EBADF);

You need to write this as:

  TEST_COMPARE (write (fd3, "boo!", 4), -1);
  TEST_COMPARE (errno, EBADF);

xwrite will terminate the process.

>    /* Now try to read the first file.  First make sure it is not opened.  */
> +  TEST_COMPARE (xlseek (fd1, 0, SEEK_CUR), (off_t) -1);
> +  TEST_COMPARE (errno, EBADF);

Same problem: Use lseek, not xlseek.

> +  TEST_COMPARE (read (fd1, buf, sizeof buf), strlen (fd1string));
> +  TEST_COMPARE_BLOB (fd1string, strlen (fd1string), buf, strlen (fd2string));

There's a typo here: strlen (fd2string) should be strlen (fd1string).

I also saw this:

  /* Close the third file.  It'll be opened by `spawn'.  */
  close (temp_fd3);

You could turn this into xclose for additional error checking.

Otherwise, this version is fine.  Thanks.

Florian

Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[Adhemerval Zanella]

Ping.

On 19/09/2018 19:46, adhemerval.zanella@linaro.org wrote:
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> Austin Group issue #411 [1] proposes that posix_spawn file action
> posix_spawn_file_actions_adddup2 resets the close-on-exec when
> source and destination refer to same file descriptor.
> 
> It solves the issue on multi-thread applications which uses
> close-on-exec as default, and want to hand-chose specifically
> file descriptor to purposefully inherited into a child process.
> Current approach to achieve this scenario is to use two adddup2 file
> actions and a temporary file description which do not conflict with
> any other, coupled with a close file action to avoid leaking the
> temporary file descriptor.  This approach, besides being complex,
> may fail with EMFILE/ENFILE file descriptor exaustion.
> 
> This can be more easily accomplished with an in-place removal of
> FD_CLOEXEC.  Although the resulting adddup2 semantic is slight
> different than dup2 (equal file descriptors should be handled as
> no-op), the proposed possible solution are either more complex
> (fcntl action which a limited set of operations) or results in
> unrequired operations (dup3 which also returns EINVAL for same
> file descriptor).
> 
> Checked on aarch64-linux-gnu.
> 
> 	* posix/tst-spawn.c (do_prepare, handle_restart, do_test): Add
> 	posix_spawn_file_actions_adddup2 test to check O_CLOCEXEC reset.
> 	* sysdeps/unix/sysv/linux/spawni.c (__spawni_child): Add
> 	close-on-exec reset for adddup2 file action.
> 	* sysdeps/posix/spawni.c (__spawni_child): Likewise.
> 
> [1] http://austingroupbugs.net/view.php?id=411
> ---
>  ChangeLog                        |  6 ++++
>  posix/tst-spawn.c                | 47 +++++++++++++++++++++++++-------
>  sysdeps/posix/spawni.c           | 21 ++++++++++++--
>  sysdeps/unix/sysv/linux/spawni.c | 21 ++++++++++++--
>  4 files changed, 79 insertions(+), 16 deletions(-)
> 
> diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
> index 638ba1ba55..2354417020 100644
> --- a/posix/tst-spawn.c
> +++ b/posix/tst-spawn.c
> @@ -40,17 +40,19 @@ static int restart;
>  static char *name1;
>  static char *name2;
>  static char *name3;
> +static char *name5;
>  
>  /* Descriptors for the temporary files.  */
>  static int temp_fd1 = -1;
>  static int temp_fd2 = -1;
>  static int temp_fd3 = -1;
> +static int temp_fd5 = -1;
>  
>  /* The contents of our files.  */
>  static const char fd1string[] = "This file should get closed";
>  static const char fd2string[] = "This file should stay opened";
>  static const char fd3string[] = "This file will be opened";
> -
> +static const char fd5string[] = "This file should stay opened (O_CLOEXEC)";
>  
>  /* We have a preparation function.  */
>  static void
> @@ -63,24 +65,31 @@ do_prepare (int argc, char *argv[])
>    TEST_VERIFY_EXIT ((temp_fd1 = create_temp_file ("spawn", &name1)) != -1);
>    TEST_VERIFY_EXIT ((temp_fd2 = create_temp_file ("spawn", &name2)) != -1);
>    TEST_VERIFY_EXIT ((temp_fd3 = create_temp_file ("spawn", &name3)) != -1);
> +  TEST_VERIFY_EXIT ((temp_fd5 = create_temp_file ("spawn", &name5)) != -1);
> +
> +  int flags;
> +  TEST_VERIFY_EXIT ((flags = fcntl (temp_fd5, F_GETFD, &flags)) != -1);
> +  TEST_VERIFY_EXIT (fcntl (temp_fd5, F_SETFD, flags | FD_CLOEXEC) == 0);
>  }
>  #define PREPARE do_prepare
>  
>  static int
>  handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
> -		const char *fd4s, const char *name)
> +		const char *fd4s, const char *name, const char *fd5s)
>  {
>    char buf[100];
>    int fd1;
>    int fd2;
>    int fd3;
>    int fd4;
> +  int fd5;
>  
>    /* First get the descriptors.  */
>    fd1 = atol (fd1s);
>    fd2 = atol (fd2s);
>    fd3 = atol (fd3s);
>    fd4 = atol (fd4s);
> +  fd5 = atol (fd5s);
>  
>    /* Sanity check.  */
>    TEST_VERIFY_EXIT (fd1 != fd2);
> @@ -89,6 +98,7 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>    TEST_VERIFY_EXIT (fd2 != fd3);
>    TEST_VERIFY_EXIT (fd2 != fd4);
>    TEST_VERIFY_EXIT (fd3 != fd4);
> +  TEST_VERIFY_EXIT (fd4 != fd5);
>  
>    /* First the easy part: read from the file descriptor which is
>       supposed to be open.  */
> @@ -116,6 +126,10 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>    TEST_VERIFY_EXIT (read (fd1, buf, sizeof buf) == strlen (fd1string));
>    TEST_VERIFY_EXIT (memcmp (fd1string, buf, strlen (fd1string)) == 0);
>  
> +  TEST_VERIFY_EXIT (lseek (fd5, 0, SEEK_SET) == 0);
> +  TEST_VERIFY_EXIT (read (fd5, buf, sizeof buf) == strlen (fd5string));
> +  TEST_VERIFY_EXIT (memcmp (fd5string, buf, strlen (fd5string)) == 0);
> +
>    return 0;
>  }
>  
> @@ -131,6 +145,7 @@ do_test (int argc, char *argv[])
>    char fd2name[18];
>    char fd3name[18];
>    char fd4name[18];
> +  char fd5name[18];
>    char *name3_copy;
>    char *spargv[12];
>    int i;
> @@ -141,21 +156,24 @@ do_test (int argc, char *argv[])
>         + "--library-path"	optional
>         + the library path	optional
>         + the application name
> -     - five parameters left if called through re-execution
> +     - six parameters left if called through re-execution
>         + file descriptor number which is supposed to be closed
>         + the open file descriptor
>         + the newly opened file descriptor
> -       + thhe duped second descriptor
> +       + the duped second descriptor
>         + the name of the closed descriptor
> +       + the duped fourth dile descriptor which O_CLOEXEC should be
> +	 remove by adddup2.
>    */
> -  if (argc != (restart ? 6 : 2) && argc != (restart ? 6 : 5))
> +  if (argc != (restart ? 7 : 2) && argc != (restart ? 7 : 5))
>      FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
>  
>    if (restart)
> -    return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5]);
> +    return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5],
> +			   argv[6]);
>  
> -  /* Prepare the test.  We are creating two files: one which file descriptor
> -     will be marked with FD_CLOEXEC, another which is not.  */
> +  /* Prepare the test.  We are creating four files: two which file descriptor
> +     will be marked with FD_CLOEXEC, another which is not  */
>  
>    /* Write something in the files.  */
>    TEST_VERIFY_EXIT (write (temp_fd1, fd1string, strlen (fd1string))
> @@ -164,6 +182,8 @@ do_test (int argc, char *argv[])
>  		    == strlen (fd2string));
>    TEST_VERIFY_EXIT (write (temp_fd3, fd3string, strlen (fd3string))
>  		    == strlen (fd3string));
> +  TEST_VERIFY_EXIT (write (temp_fd5, fd5string, strlen (fd5string))
> +		    == strlen (fd5string));
>  
>    /* Close the third file.  It'll be opened by `spawn'.  */
>    close (temp_fd3);
> @@ -183,17 +203,23 @@ do_test (int argc, char *argv[])
>    memset (name3_copy, 'X', strlen (name3_copy));
>  
>    /* We dup the second descriptor.  */
> -  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
> +  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, MAX (temp_fd3, temp_fd5)))) + 1;
>    TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd2,
>  						      fd4) == 0);
>  
> +  /* We clear the O_CLOEXEC on fourth descriptor, so it should be
> +     stay open on child.  */
> +  TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd5,
> +						      temp_fd5) == 0);
> +
>    /* Now spawn the process.  */
>    snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
>    snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
>    snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
>    snprintf (fd4name, sizeof fd4name, "%d", fd4);
> +  snprintf (fd5name, sizeof fd5name, "%d", temp_fd5);
>  
> -  for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
> +  for (i = 0; i < (argc == (restart ? 7 : 5) ? 4 : 1); i++)
>      spargv[i] = argv[i + 1];
>    spargv[i++] = (char *) "--direct";
>    spargv[i++] = (char *) "--restart";
> @@ -202,6 +228,7 @@ do_test (int argc, char *argv[])
>    spargv[i++] = fd3name;
>    spargv[i++] = fd4name;
>    spargv[i++] = name1;
> +  spargv[i++] = fd5name;
>    spargv[i] = NULL;
>  
>    TEST_VERIFY_EXIT (posix_spawn (&pid, argv[1], &actions, NULL, spargv,
> diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
> index b138ab4393..d322db5c19 100644
> --- a/sysdeps/posix/spawni.c
> +++ b/sysdeps/posix/spawni.c
> @@ -204,10 +204,25 @@ __spawni_child (void *arguments)
>  	      break;
>  
>  	    case spawn_do_dup2:
> -	      if (__dup2 (action->action.dup2_action.fd,
> -			  action->action.dup2_action.newfd)
> +	      /* Austin Group issue #411 requires adddup2 action with source
> +		 and destination being equal to remove close-on-exec flag.  */
> +	      if (action->action.dup2_action.fd
>  		  != action->action.dup2_action.newfd)
> -		goto fail;
> +		{
> +		  if (__dup2 (action->action.dup2_action.fd,
> +			     action->action.dup2_action.newfd)
> +		      != action->action.dup2_action.newfd)
> +		    goto fail;
> +		}
> +	      else
> +		{
> +		  int fd = action->action.dup2_action.newfd;
> +		  int flags = __fcntl (fd, F_GETFD, 0);
> +		  if (flags == -1)
> +		    goto fail;
> +		  if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
> +		    goto fail;
> +		}
>  	      break;
>  	    }
>  	}
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index 85239cedbf..b2304ffe60 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -253,10 +253,25 @@ __spawni_child (void *arguments)
>  	      break;
>  
>  	    case spawn_do_dup2:
> -	      if (__dup2 (action->action.dup2_action.fd,
> -			  action->action.dup2_action.newfd)
> +	      /* Austin Group issue #411 requires adddup2 action with source
> +		 and destination being equal to remove close-on-exec flag.  */
> +	      if (action->action.dup2_action.fd
>  		  != action->action.dup2_action.newfd)
> -		goto fail;
> +		{
> +		  if (__dup2 (action->action.dup2_action.fd,
> +			     action->action.dup2_action.newfd)
> +		      != action->action.dup2_action.newfd)
> +		    goto fail;
> +		}
> +	      else
> +		{
> +		  int fd = action->action.dup2_action.newfd;
> +		  int flags = __fcntl (fd, F_GETFD, 0);
> +		  if (flags == -1)
> +		    goto fail;
> +		  if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
> +		    goto fail;
> +		}
>  	      break;
>  	    }
>  	}
> 

Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[Adhemerval Zanella]

Ping (x2).

On 29/10/2018 16:32, Adhemerval Zanella wrote:
> Ping.
> 
> On 19/09/2018 19:46, adhemerval.zanella@linaro.org wrote:
>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>
>> Austin Group issue #411 [1] proposes that posix_spawn file action
>> posix_spawn_file_actions_adddup2 resets the close-on-exec when
>> source and destination refer to same file descriptor.
>>
>> It solves the issue on multi-thread applications which uses
>> close-on-exec as default, and want to hand-chose specifically
>> file descriptor to purposefully inherited into a child process.
>> Current approach to achieve this scenario is to use two adddup2 file
>> actions and a temporary file description which do not conflict with
>> any other, coupled with a close file action to avoid leaking the
>> temporary file descriptor.  This approach, besides being complex,
>> may fail with EMFILE/ENFILE file descriptor exaustion.
>>
>> This can be more easily accomplished with an in-place removal of
>> FD_CLOEXEC.  Although the resulting adddup2 semantic is slight
>> different than dup2 (equal file descriptors should be handled as
>> no-op), the proposed possible solution are either more complex
>> (fcntl action which a limited set of operations) or results in
>> unrequired operations (dup3 which also returns EINVAL for same
>> file descriptor).
>>
>> Checked on aarch64-linux-gnu.
>>
>> 	* posix/tst-spawn.c (do_prepare, handle_restart, do_test): Add
>> 	posix_spawn_file_actions_adddup2 test to check O_CLOCEXEC reset.
>> 	* sysdeps/unix/sysv/linux/spawni.c (__spawni_child): Add
>> 	close-on-exec reset for adddup2 file action.
>> 	* sysdeps/posix/spawni.c (__spawni_child): Likewise.
>>
>> [1] http://austingroupbugs.net/view.php?id=411
>> ---
>>  ChangeLog                        |  6 ++++
>>  posix/tst-spawn.c                | 47 +++++++++++++++++++++++++-------
>>  sysdeps/posix/spawni.c           | 21 ++++++++++++--
>>  sysdeps/unix/sysv/linux/spawni.c | 21 ++++++++++++--
>>  4 files changed, 79 insertions(+), 16 deletions(-)
>>
>> diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
>> index 638ba1ba55..2354417020 100644
>> --- a/posix/tst-spawn.c
>> +++ b/posix/tst-spawn.c
>> @@ -40,17 +40,19 @@ static int restart;
>>  static char *name1;
>>  static char *name2;
>>  static char *name3;
>> +static char *name5;
>>  
>>  /* Descriptors for the temporary files.  */
>>  static int temp_fd1 = -1;
>>  static int temp_fd2 = -1;
>>  static int temp_fd3 = -1;
>> +static int temp_fd5 = -1;
>>  
>>  /* The contents of our files.  */
>>  static const char fd1string[] = "This file should get closed";
>>  static const char fd2string[] = "This file should stay opened";
>>  static const char fd3string[] = "This file will be opened";
>> -
>> +static const char fd5string[] = "This file should stay opened (O_CLOEXEC)";
>>  
>>  /* We have a preparation function.  */
>>  static void
>> @@ -63,24 +65,31 @@ do_prepare (int argc, char *argv[])
>>    TEST_VERIFY_EXIT ((temp_fd1 = create_temp_file ("spawn", &name1)) != -1);
>>    TEST_VERIFY_EXIT ((temp_fd2 = create_temp_file ("spawn", &name2)) != -1);
>>    TEST_VERIFY_EXIT ((temp_fd3 = create_temp_file ("spawn", &name3)) != -1);
>> +  TEST_VERIFY_EXIT ((temp_fd5 = create_temp_file ("spawn", &name5)) != -1);
>> +
>> +  int flags;
>> +  TEST_VERIFY_EXIT ((flags = fcntl (temp_fd5, F_GETFD, &flags)) != -1);
>> +  TEST_VERIFY_EXIT (fcntl (temp_fd5, F_SETFD, flags | FD_CLOEXEC) == 0);
>>  }
>>  #define PREPARE do_prepare
>>  
>>  static int
>>  handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>> -		const char *fd4s, const char *name)
>> +		const char *fd4s, const char *name, const char *fd5s)
>>  {
>>    char buf[100];
>>    int fd1;
>>    int fd2;
>>    int fd3;
>>    int fd4;
>> +  int fd5;
>>  
>>    /* First get the descriptors.  */
>>    fd1 = atol (fd1s);
>>    fd2 = atol (fd2s);
>>    fd3 = atol (fd3s);
>>    fd4 = atol (fd4s);
>> +  fd5 = atol (fd5s);
>>  
>>    /* Sanity check.  */
>>    TEST_VERIFY_EXIT (fd1 != fd2);
>> @@ -89,6 +98,7 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>>    TEST_VERIFY_EXIT (fd2 != fd3);
>>    TEST_VERIFY_EXIT (fd2 != fd4);
>>    TEST_VERIFY_EXIT (fd3 != fd4);
>> +  TEST_VERIFY_EXIT (fd4 != fd5);
>>  
>>    /* First the easy part: read from the file descriptor which is
>>       supposed to be open.  */
>> @@ -116,6 +126,10 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>>    TEST_VERIFY_EXIT (read (fd1, buf, sizeof buf) == strlen (fd1string));
>>    TEST_VERIFY_EXIT (memcmp (fd1string, buf, strlen (fd1string)) == 0);
>>  
>> +  TEST_VERIFY_EXIT (lseek (fd5, 0, SEEK_SET) == 0);
>> +  TEST_VERIFY_EXIT (read (fd5, buf, sizeof buf) == strlen (fd5string));
>> +  TEST_VERIFY_EXIT (memcmp (fd5string, buf, strlen (fd5string)) == 0);
>> +
>>    return 0;
>>  }
>>  
>> @@ -131,6 +145,7 @@ do_test (int argc, char *argv[])
>>    char fd2name[18];
>>    char fd3name[18];
>>    char fd4name[18];
>> +  char fd5name[18];
>>    char *name3_copy;
>>    char *spargv[12];
>>    int i;
>> @@ -141,21 +156,24 @@ do_test (int argc, char *argv[])
>>         + "--library-path"	optional
>>         + the library path	optional
>>         + the application name
>> -     - five parameters left if called through re-execution
>> +     - six parameters left if called through re-execution
>>         + file descriptor number which is supposed to be closed
>>         + the open file descriptor
>>         + the newly opened file descriptor
>> -       + thhe duped second descriptor
>> +       + the duped second descriptor
>>         + the name of the closed descriptor
>> +       + the duped fourth dile descriptor which O_CLOEXEC should be
>> +	 remove by adddup2.
>>    */
>> -  if (argc != (restart ? 6 : 2) && argc != (restart ? 6 : 5))
>> +  if (argc != (restart ? 7 : 2) && argc != (restart ? 7 : 5))
>>      FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
>>  
>>    if (restart)
>> -    return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5]);
>> +    return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5],
>> +			   argv[6]);
>>  
>> -  /* Prepare the test.  We are creating two files: one which file descriptor
>> -     will be marked with FD_CLOEXEC, another which is not.  */
>> +  /* Prepare the test.  We are creating four files: two which file descriptor
>> +     will be marked with FD_CLOEXEC, another which is not  */
>>  
>>    /* Write something in the files.  */
>>    TEST_VERIFY_EXIT (write (temp_fd1, fd1string, strlen (fd1string))
>> @@ -164,6 +182,8 @@ do_test (int argc, char *argv[])
>>  		    == strlen (fd2string));
>>    TEST_VERIFY_EXIT (write (temp_fd3, fd3string, strlen (fd3string))
>>  		    == strlen (fd3string));
>> +  TEST_VERIFY_EXIT (write (temp_fd5, fd5string, strlen (fd5string))
>> +		    == strlen (fd5string));
>>  
>>    /* Close the third file.  It'll be opened by `spawn'.  */
>>    close (temp_fd3);
>> @@ -183,17 +203,23 @@ do_test (int argc, char *argv[])
>>    memset (name3_copy, 'X', strlen (name3_copy));
>>  
>>    /* We dup the second descriptor.  */
>> -  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
>> +  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, MAX (temp_fd3, temp_fd5)))) + 1;
>>    TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd2,
>>  						      fd4) == 0);
>>  
>> +  /* We clear the O_CLOEXEC on fourth descriptor, so it should be
>> +     stay open on child.  */
>> +  TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd5,
>> +						      temp_fd5) == 0);
>> +
>>    /* Now spawn the process.  */
>>    snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
>>    snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
>>    snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
>>    snprintf (fd4name, sizeof fd4name, "%d", fd4);
>> +  snprintf (fd5name, sizeof fd5name, "%d", temp_fd5);
>>  
>> -  for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
>> +  for (i = 0; i < (argc == (restart ? 7 : 5) ? 4 : 1); i++)
>>      spargv[i] = argv[i + 1];
>>    spargv[i++] = (char *) "--direct";
>>    spargv[i++] = (char *) "--restart";
>> @@ -202,6 +228,7 @@ do_test (int argc, char *argv[])
>>    spargv[i++] = fd3name;
>>    spargv[i++] = fd4name;
>>    spargv[i++] = name1;
>> +  spargv[i++] = fd5name;
>>    spargv[i] = NULL;
>>  
>>    TEST_VERIFY_EXIT (posix_spawn (&pid, argv[1], &actions, NULL, spargv,
>> diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
>> index b138ab4393..d322db5c19 100644
>> --- a/sysdeps/posix/spawni.c
>> +++ b/sysdeps/posix/spawni.c
>> @@ -204,10 +204,25 @@ __spawni_child (void *arguments)
>>  	      break;
>>  
>>  	    case spawn_do_dup2:
>> -	      if (__dup2 (action->action.dup2_action.fd,
>> -			  action->action.dup2_action.newfd)
>> +	      /* Austin Group issue #411 requires adddup2 action with source
>> +		 and destination being equal to remove close-on-exec flag.  */
>> +	      if (action->action.dup2_action.fd
>>  		  != action->action.dup2_action.newfd)
>> -		goto fail;
>> +		{
>> +		  if (__dup2 (action->action.dup2_action.fd,
>> +			     action->action.dup2_action.newfd)
>> +		      != action->action.dup2_action.newfd)
>> +		    goto fail;
>> +		}
>> +	      else
>> +		{
>> +		  int fd = action->action.dup2_action.newfd;
>> +		  int flags = __fcntl (fd, F_GETFD, 0);
>> +		  if (flags == -1)
>> +		    goto fail;
>> +		  if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>> +		    goto fail;
>> +		}
>>  	      break;
>>  	    }
>>  	}
>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>> index 85239cedbf..b2304ffe60 100644
>> --- a/sysdeps/unix/sysv/linux/spawni.c
>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>> @@ -253,10 +253,25 @@ __spawni_child (void *arguments)
>>  	      break;
>>  
>>  	    case spawn_do_dup2:
>> -	      if (__dup2 (action->action.dup2_action.fd,
>> -			  action->action.dup2_action.newfd)
>> +	      /* Austin Group issue #411 requires adddup2 action with source
>> +		 and destination being equal to remove close-on-exec flag.  */
>> +	      if (action->action.dup2_action.fd
>>  		  != action->action.dup2_action.newfd)
>> -		goto fail;
>> +		{
>> +		  if (__dup2 (action->action.dup2_action.fd,
>> +			     action->action.dup2_action.newfd)
>> +		      != action->action.dup2_action.newfd)
>> +		    goto fail;
>> +		}
>> +	      else
>> +		{
>> +		  int fd = action->action.dup2_action.newfd;
>> +		  int flags = __fcntl (fd, F_GETFD, 0);
>> +		  if (flags == -1)
>> +		    goto fail;
>> +		  if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>> +		    goto fail;
>> +		}
>>  	      break;
>>  	    }
>>  	}
>>

Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[Siddhesh Poyarekar]

My mail client ate the original submission, so responding to your ping.

On 13/12/18 2:54 AM, Adhemerval Zanella wrote:
> Ping (x2).
> 
> On 29/10/2018 16:32, Adhemerval Zanella wrote:
>> Ping.
>>
>> On 19/09/2018 19:46, adhemerval.zanella@linaro.org wrote:
>>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>>
>>> Austin Group issue #411 [1] proposes that posix_spawn file action
>>> posix_spawn_file_actions_adddup2 resets the close-on-exec when
>>> source and destination refer to same file descriptor.
>>>
>>> It solves the issue on multi-thread applications which uses
>>> close-on-exec as default, and want to hand-chose specifically
>>> file descriptor to purposefully inherited into a child process.
>>> Current approach to achieve this scenario is to use two adddup2 file
>>> actions and a temporary file description which do not conflict with
>>> any other, coupled with a close file action to avoid leaking the
>>> temporary file descriptor.  This approach, besides being complex,
>>> may fail with EMFILE/ENFILE file descriptor exaustion.
>>>
>>> This can be more easily accomplished with an in-place removal of
>>> FD_CLOEXEC.  Although the resulting adddup2 semantic is slight
>>> different than dup2 (equal file descriptors should be handled as
>>> no-op), the proposed possible solution are either more complex
>>> (fcntl action which a limited set of operations) or results in
>>> unrequired operations (dup3 which also returns EINVAL for same
>>> file descriptor).
>>>
>>> Checked on aarch64-linux-gnu.
>>>

BZ # on ChangeLog.

>>> 	* posix/tst-spawn.c (do_prepare, handle_restart, do_test): Add
>>> 	posix_spawn_file_actions_adddup2 test to check O_CLOCEXEC reset.
>>> 	* sysdeps/unix/sysv/linux/spawni.c (__spawni_child): Add
>>> 	close-on-exec reset for adddup2 file action.
>>> 	* sysdeps/posix/spawni.c (__spawni_child): Likewise.
>>>
>>> [1] http://austingroupbugs.net/view.php?id=411
>>> ---
>>>   ChangeLog                        |  6 ++++
>>>   posix/tst-spawn.c                | 47 +++++++++++++++++++++++++-------
>>>   sysdeps/posix/spawni.c           | 21 ++++++++++++--
>>>   sysdeps/unix/sysv/linux/spawni.c | 21 ++++++++++++--
>>>   4 files changed, 79 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
>>> index 638ba1ba55..2354417020 100644
>>> --- a/posix/tst-spawn.c
>>> +++ b/posix/tst-spawn.c
>>> @@ -40,17 +40,19 @@ static int restart;
>>>   static char *name1;
>>>   static char *name2;
>>>   static char *name3;
>>> +static char *name5;
>>>   
>>>   /* Descriptors for the temporary files.  */
>>>   static int temp_fd1 = -1;
>>>   static int temp_fd2 = -1;
>>>   static int temp_fd3 = -1;
>>> +static int temp_fd5 = -1;
>>>   
>>>   /* The contents of our files.  */
>>>   static const char fd1string[] = "This file should get closed";
>>>   static const char fd2string[] = "This file should stay opened";
>>>   static const char fd3string[] = "This file will be opened";
>>> -
>>> +static const char fd5string[] = "This file should stay opened (O_CLOEXEC)";
>>>   
>>>   /* We have a preparation function.  */
>>>   static void
>>> @@ -63,24 +65,31 @@ do_prepare (int argc, char *argv[])
>>>     TEST_VERIFY_EXIT ((temp_fd1 = create_temp_file ("spawn", &name1)) != -1);
>>>     TEST_VERIFY_EXIT ((temp_fd2 = create_temp_file ("spawn", &name2)) != -1);
>>>     TEST_VERIFY_EXIT ((temp_fd3 = create_temp_file ("spawn", &name3)) != -1);
>>> +  TEST_VERIFY_EXIT ((temp_fd5 = create_temp_file ("spawn", &name5)) != -1);
>>> +
>>> +  int flags;
>>> +  TEST_VERIFY_EXIT ((flags = fcntl (temp_fd5, F_GETFD, &flags)) != -1);
>>> +  TEST_VERIFY_EXIT (fcntl (temp_fd5, F_SETFD, flags | FD_CLOEXEC) == 0);
>>>   }
>>>   #define PREPARE do_prepare
>>>   
>>>   static int
>>>   handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>>> -		const char *fd4s, const char *name)
>>> +		const char *fd4s, const char *name, const char *fd5s)
>>>   {
>>>     char buf[100];
>>>     int fd1;
>>>     int fd2;
>>>     int fd3;
>>>     int fd4;
>>> +  int fd5;
>>>   
>>>     /* First get the descriptors.  */
>>>     fd1 = atol (fd1s);
>>>     fd2 = atol (fd2s);
>>>     fd3 = atol (fd3s);
>>>     fd4 = atol (fd4s);
>>> +  fd5 = atol (fd5s);
>>>   
>>>     /* Sanity check.  */
>>>     TEST_VERIFY_EXIT (fd1 != fd2);
>>> @@ -89,6 +98,7 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>>>     TEST_VERIFY_EXIT (fd2 != fd3);
>>>     TEST_VERIFY_EXIT (fd2 != fd4);
>>>     TEST_VERIFY_EXIT (fd3 != fd4);
>>> +  TEST_VERIFY_EXIT (fd4 != fd5);
>>>   
>>>     /* First the easy part: read from the file descriptor which is
>>>        supposed to be open.  */
>>> @@ -116,6 +126,10 @@ handle_restart (const char *fd1s, const char *fd2s, const char *fd3s,
>>>     TEST_VERIFY_EXIT (read (fd1, buf, sizeof buf) == strlen (fd1string));
>>>     TEST_VERIFY_EXIT (memcmp (fd1string, buf, strlen (fd1string)) == 0);
>>>   
>>> +  TEST_VERIFY_EXIT (lseek (fd5, 0, SEEK_SET) == 0);
>>> +  TEST_VERIFY_EXIT (read (fd5, buf, sizeof buf) == strlen (fd5string));
>>> +  TEST_VERIFY_EXIT (memcmp (fd5string, buf, strlen (fd5string)) == 0);
>>> +
>>>     return 0;
>>>   }
>>>   
>>> @@ -131,6 +145,7 @@ do_test (int argc, char *argv[])
>>>     char fd2name[18];
>>>     char fd3name[18];
>>>     char fd4name[18];
>>> +  char fd5name[18];
>>>     char *name3_copy;
>>>     char *spargv[12];
>>>     int i;
>>> @@ -141,21 +156,24 @@ do_test (int argc, char *argv[])
>>>          + "--library-path"	optional
>>>          + the library path	optional
>>>          + the application name
>>> -     - five parameters left if called through re-execution
>>> +     - six parameters left if called through re-execution
>>>          + file descriptor number which is supposed to be closed
>>>          + the open file descriptor
>>>          + the newly opened file descriptor
>>> -       + thhe duped second descriptor
>>> +       + the duped second descriptor
>>>          + the name of the closed descriptor
>>> +       + the duped fourth dile descriptor which O_CLOEXEC should be
>>> +	 remove by adddup2.
>>>     */
>>> -  if (argc != (restart ? 6 : 2) && argc != (restart ? 6 : 5))
>>> +  if (argc != (restart ? 7 : 2) && argc != (restart ? 7 : 5))
>>>       FAIL_EXIT1 ("wrong number of arguments (%d)", argc);
>>>   
>>>     if (restart)
>>> -    return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5]);
>>> +    return handle_restart (argv[1], argv[2], argv[3], argv[4], argv[5],
>>> +			   argv[6]);
>>>   
>>> -  /* Prepare the test.  We are creating two files: one which file descriptor
>>> -     will be marked with FD_CLOEXEC, another which is not.  */
>>> +  /* Prepare the test.  We are creating four files: two which file descriptor
>>> +     will be marked with FD_CLOEXEC, another which is not  */
>>>   
>>>     /* Write something in the files.  */
>>>     TEST_VERIFY_EXIT (write (temp_fd1, fd1string, strlen (fd1string))
>>> @@ -164,6 +182,8 @@ do_test (int argc, char *argv[])
>>>   		    == strlen (fd2string));
>>>     TEST_VERIFY_EXIT (write (temp_fd3, fd3string, strlen (fd3string))
>>>   		    == strlen (fd3string));
>>> +  TEST_VERIFY_EXIT (write (temp_fd5, fd5string, strlen (fd5string))
>>> +		    == strlen (fd5string));
>>>   
>>>     /* Close the third file.  It'll be opened by `spawn'.  */
>>>     close (temp_fd3);
>>> @@ -183,17 +203,23 @@ do_test (int argc, char *argv[])
>>>     memset (name3_copy, 'X', strlen (name3_copy));
>>>   
>>>     /* We dup the second descriptor.  */
>>> -  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, temp_fd3))) + 1;
>>> +  fd4 = MAX (2, MAX (temp_fd1, MAX (temp_fd2, MAX (temp_fd3, temp_fd5)))) + 1;
>>>     TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd2,
>>>   						      fd4) == 0);
>>>   
>>> +  /* We clear the O_CLOEXEC on fourth descriptor, so it should be
>>> +     stay open on child.  */
>>> +  TEST_VERIFY_EXIT (posix_spawn_file_actions_adddup2 (&actions, temp_fd5,
>>> +						      temp_fd5) == 0);
>>> +
>>>     /* Now spawn the process.  */
>>>     snprintf (fd1name, sizeof fd1name, "%d", temp_fd1);
>>>     snprintf (fd2name, sizeof fd2name, "%d", temp_fd2);
>>>     snprintf (fd3name, sizeof fd3name, "%d", temp_fd3);
>>>     snprintf (fd4name, sizeof fd4name, "%d", fd4);
>>> +  snprintf (fd5name, sizeof fd5name, "%d", temp_fd5);
>>>   
>>> -  for (i = 0; i < (argc == (restart ? 6 : 5) ? 4 : 1); i++)
>>> +  for (i = 0; i < (argc == (restart ? 7 : 5) ? 4 : 1); i++)
>>>       spargv[i] = argv[i + 1];
>>>     spargv[i++] = (char *) "--direct";
>>>     spargv[i++] = (char *) "--restart";
>>> @@ -202,6 +228,7 @@ do_test (int argc, char *argv[])
>>>     spargv[i++] = fd3name;
>>>     spargv[i++] = fd4name;
>>>     spargv[i++] = name1;
>>> +  spargv[i++] = fd5name;
>>>     spargv[i] = NULL;
>>>   
>>>     TEST_VERIFY_EXIT (posix_spawn (&pid, argv[1], &actions, NULL, spargv,
>>> diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
>>> index b138ab4393..d322db5c19 100644
>>> --- a/sysdeps/posix/spawni.c
>>> +++ b/sysdeps/posix/spawni.c
>>> @@ -204,10 +204,25 @@ __spawni_child (void *arguments)
>>>   	      break;
>>>   
>>>   	    case spawn_do_dup2:
>>> -	      if (__dup2 (action->action.dup2_action.fd,
>>> -			  action->action.dup2_action.newfd)
>>> +	      /* Austin Group issue #411 requires adddup2 action with source
>>> +		 and destination being equal to remove close-on-exec flag.  */
>>> +	      if (action->action.dup2_action.fd
>>>   		  != action->action.dup2_action.newfd)
>>> -		goto fail;
>>> +		{
>>> +		  if (__dup2 (action->action.dup2_action.fd,
>>> +			     action->action.dup2_action.newfd)
>>> +		      != action->action.dup2_action.newfd)
>>> +		    goto fail;
>>> +		}
>>> +	      else
>>> +		{
>>> +		  int fd = action->action.dup2_action.newfd;
>>> +		  int flags = __fcntl (fd, F_GETFD, 0);
>>> +		  if (flags == -1)
>>> +		    goto fail;
>>> +		  if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>>> +		    goto fail;
>>> +		}

It might be simpler to write this as follows to flatten out the nesting 
a bit.  It also makes the location of the comment a lot more relevant 
because the clearing of the flag happens right under it:

     if (action->action.dup2_action.fd ==
         action->action.dup2_action.newfd)
       {
	int fd = action->action.dup2_action.newfd;
	int flags = __fcntl (fd, F_GETFD, 0);
	if (flags == -1)
	  goto fail;
	if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
	  goto fail;
       }
     else if (__dup2 (action->action.dup2_action.fd,
		     action->action.dup2_action.newfd)
	     != action->action.dup2_action.newfd)
       goto fail;

>>>   	      break;
>>>   	    }
>>>   	}
>>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>>> index 85239cedbf..b2304ffe60 100644
>>> --- a/sysdeps/unix/sysv/linux/spawni.c
>>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>>> @@ -253,10 +253,25 @@ __spawni_child (void *arguments)
>>>   	      break;
>>>   
>>>   	    case spawn_do_dup2:
>>> -	      if (__dup2 (action->action.dup2_action.fd,
>>> -			  action->action.dup2_action.newfd)
>>> +	      /* Austin Group issue #411 requires adddup2 action with source
>>> +		 and destination being equal to remove close-on-exec flag.  */
>>> +	      if (action->action.dup2_action.fd
>>>   		  != action->action.dup2_action.newfd)
>>> -		goto fail;
>>> +		{
>>> +		  if (__dup2 (action->action.dup2_action.fd,
>>> +			     action->action.dup2_action.newfd)
>>> +		      != action->action.dup2_action.newfd)
>>> +		    goto fail;
>>> +		}
>>> +	      else
>>> +		{
>>> +		  int fd = action->action.dup2_action.newfd;
>>> +		  int flags = __fcntl (fd, F_GETFD, 0);
>>> +		  if (flags == -1)
>>> +		    goto fail;
>>> +		  if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>>> +		    goto fail;
>>> +		}
>>>   	      break;
>>>   	    }
>>>   	}
>>>

Likewise.

Siddhesh

Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[Florian Weimer]

* adhemerval zanella:

> diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
> index 638ba1ba55..2354417020 100644
> --- a/posix/tst-spawn.c
> +++ b/posix/tst-spawn.c
> @@ -40,17 +40,19 @@ static int restart;
>  static char *name1;
>  static char *name2;
>  static char *name3;
> +static char *name5;
>  
>  /* Descriptors for the temporary files.  */
>  static int temp_fd1 = -1;
>  static int temp_fd2 = -1;
>  static int temp_fd3 = -1;
> +static int temp_fd5 = -1;

What happened to name4 and temp_fd4?  fe4string is not used, either.

Thanks,
Florian

Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[Adhemerval Zanella]

On 02/01/2019 09:07, Florian Weimer wrote:
> * adhemerval zanella:
> 
>> diff --git a/posix/tst-spawn.c b/posix/tst-spawn.c
>> index 638ba1ba55..2354417020 100644
>> --- a/posix/tst-spawn.c
>> +++ b/posix/tst-spawn.c
>> @@ -40,17 +40,19 @@ static int restart;
>>  static char *name1;
>>  static char *name2;
>>  static char *name3;
>> +static char *name5;
>>  
>>  /* Descriptors for the temporary files.  */
>>  static int temp_fd1 = -1;
>>  static int temp_fd2 = -1;
>>  static int temp_fd3 = -1;
>> +static int temp_fd5 = -1;
> 
> What happened to name4 and temp_fd4?  fe4string is not used, either.
> 
> Thanks,
> Florian

I used fd5 for consistency with handle_restart local fdX and do_test
fdXname.

Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[Adhemerval Zanella]

On 31/12/2018 14:34, Siddhesh Poyarekar wrote:
> My mail client ate the original submission, so responding to your ping.
> 
> On 13/12/18 2:54 AM, Adhemerval Zanella wrote:
>> Ping (x2).
>>
>> On 29/10/2018 16:32, Adhemerval Zanella wrote:
>>> Ping.
>>>
>>> On 19/09/2018 19:46, adhemerval.zanella@linaro.org wrote:
>>>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>>>
>>>> Austin Group issue #411 [1] proposes that posix_spawn file action
>>>> posix_spawn_file_actions_adddup2 resets the close-on-exec when
>>>> source and destination refer to same file descriptor.
>>>>
>>>> It solves the issue on multi-thread applications which uses
>>>> close-on-exec as default, and want to hand-chose specifically
>>>> file descriptor to purposefully inherited into a child process.
>>>> Current approach to achieve this scenario is to use two adddup2 file
>>>> actions and a temporary file description which do not conflict with
>>>> any other, coupled with a close file action to avoid leaking the
>>>> temporary file descriptor.  This approach, besides being complex,
>>>> may fail with EMFILE/ENFILE file descriptor exaustion.
>>>>
>>>> This can be more easily accomplished with an in-place removal of
>>>> FD_CLOEXEC.  Although the resulting adddup2 semantic is slight
>>>> different than dup2 (equal file descriptors should be handled as
>>>> no-op), the proposed possible solution are either more complex
>>>> (fcntl action which a limited set of operations) or results in
>>>> unrequired operations (dup3 which also returns EINVAL for same
>>>> file descriptor).
>>>>
>>>> Checked on aarch64-linux-gnu.
>>>>
> 
> BZ # on ChangeLog.

Fixed.

>>>> diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
>>>> index b138ab4393..d322db5c19 100644
>>>> --- a/sysdeps/posix/spawni.c
>>>> +++ b/sysdeps/posix/spawni.c
>>>> @@ -204,10 +204,25 @@ __spawni_child (void *arguments)
>>>>             break;
>>>>             case spawn_do_dup2:
>>>> -          if (__dup2 (action->action.dup2_action.fd,
>>>> -              action->action.dup2_action.newfd)
>>>> +          /* Austin Group issue #411 requires adddup2 action with source
>>>> +         and destination being equal to remove close-on-exec flag.  */
>>>> +          if (action->action.dup2_action.fd
>>>>             != action->action.dup2_action.newfd)
>>>> -        goto fail;
>>>> +        {
>>>> +          if (__dup2 (action->action.dup2_action.fd,
>>>> +                 action->action.dup2_action.newfd)
>>>> +              != action->action.dup2_action.newfd)
>>>> +            goto fail;
>>>> +        }
>>>> +          else
>>>> +        {
>>>> +          int fd = action->action.dup2_action.newfd;
>>>> +          int flags = __fcntl (fd, F_GETFD, 0);
>>>> +          if (flags == -1)
>>>> +            goto fail;
>>>> +          if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>>>> +            goto fail;
>>>> +        }
> 
> It might be simpler to write this as follows to flatten out the nesting a bit.  It also makes the location of the comment a lot more relevant because the clearing of the flag happens right under it:

I followed your suggestion, thanks.

> 
>     if (action->action.dup2_action.fd ==
>         action->action.dup2_action.newfd)
>       {
>     int fd = action->action.dup2_action.newfd;
>     int flags = __fcntl (fd, F_GETFD, 0);
>     if (flags == -1)
>       goto fail;
>     if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>       goto fail;
>       }
>     else if (__dup2 (action->action.dup2_action.fd,
>              action->action.dup2_action.newfd)
>          != action->action.dup2_action.newfd)
>       goto fail;
> 
>>>>             break;
>>>>           }
>>>>       }
>>>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>>>> index 85239cedbf..b2304ffe60 100644
>>>> --- a/sysdeps/unix/sysv/linux/spawni.c
>>>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>>>> @@ -253,10 +253,25 @@ __spawni_child (void *arguments)
>>>>             break;
>>>>             case spawn_do_dup2:
>>>> -          if (__dup2 (action->action.dup2_action.fd,
>>>> -              action->action.dup2_action.newfd)
>>>> +          /* Austin Group issue #411 requires adddup2 action with source
>>>> +         and destination being equal to remove close-on-exec flag.  */
>>>> +          if (action->action.dup2_action.fd
>>>>             != action->action.dup2_action.newfd)
>>>> -        goto fail;
>>>> +        {
>>>> +          if (__dup2 (action->action.dup2_action.fd,
>>>> +                 action->action.dup2_action.newfd)
>>>> +              != action->action.dup2_action.newfd)
>>>> +            goto fail;
>>>> +        }
>>>> +          else
>>>> +        {
>>>> +          int fd = action->action.dup2_action.newfd;
>>>> +          int flags = __fcntl (fd, F_GETFD, 0);
>>>> +          if (flags == -1)
>>>> +            goto fail;
>>>> +          if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>>>> +            goto fail;
>>>> +        }
>>>>             break;
>>>>           }
>>>>       }
>>>>
> 
> Likewise.
> 
> Siddhesh

Pushed as 805334b26c.

Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[Gabriel F. T. Gomes]

On Thu, Jan 03 2019, Adhemerval Zanella wrote:
> Pushed as 805334b26c.

After this commit, tst-spawn and tst-spawn-static are failing for me,
with the following error messages:

tst-spawn.c:128: numeric comparison failure
   left: 29 (0x1d); from: errno
  right: 9 (0x9); from: EBADF
error: 1 test failures
tst-spawn.c:128: numeric comparison failure
   left: 29 (0x1d); from: errno
  right: 9 (0x9); from: EBADF
error: 1 test failures
tst-spawn.c:252: numeric comparison failure
   left: 1 (0x1); from: WEXITSTATUS (status)
  right: 0 (0x0); from: 0
tst-spawn.c:257: numeric comparison failure
   left: 1 (0x1); from: WEXITSTATUS (status)
  right: 0 (0x0); from: 0
error: 2 test failures

And it's weird, because tst-spawn.c:128 is not a test for the newly
added fd5, it's an old test for fd1.

Have you seen similar results somewhere else?

Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[Andreas Schwab]

On Sep 19 2018, adhemerval.zanella@linaro.org wrote:

> @@ -141,21 +156,24 @@ do_test (int argc, char *argv[])
>         + "--library-path"	optional
>         + the library path	optional
>         + the application name
> -     - five parameters left if called through re-execution
> +     - six parameters left if called through re-execution
>         + file descriptor number which is supposed to be closed
>         + the open file descriptor
>         + the newly opened file descriptor
> -       + thhe duped second descriptor
> +       + the duped second descriptor
>         + the name of the closed descriptor
> +       + the duped fourth dile descriptor which O_CLOEXEC should be

s/dile/file/

> @@ -202,6 +228,7 @@ do_test (int argc, char *argv[])
>    spargv[i++] = fd3name;
>    spargv[i++] = fd4name;
>    spargv[i++] = name1;
> +  spargv[i++] = fd5name;
>    spargv[i] = NULL;

This overruns spargv.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."