{"thread":{"id":"45890","subject":"[PATCH] checkout: check return value of resolve_refdup before using hash","startedAt":"2017-05-06T17:14:22Z","lastAt":"2017-05-09T03:58:18Z","messageCount":2,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"318934","messageId":"f2e1a050-13e8-89be-ec6e-778054882ea3@web.de","threadId":"45890","inReplyTo":null,"subject":"[PATCH] checkout: check return value of resolve_refdup before using hash","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-05-06T17:13:52Z","receivedAt":"2017-05-06T17:14:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If resolve_refdup() fails it returns NULL and possibly leaves its hash\noutput parameter untouched.  Make sure to use it only if the function\nsucceeded, in order to avoid accessing uninitialized memory.\n\nFound with t/t2011-checkout-invalid-head.sh --valgrind.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/checkout.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex bfa5419f33..6c3d2e4f4c 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -833,7 +833,8 @@ static int switch_branches(const struct checkout_opts *opts,\n \tint flag, writeout_error = 0;\n \tmemset(&old, 0, sizeof(old));\n \told.path = path_to_free = resolve_refdup(\"HEAD\", 0, rev.hash, &flag);\n-\told.commit = lookup_commit_reference_gently(rev.hash, 1);\n+\tif (old.path)\n+\t\told.commit = lookup_commit_reference_gently(rev.hash, 1);\n \tif (!(flag & REF_ISSYMREF))\n \t\told.path = NULL;\n \n-- \n2.12.2\n\n"},{"id":"319159","messageId":"20170509035654.ywjabvwxj6rwelcn@sigill.intra.peff.net","threadId":"45890","inReplyTo":"f2e1a050-13e8-89be-ec6e-778054882ea3@web.de","subject":"Re: [PATCH] checkout: check return value of resolve_refdup before using hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-09T03:56:54Z","receivedAt":"2017-05-09T03:58:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 06, 2017 at 07:13:52PM +0200, René Scharfe wrote:\n\n> If resolve_refdup() fails it returns NULL and possibly leaves its hash\n> output parameter untouched.  Make sure to use it only if the function\n> succeeded, in order to avoid accessing uninitialized memory.\n> \n> Found with t/t2011-checkout-invalid-head.sh --valgrind.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  builtin/checkout.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index bfa5419f33..6c3d2e4f4c 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -833,7 +833,8 @@ static int switch_branches(const struct checkout_opts *opts,\n>  \tint flag, writeout_error = 0;\n>  \tmemset(&old, 0, sizeof(old));\n>  \told.path = path_to_free = resolve_refdup(\"HEAD\", 0, rev.hash, &flag);\n> -\told.commit = lookup_commit_reference_gently(rev.hash, 1);\n> +\tif (old.path)\n> +\t\told.commit = lookup_commit_reference_gently(rev.hash, 1);\n\nI wondered for a second what the value of old.commit would be in the\nerror case. But it should be NULL due to the memset above.\n\nBut what happens after that? Is everybody OK with NULL? I briefly poked\nthrough the callees (merge_working_tree, update_refs_for_switch, and\npost_checkout_hook) and they all seem to explicitly handle the NULL.\nSo I think this is good (well, clearly your change was an improvement\neither way, but I feel more confident now that there is nothing else to\nbe fixed on top of it).\n\n-Peff\n"}]}