From: Derrick Stolee <derrickstolee@github.com>
To: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCH] clone: plug a miniscule leak
Date: Mon, 2 May 2022 09:43:05 -0400 [thread overview]
Message-ID: <5e8eff95-d768-439d-e2df-d575a0038ffd@github.com> (raw)
In-Reply-To: <xmqqlevl4ysk.fsf@gitster.g>
On 5/1/2022 1:17 AM, Junio C Hamano wrote:
> Perhaps a Coccinelle rule like this might have caught similar
> leaks:
>
> @@
> expression E;
> expression V;
> @@
> - if (E)
> - V = xstrdup(E);
> + if (E) {
> + free(V);
> + V = xstrdup(E);
> + }
>
> The fact that the result of xstrdup() is assigned to V is that V
> is meant to hold a pointer to an allocated piece of memory.
>
> With the preimage of the above semantic patch, it is reasonable
> to expect that V may be initialized to NULL or may be holding a
> pointer to a piece of allocated memory when the control reaches
> here, because otherwise, V will be either need to be freed (when
> E was not NULL, in which case we assigned the result of
> xstrdup() to it) or V has garbage that cannot be freed later.
Initially, I did think "what if V is not initialized to NULL?" but
you are right that the code would already be broken in that case.
> - if (option_origin != NULL)
This technically wouldn't hit your rule, since "E" isn't just the
variable name, as we typically do with our style. Is that something
that Coccinelle automatically simplifies?
> + if (option_origin != NULL) {
Do you want to take this opportunity to drop the "!= NULL" here?
> + free(remote_name);
> remote_name = xstrdup(option_origin);
> + }
> > if (remote_name == NULL)
Or do you want to keep similar style from the surrounding code?
Either way, looks good to me.
Thanks,
-Stolee
next prev parent reply other threads:[~2022-05-02 13:43 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-01 5:17 [PATCH] clone: plug a miniscule leak Junio C Hamano
2022-05-02 13:43 ` Derrick Stolee [this message]
2022-05-02 17:12 ` Junio C Hamano
2022-05-02 20:39 ` Philip Oakley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5e8eff95-d768-439d-e2df-d575a0038ffd@github.com \
--to=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).