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

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

[René Scharfe]

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);
 	if (!(flag & REF_ISSYMREF))
 		old.path = NULL;
 
-- 
2.12.2

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

[Jeff King]

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

> 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