threads / patch / 24429

patchgit-svn: write memoized data explicitly to avoid Storable bug

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

## tl;dr

5 messages between Jul 18, 2010 and Jul 20, 2010. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Sergey Vlasov· Jul 18, 2010, 12:17 UTC · lore

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").

Show changes to git-svn.perl +16 −0

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

Eric Wong· Jul 19, 2010, 06:39 UTC · re: Sergey Vlasov · lore

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

Sergey Vlasov <vsu@altlinux.ru> wrote:

Show 13 quoted lines

> 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

Junio C Hamano· Jul 19, 2010, 16:37 UTC · re: Eric Wong · lore

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

Eric Wong <normalperson@yhbt.net> writes:

Show 5 quoted lines

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

Thanks all; pulled.

Ævar Arnfjörð Bjarmason· Jul 20, 2010, 17:32 UTC · re: Sergey Vlasov · lore

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

On Sun, Jul 18, 2010 at 12:17, Sergey Vlasov <vsu@altlinux.ru> wrote:

Show 5 quoted lines

> 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

Show 5 quoted lines

> 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.

Eric Wong· Jul 20, 2010, 19:02 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 28 quoted lines

> 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