{"thread":{"id":"59229","subject":"[PATCH] commit-reach: avoid NULL dereference","startedAt":"2023-02-11T11:15:29Z","lastAt":"2023-02-13T17:29:15Z","messageCount":4,"participants":["Eric Wong","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471976","messageId":"20230211111526.2028178-1-e@80x24.org","threadId":"59229","inReplyTo":null,"subject":"[PATCH] commit-reach: avoid NULL dereference","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-02-11T11:15:26Z","receivedAt":"2023-02-11T11:15:29Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"The loop at the top of can_all_from_reach_with_flag() already\naccounts for `from->objects[i].item' being NULL, so it follows\nthe cleanup loop should also account for a NULL `from_one'.\n\nI managed to segfault here on one of my giant, many-remote repos\nusing `git fetch --negotiation-tip=...  --negotiation-only'\nwhere the --negotiation-tip= argument was a glob which (inadvertently)\ncaptured more refs than I wanted.  I have not reproduced this\nin a standalone test case.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n Not sure if somebody who understands the code better can come\n up with a good standalone test case.  I figure using the top\n loop as reference is sufficient evidence that this fix is needed.\n\n commit-reach.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 2e33c599a82..1d7056338b7 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -807,8 +807,12 @@ int can_all_from_reach_with_flag(struct object_array *from,\n \tclear_commit_marks_many(nr_commits, list, RESULT | assign_flag);\n \tfree(list);\n \n-\tfor (i = 0; i < from->nr; i++)\n-\t\tfrom->objects[i].item->flags &= ~assign_flag;\n+\tfor (i = 0; i < from->nr; i++) {\n+\t\tstruct object *from_one = from->objects[i].item;\n+\n+\t\tif (from_one)\n+\t\t\tfrom_one->flags &= ~assign_flag;\n+\t}\n \n \treturn result;\n }\n"},{"id":"471988","messageId":"xmqqcz6fesca.fsf@gitster.g","threadId":"59229","inReplyTo":"20230211111526.2028178-1-e@80x24.org","subject":"Re: [PATCH] commit-reach: avoid NULL dereference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-11T22:43:17Z","receivedAt":"2023-02-11T22:43:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n>  Not sure if somebody who understands the code better can come\n>  up with a good standalone test case.  I figure using the top\n>  loop as reference is sufficient evidence that this fix is needed.\n\nGood comment.\n\n>  commit-reach.c | 8 ++++++--\n>  1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/commit-reach.c b/commit-reach.c\n> index 2e33c599a82..1d7056338b7 100644\n> --- a/commit-reach.c\n> +++ b/commit-reach.c\n> @@ -807,8 +807,12 @@ int can_all_from_reach_with_flag(struct object_array *from,\n>  \tclear_commit_marks_many(nr_commits, list, RESULT | assign_flag);\n>  \tfree(list);\n>  \n> -\tfor (i = 0; i < from->nr; i++)\n> -\t\tfrom->objects[i].item->flags &= ~assign_flag;\n> +\tfor (i = 0; i < from->nr; i++) {\n> +\t\tstruct object *from_one = from->objects[i].item;\n> +\n> +\t\tif (from_one)\n> +\t\t\tfrom_one->flags &= ~assign_flag;\n> +\t}\n\nThe flag clearing rule of this function smells somewhat iffy.  There\nare three primary callers of the function:\n\n * commit-reach.c::can_all_from_reach() calls the function, but it\n   has its own loop to clear the flag it asked the function to add.\n   If the function uses the flag as a temporary mark and is designed\n   to clear it from all the objects, as 4067a646 (commit-reach: fix\n   memory and flag leaks, 2018-09-21) states, why should the caller\n   have a separate loop to clear them?\n\n * fetch-pack.c::negotiate_using_fetch() calls this function in a\n   loop, so it does depend on it to clear the flag upon returning.\n\n * upload-pack.c::ok_to_give_up() is a thin wrapper around this\n   function and none callers of it have any logic to clear flag, so\n   it clearly depends on the function to clear the flag.\n\nThe above seems to indicate that the expectation by callers is a bit\nuneven.  Shouldn't the first onetrust the callee to clear the flag?\n\nEven before 4067a646 (commit-reach: fix memory and flag leaks,\n2018-09-21), the function had a call to clear_commit_marks() to\nclear two bits it used temporarily.  The reason why 4067a646 needed\nto add this additional flag clearing, whose NULL-dereference bug is\nbeing fixed with the patch in this thread, is because it marks any\nincoming object that peels to a non-commit (e.g. a blob, a tree, or\na tag that points at a non-commit) with the flag bit, but such a\nnon-commit object is not added to the list[] of commits to be\nprocessed, before the main processing of this function.\n\n\t\tfrom_one = deref_tag(the_repository, from_one,\n\t\t\t\t     \"a from object\", 0);\n\t\tif (!from_one || from_one->type != OBJ_COMMIT) {\n\t\t\t/*\n\t\t\t * no way to tell if this is reachable by\n\t\t\t * looking at the ancestry chain alone, so\n\t\t\t * leave a note to ourselves not to worry about\n\t\t\t * this object anymore.\n\t\t\t */\n\t\t\tfrom->objects[i].item->flags |= assign_flag;\n\t\t\tcontinue;\n\t\t}\n\n\t\tlist[nr_commits] = (struct commit *)from_one;\n\nBut I am not sure if it is even necessary to smudge the flag for the\nobject that was a non-commit (or the tag that peeled down to a\nnon-commit).  The main process of this function is a history\ntraversal that stops when the \"assign_flag\" bit is already set on\nthe found object, but the object that was part of the incoming\nobjects (i.e. in from->objects[] array) that turned out not to be a\nnon-commit would not be discovered during this history walk, would\nit?  In other words, if we walk from list[] that is an array or\ncommits to the parents (but not its trees and blobs), we won't\nencounter anything but commit.  What does it help to smudge an\nobject that peeled down to a non-commit in the incoming set of\nobjects, if it would not appear in the walk from list[]?  It would\nnot stop the traversal by having the flag.\n\nSo I wonder if we can just stop smudging the assign_flag bit for\nthese objects in from->objects[] that do not make it into list[]\nas a simpler fix?  Wouldn't that make the follow-up cleaning loop\nadded by 4067a646 (commit-reach: fix memory and flag leaks,\n2018-09-21) unneeded?\n\n\n\n\n\n"},{"id":"472009","messageId":"876cf920-113a-90cf-f49e-6e1b7b146acf@github.com","threadId":"59229","inReplyTo":"xmqqcz6fesca.fsf@gitster.g","subject":"Re: [PATCH] commit-reach: avoid NULL dereference","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-02-13T13:58:46Z","receivedAt":"2023-02-13T13:58:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/11/2023 5:43 PM, Junio C Hamano wrote:\n> Eric Wong <e@80x24.org> writes:\n\n>> -\tfor (i = 0; i < from->nr; i++)\n>> -\t\tfrom->objects[i].item->flags &= ~assign_flag;\n>> +\tfor (i = 0; i < from->nr; i++) {\n>> +\t\tstruct object *from_one = from->objects[i].item;\n>> +\n>> +\t\tif (from_one)\n>> +\t\t\tfrom_one->flags &= ~assign_flag;\n>> +\t}\n\nThis looks like a completely safe change to make, so this\npatch is good to go.\n\n> The flag clearing rule of this function smells somewhat iffy.  There\n> are three primary callers of the function:\n> \n>  * commit-reach.c::can_all_from_reach() calls the function, but it\n>    has its own loop to clear the flag it asked the function to add.\n>    If the function uses the flag as a temporary mark and is designed\n>    to clear it from all the objects, as 4067a646 (commit-reach: fix\n>    memory and flag leaks, 2018-09-21) states, why should the caller\n>    have a separate loop to clear them?\n\n...\n\n> The above seems to indicate that the expectation by callers is a bit\n> uneven.  Shouldn't the first onetrust the callee to clear the flag?\n\nYes, that makes sense. (But there's more!)\n \n> Even before 4067a646 (commit-reach: fix memory and flag leaks,\n> 2018-09-21), the function had a call to clear_commit_marks() to\n> clear two bits it used temporarily.  The reason why 4067a646 needed\n> to add this additional flag clearing, whose NULL-dereference bug is\n> being fixed with the patch in this thread, is because it marks any\n> incoming object that peels to a non-commit (e.g. a blob, a tree, or\n> a tag that points at a non-commit) with the flag bit, but such a\n> non-commit object is not added to the list[] of commits to be\n> processed, before the main processing of this function.\n> \n> \t\tfrom_one = deref_tag(the_repository, from_one,\n> \t\t\t\t     \"a from object\", 0);\n> \t\tif (!from_one || from_one->type != OBJ_COMMIT) {\n> \t\t\t/*\n> \t\t\t * no way to tell if this is reachable by\n> \t\t\t * looking at the ancestry chain alone, so\n> \t\t\t * leave a note to ourselves not to worry about\n> \t\t\t * this object anymore.\n> \t\t\t */\n> \t\t\tfrom->objects[i].item->flags |= assign_flag;\n> \t\t\tcontinue;\n> \t\t}\n> \n> \t\tlist[nr_commits] = (struct commit *)from_one;\n> \n> But I am not sure if it is even necessary to smudge the flag for the\n> object that was a non-commit (or the tag that peeled down to a\n> non-commit).  The main process of this function is a history\n> traversal that stops when the \"assign_flag\" bit is already set on\n> the found object, but the object that was part of the incoming\n> objects (i.e. in from->objects[] array) that turned out not to be a\n> non-commit would not be discovered during this history walk, would\n> it?  In other words, if we walk from list[] that is an array or\n> commits to the parents (but not its trees and blobs), we won't\n> encounter anything but commit.  What does it help to smudge an\n> object that peeled down to a non-commit in the incoming set of\n> objects, if it would not appear in the walk from list[]?  It would\n> not stop the traversal by having the flag.\n> \n> So I wonder if we can just stop smudging the assign_flag bit for\n> these objects in from->objects[] that do not make it into list[]\n> as a simpler fix?  Wouldn't that make the follow-up cleaning loop\n> added by 4067a646 (commit-reach: fix memory and flag leaks,\n> 2018-09-21) unneeded?\n\nThanks for digging into the details here. I agree that this extra\nassignment of the flag to these non-commit objects is unnecessary\nsince we intend to clear them by the end of the method and don't\ndo anything with the flags otherwise.\n\nWhat you seem to be suggesting is this diff:\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 2e33c599a82..8c387911228 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -740,10 +740,8 @@ int can_all_from_reach_with_flag(struct object_array *from,\n \t\t\t/*\n \t\t\t * no way to tell if this is reachable by\n \t\t\t * looking at the ancestry chain alone, so\n-\t\t\t * leave a note to ourselves not to worry about\n-\t\t\t * this object anymore.\n+\t\t\t * skip it.\n \t\t\t */\n-\t\t\tfrom->objects[i].item->flags |= assign_flag;\n \t\t\tcontinue;\n \t\t}\n \n@@ -856,17 +854,6 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,\n \n \tresult = can_all_from_reach_with_flag(&from_objs, PARENT2, PARENT1,\n \t\t\t\t\t      min_commit_date, min_generation);\n-\n-\twhile (from) {\n-\t\tclear_commit_marks(from->item, PARENT1);\n-\t\tfrom = from->next;\n-\t}\n-\n-\twhile (to) {\n-\t\tclear_commit_marks(to->item, PARENT2);\n-\t\tto = to->next;\n-\t}\n-\n \tobject_array_clear(&from_objs);\n \treturn result;\n }\n\nAnd instead of the current patch, this should allow the\nfollowing diff hunk instead:\n\n@@ -807,9 +805,6 @@ int can_all_from_reach_with_flag(struct object_array *from,\n \tclear_commit_marks_many(nr_commits, list, RESULT | assign_flag);\n \tfree(list);\n \n-\tfor (i = 0; i < from->nr; i++)\n-\t\tfrom->objects[i].item->flags &= ~assign_flag;\n-\n \treturn result;\n }\n\nThis avoids the need for the NULL check, since we are skipping the\nentire loop. The clear_commit_marks_many() is sufficient. \n\nThanks,\n-Stolee\n"},{"id":"472010","messageId":"xmqqmt5hcw4e.fsf@gitster.g","threadId":"59229","inReplyTo":"876cf920-113a-90cf-f49e-6e1b7b146acf@github.com","subject":"Re: [PATCH] commit-reach: avoid NULL dereference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-13T17:29:05Z","receivedAt":"2023-02-13T17:29:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>> The above seems to indicate that the expectation by callers is a bit\n>> uneven.  Shouldn't the first onetrust the callee to clear the flag?\n>\n> Yes, that makes sense. (But there's more!)\n> ...\n> Thanks for digging into the details here. I agree that this extra\n> assignment of the flag to these non-commit objects is unnecessary\n> since we intend to clear them by the end of the method and don't\n> do anything with the flags otherwise.\n>\n> What you seem to be suggesting is this diff:\n>\n> diff --git a/commit-reach.c b/commit-reach.c\n> index 2e33c599a82..8c387911228 100644\n> --- a/commit-reach.c\n> +++ b/commit-reach.c\n> @@ -740,10 +740,8 @@ int can_all_from_reach_with_flag(struct object_array *from,\n>  \t\t\t/*\n>  \t\t\t * no way to tell if this is reachable by\n>  \t\t\t * looking at the ancestry chain alone, so\n> -\t\t\t * leave a note to ourselves not to worry about\n> -\t\t\t * this object anymore.\n> +\t\t\t * skip it.\n>  \t\t\t */\n> -\t\t\tfrom->objects[i].item->flags |= assign_flag;\n>  \t\t\tcontinue;\n>  \t\t}\n\nAgreed with this part.\n\n> @@ -856,17 +854,6 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,\n>  \n>  \tresult = can_all_from_reach_with_flag(&from_objs, PARENT2, PARENT1,\n>  \t\t\t\t\t      min_commit_date, min_generation);\n> -\n> -\twhile (from) {\n> -\t\tclear_commit_marks(from->item, PARENT1);\n> -\t\tfrom = from->next;\n> -\t}\n\nThis too.\n\n> -\twhile (to) {\n> -\t\tclear_commit_marks(to->item, PARENT2);\n> -\t\tto = to->next;\n> -\t}\n\nBut I didn't think this is redundant.  PARENT2 is used to mark \"to\"\ncommits by this function, not the callee, before making the call.\nThe callee may clear PARENT1 used to mark the ones visited by the\ntraversal starting from the \"from\" commits, but does not do anything\nto \"with_flag\", does it?\n\n>  \tobject_array_clear(&from_objs);\n>  \treturn result;\n>  }\n>\n> And instead of the current patch, this should allow the\n> following diff hunk instead:\n>\n> @@ -807,9 +805,6 @@ int can_all_from_reach_with_flag(struct object_array *from,\n>  \tclear_commit_marks_many(nr_commits, list, RESULT | assign_flag);\n>  \tfree(list);\n>  \n> -\tfor (i = 0; i < from->nr; i++)\n> -\t\tfrom->objects[i].item->flags &= ~assign_flag;\n> -\n>  \treturn result;\n>  }\n>\n> This avoids the need for the NULL check, since we are skipping the\n> entire loop. The clear_commit_marks_many() is sufficient. \n\nYeah, that was my reading based on very limited and sufrace level of\nunderstanding of what this function was doing, although I think my\nunderstanding of it is still very skimpy (if I understand it well\nenough, I would give it a bit more understandable name but I cannot\ncome up with one yet).\n\nThanks.\n\n"}]}