From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Rubén Justo" <rjusto@gmail.com>
Subject: [PATCH 08/11] http: use git_config_string_dup()
Date: Sat, 6 Apr 2024 21:03:54 -0400 [thread overview]
Message-ID: <20240407010354.GH868358@coredump.intra.peff.net> (raw)
In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net>
There are many calls to git_config_string() in the HTTP code, leading to
possible leaks if a config variable is seen multiple times. I split
these out from the other "easy" cases because the change to non-const
pointers has some follow-on effects:
1. When these variables are passed to var_override(), that should now
take a non-const pointer (and consequently can drop the cast it
passes to free).
2. That file also has the somewhat redundant set_from_env() helper,
which did _not_ free the existing value, and so has basically the
same leak as git_config_string(). Rather than adjust it to take a
non-const pointer, we can just swap it out for var_override(),
fixing the leak and reducing the number of helpers at the same
time.
This makes for a fairly big patch, but we have to do it all at once to
keep the compiler happy (or alternatively, insert a bunch of temporary
casts into http.c).
Note that some of these are git_config_pathname_dup(); it seemed better
to convert them all together, since the root of the problem (and the
solution) are essentially the same.
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 105 +++++++++++++++++++++++++++------------------------------
1 file changed, 49 insertions(+), 56 deletions(-)
diff --git a/http.c b/http.c
index e73b136e58..f2bcc9083e 100644
--- a/http.c
+++ b/http.c
@@ -38,11 +38,11 @@ char curl_errorstr[CURL_ERROR_SIZE];
static int curl_ssl_verify = -1;
static int curl_ssl_try;
-static const char *curl_http_version = NULL;
-static const char *ssl_cert;
-static const char *ssl_cert_type;
-static const char *ssl_cipherlist;
-static const char *ssl_version;
+static char *curl_http_version;
+static char *ssl_cert;
+static char *ssl_cert_type;
+static char *ssl_cipherlist;
+static char *ssl_version;
static struct {
const char *name;
long ssl_version;
@@ -59,23 +59,23 @@ static struct {
{ "tlsv1.3", CURL_SSLVERSION_TLSv1_3 },
#endif
};
-static const char *ssl_key;
-static const char *ssl_key_type;
-static const char *ssl_capath;
-static const char *curl_no_proxy;
+static char *ssl_key;
+static char *ssl_key_type;
+static char *ssl_capath;
+static char *curl_no_proxy;
#ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY
-static const char *ssl_pinnedkey;
+static char *ssl_pinnedkey;
#endif
-static const char *ssl_cainfo;
+static char *ssl_cainfo;
static long curl_low_speed_limit = -1;
static long curl_low_speed_time = -1;
static int curl_ftp_no_epsv;
-static const char *curl_http_proxy;
-static const char *http_proxy_authmethod;
+static char *curl_http_proxy;
+static char *http_proxy_authmethod;
-static const char *http_proxy_ssl_cert;
-static const char *http_proxy_ssl_key;
-static const char *http_proxy_ssl_ca_info;
+static char *http_proxy_ssl_cert;
+static char *http_proxy_ssl_key;
+static char *http_proxy_ssl_ca_info;
static struct credential proxy_cert_auth = CREDENTIAL_INIT;
static int proxy_ssl_cert_password_required;
@@ -95,7 +95,7 @@ static struct {
*/
};
#ifdef CURLGSSAPI_DELEGATION_FLAG
-static const char *curl_deleg;
+static char *curl_deleg;
static struct {
const char *name;
long curl_deleg_param;
@@ -108,11 +108,11 @@ static struct {
static struct credential proxy_auth = CREDENTIAL_INIT;
static const char *curl_proxyuserpwd;
-static const char *curl_cookie_file;
+static char *curl_cookie_file;
static int curl_save_cookies;
struct credential http_auth = CREDENTIAL_INIT;
static int http_proactive_auth;
-static const char *user_agent;
+static char *user_agent;
static int curl_empty_auth = -1;
enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
@@ -366,28 +366,28 @@ static int http_options(const char *var, const char *value,
const struct config_context *ctx, void *data)
{
if (!strcmp("http.version", var)) {
- return git_config_string(&curl_http_version, var, value);
+ return git_config_string_dup(&curl_http_version, var, value);
}
if (!strcmp("http.sslverify", var)) {
curl_ssl_verify = git_config_bool(var, value);
return 0;
}
if (!strcmp("http.sslcipherlist", var))
- return git_config_string(&ssl_cipherlist, var, value);
+ return git_config_string_dup(&ssl_cipherlist, var, value);
if (!strcmp("http.sslversion", var))
- return git_config_string(&ssl_version, var, value);
+ return git_config_string_dup(&ssl_version, var, value);
if (!strcmp("http.sslcert", var))
- return git_config_pathname(&ssl_cert, var, value);
+ return git_config_pathname_dup(&ssl_cert, var, value);
if (!strcmp("http.sslcerttype", var))
- return git_config_string(&ssl_cert_type, var, value);
+ return git_config_string_dup(&ssl_cert_type, var, value);
if (!strcmp("http.sslkey", var))
- return git_config_pathname(&ssl_key, var, value);
+ return git_config_pathname_dup(&ssl_key, var, value);
if (!strcmp("http.sslkeytype", var))
- return git_config_string(&ssl_key_type, var, value);
+ return git_config_string_dup(&ssl_key_type, var, value);
if (!strcmp("http.sslcapath", var))
- return git_config_pathname(&ssl_capath, var, value);
+ return git_config_pathname_dup(&ssl_capath, var, value);
if (!strcmp("http.sslcainfo", var))
- return git_config_pathname(&ssl_cainfo, var, value);
+ return git_config_pathname_dup(&ssl_cainfo, var, value);
if (!strcmp("http.sslcertpasswordprotected", var)) {
ssl_cert_password_required = git_config_bool(var, value);
return 0;
@@ -436,27 +436,27 @@ static int http_options(const char *var, const char *value,
return 0;
}
if (!strcmp("http.proxy", var))
- return git_config_string(&curl_http_proxy, var, value);
+ return git_config_string_dup(&curl_http_proxy, var, value);
if (!strcmp("http.proxyauthmethod", var))
- return git_config_string(&http_proxy_authmethod, var, value);
+ return git_config_string_dup(&http_proxy_authmethod, var, value);
if (!strcmp("http.proxysslcert", var))
- return git_config_string(&http_proxy_ssl_cert, var, value);
+ return git_config_string_dup(&http_proxy_ssl_cert, var, value);
if (!strcmp("http.proxysslkey", var))
- return git_config_string(&http_proxy_ssl_key, var, value);
+ return git_config_string_dup(&http_proxy_ssl_key, var, value);
if (!strcmp("http.proxysslcainfo", var))
- return git_config_string(&http_proxy_ssl_ca_info, var, value);
+ return git_config_string_dup(&http_proxy_ssl_ca_info, var, value);
if (!strcmp("http.proxysslcertpasswordprotected", var)) {
proxy_ssl_cert_password_required = git_config_bool(var, value);
return 0;
}
if (!strcmp("http.cookiefile", var))
- return git_config_pathname(&curl_cookie_file, var, value);
+ return git_config_pathname_dup(&curl_cookie_file, var, value);
if (!strcmp("http.savecookies", var)) {
curl_save_cookies = git_config_bool(var, value);
return 0;
@@ -472,7 +472,7 @@ static int http_options(const char *var, const char *value,
}
if (!strcmp("http.useragent", var))
- return git_config_string(&user_agent, var, value);
+ return git_config_string_dup(&user_agent, var, value);
if (!strcmp("http.emptyauth", var)) {
if (value && !strcmp("auto", value))
@@ -484,7 +484,7 @@ static int http_options(const char *var, const char *value,
if (!strcmp("http.delegation", var)) {
#ifdef CURLGSSAPI_DELEGATION_FLAG
- return git_config_string(&curl_deleg, var, value);
+ return git_config_string_dup(&curl_deleg, var, value);
#else
warning(_("Delegation control is not supported with cURL < 7.22.0"));
return 0;
@@ -493,7 +493,7 @@ static int http_options(const char *var, const char *value,
if (!strcmp("http.pinnedpubkey", var)) {
#ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY
- return git_config_pathname(&ssl_pinnedkey, var, value);
+ return git_config_pathname_dup(&ssl_pinnedkey, var, value);
#else
warning(_("Public key pinning not supported with cURL < 7.39.0"));
return 0;
@@ -572,10 +572,10 @@ static void init_curl_http_auth(CURL *result)
}
/* *var must be free-able */
-static void var_override(const char **var, char *value)
+static void var_override(char **var, char *value)
{
if (value) {
- free((void *)*var);
+ free(*var);
*var = xstrdup(value);
}
}
@@ -1208,13 +1208,6 @@ static CURL *get_curl_handle(void)
return result;
}
-static void set_from_env(const char **var, const char *envname)
-{
- const char *val = getenv(envname);
- if (val)
- *var = val;
-}
-
void http_init(struct remote *remote, const char *url, int proactive_auth)
{
char *low_speed_limit;
@@ -1291,14 +1284,14 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
if (getenv("GIT_SSL_NO_VERIFY"))
curl_ssl_verify = 0;
- set_from_env(&ssl_cert, "GIT_SSL_CERT");
- set_from_env(&ssl_cert_type, "GIT_SSL_CERT_TYPE");
- set_from_env(&ssl_key, "GIT_SSL_KEY");
- set_from_env(&ssl_key_type, "GIT_SSL_KEY_TYPE");
- set_from_env(&ssl_capath, "GIT_SSL_CAPATH");
- set_from_env(&ssl_cainfo, "GIT_SSL_CAINFO");
+ var_override(&ssl_cert, getenv("GIT_SSL_CERT"));
+ var_override(&ssl_cert_type, getenv("GIT_SSL_CERT_TYPE"));
+ var_override(&ssl_key, getenv("GIT_SSL_KEY"));
+ var_override(&ssl_key_type, getenv("GIT_SSL_KEY_TYPE"));
+ var_override(&ssl_capath, getenv("GIT_SSL_CAPATH"));
+ var_override(&ssl_cainfo, getenv("GIT_SSL_CAINFO"));
- set_from_env(&user_agent, "GIT_HTTP_USER_AGENT");
+ var_override(&user_agent, getenv("GIT_HTTP_USER_AGENT"));
low_speed_limit = getenv("GIT_HTTP_LOW_SPEED_LIMIT");
if (low_speed_limit)
@@ -1314,9 +1307,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
if (max_requests < 1)
max_requests = DEFAULT_MAX_REQUESTS;
- set_from_env(&http_proxy_ssl_cert, "GIT_PROXY_SSL_CERT");
- set_from_env(&http_proxy_ssl_key, "GIT_PROXY_SSL_KEY");
- set_from_env(&http_proxy_ssl_ca_info, "GIT_PROXY_SSL_CAINFO");
+ var_override(&http_proxy_ssl_cert, getenv("GIT_PROXY_SSL_CERT"));
+ var_override(&http_proxy_ssl_key, getenv("GIT_PROXY_SSL_KEY"));
+ var_override(&http_proxy_ssl_ca_info, getenv("GIT_PROXY_SSL_CAINFO"));
if (getenv("GIT_PROXY_SSL_CERT_PASSWORD_PROTECTED"))
proxy_ssl_cert_password_required = 1;
--
2.44.0.872.g288abe5b5b
next prev parent reply other threads:[~2024-04-07 1:04 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-06 18:11 [PATCH] config: do not leak excludes_file Junio C Hamano
2024-04-06 19:17 ` [WIP] git_config_pathname() leakfix Junio C Hamano
2024-04-07 0:56 ` [PATCH 0/12] git_config_string() considered harmful Jeff King
2024-04-07 0:58 ` [PATCH 01/11] config: make sequencer.c's git_config_string_dup() public Jeff King
2024-04-07 1:00 ` [PATCH 02/11] config: add git_config_pathname_dup() Jeff King
2024-04-07 1:00 ` [PATCH 03/11] config: prefer git_config_string_dup() for temp variables Jeff King
2024-04-07 1:50 ` Eric Sunshine
2024-04-07 2:45 ` Jeff King
2024-04-07 1:01 ` [PATCH 04/11] config: use git_config_string_dup() for open-coded equivalents Jeff King
2024-04-07 1:01 ` [PATCH 05/11] config: use git_config_string_dup() to fix leaky open coding Jeff King
2024-04-07 1:02 ` [PATCH 06/11] config: use git_config_string() in easy cases Jeff King
2024-04-07 2:05 ` Eric Sunshine
2024-04-07 2:47 ` Jeff King
2024-04-07 1:03 ` [PATCH 07/11] config: use git_config_pathname_dup() " Jeff King
2024-04-07 1:03 ` Jeff King [this message]
2024-04-07 1:04 ` [PATCH 09/11] merge: use git_config_string_dup() for pull strategies Jeff King
2024-04-07 1:04 ` [PATCH 10/11] userdiff: use git_config_string_dup() when we can Jeff King
2024-04-07 1:07 ` [PATCH 11/11] blame: use "dup" string_list for ignore-revs files Jeff King
2024-04-07 2:42 ` Eric Sunshine
2024-04-07 1:08 ` [PATCH 0/12] git_config_string() considered harmful Jeff King
2024-04-07 17:58 ` Rubén Justo
2024-04-08 20:55 ` Jeff King
2024-04-11 23:36 ` Rubén Justo
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=20240407010354.GH868358@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=rjusto@gmail.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).