{"thread":{"id":"55360","subject":"[PATCH] fix null pointer dereference","startedAt":"2021-03-21T10:53:24Z","lastAt":"2021-03-21T17:26:14Z","messageCount":4,"participants":["Kleber Tarcísio via GitGitGadget","Ævar Arnfjörð Bjarmason","Matheus Tavares Bernardino","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"419845","messageId":"pull.983.git.git.1616323936790.gitgitgadget@gmail.com","threadId":"55360","inReplyTo":null,"subject":"[PATCH] fix null pointer dereference","fromName":"Kleber Tarcísio via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-21T10:52:16Z","receivedAt":"2021-03-21T10:53:24Z","isPatch":true,"sender":{"key":"name:Kleber Tarcísio","avatar":null},"body":"From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= \n\nThe malloc function can return null when the memory allocation fails. This commit adds a condition to handle these cases properly. https://cwe.mitre.org/data/definitions/476.html\n\nSigned-off-by: Kleber Tarcísio \n---\n Avoiding null pointer dereference\n \n This pull request aims to fix null pointer dereference.\n \n Null pointer dereference\n [https://cwe.mitre.org/data/definitions/476.html]\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-983%2Fklebertarcisio%2Fpatch-1-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-983/klebertarcisio/patch-1-v1\nPull-Request: https://github.com/git/git/pull/983\n\n builtin/submodule--helper.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 9d505a6329c8..92349d715a78 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1215,6 +1215,8 @@ static void submodule_summary_callback(struct diff_queue_struct *q,\n \t\tif (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode))\n \t\t\tcontinue;\n \t\ttemp = (struct module_cb*)malloc(sizeof(struct module_cb));\n+\t\tif (!temp) \n+\t\t\tdie(_(\"out of memory\"));\n \t\ttemp->mod_src = p->one->mode;\n \t\ttemp->mod_dst = p->two->mode;\n \t\ttemp->oid_src = p->one->oid;\n\nbase-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f\n-- \ngitgitgadget\n"},{"id":"419852","messageId":"87tup4r81k.fsf@evledraar.gmail.com","threadId":"55360","inReplyTo":"pull.983.git.git.1616323936790.gitgitgadget@gmail.com","subject":"Re: [PATCH] fix null pointer dereference","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-21T12:43:51Z","receivedAt":"2021-03-21T12:44:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Mar 21 2021, Kleber Tarcísio via GitGitGadget wrote:\n\n> From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= \n>\n> The malloc function can return null when the memory allocation fails. This commit adds a condition to handle these cases properly. https://cwe.mitre.org/data/definitions/476.html\n>\n> Signed-off-by: Kleber Tarcísio \n> ---\n> Avoiding null pointer dereference\n> \n> This pull request aims to fix null pointer dereference.\n> \n> Null pointer dereference\n> [https://cwe.mitre.org/data/definitions/476.html]\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-983%2Fklebertarcisio%2Fpatch-1-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-983/klebertarcisio/patch-1-v1\n> Pull-Request: https://github.com/git/git/pull/983\n>\n> builtin/submodule--helper.c | 2 ++\n> 1 file changed, 2 insertions(+)\n\nThanks, from my brief grepping of the remaining code in git.git there is\nno other malloc() that doesn't have its return value checked\nappropriately.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 9d505a6329c8..92349d715a78 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1215,6 +1215,8 @@ static void submodule_summary_callback(struct diff_queue_struct *q,\n> \t\tif (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode))\n> \t\t\tcontinue;\n> \t\ttemp = (struct module_cb*)malloc(sizeof(struct module_cb));\n> +\t\tif (!temp) \n> +\t\t\tdie(_(\"out of memory\"));\n> \t\ttemp->mod_src = p->one->mode;\n> \t\ttemp->mod_dst = p->two->mode;\n> \t\ttemp->oid_src = p->one->oid;\n\nWhen we just want to die if we can't allocate memory we should use the\nxmalloc() wrapper instead.\n"},{"id":"419859","messageId":"CAHd-oW6hOQ5Qim1xMOJ23kjcpo1M-K1dDvzGKYjBCD9XoT2G+A@mail.gmail.com","threadId":"55360","inReplyTo":"pull.983.git.git.1616323936790.gitgitgadget@gmail.com","subject":"Re: [PATCH] fix null pointer dereference","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-03-21T14:47:16Z","receivedAt":"2021-03-21T14:48:46Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Kleber. And welcome to the list!\n\nOn Sun, Mar 21, 2021 at 7:53 AM Kleber Tarcísio via GitGitGadget\n wrote:\n>\n> From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= \n>\n> The malloc function can return null when the memory allocation fails. This commit adds a condition to handle these cases properly. https://cwe.mitre.org/data/definitions/476.html\n\nIf you are going to re-roll this series, please wrap the commit\nmessage body at 72 columns. This helps viewing the message in\n80-columns terminals. (For more info on this and other commit message\nconventions used by the Git project, please take a look at the\ncorresponding sections at Documentation/MyFirstContribution.txt and\nDocumentation/SubmittingPatches).\n\nThanks,\nMatheus\n"},{"id":"419874","messageId":"xmqqft0oberu.fsf@gitster.g","threadId":"55360","inReplyTo":"pull.983.git.git.1616323936790.gitgitgadget@gmail.com","subject":"Re: [PATCH] fix null pointer dereference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-21T17:25:09Z","receivedAt":"2021-03-21T17:26:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kleber Tarcísio via GitGitGadget\" writes:\n\n> From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= \n> Subject: Re: [PATCH] fix null pointer dereference\n\nThanks, but see Documentation/SubmittingPatches for general guidelines.\nOur commit title begins with \":\" so that when this change is\nburied among hundreds of other commits in \"git shortlog --no-merges\",\nthe readers can tell what it is about.\n\n\tSubject: [PATCH] submodule-helper: avoid unchecked malloc()\n\nperhaps.\n\n> The malloc function can return null when the memory allocation fails. This commit adds a condition to handle these cases properly. https://cwe.mitre.org/data/definitions/476.html\n\nOverlong line. Also when you can say something without forcing\nreaders to refer to external material, do so (and if you do not\nthink you can, try harder ;-).\n\nIn this case, I think you do not need to say\nanything more than\n\n\tsubmodule-helper.c::submodule_summary_callback() calls\n\tmalloc() and uses the returned value without checking for\n\tNULLness. Use xmalloc() instaed.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 9d505a6329c8..92349d715a78 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1215,6 +1215,8 @@ static void submodule_summary_callback(struct diff_queue_struct *q,\n> \t\tif (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode))\n> \t\t\tcontinue;\n> \t\ttemp = (struct module_cb*)malloc(sizeof(struct module_cb));\n> +\t\tif (!temp) \n> +\t\t\tdie(_(\"out of memory\"));\n\nAnd this should be just\n\n- \t\ttemp = (struct module_cb*)malloc(sizeof(struct module_cb));\n+ \t\ttemp = (struct module_cb*)xmalloc(sizeof(struct module_cb));\n\nwithout any \"check and die\" on its own.\n\nNote that if this were a new code that adds a call to xmalloc(),\ncompetent reviewers would say it should be spelled more like so:\n\n \t\ttemp = xmalloc(sizeof(*temp));\n\nto lose unnecessary cast and to prepare for future evolution of the\ncode (e.g. the type of 'temp' may change from 'struct module_cb' to\nsomethng else). But for this \"malloc is wrong, use xmalloc instead\"\nfix, we do not mix such a code improvement in the same patch.\n\nThanks.\n\n"}]}