t9128 failing randomly with svn 1.9?

PATCH In review brian m. carlson 4 participants 8 messages Jan 29, 2018 – Jan 30, 2018

Share & tools

brian m. carlson

author

2018-01-29 01:51 UTC #0 lore raw

While running tests for my object_id part 11 series, I noticed a random segfault in git svn while running t9128. I've reproduced this on a different machine as well, using both Subversion 1.9.5 and 1.9.7 (Debian stable and unstable). It is reproducible on master.

When the test fails, it fails on the "git svn tag tag3" step, and I get the following:

Copying file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/trunk at r2 to file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/tags/tag3... Found possible branch point: file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/trunk => file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/tags/tag3, 2 Found branch parent: (refs/remotes/origin/tags/tag3) 0604824a81a121ad05aaf8caea65d8ca8f86c018
Following parent with do_switch
Successfully followed parent
r7 = f8467f2cee3bcead03e84cb51cf44f467a87457d (refs/remotes/origin/tags/tag3)
error: git-svn died of signal 11

Doing the following three times, I had two crashes.

(set -e; for i in $(seq 1 20); do (cd t && ./t9128-git-svn-cmd-branch.sh --verbose); done)

I'm not really familiar with git svn or its internals, and I didn't see anything recently on the list about this. Is this issue known?

Signature · 3 lines

brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

Todd Zullinger

2018-01-29 02:58 UTC #1 lore raw

Quoted 16 lines · brian m. carlson

While running tests for my object_id part 11 series, I noticed a random segfault in git svn while running t9128. I've reproduced this on a different machine as well, using both Subversion 1.9.5 and 1.9.7 (Debian stable and unstable). It is reproducible on master.
When the test fails, it fails on the "git svn tag tag3" step, and I get the following:
Copying file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/trunk at r2 to file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/tags/tag3... Found possible branch point: file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/trunk => file:///home/bmc/checkouts/git/t/trash%20directory.t9128-git-svn-cmd-branch/svnrepo/tags/tag3, 2 Found branch parent: (refs/remotes/origin/tags/tag3) 0604824a81a121ad05aaf8caea65d8ca8f86c018
Following parent with do_switch
Successfully followed parent
r7 = f8467f2cee3bcead03e84cb51cf44f467a87457d (refs/remotes/origin/tags/tag3)
error: git-svn died of signal 11

Quoted 6 lines

Doing the following three times, I had two crashes.
(set -e; for i in $(seq 1 20); do (cd t && ./t9128-git-svn-cmd-branch.sh --verbose); done)
I'm not really familiar with git svn or its internals, and I didn't see anything recently on the list about this. Is this issue known?

I see the same test fail randomly on Fedora (and I think on CentOS/RHEL, but I run the full tests more often on Fedora).

For me, it's tests 3 and 4 which fail with the same error. I thought it was a failure in subversion or the perl bindings rather than git, so I simply disabled them in the Fedora builds. The Debian packages skip 9128 as well (and 9167, which fails similarly).

I've seen the failures in t9141 from 'git svn branch' as well. I made a note to re-enable those tests after Jeff's work to make it easier to run with shell tracing enabled by default, but have not done so yet.

The 'git svn' tests are not run in Travis because the perl subversion bindings are not installed. I haven't made time to try installing them and running the tests in Travis to see if the failures occur there, but I suspect they would.

Signature · 5 lines

Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Going to trial with a lawyer who considers your whole life-style a
Crime in Progress is not a happy prospect.
    -- Hunter S. Thompson

Eric Wong

2018-01-29 12:06 UTC #2 lore raw

Quoted 10 lines · Todd Zullinger

Quoted 9 lines · brian m. carlson
r7 = f8467f2cee3bcead03e84cb51cf44f467a87457d (refs/remotes/origin/tags/tag3)
error: git-svn died of signal 11
Doing the following three times, I had two crashes.
(set -e; for i in $(seq 1 20); do (cd t && ./t9128-git-svn-cmd-branch.sh --verbose); done)
I'm not really familiar with git svn or its internals, and I didn't see anything recently on the list about this. Is this issue known?

The SVN bindings have been known to crash occasionally and I can't really get my head around XS/Swig or Perl internals :/ Looking at the coredump, it seems to be around exit and destruction (only Perl in the stacktrace, nothing from libsvn). So I suspect it's a lifetime or ordering bug, testing a patch below (slowly).

"branch" is also the only place we use SVN::Client->new these days.

Quoted 10 lines

For me, it's tests 3 and 4 which fail with the same error. I thought it was a failure in subversion or the perl bindings rather than git, so I simply disabled them in the Fedora builds. The Debian packages skip 9128 as well (and 9167, which fails similarly).
I've seen the failures in t9141 from 'git svn branch' as well. I made a note to re-enable those tests after Jeff's work to make it easier to run with shell tracing enabled by default, but have not done so yet.

Just a guess, but it might be related to destruction order. Running t9128 on a 32-bit Pentium-M, it took me 39 tries to fail.

git-svn.perl +5 -0

index 76a75d0b3d..2ba14269bb 100755
@@ -1200,6 +1200,11 @@ sub cmd_branch {
 	$ctx->copy($src, $rev, $dst)
 		unless $_dry_run;
 
+	# Release resources held by ctx before creating another SVN::Ra
+	# so destruction is orderly.  This seems necessary Subversion 1.9.5
+	# to avoid segfaults.
+	$ctx = undef;
+
 	$gs->fetch_all;
 }

I'll be looping t9128, t9141 and t9167 with that for a few hours or day. Will report back sooner if it fails. I'm on an ancient 32-bit system, I guess you guys encountered it on 64-bit machines?

Todd Zullinger

2018-01-29 18:43 UTC #3 lore raw

Quoted 21 lines · Eric Wong

Todd Zullinger <[email protected]> wrote:
Just a guess, but it might be related to destruction order. Running t9128 on a 32-bit Pentium-M, it took me 39 tries to fail.
git-svn.perl +5 -0
index 76a75d0b3d..2ba14269bb 100755
@@ -1200,6 +1200,11 @@ sub cmd_branch {
 	$ctx->copy($src, $rev, $dst)
 		unless $_dry_run;
 
+	# Release resources held by ctx before creating another SVN::Ra
+	# so destruction is orderly.  This seems necessary Subversion 1.9.5
+	# to avoid segfaults.
+	$ctx = undef;
+
 	$gs->fetch_all;
 }
 

Quoted 4 lines

I'll be looping t9128, t9141 and t9167 with that for a few hours or day. Will report back sooner if it fails. I'm on an ancient 32-bit system, I guess you guys encountered it on 64-bit machines?

Yeah. I saw it on numerous architectures, x86 and x86_64. I believe I saw it on aarch64 and ppc as well, but I don't have build logs at hand to confirm.

I'm running the tests with and without your patch as well. So far I've run t9128 300 times with the patch and no failures. Without it, it's failed 3 times in only a few dozen runs. That's promising.

Thanks for poking this Eric, and Brian for bringing it up. I had intended to look at it again and mention it, eventually. :)

Signature · 5 lines

Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I figure that if God actually does exist, He's big enough to
understand an honest difference of opinion.
    -- Isaac Asimov

Eric Wong

2018-01-29 23:16 UTC #4 lore raw

PATCH 1/1 git-svn: control destruction order to avoid segfault

Quoted 4 lines · Todd Zullinger

I'm running the tests with and without your patch as well. So far I've run t9128 300 times with the patch and no failures. Without it, it's failed 3 times in only a few dozen runs. That's promising.

Thanks for confirming it works on other systems.
Pull request and patch below:

The following changes since commit 5be1f00a9a701532232f57958efab4be8c959a29:

First batch after 2.16 (2018-01-23 13:21:10 -0800)

are available in the Git repository at:

git://bogomips.org/git-svn.git svn-branch-segfault

for you to fetch changes up to 2784b8d68faca823489949cbc69ead2f296cfc07:

git-svn: control destruction order to avoid segfault (2018-01-29 23:12:00 +0000)

----------------------------------------------------------------
Eric Wong (1):
git-svn: control destruction order to avoid segfault

 git-svn.perl | 5 +++++
 1 file changed, 5 insertions(+)

---------8<---------
Subject: [PATCH] git-svn: control destruction order to avoid segfault

It seems necessary to control destruction ordering to avoid a segfault with SVN 1.9.5 when using "git svn branch". I've also reported the problem against libsvn-perl to Debian [Bug #888791], but releasing the SVN::Client instance can be beneficial anyways to save memory.

ref: https://bugs.debian.org/888791

Tested-by Todd Zullinger <[email protected]>

Reported-by brian m. carlson <[email protected]>

Signed-off-by Eric Wong <[email protected]>

---
 git-svn.perl | 5 +++++
 1 file changed, 5 insertions(+)

git-svn.perl +5 -0

index 76a75d0b3d..a6b6c3e40c 100755
@@ -1200,6 +1200,11 @@ sub cmd_branch {
 	$ctx->copy($src, $rev, $dst)
 		unless $_dry_run;
 
+	# Release resources held by ctx before creating another SVN::Ra
+	# so destruction is orderly.  This seems necessary with SVN 1.9.5
+	# to avoid segfaults.
+	$ctx = undef;
+
 	$gs->fetch_all;
 }

Signature · 1 line

EW

Junio C Hamano

2018-01-30 21:15 UTC #5 lore raw

Re: [PATCH] git-svn: control destruction order to avoid segfault

Quoted 19 lines · Eric Wong

Quoted 4 lines · Todd Zullinger
I'm running the tests with and without your patch as well. So far I've run t9128 300 times with the patch and no failures. Without it, it's failed 3 times in only a few dozen runs. That's promising.
Thanks for confirming it works on other systems.
Pull request and patch below:
The following changes since commit 5be1f00a9a701532232f57958efab4be8c959a29:
First batch after 2.16 (2018-01-23 13:21:10 -0800)
are available in the Git repository at:
git://bogomips.org/git-svn.git svn-branch-segfault
for you to fetch changes up to 2784b8d68faca823489949cbc69ead2f296cfc07:

Quoted 8 lines

git-svn: control destruction order to avoid segfault (2018-01-29 23:12:00 +0000)
----------------------------------------------------------------
Eric Wong (1):
git-svn: control destruction order to avoid segfault
 git-svn.perl | 5 +++++
 1 file changed, 5 insertions(+)

Thanks. I'd actually apply this as a patch instead of pullilng, as I suspect you'd want it in 'maint' as well, though.

Quoted 32 lines

---------8<---------
Subject: [PATCH] git-svn: control destruction order to avoid segfault
It seems necessary to control destruction ordering to avoid a segfault with SVN 1.9.5 when using "git svn branch". I've also reported the problem against libsvn-perl to Debian [Bug #888791], but releasing the SVN::Client instance can be beneficial anyways to save memory.
ref: https://bugs.debian.org/888791
Tested-by Todd Zullinger <[email protected]>
Reported-by brian m. carlson <[email protected]>
Signed-off-by Eric Wong <[email protected]>
---
 git-svn.perl | 5 +++++
 1 file changed, 5 insertions(+)
git-svn.perl +5 -0
index 76a75d0b3d..a6b6c3e40c 100755
@@ -1200,6 +1200,11 @@ sub cmd_branch {
 	$ctx->copy($src, $rev, $dst)
 		unless $_dry_run;
 
+	# Release resources held by ctx before creating another SVN::Ra
+	# so destruction is orderly.  This seems necessary with SVN 1.9.5
+	# to avoid segfaults.
+	$ctx = undef;
+
 	$gs->fetch_all;
 }

brian m. carlson

author

2018-01-30 00:25 UTC #6 lore raw

In reply to Eric Wong

Quoted 6 lines · Eric Wong

Todd Zullinger <[email protected]> wrote:
git-svn.perl +0 -0
index 76a75d0b3d..2ba14269bb 100755

git-svn.perl sub cmd_branch { · line 1207

Quoted 8 lines

git-svn.perl +5 -0

@@ -1200,6 +1200,11 @@ sub cmd_branch {
 	$ctx->copy($src, $rev, $dst)
 		unless $_dry_run;
 
+	# Release resources held by ctx before creating another SVN::Ra
+	# so destruction is orderly.  This seems necessary Subversion 1.9.5
+	# to avoid segfaults.
+	$ctx = undef;
+

This may be the right thing to do. I've seen a decent number of cases in Perl where global destruction randomly causes segfaults.

Quoted 8 lines

git-svn.perl +0 -0
 	$gs->fetch_all;
 }
 
I'll be looping t9128, t9141 and t9167 with that for a few hours or day. Will report back sooner if it fails. I'm on an ancient 32-bit system, I guess you guys encountered it on 64-bit machines?

Yes, both systems are 64-bit Debian systems, one stable, and the other unstable.

Signature · 3 lines

brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

Todd Zullinger

2018-01-30 03:09 UTC #7 lore raw

In reply to Todd Zullinger

Quoted 4 lines · I wrote:

The 'git svn' tests are not run in Travis because the perl subversion bindings are not installed. I haven't made time to try installing them and running the tests in Travis to see if the failures occur there, but I suspect they would.

Before anyone spends time wondering about this, I was apparently looking at the MacOS build logs rather than the Linux logs. The git svn tests are indeed run in Travis, as long as you look at the right build logs. :/

Sadly (or amusingly), I think I looked at this before and made the same mistake, realized it, and then did it again today. Hopefully if I make the same mistake again I'll realize it before I post it to the list. ;)

Anyway, the Travis Linux container uses Ubuntu Trusty, which has subversion 1.8.8. I suppose that's why these random failures haven't turned up there.

Signature · 5 lines

Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I got stopped by a cop the other day.  He said, "Why'd you run that
stop sign?"  I said, "Because I don't believe everything I read."
    -- Stephen Wright