From jgmyers@proofpoint.com

Created by jgmyers@proofpoint.com

The following test program crashes on a multiprocessor box. Wrapping
the closure inside of an eval eliminates the crash​:

  my $obj = save(eval 'sub { $bar = shift; }');

The various threads all share one underlying CV object; the object is
not marked permanent. The CV's refcount is modified without locking,
resulting in the CV being prematurely freed.

use threads;

sub init {
  my $thr;

  for (my $i = 0; $i < 8; $i++) {
  $thr = threads->new(\&task);
  }

  foreach $thr (threads->list) {
  if ($thr->tid && !threads​::equal($thr, threads->self)) {
  $thr->join;
  }
  }
}

sub save
{
  my $self = {};
  $self->{arg} = shift;
  return $self;
}

sub task {
  print "task starting\n";
  for (my $i = 1; $i < 10000; $i++) {
  my $bar;
  my $obj = save(sub { $bar = shift; });
  }
}

init();

Flags:
    category=core
    severity=medium

Site configuration information for perl v5.8.5:

Configured by jgmyers at Tue Sep 28 10:04:28 PDT 2004.

Summary of my perl5 (revision 5 version 8 subversion 5) configuration:
  Platform:
    osname=linux, osvers=2.4.21-20.elsmp, archname=i686-linux-thread-multi
    uname='linux pong 2.4.21-20.elsmp #1 smp wed aug 18 20:46:40 edt 
2004 i686 i686 i386 gnulinux '
    config_args=''
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=define use5005threads=undef useithreads=define 
usemultiplicity=define
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=undef use64bitall=undef uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS 
-DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include 
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I/usr/include/gdbm',
    optimize='-g',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBUGGING 
-fno-strict-aliasing -pipe -I/usr/local/include -I/usr/include/gdbm'
    ccversion='', gccversion='3.3.3', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', 
lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=/lib/libc-2.3.2.so, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.3.2'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E 
-Wl,-rpath,/u/jgmyers/perl/lib/5.8.5/i686-linux-thread-multi/CORE'
    cccdlflags='-fpic', lddlflags='-shared -L/usr/local/lib'

Locally applied patches:
   


@INC for perl v5.8.5:
    /u/jgmyers/perl/lib/5.8.5/i686-linux-thread-multi
    /u/jgmyers/perl/lib/5.8.5
    /u/jgmyers/perl/lib/site_perl/5.8.5/i686-linux-thread-multi
    /u/jgmyers/perl/lib/site_perl/5.8.5
    /u/jgmyers/perl/lib/site_perl/5.8.3/i686-linux-thread-multi
    /u/jgmyers/perl/lib/site_perl/5.8.3
    /u/jgmyers/perl/lib/site_perl
    .


Environment for perl v5.8.5:
    HOME=/u/jgmyers
    LANG=en_US
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    
PATH=/tools/x/bin:/usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin:/usr/X11R6/bin:/u/jgmyers/bin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

From @iabyn

On Tue, Oct 05, 2004 at 05​:25​:15PM -0000, John Gardiner Myers wrote​:

The following test program crashes on a multiprocessor box. Wrapping
the closure inside of an eval eliminates the crash​:

    my $obj = save\(eval 'sub \{ $bar = shift; \}'\);

The various threads all share one underlying CV object; the object is
not marked permanent. The CV's refcount is modified without locking,
resulting in the CV being prematurely freed.

Thanks for the report. Your analysis is *nearly* spot on ;-)
The threads don't share the CVs; a copy is cloned for each thread
that is created; however, all the CV structs share the same OP tree,
which is supposed to the read-only. The code that updates the refcount of
the op tree doesn't appear to be thread-safe.

op_free() does a OP_REFCNT_LOCK before decrementing it, but none of
the code that increments it appears to do so.

I'll try to look into this properly when I get more time.

(P5Pers​: is there any reason not to just wrap the OpREFCNT_inc/OpREFCNT_dec
macrcos with OP_REFCNT_LOCK/OP_REFCNT_UNLOCK ?)

Dave.

--
You live and learn (although usually you just live).

The RT System itself - Status changed from 'new' to 'open'

From @rgs

Dave Mitchell wrote​:

op_free() does a OP_REFCNT_LOCK before decrementing it, but none of
the code that increments it appears to do so.

I'll try to look into this properly when I get more time.

(P5Pers​: is there any reason not to just wrap the OpREFCNT_inc/OpREFCNT_dec
macrcos with OP_REFCNT_LOCK/OP_REFCNT_UNLOCK ?)

Seems sensible.

From @iabyn

On Thu, Oct 07, 2004 at 12​:16​:55AM +0100, Dave Mitchell wrote​:

Thanks for the report. Your analysis is *nearly* spot on ;-)
The threads don't share the CVs; a copy is cloned for each thread
that is created; however, all the CV structs share the same OP tree,
which is supposed to the read-only. The code that updates the refcount of
the op tree doesn't appear to be thread-safe.

op_free() does a OP_REFCNT_LOCK before decrementing it, but none of
the code that increments it appears to do so.

Now fixed by the change below.

It appears to make closure creation about 4% slower on ITHREAD builds.

Dave.

--
Nothing ventured, nothing lost.

Change 23433 by davem@​davem-splatty on 2004/10/29 21​:04​:17

  [perl #31851] Threading crash with closures
  various OpREFCNT_inc() operations weren't doing locking

Affected files ...

... //depot/perl/pad.c#37 edit
... //depot/perl/regcomp.c#340 edit
... //depot/perl/sv.c#767 edit

Differences ...

==== //depot/perl/pad.c#37 (text) ====

@​@​ -1410,7 +1410,9 @​@​
#endif
  CvGV(cv) = CvGV(proto);
  CvSTASH(cv) = CvSTASH(proto);
+ OP_REFCNT_LOCK;
  CvROOT(cv) = OpREFCNT_inc(CvROOT(proto));
+ OP_REFCNT_UNLOCK;
  CvSTART(cv) = CvSTART(proto);
  CvOUTSIDE(cv) = (CV*)SvREFCNT_inc(outside);
  CvOUTSIDE_SEQ(cv) = CvOUTSIDE_SEQ(proto);

==== //depot/perl/regcomp.c#340 (text) ====

@​@​ -4983,9 +4983,14 @​@​
  (SvTYPE(new_comppad) == SVt_PVAV) ?
  new_comppad : Null(PAD *)
  );
+ OP_REFCNT_LOCK;
  if (!OpREFCNT_dec((OP_4tree*)r->data->data[n])) {
+ OP_REFCNT_UNLOCK;
  op_free((OP_4tree*)r->data->data[n]);
  }
+ else {
+ OP_REFCNT_UNLOCK;
+ }

  PAD_RESTORE_LOCAL(old_comppad);
  SvREFCNT_dec((SV*)new_comppad);

==== //depot/perl/sv.c#767 (text) ====

@​@​ -10244,7 +10244,9 @​@​
  case 'o'​:
  /* Compiled op trees are readonly, and can thus be
  shared without duplication. */
+ OP_REFCNT_LOCK;
  d->data[i] = (void*)OpREFCNT_inc((OP*)r->data->data[i]);
+ OP_REFCNT_UNLOCK;
  break;
  case 'n'​:
  d->data[i] = r->data->data[i];
@​@​ -10955,7 +10957,9 @​@​
  Perl_rvpv_dup(aTHX_ dstr, sstr, param);
  CvSTASH(dstr) = hv_dup(CvSTASH(sstr), param); /* NOTE​: not refcounted */
  CvSTART(dstr) = CvSTART(sstr);
+ OP_REFCNT_LOCK;
  CvROOT(dstr) = OpREFCNT_inc(CvROOT(sstr));
+ OP_REFCNT_UNLOCK;
  CvXSUB(dstr) = CvXSUB(sstr);
  CvXSUBANY(dstr) = CvXSUBANY(sstr);
  if (CvCONST(sstr)) {

@iabyn - Status changed from 'open' to 'resolved'

From ajsavige@yahoo.com.au

Further to John Myers' closure threading crash, I think I've
stumbled across the same problem, as further discussed here​:

<http​://www.perlmonks.org/?node_id=409473>

I'm pleased to report that Dave Mitchell's patch (Change 23433 by
davem on 2004/10/29 21​:04​:17) does indeed solve my crashing problem
(on both Linux and Windows). Notice that my crashes seem to occur
on multi-cpu machines _only_.

To shoehorn Dave's patch into Windows, I crudely hacked globals.c
and perlvars.h (in perl 5.8.5) as follows​:

--- globals-orig.c      Thu Apr 17 06:43:29 2003
+++ globals.c   Tue Nov 23 15:58:26 2004
@@ -16,6 +16,10 @@
 #define PERL_IN_GLOBALS_C
 #include "perl.h"

+#if defined(USE_ITHREADS) && defined(WIN32)
+__declspec(dllexport) perl_mutex PL_op_mutex;
+#endif
+
 int
 Perl_fprintf_nocontext(PerlIO *stream, const char *format, ...)
 {
--- perlvars-orig.h     Tue Aug 12 23:13:58 2003
+++ perlvars.h  Tue Nov 23 16:03:09 2004
@@ -44,7 +44,7 @@
 PERLVAR(Gmalloc_mutex, perl_mutex)     /* Mutex for malloc */
 #endif

-#if defined(USE_ITHREADS)
+#if defined(USE_ITHREADS) && !(defined(WIN32) && defined(PERL_IN_GLOBALS_C))
 PERLVAR(Gop_mutex,     perl_mutex)     /* Mutex for op refcounting */
 #endif

/-\

Find local movie times and trailers on Yahoo! Movies.
http​://au.movies.yahoo.com

From @steve-m-hay

Andrew Savige wrote​:

Further to John Myers' closure threading crash, I think I've
stumbled across the same problem, as further discussed here​:

<http​://www.perlmonks.org/?node_id=409473>

I'm pleased to report that Dave Mitchell's patch (Change 23433 by
davem on 2004/10/29 21​:04​:17) does indeed solve my crashing problem
(on both Linux and Windows). Notice that my crashes seem to occur
on multi-cpu machines _only_.

To shoehorn Dave's patch into Windows, I crudely hacked globals.c
and perlvars.h (in perl 5.8.5) as follows​:
[snip]
Note that this is not a clean fix.
I understand that Steve Hay is currently working on a clean fix
for this bug (in "Smoke [5.9.2] 23450" thread).

Dave's patch should have been fixed for Win32 by change 23499​:

http​://public.activestate.com/cgi-bin/perlbrowse?patch=23499

- Steve

Radan Computational Ltd.

The information contained in this message and any files transmitted with it are confidential and intended for the addressee(s) only. If you have received this message in error or there are any problems, please notify the sender immediately. The unauthorized use, disclosure, copying or alteration of this message is strictly forbidden. Note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of Radan Computational Ltd. The recipient(s) of this message should check it and any attached files for viruses​: Radan Computational will accept no liability for any damage caused by any virus transmitted by this email.