This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 1/2] posix: Use posix_spawn on popen
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Florian Weimer <fweimer at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 28 Nov 2018 17:27:57 -0200
- Subject: Re: [PATCH v3 1/2] posix: Use posix_spawn on popen
- References: <20181025174103.31596-1-adhemerval.zanella@linaro.org> <87ftvlcigq.fsf@oldenburg.str.redhat.com>
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