blame.c: fix garbled error message

The helper functions prepare_final() and prepare_initial() return a pointer to a string that is a member of an object in the revs->pending array. This array is later rebuilt when running prepare_revision_walk() which potentially transforms the pointer target into a bogus string. Fix this by maintaining a copy of the original string.

Signed-off-by Lukas Fleischer <[email protected]>

---
The bug manifests when running git blame HEAD^ -- nonexistent.file.

Note that I could have reduced code churn a little by moving the xstrdup() invocations to the call sites. However, I think that the return value of these functions should not depend on the consistency of a volatile data structure. On the other hand, you might also argue that there currently are only two call sites and that the functions are marked static, so if you prefer the less intrusive version, please let me know.

 builtin/blame.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

builtin/blame.c +7 -5

index 303e217..34d6f4f 100644
@@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	return commit;
 }
 
-static const char *prepare_final(struct scoreboard *sb)
+static char *prepare_final(struct scoreboard *sb)
 {
 	int i;
 	const char *final_commit_name = NULL;

@@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)
 		sb->final = (struct commit *) obj;
 		final_commit_name = revs->pending.objects[i].name;
 	}
-	return final_commit_name;
+	return xstrdup(final_commit_name);
 }
 
-static const char *prepare_initial(struct scoreboard *sb)
+static char *prepare_initial(struct scoreboard *sb)
 {
 	int i;
 	const char *final_commit_name = NULL;

@@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)
 	}
 	if (!final_commit_name)
 		die("No commit to dig down to?");
-	return final_commit_name;
+	return xstrdup(final_commit_name);
 }
 
 static int blame_copy_callback(const struct option *option, const char *arg, int unset)

@@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct origin *o;
 	struct blame_entry *ent = NULL;
 	long dashdash_pos, lno;
-	const char *final_commit_name = NULL;
+	char *final_commit_name = NULL;
 	enum object_type type;
 
 	static struct string_list range_list;

@@ -2786,6 +2786,8 @@ parse_done:
 
 	assign_blame(&sb, opt);
 
+	free(final_commit_name);
+
 	if (incremental)
 		return 0;

Quoted 9 lines · Lukas Fleischer

The helper functions prepare_final() and prepare_initial() return a pointer to a string that is a member of an object in the revs->pending array. This array is later rebuilt when running prepare_revision_walk() which potentially transforms the pointer target into a bogus string. Fix this by maintaining a copy of the original string.

Signed-off-by Lukas Fleischer <[email protected]>

---
The bug manifests when running git blame HEAD^ -- nonexistent.file.

Before 1da1e07c (clean up name allocation in prepare_revision_walk, 2014-10-15), these strings used to be non-volatile; they were instead leaked more or less deliberately. But these days, these strings are cleared, so your patch is absolutely the right thing to do.

Thanks for catching and fixing. This fix needs to go to the 2.2.x maintenance track.

Quoted 9 lines · Lukas Fleischer

The helper functions prepare_final() and prepare_initial() return a pointer to a string that is a member of an object in the revs->pending array. This array is later rebuilt when running prepare_revision_walk() which potentially transforms the pointer target into a bogus string. Fix this by maintaining a copy of the original string.

Signed-off-by Lukas Fleischer <[email protected]>

---
The bug manifests when running git blame HEAD^ -- nonexistent.file.

Before 1da1e07c (clean up name allocation in prepare_revision_walk, 2014-10-15), these strings used to be non-volatile; they were instead leaked more or less deliberately. But these days, these strings are cleared, so your patch is absolutely the right thing to do.

Thanks for catching and fixing. This fix needs to go to the 2.2.x maintenance track.

Quoted 7 lines

Note that I could have reduced code churn a little by moving the xstrdup() invocations to the call sites. However, I think that the return value of these functions should not depend on the consistency of a volatile data structure. On the other hand, you might also argue that there currently are only two call sites and that the functions are marked static, so if you prefer the less intrusive version, please let me know.

FWIW, I agree that this can be argued either way.

If we had a common low-level API that is used by short-term users to grab these names, it would be reasonable to make it the callers' responsibility to strdup() the return values for safekeeping if they want to keep using them long after the function returns. But I agree that prepare_final/prepare_initial are not such a low-level common API functions. I do not care too much about who does the strdup(), either the callers of prepare_* or the callee.

Quoted 56 lines

 builtin/blame.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

builtin/blame.c +7 -5

index 303e217..34d6f4f 100644
@@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	return commit;
 }
 
-static const char *prepare_final(struct scoreboard *sb)
+static char *prepare_final(struct scoreboard *sb)
 {
 	int i;
 	const char *final_commit_name = NULL;

@@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)
 		sb->final = (struct commit *) obj;
 		final_commit_name = revs->pending.objects[i].name;
 	}
-	return final_commit_name;
+	return xstrdup(final_commit_name);
 }
 
-static const char *prepare_initial(struct scoreboard *sb)
+static char *prepare_initial(struct scoreboard *sb)
 {
 	int i;
 	const char *final_commit_name = NULL;

@@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)
 	}
 	if (!final_commit_name)
 		die("No commit to dig down to?");
-	return final_commit_name;
+	return xstrdup(final_commit_name);
 }
 
 static int blame_copy_callback(const struct option *option, const char *arg, int unset)

@@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	struct origin *o;
 	struct blame_entry *ent = NULL;
 	long dashdash_pos, lno;
-	const char *final_commit_name = NULL;
+	char *final_commit_name = NULL;
 	enum object_type type;
 
 	static struct string_list range_list;

@@ -2786,6 +2786,8 @@ parse_done:
 
 	assign_blame(&sb, opt);
 
+	free(final_commit_name);
+
 	if (incremental)
 		return 0;

With the patch applied on top of 1da1e07c (or the result merged to 'next' for that matter), I see test breakages in many places "git blame" is used, e.g. t7010. Did you run the test suite?

This is because it is perfectly normal for prepare_final() to return NULL. Unconditionally running xstrdup() would of course fail.

As an aside, I have often been tempted to have xstrdup silently propagate a NULL. It would have been the right thing to do here, but maybe there are cases where the segfault is preferable for catching a mistake early (otherwise you might store the NULL and then segfault much later).

Quoted 5 lines · Jeff King

As an aside, I have often been tempted to have xstrdup silently propagate a NULL. It would have been the right thing to do here, but maybe there are cases where the segfault is preferable for catching a mistake early (otherwise you might store the NULL and then segfault much later).

Great minds think alike. The sentence after "but maybe ..." was what I had in mind as a response in anticipation that somebody might suggest that; a separate xstrdup_or_null() might be fine, but I'd rather not to have xstrdup() that is _too_ magical.

Quoted 6 lines · Jeff King

git-compat-util.h +0 -0

@@ -675,6 +675,11 @@ extern char *xgetcwd(void);
 
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
 

git-compat-util.h extern char *xgetcwd(void); · line 681

Quoted 4 lines

git-compat-util.h +4 -0

+static inline char *xstrdup_or_null(const char *str)
+{
+	return str ? xstrdup(str) : NULL;
+}

Would it make sense for xstrdup to always include the NULL check, avoiding the need for the more verbose xstrdup_or_null?

Quoted 8 lines · Junio C Hamano

Quoted 5 lines · Jeff King

As an aside, I have often been tempted to have xstrdup silently propagate a NULL. It would have been the right thing to do here, but maybe there are cases where the segfault is preferable for catching a mistake early (otherwise you might store the NULL and then segfault much later).

Quoted 4 lines

Great minds think alike. The sentence after "but maybe ..." was what I had in mind as a response in anticipation that somebody might suggest that; a separate xstrdup_or_null() might be fine, but I'd rather not to have xstrdup() that is _too_ magical.

Yeah. Of course, it is not _that_ many more characters to do a ternary conditional. I guess the main benefit is that you do not have to repeat the name of the variable (which lets you reuse a function result directly, avoiding an explicit temporary).

Here's my attempt. Some cases are a little nicer, but overall, it does not feel significantly more readable to me. I dunno. I could go either way. I stuck Lukas's patch on top (modified to use xstrdup_or_null), if we do want to go that route. Otherwise it needs the ?: treatment.

[1/5]: git-compat-util: add xstrdup_or_null helper [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup [4/5]: use xstrdup_or_null to replace ternary conditionals [5/5]: blame.c: fix garbled error message

Quoted 6 lines

[1/5]: git-compat-util: add xstrdup_or_null helper [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup [4/5]: use xstrdup_or_null to replace ternary conditionals [5/5]: blame.c: fix garbled error message

Looks good to me! I am not sure whether those patches should be built on top of (a fixed version of) my patch, though, which would make backporting the fix to the maintenance branch straightforward. Junio?

Quoted 10 lines · Lukas Fleischer

Quoted 6 lines

[1/5]: git-compat-util: add xstrdup_or_null helper [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup [4/5]: use xstrdup_or_null to replace ternary conditionals [5/5]: blame.c: fix garbled error message

Looks good to me! I am not sure whether those patches should be built on top of (a fixed version of) my patch, though, which would make backporting the fix to the maintenance branch straightforward. Junio?

We can queue these five on top of 1da1e07c (clean up name allocation in prepare_revision_walk, 2014-10-15), which changed the rule of the game to break this code, that only is in v2.2 and later.

And the result should merge just fine to 'maint'.

On Wed, Jan 14, 2015 at 09:22:52AM -0800, Junio C Hamano wrote:
...

Quoted 1 line

And the result should merge just fine to 'maint'.

Are we in agreement then that the resulting code with the helper is actually easier to read? I think replacing the straight ?: lines is, but I am on the fence on whether:

  const char *x = some_fun(...);
  return xstrdup_or_null(x);

is better or worse than:

  return xstrdup_or_null(some_fun(....));

Quoted 17 lines · Junio C Hamano

Quoted 9 lines · Lukas Fleischer

The helper functions prepare_final() and prepare_initial() return a pointer to a string that is a member of an object in the revs->pending array. This array is later rebuilt when running prepare_revision_walk() which potentially transforms the pointer target into a bogus string. Fix this by maintaining a copy of the original string.

Signed-off-by Lukas Fleischer <[email protected]>

---
The bug manifests when running git blame HEAD^ -- nonexistent.file.

Before 1da1e07c (clean up name allocation in prepare_revision_walk, 2014-10-15), these strings used to be non-volatile; they were instead leaked more or less deliberately. But these days, these strings are cleared, so your patch is absolutely the right thing to do.

Quoted 2 lines

Thanks for catching and fixing. This fix needs to go to the 2.2.x maintenance track.

Sigh, but not so fast.

With the patch applied on top of 1da1e07c (or the result merged to 'next' for that matter), I see test breakages in many places "git blame" is used, e.g. t7010. Did you run the test suite?

This is because it is perfectly normal for prepare_final() to return NULL. Unconditionally running xstrdup() would of course fail. [...]