[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [bug #43404] gl_locale_name_default() thread issues on OS X
From: |
Noah Misch |
Subject: |
Re: [bug #43404] gl_locale_name_default() thread issues on OS X |
Date: |
Thu, 16 Oct 2014 01:09:06 -0400 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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