| Message ID | pull.983.git.git.1616323936790.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix null pointer dereference | expand |
On Sun, Mar 21 2021, Kleber Tarcísio via GitGitGadget wrote: > From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= <klebertarcisio@yahoo.com.br> > > 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 > > Signed-off-by: Kleber Tarcísio <klebertarcisio@yahoo.com.br> > --- > Avoiding null pointer dereference > > This pull request aims to fix null pointer dereference. > > Null pointer dereference > [https://cwe.mitre.org/data/definitions/476.html] > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-983%2Fklebertarcisio%2Fpatch-1-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-983/klebertarcisio/patch-1-v1 > Pull-Request: https://github.com/git/git/pull/983 > > builtin/submodule--helper.c | 2 ++ > 1 file changed, 2 insertions(+) Thanks, from my brief grepping of the remaining code in git.git there is no other malloc() that doesn't have its return value checked appropriately. > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 9d505a6329c8..92349d715a78 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1215,6 +1215,8 @@ static void submodule_summary_callback(struct diff_queue_struct *q, > if (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode)) > continue; > temp = (struct module_cb*)malloc(sizeof(struct module_cb)); > + if (!temp) > + die(_("out of memory")); > temp->mod_src = p->one->mode; > temp->mod_dst = p->two->mode; > temp->oid_src = p->one->oid; When we just want to die if we can't allocate memory we should use the xmalloc() wrapper instead.
Hi, Kleber. And welcome to the list! On Sun, Mar 21, 2021 at 7:53 AM Kleber Tarcísio via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= <klebertarcisio@yahoo.com.br> > > 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 If you are going to re-roll this series, please wrap the commit message body at 72 columns. This helps viewing the message in 80-columns terminals. (For more info on this and other commit message conventions used by the Git project, please take a look at the corresponding sections at Documentation/MyFirstContribution.txt and Documentation/SubmittingPatches). Thanks, Matheus
"Kleber Tarcísio via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= <klebertarcisio@yahoo.com.br> > Subject: Re: [PATCH] fix null pointer dereference Thanks, but see Documentation/SubmittingPatches for general guidelines. Our commit title begins with "<area>:" so that when this change is buried among hundreds of other commits in "git shortlog --no-merges", the readers can tell what it is about. Subject: [PATCH] submodule-helper: avoid unchecked malloc() perhaps. > 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 Overlong line. Also when you can say something without forcing readers to refer to external material, do so (and if you do not think you can, try harder ;-). In this case, I think you do not need to say anything more than submodule-helper.c::submodule_summary_callback() calls malloc() and uses the returned value without checking for NULLness. Use xmalloc() instaed. > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 9d505a6329c8..92349d715a78 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1215,6 +1215,8 @@ static void submodule_summary_callback(struct diff_queue_struct *q, > if (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode)) > continue; > temp = (struct module_cb*)malloc(sizeof(struct module_cb)); > + if (!temp) > + die(_("out of memory")); And this should be just - temp = (struct module_cb*)malloc(sizeof(struct module_cb)); + temp = (struct module_cb*)xmalloc(sizeof(struct module_cb)); without any "check and die" on its own. Note that if this were a new code that adds a call to xmalloc(), competent reviewers would say it should be spelled more like so: temp = xmalloc(sizeof(*temp)); to lose unnecessary cast and to prepare for future evolution of the code (e.g. the type of 'temp' may change from 'struct module_cb' to somethng else). But for this "malloc is wrong, use xmalloc instead" fix, we do not mix such a code improvement in the same patch. Thanks.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 9d505a6329c8..92349d715a78 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1215,6 +1215,8 @@ static void submodule_summary_callback(struct diff_queue_struct *q, if (!S_ISGITLINK(p->one->mode) && !S_ISGITLINK(p->two->mode)) continue; temp = (struct module_cb*)malloc(sizeof(struct module_cb)); + if (!temp) + die(_("out of memory")); temp->mod_src = p->one->mode; temp->mod_dst = p->two->mode; temp->oid_src = p->one->oid;