* [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
2018-09-19 22:46 [PATCH 1/2] Use libsupport for tst-spawn.c adhemerval.zanella
@ 2018-09-19 22:46 ` adhemerval.zanella
2018-10-29 20:05 ` Adhemerval Zanella
` (2 more replies)
2018-09-20 5:19 ` [PATCH 1/2] Use libsupport for tst-spawn.c Florian Weimer
1 sibling, 3 replies; 15+ messages in thread
From: adhemerval.zanella @ 2018-09-19 22:46 UTC (permalink / raw)
To: libc-alpha; +Cc: 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] Use libsupport for tst-spawn.c
@ 2018-09-19 22:46 adhemerval.zanella
2018-09-19 22:46 ` [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640) adhemerval.zanella
2018-09-20 5:19 ` [PATCH 1/2] Use libsupport for tst-spawn.c Florian Weimer
0 siblings, 2 replies; 15+ messages in thread
From: adhemerval.zanella @ 2018-09-19 22:46 UTC (permalink / raw)
To: libc-alpha; +Cc: 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Use libsupport for tst-spawn.c
2018-09-19 22:46 [PATCH 1/2] Use libsupport for tst-spawn.c adhemerval.zanella
2018-09-19 22:46 ` [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640) adhemerval.zanella
@ 2018-09-20 5:19 ` Florian Weimer
2018-09-20 17:14 ` Adhemerval Zanella
1 sibling, 1 reply; 15+ messages in thread
From: Florian Weimer @ 2018-09-20 5:19 UTC (permalink / raw)
To: adhemerval.zanella; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Use libsupport for tst-spawn.c
2018-09-20 5:19 ` [PATCH 1/2] Use libsupport for tst-spawn.c Florian Weimer
@ 2018-09-20 17:14 ` Adhemerval Zanella
2018-09-21 12:56 ` Florian Weimer
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2018-09-20 17:14 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Use libsupport for tst-spawn.c
2018-09-20 17:14 ` Adhemerval Zanella
@ 2018-09-21 12:56 ` Florian Weimer
2018-09-21 17:23 ` Adhemerval Zanella
0 siblings, 1 reply; 15+ messages in thread
From: Florian Weimer @ 2018-09-21 12:56 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Use libsupport for tst-spawn.c
2018-09-21 12:56 ` Florian Weimer
@ 2018-09-21 17:23 ` Adhemerval Zanella
2018-09-22 11:59 ` Florian Weimer
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2018-09-21 17:23 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] Use libsupport for tst-spawn.c
2018-09-21 17:23 ` Adhemerval Zanella
@ 2018-09-22 11:59 ` Florian Weimer
0 siblings, 0 replies; 15+ messages in thread
From: Florian Weimer @ 2018-09-22 11:59 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
2018-09-19 22:46 ` [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640) adhemerval.zanella
@ 2018-10-29 20:05 ` Adhemerval Zanella
2018-12-12 21:32 ` Adhemerval Zanella
2019-01-02 11:07 ` Florian Weimer
2019-01-04 21:37 ` Andreas Schwab
2 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2018-10-29 20:05 UTC (permalink / raw)
To: GNU C Library
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;
> }
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
2018-10-29 20:05 ` Adhemerval Zanella
@ 2018-12-12 21:32 ` Adhemerval Zanella
2018-12-31 16:59 ` Siddhesh Poyarekar
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2018-12-12 21:32 UTC (permalink / raw)
To: GNU C Library
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;
>> }
>> }
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
2018-12-12 21:32 ` Adhemerval Zanella
@ 2018-12-31 16:59 ` Siddhesh Poyarekar
2019-01-03 16:49 ` Adhemerval Zanella
0 siblings, 1 reply; 15+ messages in thread
From: Siddhesh Poyarekar @ 2018-12-31 16:59 UTC (permalink / raw)
To: Adhemerval Zanella, GNU C Library
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
2018-09-19 22:46 ` [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640) adhemerval.zanella
2018-10-29 20:05 ` Adhemerval Zanella
@ 2019-01-02 11:07 ` Florian Weimer
2019-01-02 13:39 ` Adhemerval Zanella
2019-01-04 21:37 ` Andreas Schwab
2 siblings, 1 reply; 15+ messages in thread
From: Florian Weimer @ 2019-01-02 11:07 UTC (permalink / raw)
To: adhemerval.zanella; +Cc: libc-alpha
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
2019-01-02 11:07 ` Florian Weimer
@ 2019-01-02 13:39 ` Adhemerval Zanella
0 siblings, 0 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2019-01-02 13:39 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
2018-12-31 16:59 ` Siddhesh Poyarekar
@ 2019-01-03 16:49 ` Adhemerval Zanella
2019-01-04 20:49 ` Gabriel F. T. Gomes
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2019-01-03 16:49 UTC (permalink / raw)
To: Siddhesh Poyarekar, GNU C Library
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
2019-01-03 16:49 ` Adhemerval Zanella
@ 2019-01-04 20:49 ` Gabriel F. T. Gomes
0 siblings, 0 replies; 15+ messages in thread
From: Gabriel F. T. Gomes @ 2019-01-04 20:49 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Siddhesh Poyarekar, GNU C Library
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?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)
2018-09-19 22:46 ` [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640) adhemerval.zanella
2018-10-29 20:05 ` Adhemerval Zanella
2019-01-02 11:07 ` Florian Weimer
@ 2019-01-04 21:37 ` Andreas Schwab
2 siblings, 0 replies; 15+ messages in thread
From: Andreas Schwab @ 2019-01-04 21:37 UTC (permalink / raw)
To: adhemerval.zanella; +Cc: libc-alpha
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."
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-01-04 21:37 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-19 22:46 [PATCH 1/2] Use libsupport for tst-spawn.c adhemerval.zanella
2018-09-19 22:46 ` [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640) adhemerval.zanella
2018-10-29 20:05 ` Adhemerval Zanella
2018-12-12 21:32 ` Adhemerval Zanella
2018-12-31 16:59 ` Siddhesh Poyarekar
2019-01-03 16:49 ` Adhemerval Zanella
2019-01-04 20:49 ` Gabriel F. T. Gomes
2019-01-02 11:07 ` Florian Weimer
2019-01-02 13:39 ` Adhemerval Zanella
2019-01-04 21:37 ` Andreas Schwab
2018-09-20 5:19 ` [PATCH 1/2] Use libsupport for tst-spawn.c Florian Weimer
2018-09-20 17:14 ` Adhemerval Zanella
2018-09-21 12:56 ` Florian Weimer
2018-09-21 17:23 ` Adhemerval Zanella
2018-09-22 11:59 ` Florian Weimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).