From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Subject: [PATCH] clone: plug a miniscule leak
Date: Sat, 30 Apr 2022 22:17:15 -0700 [thread overview]
Message-ID: <xmqqlevl4ysk.fsf@gitster.g> (raw)
The remote_name variable is first assigned a copy of the value of
the "clone.defaultremotename" configuration variable and then by the
value of the "--origin" command line option. The former is prepared
to see multiple instances of the variable by freeing the current
value of the variable before a copy of the newly discovered value
gets assigned to it. The latter blindly assigned a copy of the new
value to it, thereby leaking the value read from the configuration.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
This was discovered by a recently removed bogus coccinelle
rewrite rule---if you apply an incorrect change suggested by the
bogus rewrite rule to unconditionally assign to remote_name a
copy of option_origin, or NULL, the value read from the
configuration would be lost and never be used, thereby breaking
a test to ensure the configuration is used, instead of the
default remote nickname "origin".
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.
builtin/clone.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git c/builtin/clone.c w/builtin/clone.c
index 5231656379..194d50f75f 100644
--- c/builtin/clone.c
+++ w/builtin/clone.c
@@ -1106,8 +1106,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
* apply the remote name provided by --origin only after this second
* call to git_config, to ensure it overrides all config-based values.
*/
- if (option_origin != NULL)
+ if (option_origin != NULL) {
+ free(remote_name);
remote_name = xstrdup(option_origin);
+ }
if (remote_name == NULL)
remote_name = xstrdup("origin");
next reply other threads:[~2022-05-01 5:17 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-01 5:17 Junio C Hamano [this message]
2022-05-02 13:43 ` [PATCH] clone: plug a miniscule leak Derrick Stolee
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqlevl4ysk.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
/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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.