Re: [PATCH] git-svn: workaround for a bug in svn serf backend

[Roman Kagan]

2013/12/30 Thomas Rast :
> Roman Kagan  writes:
>
>> + # workaround for a bug in svn serf backend (v1.8.5 and below):
>> + # store 3d argument to ->add_file() in a local variable, to make it
>> + # have the same lifetime as $fbat
>> + my $upa = $self->url_path($m->{file_a});
>>   my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat,
>> - $self->url_path($m->{file_a}), $self->{r});
>> + $upa, $self->{r});
>
> Hmm, now that you put it that way, I wonder if the patch is correct.
>
> Let me first rephrase the problem to verify that I understand the issue:
>
>   $fbat keeps a pointer to the $upa string, without maintaining a
>   reference to it.  When $fbat is destroyed, it needs this string, so we
>   must ensure that the lifetime of $upa is at least as long as that of
>   $fbat.

No.  The string is needed in subversion's close_file(), so we want to
keep it alive until close_file() returns. Surviving till the end of
the current function scope is sufficient for that.

> However, does Perl make any guarantees as to the order in which local
> variables are unreferenced and then destroyed?

We don't care about the order they are destroyed WRT each other.

Roman.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-svn: workaround for a bug in svn serf backend

[Thomas Rast]

Roman Kagan  writes:

> + # workaround for a bug in svn serf backend (v1.8.5 and below):
> + # store 3d argument to ->add_file() in a local variable, to make it
> + # have the same lifetime as $fbat
> + my $upa = $self->url_path($m->{file_a});
>   my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat,
> - $self->url_path($m->{file_a}), $self->{r});
> + $upa, $self->{r});

Hmm, now that you put it that way, I wonder if the patch is correct.

Let me first rephrase the problem to verify that I understand the issue:

  $fbat keeps a pointer to the $upa string, without maintaining a
  reference to it.  When $fbat is destroyed, it needs this string, so we
  must ensure that the lifetime of $upa is at least as long as that of
  $fbat.

However, does Perl make any guarantees as to the order in which local
variables are unreferenced and then destroyed?  I can't find any such
guarantee.

In the absence of such, wouldn't we have to keep $upa in an outer,
separate scope to ensure that $fbat is destroyed first?

-- 
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-svn: workaround for a bug in svn serf backend

[Roman Kagan]

2013/12/27 Roman Kagan :
> 2013/12/27 Jonathan Nieder :
>> Could this be reproduced with a test script to make sure we don't
>> reintroduce the bug again later?  (It's okay if the test only fails on
>> machines with the problematic svn version.)
>
> That would need a fairly fancy setup phase, as the bug triggers only
> on http(s)-accessed svn repositories.  I'll take a look if there's
> something already available in the existing test scripts

Turns out the stuff is all there, and the tests doing file renames and
dcomit-ting them do exist (t9115-git-svn-dcommit-funky-renames.sh for
one).

However, the httpd setup is seriously broken; I haven't managed to get
it to run on my Fedora 20 with apache 2.4.6.  Apparently git-svn tests
(almost) never get executed against an http-based repository; even
those who don't set NO_SVN_TESTS get them run against file-based
repository and thus don't trigger the error.

Someone with better apache-foo needs to take a look into that.  Once
that is sorted out I believe the tests will start triggering the bug.

Meanwhile I assume that the patch doesn't need to include an extra testcase.

Roman.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-svn: workaround for a bug in svn serf backend

[Roman Kagan]

2013/12/27 Jonathan Nieder :
> Roman Kagan wrote:
>
>> Subversion serf backend in versions 1.8.5 and below has a bug that the
>> function creating the descriptor of a file change -- add_file() --
>> doesn't make a copy of its 3d argument when storing it on the returned
>
> 3d makes me think of 3-dimensional. ;-)  I think you mean third
> (or the abbreviation 3rd).

Indeed.

>> descriptor.  As a result, by the time this field is used (in
>> transactions of file copying or renaming) it may well be released.
>
> Please describe the symptom so this patch is easy to find when other
> people run into it.

OK

> Do I remember correctly that "... released and scribbled over with a
> new value, causing such-and-such assertion to fire" was what happened?

Exactly.

>> This patch works around this bug, by storing the value to be passed as
>> the 3d argument to add_file() in a local variable with the same scope as
>> the file change descriptor, making sure their lifetime is the same.
>
> Could this be reproduced with a test script to make sure we don't
> reintroduce the bug again later?  (It's okay if the test only fails on
> machines with the problematic svn version.)

That would need a fairly fancy setup phase, as the bug triggers only
on http(s)-accessed svn repositories.  I'll take a look if there's
something already available in the existing test scripts; writing one
from scratch for this specific case is IMO beyond the reasonable
effort.

> Modulo the confusing 3-dimensional arguments in comments, the code
> change looks good.

Thanks, I'll adjust the wording and resubmit.

Roman.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-svn: workaround for a bug in svn serf backend

[Jonathan Nieder]

Roman Kagan wrote:

> Subversion serf backend in versions 1.8.5 and below has a bug that the
> function creating the descriptor of a file change -- add_file() --
> doesn't make a copy of its 3d argument when storing it on the returned

3d makes me think of 3-dimensional. ;-)  I think you mean third
(or the abbreviation 3rd).

> descriptor.  As a result, by the time this field is used (in
> transactions of file copying or renaming) it may well be released.

Please describe the symptom so this patch is easy to find when other
people run into it.

Do I remember correctly that "... released and scribbled over with a
new value, causing such-and-such assertion to fire" was what happened?

> This patch works around this bug, by storing the value to be passed as
> the 3d argument to add_file() in a local variable with the same scope as
> the file change descriptor, making sure their lifetime is the same.

Could this be reproduced with a test script to make sure we don't
reintroduce the bug again later?  (It's okay if the test only fails on
machines with the problematic svn version.)

Modulo the confusing 3-dimensional arguments in comments, the code
change looks good.

Thanks and hope that helps,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git-svn: workaround for a bug in svn serf backend

[Roman Kagan]

Subversion serf backend in versions 1.8.5 and below has a bug that the
function creating the descriptor of a file change -- add_file() --
doesn't make a copy of its 3d argument when storing it on the returned
descriptor.  As a result, by the time this field is used (in
transactions of file copying or renaming) it may well be released.

This patch works around this bug, by storing the value to be passed as
the 3d argument to add_file() in a local variable with the same scope as
the file change descriptor, making sure their lifetime is the same.

Cc: Benjamin Pabst 
Cc: Eric Wong 
Signed-off-by: Roman Kagan 
---
 perl/Git/SVN/Editor.pm | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm
index b3bcd47..ae399c3 100644
--- a/perl/Git/SVN/Editor.pm
+++ b/perl/Git/SVN/Editor.pm
@@ -304,8 +304,12 @@ sub C {
my ($self, $m, $deletions) = @_;
my ($dir, $file) = split_path($m->{file_b});
my $pbat = $self->ensure_path($dir, $deletions);
+   # workaround for a bug in svn serf backend (v1.8.5 and below):
+   # store 3d argument to ->add_file() in a local variable, to make it
+   # have the same lifetime as $fbat
+   my $upa = $self->url_path($m->{file_a});
my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat,
-   $self->url_path($m->{file_a}), $self->{r});
+   $upa, $self->{r});
print "\tC\t$m->{file_a} => $m->{file_b}\n" unless $::_q;
$self->chg_file($fbat, $m);
$self->close_file($fbat,undef,$self->{pool});
@@ -323,8 +327,10 @@ sub R {
my ($self, $m, $deletions) = @_;
my ($dir, $file) = split_path($m->{file_b});
my $pbat = $self->ensure_path($dir, $deletions);
+   # workaround for a bug in svn serf backend, see comment in C() above
+   my $upa = $self->url_path($m->{file_a});
my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat,
-   $self->url_path($m->{file_a}), $self->{r});
+   $upa, $self->{r});
print "\tR\t$m->{file_a} => $m->{file_b}\n" unless $::_q;
$self->apply_autoprops($file, $fbat);
$self->chg_file($fbat, $m);
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html