threads / patch / 31628

patchgraph: avoid infinite loop in graph_show_commit()

Subject: [PATCH] graph: avoid infinite loop in graph_show_commit()

## tl;dr

5 messages between Sep 22, 2012 and Sep 25, 2012. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Sep 22, 2012, 14:24 UTC · lore

The loop can be triggered with "git diff-tree --graph commit" where the commit is a non-merge. It goes like this

 - graph_show_commit
 - graph_next_line
 - graph_output_padding_line

The last function quits because graph->commit is NULL, but graph_next_line() does not return "shown", so the loop in graph_show_commit keeps going.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Corner case. Nobody sane would do that. But still worth plugging.
 graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to graph.c +1 −1
diff --git a/graph.c b/graph.c
index e864fe2..1735b26 100644
--- a/graph.c
+++ b/graph.c
@@ -1224,7 +1224,7 @@ void graph_show_commit(struct git_graph *graph)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int shown_commit_line = 0;
 
-	if (!graph)
+	if (!graph || !graph->commit)
 		return;
 
 	while (!shown_commit_line) {

-- 
1.7.12.1.389.gc2218b5
Michal Kiedrowicz· Sep 23, 2012, 11:55 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] graph: avoid infinite loop in graph_show_commit()

Nguyễn Thái Ngọc Duy <pclouds <at> gmail.com> writes:
> 
> The loop can be triggered with "git diff-tree --graph commit" where
> the commit is a non-merge. It goes like this

Isn't this the same issue as in http://article.gmane.org/gmane.comp.version-control.git/123979 ? (with slightly different fix)

Nguyen Thai Ngoc Duy· Sep 23, 2012, 12:14 UTC · re: Michal Kiedrowicz · lore

Re: [PATCH] graph: avoid infinite loop in graph_show_commit()

On Sun, Sep 23, 2012 at 6:55 PM, Michal Kiedrowicz <michal.kiedrowicz@gmail.com> wrote:

Show 10 quoted lines
> Nguyễn Thái Ngọc Duy <pclouds <at> gmail.com> writes:
>
>>
>> The loop can be triggered with "git diff-tree --graph commit" where
>> the commit is a non-merge. It goes like this
>
>
> Isn't this the same issue as in
> http://article.gmane.org/gmane.comp.version-control.git/123979
> ? (with slightly different fix)

I don't know. I'm not familiar enough with graph.c to tell. Maybe Adam can have a look?

The patch that is cut out is http://article.gmane.org/gmane.comp.version-control.git/206205

-- 
Duy
Junio C Hamano· Sep 24, 2012, 23:36 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] graph: avoid infinite loop in graph_show_commit()

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
Show 15 quoted lines
> On Sun, Sep 23, 2012 at 6:55 PM, Michal Kiedrowicz
> <michal.kiedrowicz@gmail.com> wrote:
>> Nguyễn Thái Ngọc Duy <pclouds <at> gmail.com> writes:
>>
>>>
>>> The loop can be triggered with "git diff-tree --graph commit" where
>>> the commit is a non-merge. It goes like this
>>
>>
>> Isn't this the same issue as in
>> http://article.gmane.org/gmane.comp.version-control.git/123979
>> ? (with slightly different fix)
>
> I don't know. I'm not familiar enough with graph.c to tell. Maybe Adam
> can have a look?

Has either of you tried the patch with the problematic case the other patch tries to solve? Michal's old patch does smell like it is going in the better direction in that it stops looping when we know we would only be showing the padding, which is a sign that we are done with showing the commit.

But I didn't look at it too closely. I'd prefer to see the assert(0) turned into die("BUG: internal error") at the end of graph_next_line() to catch these cases. Also I am not sure if assignment of the return value from graph_next_line() to shown_comit_line in the loop is correct (shouldn't it be OR'ing it in, so that "we have shown the information on this commit" is not lost when the function adds things after showing the commit???)

Junio C Hamano· Sep 25, 2012, 18:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] graph: avoid infinite loop in graph_show_commit()

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> Has either of you tried the patch with the problematic case the
> other patch tries to solve?  Michal's old patch does smell like it
> is going in the better direction in that it stops looping when we
> know we would only be showing the padding, which is a sign that we
> are done with showing the commit.

I think this should suffice. I do not know if Michal's patch is the right fix, though. It appears to me that "--graph" assumes one commit is shown only once, but "diff-tree -m" and friends want to show a merge commit twice and is fundamentally incompatible with the assumption. We might be off either fixing that in the "graph" code (not with a band-aid like patches from you two to make it punt), or forbidding the combination altogether.

 t/t4202-log.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)
Show changes to t/t4202-log.sh +10 −0
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 71be59d..14f73e3 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -264,6 +264,16 @@ test_expect_success 'log --graph with merge' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log --raw --graph -m with merge' '
+	git log --raw --graph --oneline -m master | head -n 500 >actual &&
+	grep "initial" actual
+'
+
+test_expect_success 'diff-tree --graph' '
+	git diff-tree --graph master^ | head -n 500 >actual &&
+	grep "one" actual
+'
+
 cat > expect <<\EOF
 *   commit master
 |\  Merge: A B

-- 
1.7.12.1.451.gb433296

← back to recent threads