Re: [PATCH 30/33] tree-diff: convert diff_tree_paths to struct object_id

["René Scharfe" <l.s.r@web.de>]

Am 30.05.2017 um 19:31 schrieb Brandon Williams:
> @@ -273,21 +274,20 @@ static struct combine_diff_path *emit_path(struct combine_diff_path *p,
>   	}
>   
>   	if (recurse) {
> -		const unsigned char **parents_sha1;
> +		const struct object_id **parents_oid;
>   
> -		FAST_ARRAY_ALLOC(parents_sha1, nparent);
> +		FAST_ARRAY_ALLOC(parents_oid, nparent);
>   		for (i = 0; i < nparent; ++i) {
>   			/* same rule as in emitthis */
>   			int tpi_valid = tp && !(tp[i].entry.mode & S_IFXMIN_NEQ);
>   
> -			parents_sha1[i] = tpi_valid ? tp[i].entry.oid->hash
> -						    : NULL;
> +			parents_oid[i] = tpi_valid ? tp[i].entry.oid : NULL;
>   		}

So elements of parents_oid can be NULL...

>   
>   		strbuf_add(base, path, pathlen);
>   		strbuf_addch(base, '/');
> -		p = ll_diff_tree_paths(p, sha1, parents_sha1, nparent, base, opt);
> -		FAST_ARRAY_FREE(parents_sha1, nparent);
> +		p = ll_diff_tree_paths(p, oid, parents_oid, nparent, base, opt);

... and we pass that array to ll_diff_tree_paths()...

> +		FAST_ARRAY_FREE(parents_oid, nparent);
>   	}
>   
>   	strbuf_setlen(base, old_baselen);
> @@ -312,7 +312,7 @@ static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,
>   
>   
>   /*
> - * generate paths for combined diff D(sha1,parents_sha1[])
> + * generate paths for combined diff D(sha1,parents_oid[])
>    *
>    * Resulting paths are appended to combine_diff_path linked list, and also, are
>    * emitted on the go via opt->pathchange() callback, so it is possible to
> @@ -404,8 +404,8 @@ static inline void update_tp_entries(struct tree_desc *tp, int nparent)
>   }
>   
>   static struct combine_diff_path *ll_diff_tree_paths(
> -	struct combine_diff_path *p, const unsigned char *sha1,
> -	const unsigned char **parents_sha1, int nparent,
> +	struct combine_diff_path *p, const struct object_id *oid,
> +	const struct object_id **parents_oid, int nparent,
>   	struct strbuf *base, struct diff_options *opt)
>   {
>   	struct tree_desc t, *tp;
> @@ -422,8 +422,8 @@ static struct combine_diff_path *ll_diff_tree_paths(
>   	 *   diff_tree_oid(parent, commit) )
>   	 */
>   	for (i = 0; i < nparent; ++i)
> -		tptree[i] = fill_tree_descriptor(&tp[i], parents_sha1[i]);
> -	ttree = fill_tree_descriptor(&t, sha1);
> +		tptree[i] = fill_tree_descriptor(&tp[i], parents_oid[i]->hash);

... and here we are in that function, dereferencing a pointer that may
be NULL.

> +	ttree = fill_tree_descriptor(&t, oid->hash);

oid here can also be NULL, but I think that's not as easy to see.

-- >8 --
Subject: [PATCH] tree-diff: don't access hash of NULL object_id pointer

The object_id pointers can be NULL for invalid entries.  Don't try to
dereference them and pass NULL along to fill_tree_descriptor() instead,
which handles them just fine.

Found with Clang's UBSan.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
fill_tree_descriptor() can easily be converted to object_id, by the
way, which would get us rid of the extra check introduced here, but
this patch is meant as a minimal fix.

 tree-diff.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index bd6d65a409..2357f72899 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -421,8 +421,9 @@ static struct combine_diff_path *ll_diff_tree_paths(
 	 *   diff_tree_oid(parent, commit) )
 	 */
 	for (i = 0; i < nparent; ++i)
-		tptree[i] = fill_tree_descriptor(&tp[i], parents_oid[i]->hash);
-	ttree = fill_tree_descriptor(&t, oid->hash);
+		tptree[i] = fill_tree_descriptor(&tp[i],
+				parents_oid[i] ? parents_oid[i]->hash : NULL);
+	ttree = fill_tree_descriptor(&t, oid ? oid->hash : NULL);
 
 	/* Enable recursion indefinitely */
 	opt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
-- 
2.13.3