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

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> 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).

“multithreaded environment”

Much clearer, thanks.
>>> +	/* 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

Okay, thanks for providing the reference.

It think the fix is incorrect in the sense that POSIX is wrong here.  As
specified, popen can launch the new subprocess with a close standard
input/output/error if it came from popen in the parent process, which is
wrong.  I believe the actual check should be against fd <= 2, not fd !=
child_std_end.  But this is a separate discussion, not related to this
patch.

But looking at this, I realized that we have a race condition due to the
use of proc_file_chain_lock: A concurrent call to popen may add a
descriptor to the chain after the list traversal and before the
posix_spawn call.  Or the descriptor might now be something else because
pclose was called concurrently.  The old code did not have this
particular problem because after the fork, the set of open descriptors
was stable.

Do you thin kit would be acceptable to extend the scope of the critical
section to include the posix_spawn call?  This obviously reduces
concurrency somewhat.  I don't think there is a lock ordering issue,
though.

Thanks,
Florian