Avoiding null pointer dereference

[klebertarcisio]

This pull request aims to fix null pointer dereference.

Null pointer dereference

cc: Ævar Arnfjörð Bjarmason [email protected] cc: Matheus Tavares Bernardino [email protected]

[gitgitgadget-git[bot]]

Welcome to GitGitGadget

Hi @klebertarcisio, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[gitgitgadget-git[bot]]

There is an issue in commit ab05693b5bcb2fe564cf48839870a71d3940a0ac: Commit checks stopped - the message is too short

[dscho]

/allow

[dscho]

There is an issue in commit ab05693: Commit checks stopped - the message is too short

@klebertarcisio please have a look at the existing (non-merge) commit messages, and imitate them.

[gitgitgadget-git[bot]]

User klebertarcisio is now allowed to use GitGitGadget.

WARNING: klebertarcisio has no public email address set on GitHub

[klebertarcisio]

@klebertarcisio please have a look at the existing (non-merge) commit messages, and imitate them.

Hi @dscho, thanks for the feedback. Now it's ok.

[dscho]

@klebertarcisio please have a look at the existing (non-merge) commit messages, and imitate them.

Hi @dscho, thanks for the feedback. Now it's ok.

Yep! Maybe even use the prefix submodule: ? And then /submit.

[klebertarcisio]

/submit

[gitgitgadget-git[bot]]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-git-983/klebertarcisio/patch-1-v1

To fetch this version to local tag pr-git-983/klebertarcisio/patch-1-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-git-983/klebertarcisio/patch-1-v1

[gitgitgadget-git[bot]]

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Sun, Mar 21 2021, Kleber Tarcísio via GitGitGadget wrote:

> From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= <[email protected]>
>
> 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 <[email protected]>
> ---
>     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.

[gitgitgadget-git[bot]]

User Ævar Arnfjörð Bjarmason <[email protected]> has been added to the cc: list.

[gitgitgadget-git[bot]]

On the Git mailing list, Matheus Tavares Bernardino wrote (reply to this):

Hi, Kleber. And welcome to the list!

On Sun, Mar 21, 2021 at 7:53 AM Kleber Tarcísio via GitGitGadget
<[email protected]> wrote:
>
> From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= <[email protected]>
>
> 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

[gitgitgadget-git[bot]]

User Matheus Tavares Bernardino <[email protected]> has been added to the cc: list.

[gitgitgadget-git[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Kleber Tarcísio via GitGitGadget"  <[email protected]> writes:

> From: =?UTF-8?q?Kleber=20Tarc=C3=ADsio?= <[email protected]>
> 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.

[klebertarcisio]

Hi everyone, thanks for the suggestions.

[dscho]

Hi everyone, thanks for the suggestions.

Please reply on the Git mailing list (see the "reply to this" links in the comments to which you want to reply).

[siseko1994]

Thank you for your response, let's work together on this, I don't want a froud with pleasure I'm professional to everyone,thing let's work hard for our future, null pointer to me make know sence, because I figure nulls to be public not private. null and fofo something, best way thinks must be legal ✊🇿🇦

On Mon, 22 Mar 2021, 11:51 Johannes Schindelin, @.***> wrote:

Hi everyone, thanks for the suggestions.

Please reply on the Git mailing list (see the "reply to this" links in the comments to which you want to reply).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/git/git/pull/983#issuecomment-803925910, or unsubscribe https://github.com/notifications/unsubscribe-auth/ATFLDWNLFSZU6IG3TNMI6K3TE4HLVANCNFSM4ZNHETNA .

[t0p0lino]

Ashoka

[dscho]

@binho1991 are you still interested in this?

[dscho]

/allow

[gitgitgadget-git[bot]]

User binho1991 is now allowed to use GitGitGadget.

WARNING: binho1991 has no public email address set on GitHub