All of lore.kernel.org
 help / color / mirror / Atom feed
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");

             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.