{"thread":{"id":"48605","subject":"[PATCH 5/5] refspec.c: use rhs in parse_refspec instead of potentially uninitialized item->dst","startedAt":"2018-05-30T17:05:02Z","lastAt":"2018-06-01T19:38:08Z","messageCount":4,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"348820","messageId":"20180530170433.191495-1-sbeller@google.com","threadId":"48605","inReplyTo":null,"subject":"[PATCH 5/5] refspec.c: use rhs in parse_refspec instead of potentially uninitialized item->dst","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-30T17:04:33Z","receivedAt":"2018-05-30T17:05:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"'item->dst' has not been assigned if '!rhs' is true. As the caller is allowed to pass in uninitialized\nmemory (we don't assume 'item' was zeroed out before calling), this fixes an access to\nuninitialized memory.\n\nSigned-off-by: Stefan Beller \n---\n\napplies on bw/refspec-api\n\n refspec.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refspec.c b/refspec.c\nindex c59a4ccf1e5..ea169dec0d3 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -108,7 +108,7 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet\n \t\t * - empty is not allowed.\n \t\t * - otherwise it must be a valid looking ref.\n \t\t */\n-\t\tif (!item->dst) {\n+\t\tif (!rhs) {\n \t\t\tif (check_refname_format(item->src, flags))\n \t\t\t\treturn 0;\n \t\t} else if (!*item->dst) {\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"348959","messageId":"xmqqzi0f9see.fsf@gitster-ct.c.googlers.com","threadId":"48605","inReplyTo":"20180530170433.191495-1-sbeller@google.com","subject":"Re: [PATCH 5/5] refspec.c: use rhs in parse_refspec instead of potentially uninitialized item->dst","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-01T02:01:29Z","receivedAt":"2018-06-01T02:01:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> 'item->dst' has not been assigned if '!rhs' is true. As the caller is allowed to pass in uninitialized\n> memory (we don't assume 'item' was zeroed out before calling), this fixes an access to\n> uninitialized memory.\n\nDid I miss the other 4 patches that this might depend on it?\n\n> diff --git a/refspec.c b/refspec.c\n> index c59a4ccf1e5..ea169dec0d3 100644\n> --- a/refspec.c\n> +++ b/refspec.c\n> @@ -108,7 +108,7 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet\n> \t\t * - empty is not allowed.\n> \t\t * - otherwise it must be a valid looking ref.\n> \t\t */\n> -\t\tif (!item->dst) {\n> +\t\tif (!rhs) {\n> \t\t\tif (check_refname_format(item->src, flags))\n> \t\t\t\treturn 0;\n> \t\t} else if (!*item->dst) {\n\nPerhaps a better fisx is to explicitly assign NULL to item->dst when\nwe see there is no right-hand-side.\n\nAside from the \"uninitialized\" issue, the original if/else cascade\naround here makes a lot more sense than the updated version. If we\ndo not leave item->dst uninitialized, the control (and the readers'\nunderstanding) can flow without having to carry the invariant\n\"item->dst is set ONLY when rhs != NULL\" throughout this codepath,\nin order to understand that this if/else cascade is asking: is\npointer NULL? then do one thing, otherwise is pointee NUL? then do\nanother thing, otherwise we have a non-empty string so do something\non it.\n\n\n\n"},{"id":"348965","messageId":"xmqq8t7z9qc6.fsf@gitster-ct.c.googlers.com","threadId":"48605","inReplyTo":"xmqqzi0f9see.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 5/5] refspec.c: use rhs in parse_refspec instead of potentially uninitialized item->dst","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-01T02:46:01Z","receivedAt":"2018-06-01T02:46:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps a better fisx is to explicitly assign NULL to item->dst when\n> we see there is no right-hand-side.\n\n-- >8 --\nSubject: [PATCH] refspec-api: avoid uninitialized field in refspec item\n\nWhen parse_refspec() function was created at 3eec3700 (\"refspec:\nfactor out parsing a single refspec\", 2018-05-16) to take a caller\nsupplied piece of memory to fill parsed refspec_item, it forgot that\na refspec without colon must set item->dst to NULL to let the users\nof refspec know that the result of the fetch does not get stored in\nan ref on our side.\n\nSigned-off-by: Junio C Hamano \n---\n\n * The original before that change filled a callee prepared piece of\n memory that was obtained from xcalloc(), and did not need to\n explicitly assign NULL to the field after noticing that there is\n no colon in the refspec, so it is understandable how this\n misconvesion happened.\n\n refspec.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/refspec.c b/refspec.c\nindex 97e76e8b1d..6e45365a23 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -48,6 +48,8 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet\n \t\tsize_t rlen = strlen(++rhs);\n \t\tis_glob = (1 <= rlen && strchr(rhs, '*'));\n \t\titem->dst = xstrndup(rhs, rlen);\n+\t} else {\n+\t\titem->dst = NULL;\n \t}\n \n \tllen = (rhs ? (rhs - lhs - 1) : strlen(lhs));\n-- \n2.18.0-rc0\n\n"},{"id":"349012","messageId":"CAGZ79kaJ-k3rTiksJqkiJSvDv_bA-CKHskGXcTuA86Yyigf0oQ@mail.gmail.com","threadId":"48605","inReplyTo":"xmqq8t7z9qc6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 5/5] refspec.c: use rhs in parse_refspec instead of potentially uninitialized item->dst","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-01T19:38:04Z","receivedAt":"2018-06-01T19:38:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 31, 2018 at 7:46 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Perhaps a better fisx is to explicitly assign NULL to item->dst when\n>> we see there is no right-hand-side.\n>\n> -- >8 --\n> Subject: [PATCH] refspec-api: avoid uninitialized field in refspec item\n>\n> When parse_refspec() function was created at 3eec3700 (\"refspec:\n> factor out parsing a single refspec\", 2018-05-16) to take a caller\n> supplied piece of memory to fill parsed refspec_item, it forgot that\n> a refspec without colon must set item->dst to NULL to let the users\n> of refspec know that the result of the fetch does not get stored in\n> an ref on our side.\n>\n> Signed-off-by: Junio C Hamano \n\nThis looks correct. Thanks for writing the patch.\n\n> Did I miss the other 4 patches that this might depend on it?\n\nNo. I was sloppy and developed a couple of patches on top\nof pu and then tried to put them onto their respective branches.\nand then I forgot to correct the patch counts after figuring out the right\nbranch to apply this to.\n\nThanks,\nStefan\n"}]}