This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v3 1/2] posix: Use posix_spawn on popen



On 28/11/2018 13:47, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>   * BZ#22834: now that proc_file_chain is not copied on another process, it
>>     just require to access is through the proc_file_chain_lock.
> 
> Sorry, what does that mean? (“copied on another process” in particular.)

What I mean is with posix_spawn the scenario described in BZ#22834, where the
forked process might contain invalid internal state when updating
proc_file_chain should not happen because helper process used by
posix_spawn does not access proc_file_chain.  The wording I used indeed
might be improved, what about:

  * BZ#22834: the described scenario, where the forked process might access
    invalid memory due an inconsistent state in multithread environment,
    should not happen because posix_spawn does not access the affected
    data structure (proc_file_chain).

> 
>> +* The popen and system do not run atfork handlers anymore (BZ#17490).
>> +  Although it is a possible POSIX violation, the POSIX rationale in
>> +  pthread_atfork documentation regarding atfork handlers is to handle
>> +  incosistent mutex state after fork call in multithread environment.
>> +  In both popen and system there is no direct access to user-defined mutexes.
>> +
>>  Deprecated and removed features, and other changes affecting compatibility:
>>  
>>  * The glibc.tune tunable namespace has been renamed to glibc.cpu and the
>> diff --git a/libio/iopopen.c b/libio/iopopen.c
>> index 2eff45b4c8..da24e60eef 100644
>> --- a/libio/iopopen.c
>> +++ b/libio/iopopen.c
>> @@ -34,7 +34,8 @@
>>  #include <not-cancel.h>
>>  #include <sys/types.h>
>>  #include <sys/wait.h>
>> -#include <kernel-features.h>
>> +#include <spawn.h>
>> +#include <paths.h>
>>  
>>  struct _IO_proc_file
>>  {
>> @@ -64,8 +65,8 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode)
>>  {
>>    int read_or_write;
>>    int parent_end, child_end;
>> +  int child_std_end;
> 
> Should this be child_pipe_fd or something more descriptive?  Perhaps
> also add a comment to the previous line that these are indexes into
> pipe_fds below?

I don't have a strong opinion here, I renamed child_std_end to child_pipe_fd
and add a comment stating parent_end, child_end are both indexes.

> 
>> +    if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[child_end],
>> +	child_std_end) != 0)
>> +      goto spawn_failure;
>> +
>> +    /* POSIX.2: "popen() shall ensure that any streams from previous popen()
>> +       calls that remain open in the parent process are closed in the new
>> +       child process." */
>> +    bool addclose_failure = false;
>> +#ifdef _IO_MTSAFE_IO
>> +    _IO_cleanup_region_start_noarg (unlock);
>> +    _IO_lock_lock (proc_file_chain_lock);
>> +#endif
>> +    for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next)
>> +      {
>> +	int fd = _IO_fileno ((FILE *) p);
>> +
>> +	/* If any stream from previous popen() calls has fileno
>> +	   child_send, it has been already closed by the dup2 syscall
>> +	   above.  */
> 
> Typo: child_send.  But I'm not sure how this can happen.  Shouldn't the
> descriptor still be open if the stream is linked into the chain?

Ack.

> 
> If not, that doesn't really fix the race.

This originally came from a RH issue [1] where the program:

---
#include <assert.h>
#include <stdio.h>

int main(void)
{
  FILE *p1, *p2;
  int result1, result2;
  
  fclose(stdout);
  p1 = popen("echo a", "r"); assert(p1 != NULL);
  p2 = popen("echo b", "r"); assert(p2 != NULL);
  result1 = pclose(p1); result2 = pclose(p2);
  fprintf(stderr, "result1 = %d, result2 = %d\n", result1, result2);
  return 0;
}
---

The issue is adddup2 action will dup2 the pipe child end to child_pipe_fd
(in this case '1' since it has closed stdout), so adding another action to
close is wrong.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=248281

> 
> Rest looks okay to me.

Ok to push with the modifications above?

> 
> Thanks,
> Florian


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]