Re: [bug-gettext] [bug #43404] gl_locale_name_default() thread issues on OS X

[Noah Misch]

On Tue, Oct 14, 2014 at 02:53:25PM +0900, Daiki Ueno wrote:
> Pádraig Brady <address@hidden> 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