From patchwork Sun Apr 7 00:58:31 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619953 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 10939812 for ; Sun, 7 Apr 2024 00:58:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451515; cv=none; b=hhvryCmlYBG+W0Dv4KfZ9iVhSaHirCfa9zAZo8U248VXMaqwlhI+E5F07vrzeodv7BGnjxor+SfoRlL7wJa0igCKFQyDkuTcwey26unEy9CxSHQyyJwTMm/arHJctVxp5qadSmCSvLuurPxKCFdQZG/1uCDEzy8wTcEIJKCDTSA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451515; c=relaxed/simple; bh=rqVGd55dbd4HmjfQh20bnVInRKG1qk+lmsHWL5zDcHg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Dk/sDzI7RMaol7hlQmvGy0X/sI72Ilj8Q8GoK1ID1ZGqxYOxs1zHkowg9S7qAYgamaUM+5DhWxmirU8Gs5AwB8vEqPYoRxQQN3Z+OOwELvsQKXc5rWIhn0mDQZVfC+6X5PUe2KXY47fxJFCwmf9tB6SzMkUAEhX052BEz+HBGW0= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7267 invoked by uid 109); 7 Apr 2024 00:58:32 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 00:58:32 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11184 invoked by uid 111); 7 Apr 2024 00:58:34 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 20:58:34 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 20:58:31 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 01/11] config: make sequencer.c's git_config_string_dup() public Message-ID: <20240407005831.GA868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> Just about every caller of git_config_string() has a possible leak in it: if we parse a config variable twice, it will overwrite the pointer that was allocated the first time, leaving the memory unreferenced. Unfortunately we can't just fix this directly in git_config_string(). Some callers do something like: const char *foo = "default_value"; ... git_config_string(&foo, var, value); And we must _not_ free that initial value, as it's a string literal. We can't help those cases easily, as there's no way to distinguish a heap-allocated variable from the default one. But let's start by at least providing a helper that avoids the leak. That will let us convert some cases right away, and give us one potential path forward for the more complex ones. It turns out we already have such a helper, courtesy of 03a4e260e2 (sequencer: plug memory leaks for the option values, 2016-10-21). The problem is more acute in sequencer.c, which may load config multiple times. Hence the solution was limited to that file back then. But this really is a more general problem within our config callbacks. Note that the new helper takes a "char *" rather than a const pointer. This is more appropriate, since we tend to use "const" as a signal for a lack of memory ownership (and this function is most certainly asserting ownership over the pointed-to memory). Signed-off-by: Jeff King --- config.c | 9 +++++++++ config.h | 12 ++++++++++++ sequencer.c | 10 ---------- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/config.c b/config.c index eebce8c7e0..2194fb078a 100644 --- a/config.c +++ b/config.c @@ -1345,6 +1345,15 @@ int git_config_string(const char **dest, const char *var, const char *value) return 0; } +int git_config_string_dup(char **dest, const char *var, const char *value) +{ + if (!value) + return config_error_nonbool(var); + free(*dest); + *dest = xstrdup(value); + return 0; +} + int git_config_pathname(const char **dest, const char *var, const char *value) { if (!value) diff --git a/config.h b/config.h index f4966e3749..cdffc14ccf 100644 --- a/config.h +++ b/config.h @@ -279,9 +279,21 @@ int git_config_bool(const char *, const char *); /** * Allocates and copies the value string into the `dest` parameter; if no * string is given, prints an error message and returns -1. + * + * Note that this function does _not_ free the memory referenced by the + * destination pointer. This makes it safe to use on a variable that initially + * points to a string literal, but it also means that it leaks if the config + * option is seen multiple times. */ int git_config_string(const char **, const char *, const char *); +/** + * Like git_config_string(), but frees any previously-allocated + * string at the destination pointer, avoiding a leak when a + * config variable is seen multiple times. + */ +int git_config_string_dup(char **, const char *, const char *); + /** * Similar to `git_config_string`, but expands `~` or `~user` into the * user's home directory when found at the beginning of the path. diff --git a/sequencer.c b/sequencer.c index 2c19846385..3e5d82e0e5 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2920,16 +2920,6 @@ static int read_populate_todo(struct repository *r, return 0; } -static int git_config_string_dup(char **dest, - const char *var, const char *value) -{ - if (!value) - return config_error_nonbool(var); - free(*dest); - *dest = xstrdup(value); - return 0; -} - static int populate_opts_cb(const char *key, const char *value, const struct config_context *ctx, void *data) From patchwork Sun Apr 7 01:00:08 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619954 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F284D812 for ; Sun, 7 Apr 2024 01:00:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451611; cv=none; b=lRNgIxru1dJhkPbrQi1Izg3U3exUWN5x1uX8c5DMPVZ2hXAeullItC3cam/LQ0wLH1pcwY+lB4l/PUjVN52bEluLVRCuZobok6ZclB5pBZ8k8u6/KhFElzC6lDVrvCv53laYCGdt2bxi+/ubOuMDeDLDgqSKHHm7B40o1V/omW8= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451611; c=relaxed/simple; bh=WBpHAygscHfE50QgOD73xaOZMJa4Ks3/K8YNPNb4Ijw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=B8YwAryHkhtYQisNInB9+/LLbxtnsVS6WGCO8qXYQwagTf0dLdLHm5JD/OgSgL3K7b2gtB2U5/onJecf/Zm1onqDq9OecZNmXsongfFVxtncjOtIJs9rr0NdkRDUI15an3nWIH0RL4eGcB8FAzjYjeWOYUYOW9pjyIHndSM5jXM= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7286 invoked by uid 109); 7 Apr 2024 01:00:09 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:00:09 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11225 invoked by uid 111); 7 Apr 2024 01:00:11 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:00:11 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:00:08 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 02/11] config: add git_config_pathname_dup() Message-ID: <20240407010008.GB868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> The git_config_pathname() function suffers the same potential leak issue as git_config_string(), since it is basically the same thing but with the added twist of interpolating the path rather than just duplicating the value. Let's provide a similar "dup()" variant to help call sites transition to using the leak-free variant. Signed-off-by: Jeff King --- config.c | 11 +++++++++++ config.h | 7 +++++++ 2 files changed, 18 insertions(+) diff --git a/config.c b/config.c index 2194fb078a..a0aa45abd5 100644 --- a/config.c +++ b/config.c @@ -1364,6 +1364,17 @@ int git_config_pathname(const char **dest, const char *var, const char *value) return 0; } +int git_config_pathname_dup(char **dest, const char *var, const char *value) +{ + if (!value) + return config_error_nonbool(var); + free(*dest); + *dest = interpolate_path(value, 0); + if (!*dest) + die(_("failed to expand user dir in: '%s'"), value); + return 0; +} + int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value) { if (!value) diff --git a/config.h b/config.h index cdffc14ccf..fed21d3144 100644 --- a/config.h +++ b/config.h @@ -300,6 +300,13 @@ int git_config_string_dup(char **, const char *, const char *); */ int git_config_pathname(const char **, const char *, const char *); +/** + * Like git_config_pathname(), but frees any previously-allocated + * string at the destination pointer, avoiding a leak when a + * config variable is seen multiple times. + */ +int git_config_pathname_dup(char **, const char *, const char *); + int git_config_expiry_date(timestamp_t *, const char *, const char *); int git_config_color(char *, const char *, const char *); int git_config_set_in_file_gently(const char *, const char *, const char *, const char *); From patchwork Sun Apr 7 01:00:37 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619955 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4F26E136A for ; Sun, 7 Apr 2024 01:00:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451641; cv=none; b=JIOUOnN0MHXWhZgMQUH4A7HysGsaWE+HHYFtPZRWQBhfyJjtiyUEa3PUQCPSCpMeHoxN74tE4yCp9UbWQL6NA3N41d0/tN39wWHEWIxaxfNQAKSzjTpKIp1i45M2wriQHn29yCAbtFh77wOb6Q8W70HQa/qLVCmw74clHg8vu2U= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451641; c=relaxed/simple; bh=/shAywjN5x3VZfZXlm6XVRQRrJ7iaJukjKg8JavxzOY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=b2dZmOHPKO7TSfDo+OSrcE+scN1D4KlzcVj3BVO3oxeNkJtPGPvo4aCgIo4M9McI4uMyoS3FBg9x3bRig7OTnJCGiPhi046LrZffjoKvnK6oJ7UCxg3zgIwnW1AEUnUNri1cboWGEPzkvY7I4vhd2r1HK3DugQKuDsgdj7R9sxo= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7297 invoked by uid 109); 7 Apr 2024 01:00:38 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:00:38 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11230 invoked by uid 111); 7 Apr 2024 01:00:41 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:00:41 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:00:37 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 03/11] config: prefer git_config_string_dup() for temp variables Message-ID: <20240407010037.GC868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> In some cases we may use git_config_string() or git_config_pathname() to read a value into a temporary variable, and then either hand off memory ownership of the new variable or free it. These are not potential leaks, since we know that there is no previous value we are overwriting. However, it's worth converting these to use git_config_string_dup() and git_config_pathname_dup(). It makes it easier to audit for leaky cases, and possibly we can get rid of the leak-prone functions in the future. Plus it lets the const-ness of our variables match their expected memory ownership, which avoids some casts when calling free(). Signed-off-by: Jeff King --- builtin/blame.c | 4 ++-- builtin/config.c | 6 +++--- builtin/receive-pack.c | 6 +++--- fetch-pack.c | 6 +++--- fsck.c | 6 +++--- remote.c | 28 ++++++++++++++-------------- setup.c | 6 +++--- 7 files changed, 31 insertions(+), 31 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index db1f56de61..0b07ceb110 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -718,10 +718,10 @@ static int git_blame_config(const char *var, const char *value, return 0; } if (!strcmp(var, "blame.ignorerevsfile")) { - const char *str; + char *str = NULL; int ret; - ret = git_config_pathname(&str, var, value); + ret = git_config_pathname_dup(&str, var, value); if (ret) return ret; string_list_insert(&ignore_revs_file_list, str); diff --git a/builtin/config.c b/builtin/config.c index 0015620dde..fc5d96bb4c 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -282,11 +282,11 @@ static int format_config(struct strbuf *buf, const char *key_, else strbuf_addstr(buf, v ? "true" : "false"); } else if (type == TYPE_PATH) { - const char *v; - if (git_config_pathname(&v, key_, value_) < 0) + char *v = NULL; + if (git_config_pathname_dup(&v, key_, value_) < 0) return -1; strbuf_addstr(buf, v); - free((char *)v); + free(v); } else if (type == TYPE_EXPIRY_DATE) { timestamp_t t; if (git_config_expiry_date(&t, key_, value_) < 0) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 56d8a77ed7..15ed81a3f6 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -168,13 +168,13 @@ static int receive_pack_config(const char *var, const char *value, } if (strcmp(var, "receive.fsck.skiplist") == 0) { - const char *path; + char *path = NULL; - if (git_config_pathname(&path, var, value)) + if (git_config_pathname_dup(&path, var, value)) return 1; strbuf_addf(&fsck_msg_types, "%cskiplist=%s", fsck_msg_types.len ? ',' : '=', path); - free((char *)path); + free(path); return 0; } diff --git a/fetch-pack.c b/fetch-pack.c index 091f9a80a9..fd59c497b4 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1863,13 +1863,13 @@ static int fetch_pack_config_cb(const char *var, const char *value, const char *msg_id; if (strcmp(var, "fetch.fsck.skiplist") == 0) { - const char *path; + char *path = NULL; - if (git_config_pathname(&path, var, value)) + if (git_config_pathname_dup(&path, var, value)) return 1; strbuf_addf(&fsck_msg_types, "%cskiplist=%s", fsck_msg_types.len ? ',' : '=', path); - free((char *)path); + free(path); return 0; } diff --git a/fsck.c b/fsck.c index 78af29d264..a9d27a660f 100644 --- a/fsck.c +++ b/fsck.c @@ -1274,13 +1274,13 @@ int git_fsck_config(const char *var, const char *value, const char *msg_id; if (strcmp(var, "fsck.skiplist") == 0) { - const char *path; + char *path = NULL; struct strbuf sb = STRBUF_INIT; - if (git_config_pathname(&path, var, value)) + if (git_config_pathname_dup(&path, var, value)) return 1; strbuf_addf(&sb, "skiplist=%s", path); - free((char *)path); + free(path); fsck_set_msg_types(options, sb.buf); strbuf_release(&sb); return 0; diff --git a/remote.c b/remote.c index 2b650b813b..09912bebf1 100644 --- a/remote.c +++ b/remote.c @@ -428,38 +428,38 @@ static int handle_config(const char *key, const char *value, else if (!strcmp(subkey, "prunetags")) remote->prune_tags = git_config_bool(key, value); else if (!strcmp(subkey, "url")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; add_url(remote, v); } else if (!strcmp(subkey, "pushurl")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; add_pushurl(remote, v); } else if (!strcmp(subkey, "push")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; refspec_append(&remote->push, v); - free((char *)v); + free(v); } else if (!strcmp(subkey, "fetch")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; refspec_append(&remote->fetch, v); - free((char *)v); + free(v); } else if (!strcmp(subkey, "receivepack")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; if (!remote->receivepack) remote->receivepack = v; else error(_("more than one receivepack given, using the first")); } else if (!strcmp(subkey, "uploadpack")) { - const char *v; - if (git_config_string(&v, key, value)) + char *v = NULL; + if (git_config_string_dup(&v, key, value)) return -1; if (!remote->uploadpack) remote->uploadpack = v; diff --git a/setup.c b/setup.c index f4b32f76e3..9f35a27978 100644 --- a/setup.c +++ b/setup.c @@ -1176,13 +1176,13 @@ static int safe_directory_cb(const char *key, const char *value, } else if (!strcmp(value, "*")) { data->is_safe = 1; } else { - const char *interpolated = NULL; + char *interpolated = NULL; - if (!git_config_pathname(&interpolated, key, value) && + if (!git_config_pathname_dup(&interpolated, key, value) && !fspathcmp(data->path, interpolated ? interpolated : value)) data->is_safe = 1; - free((char *)interpolated); + free(interpolated); } return 0; From patchwork Sun Apr 7 01:01:00 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619956 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BB7A11849 for ; Sun, 7 Apr 2024 01:01:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451663; cv=none; b=UIEMXbOrvhVSH8ro02UNtlsmR4jtUC6I4iyMovIrwbFcBpSh01grCHQB3K6mqpLwdXOzRxC0q+za81FOSH7ad+iGnS6LawVUWmSkQPfcDleLLfTw2xKa+6v3Ue7l/uYNelQJ/ZeKyG6D1sGQFgV29931E0/e6mZfACxxNVQMxFk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451663; c=relaxed/simple; bh=Leanzy9Bn6oeMEdNGetzQUHj6oNhvgRC5Mpbb6EWb2M=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=kHh9+hk4HueMpoiTUv4taSbu6EwPcQvDIDQjDYfrlwmUI1nvkvCERiy8fmw2vcEGrQF1HC/BeTKeuVS/zdExBK8KRUWbnGkUpNJyybePn+hF/r9MIMKWxv40PjBQ+qSasYwpx0MAtQfKl1S8o2I9NqeYRynlAKd28dd6XKT+6M0= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7309 invoked by uid 109); 7 Apr 2024 01:01:01 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:01:01 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11235 invoked by uid 111); 7 Apr 2024 01:01:03 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:01:03 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:01:00 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 04/11] config: use git_config_string_dup() for open-coded equivalents Message-ID: <20240407010100.GD868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> These are cases where the calling code does the exact same thing git_config_string_dup() would do. We can shorten the code a bit by using it. Note in the final case that we rely on leaving the if-else chain to return "0" for success, and now we'll return more directly. The two are equivalent. Signed-off-by: Jeff King --- archive-tar.c | 10 +++------- compat/mingw.c | 7 ++----- setup.c | 5 +---- 3 files changed, 6 insertions(+), 16 deletions(-) diff --git a/archive-tar.c b/archive-tar.c index 8ae30125f8..6da7101553 100644 --- a/archive-tar.c +++ b/archive-tar.c @@ -393,13 +393,9 @@ static int tar_filter_config(const char *var, const char *value, tar_filters[nr_tar_filters++] = ar; } - if (!strcmp(type, "command")) { - if (!value) - return config_error_nonbool(var); - free(ar->filter_command); - ar->filter_command = xstrdup(value); - return 0; - } + if (!strcmp(type, "command")) + return git_config_string_dup(&ar->filter_command, var, value); + if (!strcmp(type, "remote")) { if (git_config_bool(var, value)) ar->flags |= ARCHIVER_REMOTE; diff --git a/compat/mingw.c b/compat/mingw.c index 320fb99a90..aeccb3957f 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -255,11 +255,8 @@ int mingw_core_config(const char *var, const char *value, } if (!strcmp(var, "core.unsetenvvars")) { - if (!value) - return config_error_nonbool(var); - free(unset_environment_variables); - unset_environment_variables = xstrdup(value); - return 0; + return git_config_string_dup(&unset_environment_variables, var, + value); } if (!strcmp(var, "core.restrictinheritedhandles")) { diff --git a/setup.c b/setup.c index 9f35a27978..7204fd2815 100644 --- a/setup.c +++ b/setup.c @@ -529,10 +529,7 @@ static int read_worktree_config(const char *var, const char *value, if (strcmp(var, "core.bare") == 0) { data->is_bare = git_config_bool(var, value); } else if (strcmp(var, "core.worktree") == 0) { - if (!value) - return config_error_nonbool(var); - free(data->work_tree); - data->work_tree = xstrdup(value); + return git_config_string_dup(&data->work_tree, var, value); } return 0; } From patchwork Sun Apr 7 01:01:43 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619957 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 871045223 for ; Sun, 7 Apr 2024 01:01:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451706; cv=none; b=hUK/J4ky2K90fUb3Woa7hM98CqikBW7RXNsT/r3tPxwOak7BhTIFGgEeWGZvupS2dPD0sC6F/U/Z4xGC1Zqu1Z5ERILKpj9GxQnRYEQzsNb9RfX6IhzbDS5sZaUxrKcfeyXOlgGvs2a4H8glEK62yPJ0jsxKLDK7ZCAZAH6+TF8= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451706; c=relaxed/simple; bh=5DqckvpAG1SJYEPPA8KAZl/rocT08yu1ypZRk8AZe0I=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=tsU78x7yLtU5bGACZ6ODUn04KIzJuJXHWcN3iObg+O2Tg5yBhQ/Y+00LX1yca6IuCeqN1k5lRROHwJq6BK59cC57g6RQDAYKjPku/rA3tENxNnAGVGbUFXiu+uGwS0pGd2wan1ozOPDgOMND4RoHg4UjQAs6FwSXK60QXwiKjdA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7319 invoked by uid 109); 7 Apr 2024 01:01:44 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:01:44 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11243 invoked by uid 111); 7 Apr 2024 01:01:46 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:01:46 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:01:43 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 05/11] config: use git_config_string_dup() to fix leaky open coding Message-ID: <20240407010143.GE868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> These are cases which open-code the equivalent of git_config_string(), but end up leaking the resulting value if the config value is seen multiple times (because they never free an existing value). Using git_config_string_dup() plug these potential leaks. Signed-off-by: Jeff King --- builtin/help.c | 7 ++----- config.c | 8 ++------ merge-ll.c | 5 +---- 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/builtin/help.c b/builtin/help.c index dc1fbe2b98..1bdd2faee0 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -52,7 +52,7 @@ static enum help_action { HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, } cmd_mode; -static const char *html_path; +static char *html_path; static int verbose = 1; static enum help_format help_format = HELP_FORMAT_NONE; static int exclude_guides; @@ -407,10 +407,7 @@ static int git_help_config(const char *var, const char *value, return 0; } if (!strcmp(var, "help.htmlpath")) { - if (!value) - return config_error_nonbool(var); - html_path = xstrdup(value); - return 0; + return git_config_string_dup(&html_path, var, value); } if (!strcmp(var, "man.viewer")) { if (!value) diff --git a/config.c b/config.c index a0aa45abd5..c115e6d8c9 100644 --- a/config.c +++ b/config.c @@ -1575,12 +1575,8 @@ static int git_default_core_config(const char *var, const char *value, if (!strcmp(var, "core.checkroundtripencoding")) return git_config_string(&check_roundtrip_encoding, var, value); - if (!strcmp(var, "core.notesref")) { - if (!value) - return config_error_nonbool(var); - notes_ref_name = xstrdup(value); - return 0; - } + if (!strcmp(var, "core.notesref")) + return git_config_string_dup(¬es_ref_name, var, value); if (!strcmp(var, "core.editor")) return git_config_string(&editor_program, var, value); diff --git a/merge-ll.c b/merge-ll.c index bf1077ae09..660d9a3bd6 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -308,8 +308,6 @@ static int read_merge_config(const char *var, const char *value, return git_config_string(&fn->description, var, value); if (!strcmp("driver", key)) { - if (!value) - return config_error_nonbool(var); /* * merge..driver specifies the command line: * @@ -333,8 +331,7 @@ static int read_merge_config(const char *var, const char *value, * file named by %A, and signal that it has done with zero exit * status. */ - fn->cmdline = xstrdup(value); - return 0; + return git_config_string_dup(&fn->cmdline, var, value); } if (!strcmp("recursive", key)) From patchwork Sun Apr 7 01:02:28 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619958 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CB34C7460 for ; Sun, 7 Apr 2024 01:02:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451752; cv=none; b=SKGhXmgxyHHuAn5eHaohXNPm1BezVPoMmtwXd6xUDdHMfAPL12qC+HYzTC5FdCE1fIJZoYgbMAY3q6OsMQwGfiIKYA+s57dsATqaDOTZaLxBiT5a3TUb2OdJ4nmyAuwysysP4H5mdKz6VAOrsY5jEDoQpWcHPcmpV7NM+VVgLP4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451752; c=relaxed/simple; bh=5YbYvAXMYVIY6sW5j16GCC0AUX/+YbeRxeH1LwIFsA0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=MXSpKzt4l8T4JfS8MR7VbweIeVEfd+azQr3uvY2ISA3AmcIeZNyJEW2G6xM/dhLOTT6Wy5xUwyOtWdLZ4HSAqf2shlcIyp40NcJ0iq7wXsAJ+CdkhbrpteEkokqeKrlRYBXnCoVfY9v17HJXwCH6M1kPm6/2Sv1vWhBw0YJy1A8= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7334 invoked by uid 109); 7 Apr 2024 01:02:29 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:02:29 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11250 invoked by uid 111); 7 Apr 2024 01:02:31 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:02:31 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:02:28 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 06/11] config: use git_config_string() in easy cases Message-ID: <20240407010228.GF868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> The git_config_string() interface is very prone to leaking; it overwrites the contents of the pointer without freeing any existing value, so: const char *foo; ... if (!strcmp(var, "foo.bar")) git_config_string(&foo, var, value); is always going to leak if we see the "foo.bar" config more than once. We don't tend to notice in practice because people don't have a lot of redundant config, but basically every use of git_config_string() has this problem. For simple cases where the initial value of the "foo" pointer is NULL (like the one above), we can easily convert them to use a variant of the helper that frees the existing value before writing over it. That works because free(NULL) is a noop. What we can't change, though, is cases where the pointer is initialized to a string literal, like: const char *foo = "some string literal"; In that case it would be incorrect to free() the original value. Those cases will take more refactoring to make them leak-free, and this patch punts on that for now. In each case we switch the underlying variable from "const char *" to "char *" to indicate that it will always point to allocated memory (and our git_config_string_dup() interface enforces this). Signed-off-by: Jeff King --- alias.c | 3 +-- attr.c | 2 +- attr.h | 2 +- builtin/commit.c | 4 ++-- builtin/log.c | 12 ++++++------ builtin/merge.c | 4 ++-- builtin/receive-pack.c | 4 ++-- builtin/repack.c | 16 ++++++++-------- config.c | 12 ++++++------ convert.c | 12 ++++++------ delta-islands.c | 4 ++-- diff.c | 8 ++++---- environment.c | 8 ++++---- environment.h | 8 ++++---- gpg-interface.c | 5 +++-- imap-send.c | 20 ++++++++++---------- mailmap.c | 2 +- mailmap.h | 2 +- merge-ll.c | 12 ++++++------ pager.c | 4 ++-- promisor-remote.c | 2 +- promisor-remote.h | 2 +- remote.c | 17 ++++++++--------- remote.h | 8 ++++---- sequencer.c | 2 +- upload-pack.c | 4 ++-- 26 files changed, 89 insertions(+), 90 deletions(-) diff --git a/alias.c b/alias.c index 5a238f2e30..7f8c1c3e7d 100644 --- a/alias.c +++ b/alias.c @@ -22,8 +22,7 @@ static int config_alias_cb(const char *key, const char *value, if (data->alias) { if (!strcasecmp(p, data->alias)) - return git_config_string((const char **)&data->v, - key, value); + return git_config_string_dup(&data->v, key, value); } else if (data->list) { string_list_append(data->list, p); } diff --git a/attr.c b/attr.c index 679e42258c..4eda36c865 100644 --- a/attr.c +++ b/attr.c @@ -25,7 +25,7 @@ #include "tree-walk.h" #include "object-name.h" -const char *git_attr_tree; +char *git_attr_tree; const char git_attr__true[] = "(builtin)true"; const char git_attr__false[] = "\0(builtin)false"; diff --git a/attr.h b/attr.h index 127998ae01..e7cc318b0c 100644 --- a/attr.h +++ b/attr.h @@ -236,6 +236,6 @@ const char *git_attr_global_file(void); /* Return whether the system gitattributes file is enabled and should be used. */ int git_attr_system_is_enabled(void); -extern const char *git_attr_tree; +extern char *git_attr_tree; #endif /* ATTR_H */ diff --git a/builtin/commit.c b/builtin/commit.c index 7ba7201cfb..9b6546d401 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -133,7 +133,7 @@ static struct strvec trailer_args = STRVEC_INIT; * is specified explicitly. */ static enum commit_msg_cleanup_mode cleanup_mode; -static const char *cleanup_arg; +static char *cleanup_arg; static enum commit_whence whence; static int use_editor = 1, include_status = 1; @@ -1632,7 +1632,7 @@ static int git_commit_config(const char *k, const char *v, return 0; } if (!strcmp(k, "commit.cleanup")) - return git_config_string(&cleanup_arg, k, v); + return git_config_string_dup(&cleanup_arg, k, v); if (!strcmp(k, "commit.gpgsign")) { sign_commit = git_config_bool(k, v) ? "" : NULL; return 0; diff --git a/builtin/log.c b/builtin/log.c index c0a8bb95e9..eb336f7efb 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -49,7 +49,7 @@ #define FORMAT_PATCH_NAME_MAX_DEFAULT 64 /* Set a default date-time format for git log ("log.date" config variable) */ -static const char *default_date_mode = NULL; +static char *default_date_mode; static int default_abbrev_commit; static int default_show_root = 1; @@ -63,7 +63,7 @@ static unsigned int force_in_body_from; static int stdout_mboxrd; static const char *fmt_patch_subject_prefix = "PATCH"; static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT; -static const char *fmt_pretty; +static char *fmt_pretty; static int format_no_prefix; static const char * const builtin_log_usage[] = { @@ -569,7 +569,7 @@ static int git_log_config(const char *var, const char *value, const char *slot_name; if (!strcmp(var, "format.pretty")) - return git_config_string(&fmt_pretty, var, value); + return git_config_string_dup(&fmt_pretty, var, value); if (!strcmp(var, "format.subjectprefix")) return git_config_string(&fmt_patch_subject_prefix, var, value); if (!strcmp(var, "format.filenamemaxlength")) { @@ -585,7 +585,7 @@ static int git_log_config(const char *var, const char *value, return 0; } if (!strcmp(var, "log.date")) - return git_config_string(&default_date_mode, var, value); + return git_config_string_dup(&default_date_mode, var, value); if (!strcmp(var, "log.decorate")) { decoration_style = parse_decoration_style(value); if (decoration_style < 0) @@ -959,7 +959,7 @@ static char *from; static const char *signature = git_version_string; static const char *signature_file; static enum cover_setting config_cover_letter; -static const char *config_output_directory; +static char *config_output_directory; static enum cover_from_description cover_from_description_mode = COVER_FROM_MESSAGE; static int show_notes; static struct display_notes_opt notes_opt; @@ -1054,7 +1054,7 @@ static int git_format_config(const char *var, const char *value, return 0; } if (!strcmp(var, "format.outputdirectory")) - return git_config_string(&config_output_directory, var, value); + return git_config_string_dup(&config_output_directory, var, value); if (!strcmp(var, "format.useautobase")) { if (value && !strcasecmp(value, "whenAble")) { auto_base = AUTO_BASE_WHEN_ABLE; diff --git a/builtin/merge.c b/builtin/merge.c index 6f4fec87fc..c2be29ed2f 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -111,7 +111,7 @@ enum ff_type { static enum ff_type fast_forward = FF_ALLOW; -static const char *cleanup_arg; +static char *cleanup_arg; static enum commit_msg_cleanup_mode cleanup_mode; static int option_parse_message(const struct option *opt, @@ -619,7 +619,7 @@ static int git_merge_config(const char *k, const char *v, else if (!strcmp(k, "pull.octopus")) return git_config_string(&pull_octopus, k, v); else if (!strcmp(k, "commit.cleanup")) - return git_config_string(&cleanup_arg, k, v); + return git_config_string_dup(&cleanup_arg, k, v); else if (!strcmp(k, "merge.ff")) { int boolval = git_parse_maybe_bool(v); if (0 <= boolval) { diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 15ed81a3f6..2ba71b6673 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -88,7 +88,7 @@ static struct strbuf push_cert = STRBUF_INIT; static struct object_id push_cert_oid; static struct signature_check sigcheck; static const char *push_cert_nonce; -static const char *cert_nonce_seed; +static char *cert_nonce_seed; static struct strvec hidden_refs = STRVEC_INIT; static const char *NONCE_UNSOLICITED = "UNSOLICITED"; @@ -230,7 +230,7 @@ static int receive_pack_config(const char *var, const char *value, } if (strcmp(var, "receive.certnonceseed") == 0) - return git_config_string(&cert_nonce_seed, var, value); + return git_config_string_dup(&cert_nonce_seed, var, value); if (strcmp(var, "receive.certnonceslop") == 0) { nonce_stamp_slop_limit = git_config_ulong(var, value, ctx->kvi); diff --git a/builtin/repack.c b/builtin/repack.c index 15e4cccc45..c144e18d9f 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -48,10 +48,10 @@ static const char incremental_bitmap_conflict_error[] = N_( ); struct pack_objects_args { - const char *window; - const char *window_memory; - const char *depth; - const char *threads; + char *window; + char *window_memory; + char *depth; + char *threads; unsigned long max_pack_size; int no_reuse_delta; int no_reuse_object; @@ -86,13 +86,13 @@ static int repack_config(const char *var, const char *value, return 0; } if (!strcmp(var, "repack.cruftwindow")) - return git_config_string(&cruft_po_args->window, var, value); + return git_config_string_dup(&cruft_po_args->window, var, value); if (!strcmp(var, "repack.cruftwindowmemory")) - return git_config_string(&cruft_po_args->window_memory, var, value); + return git_config_string_dup(&cruft_po_args->window_memory, var, value); if (!strcmp(var, "repack.cruftdepth")) - return git_config_string(&cruft_po_args->depth, var, value); + return git_config_string_dup(&cruft_po_args->depth, var, value); if (!strcmp(var, "repack.cruftthreads")) - return git_config_string(&cruft_po_args->threads, var, value); + return git_config_string_dup(&cruft_po_args->threads, var, value); return git_default_config(var, value, ctx, cb); } diff --git a/config.c b/config.c index c115e6d8c9..449b8f4f66 100644 --- a/config.c +++ b/config.c @@ -1579,7 +1579,7 @@ static int git_default_core_config(const char *var, const char *value, return git_config_string_dup(¬es_ref_name, var, value); if (!strcmp(var, "core.editor")) - return git_config_string(&editor_program, var, value); + return git_config_string_dup(&editor_program, var, value); if (!strcmp(var, "core.commentchar") || !strcmp(var, "core.commentstring")) { @@ -1598,7 +1598,7 @@ static int git_default_core_config(const char *var, const char *value, } if (!strcmp(var, "core.askpass")) - return git_config_string(&askpass_program, var, value); + return git_config_string_dup(&askpass_program, var, value); if (!strcmp(var, "core.excludesfile")) return git_config_pathname(&excludes_file, var, value); @@ -1703,10 +1703,10 @@ static int git_default_sparse_config(const char *var, const char *value) static int git_default_i18n_config(const char *var, const char *value) { if (!strcmp(var, "i18n.commitencoding")) - return git_config_string(&git_commit_encoding, var, value); + return git_config_string_dup(&git_commit_encoding, var, value); if (!strcmp(var, "i18n.logoutputencoding")) - return git_config_string(&git_log_output_encoding, var, value); + return git_config_string_dup(&git_log_output_encoding, var, value); /* Add other config variables here and to Documentation/config.txt. */ return 0; @@ -1782,7 +1782,7 @@ static int git_default_mailmap_config(const char *var, const char *value) if (!strcmp(var, "mailmap.file")) return git_config_pathname(&git_mailmap_file, var, value); if (!strcmp(var, "mailmap.blob")) - return git_config_string(&git_mailmap_blob, var, value); + return git_config_string_dup(&git_mailmap_blob, var, value); /* Add other config variables here and to Documentation/config.txt. */ return 0; @@ -1791,7 +1791,7 @@ static int git_default_mailmap_config(const char *var, const char *value) static int git_default_attr_config(const char *var, const char *value) { if (!strcmp(var, "attr.tree")) - return git_config_string(&git_attr_tree, var, value); + return git_config_string_dup(&git_attr_tree, var, value); /* * Add other attribute related config variables here and to diff --git a/convert.c b/convert.c index 35b25eb3cb..d462485d1f 100644 --- a/convert.c +++ b/convert.c @@ -979,9 +979,9 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_p static struct convert_driver { const char *name; struct convert_driver *next; - const char *smudge; - const char *clean; - const char *process; + char *smudge; + char *clean; + char *process; int required; } *user_convert, **user_convert_tail; @@ -1047,13 +1047,13 @@ static int read_convert_config(const char *var, const char *value, */ if (!strcmp("smudge", key)) - return git_config_string(&drv->smudge, var, value); + return git_config_string_dup(&drv->smudge, var, value); if (!strcmp("clean", key)) - return git_config_string(&drv->clean, var, value); + return git_config_string_dup(&drv->clean, var, value); if (!strcmp("process", key)) - return git_config_string(&drv->process, var, value); + return git_config_string_dup(&drv->process, var, value); if (!strcmp("required", key)) { drv->required = git_config_bool(var, value); diff --git a/delta-islands.c b/delta-islands.c index f7e079425f..c8ff736a4d 100644 --- a/delta-islands.c +++ b/delta-islands.c @@ -313,7 +313,7 @@ struct island_load_data { size_t nr; size_t alloc; }; -static const char *core_island_name; +static char *core_island_name; static void free_config_regexes(struct island_load_data *ild) { @@ -362,7 +362,7 @@ static int island_config_callback(const char *k, const char *v, } if (!strcmp(k, "pack.islandcore")) - return git_config_string(&core_island_name, k, v); + return git_config_string_dup(&core_island_name, k, v); return 0; } diff --git a/diff.c b/diff.c index 108c187577..bb00bc1110 100644 --- a/diff.c +++ b/diff.c @@ -56,8 +56,8 @@ static int diff_color_moved_default; static int diff_color_moved_ws_default; static int diff_context_default = 3; static int diff_interhunk_context_default; -static const char *diff_word_regex_cfg; -static const char *external_diff_cmd_cfg; +static char *diff_word_regex_cfg; +static char *external_diff_cmd_cfg; static const char *diff_order_file_cfg; int diff_auto_refresh_index = 1; static int diff_mnemonic_prefix; @@ -429,9 +429,9 @@ int git_diff_ui_config(const char *var, const char *value, return 0; } if (!strcmp(var, "diff.external")) - return git_config_string(&external_diff_cmd_cfg, var, value); + return git_config_string_dup(&external_diff_cmd_cfg, var, value); if (!strcmp(var, "diff.wordregex")) - return git_config_string(&diff_word_regex_cfg, var, value); + return git_config_string_dup(&diff_word_regex_cfg, var, value); if (!strcmp(var, "diff.orderfile")) return git_config_pathname(&diff_order_file_cfg, var, value); diff --git a/environment.c b/environment.c index a73ba9c12c..5e9fab4d1a 100644 --- a/environment.c +++ b/environment.c @@ -42,8 +42,8 @@ int is_bare_repository_cfg = -1; /* unspecified */ int warn_ambiguous_refs = 1; int warn_on_object_refname_ambiguity = 1; int repository_format_precious_objects; -const char *git_commit_encoding; -const char *git_log_output_encoding; +char *git_commit_encoding; +char *git_log_output_encoding; char *apply_default_whitespace; char *apply_default_ignorewhitespace; const char *git_attributes_file; @@ -58,8 +58,8 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; size_t delta_base_cache_limit = 96 * 1024 * 1024; unsigned long big_file_threshold = 512 * 1024 * 1024; -const char *editor_program; -const char *askpass_program; +char *editor_program; +char *askpass_program; const char *excludes_file; enum auto_crlf auto_crlf = AUTO_CRLF_FALSE; enum eol core_eol = EOL_UNSET; diff --git a/environment.h b/environment.h index 05fd94d7be..f06024a457 100644 --- a/environment.h +++ b/environment.h @@ -217,11 +217,11 @@ int odb_pack_keep(const char *name); const char *get_log_output_encoding(void); const char *get_commit_output_encoding(void); -extern const char *git_commit_encoding; -extern const char *git_log_output_encoding; +extern char *git_commit_encoding; +extern char *git_log_output_encoding; -extern const char *editor_program; -extern const char *askpass_program; +extern char *editor_program; +extern char *askpass_program; extern const char *excludes_file; /* diff --git a/gpg-interface.c b/gpg-interface.c index b5993385ff..4b2f70ef44 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -27,7 +27,8 @@ static void gpg_interface_lazy_init(void) } static char *configured_signing_key; -static const char *ssh_default_key_command, *ssh_allowed_signers, *ssh_revocation_file; +static char *ssh_default_key_command; +static const char *ssh_allowed_signers, *ssh_revocation_file; static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED; struct gpg_format { @@ -762,7 +763,7 @@ static int git_gpg_config(const char *var, const char *value, } if (!strcmp(var, "gpg.ssh.defaultkeycommand")) - return git_config_string(&ssh_default_key_command, var, value); + return git_config_string_dup(&ssh_default_key_command, var, value); if (!strcmp(var, "gpg.ssh.allowedsignersfile")) return git_config_pathname(&ssh_allowed_signers, var, value); diff --git a/imap-send.c b/imap-send.c index f2e1947e63..d4454a3b49 100644 --- a/imap-send.c +++ b/imap-send.c @@ -87,16 +87,16 @@ static int nfvasprintf(char **strp, const char *fmt, va_list ap) struct imap_server_conf { const char *name; - const char *tunnel; + char *tunnel; const char *host; int port; - const char *folder; - const char *user; - const char *pass; + char *folder; + char *user; + char *pass; int use_ssl; int ssl_verify; int use_html; - const char *auth_method; + char *auth_method; }; static struct imap_server_conf server = { @@ -1332,15 +1332,15 @@ static int git_imap_config(const char *var, const char *val, else if (!strcmp("imap.preformattedhtml", var)) server.use_html = git_config_bool(var, val); else if (!strcmp("imap.folder", var)) - return git_config_string(&server.folder, var, val); + return git_config_string_dup(&server.folder, var, val); else if (!strcmp("imap.user", var)) - return git_config_string(&server.user, var, val); + return git_config_string_dup(&server.user, var, val); else if (!strcmp("imap.pass", var)) - return git_config_string(&server.pass, var, val); + return git_config_string_dup(&server.pass, var, val); else if (!strcmp("imap.tunnel", var)) - return git_config_string(&server.tunnel, var, val); + return git_config_string_dup(&server.tunnel, var, val); else if (!strcmp("imap.authmethod", var)) - return git_config_string(&server.auth_method, var, val); + return git_config_string_dup(&server.auth_method, var, val); else if (!strcmp("imap.port", var)) server.port = git_config_int(var, val, ctx->kvi); else if (!strcmp("imap.host", var)) { diff --git a/mailmap.c b/mailmap.c index 3d6a5e9400..9bd11c90e6 100644 --- a/mailmap.c +++ b/mailmap.c @@ -7,7 +7,7 @@ #include "setup.h" const char *git_mailmap_file; -const char *git_mailmap_blob; +char *git_mailmap_blob; struct mailmap_info { char *name; diff --git a/mailmap.h b/mailmap.h index 0f8fd2c586..a1bc019b52 100644 --- a/mailmap.h +++ b/mailmap.h @@ -4,7 +4,7 @@ struct string_list; extern const char *git_mailmap_file; -extern const char *git_mailmap_blob; +extern char *git_mailmap_blob; int read_mailmap(struct string_list *map); void clear_mailmap(struct string_list *map); diff --git a/merge-ll.c b/merge-ll.c index 660d9a3bd6..5ef96309d8 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -27,9 +27,9 @@ typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *, struct ll_merge_driver { const char *name; - const char *description; + char *description; ll_merge_fn fn; - const char *recursive; + char *recursive; struct ll_merge_driver *next; char *cmdline; }; @@ -268,7 +268,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, * merge.default and merge.driver configuration items */ static struct ll_merge_driver *ll_user_merge, **ll_user_merge_tail; -static const char *default_ll_merge; +static char *default_ll_merge; static int read_merge_config(const char *var, const char *value, const struct config_context *ctx UNUSED, @@ -279,7 +279,7 @@ static int read_merge_config(const char *var, const char *value, size_t namelen; if (!strcmp(var, "merge.default")) - return git_config_string(&default_ll_merge, var, value); + return git_config_string_dup(&default_ll_merge, var, value); /* * We are not interested in anything but "merge..variable"; @@ -305,7 +305,7 @@ static int read_merge_config(const char *var, const char *value, } if (!strcmp("name", key)) - return git_config_string(&fn->description, var, value); + return git_config_string_dup(&fn->description, var, value); if (!strcmp("driver", key)) { /* @@ -335,7 +335,7 @@ static int read_merge_config(const char *var, const char *value, } if (!strcmp("recursive", key)) - return git_config_string(&fn->recursive, var, value); + return git_config_string_dup(&fn->recursive, var, value); return 0; } diff --git a/pager.c b/pager.c index b8822a9381..6d75528e56 100644 --- a/pager.c +++ b/pager.c @@ -13,7 +13,7 @@ int pager_use_color = 1; #endif static struct child_process pager_process; -static const char *pager_program; +static char *pager_program; /* Is the value coming back from term_columns() just a guess? */ static int term_columns_guessed; @@ -47,7 +47,7 @@ static int core_pager_config(const char *var, const char *value, void *data UNUSED) { if (!strcmp(var, "core.pager")) - return git_config_string(&pager_program, var, value); + return git_config_string_dup(&pager_program, var, value); return 0; } diff --git a/promisor-remote.c b/promisor-remote.c index ac3aa1e365..65693e7931 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -139,7 +139,7 @@ static int promisor_remote_config(const char *var, const char *value, if (!r) return 0; - return git_config_string(&r->partial_clone_filter, var, value); + return git_config_string_dup(&r->partial_clone_filter, var, value); } return 0; diff --git a/promisor-remote.h b/promisor-remote.h index 2cb9eda9ea..88cb599c39 100644 --- a/promisor-remote.h +++ b/promisor-remote.h @@ -13,7 +13,7 @@ struct object_id; */ struct promisor_remote { struct promisor_remote *next; - const char *partial_clone_filter; + char *partial_clone_filter; const char name[FLEX_ARRAY]; }; diff --git a/remote.c b/remote.c index 09912bebf1..761a8b8fe3 100644 --- a/remote.c +++ b/remote.c @@ -367,9 +367,9 @@ static int handle_config(const char *key, const char *value, return -1; branch = make_branch(remote_state, name, namelen); if (!strcmp(subkey, "remote")) { - return git_config_string(&branch->remote_name, key, value); + return git_config_string_dup(&branch->remote_name, key, value); } else if (!strcmp(subkey, "pushremote")) { - return git_config_string(&branch->pushremote_name, key, value); + return git_config_string_dup(&branch->pushremote_name, key, value); } else if (!strcmp(subkey, "merge")) { if (!value) return config_error_nonbool(key); @@ -401,8 +401,8 @@ static int handle_config(const char *key, const char *value, /* Handle remote.* variables */ if (!name && !strcmp(subkey, "pushdefault")) - return git_config_string(&remote_state->pushremote_name, key, - value); + return git_config_string_dup(&remote_state->pushremote_name, key, + value); if (!name) return 0; @@ -471,13 +471,12 @@ static int handle_config(const char *key, const char *value, else if (!strcmp(value, "--tags")) remote->fetch_tags = 2; } else if (!strcmp(subkey, "proxy")) { - return git_config_string((const char **)&remote->http_proxy, - key, value); + return git_config_string_dup(&remote->http_proxy, key, value); } else if (!strcmp(subkey, "proxyauthmethod")) { - return git_config_string((const char **)&remote->http_proxy_authmethod, - key, value); + return git_config_string_dup(&remote->http_proxy_authmethod, + key, value); } else if (!strcmp(subkey, "vcs")) { - return git_config_string(&remote->foreign_vcs, key, value); + return git_config_string_dup(&remote->foreign_vcs, key, value); } return 0; } diff --git a/remote.h b/remote.h index dfd4837e60..e8c6655e42 100644 --- a/remote.h +++ b/remote.h @@ -46,7 +46,7 @@ struct remote_state { struct hashmap branches_hash; struct branch *current_branch; - const char *pushremote_name; + char *pushremote_name; struct rewrites rewrites; struct rewrites rewrites_push; @@ -65,7 +65,7 @@ struct remote { int origin, configured_in_repo; - const char *foreign_vcs; + char *foreign_vcs; /* An array of all of the url_nr URLs configured for the remote */ const char **url; @@ -309,9 +309,9 @@ struct branch { const char *refname; /* The name of the remote listed in the configuration. */ - const char *remote_name; + char *remote_name; - const char *pushremote_name; + char *pushremote_name; /* An array of the "merge" lines in the configuration. */ const char **merge_name; diff --git a/sequencer.c b/sequencer.c index 3e5d82e0e5..29f019b2a0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -266,7 +266,7 @@ static int git_sequencer_config(const char *k, const char *v, } if (!opts->default_strategy && !strcmp(k, "pull.twohead")) { - int ret = git_config_string((const char**)&opts->default_strategy, k, v); + int ret = git_config_string_dup(&opts->default_strategy, k, v); if (ret == 0) { /* * pull.twohead is allowed to be multi-valued; we only diff --git a/upload-pack.c b/upload-pack.c index 902144b9d3..5f5943cc19 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -94,7 +94,7 @@ struct upload_pack_data { struct packet_writer writer; - const char *pack_objects_hook; + char *pack_objects_hook; unsigned stateless_rpc : 1; /* v0 only */ unsigned no_done : 1; /* v0 only */ @@ -1386,7 +1386,7 @@ static int upload_pack_protected_config(const char *var, const char *value, struct upload_pack_data *data = cb_data; if (!strcmp("uploadpack.packobjectshook", var)) - return git_config_string(&data->pack_objects_hook, var, value); + return git_config_string_dup(&data->pack_objects_hook, var, value); return 0; } From patchwork Sun Apr 7 01:03:01 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619959 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 20A7A846C for ; Sun, 7 Apr 2024 01:03:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451784; cv=none; b=Z+Cg+YQE8ahPGNDOr89ThLkHbCUBvcDFB/bBRR8uVcQOfVV3TieRtddAdELV2nBiwFqR9E6Jcy0+61L39zT2Vda/Kb043EbMeMiihlQM46LHyU0qbaQiwY620Mk9+Z/mCjHFKKpmL5C2zc9roM41JG2AjsooLefLtBAVusQO2CA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451784; c=relaxed/simple; bh=furYD7rJjb5L1dTjoCFf484t83zght39Jeg3a5fqPHg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=JMwo4vMfHOzuHfPjzdgT5RANm9AVFJ9gft+xdHn5OOIg4b+7pgaqXH8J8kFiALDgM22Uj/TpQ72nTI+zZwVqcBmY5B16o/nOsnchECHTfUgNs6uW/NvihPu6lbyNYEBfBMv0fJwaaNS+r4b2a5P1wDKnTHtGBP5E0F9OvG0nsEY= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7347 invoked by uid 109); 7 Apr 2024 01:03:02 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:03:02 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11256 invoked by uid 111); 7 Apr 2024 01:03:05 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:03:05 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:03:01 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 07/11] config: use git_config_pathname_dup() in easy cases Message-ID: <20240407010301.GG868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> As with the previous commit for git_config_string(), this fixes simple cases where calling git_config_pathname() may leak if a config variable is seen multiple times. Signed-off-by: Jeff King --- I mostly split this out because the diff is big, but maybe it would make sense squashed to share the rationale? builtin/commit.c | 4 ++-- builtin/log.c | 4 ++-- config.c | 8 ++++---- diff.c | 4 ++-- environment.c | 6 +++--- environment.h | 6 +++--- gpg-interface.c | 6 +++--- mailmap.c | 2 +- mailmap.h | 2 +- 9 files changed, 21 insertions(+), 21 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 9b6546d401..693175e5c4 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -107,7 +107,7 @@ static enum { } commit_style; static const char *logfile, *force_author; -static const char *template_file; +static char *template_file; /* * The _message variables are commit names from which to take * the commit message and/or authorship. @@ -1626,7 +1626,7 @@ static int git_commit_config(const char *k, const char *v, struct wt_status *s = cb; if (!strcmp(k, "commit.template")) - return git_config_pathname(&template_file, k, v); + return git_config_pathname_dup(&template_file, k, v); if (!strcmp(k, "commit.status")) { include_status = git_config_bool(k, v); return 0; diff --git a/builtin/log.c b/builtin/log.c index eb336f7efb..3df261f94a 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -957,7 +957,7 @@ static int do_signoff; static enum auto_base_setting auto_base; static char *from; static const char *signature = git_version_string; -static const char *signature_file; +static char *signature_file; static enum cover_setting config_cover_letter; static char *config_output_directory; static enum cover_from_description cover_from_description_mode = COVER_FROM_MESSAGE; @@ -1044,7 +1044,7 @@ static int git_format_config(const char *var, const char *value, if (!strcmp(var, "format.signature")) return git_config_string(&signature, var, value); if (!strcmp(var, "format.signaturefile")) - return git_config_pathname(&signature_file, var, value); + return git_config_pathname_dup(&signature_file, var, value); if (!strcmp(var, "format.coverletter")) { if (value && !strcasecmp(value, "auto")) { config_cover_letter = COVER_AUTO; diff --git a/config.c b/config.c index 449b8f4f66..c2863d6514 100644 --- a/config.c +++ b/config.c @@ -1434,10 +1434,10 @@ static int git_default_core_config(const char *var, const char *value, } if (!strcmp(var, "core.attributesfile")) - return git_config_pathname(&git_attributes_file, var, value); + return git_config_pathname_dup(&git_attributes_file, var, value); if (!strcmp(var, "core.hookspath")) - return git_config_pathname(&git_hooks_path, var, value); + return git_config_pathname_dup(&git_hooks_path, var, value); if (!strcmp(var, "core.bare")) { is_bare_repository_cfg = git_config_bool(var, value); @@ -1601,7 +1601,7 @@ static int git_default_core_config(const char *var, const char *value, return git_config_string_dup(&askpass_program, var, value); if (!strcmp(var, "core.excludesfile")) - return git_config_pathname(&excludes_file, var, value); + return git_config_pathname_dup(&excludes_file, var, value); if (!strcmp(var, "core.whitespace")) { if (!value) @@ -1780,7 +1780,7 @@ static int git_default_push_config(const char *var, const char *value) static int git_default_mailmap_config(const char *var, const char *value) { if (!strcmp(var, "mailmap.file")) - return git_config_pathname(&git_mailmap_file, var, value); + return git_config_pathname_dup(&git_mailmap_file, var, value); if (!strcmp(var, "mailmap.blob")) return git_config_string_dup(&git_mailmap_blob, var, value); diff --git a/diff.c b/diff.c index bb00bc1110..6236746920 100644 --- a/diff.c +++ b/diff.c @@ -58,7 +58,7 @@ static int diff_context_default = 3; static int diff_interhunk_context_default; static char *diff_word_regex_cfg; static char *external_diff_cmd_cfg; -static const char *diff_order_file_cfg; +static char *diff_order_file_cfg; int diff_auto_refresh_index = 1; static int diff_mnemonic_prefix; static int diff_no_prefix; @@ -433,7 +433,7 @@ int git_diff_ui_config(const char *var, const char *value, if (!strcmp(var, "diff.wordregex")) return git_config_string_dup(&diff_word_regex_cfg, var, value); if (!strcmp(var, "diff.orderfile")) - return git_config_pathname(&diff_order_file_cfg, var, value); + return git_config_pathname_dup(&diff_order_file_cfg, var, value); if (!strcmp(var, "diff.ignoresubmodules")) { if (!value) diff --git a/environment.c b/environment.c index 5e9fab4d1a..87123f902c 100644 --- a/environment.c +++ b/environment.c @@ -46,8 +46,8 @@ char *git_commit_encoding; char *git_log_output_encoding; char *apply_default_whitespace; char *apply_default_ignorewhitespace; -const char *git_attributes_file; -const char *git_hooks_path; +char *git_attributes_file; +char *git_hooks_path; int zlib_compression_level = Z_BEST_SPEED; int pack_compression_level = Z_DEFAULT_COMPRESSION; int fsync_object_files = -1; @@ -60,7 +60,7 @@ size_t delta_base_cache_limit = 96 * 1024 * 1024; unsigned long big_file_threshold = 512 * 1024 * 1024; char *editor_program; char *askpass_program; -const char *excludes_file; +char *excludes_file; enum auto_crlf auto_crlf = AUTO_CRLF_FALSE; enum eol core_eol = EOL_UNSET; int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN; diff --git a/environment.h b/environment.h index f06024a457..c33f101b24 100644 --- a/environment.h +++ b/environment.h @@ -124,8 +124,8 @@ extern int warn_ambiguous_refs; extern int warn_on_object_refname_ambiguity; extern char *apply_default_whitespace; extern char *apply_default_ignorewhitespace; -extern const char *git_attributes_file; -extern const char *git_hooks_path; +extern char *git_attributes_file; +extern char *git_hooks_path; extern int zlib_compression_level; extern int pack_compression_level; extern size_t packed_git_window_size; @@ -222,7 +222,7 @@ extern char *git_log_output_encoding; extern char *editor_program; extern char *askpass_program; -extern const char *excludes_file; +extern char *excludes_file; /* * Should we print an ellipsis after an abbreviated SHA-1 value diff --git a/gpg-interface.c b/gpg-interface.c index 4b2f70ef44..d6e45ff09c 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -28,7 +28,7 @@ static void gpg_interface_lazy_init(void) static char *configured_signing_key; static char *ssh_default_key_command; -static const char *ssh_allowed_signers, *ssh_revocation_file; +static char *ssh_allowed_signers, *ssh_revocation_file; static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED; struct gpg_format { @@ -766,10 +766,10 @@ static int git_gpg_config(const char *var, const char *value, return git_config_string_dup(&ssh_default_key_command, var, value); if (!strcmp(var, "gpg.ssh.allowedsignersfile")) - return git_config_pathname(&ssh_allowed_signers, var, value); + return git_config_pathname_dup(&ssh_allowed_signers, var, value); if (!strcmp(var, "gpg.ssh.revocationfile")) - return git_config_pathname(&ssh_revocation_file, var, value); + return git_config_pathname_dup(&ssh_revocation_file, var, value); if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program")) fmtname = "openpgp"; diff --git a/mailmap.c b/mailmap.c index 9bd11c90e6..b2efe29b3d 100644 --- a/mailmap.c +++ b/mailmap.c @@ -6,7 +6,7 @@ #include "object-store-ll.h" #include "setup.h" -const char *git_mailmap_file; +char *git_mailmap_file; char *git_mailmap_blob; struct mailmap_info { diff --git a/mailmap.h b/mailmap.h index a1bc019b52..cbda9bc5e0 100644 --- a/mailmap.h +++ b/mailmap.h @@ -3,7 +3,7 @@ struct string_list; -extern const char *git_mailmap_file; +extern char *git_mailmap_file; extern char *git_mailmap_blob; int read_mailmap(struct string_list *map); From patchwork Sun Apr 7 01:03:54 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619960 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D012CA92E for ; Sun, 7 Apr 2024 01:03:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451837; cv=none; b=smfB0GKIB3I9Z4mC7yqNnvWNMzkXDNQ4diHw4cKL84KRByIC2Dgjs+ZwLpIs99rXuUgaCmy7sYjNVosBN2L/XOJ9uSue9TDSbZf2VBSqk5/yZDxznlNr6teYV3yht4yKkcp7r8baXCJ5lbnanGm4Rk5WSEAQaYR5OK69y5LpQyY= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451837; c=relaxed/simple; bh=PRj748hYNPeWkCq/E3EbeUP1FiL3fObn2emJ+y4CqMY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=aTvD3zCDl5dv1WksCHVZbJG2XZ1nLnypTuQ2Ldp5f+EF1ZbkgoF7MZqcBWUglfnnf5NpdLr5xPV04OU9M/h+yUtXjNCtXx9pPhoOO/1BQmmmpZP+95qAeVnNNH2q5ezgFTHAl62jiZWoXfctLs5qphTJE6oVQ2XI1nlXeQSNIMQ= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7367 invoked by uid 109); 7 Apr 2024 01:03:55 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:03:55 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11261 invoked by uid 111); 7 Apr 2024 01:03:57 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:03:57 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:03:54 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 08/11] http: use git_config_string_dup() Message-ID: <20240407010354.GH868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline 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 --- 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; From patchwork Sun Apr 7 01:04:29 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619961 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 886B610A0A for ; Sun, 7 Apr 2024 01:04:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451873; cv=none; b=Ec/YrF1/XlvEwpXrZI3hoTIdsOO7InDLnXWAqyqa7XuZonBdxFc8y910ZMbP6mCdt7b50lKdv+bIViuSMf6CgxxmVWYCQ2tCChzfNJvHjSARiXQR9GaAT55rV8itrD6LTOO7Vm+m3IRYcpiue7O92qd6oFm7LgVHWJ1dfC8DgRk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451873; c=relaxed/simple; bh=aOK5oyMRCpazZ630LJsRrdQ/ErzdHFBSOSpkp8vb+NE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=sCNfE8W+1/BO8pXDu2+eAha8Jx+NMbw7N44H4bddTYmrzW6bf8A1sHuq9kLgAdmwukvYgXgwtRxK44HUcAb81jIvFWCpCfCxk2LxAXVWRe1Lh4C/238m6nEl8hwm/0rcJ1cJ/OgUVqCmFnl2nsdDLkoPV5dGlw6Tt6PVwcHpNUI= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7381 invoked by uid 109); 7 Apr 2024 01:04:30 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:04:30 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11266 invoked by uid 111); 7 Apr 2024 01:04:32 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:04:32 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:04:29 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 09/11] merge: use git_config_string_dup() for pull strategies Message-ID: <20240407010429.GI868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> Converting pull.twohead and pull.octopus to use git_config_string_dup() fixes possible leaks if we see those config variables defined multiple times. Doing so is mostly an "easy" case, except that we later may assign a string literal to pull_twohead (which is now a non-const pointer). That's actually not _too_ bad in practice, as it happens after we've done all of our config parsing (so nobody would try to free it). And the compiler won't complain unless -Wwrite-strings is used (and turning that on creates a host of other warnings, some useful and some not). But while we're here let's future proof it as best we can. Signed-off-by: Jeff King --- builtin/merge.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index c2be29ed2f..a9e5100e70 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -101,7 +101,7 @@ static struct strategy all_strategy[] = { { "subtree", NO_FAST_FORWARD | NO_TRIVIAL }, }; -static const char *pull_twohead, *pull_octopus; +static char *pull_twohead, *pull_octopus; enum ff_type { FF_NO, @@ -615,9 +615,9 @@ static int git_merge_config(const char *k, const char *v, else if (!strcmp(k, "merge.verifysignatures")) verify_signatures = git_config_bool(k, v); else if (!strcmp(k, "pull.twohead")) - return git_config_string(&pull_twohead, k, v); + return git_config_string_dup(&pull_twohead, k, v); else if (!strcmp(k, "pull.octopus")) - return git_config_string(&pull_octopus, k, v); + return git_config_string_dup(&pull_octopus, k, v); else if (!strcmp(k, "commit.cleanup")) return git_config_string_dup(&cleanup_arg, k, v); else if (!strcmp(k, "merge.ff")) { @@ -1291,7 +1291,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (!pull_twohead) { char *default_strategy = getenv("GIT_TEST_MERGE_ALGORITHM"); if (default_strategy && !strcmp(default_strategy, "ort")) - pull_twohead = "ort"; + pull_twohead = xstrdup("ort"); } init_diff_ui_defaults(); From patchwork Sun Apr 7 01:04:50 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619962 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7D99C11181 for ; Sun, 7 Apr 2024 01:04:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451892; cv=none; b=MAtTDeskaj7WW8ZmMAUyM3ocwE/Ai3oIT6r1oRqnPamHGIrokFC/qfz49mbxmPx/jiuyCNzRB1ufkycAE+AW3zrGAavDRdAV0sdD+hk7YTSTjp7LsOz4VuwtFV6Qk4CVxqmgNKsx/2XHYrVciPeKBFSLclLWhGZSt3LBF5bByck= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712451892; c=relaxed/simple; bh=vrPE1BYB2tZTQMsb8ePmXoAII92Tsf0g8/n2x0L0c4I=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=J9uDM71qNAnu+7U9+3HMZRSFg8Hz2W5Qz9exNScjesgtVJbNTP+VmT9i1ZDwY/QQtKeo4s5mcQ78biTDrb5agUbWn68l2bQTPuRwz+nT4kWgt4OJrYxVK2WMXshhtAwRc/P+2hvJbE5NE8US+80wBtldLnaEGeYnKHxQ3E1sCPE= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7398 invoked by uid 109); 7 Apr 2024 01:04:51 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:04:51 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11271 invoked by uid 111); 7 Apr 2024 01:04:53 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:04:53 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:04:50 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 10/11] userdiff: use git_config_string_dup() when we can Message-ID: <20240407010450.GJ868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> There are many uses of git_config_string() in the userdiff code which have the usual "leak when we see the variable multiple times" problem. We can fix many of these in the usual way by switching to the _dup() variant. But note there are some adjacent ones we cannot fix right now: funcname patterns and word regexes default to string literals, and we can't use those with our dup variant (which would try to free the literals). For now let's convert what we can. Signed-off-by: Jeff King --- userdiff.c | 6 +++--- userdiff.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/userdiff.c b/userdiff.c index 92ef649c99..ba168f50b5 100644 --- a/userdiff.c +++ b/userdiff.c @@ -409,15 +409,15 @@ int userdiff_config(const char *k, const char *v) if (!strcmp(type, "binary")) return parse_tristate(&drv->binary, k, v); if (!strcmp(type, "command")) - return git_config_string(&drv->external, k, v); + return git_config_string_dup(&drv->external, k, v); if (!strcmp(type, "textconv")) - return git_config_string(&drv->textconv, k, v); + return git_config_string_dup(&drv->textconv, k, v); if (!strcmp(type, "cachetextconv")) return parse_bool(&drv->textconv_want_cache, k, v); if (!strcmp(type, "wordregex")) return git_config_string(&drv->word_regex, k, v); if (!strcmp(type, "algorithm")) - return git_config_string(&drv->algorithm, k, v); + return git_config_string_dup(&drv->algorithm, k, v); return 0; } diff --git a/userdiff.h b/userdiff.h index d726804c3e..7cae1f4da0 100644 --- a/userdiff.h +++ b/userdiff.h @@ -13,13 +13,13 @@ struct userdiff_funcname { struct userdiff_driver { const char *name; - const char *external; - const char *algorithm; + char *external; + char *algorithm; int binary; struct userdiff_funcname funcname; const char *word_regex; const char *word_regex_multi_byte; - const char *textconv; + char *textconv; struct notes_cache *textconv_cache; int textconv_want_cache; }; From patchwork Sun Apr 7 01:07:04 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619963 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A0CBB812 for ; Sun, 7 Apr 2024 01:07:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712452027; cv=none; b=Y2Q9TXCEvaNA8h/fs9sCNjHkWMv7iBMebXRp9W4IcHH+oueaj8pelq/uQLOJhNOwoNM3/vJu2HyfvMKZYgWNw/EG+wksQFD8RJmnjWshgStMjaamEfwOC/ovp+bAEGJMYhod9RvkZiqj5OfoYPBf/flwSl1nmGDJqye1cDB+m7M= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712452027; c=relaxed/simple; bh=q0ZY4CtALGR6RvelDSXhFi8Mll1I5lfDQzW7n2FuwIQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=TYNTV6L3PrF1gqJcnqHj3+UtiO5mY7rDzW6+xRXjTpdlLEaX2c7CXkKq2pQ+jeJd/CbSg+X/rVl0I2/tzAGuGeEiG/IOvBTRfNSBVBP754e++ufSaaGNvCMWOGuT1hxKW7MQqNOzzyggVUGAbN7etCPJyE3VwnwJou58cyzdq/s= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 7459 invoked by uid 109); 7 Apr 2024 01:07:05 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sun, 07 Apr 2024 01:07:05 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11349 invoked by uid 111); 7 Apr 2024 01:07:07 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 06 Apr 2024 21:07:07 -0400 Authentication-Results: peff.net; auth=none Date: Sat, 6 Apr 2024 21:07:04 -0400 From: Jeff King To: Junio C Hamano Cc: git@vger.kernel.org, =?utf-8?b?UnViw6lu?= Justo Subject: [PATCH 11/11] blame: use "dup" string_list for ignore-revs files Message-ID: <20240407010704.GK868358@coredump.intra.peff.net> References: <20240407005656.GA436890@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net> We feed items into ignore_revs_file_list in two ways: from config and from the command-line. Items from the command-line point to argv entries that we don't own, but items from config are hea-allocated strings whose ownership is handed to the string list when we insert them. Because the string_list is declared with "NODUP", our string_list_clear() call will not free the entries themselves. This is the right thing for the argv entries, but means that we leak the allocated config entries. Let's declare the string list as owning its own strings, which means that the argv entries will be copied. For the config entries we would ideally use string_list_append_nodup() to just hand off ownership of our strings. But we insert them into the sorted list with string_list_insert(), which doesn't have a nodup variant. Since this isn't a hot path, we can accept that the path interpolation will produce a temporary string which is then freed. Signed-off-by: Jeff King --- Not strictly related to this series, but something I noticed while converting this spot in an earlier patch. I had thought originally we could switch to avoid the extra copy altogether: if (!value) return config_error_nonbool(); string_list_insert(&ignore_revs_file_list, value); but of course that is missing the call to interpolate_path(). I imagine that it could also be further refactored to append while reading the config, and then sort after. That's more efficient overall, and would let us use append_nodup(). builtin/blame.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/blame.c b/builtin/blame.c index 0b07ceb110..fa7f8fce09 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -67,7 +67,7 @@ static int no_whole_file_rename; static int show_progress; static char repeated_meta_color[COLOR_MAXLEN]; static int coloring_mode; -static struct string_list ignore_revs_file_list = STRING_LIST_INIT_NODUP; +static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP; static int mark_unblamable_lines; static int mark_ignored_lines; @@ -725,6 +725,7 @@ static int git_blame_config(const char *var, const char *value, if (ret) return ret; string_list_insert(&ignore_revs_file_list, str); + free(str); return 0; } if (!strcmp(var, "blame.markunblamablelines")) {