Re: [PATCH v4] reflog-walk: don't segfault on non-commit sha1's in the reflog
On Wed, Jan 6, 2016 at 4:13 PM, Dennis Kaarsemaker
wrote:
> On di, 2016-01-05 at 20:52 -0500, Eric Sunshine wrote:
>> On Tue, Jan 5, 2016 at 8:28 PM, Eric Sunshine <
>> sunsh...@sunshineco.com> wrote:
>> > On Tue, Jan 5, 2016 at 8:20 PM, Dennis Kaarsemaker
>> > wrote:
>> > > On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:
>> > > > Hmm, this test is successful for me on OS X even without the
>> > > > reflog-walk.c changes applied.
>> > > >
>> > > > And this test actually fails (inversely) because it's expecting
>> > > > a
>> > > > failure, but doesn't get one since the command produces the
>> > > > expected
>> > > > output.
>> > >
>> > > That's... surprising to say the least. What's the content of
>> > > 'actual',
>> > > and which git.git commit are you on?
>> >
>> > % cat t/trash\ directory.t1410-reflog/actual
>> > b60a214 refs/tests/tree-in-reflog@{0}: Restoring to commit
>> > 140c527 refs/tests/tree-in-reflog@{1}: Forcing tree
>> > b60a214 refs/tests/tree-in-reflog@{2}: Creating ref
>> > %
>> >
>> > This is with only the t/t1410-reflog.sh changes from your patch
>> > applied atop current 'master' (SHA1 7548842).
>>
>> By the way, the segfault does occur for me on Linux and FreeBSD.
>>
>> And, in all cases, on all tested platforms, with the full patch
>> applied, both tests behave sanely (in the expected fashion). So, even
>> though the crash doesn't manifest everywhere, the fact that the tests
>> are meaningfully testing it on the "affected" platforms may mean that
>> it's not worth worrying about why it doesn't segfault on OS X.
>>
>> (Of course, practicality aside, one might want to satisfy one's
>> intellectual curiosity about why it behaves differently on OS X.)
>
> The only explanation I can think of (and that's with practically no
> knowledge of OS X internals) is that OS X's memory allocation strategy
> is unlucky. Git is definitely writing to a location it should not write
> to. On linux and freebsd this is unallocated memory, so you get a
> segfault. On OS X, it happens to be memory actually allocated by git,
> resulting not in a segfault but in silent corruption of other in-memory
> data. I would argue that this is a much worse result, even though in
> this small test that corruption seems to not trigger a crash.
Agreed. For a guaranteed crash, put assert(c->object.type ==
OBJ_COMMIT); in the macro function "slabname## _at_peek" in
commit-slab.h. That is if I analyzed the crash correctly. I'm not
suggesting to put the assert() in the code permanently though because
I don't know how often this function is called.
--
Duy
--
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 v4] reflog-walk: don't segfault on non-commit sha1's in the reflog
On di, 2016-01-05 at 20:52 -0500, Eric Sunshine wrote:
> On Tue, Jan 5, 2016 at 8:28 PM, Eric Sunshine <
> sunsh...@sunshineco.com> wrote:
> > On Tue, Jan 5, 2016 at 8:20 PM, Dennis Kaarsemaker
> > wrote:
> > > On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:
> > > > Hmm, this test is successful for me on OS X even without the
> > > > reflog-walk.c changes applied.
> > > >
> > > > And this test actually fails (inversely) because it's expecting
> > > > a
> > > > failure, but doesn't get one since the command produces the
> > > > expected
> > > > output.
> > >
> > > That's... surprising to say the least. What's the content of
> > > 'actual',
> > > and which git.git commit are you on?
> >
> > % cat t/trash\ directory.t1410-reflog/actual
> > b60a214 refs/tests/tree-in-reflog@{0}: Restoring to commit
> > 140c527 refs/tests/tree-in-reflog@{1}: Forcing tree
> > b60a214 refs/tests/tree-in-reflog@{2}: Creating ref
> > %
> >
> > This is with only the t/t1410-reflog.sh changes from your patch
> > applied atop current 'master' (SHA1 7548842).
>
> By the way, the segfault does occur for me on Linux and FreeBSD.
>
> And, in all cases, on all tested platforms, with the full patch
> applied, both tests behave sanely (in the expected fashion). So, even
> though the crash doesn't manifest everywhere, the fact that the tests
> are meaningfully testing it on the "affected" platforms may mean that
> it's not worth worrying about why it doesn't segfault on OS X.
>
> (Of course, practicality aside, one might want to satisfy one's
> intellectual curiosity about why it behaves differently on OS X.)
The only explanation I can think of (and that's with practically no
knowledge of OS X internals) is that OS X's memory allocation strategy
is unlucky. Git is definitely writing to a location it should not write
to. On linux and freebsd this is unallocated memory, so you get a
segfault. On OS X, it happens to be memory actually allocated by git,
resulting not in a segfault but in silent corruption of other in-memory
data. I would argue that this is a much worse result, even though in
this small test that corruption seems to not trigger a crash.
--
Dennis Kaarsemaker
http://www.kaarsemaker.net
--
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 v4] reflog-walk: don't segfault on non-commit sha1's in the reflog
On Tue, Jan 5, 2016 at 8:28 PM, Eric Sunshine wrote:
> On Tue, Jan 5, 2016 at 8:20 PM, Dennis Kaarsemaker
> wrote:
>> On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:
>>> Hmm, this test is successful for me on OS X even without the
>>> reflog-walk.c changes applied.
>>>
>>> And this test actually fails (inversely) because it's expecting a
>>> failure, but doesn't get one since the command produces the expected
>>> output.
>>
>> That's... surprising to say the least. What's the content of 'actual',
>> and which git.git commit are you on?
>
> % cat t/trash\ directory.t1410-reflog/actual
> b60a214 refs/tests/tree-in-reflog@{0}: Restoring to commit
> 140c527 refs/tests/tree-in-reflog@{1}: Forcing tree
> b60a214 refs/tests/tree-in-reflog@{2}: Creating ref
> %
>
> This is with only the t/t1410-reflog.sh changes from your patch
> applied atop current 'master' (SHA1 7548842).
By the way, the segfault does occur for me on Linux and FreeBSD.
And, in all cases, on all tested platforms, with the full patch
applied, both tests behave sanely (in the expected fashion). So, even
though the crash doesn't manifest everywhere, the fact that the tests
are meaningfully testing it on the "affected" platforms may mean that
it's not worth worrying about why it doesn't segfault on OS X.
(Of course, practicality aside, one might want to satisfy one's
intellectual curiosity about why it behaves differently on OS X.)
--
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 v4] reflog-walk: don't segfault on non-commit sha1's in the reflog
On Tue, Jan 5, 2016 at 8:20 PM, Dennis Kaarsemaker
wrote:
> On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:
>> On Tue, Jan 5, 2016 at 4:12 PM, Dennis Kaarsemaker
>> > + git update-ref --create-reflog -m "Creating ref" \
>> > + refs/tests/tree-in-reflog HEAD &&
>> > + git update-ref -m "Forcing tree" refs/tests/tree-in-reflog
>> > HEAD^{tree} &&
>> > + git update-ref -m "Restoring to commit" refs/tests/tree-in
>> > -reflog HEAD &&
>> > + git reflog refs/tests/tree-in-reflog
>> > +'
>>
>> Hmm, this test is successful for me on OS X even without the
>> reflog-walk.c changes applied.
>>
>> > +test_expect_failure 'reflog with non-commit entries displays all
>> > entries' '
>> > + git reflog refs/tests/tree-in-reflog >actual &&
>> > + test_line_count = 3 actual
>> > +'
>>
>> And this test actually fails (inversely) because it's expecting a
>> failure, but doesn't get one since the command produces the expected
>> output.
>
> That's... surprising to say the least. What's the content of 'actual',
> and which git.git commit are you on?
% cat t/trash\ directory.t1410-reflog/actual
b60a214 refs/tests/tree-in-reflog@{0}: Restoring to commit
140c527 refs/tests/tree-in-reflog@{1}: Forcing tree
b60a214 refs/tests/tree-in-reflog@{2}: Creating ref
%
This is with only the t/t1410-reflog.sh changes from your patch
applied atop current 'master' (SHA1 7548842).
--
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 v4] reflog-walk: don't segfault on non-commit sha1's in the reflog
On di, 2016-01-05 at 20:05 -0500, Eric Sunshine wrote:
> On Tue, Jan 5, 2016 at 4:12 PM, Dennis Kaarsemaker
> wrote:
> > git reflog (ab)uses the log machinery to display its list of log
> > entries. To do so it must fake commit parent information for the
> > log
> > walker.
> >
> > For refs in refs/heads this is no problem, as they should only ever
> > point to commits. Tags and other refs however can point to
> > anything,
> > thus their reflog may contain non-commit objects.
> >
> > To avoid segfaulting, we check whether reflog entries are commits
> > before
> > feeding them to the log walker and skip any non-commits. This means
> > that
> > git reflog output will be incomplete for such refs, but that's one
> > step
> > up from segfaulting. A more complete solution would be to decouple
> > git
> > reflog from the log walker machinery.
> >
> > Signed-off-by: Dennis Kaarsemaker
> > ---
> > diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> > @@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs
> > at BUFSIZ boundaries' '
> > +test_expect_success 'no segfaults for reflog containing non-commit
> > sha1s' '
>
> Nit: It's kind of strange for a test title to talk about not
> segfaulting; that's behavior you'd expect to be true for all tests.
> Perhaps describe it as "non-commit reflog entries handled sanely" or
> something.
To paraphrase what Junio said earlier in this thread: tests determine
what is sane behavior, so using the word 'sanely' isn't really
appropriate. This is a regression test to make sure we don't
accidentally reintroduce behavior that segfaults, which I think is an
easy mistake to make with the current code, so I think the title is
appropriate.
> > + git update-ref --create-reflog -m "Creating ref" \
> > + refs/tests/tree-in-reflog HEAD &&
> > + git update-ref -m "Forcing tree" refs/tests/tree-in-reflog
> > HEAD^{tree} &&
> > + git update-ref -m "Restoring to commit" refs/tests/tree-in
> > -reflog HEAD &&
> > + git reflog refs/tests/tree-in-reflog
> > +'
>
> Hmm, this test is successful for me on OS X even without the
> reflog-walk.c changes applied.
>
> > +test_expect_failure 'reflog with non-commit entries displays all
> > entries' '
> > + git reflog refs/tests/tree-in-reflog >actual &&
> > + test_line_count = 3 actual
> > +'
>
> And this test actually fails (inversely) because it's expecting a
> failure, but doesn't get one since the command produces the expected
> output.
That's... surprising to say the least. What's the content of 'actual',
and which git.git commit are you on?
> By the way, it may make sense to combine these two tests. If a
> segfault occurs, the actual output likely will not match the expected
> output, thus the test will fail anyhow (unless the segfault occurs
> after all output).
I kept them separate to show that while this no longer segfaults, it's
still not the correct output, but showing correct output is a much
bigger project.
--
Dennis Kaarsemaker
www.kaarsemaker.net
--
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 v4] reflog-walk: don't segfault on non-commit sha1's in the reflog
On Tue, Jan 5, 2016 at 4:12 PM, Dennis Kaarsemaker
wrote:
> git reflog (ab)uses the log machinery to display its list of log
> entries. To do so it must fake commit parent information for the log
> walker.
>
> For refs in refs/heads this is no problem, as they should only ever
> point to commits. Tags and other refs however can point to anything,
> thus their reflog may contain non-commit objects.
>
> To avoid segfaulting, we check whether reflog entries are commits before
> feeding them to the log walker and skip any non-commits. This means that
> git reflog output will be incomplete for such refs, but that's one step
> up from segfaulting. A more complete solution would be to decouple git
> reflog from the log walker machinery.
>
> Signed-off-by: Dennis Kaarsemaker
> ---
> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> @@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ
> boundaries' '
> +test_expect_success 'no segfaults for reflog containing non-commit sha1s' '
Nit: It's kind of strange for a test title to talk about not
segfaulting; that's behavior you'd expect to be true for all tests.
Perhaps describe it as "non-commit reflog entries handled sanely" or
something.
> + git update-ref --create-reflog -m "Creating ref" \
> + refs/tests/tree-in-reflog HEAD &&
> + git update-ref -m "Forcing tree" refs/tests/tree-in-reflog
> HEAD^{tree} &&
> + git update-ref -m "Restoring to commit" refs/tests/tree-in-reflog
> HEAD &&
> + git reflog refs/tests/tree-in-reflog
> +'
Hmm, this test is successful for me on OS X even without the
reflog-walk.c changes applied.
> +test_expect_failure 'reflog with non-commit entries displays all entries' '
> + git reflog refs/tests/tree-in-reflog >actual &&
> + test_line_count = 3 actual
> +'
And this test actually fails (inversely) because it's expecting a
failure, but doesn't get one since the command produces the expected
output.
By the way, it may make sense to combine these two tests. If a
segfault occurs, the actual output likely will not match the expected
output, thus the test will fail anyhow (unless the segfault occurs
after all output).
> +
> test_done
> --
> 2.7.0-rc3-219-g24972d4
--
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 v4] reflog-walk: don't segfault on non-commit sha1's in the reflog
git reflog (ab)uses the log machinery to display its list of log
entries. To do so it must fake commit parent information for the log
walker.
For refs in refs/heads this is no problem, as they should only ever
point to commits. Tags and other refs however can point to anything,
thus their reflog may contain non-commit objects.
To avoid segfaulting, we check whether reflog entries are commits before
feeding them to the log walker and skip any non-commits. This means that
git reflog output will be incomplete for such refs, but that's one step
up from segfaulting. A more complete solution would be to decouple git
reflog from the log walker machinery.
Signed-off-by: Dennis Kaarsemaker
Helped-by: Nguyễn Thái Ngọc Duy
Helped-by: Junio C Hamano
---
Changes since v3: tweaks to the second test.
reflog-walk.c | 16 +++-
t/t1410-reflog.sh | 13 +
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/reflog-walk.c b/reflog-walk.c
index 85b8a54..0ebd1da 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -221,6 +221,7 @@ void fake_reflog_parent(struct reflog_walk_info *info,
struct commit *commit)
struct commit_info *commit_info =
get_commit_info(commit, &info->reflogs, 0);
struct commit_reflog *commit_reflog;
+ struct object *logobj;
struct reflog_info *reflog;
info->last_commit_reflog = NULL;
@@ -232,15 +233,20 @@ void fake_reflog_parent(struct reflog_walk_info *info,
struct commit *commit)
commit->parents = NULL;
return;
}
-
- reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
info->last_commit_reflog = commit_reflog;
- commit_reflog->recno--;
- commit_info->commit = (struct commit *)parse_object(reflog->osha1);
- if (!commit_info->commit) {
+
+ do {
+ reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
+ commit_reflog->recno--;
+ logobj = parse_object(reflog->osha1);
+ } while (commit_reflog->recno && (logobj && logobj->type !=
OBJ_COMMIT));
+
+ if (!logobj || logobj->type != OBJ_COMMIT) {
+ commit_info->commit = NULL;
commit->parents = NULL;
return;
}
+ commit_info->commit = (struct commit *)logobj;
commit->parents = xcalloc(1, sizeof(struct commit_list));
commit->parents->item = commit_info->commit;
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index b79049f..17a194b 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -325,4 +325,17 @@ test_expect_success 'parsing reverse reflogs at BUFSIZ
boundaries' '
test_cmp expect actual
'
+test_expect_success 'no segfaults for reflog containing non-commit sha1s' '
+ git update-ref --create-reflog -m "Creating ref" \
+ refs/tests/tree-in-reflog HEAD &&
+ git update-ref -m "Forcing tree" refs/tests/tree-in-reflog HEAD^{tree}
&&
+ git update-ref -m "Restoring to commit" refs/tests/tree-in-reflog HEAD
&&
+ git reflog refs/tests/tree-in-reflog
+'
+
+test_expect_failure 'reflog with non-commit entries displays all entries' '
+ git reflog refs/tests/tree-in-reflog >actual &&
+ test_line_count = 3 actual
+'
+
test_done
--
2.7.0-rc3-219-g24972d4
--
Dennis Kaarsemaker
http://twitter.com/seveas
--
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
