{"thread":{"id":"59621","subject":"[PATCH] merge-ort: fix calling merge_finalize() with no intermediate merge","startedAt":"2023-04-20T07:14:13Z","lastAt":"2023-04-24T15:17:31Z","messageCount":7,"participants":["Elijah Newren via GitGitGadget","Derrick Stolee","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"475728","messageId":"pull.1518.git.1681974847078.gitgitgadget@gmail.com","threadId":"59621","inReplyTo":null,"subject":"[PATCH] merge-ort: fix calling merge_finalize() with no intermediate merge","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-20T07:14:06Z","receivedAt":"2023-04-20T07:14:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIf some code sets up the data structures for a merge, but then never\nactually performs one before calling merge_finalize(), then\nmerge_finalize() wouldn't notice that result->priv was NULL and\nreturn early, resulting in following that NULL pointer and getting\na segfault. There is currently no code in the git codebase that does\nthis, but this issue was found during testing of some proposed patches\nthat had the following structure:\n\n struct merge_options merge_opt;\n struct merge_result result;\n\n init_merge_options(&merge_opt, the_repository);\n memset(&result, 0, sizeof(result));\n\n \n\n merge_finalize(&merge_opt, &result);\n\nwhere some flags could cause the code to have N=0, i.e. doing no merges.\nAdd a check for result->priv being NULL and return early to avoid a\nsegfault in these kinds of cases.\n\nWhile at it, ensure the FREE_AND_NULL() in the function does something\nuseful with the nulling aspect, namely sets result->priv to NULL rather\nthan a mere temporary.\n\nReported-by: Derrick Stolee \nSigned-off-by: Elijah Newren \n---\n merge-ort: fix calling merge_finalize() with no intermediate merge\n \n See\n https://lore.kernel.org/git/CABPp-BHCdjOutYqdMO1NbYKNA0BgkXRgwUEKK=MX0kXM-5G_DQ@mail.gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1518%2Fnewren%2Ffix-merge-finalize-with-no-merge-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1518/newren/fix-merge-finalize-with-no-merge-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1518\n\n merge-ort.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 5bf64354d16..cba3662e497 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4718,14 +4718,14 @@ void merge_switch_to_result(struct merge_options *opt,\n void merge_finalize(struct merge_options *opt,\n \t\t struct merge_result *result)\n {\n-\tstruct merge_options_internal *opti = result->priv;\n-\n \tif (opt->renormalize)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \tassert(opt->priv == NULL);\n \n-\tclear_or_reinit_internal_opts(opti, 0);\n-\tFREE_AND_NULL(opti);\n+\tif (!result->priv)\n+\t\treturn;\n+\tclear_or_reinit_internal_opts(result->priv, 0);\n+\tFREE_AND_NULL(result->priv);\n }\n \n /*** Function Grouping: helper functions for merge_incore_*() ***/\n\nbase-commit: 667fcf4e15379790f0b609d6a83d578e69f20301\n-- \ngitgitgadget\n"},{"id":"475732","messageId":"d5c93b5b-32b9-ec50-1661-06f73fa37f5f@github.com","threadId":"59621","inReplyTo":"pull.1518.git.1681974847078.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-ort: fix calling merge_finalize() with no intermediate merge","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-20T13:10:47Z","receivedAt":"2023-04-20T13:10:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/20/2023 3:14 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n\n> While at it, ensure the FREE_AND_NULL() in the function does something\n> useful with the nulling aspect, namely sets result->priv to NULL rather\n> than a mere temporary.\n\nGood call. It also makes the code look better.\n \n> void merge_finalize(struct merge_options *opt,\n> \t\t struct merge_result *result)\n> {\n> -\tstruct merge_options_internal *opti = result->priv;\n> -\n> \tif (opt->renormalize)\n> \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n> \tassert(opt->priv == NULL);\n> \n> -\tclear_or_reinit_internal_opts(opti, 0);\n> -\tFREE_AND_NULL(opti);\n> +\tif (!result->priv)\n> +\t\treturn;\n> +\tclear_or_reinit_internal_opts(result->priv, 0);\n> +\tFREE_AND_NULL(result->priv);\n\nPerhaps this would be better as\n\n\tif (result->priv) {\n\t\tclear_or_reinit_internal_opts(result->priv, 0);\n\t\tFREE_AND_NULL(result->priv);\n\t}\n\nto avoid an accidental addition of code to the end of this\nmethod that doesn't depend on result->priv?\n\nThanks,\n-Stolee\n"},{"id":"475739","messageId":"xmqqedoe4jqf.fsf@gitster.g","threadId":"59621","inReplyTo":"pull.1518.git.1681974847078.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-ort: fix calling merge_finalize() with no intermediate merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T16:11:04Z","receivedAt":"2023-04-20T16:11:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> If some code sets up the data structures for a merge, but then never\n> actually performs one before calling merge_finalize(), then\n> merge_finalize() wouldn't notice that result->priv was NULL and\n> return early, resulting in following that NULL pointer and getting\n> a segfault. There is currently no code in the git codebase that does\n> this, but this issue was found during testing of some proposed patches\n> that had the following structure:\n>\n> struct merge_options merge_opt;\n> struct merge_result result;\n>\n> init_merge_options(&merge_opt, the_repository);\n> memset(&result, 0, sizeof(result));\n>\n> \n>\n> merge_finalize(&merge_opt, &result);\n>\n> where some flags could cause the code to have N=0, i.e. doing no merges.\n> Add a check for result->priv being NULL and return early to avoid a\n> segfault in these kinds of cases.\n>\n> While at it, ensure the FREE_AND_NULL() in the function does something\n> useful with the nulling aspect, namely sets result->priv to NULL rather\n> than a mere temporary.\n\nClearly written. The FREE_AND_NULL() bit is a bit embarrassing\nslip-up for everybody who reviewed and applied the initial patch\nX-<.\n\nAnd we need to undo the attribute direction change in merge_start()\neven if we didn't perform any, so the early return cannot be done\nbefore that. So everything makes sense as a fix to the reported\nproblem.\n\n> Reported-by: Derrick Stolee \n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort: fix calling merge_finalize() with no intermediate merge\n> \n> See\n> https://lore.kernel.org/git/CABPp-BHCdjOutYqdMO1NbYKNA0BgkXRgwUEKK=MX0kXM-5G_DQ@mail.gmail.com/\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1518%2Fnewren%2Ffix-merge-finalize-with-no-merge-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1518/newren/fix-merge-finalize-with-no-merge-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1518\n>\n> merge-ort.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 5bf64354d16..cba3662e497 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -4718,14 +4718,14 @@ void merge_switch_to_result(struct merge_options *opt,\n> void merge_finalize(struct merge_options *opt,\n> \t\t struct merge_result *result)\n> {\n> -\tstruct merge_options_internal *opti = result->priv;\n> -\n> \tif (opt->renormalize)\n> \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n> \tassert(opt->priv == NULL);\n> \n> -\tclear_or_reinit_internal_opts(opti, 0);\n> -\tFREE_AND_NULL(opti);\n> +\tif (!result->priv)\n> +\t\treturn;\n> +\tclear_or_reinit_internal_opts(result->priv, 0);\n> +\tFREE_AND_NULL(result->priv);\n> }\n> \n> /*** Function Grouping: helper functions for merge_incore_*() ***/\n>\n> base-commit: 667fcf4e15379790f0b609d6a83d578e69f20301\n"},{"id":"475746","messageId":"xmqqa5z21nuq.fsf@gitster.g","threadId":"59621","inReplyTo":"d5c93b5b-32b9-ec50-1661-06f73fa37f5f@github.com","subject":"Re: [PATCH] merge-ort: fix calling merge_finalize() with no intermediate merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-20T17:10:21Z","receivedAt":"2023-04-20T17:10:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 4/20/2023 3:14 AM, Elijah Newren via GitGitGadget wrote:\n>> From: Elijah Newren \n>\n>> While at it, ensure the FREE_AND_NULL() in the function does something\n>> useful with the nulling aspect, namely sets result->priv to NULL rather\n>> than a mere temporary.\n>\n> Good call. It also makes the code look better.\n\nAnd more correct, if the NULLness of result->priv matters.\n\n> Perhaps this would be better as\n>\n> \tif (result->priv) {\n> \t\tclear_or_reinit_internal_opts(result->priv, 0);\n> \t\tFREE_AND_NULL(result->priv);\n> \t}\n>\n> to avoid an accidental addition of code to the end of this\n> method that doesn't depend on result->priv?\n\nIt does sound like a good idea.\n\nThanks.\n"},{"id":"475827","messageId":"CABPp-BGVEWM_N3eXjfEGjemeNzrLMmqDOuWKCvzOuHGeMueoJQ@mail.gmail.com","threadId":"59621","inReplyTo":"d5c93b5b-32b9-ec50-1661-06f73fa37f5f@github.com","subject":"Re: [PATCH] merge-ort: fix calling merge_finalize() with no intermediate merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-22T02:04:41Z","receivedAt":"2023-04-22T02:05:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Apr 20, 2023 at 6:10 AM Derrick Stolee wrote:\n>\n> On 4/20/2023 3:14 AM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n>\n> > While at it, ensure the FREE_AND_NULL() in the function does something\n> > useful with the nulling aspect, namely sets result->priv to NULL rather\n> > than a mere temporary.\n>\n> Good call. It also makes the code look better.\n>\n> > void merge_finalize(struct merge_options *opt,\n> > struct merge_result *result)\n> > {\n> > - struct merge_options_internal *opti = result->priv;\n> > -\n> > if (opt->renormalize)\n> > git_attr_set_direction(GIT_ATTR_CHECKIN);\n> > assert(opt->priv == NULL);\n> >\n> > - clear_or_reinit_internal_opts(opti, 0);\n> > - FREE_AND_NULL(opti);\n> > + if (!result->priv)\n> > + return;\n> > + clear_or_reinit_internal_opts(result->priv, 0);\n> > + FREE_AND_NULL(result->priv);\n>\n> Perhaps this would be better as\n>\n> if (result->priv) {\n> clear_or_reinit_internal_opts(result->priv, 0);\n> FREE_AND_NULL(result->priv);\n> }\n>\n> to avoid an accidental addition of code to the end of this\n> method that doesn't depend on result->priv?\n\nOoh, yes, that'd be a good improvement; thanks.\n"},{"id":"475876","messageId":"pull.1518.v2.git.1682194930766.gitgitgadget@gmail.com","threadId":"59621","inReplyTo":"pull.1518.git.1681974847078.gitgitgadget@gmail.com","subject":"[PATCH v2] merge-ort: fix calling merge_finalize() with no intermediate merge","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-22T20:22:10Z","receivedAt":"2023-04-22T20:22:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIf some code sets up the data structures for a merge, but then never\nactually performs one before calling merge_finalize(), then\nmerge_finalize() wouldn't notice that result->priv was NULL and\nreturn early, resulting in following that NULL pointer and getting\na segfault. There is currently no code in the git codebase that does\nthis, but this issue was found during testing of some proposed patches\nthat had the following structure:\n\n struct merge_options merge_opt;\n struct merge_result result;\n\n init_merge_options(&merge_opt, the_repository);\n memset(&result, 0, sizeof(result));\n\n \n\n merge_finalize(&merge_opt, &result);\n\nwhere some flags could cause the code to have N=0, i.e. doing no merges.\nAdd a check for result->priv being NULL and return early to avoid a\nsegfault in these kinds of cases.\n\nWhile at it, ensure the FREE_AND_NULL() in the function does something\nuseful with the nulling aspect, namely sets result->priv to NULL rather\nthan a mere temporary.\n\nReported-by: Derrick Stolee \nSigned-off-by: Elijah Newren \n---\n merge-ort: fix calling merge_finalize() with no intermediate merge\n \n Changes since v1:\n \n * Moved code into an if-block instead of returning early, as suggested\n by Stolee.\n \n See\n https://lore.kernel.org/git/CABPp-BHCdjOutYqdMO1NbYKNA0BgkXRgwUEKK=MX0kXM-5G_DQ@mail.gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1518%2Fnewren%2Ffix-merge-finalize-with-no-merge-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1518/newren/fix-merge-finalize-with-no-merge-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1518\n\nRange-diff vs v1:\n\n 1: e636027f60c ! 1: 2cb1c63f05b merge-ort: fix calling merge_finalize() with no intermediate merge\n @@ merge-ort.c: void merge_switch_to_result(struct merge_options *opt,\n \n -\tclear_or_reinit_internal_opts(opti, 0);\n -\tFREE_AND_NULL(opti);\n -+\tif (!result->priv)\n -+\t\treturn;\n -+\tclear_or_reinit_internal_opts(result->priv, 0);\n -+\tFREE_AND_NULL(result->priv);\n ++\tif (result->priv) {\n ++\t\tclear_or_reinit_internal_opts(result->priv, 0);\n ++\t\tFREE_AND_NULL(result->priv);\n ++\t}\n }\n \n /*** Function Grouping: helper functions for merge_incore_*() ***/\n\n\n merge-ort.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 5bf64354d16..29966fc082f 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4718,14 +4718,14 @@ void merge_switch_to_result(struct merge_options *opt,\n void merge_finalize(struct merge_options *opt,\n \t\t struct merge_result *result)\n {\n-\tstruct merge_options_internal *opti = result->priv;\n-\n \tif (opt->renormalize)\n \t\tgit_attr_set_direction(GIT_ATTR_CHECKIN);\n \tassert(opt->priv == NULL);\n \n-\tclear_or_reinit_internal_opts(opti, 0);\n-\tFREE_AND_NULL(opti);\n+\tif (result->priv) {\n+\t\tclear_or_reinit_internal_opts(result->priv, 0);\n+\t\tFREE_AND_NULL(result->priv);\n+\t}\n }\n \n /*** Function Grouping: helper functions for merge_incore_*() ***/\n\nbase-commit: 667fcf4e15379790f0b609d6a83d578e69f20301\n-- \ngitgitgadget\n"},{"id":"475922","messageId":"753bfefb-b80c-e1de-c4f9-90737bd798db@github.com","threadId":"59621","inReplyTo":"pull.1518.v2.git.1682194930766.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] merge-ort: fix calling merge_finalize() with no intermediate merge","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-24T15:17:24Z","receivedAt":"2023-04-24T15:17:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/22/2023 4:22 PM, Elijah Newren via GitGitGadget wrote:\n> merge-ort: fix calling merge_finalize() with no intermediate merge\n> \n> Changes since v1:\n> \n> * Moved code into an if-block instead of returning early, as suggested\n> by Stolee.\n\nv2 LGTM. Thanks!\n\n-Stolee\n"}]}