[v2,5/5] posix: Use posix_spawn for wordexp

  On 07/10/2019 16:33, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c
>> index 10a0768a6b..ef780b0a65 100644
>> --- a/posix/wordexp-test.c
>> +++ b/posix/wordexp-test.c
> 
>> -/* For each fork increment the fork count.  */
>> -static void
>> -register_fork (void)
>> -{
>> -  registered_forks++;
>> -}
> 
> It's a bit sad to see this testing go away.  It was originally added to
> catch command execution with WRDE_NOCMD.
> 
> On Linux, could you enter a PID namespace instead and check that the
> next PID has the expected value?
> 
> Carlos, you added this testing.  Do you have an opinion here?
> 
>> diff --git a/posix/wordexp.c b/posix/wordexp.c
>> index 22c6d18a9c..e1aafcaceb 100644
>> --- a/posix/wordexp.c
>> +++ b/posix/wordexp.c
> 
>> +static char **
>> +create_environment (void)
>> +{
>> +  size_t s = 0;
>> +
>> +  /* Calculate total environment size, including 'IFS' if is present.  */
>> +  for (char **ep = __environ; *ep != NULL; ep++, s++);
> 
> I would put s++ into the body of the for loop, for clarity.  Or give ep
> a wider scope and use s = ep -- __environ.

Ack, I moved s++ to main loop.

> 
>> +  /* Include final NULL pointer.  */
>> +  char **newenviron = malloc (s * sizeof (char*));
>> +  if (newenviron == NULL)
>> +    return NULL;
> 
> char* should be char *.  I don't see how this includes the final NULL?
> 
> Should we do all this work only if IFS= is actually present?  That is,
> skip all this for getenv ("IFS) == NULL?

Ack.

> 
>> +  /* Copy current environment excluding 'IFS', to make sure the subshell
>> +     doesn't field-split on our behalf. */
> 
> That comment should apply to the entire function, I think.

Ack.

> 
>> @@ -884,13 +898,13 @@ exec_comm (char *comm, char **word, size_t *word_length, size_t *max_length,
>>    size_t maxnewlines = 0;
>>    char buffer[bufsize];
>>    pid_t pid;
>> -  int noexec = 0;
>> +  bool noexec = false;
>>  
>>    /* Do nothing if command substitution should not succeed.  */
>>    if (flags & WRDE_NOCMD)
>>      return WRDE_CMDSUB;
>>  
>> -  /* Don't fork() unless necessary */
>> +  /* Don't posix_spawn() unless necessary */
> 
> GNU style doesn't use () after function names.

Ack.

I fixed your remarks, rebased against master to pick-up your changed to 
wordexp tests and used dynarray to construct the new environment (it 
simplifies a bit the creation on a new environment for the IFS case).

	* include/spawn.h (__posix_spawn_file_actions_addopen): New
	prototype.
	* posix/spawn_faction_addopen.c (posix_spawn_file_actions_addopen):
	Add internal alias.
	* posix/wordexp.c (create_environment, free_environment): New
	functions.
	(exec_comm_child, exec_comm): Use posix_spawn instead of fork+exec.
	* posix/wordexp-test.c: Use libsupport and remove atfork usage.
---

[v2,5/5] posix: Use posix_spawn for wordexp

Commit Message

Comments

Patch