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

Message ID 20180919224624.3920-2-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • [1/2] Use libsupport for tst-spawn.c
Related show

Commit Message

Adhemerval Zanella Sept. 19, 2018, 10:46 p.m.
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(-)

-- 
2.17.1

Comments

Adhemerval Zanella Oct. 29, 2018, 7:32 p.m. | #1
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;

>  	    }

>  	}

>
Adhemerval Zanella Dec. 12, 2018, 9:24 p.m. | #2
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;

>>  	    }

>>  	}

>>
Siddhesh Poyarekar Dec. 31, 2018, 4:34 p.m. | #3
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
Florian Weimer Jan. 2, 2019, 11:07 a.m. | #4
* 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
Adhemerval Zanella Jan. 2, 2019, 1:39 p.m. | #5
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.
Adhemerval Zanella Jan. 3, 2019, 4:49 p.m. | #6
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.
Gabriel F. T. Gomes Jan. 4, 2019, 8:49 p.m. | #7
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?
Andreas Schwab Jan. 4, 2019, 9:37 p.m. | #8
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."

Patch

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;
 	    }
 	}