Mailing List Archive: Perl: porters

[perl #108798] PL_check is not thread-safe

Index | Next | Previous | View Threaded

perlbug-followup at perl

Jan 22, 2012, 4:40 PM

Post #1 of 6 (50 views)
Permalink

[perl #108798] PL_check is not thread-safe

# New Ticket Created by Father Chrysostomos
# Please include the string: [perl #108798]
# in the subject line of all future correspondence about this issue.
# <URL: https://rt.perl.org:443/rt3/Ticket/Display.html?id=108798 >

See the thread starting at:

http://www.nntp.perl.org/group/perl.perl5.porters/;msgid=12CCDF0E-9CEE-41E3-AEC1-BD8258AFDB4F [at] cpan

Options are:

• Provide a mutex
• Have modules use the op mutex
• Make it an interpreter var
• Completely new interface
---
Flags:
category=core
severity=low
---
Site configuration information for perl 5.15.6:

Configured by sprout at Mon Jan 16 08:41:41 PST 2012.

Summary of my perl5 (revision 5 version 15 subversion 6) configuration:
Commit id: bdd2b37b5c6133a3b8647bb19d63f9598457d4d4
Platform:
osname=darwin, osvers=10.5.0, archname=darwin-2level
uname='darwin pint.local 10.5.0 darwin kernel version 10.5.0: fri nov 5 23:20:39 pdt 2010; root:xnu-1504.9.17~1release_i386 i386 '
config_args='-de -Dusedevel -DDEBUGGING'
hint=recommended, useposix=true, d_sigaction=define
useithreads=undef, usemultiplicity=undef
useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
use64bitint=undef, use64bitall=undef, uselongdouble=undef
usemymalloc=n, bincompat5005=undef
Compiler:
cc='cc', ccflags ='-fno-common -DPERL_DARWIN -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include',
optimize='-O3 -g',
cppflags='-fno-common -DPERL_DARWIN -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
ccversion='', gccversion='4.2.1 (Apple Inc. build 5664)', gccosandvers=''
intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
alignbytes=8, prototype=define
Linker and Libraries:
ld='env MACOSX_DEPLOYMENT_TARGET=10.3 cc', ldflags =' -fstack-protector -L/usr/local/lib'
libpth=/usr/local/lib /usr/lib
libs=-ldbm -ldl -lm -lutil -lc
perllibs=-ldl -lm -lutil -lc
libc=, so=dylib, useshrplib=false, libperl=libperl.a
gnulibc_version=''
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=bundle, d_dlsymun=undef, ccdlflags=' '
cccdlflags=' ', lddlflags=' -bundle -undefined dynamic_lookup -L/usr/local/lib -fstack-protector'

Locally applied patches:

---
@INC for perl 5.15.6:
/usr/local/lib/perl5/site_perl/5.15.6/darwin-2level
/usr/local/lib/perl5/site_perl/5.15.6
/usr/local/lib/perl5/5.15.6/darwin-2level
/usr/local/lib/perl5/5.15.6
/usr/local/lib/perl5/site_perl
.

---
Environment for perl 5.15.6:
DYLD_LIBRARY_PATH (unset)
HOME=/Users/sprout
LANG=en_US.UTF-8
LANGUAGE (unset)
LD_LIBRARY_PATH (unset)
LOGDIR (unset)
PATH=/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/X11/bin:/usr/local/bin
PERL_BADLANG (unset)
SHELL=/bin/bash

zefram at fysh

Feb 8, 2012, 5:38 AM

Post #2 of 6 (31 views)
Permalink

Re: [perl #108798] PL_check is not thread-safe [In reply to]

Father Chrysostomos wrote:
>Options are:
>
>* Provide a mutex
>* Have modules use the op mutex
>* Make it an interpreter var
>* Completely new interface

I'm now working on making my check-hooking modules thread safe, so I've
looked at the issues here in some detail.

Firstly, I've determined that Perl's "global" variables really are
always once-per-process, despite the fuss about them sometimes being in
an indirectly-accessed struct. Making PL_check an interpreter variable
doesn't seem useful. It's not a priori problematic for checkers to be
process-global, and indeed it's convenient for a module to be able to use
a static variable to hold the address of the next checker in the chain.
The performance win of avoiding a complex lookup of per-thread module
state also shouldn't be overlooked, given how low-level the op chaining
mechanism is. (Of course, in any case I have to deal with existing
Perls where PL_check is unavoidably global.)

There are two thread safety issues in hooking an op checker. One is
the very brief race condition between two threads (running actually
simultaneously or with preemptive scheduling) that attempt to hook
at the same time. Fixing this requires a mutex. In the future the
core should provide a specific mutex for this, but for retrospective
use on old Perls I'm adopting the op refcount mutex. This is a purely
conventional choice; it works as long as everyone writing such modules
adopts the same convention.

The second issue is of a module being bootstrapped more than once
in a single process. This can be made to happen very reliably, by
(for example) loading the module in two threads, having not previously
loaded it in the main program. Naive hooking, such as my modules have
used so far, results in the checking chain becoming a loop, leading
to hangs or crashes. I think a couple of my RT bug reports come from
this cause. To fix it, the BOOT code needs to recognise that it can
be called more than once per process, and arrange to be idempotent with
respect to globals. It needs to not hook the checker if it has already
done so in this process. The easiest test of whether it's already been
hooked is simply whether the next-checker variable has been filled.

A convenience function for hooking ought to address both of these inherent
thread safety issues. Here is the code that I've now got in my latest
dev version of Devel::CallChecker, and which I'm intending to clone into
each of my op check hooking modules:

#ifndef wrap_op_checker
# define wrap_op_checker(c,n,o) THX_wrap_op_checker(aTHX_ c,n,o)
static void THX_wrap_op_checker(pTHX_ Optype opcode,
Perl_check_t new_checker, Perl_check_t *old_checker_p)
{
if(*old_checker_p) return;
OP_REFCNT_LOCK;
if(!*old_checker_p) {
*old_checker_p = PL_check[opcode];
PL_check[opcode] = new_checker;
}
OP_REFCNT_UNLOCK;
}
#endif /* !wrap_op_checker */

I propose the adoption of this code for all op check hooking modules, and
furthermore that the core in future should provide an equivalent function
with exactly this API (but, of course, using a purpose-specific mutex).
The #ifndef aspect of the code I'm using will yield a seamless transition
if the core adopts exactly this API.

If there are no objections, I'd like to add this to the core this week.
This is the last chance to get it in for 5.16.

-zefram

arc at cpan

Feb 8, 2012, 7:42 AM

Post #3 of 6 (32 views)
Permalink

Re: [perl #108798] PL_check is not thread-safe [In reply to]

Zefram <zefram [at] fysh> wrote:
> static void THX_wrap_op_checker(pTHX_ Optype opcode,
> Perl_check_t new_checker, Perl_check_t *old_checker_p)
> {
> if(*old_checker_p) return;
> OP_REFCNT_LOCK;
> if(!*old_checker_p) {
> *old_checker_p = PL_check[opcode];
> PL_check[opcode] = new_checker;
> }
> OP_REFCNT_UNLOCK;
> }

Do we support any platforms where an access to a Perl_check_t might
not be atomic? (For that matter, do any such platforms exist? Since
it's a function pointer, that would presumably mean 64-bit platforms
which only do atomic memory accesses of up to 32 bits.)

If so, I think the OP_REFCNT_LOCK needs to precede the initial
test/return, to avoid this race:

1. Thread A loads the first half of *old_checker_p, which is all-bits-zero
2. Thread B takes the lock
3. Thread B writes 64 bits into *old_checker_p; the second half
happens to be all-bits-zero
4. Thread B releases the lock
5. Thread A loads the second half of the new value of *old_checker_p
(all-bits-zero again)
6. Thread A thinks the full value was 0, so continues; but it wasn't,
so it should have returned

Or am I misunderstanding something?

--
Aaron Crane ** http://aaroncrane.co.uk/

public at khwilliamson

Feb 8, 2012, 8:06 AM

Post #4 of 6 (31 views)
Permalink

Re: [perl #108798] PL_check is not thread-safe [In reply to]

On 02/08/2012 06:38 AM, Zefram wrote:
> Father Chrysostomos wrote:
>> Options are:
>>
>> * Provide a mutex
>> * Have modules use the op mutex
>> * Make it an interpreter var
>> * Completely new interface
>
> I'm now working on making my check-hooking modules thread safe, so I've
> looked at the issues here in some detail.
>
> Firstly, I've determined that Perl's "global" variables really are
> always once-per-process, despite the fuss about them sometimes being in
> an indirectly-accessed struct. Making PL_check an interpreter variable
> doesn't seem useful. It's not a priori problematic for checkers to be
> process-global, and indeed it's convenient for a module to be able to use
> a static variable to hold the address of the next checker in the chain.
> The performance win of avoiding a complex lookup of per-thread module
> state also shouldn't be overlooked, given how low-level the op chaining
> mechanism is. (Of course, in any case I have to deal with existing
> Perls where PL_check is unavoidably global.)
>
> There are two thread safety issues in hooking an op checker. One is
> the very brief race condition between two threads (running actually
> simultaneously or with preemptive scheduling) that attempt to hook
> at the same time. Fixing this requires a mutex. In the future the
> core should provide a specific mutex for this, but for retrospective
> use on old Perls I'm adopting the op refcount mutex. This is a purely
> conventional choice; it works as long as everyone writing such modules
> adopts the same convention.
>
> The second issue is of a module being bootstrapped more than once
> in a single process. This can be made to happen very reliably, by
> (for example) loading the module in two threads, having not previously
> loaded it in the main program. Naive hooking, such as my modules have
> used so far, results in the checking chain becoming a loop, leading
> to hangs or crashes. I think a couple of my RT bug reports come from
> this cause. To fix it, the BOOT code needs to recognise that it can
> be called more than once per process, and arrange to be idempotent with
> respect to globals. It needs to not hook the checker if it has already
> done so in this process. The easiest test of whether it's already been
> hooked is simply whether the next-checker variable has been filled.
>
> A convenience function for hooking ought to address both of these inherent
> thread safety issues. Here is the code that I've now got in my latest
> dev version of Devel::CallChecker, and which I'm intending to clone into
> each of my op check hooking modules:
>
> #ifndef wrap_op_checker
> # define wrap_op_checker(c,n,o) THX_wrap_op_checker(aTHX_ c,n,o)
> static void THX_wrap_op_checker(pTHX_ Optype opcode,
> Perl_check_t new_checker, Perl_check_t *old_checker_p)
> {
> if(*old_checker_p) return;
> OP_REFCNT_LOCK;
> if(!*old_checker_p) {
> *old_checker_p = PL_check[opcode];
> PL_check[opcode] = new_checker;
> }
> OP_REFCNT_UNLOCK;
> }
> #endif /* !wrap_op_checker */
>
> I propose the adoption of this code for all op check hooking modules, and
> furthermore that the core in future should provide an equivalent function
> with exactly this API (but, of course, using a purpose-specific mutex).
> The #ifndef aspect of the code I'm using will yield a seamless transition
> if the core adopts exactly this API.
>
> If there are no objections, I'd like to add this to the core this week.
> This is the last chance to get it in for 5.16.
>
> -zefram
>

+1

zefram at fysh

Feb 8, 2012, 2:30 PM

Post #5 of 6 (32 views)
Permalink

Re: [perl #108798] PL_check is not thread-safe [In reply to]

Aaron Crane wrote:
>Do we support any platforms where an access to a Perl_check_t might
>not be atomic?
...
>If so, I think the OP_REFCNT_LOCK needs to precede the initial
>test/return, to avoid this race:

If so, then we'd need a mutex around all reads of PL_check entries during
op checking. Currently they're unlocked. I think for pointer reading to
be non-atomic is rather unlikely, and would cause a great many problems
of this nature all over the code. Not worth worrying about.

(Btw, if pointers cannot be read atomically then you don't move the lock
operation there, you remove the inital check entirely. It's redundant
with the check that occurs in the locked section: it exists only as an
optimisation, to avoid the expense of acquiring the lock in a common
case.)

-zefram

zefram at fysh

Feb 11, 2012, 3:38 AM

Post #6 of 6 (26 views)
Permalink

Re: [perl #108798] PL_check is not thread-safe [In reply to]

I wrote:
>furthermore that the core in future should provide an equivalent function
>with exactly this API (but, of course, using a purpose-specific mutex).

Now implemented as e8570548af49b057631f1011e4b19c8c4a1342dd. I would
have liked to add similar functions to wrap PL_peep/PL_rpeep, but those
are per-interpreter variables, so need different handling.

-zefram

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads