[PATCH] git-svn: write memoized data explicitly to avoid Storable bug

[PATCH] git-svn: write memoized data explicitly to avoid Storable bug

[Sergey Vlasov]

Apparently using the Storable module during global destruction is
unsafe - there is a bug which can cause segmentation faults:

  http://rt.cpan.org/Public/Bug/Display.html?id=36087
  http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=482355

The persistent memoization support introduced in commit 8bff7c538
relied on global destruction to write cached data, which was leading
to segfaults in some Perl configurations.  Calling Memoize::unmemoize
in the END block forces the cache writeout to be performed earlier,
thus avoiding the bug.

Signed-off-by: Sergey Vlasov <vsu@altlinux.ru>
---
 git-svn.perl |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

 In my case segfaults happen only when the perl-IO-Compress module is
 installed - apparently the attempt to load Compress::Zlib (now
 provided by IO::Compress) changes something.

 This unaswered report is also suspicious:

   http://thread.gmane.org/gmane.comp.version-control.git/142161

 (when running ./t9151-svn-mergeinfo.sh -v, the segfault should be
 visible: "error: git-svn died of signal 11").

diff --git a/git-svn.perl b/git-svn.perl
index 19d6848..c416358 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3169,6 +3169,22 @@ sub has_no_changes {
 			LIST_CACHE => 'FAULT',
 		;
 	}
+
+	sub unmemoize_svn_mergeinfo_functions {
+		return if not $memoized;
+		$memoized = 0;
+
+		Memoize::unmemoize 'lookup_svn_merge';
+		Memoize::unmemoize 'check_cherry_pick';
+		Memoize::unmemoize 'has_no_changes';
+	}
+}
+
+END {
+	# Force cache writeout explicitly instead of waiting for
+	# global destruction to avoid segfault in Storable:
+	# http://rt.cpan.org/Public/Bug/Display.html?id=36087
+	unmemoize_svn_mergeinfo_functions();
 }
 
 sub parents_exclude {
-- 
1.6.0.2.321.g8406

Re: [PATCH] git-svn: write memoized data explicitly to avoid Storable bug

[Eric Wong]

Sergey Vlasov <vsu@altlinux.ru> wrote:
> Apparently using the Storable module during global destruction is
> unsafe - there is a bug which can cause segmentation faults:
> 
>   http://rt.cpan.org/Public/Bug/Display.html?id=36087
>   http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=482355
> 
> The persistent memoization support introduced in commit 8bff7c538
> relied on global destruction to write cached data, which was leading
> to segfaults in some Perl configurations.  Calling Memoize::unmemoize
> in the END block forces the cache writeout to be performed earlier,
> thus avoiding the bug.
> 
> Signed-off-by: Sergey Vlasov <vsu@altlinux.ru>

Thanks Sergey,

Acked-by: Eric Wong <normalperson@yhbt.net>

...and pushed to git://git.bogomips.org/git-svn

-- 
Eric Wong

Re: [PATCH] git-svn: write memoized data explicitly to avoid Storable bug

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

> Thanks Sergey,
>
> Acked-by: Eric Wong <normalperson@yhbt.net>
>
> ...and pushed to git://git.bogomips.org/git-svn

Thanks all; pulled.

Re: [PATCH] git-svn: write memoized data explicitly to avoid Storable bug

[Ævar Arnfjörð Bjarmason]

On Sun, Jul 18, 2010 at 12:17, Sergey Vlasov <vsu@altlinux.ru> wrote:
> Apparently using the Storable module during global destruction is
> unsafe - there is a bug which can cause segmentation faults:
>
>  http://rt.cpan.org/Public/Bug/Display.html?id=36087
>  http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=482355

I did some investigation into the upstream issue:
https://rt.cpan.org/Ticket/Display.html?id=36087#txn-806832

> The persistent memoization support introduced in commit 8bff7c538
> relied on global destruction to write cached data, which was leading
> to segfaults in some Perl configurations.  Calling Memoize::unmemoize
> in the END block forces the cache writeout to be performed earlier,
> thus avoiding the bug.

Maybe I'm missing something obvious, but this seems like the wrong
solution. The core issue is that we don't want to clean up during
global destruction, but then we should just do:

       sub DESTROY {
               return if not $memoized;
               $memoized = 0;

               Memoize::unmemoize 'lookup_svn_merge';
               Memoize::unmemoize 'check_cherry_pick';
               Memoize::unmemoize 'has_no_changes';
       }

That should work since memoize_svn_mergeinfo_functions(); is being
called in find_extra_svn_parents, which is a Git::SVN object
method. Can you try this and confirm/deny? I can't because I can't get
the original to segfault on my box when run within git-svn.

Re: [PATCH] git-svn: write memoized data explicitly to avoid Storable bug

[Eric Wong]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Sun, Jul 18, 2010 at 12:17, Sergey Vlasov <vsu@altlinux.ru> wrote:
> > Apparently using the Storable module during global destruction is
> > unsafe - there is a bug which can cause segmentation faults:
> >
> >  http://rt.cpan.org/Public/Bug/Display.html?id=36087
> >  http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=482355
> 
> I did some investigation into the upstream issue:
> https://rt.cpan.org/Ticket/Display.html?id=36087#txn-806832
> 
> > The persistent memoization support introduced in commit 8bff7c538
> > relied on global destruction to write cached data, which was leading
> > to segfaults in some Perl configurations.  Calling Memoize::unmemoize
> > in the END block forces the cache writeout to be performed earlier,
> > thus avoiding the bug.
> 
> Maybe I'm missing something obvious, but this seems like the wrong
> solution. The core issue is that we don't want to clean up during
> global destruction, but then we should just do:
> 
>        sub DESTROY {
>                return if not $memoized;
>                $memoized = 0;
> 
>                Memoize::unmemoize 'lookup_svn_merge';
>                Memoize::unmemoize 'check_cherry_pick';
>                Memoize::unmemoize 'has_no_changes';
>        }

I haven't looked at this issue in-depth, but I believe the problem is
triggered due to Memoize::Storable trying to use Storable::nstore
in its own DESTROY function.  So trying to do the same in our own
DESTROY would be just as bad.

> That should work since memoize_svn_mergeinfo_functions(); is being
> called in find_extra_svn_parents, which is a Git::SVN object
> method. Can you try this and confirm/deny? I can't because I can't get
> the original to segfault on my box when run within git-svn.

I wasn't able to reproduce the segfault on my systems, either, but
it seems plausible it would only happen on some systems.

-- 
Eric Wong