[patch] make Storable thread-safe

As Jan Dubois has suggested a few months ago Storable 2.09 is not thread-safe, 
because it doesn't handle PL_modglobal's Storable context entry cloning on 
perl_clone. One of the windows users with mod_perl 2.0 has already been beaten 
by this bug.

I've spent a lot of time trying to reproduce the problem on linux, but the 
problem is this: All Storable user API functions are atomic context wise, i.e. 
you can't run two Storable functions one after another so that they will 
interfere with each other. So from the tester point of view I couldn't figure 
out how to show that the problem is there. The problem will happen on unix 
every time two threads will try to change the same context (since the actual 
implementation is not thread-atomic, i.e. not serialized), not talking about 
race conditions. It failed to come up with a reliable test that will fail 
deterministically. If you have an idea how to come up with such an 'exploit' 
I'm all ears.

Luckily on win32 we have the issue with private memory pool per thread and it 
fails on win32 with a trivial test. "free to wrong pool" is the failure message.

The following patch [inlined and attached] against Storable 2.09 makes it 
thread-safe by allocating a new context for each new thread and adds a simple 
test which will fail on Storable 2.09 on win32. Actually I haven't tested it, 
since I don't have win32, so please verify that.

I'm not sure whether it's better to expose init_perinterp() to perl and have 
CLONE on the perl side like I did, or just have CLONE defined in Storable.xs 
like so:

void
CLONE()
   CODE:
   init_perinterp();

it's up to you to decide what's the best way.


diff --new-file -ur Storable-2.09/ChangeLog Storable-2.10/ChangeLog
--- Storable-2.09/ChangeLog	2004-01-05 07:43:38.000000000 -0800
+++ Storable-2.10/ChangeLog	2004-01-19 00:04:02.000000000 -0800
@@ -1,3 +1,13 @@
+
+    Version 2.10
+
+	Add Storable::CLONE which calls Storable::init_perinterp() to
+	create a new context for each new perl ithread when it starts, to
+	make Storable thread-safe. add a test, which unfortunately fail
+	only on win32, as I wasn't able to come up with one failing on
+	unix even though the problem was there.
+	[Stas Bekman <stas@stason.org>, Jan Dubois <jand@ActiveState.com>]
+
  Sat Jan  3 18:49:18 GMT 2004   Nicholas Clark <nick@ccl4.org>

      Version 2.09
diff --new-file -ur Storable-2.09/MANIFEST Storable-2.10/MANIFEST
--- Storable-2.09/MANIFEST	2004-01-05 07:43:38.000000000 -0800
+++ Storable-2.10/MANIFEST	2004-01-18 23:48:52.000000000 -0800
@@ -29,6 +29,7 @@
  t/tied.t		    See if Storable works
  t/tied_hook.t		    See if Storable works
  t/tied_items.t		    See if Storable works
+t/threads.t                 See if Storable works under ithreads
  t/utf8.t		    See if Storable works
  t/utf8hash.t		    See if Storable works
  t/Test/Builder.pm	    For testing the CPAN release on pre 5.6.2
diff --new-file -ur Storable-2.09/Storable.pm Storable-2.10/Storable.pm
--- Storable-2.09/Storable.pm	2004-01-05 07:43:43.000000000 -0800
+++ Storable-2.10/Storable.pm	2004-01-15 17:39:00.000000000 -0800
@@ -21,7 +21,7 @@
  use AutoLoader;
  use vars qw($canonical $forgive_me $VERSION);

-$VERSION = '2.09';
+$VERSION = '2.10';
  *AUTOLOAD = \&AutoLoader::AUTOLOAD;		# Grrr...

  #
@@ -47,6 +47,11 @@
  	}
  }

+sub CLONE {
+    # clone context under threads
+    Storable::init_perinterp();
+}
+
  # Can't Autoload cleanly as this clashes 8.3 with &retrieve
  sub retrieve_fd { &fd_retrieve }		# Backward compatibility

diff --new-file -ur Storable-2.09/Storable.xs Storable-2.10/Storable.xs
--- Storable-2.09/Storable.xs	2003-09-21 15:32:49.000000000 -0700
+++ Storable-2.10/Storable.xs	2004-01-15 17:29:22.000000000 -0800
@@ -5901,6 +5901,9 @@
      gv_fetchpv("Storable::interwork_56_64bit",   GV_ADDMULTI, SVt_PV);
  #endif

+void
+init_perinterp()
+
  int
  pstore(f,obj)
  OutputStream	f
diff --new-file -ur Storable-2.09/t/threads.t Storable-2.10/t/threads.t
--- Storable-2.09/t/threads.t	1969-12-31 16:00:00.000000000 -0800
+++ Storable-2.10/t/threads.t	2004-01-18 23:59:23.000000000 -0800
@@ -0,0 +1,55 @@
+
+# as of 2.09 on win32 Storable w/threads dies with "free to wrong
+# pool" since it uses the same context for different threads. since
+# win32 perl implementation allocates a different memory pool for each
+# thread using the a memory pool from one thread to allocate memory
+# for another thread makes win32 perl very unhappy
+#
+# but the problem exists everywhere, not only on win32 perl , it's
+# just hard to catch it deterministically - since the same context is
+# used if two or more threads happen to change the state of the
+# context in the middle of the operation, and those operations aren't
+# atomic per thread, bad things including data loss and corrupted data
+# can happen.
+#
+# this has been solved in 2.10 by adding a Storable::CLONE which calls
+# Storable::init_perinterp() to create a new context for each new
+# thread when it starts
+
+sub BEGIN {
+    if ($ENV{PERL_CORE}){
+	chdir('t') if -d 't';
+	@INC = ('.', '../lib');
+    } else {
+	unshift @INC, 't';
+    }
+    require Config; import Config;
+    if ($ENV{PERL_CORE} and $Config{'extensions'} !~ /\bStorable\b/) {
+        print "1..0 # Skip: Storable was not built\n";
+        exit 0;
+    }
+    unless ($Config{'useithreads'} and eval { require threads; 1 }) {
+        print "1..0 # Skip: no threads\n";
+        exit 0;
+    }
+}
+
+use Test::More;
+
+use strict;
+
+use threads;
+use Storable qw(nfreeze);
+
+plan tests => 2;
+
+threads->new(\&sub1);
+
+$_->join() for threads->list();
+
+ok 1;
+
+sub sub1 {
+    nfreeze {};
+    ok 1;
+}


__________________________________________________________________
Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/     mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com

• [patch] make Storable thread-safe by Stas Bekman
• Re: [patch] make Storable thread-safe by Steve Hay