[PATCH] checkout: check return value of resolve_refdup before using hash

René Scharfe· May 6, 2017, 17:13 UTC · lore

If resolve_refdup() fails it returns NULL and possibly leaves its hash output parameter untouched. Make sure to use it only if the function succeeded, in order to avoid accessing uninitialized memory.

Found with t/t2011-checkout-invalid-head.sh --valgrind.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/checkout.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Show changes to builtin/checkout.c +2 −1

diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f33..6c3d2e4f4c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -833,7 +833,8 @@ static int switch_branches(const struct checkout_opts *opts,
 	int flag, writeout_error = 0;
 	memset(&old, 0, sizeof(old));
 	old.path = path_to_free = resolve_refdup("HEAD", 0, rev.hash, &flag);
-	old.commit = lookup_commit_reference_gently(rev.hash, 1);
+	if (old.path)
+		old.commit = lookup_commit_reference_gently(rev.hash, 1);
 	if (!(flag & REF_ISSYMREF))
 		old.path = NULL;

-- 
2.12.2

Jeff King· May 9, 2017, 03:56 UTC · re: René Scharfe · lore

Re: [PATCH] checkout: check return value of resolve_refdup before using hash

On Sat, May 06, 2017 at 07:13:52PM +0200, René Scharfe wrote:

Show 22 quoted lines

> If resolve_refdup() fails it returns NULL and possibly leaves its hash
> output parameter untouched.  Make sure to use it only if the function
> succeeded, in order to avoid accessing uninitialized memory.
> 
> Found with t/t2011-checkout-invalid-head.sh --valgrind.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/checkout.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfa5419f33..6c3d2e4f4c 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -833,7 +833,8 @@ static int switch_branches(const struct checkout_opts *opts,
>  	int flag, writeout_error = 0;
>  	memset(&old, 0, sizeof(old));
>  	old.path = path_to_free = resolve_refdup("HEAD", 0, rev.hash, &flag);
> -	old.commit = lookup_commit_reference_gently(rev.hash, 1);
> +	if (old.path)
> +		old.commit = lookup_commit_reference_gently(rev.hash, 1);

I wondered for a second what the value of old.commit would be in the error case. But it should be NULL due to the memset above.

But what happens after that? Is everybody OK with NULL? I briefly poked through the callees (merge_working_tree, update_refs_for_switch, and post_checkout_hook) and they all seem to explicitly handle the NULL. So I think this is good (well, clearly your change was an improvement either way, but I feel more confident now that there is nothing else to be fixed on top of it).

-Peff

patchcheckout: check return value of resolve_refdup before using hash