git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Clemens Buchacher <drizzd@aon.at>, git@vger.kernel.org
Subject: [PATCH 1/2] http: clean up leak in init_curl_http_auth
Date: Fri, 13 Apr 2012 02:18:35 -0400	[thread overview]
Message-ID: <20120413061835.GA13690@sigill.intra.peff.net> (raw)
In-Reply-To: <20120413061622.GA27591@sigill.intra.peff.net>

When we have a credential to give to curl, we must copy it
into a "user:pass" buffer and then hand the buffer to curl.
Old versions of curl did not copy the buffer, and we were
expected to keep it valid. Newer versions of curl will copy
the buffer.

Our solution was to use a strbuf and detach it, giving
ownership of the resulting buffer to curl. However, this
meant that we were leaking the buffer on newer versions of
curl, since curl was just copying it and throwing away the
string we passed. Furthermore, when we replaced a
credential (e.g., because our original one was rejected), we
were also leaking on both old and new versions of curl.

This got even worse in the last patch, which started
replacing the credential (and thus leaking) on every http
request.

Instead, let's use a static buffer to make the ownership
more clear and less leaky.  We already keep a static "struct
credential", so we are only handling a single credential at
a time, anyway.

Signed-off-by: Jeff King <peff@peff.net>
---
The "old" version was prior to 7.17.0, from 2007. That is probably still
new enough to care about. But at some point we should probably make a
sweep through http.c and get rid of conditional blocks for really old
cruft.

 http.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index c6dc9b7..eaf7f40 100644
--- a/http.c
+++ b/http.c
@@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb)
 static void init_curl_http_auth(CURL *result)
 {
 	if (http_auth.username) {
-		struct strbuf up = STRBUF_INIT;
+		static struct strbuf up = STRBUF_INIT;
 		credential_fill(&http_auth);
+		strbuf_reset(&up);
 		strbuf_addf(&up, "%s:%s",
 			    http_auth.username, http_auth.password);
-		curl_easy_setopt(result, CURLOPT_USERPWD,
-				 strbuf_detach(&up, NULL));
+		curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
 	}
 }
 
-- 
1.7.9.6.6.g6b3b56

  reply	other threads:[~2012-04-13  6:18 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-04-01 18:48 Cannot clone the git repository shared over http with authorization Artur R. Czechowski
2012-04-01 19:45 ` Clemens Buchacher
2012-04-01 20:53   ` Clemens Buchacher
2012-04-02  8:31   ` Jeff King
2012-04-10  9:53     ` Clemens Buchacher
2012-04-10  9:53       ` [PATCH 1/2] http auth fails with multiple curl handles Clemens Buchacher
2012-04-10  9:53       ` [PATCH 2/2] fix http auth " Clemens Buchacher
2012-04-12  7:09         ` Jeff King
2012-04-12 22:50           ` Junio C Hamano
2012-04-13  6:16             ` Jeff King
2012-04-13  6:18               ` Jeff King [this message]
2012-04-13  6:19               ` [PATCH 2/2] http: use newer curl options for setting credentials Jeff King

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=20120413061835.GA13690@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=drizzd@aon.at \
    --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.
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 NNTP newsgroup(s).