"git-diff -p :/anything" always segfaults

"git-diff -p :/anything" always segfaults

[Jim Meyering]

I like the idea of the new ':/<oneline prefix>' notation, and gave it
a try, but all I could get was a segfault.  It was dereferencing a NULL
commit list.  Fix below.  With it, this example now works:

$ mkdir .j; cd .j; touch f
$ (git-init; git-add f; git-commit -mc f; echo x >f; git-commit -md f)>/dev/null
$ git-diff -p :/c :/d
diff --git a/f b/f
index e69de29..587be6b 100644
--- a/f
+++ b/f
@@ -0,0 +1 @@
+x

Signed-off-by: Jim Meyering <jim@meyering.net>
---
 sha1_name.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 31812d3..6b6270b 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -617,7 +617,7 @@ int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 	for_each_ref(handle_one_ref, &list);
 	for (l = list; l; l = l->next)
 		commit_list_insert(l->item, &backup);
-	while ((commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {
+	while (list && (commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {
 		char *p;
 		parse_object(commit->object.sha1);
 		if (!commit->buffer || !(p = strstr(commit->buffer, "\n\n")))
--
1.5.0.3.316.gbd1fc

Re: "git-diff -p :/anything" always segfaults

[Johannes Schindelin]

Hi,

On Sun, 11 Mar 2007, Jim Meyering wrote:

> I like the idea of the new ':/<oneline prefix>' notation, and gave it a 
> try, but all I could get was a segfault.

Thanks. I did not realize that it did not work correctly, since the pager 
hid the segfault.

Ciao,
Dscho

Re: "git-diff -p :/anything" always segfaults

[Linus Torvalds]

On Sun, 11 Mar 2007, Jim Meyering wrote:
>
> I like the idea of the new ':/<oneline prefix>' notation, and gave it
> a try, but all I could get was a segfault.  It was dereferencing a NULL
> commit list.  Fix below.  With it, this example now works:

The fix is correct, but not complete.

> -	while ((commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {
> +	while (list && (commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {

The old code was broken, but the new one isn't much better.

"pop_most_recent_commit()" simply doesn't work that way. It *never* 
returns NULL. So having it as part of a while-loop was buggy to begin 
with, and you fixed the test, but the thing is, it should just look like

	while (list) {
		struct commit *commit;

		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
		..

and the "pop_most_recent_commit()" simply shouldn't be part of the 
conditional at all.

Alternatively, we could just change the semantics, and have it return NULL 
when the list is empty. That would probably be fine too, but then the old 
code was correct.

Hmm?

		Linus

Re: "git-diff -p :/anything" always segfaults

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, 11 Mar 2007, Jim Meyering wrote:
>>
>> I like the idea of the new ':/<oneline prefix>' notation, and gave it
>> a try, but all I could get was a segfault.  It was dereferencing a NULL
>> commit list.  Fix below.  With it, this example now works:
>
> The fix is correct, but not complete.
>
>> -	while ((commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {
>> +	while (list && (commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {
>
> The old code was broken, but the new one isn't much better.
>
> "pop_most_recent_commit()" simply doesn't work that way. It *never* 
> returns NULL. So having it as part of a while-loop was buggy to begin 
> with, and you fixed the test, but the thing is, it should just look like
>
> 	while (list) {
> 		struct commit *commit;
>
> 		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
> 		..
>
> and the "pop_most_recent_commit()" simply shouldn't be part of the 
> conditional at all.

That's what I did in my tentative commit based on Jim's patch
(except "commit" is also used to determine the return value from
the function).

Re: "git-diff -p :/anything" always segfaults

[Linus Torvalds]

On Sun, 11 Mar 2007, Junio C Hamano wrote:
> >
> > "pop_most_recent_commit()" simply doesn't work that way. It *never* 
> > returns NULL. So having it as part of a while-loop was buggy to begin 
> > with, and you fixed the test, but the thing is, it should just look like
> >
> > 	while (list) {
> > 		struct commit *commit;
> >
> > 		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
> > 		..
> >
> > and the "pop_most_recent_commit()" simply shouldn't be part of the 
> > conditional at all.
> 
> That's what I did in my tentative commit based on Jim's patch
> (except "commit" is also used to determine the return value from
> the function).

Your version is *also* buggy.

And the bug is exactly that return value.

You simply cannot do it that way. You do

	...
	commit = pop_most_recent_commit(&list, ONELINE_SEEN);
	if (!commit)
		break;
	...

which is totally broken and pointless, and more importantly, *exactly* 
because you use "commit" outside the loop, it's broken.

It will now return the last commit it ever saw, whether that commit 
matches the buffer or not. Because "commit" will *never* be set to NULL 
ever again after the initial assignment!

The fact is, if you don't do it the way I described:

	while (list) {
		struct commit *commit;

		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
		...

you will almost certainly have a bug. Why? Because the return value of 
"pop_most_recent_commit()" simply *makes*no*sense* outside the loop. It's 
that simple. 

Outside the loop, you have to have a *different* return value, namely the 
one that matches. Which may be NULL. Something that 
pop_most_recent_commit() simply never returns!

So I repeat my suggestion:

 - either use the sequence above
 - or change the semantics of "pop_most_recent_commit()" to return NULL if 
   the list is empty.

Considering that there's now been two totally broken versions of this 
loop, with the exact same bug, I'm inclined to say that you should just 
change "pop_most_recent_commit()" instead, and then change the loop back 
to its original form of "while ((commit = pop_..) != NULL) {"

Otherwise, please apply something like this, which also fixes the return 
value. We should return -1 on error!

We should add a test with

	git log :/hjashjs

or similar. It's supposed to return an error ("fatal: ambiguous argument 
':/hjashjs': unknown revision or path not in the working tree.")

		Linus

 [ Same return value rules as for the kernel and for system calls, either:

    - 0/1 for "false/true", where _1_ is success, and 0 is "false".

    - negative/0 for "error/no-error", where _0_ is success, and negative 
      is a failure.

   but not some mixture with 1 being failure! ]
---
diff --git a/sha1_name.c b/sha1_name.c
index 6b8b67b..40dd2d1 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -602,10 +602,10 @@ static int handle_one_ref(const char *path,
  */
 
 #define ONELINE_SEEN (1u<<20)
-int get_sha1_oneline(const char *prefix, unsigned char *sha1)
+static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 {
 	struct commit_list *list = NULL, *backup = NULL, *l;
-	struct commit *commit = NULL;
+	int retval = -1;
 
 	if (prefix[0] == '!') {
 		if (prefix[1] != '!')
@@ -619,6 +619,7 @@ int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 		commit_list_insert(l->item, &backup);
 	while (list) {
 		char *p;
+		struct commit *commit;
 
 		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
 		if (!commit)
@@ -628,13 +629,14 @@ int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 			continue;
 		if (!prefixcmp(p + 2, prefix)) {
 			hashcpy(sha1, commit->object.sha1);
+			retval = 0;
 			break;
 		}
 	}
 	free_commit_list(list);
 	for (l = backup; l; l = l->next)
 		clear_commit_marks(l->item, ONELINE_SEEN);
-	return commit == NULL;
+	return retval;
 }
 
 /*

Re: "git-diff -p :/anything" always segfaults

[Linus Torvalds]

On Mon, 12 Mar 2007, Linus Torvalds wrote:
> 
> Otherwise, please apply something like this, which also fixes the return 
> value. We should return -1 on error!

Btw, I didn't remove the 

	if (!commit)
		break;

but I should have. It doesn't actually matter (since it will never 
trigger), but it adds to the confusion around pop_most_recent_commit.

		Linus

Re: "git-diff -p :/anything" always segfaults

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Your version is *also* buggy.

You are right.

Applied, and thanks.