On Tue, Oct 14, 2014 at 02:53:25PM +0900, Daiki Ueno wrote:
> Pádraig Brady <p...@draigbrady.com> writes:
> > You mentioned it would be conditionalized on pthread_is_threaded_np
> > for performance I suppose, as threaded progs would already
> > have to deal with the consequences of a separate thread.
> > So races in pthread_is_threaded_np would only be a small
> > performance issue.
It is not merely for performance; per the report Daiki Ueno cited in the lead
message of this thread, gl_locale_name_default() callees are known to crash if
run in a forked copy of a multi-threaded program. Also, thread cancellation
at a bad moment in gl_locale_name_default_from_CoreFoundation_forked() would
leak file descriptors and a child process.
> Perhaps it might require a copyright assignment
> from the original author (Cc'ed).
No problem. I will send you an off-list message verifying my assignment.
> Subject: [PATCH] localename: Avoid implicit thread creation in CoreFoundation
This patch supposes that if the process is multi-threaded, it's okay to add more
threads. Problems would remain in a process that becomes multi-threaded, then
expects to become single-threaded again. Example timeline:
Thread 1: pthread_create()
Thread 1: gl_locale_name_default()
Thread 2: finish execution
Thread 1: signal(), sigprocmask(), fork(), etc
Note also that pthread_is_threaded_np() returns 1 if the current process was
ever multi-threaded, not just if it is multi-threaded now. As such, swapping
the two middle steps in that example retains the hazard. Even so, I think
your patch won't make anything worse. It helps programs that call setlocale()
before starting any thread, and it changes nothing for other programs. It
covers PostgreSQL's needs perfectly. +1 from me.
> + if (close (fd[1]) < 0)
> + {
> + close (fd[0]);
> + goto done;
If the function follows this goto, it neglects waitpid().
Though orthogonal to this patch, libintl_setlocale() and libintl_newlocale()
would do better to return 0, not "C", after such an internal error.
Thanks,
nm
