Re: [PATCH] Fix mkpath abuse in dwim_ref/sha1_name.c

Junio C Hamano, Wed, Oct 15, 2008 01:17:52 +0200:

"Alex Riesen" [off-list ref] writes:

quoted

Otherwise the function sometimes fail to resolve obviously correct refnames,
because the string data pointed to by "ref" argument were reused.

But your patch instead rewrites the computation of str2 by bypassing the
call to "another_function_that_uses_get_pathname()" and duplicating its
logic, which I do not think is a viable approach in the longer term.

There is not enough logic to bypass there. It's just a dumb sprintf!
I actually saved a useless call (two, if automatic inlining
optimization doesn't work).

quoted

diff --git a/sha1_name.c b/sha1_name.c
index 41b6809..b5b53bf 100644
--- a/sha1_name.c
+++ b/sha1_name.c

@@ -242,6 +242,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 {
 	const char **p, *r;
 	int refs_found = 0;
+	char fullref[PATH_MAX];
 
 	*ref = NULL;
 	for (p = ref_rev_parse_rules; *p; p++) {

@@ -249,7 +250,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 		unsigned char *this_result;
 
 		this_result = refs_found ? sha1_from_ref : sha1;
-		r = resolve_ref(mkpath(*p, len, str), this_result, 1, NULL);
+		snprintf(fullref, sizeof(fullref), *p, len, str);
+		r = resolve_ref(fullref, this_result, 1, NULL);
 		if (r) {
 			if (!refs_found++)
 				*ref = xstrdup(r);

I suspect that I am grossly misleading the code, but I wonder why this xstrdup()
is not protecting us from the reusing of "the string data pointed to by
"ref" argument".  Are you fixing the overwriting of the string pointed to
by "str" argument instead?

Yes, I mistyped. xstrdup's called after the breakage happened.

What specific call chain has this breakage you found?

That's up to this function from builtin_rev_parse and resolve_ref
we're about to call.