From patchwork Tue Sep 24 21:50:09 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811237 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 E622382488 for ; Tue, 24 Sep 2024 21:50:10 +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=1727214612; cv=none; b=VWcLik++CB2XnvsPVPj27Kqpn6vXy9ZBLEn/AOq/6e4eL0f0oFK+ZpR5Gs6MSSYF0equ3dZuwqIAL3KqKq7BfB5HsZXQuJ+Ne87V3KjoJ37NRlodDA4UrmWvZyPjeIw1eH1wUh8FXtfpgGrPf/GubQUYP3Zf55EjMTI07JCXakQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727214612; c=relaxed/simple; bh=D8W2xKdwMML+BggqQ8ze2mPgDToyp+ebd/jsbzvbYn0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=rO0PdF+kJ7F4lh+rnxHWF68b7KHb7Fu6q72uIFaGDYkdgae90T0vGPMpvWUpF+ZOqzFjtp+3OJC7uSG+3qNJMnY+WDoqvuOyQXmdRXCcDesWr7G1tXrTpS7CQR6TpFy3BEkFUYakeGZxIFNm2L+S+sD80ZvcNX0tjVcH+mUDfUE= 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 15399 invoked by uid 109); 24 Sep 2024 21:50:10 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:50:10 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18213 invoked by uid 111); 24 Sep 2024 21:50:09 -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; Tue, 24 Sep 2024 17:50:09 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:50:09 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 01/28] http-fetch: clear leaking git-index-pack(1) arguments Message-ID: <20240924215009.GA1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> From: Patrick Steinhardt We never clear the arguments that we pass to git-index-pack(1). Create a common exit path and release them there to plug this leak. This is leak is exposed by t5702, but plugging the leak does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Jeff King --- Obviously I didn't write this or the next two patches, but FWIW, I reviewed them and they all look good to me. http-fetch.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/http-fetch.c b/http-fetch.c index d460bb1837..02ab80533f 100644 --- a/http-fetch.c +++ b/http-fetch.c @@ -106,6 +106,7 @@ int cmd_main(int argc, const char **argv) int nongit; struct object_id packfile_hash; struct strvec index_pack_args = STRVEC_INIT; + int ret; setup_git_directory_gently(&nongit); @@ -157,8 +158,8 @@ int cmd_main(int argc, const char **argv) fetch_single_packfile(&packfile_hash, argv[arg], index_pack_args.v); - - return 0; + ret = 0; + goto out; } if (index_pack_args.nr) @@ -170,7 +171,12 @@ int cmd_main(int argc, const char **argv) commit_id = (char **) &argv[arg++]; commits = 1; } - return fetch_using_walker(argv[arg], get_verbosely, get_recover, - commits, commit_id, write_ref, - commits_on_stdin); + + ret = fetch_using_walker(argv[arg], get_verbosely, get_recover, + commits, commit_id, write_ref, + commits_on_stdin); + +out: + strvec_clear(&index_pack_args); + return ret; } From patchwork Tue Sep 24 21:50:39 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811238 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 DB53A82488 for ; Tue, 24 Sep 2024 21:50:40 +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=1727214642; cv=none; b=Ee/6wapXl+yU1LLzsxl0R+pnc/lToENfTvSqNhcD3Hxwgzj6vkMlNiCCVEj7v5rJj2IjDM+VW9KATRiAoyP/oH45usjMFwVcp88VHIKjhrmjfSAnTbIK6Hgmfqe2ghLfX38KgA5ccMIHSvDBwjXoooEP0JSoO3I+BKZvUyiNvaM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727214642; c=relaxed/simple; bh=jDMMe2wBFJr3UMEyfu1nC3a6lBycStQKPsGNU5qEclk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=iZLjJtT0qyuUhvAPfX7xE++iQhgb42URzMn+SF0WkQ3Zae8ITV6tcLx5Idwp8VSk8z3pKSnyd0C72X3LU7a+QcO/LkQ3jUdijAh5pXmxhSRdfmpIz/p966qaRBfetQPZA+xu1ZDCaSSEsLDNj0jq2yhoXN7d26gLzb7NA4Z1KZQ= 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 15405 invoked by uid 109); 24 Sep 2024 21:50:40 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:50:40 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18361 invoked by uid 111); 24 Sep 2024 21:50:39 -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; Tue, 24 Sep 2024 17:50:39 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:50:39 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 02/28] shallow: fix leak when unregistering last shallow root Message-ID: <20240924215039.GB1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> From: Patrick Steinhardt When unregistering a shallow root we shrink the array of grafts by one and move remaining grafts one to the left. This can of course only happen when there are any grafts left, because otherwise there is nothing to move. As such, this code is guarded by a condition that only performs the move in case there are grafts after the position of the graft to be unregistered. By mistake we also put the call to free the unregistered graft into that condition. But that doesn't make any sense, as we want to always free the graft when it exists. Fix the resulting memory leak by doing so. This leak is exposed by t5500, but plugging it does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Jeff King --- shallow.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/shallow.c b/shallow.c index dcebc263d7..4bb1518dbc 100644 --- a/shallow.c +++ b/shallow.c @@ -51,12 +51,11 @@ int unregister_shallow(const struct object_id *oid) int pos = commit_graft_pos(the_repository, oid); if (pos < 0) return -1; - if (pos + 1 < the_repository->parsed_objects->grafts_nr) { - free(the_repository->parsed_objects->grafts[pos]); + free(the_repository->parsed_objects->grafts[pos]); + if (pos + 1 < the_repository->parsed_objects->grafts_nr) MOVE_ARRAY(the_repository->parsed_objects->grafts + pos, the_repository->parsed_objects->grafts + pos + 1, the_repository->parsed_objects->grafts_nr - pos - 1); - } the_repository->parsed_objects->grafts_nr--; return 0; } From patchwork Tue Sep 24 21:51:03 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811239 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 393FF82488 for ; Tue, 24 Sep 2024 21:51:04 +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=1727214666; cv=none; b=PaT3HbfCsh2VZRMlDOxF/sLBEhsbHmBuWofR7uE9oherRZNT21LofLpHe5BAlyWUGYQiyJVG8E+ges5sgdvLJiH7tiCFo9pvSBQyAvrEipmnnKEtPxw9d39SxPfYcECnvlRQcj2RirNWhCeLFq7Ym7Q1GCYDD58SeiHkm/MQ6gY= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727214666; c=relaxed/simple; bh=o56a2aNQOF1o4l1//J61Ow9LxbmcQFfy5UFvgGahwqw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=WF7ms+guK+8XuhHwsPWHm/8pqFsV36z/5yxo6sxMfR7NlC0FMd3ABEeqM33f2LJFuc2TV3Q0nmEZD2PIC1A9HuKSZKCMTzV/K+WAQqdinZxSr0figCHBWnyPY+GyhPndHXCdTMuavKTrQORGpUngykWJmD4nOgvOT3BDk4aMUGg= 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 15411 invoked by uid 109); 24 Sep 2024 21:51:04 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:51:04 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18494 invoked by uid 111); 24 Sep 2024 21:51: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; Tue, 24 Sep 2024 17:51:03 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:51:03 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 03/28] fetch-pack: fix leaking sought refs Message-ID: <20240924215103.GC1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> From: Patrick Steinhardt When calling `fetch_pack()` the caller is expected to pass in a set of sought-after refs that they want to fetch. This array gets massaged to not contain duplicate entries, which is done by replacing duplicate refs with `NULL` pointers. This modifies the caller-provided array, and in case we do unset any pointers the caller now loses track of that ref and cannot free it anymore. Now the obvious fix would be to not only unset these pointers, but to also free their contents. But this doesn't work because callers continue to use those refs. Another potential solution would be to copy the array in `fetch_pack()` so that we dont modify the caller-provided one. But that doesn't work either because the NULL-ness of those entries is used by callers to skip over ref entries that we didn't even try to fetch in `report_unmatched_refs()`. Instead, we make it the responsibility of our callers to duplicate these arrays as needed. It ain't pretty, but it works to plug the memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Jeff King --- builtin/fetch-pack.c | 11 ++++++++++- t/t5700-protocol-v1.sh | 1 + transport.c | 11 ++++++++++- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index 49222a36fa..c5319232a5 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -53,6 +53,7 @@ int cmd_fetch_pack(int argc, struct ref *fetched_refs = NULL, *remote_refs = NULL; const char *dest = NULL; struct ref **sought = NULL; + struct ref **sought_to_free = NULL; int nr_sought = 0, alloc_sought = 0; int fd[2]; struct string_list pack_lockfiles = STRING_LIST_INIT_DUP; @@ -243,6 +244,13 @@ int cmd_fetch_pack(int argc, BUG("unknown protocol version"); } + /* + * Create a shallow copy of `sought` so that we can free all of its entries. + * This is because `fetch_pack()` will modify the array to evict some + * entries, but won't free those. + */ + DUP_ARRAY(sought_to_free, sought, nr_sought); + fetched_refs = fetch_pack(&args, fd, remote_refs, sought, nr_sought, &shallow, pack_lockfiles_ptr, version); @@ -280,7 +288,8 @@ int cmd_fetch_pack(int argc, oid_to_hex(&ref->old_oid), ref->name); for (size_t i = 0; i < nr_sought; i++) - free_one_ref(sought[i]); + free_one_ref(sought_to_free[i]); + free(sought_to_free); free(sought); free_refs(fetched_refs); free_refs(remote_refs); diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh index a73b4d4ff6..985e04d06e 100755 --- a/t/t5700-protocol-v1.sh +++ b/t/t5700-protocol-v1.sh @@ -11,6 +11,7 @@ export GIT_TEST_PROTOCOL_VERSION GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Test protocol v1 with 'git://' transport diff --git a/transport.c b/transport.c index 3c4714581f..1098bbd60e 100644 --- a/transport.c +++ b/transport.c @@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport, struct git_transport_data *data = transport->data; struct ref *refs = NULL; struct fetch_pack_args args; - struct ref *refs_tmp = NULL; + struct ref *refs_tmp = NULL, **to_fetch_dup = NULL; memset(&args, 0, sizeof(args)); args.uploadpack = data->options.uploadpack; @@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport, goto cleanup; } + /* + * Create a shallow copy of `sought` so that we can free all of its entries. + * This is because `fetch_pack()` will modify the array to evict some + * entries, but won't free those. + */ + DUP_ARRAY(to_fetch_dup, to_fetch, nr_heads); + to_fetch = to_fetch_dup; + refs = fetch_pack(&args, data->fd, refs_tmp ? refs_tmp : transport->remote_refs, to_fetch, nr_heads, &data->shallow, @@ -500,6 +508,7 @@ static int fetch_refs_via_pack(struct transport *transport, ret = -1; data->conn = NULL; + free(to_fetch_dup); free_refs(refs_tmp); free_refs(refs); list_objects_filter_release(&args.filter_options); From patchwork Tue Sep 24 21:51:24 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811240 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 251471420DD for ; Tue, 24 Sep 2024 21:51:25 +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=1727214687; cv=none; b=SI/cC11eX+vi7/UX8/6BG8bb3lYo4hGZyTBtIWYXSFKqrFZ8I9/wvEHJSG6Suoi6WpmkzKKoL6hnlSQyb1ORt13q9albUERhPUJFlYkf7Fn1DLiMdRMPQkww9SFqRvTLMSVXeepwdsdL9EuVJlz+gxSLudj05Nj4MeVcbXYZeDQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727214687; c=relaxed/simple; bh=a6iNkZcrpnIei7os34GZglKI1jg+ODJTNBp6r/Ko2X8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PoAa9WC7XqrSqKDCIMFIn56Nr4+dr63nyR+MRDHTVmhFsbHbE212EV83YqcfK0jjleH6+sbj/lZHCcZZ1x5RbCmtfoK5+j/u2CsuXO2rO3X/z4y6u95VNo0GxZBGM90SANPxlv2VczNXvnFUbcC1gChIcXGAVvqP0FjGGH1jE6Q= 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 15419 invoked by uid 109); 24 Sep 2024 21:51:25 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:51:25 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18498 invoked by uid 111); 24 Sep 2024 21:51:24 -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; Tue, 24 Sep 2024 17:51:24 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:51:24 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 04/28] connect: clear child process before freeing in diagnostic mode Message-ID: <20240924215124.GD1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> The git_connect() function has a special CONNECT_DIAG_URL mode, where we stop short of actually connecting to the other side and just print some parsing details. For URLs that require a child process (like ssh), we free() the child_process struct but forget to clear it, leaking the strings we stuffed into its "env" list. This leak is triggered many times in t5500, which uses "fetch-pack --diag-url", but we're not yet ready to mark it as leak-free. Signed-off-by: Jeff King --- connect.c | 1 + 1 file changed, 1 insertion(+) diff --git a/connect.c b/connect.c index 6829ab3974..58f53d8dcb 100644 --- a/connect.c +++ b/connect.c @@ -1485,6 +1485,7 @@ struct child_process *git_connect(int fd[2], const char *url, free(hostandport); free(path); + child_process_clear(conn); free(conn); strbuf_release(&cmd); return NULL; From patchwork Tue Sep 24 21:52:07 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811241 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 61C2483CD5 for ; Tue, 24 Sep 2024 21:52:08 +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=1727214730; cv=none; b=B0oXCELaB8x9kCL7pKXv5Ayf8ksKPsQaG3TcQGaxJ2ZnvzxlaTDz0jSYeLzEVWRs60XdEiuCsBeGOp0zib4j/XY+mg5QHaWHeMG4Tg7DPU3lbwpJhkAycJ2E5Zp7Dud5mfXsbcEQDj3NvkL77WJjgbhoRKCtx/wq9a2cvSmVORk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727214730; c=relaxed/simple; bh=5xwbp1WLpvje9S2WhnjjI1fOx3Ec6oQCcXsMDT5UQCg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=GBDyfzuVMovZhHJai0xPS1WJHdRKR/n59053bFExGPIG2ImPeEDyuREcfL02W9FKA0B05SX8TpRTiPh19OMZyJxL9N7Wc/DZnI3uWDIkUNJD1bCVTIO+0mgc8rICNs+B1IMs+7D802GfmrAjNSSUvuh6zGDHHzoU0nm2CH/NREQ= 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 15425 invoked by uid 109); 24 Sep 2024 21:52:08 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:52:08 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18507 invoked by uid 111); 24 Sep 2024 21:52: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; Tue, 24 Sep 2024 17:52:07 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:52:07 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 05/28] fetch-pack: free object filter before exiting Message-ID: <20240924215207.GE1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> Our fetch_pack_args holds a filter_options struct that may be populated with allocated strings by the by the "--filter" command-line option. We must free it before exiting to avoid a leak when the program exits. The usual fetch code paths that use transport.c don't have the same leak, because we do the cleanup in disconnect_git(). Fixing this leak lets us mark t5500 as leak-free. Signed-off-by: Jeff King --- builtin/fetch-pack.c | 1 + t/t5500-fetch-pack.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index c5319232a5..cfc6951d23 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -293,5 +293,6 @@ int cmd_fetch_pack(int argc, free(sought); free_refs(fetched_refs); free_refs(remote_refs); + list_objects_filter_release(&args.filter_options); return ret; } diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 585ea0ee16..605f17240c 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -8,6 +8,7 @@ test_description='Testing multi_ack pack fetching' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Test fetch-pack/upload-pack pair. From patchwork Tue Sep 24 21:52:25 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811242 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 9B64F80043 for ; Tue, 24 Sep 2024 21:52:27 +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=1727214749; cv=none; b=CpdG7FMiBX6bIMLYYUVsYpWYrqeXMTNCnWAyiBdlVlUPeT6G2Z/+3ekQZrw1LiqXLUfiHN4+vvPrtiJTVvLeiv1K6KMVJG5/f6BHElf8VUsuqAArfquSLOIHitD95C+PU+DcIBDWcMu2zLnYLRxxcDrjjj1Qkw0rHem+njLkSbQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727214749; c=relaxed/simple; bh=Hnt86+wKgbnuSffOjHBa/SMXsgpIv8BKWgcJwfBtbek=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=TNI7a5n67PHpgP212huh4SmVTGYW55fS4uvmr5m4fiP69/zBM7BR3v9jtKhOJ4lIXi3v15nToEYMAZJj7FHQy3ofTLmR0/OEGDp9exRW7IMmTviSlrRboHaj+HBrJRIcvOIfhuyHlpKjklBiIFttx6IVIcx4dhbHfdUKiqOzfvk= 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 15431 invoked by uid 109); 24 Sep 2024 21:52:27 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:52:27 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18511 invoked by uid 111); 24 Sep 2024 21:52:26 -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; Tue, 24 Sep 2024 17:52:26 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:52:25 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array Message-ID: <20240924215225.GF1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> When we call get_remote_heads() for protocol v0, that may populate the "shallow" oid_array, which must be cleaned up to avoid a leak at the program exit. The same problem exists for both fetch-pack and send-pack, but not for the usual transport.c code paths, since we already do this cleanup in disconnect_git(). Fixing this lets us mark t5542 as leak-free for the send-pack side, but fetch-pack will need some more fixes before we can do the same for t5539. Signed-off-by: Jeff King --- builtin/fetch-pack.c | 1 + builtin/send-pack.c | 1 + t/t5542-push-http-shallow.sh | 1 + 3 files changed, 3 insertions(+) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index cfc6951d23..ef4143eef3 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc, free_refs(fetched_refs); free_refs(remote_refs); list_objects_filter_release(&args.filter_options); + oid_array_clear(&shallow); return ret; } diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 81fc96d423..c49fe6c53c 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -343,5 +343,6 @@ int cmd_send_pack(int argc, free_refs(remote_refs); free_refs(local_refs); refspec_clear(&rs); + oid_array_clear(&shallow); return ret; } diff --git a/t/t5542-push-http-shallow.sh b/t/t5542-push-http-shallow.sh index c2cc83182f..07624a1d7f 100755 --- a/t/t5542-push-http-shallow.sh +++ b/t/t5542-push-http-shallow.sh @@ -5,6 +5,7 @@ test_description='push from/to a shallow clone over http' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd From patchwork Tue Sep 24 21:54:34 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811248 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 794DE146A6B for ; Tue, 24 Sep 2024 21:54:36 +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=1727214878; cv=none; b=qPSQ7KT5cSLkUlq1+Pl3jlZXWNfJIxjyJVY8qB0kgKiJSdk96MoH/VyiDcngKcnI6oRSKer2LXWJX/4wUcDUowKfbUFze7EI6dE39puvhTd3+1bRuf8CD1APRS6XOsz06ds8xfFL3pCe+THV3ZRJDSUE3MVye44FiK8mazlexzA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727214878; c=relaxed/simple; bh=SlhFq/LhmRnIPGExR8cs48DuqSznyLxQRo8SIzMUICw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=hylDQ/BEvmp5bRBpJ1sjfJC+WTeA5nDm3K8QNNSL38kndXUOZ/ejC3pt9EnBowqU6QopbBRBnYWkIlsQ/yBx4VXWLFm4J7jUcPd4AULpmsI4HX9s9MC0VjFY8CImxmT5GJU61ZLYoc1fEZpe6QS/8SNBQRQD+vgg5Fjf7HnW+VU= 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 15444 invoked by uid 109); 24 Sep 2024 21:54:35 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:54:35 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18519 invoked by uid 111); 24 Sep 2024 21:54:35 -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; Tue, 24 Sep 2024 17:54:35 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:54:34 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 07/28] commit: avoid leaking already-saved buffer Message-ID: <20240924215434.GG1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> When we parse a commit via repo_parse_commit_internal(), if save_commit_buffer is set we'll stuff the buffer of the object contents into a cache, overwriting any previous value. This can result in a leak of that previously cached value, though it's rare in practice. If we have a value in the cache it would have come from a previous parse, and during that parse we'd set the object.parsed flag, causing any subsequent parse attempts to exit without doing any work. But it's possible to "unparse" a commit, which we do when registering a commit graft. And since shallow fetches are implemented using grafts, the leak is triggered in practice by t5539. There are a number of possible ways to address this: 1. the unparsing function could clear the cached commit buffer, too. I think this would work for the case I found, but I'm not sure if there are other ways to end up in the same state (an unparsed commit with an entry in the commit buffer cache). 2. when we parse, we could check the buffer cache and prefer it to reading the contents from the object database. In theory the contents of a particular sha1 are immutable, but the code in question is violating the immutability with grafts. So this approach makes me a bit nervous, although I think it would work in practice (the grafts are applied to what we parse, but we still retain the original contents). 3. We could realize the cache is already populated and discard its contents before overwriting. It's possible some other code could be holding on to a pointer to the old cache entry (and we'd introduce a use-after-free), but I think the risk of that is relatively low. 4. The reverse of (3): when the cache is populated, don't bother saving our new copy. This is perhaps a little weird, since we'll have just populated the commit struct based on a different buffer. But the two buffers should be the same, even in the presence of grafts (as in (2) above). I went with option 4. It addresses the leak directly and doesn't carry any risk of breaking other assumptions. And it's the same technique used by parse_object_buffer() for this situation, though I'm not sure when it would even come up there. The extra safety has been there since bd1e17e245 (Make "parse_object()" also fill in commit message buffer data., 2005-05-25). This lets us mark t5539 as leak-free. Signed-off-by: Jeff King --- commit.c | 3 ++- t/t5539-fetch-http-shallow.sh | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/commit.c b/commit.c index 3a54e4db0d..cc03a93036 100644 --- a/commit.c +++ b/commit.c @@ -595,7 +595,8 @@ int repo_parse_commit_internal(struct repository *r, } ret = parse_commit_buffer(r, item, buffer, size, 0); - if (save_commit_buffer && !ret) { + if (save_commit_buffer && !ret && + !get_cached_commit_buffer(r, item, NULL)) { set_commit_buffer(r, item, buffer, size); return 0; } diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh index 3ea75d34ca..82fe09d0a9 100755 --- a/t/t5539-fetch-http-shallow.sh +++ b/t/t5539-fetch-http-shallow.sh @@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone over http' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd From patchwork Tue Sep 24 21:55:39 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811249 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 D0C4D13F43B for ; Tue, 24 Sep 2024 21:55:40 +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=1727214942; cv=none; b=N6uhPyd9nVJ9em3KC7YOk4ksAvje2Nm60CF6DkuMhR4zJqO3Mj7+yUinjoAyARUM1Jzb+jrtcmycqJjjUuhz3qqKu3/O8IgbMfelnW1SbUbUycZpWtI1yuW9LBnQXZce63ZJTsK6OyLY74v31wjzoE1UlXH1/KtjPOGRDkL7aKE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727214942; c=relaxed/simple; bh=fdZcMDnrqsd0Ewj6CRx6vePAO0z9kRDfbf3yanMdyEk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=AV5wU1alqxiZTTCG3OmOcYk4OHnW58Gmx8HKB6MUAgeZLz4X+DLIwpatQFUwpL9eO7rpigzR0vAbupu2L6P7muA9Qq9LiyZwPS4Ogd559LkPPCgLBEl4nu0Vyg7KxiOjfaiEcyecBoRrzd0XDYuZeKokPp3PYuoq3IE2hNid1xo= 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 15452 invoked by uid 109); 24 Sep 2024 21:55:40 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:55:40 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18577 invoked by uid 111); 24 Sep 2024 21:55:39 -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; Tue, 24 Sep 2024 17:55:39 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:55:39 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 08/28] send-pack: free cas options before exit Message-ID: <20240924215539.GH1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> The send-pack --force-with-lease option populates a push_cas_option struct with allocated strings. Exiting without cleaning this up will cause leak-checkers to complain. We can fix this by calling clear_cas_option(), after making it publicly available. Previously it was used only for resetting the list when we saw --no-force-with-lease. The git-push command has the same "leak", though in this case it won't trigger a leak-checker since it stores the push_cas_option struct as a global rather than on the stack (and is thus reachable even after main() exits). I've added cleanup for it here anyway, though, as future-proofing. The leak is triggered by t5541 (it tests --force-with-lease over http, which requires a separate send-pack process under the hood), but we can't mark it as leak-free yet. Signed-off-by: Jeff King --- builtin/push.c | 1 + builtin/send-pack.c | 1 + remote.c | 2 +- remote.h | 1 + 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/push.c b/builtin/push.c index e6f48969b8..59d4485603 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -669,6 +669,7 @@ int cmd_push(int argc, rc = do_push(flags, push_options, remote); string_list_clear(&push_options_cmdline, 0); string_list_clear(&push_options_config, 0); + clear_cas_option(&cas); if (rc == -1) usage_with_options(push_usage, options); else diff --git a/builtin/send-pack.c b/builtin/send-pack.c index c49fe6c53c..8b1d46e79a 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -344,5 +344,6 @@ int cmd_send_pack(int argc, free_refs(local_refs); refspec_clear(&rs); oid_array_clear(&shallow); + clear_cas_option(&cas); return ret; } diff --git a/remote.c b/remote.c index 390a03c264..e291e8ff5c 100644 --- a/remote.c +++ b/remote.c @@ -2544,7 +2544,7 @@ struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map) /* * Compare-and-swap */ -static void clear_cas_option(struct push_cas_option *cas) +void clear_cas_option(struct push_cas_option *cas) { int i; diff --git a/remote.h b/remote.h index a58713f20a..ad4513f639 100644 --- a/remote.h +++ b/remote.h @@ -409,6 +409,7 @@ struct push_cas_option { }; int parseopt_push_cas_option(const struct option *, const char *arg, int unset); +void clear_cas_option(struct push_cas_option *); int is_empty_cas(const struct push_cas_option *); void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *); From patchwork Tue Sep 24 21:56:34 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811250 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 8725D83CD5 for ; Tue, 24 Sep 2024 21:56:36 +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=1727214997; cv=none; b=LiH8ROD8YUeZc5y2KTiIX+riqBm5G4U9BrxO4SzVBb1TTf+6b55+VVKwc7yF6GgU14T4pzrniKgg9chaVieGukxLKgcar9MzSiVkrcPajFooCVxlBmdhChdJtahruuhgFovL5uyccykTSfX9cPllgZT1jq8QrYvGhyuMl62XSow= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727214997; c=relaxed/simple; bh=LknClnxUvpET7D+IKSBbInI+BKE6rSt+hxWgqroEMJs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=DSOQuj4Beoa/9AVpbBunDXuqOaF4zPIfQZRqnPvLC/tiwmen400yLeDKsm2VSZt9WMnX1D6TG76/5327TkUOozJOZ5Tkiny76kVn4rlBmk/6IspH/Oweo59xIz4S3AwBm/tDbagIh2C33Vn0OZA2nq2oHOhMmA+fxs0zfGivrp8= 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 15465 invoked by uid 109); 24 Sep 2024 21:56:36 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:56:35 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18592 invoked by uid 111); 24 Sep 2024 21:56:35 -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; Tue, 24 Sep 2024 17:56:35 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:56:34 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 09/28] transport-helper: fix strbuf leak in push_refs_with_push() Message-ID: <20240924215634.GI1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> We loop over the refs to push, building up a strbuf with the set of "push" directives to send to the remote helper. But if the atomic-push flag is set and we hit a rejected ref, we'll bail from the function early. We clean up most things, but forgot to release the strbuf. Fixing this lets us mark t5541 as leak-free. Signed-off-by: Jeff King --- Arguably this whole function could benefit from a cleanup goto, but it's a little non-trivial. So I stuck with simple-and-stupid. t/t5541-http-push-smart.sh | 1 + transport-helper.c | 1 + 2 files changed, 2 insertions(+) diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh index 71428f3d5c..3ad514bbd4 100755 --- a/t/t5541-http-push-smart.sh +++ b/t/t5541-http-push-smart.sh @@ -7,6 +7,7 @@ test_description='test smart pushing over http via http-backend' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh ROOT_PATH="$PWD" diff --git a/transport-helper.c b/transport-helper.c index c688967b8c..9c8abd8eca 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport, if (atomic) { reject_atomic_push(remote_refs, mirror); string_list_clear(&cas_options, 0); + strbuf_release(&buf); return 0; } else continue; From patchwork Tue Sep 24 21:57:40 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811251 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 5F1F583CD5 for ; Tue, 24 Sep 2024 21:57:42 +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=1727215063; cv=none; b=pTLR83MUwM2ogx3f3FfI53op6FzJ9wqsa85VbzJf3s2iZFYZq1a8IYABHYdMjxuqZm959+QTxJKbxphVZx3EXVAXEv8rytouU1x6GtatlJyvW1rQwn3nCwRDFknwp/qfVXu9u8uAzcB8rpyre31fVTsnfFXN2WJNka6iQZ2X3Mw= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215063; c=relaxed/simple; bh=9tQaFjr84FV2JD2MxbzIKFGSxPCyaZr8Qa/9m+aVZCc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=tNGLn+Y0W0f1olBmJs78CoNEBNOjsL+cIn194vCvzSukjpMqQltUBA9TdXokPkUALP55Gqa9QcnGxgXpsP56KxO7uDVUuhQrsAFTcxwXqVeDld7EEkRfCzicfKzhGYX+ynF0W1ISNUomThzrNqV0fC6UunC9jxi8blvaNYp4h18= 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 15476 invoked by uid 109); 24 Sep 2024 21:57:41 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:57:41 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18597 invoked by uid 111); 24 Sep 2024 21:57: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; Tue, 24 Sep 2024 17:57:41 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:57:40 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 10/28] fetch: free "raw" string when shrinking refspec Message-ID: <20240924215740.GJ1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> The "--prefetch" option to git-fetch modifies the default refspec, including eliminating some entries entirely. When we drop an entry we free the strings in the refspec_item, but we forgot to free the matching string in the "raw" array of the refspec struct. There's no behavioral bug here (since we correctly shrink the raw array, too), but we're leaking the allocated string. Let's add in the leak-fix, and while we're at it drop "const" from the type of the raw string array. These strings are always allocated by refspec_append(), etc, and this makes the memory ownership more clear. This is all a bit more intimate with the refspec code than I'd like, and I suspect it would be better if each refspec_item held on to its own raw string, we had a single array, and we could use refspec_item_clear() to clean up everything. But that's a non-trivial refactoring, since refspec_item structs can be held outside of a "struct refspec", without having a matching raw string at all. So let's leave that for now and just fix the leak in the most immediate way. This lets us mark t5582 as leak-free. Signed-off-by: Jeff King --- builtin/fetch.c | 1 + refspec.c | 2 +- refspec.h | 2 +- t/t5582-fetch-negative-refspec.sh | 1 + 4 files changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index c900f57721..80a64d0d26 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -456,6 +456,7 @@ static void filter_prefetch_refspec(struct refspec *rs) free(rs->items[i].src); free(rs->items[i].dst); + free(rs->raw[i]); for (j = i + 1; j < rs->nr; j++) { rs->items[j - 1] = rs->items[j]; diff --git a/refspec.c b/refspec.c index ec90ab349a..c3cf003443 100644 --- a/refspec.c +++ b/refspec.c @@ -225,7 +225,7 @@ void refspec_clear(struct refspec *rs) rs->nr = 0; for (i = 0; i < rs->raw_nr; i++) - free((char *)rs->raw[i]); + free(rs->raw[i]); FREE_AND_NULL(rs->raw); rs->raw_alloc = 0; rs->raw_nr = 0; diff --git a/refspec.h b/refspec.h index 754be45cee..3760fdaf2b 100644 --- a/refspec.h +++ b/refspec.h @@ -43,7 +43,7 @@ struct refspec { int alloc; int nr; - const char **raw; + char **raw; int raw_alloc; int raw_nr; diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh index 7a80e47c2b..7fa54a4029 100755 --- a/t/t5582-fetch-negative-refspec.sh +++ b/t/t5582-fetch-negative-refspec.sh @@ -8,6 +8,7 @@ test_description='"git fetch" with negative refspecs. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup ' From patchwork Tue Sep 24 21:58: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: 13811252 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 2DD7713B7BC for ; Tue, 24 Sep 2024 21:58:02 +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=1727215083; cv=none; b=fk86f6HouLghXYw/fNVJcd+T1b2zOiedGJPhernIYIuon9Uxw7Xr3b/76nHjZF4yTix3Jv59Z27wth3TtbVimysyawsPHiDbASoMJQOqhZ8fknZIvxCvrD6fnveuqWd28wTVZvijkyuaZt7s9KjON1AZ72cQcWM3b7WCD9fTMqg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215083; c=relaxed/simple; bh=SJSJLrZueDT8kZzYhZA+eO9mRzSskgGH82LtB7OOCHY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=CugZh3QQIBgWryNNz7MmWVPwcM1kifxp4Eb0qeaFRrCS4QvoTNPzs8fwMX0jjW8lHDGSgsY/FRL46jwBc9EYJeao0/7pUue9Vzf0T39Wv/l5MCUzzo+BPz1Iu2pN/lydEgxK/nh0UF4EzIlW5D8CX8DV/JcDMwcqPnpsRr9CshY= 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 15483 invoked by uid 109); 24 Sep 2024 21:58:01 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:58:01 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18601 invoked by uid 111); 24 Sep 2024 21:58:00 -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; Tue, 24 Sep 2024 17:58:00 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:58:00 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 11/28] fetch-pack: clear pack lockfiles list Message-ID: <20240924215800.GK1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> If the --lock-pack option is passed (which it typically is when fetch-pack is used under the hood by smart-http), then we may end up with entries in our pack_lockfiles string_list. We need to clear them before returning to avoid a leak. In git-fetch this isn't a problem, since the same cleanup happens via transport_unlock_pack(). But the leak is detectable in t5551, which does http fetches. Signed-off-by: Jeff King --- builtin/fetch-pack.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index ef4143eef3..62e8c3aa6b 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -295,5 +295,6 @@ int cmd_fetch_pack(int argc, free_refs(remote_refs); list_objects_filter_release(&args.filter_options); oid_array_clear(&shallow); + string_list_clear(&pack_lockfiles, 0); return ret; } From patchwork Tue Sep 24 21:58:47 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811253 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 99A1413B7BC for ; Tue, 24 Sep 2024 21:58:48 +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=1727215130; cv=none; b=tATKRRl1xh3S+EF0UPoNEMMxnmUTDLzRRUSd3YkgkhK/+dmhjT+UcuIQ1tMGaD5DBFhpDnf8son85hTHOQQTPdV7/k/8nZoQmZliFPWxNm97pbwiyrFMOIZco6Zf6ArRNOUB/sIwkwMferT0WDDptCuXJNOcMJ52VLdFxDJ2WtM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215130; c=relaxed/simple; bh=a4vOnXl/zkSeI5KFYQGxoYLuj936FcOFhSXEq2bsAV8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=EyD1M0L2NZY0d5mcgCcwL4Lh3yUC6T3C6nID00aHi/VVn7OAAi6AVJmMuOv2yxrwE/TeYlBZKyH0BLMfLWW2l9Zxuy4vX+6dr9cd0auBp1gTI3DDejoGG+Y9pmatJ77om7Zjw5A+IFp49DP8dBtj2OkHdgS4sZXydTU50MncGHs= 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 15490 invoked by uid 109); 24 Sep 2024 21:58:48 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:58:48 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18611 invoked by uid 111); 24 Sep 2024 21:58:47 -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; Tue, 24 Sep 2024 17:58:47 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:58:47 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 12/28] transport-helper: fix leak of dummy refs_list Message-ID: <20240924215847.GL1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> When using a remote-helper, the fetch_refs() function will issue a "list" command if we haven't already done so. We don't care about the result, but this is just to maintain compatibility as explained in ac3fda82bf (transport-helper: skip ls-refs if unnecessary, 2019-08-21). But get_refs_list_using_list(), the function we call to issue the command, does parse and return the resulting ref list, which we simply leak. We should record the return value and free it immediately (another approach would be to teach it to avoid allocating at all, but it does not seem worth the trouble to micro-optimize this mostly historical case). Triggering this requires the v0 protocol (since in v2 we use stateless connect to take over the connection). You can see it in t5551.37, "fetch by SHA-1 without tag following", as it explicitly enables v0. Signed-off-by: Jeff King --- transport-helper.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 9c8abd8eca..013ec79dc9 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -717,8 +717,14 @@ static int fetch_refs(struct transport *transport, return -1; } - if (!data->get_refs_list_called) - get_refs_list_using_list(transport, 0); + if (!data->get_refs_list_called) { + /* + * We do not care about the list of refs returned, but only + * that the "list" command was sent. + */ + struct ref *dummy = get_refs_list_using_list(transport, 0); + free_refs(dummy); + } count = 0; for (i = 0; i < nr_heads; i++) From patchwork Tue Sep 24 21:59:14 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811254 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 0C34B14831E for ; Tue, 24 Sep 2024 21:59:15 +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=1727215157; cv=none; b=fGcv+GsUR6tBrNeumL1QgpmyjSSNhzRAX2md4M2vtEeLa+FoUo1fSBvAijLMyBIueTkv+x+gfv7U1KVnYj5s2VU/LBJSdpO6aSiVuziw1cQn9EkreczUHSc+GIqGqeNDpdKymwdgX1qqu1DB7InZhXe4BtFIeTgw5TxjKtXOp60= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215157; c=relaxed/simple; bh=FvTuMYIjlrEgXXutyQopmn0reLw6+WtFHFZ4Mb4Pslw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=tnWeGzYzOsjr/dRQV+fusMgDGue5wBV+Q+m0eO3iXP05DL09UihYKfuZdi8r/+a39i9dxryIzSpdL4Q0SE8QeWTT54Piyc6eccGXkVcECff6ynH+eW4ffvyBF5vhZvgx9HIFKQMIxA9YP9Bt/mYzw4RICnVwvcB+7w50CZL/mgs= 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 15499 invoked by uid 109); 24 Sep 2024 21:59:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 21:59:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18615 invoked by uid 111); 24 Sep 2024 21:59:14 -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; Tue, 24 Sep 2024 17:59:14 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 17:59:14 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 13/28] http: fix leak when redacting cookies from curl trace Message-ID: <20240924215914.GM1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> When redacting headers for GIT_TRACE_CURL, we build up a redacted cookie header in a local strbuf, and then copy it into the output. But we forget to release the temporary strbuf, leaking it for every cookie header we show. The other redacted headers don't run into this problem, since they're able to work in-place in the output buffer. But the cookie parsing is too complicated for that, since we redact the cookies individually. This leak is triggered by the cookie tests in t5551. Signed-off-by: Jeff King --- http.c | 1 + 1 file changed, 1 insertion(+) diff --git a/http.c b/http.c index 6c6cc5c822..cc136408c0 100644 --- a/http.c +++ b/http.c @@ -800,6 +800,7 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset) strbuf_setlen(header, sensitive_header - header->buf); strbuf_addbuf(header, &redacted_header); + strbuf_release(&redacted_header); ret = 1; } return ret; From patchwork Tue Sep 24 22:01:09 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811263 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 8C29142A8F for ; Tue, 24 Sep 2024 22:01:11 +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=1727215273; cv=none; b=ZDTcRtEZLIybJfkUfYIleY+qgZNfY9uGGqdPxb1hjJqpsL/UbJNYtBHZsbOBztYqvS21HyqhtVEuJR8qc+zRRieE/LVkpVQirg/w2T6QsY4tz46wp5yEVV5HQYXF0C65YZ8q22BFgOcH85eXH2OEvqyjrBkxf3+7/id/uYv8xWA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215273; c=relaxed/simple; bh=0vEa0m7VoJnlHdwofMDX++IpiaG83CEMI3ZspuG3htE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=qLHx+nMgnBX49+nB6nzr9m2Bb00Hq0Y2GOHDEJbBQX9g4kqsDnEf9eOXApgK1iWzCx1z0IHIEUwOkxJaUnuaTABpPCwgeC0zITsxBuYbQY5W8j1krxXLiBz2q8Ul6jaiVu8HwdueFmeta58jNnYTGjbxabkZok96My5zbyNtcAU= 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 15525 invoked by uid 109); 24 Sep 2024 22:01:11 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:01:11 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18696 invoked by uid 111); 24 Sep 2024 22:01:10 -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; Tue, 24 Sep 2024 18:01:10 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:01:09 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 14/28] http: fix leak of http_object_request struct Message-ID: <20240924220109.GN1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> The new_http_object_request() function allocates a struct on the heap, along with some fields inside the struct. But the matching function to clean it up, release_http_object_request(), only frees the interior fields without freeing the struct itself, causing a leak. The related http_pack_request new/release pair gets this right, and at first glance we should be able to do the same thing and just add a single free() call. But there's a catch. These http_object_request structs are typically embedded in the object_request struct of http-walker.c. And when we clean up that parent struct, it sanity-checks the embedded struct to make sure we are not leaking descriptors. Which means a use-after-free if we simply free() the embedded struct. I have no idea how valuable that sanity-check is, or whether it can simply be deleted. This all goes back to 5424bc557f (http*: add helper methods for fetching objects (loose), 2009-06-06). But the obvious way to make it all work is to be sure we set the pointer to NULL after freeing it (and our freeing process closes the descriptor, so we know there is no leak). To make sure we do that consistently, we'll switch the pointer we take in release_http_object_request() to a pointer-to-pointer, and we'll set it to NULL ourselves. And then the compiler can help us find each caller which needs to be updated. Most cases will just pass "&obj_req->req", which will obviously do the right thing. In a few cases, like http-push's finish_request(), we are working with a copy of the pointer, so we don't NULL the original. But it's OK because the next step is to free the struct containing the original pointer anyway. This lets us mark t5551 as leak-free. Ironically this is the "smart" http test, and the leak here only affects dumb http. But there's a single dumb-http invocation in there. The full dumb tests are in t5550, which still has some more leaks. This also makes t5559 leak-free, as it's just an HTTP/2 variant of t5551. But we don't need to mark it as such, since it inherits the flag from t5551. Signed-off-by: Jeff King --- http-push.c | 4 ++-- http-walker.c | 8 ++++---- http.c | 11 ++++++++--- http.h | 4 ++-- t/t5551-http-fetch-smart.sh | 1 + 5 files changed, 17 insertions(+), 11 deletions(-) diff --git a/http-push.c b/http-push.c index 7315a694aa..7196ffa525 100644 --- a/http-push.c +++ b/http-push.c @@ -275,7 +275,7 @@ static void start_fetch_loose(struct transfer_request *request) if (!start_active_slot(slot)) { fprintf(stderr, "Unable to start GET request\n"); repo->can_update_info_refs = 0; - release_http_object_request(obj_req); + release_http_object_request(&obj_req); release_request(request); } } @@ -580,7 +580,7 @@ static void finish_request(struct transfer_request *request) /* Try fetching packed if necessary */ if (request->obj->flags & LOCAL) { - release_http_object_request(obj_req); + release_http_object_request(&obj_req); release_request(request); } else start_fetch_packed(request); diff --git a/http-walker.c b/http-walker.c index e417a7f51c..9c1e5c37e6 100644 --- a/http-walker.c +++ b/http-walker.c @@ -74,7 +74,7 @@ static void start_object_request(struct object_request *obj_req) obj_req->state = ACTIVE; if (!start_active_slot(slot)) { obj_req->state = ABORTED; - release_http_object_request(req); + release_http_object_request(&req); return; } } @@ -110,7 +110,7 @@ static void process_object_response(void *callback_data) if (obj_req->repo->next) { obj_req->repo = obj_req->repo->next; - release_http_object_request(obj_req->req); + release_http_object_request(&obj_req->req); start_object_request(obj_req); return; } @@ -495,7 +495,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash) if (repo_has_object_file(the_repository, &obj_req->oid)) { if (obj_req->req) - abort_http_object_request(obj_req->req); + abort_http_object_request(&obj_req->req); abort_object_request(obj_req); return 0; } @@ -543,7 +543,7 @@ static int fetch_object(struct walker *walker, unsigned char *hash) strbuf_release(&buf); } - release_http_object_request(req); + release_http_object_request(&obj_req->req); release_object_request(obj_req); return ret; } diff --git a/http.c b/http.c index cc136408c0..d0242ffb50 100644 --- a/http.c +++ b/http.c @@ -2816,15 +2816,17 @@ int finish_http_object_request(struct http_object_request *freq) return freq->rename; } -void abort_http_object_request(struct http_object_request *freq) +void abort_http_object_request(struct http_object_request **freq_p) { + struct http_object_request *freq = *freq_p; unlink_or_warn(freq->tmpfile.buf); - release_http_object_request(freq); + release_http_object_request(freq_p); } -void release_http_object_request(struct http_object_request *freq) +void release_http_object_request(struct http_object_request **freq_p) { + struct http_object_request *freq = *freq_p; if (freq->localfile != -1) { close(freq->localfile); freq->localfile = -1; @@ -2838,4 +2840,7 @@ void release_http_object_request(struct http_object_request *freq) } curl_slist_free_all(freq->headers); strbuf_release(&freq->tmpfile); + + free(freq); + *freq_p = NULL; } diff --git a/http.h b/http.h index a516ca4a9a..46e334c2c2 100644 --- a/http.h +++ b/http.h @@ -240,8 +240,8 @@ struct http_object_request *new_http_object_request( const char *base_url, const struct object_id *oid); void process_http_object_request(struct http_object_request *freq); int finish_http_object_request(struct http_object_request *freq); -void abort_http_object_request(struct http_object_request *freq); -void release_http_object_request(struct http_object_request *freq); +void abort_http_object_request(struct http_object_request **freq); +void release_http_object_request(struct http_object_request **freq); /* * Instead of using environment variables to determine if curl tracing happens, diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 7b5ab0eae1..e36dfde17e 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -5,6 +5,7 @@ test_description="test smart fetching over http via http-backend ($HTTP_PROTO)" GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-httpd.sh test "$HTTP_PROTO" = "HTTP/2" && enable_http2 From patchwork Tue Sep 24 22:02:13 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811264 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 7AF6C42A8F for ; Tue, 24 Sep 2024 22:02:15 +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=1727215337; cv=none; b=qYam2GhXOo+FEV+y9wfVuPpDnGj/Tk092+jzyUzUPmfIGLNyxS9Yfcvf2ZIj4u1YyhEgrpRw4viEEpHfejvzLyrpKZ3Jpim3m6T+n8lcmqAfd/f9N8wKW9bOrN34EMnwuujWeDuC38vFXr8AcXX+VNxhuJtnvdyIdn3oFS5K1Ew= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215337; c=relaxed/simple; bh=U8/iGqqOYrUN8GjpO6H0TN8+Ebb5SSKFI3MYtjgeBKM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=BpqyBXvuYhO67cMjG9qk/JjGqn3kgQJBhBrTxTRcslVcW6WgIF/Yz9db4VGdB5t82sDkfX2cSbf1D7Kgla72Z4extXM2I1V91QbDjS1oDZgvKxeGvHPFs2PspPL9r4+wmfISQ9Bfr/Iib/D732G9I17UZLWayg5zy6rIPTmm5S8= 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 15535 invoked by uid 109); 24 Sep 2024 22:02:14 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:02:14 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18702 invoked by uid 111); 24 Sep 2024 22:02:14 -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; Tue, 24 Sep 2024 18:02:14 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:02:13 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 15/28] http: call git_inflate_end() when releasing http_object_request Message-ID: <20240924220213.GO1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> In new_http_object_request(), we initialize the zlib stream with git_inflate_init(). We must have a matching git_inflate_end() to avoid leaking any memory allocated by zlib. In most cases this happens in finish_http_object_request(), but we don't always get there. If we abort a request mid-stream, then we may clean it up without hitting that function. We can't just add a git_inflate_end() call to the release function, though. That would double-free the cases that did actually finish. Instead, we'll move the call from the finish function to the release function. This does delay it for the cases that do finish, but I don't think it matters. We should have already reached Z_STREAM_END (and complain if we didn't), and we do not record any status code from git_inflate_end(). This leak is triggered by t5550 at least (and probably other dumb-http tests). I did find one other related spot of interest. If we try to read a previously downloaded file and fail, we reset the stream by calling memset() followed by a fresh git_inflate_init(). I don't think this case is triggered in the test suite, but it seemed like an obvious leak, so I added the appropriate git_inflate_end() before the memset() there. Signed-off-by: Jeff King --- http.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/http.c b/http.c index d0242ffb50..4d841becca 100644 --- a/http.c +++ b/http.c @@ -2726,6 +2726,7 @@ struct http_object_request *new_http_object_request(const char *base_url, * file; also rewind to the beginning of the local file. */ if (prev_read == -1) { + git_inflate_end(&freq->stream); memset(&freq->stream, 0, sizeof(freq->stream)); git_inflate_init(&freq->stream); the_hash_algo->init_fn(&freq->c); @@ -2799,7 +2800,6 @@ int finish_http_object_request(struct http_object_request *freq) return -1; } - git_inflate_end(&freq->stream); the_hash_algo->final_oid_fn(&freq->real_oid, &freq->c); if (freq->zret != Z_STREAM_END) { unlink_or_warn(freq->tmpfile.buf); @@ -2840,6 +2840,7 @@ void release_http_object_request(struct http_object_request **freq_p) } curl_slist_free_all(freq->headers); strbuf_release(&freq->tmpfile); + git_inflate_end(&freq->stream); free(freq); *freq_p = NULL; From patchwork Tue Sep 24 22:02:27 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811265 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 0692780043 for ; Tue, 24 Sep 2024 22:02:28 +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=1727215350; cv=none; b=hRQmJSY9iN6JW1JVh2yHLrCTmxcy4GiJUZhyqwvT5XPEcaAqsQxJsRduPqr+j9/w0zxh/pR9c4KQRdt1wSL2G8KciP+gobErtrU5oM+lVGA9vEECF+SaRY9tjm16tsBkMDK8jX9o6UbBtxAnJiHHiQROEeysWqnSqOXZvjx3C2g= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215350; c=relaxed/simple; bh=4+HhupHR/7ev57JGoQKGeQr5NzYEjOMlwk19sGKRgSU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=NVvMm/QDJ0R36PJfAQZ5afCkJE3EkFKYIFreilzc1uS+8pJ7YUsinNZ0EV/lHBb0FWWj6D8D1t3urzXadHwybAhKMKytnvZn4HfQawILZdo/Wux1bc104AnMVu4IcSG5omLKWJbfeOXkXZSNKBPuPO4FC0+bCv4V5dNQTR9Qpm8= 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 15540 invoked by uid 109); 24 Sep 2024 22:02:28 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:02:28 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18706 invoked by uid 111); 24 Sep 2024 22:02:27 -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; Tue, 24 Sep 2024 18:02:27 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:02:27 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 16/28] http: stop leaking buffer in http_get_info_packs() Message-ID: <20240924220227.GP1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> We use http_get_strbuf() to fetch the remote info/packs content into a strbuf, but never free it, causing a leak. There's no need to hold onto it, as we've already parsed it completely. This lets us mark t5619 as leak-free. Signed-off-by: Jeff King --- http.c | 1 + t/t5619-clone-local-ambiguous-transport.sh | 1 + 2 files changed, 2 insertions(+) diff --git a/http.c b/http.c index 4d841becca..54463770b4 100644 --- a/http.c +++ b/http.c @@ -2475,6 +2475,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head) cleanup: free(url); + strbuf_release(&buf); return ret; } diff --git a/t/t5619-clone-local-ambiguous-transport.sh b/t/t5619-clone-local-ambiguous-transport.sh index cce62bf78d..1d4efe414d 100755 --- a/t/t5619-clone-local-ambiguous-transport.sh +++ b/t/t5619-clone-local-ambiguous-transport.sh @@ -2,6 +2,7 @@ test_description='test local clone with ambiguous transport' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-httpd.sh" From patchwork Tue Sep 24 22:03:10 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811266 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 2A399149E1A for ; Tue, 24 Sep 2024 22:03:11 +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=1727215393; cv=none; b=DNqcyzPCDTZRCWmcI2iUgiRzL0jbKs/PaY8FcL2yIUwmzS1HP5rx+s5eGH+HryvtNTd0wh6v66lBuUb+JNZ/Ap83v4Js/lWnUKnb23TYfw1krlssCVzBnOWHGV2iF6Lto/R2cDpIH35enf7CrfEGE3C3M023IZqCJQf7ZO4HzwM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215393; c=relaxed/simple; bh=60lrT4z+Q/l88KkEMWOC7ehlO6U1WzsIYc+eEJtvsrw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Ak8eotAdKWvKkAdQuQ8UWlqwfe1nDjnF4Z1ENo1NCJ9ipa9+qctBPKVZ7CsUHR9dijzvJDlmCttEBarrQCuBXJ/kFSla+iqAIbKvcWE3/XHbiBqCHmIclt3M87Gm/R1gxcpzcfI2+/4LrPn5aZEF8Fg/vswpETzBAR9IiygbrDA= 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 15552 invoked by uid 109); 24 Sep 2024 22:03:11 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:03:11 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18724 invoked by uid 111); 24 Sep 2024 22:03:10 -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; Tue, 24 Sep 2024 18:03:10 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:03:10 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 17/28] remote-curl: free HEAD ref with free_one_ref() Message-ID: <20240924220310.GQ1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> After dumb-http downloads the remote info/refs file, it adds an extra HEAD ref struct to our list by downloading the remote symref and finding the matching ref within our list. If either of those fails, we throw away the ref struct. But we do so with free(), when we should use free_one_ref() to catch any embedded allocations (in particular, if fetching the remote HEAD succeeded but the branch is unborn, its ref->symref field will be populated but we'll still throw it all away). This leak is triggered by t5550 (but we still have a little more work to mark it leak-free). Signed-off-by: Jeff King --- remote-curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index 4adcf25ed6..9a71e04301 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -347,7 +347,7 @@ static struct ref *parse_info_refs(struct discovery *heads) ref->next = refs; refs = ref; } else { - free(ref); + free_one_ref(ref); } return refs; From patchwork Tue Sep 24 22:04:12 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811267 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 A72D242A8F for ; Tue, 24 Sep 2024 22:04:14 +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=1727215456; cv=none; b=UtZOTTLelh3FrREoQCyN6pwbLodGbyjI3RSn3YUEvJQeBU6bFVOThQI5e/3fH9Mr2l3pdjQ1rcq7DJG3AIFLkzp2ailTNfJzrsop7NRMaByQEAFFn3xy5ec1AhRSRaLSpbywzMo4VMFrTVphofzH8P8iRvqIKyQzOF5xgcGVTcI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215456; c=relaxed/simple; bh=yxy/ehjSlRULAz+qW4t1MyZCqRtjbz1m5luK9HJvDvI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=IVwIWCb/I9DA5p09AsKSqVoMQzrluiWIdWP/yPhssiP/g2GX7Bdibs3BBfhdNhzjXNaLuWN5cU6w7ghFRdS2v8Qooykl9sIttNVRjPvAwlhVWfftmMBWcjfk3mDICJqK2h5qbL9NtpzrdIj4KNf4iQ8iJGFYB7Ry2AAJzcEX/uI= 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 15562 invoked by uid 109); 24 Sep 2024 22:04:14 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:04:14 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18741 invoked by uid 111); 24 Sep 2024 22:04:13 -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; Tue, 24 Sep 2024 18:04:13 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:04:12 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 18/28] http-walker: free fake packed_git list Message-ID: <20240924220412.GR1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> The dumb-http walker code creates a "fake" packed_git list representing packs we've downloaded from the remote (I call it "fake" because generally that struct is only used and managed by the local repository struct). But during our cleanup phase we don't touch those at all, causing a leak. There's no support here from the rest of the object-database API, as these structs are not meant to be freed, except when closing the object store completely. But we can see that raw_object_store_clear() just calls free() on them, and that's enough here to fix the leak. I also added a call to close_pack() before each. In the regular code this happens via close_object_store(), which we do as part of raw_object_store_clear(). This is necessary to prevent leaking mmap'd data (like the pack idx) or descriptors. The leak-checker won't catch either of these itself, but I did confirm with some hacky warning() calls and running t5550 that it's easy to leak at least index data. This is all much more intimate with the packed_git struct than I'd like, but I think fixing it would be a pretty big refactor. And it's just not worth it for dumb-http code which is rarely used these days. If we can silence the leak-checker without creating too much hassle, we should just do that. This lets us mark t5550 as leak-free. Signed-off-by: Jeff King --- http-walker.c | 10 ++++++++++ t/t5550-http-fetch-dumb.sh | 1 + 2 files changed, 11 insertions(+) diff --git a/http-walker.c b/http-walker.c index 9c1e5c37e6..fb2d86d5e7 100644 --- a/http-walker.c +++ b/http-walker.c @@ -579,8 +579,18 @@ static void cleanup(struct walker *walker) if (data) { alt = data->alt; while (alt) { + struct packed_git *pack; + alt_next = alt->next; + pack = alt->packs; + while (pack) { + struct packed_git *pack_next = pack->next; + close_pack(pack); + free(pack); + pack = pack_next; + } + free(alt->base); free(alt); diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index ea8e48f627..58189c9f7d 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -4,6 +4,7 @@ test_description='test dumb fetching over http via static file' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh if test_have_prereq !REFFILES From patchwork Tue Sep 24 22:04:30 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811274 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 B488A42A8F for ; Tue, 24 Sep 2024 22:04:31 +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=1727215473; cv=none; b=kM+r0+rGteFnm/SBotjrniEo5BDy7Qg/l/tNdNm6E7+LDD1MlOkAx3B9qXidNOycO1L/oE9ghDJdHLpPYh+n1WwmdMhKTuHJ6+msJIiW2m4l3TP24eQu42rQu3ZoqLmTPMHlPx7iVTLfhP/KQLsgMEwc4M2TRtenvHxjQUGd1GI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215473; c=relaxed/simple; bh=O4PVEIYSjjKMV+GBLyOZNCSDWsZPY9T3UEJvuinv1Wk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=MeXsjsRMqmzn5FgCnHJm3HldI4dfeyaawJ83Av8NvP3xNBSB6mGcz9VZ8fAsiyPyB28WDOfj9aNgIkJY0wa4bIIa7g+eRgke9cmgJS2YgdCnOJGqB6Vs/UraWp+bfiaj8cLi0tyCfWuvemal/b292yAiI8iImvn0hua2uz5ykN0= 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 15570 invoked by uid 109); 24 Sep 2024 22:04:31 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:04:31 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18745 invoked by uid 111); 24 Sep 2024 22:04:30 -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; Tue, 24 Sep 2024 18:04:30 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:04:30 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 19/28] http-push: clear refspecs before exiting Message-ID: <20240924220430.GS1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> We parse the command-line arguments into a refspec struct, but we never free them. We should do so before exiting to avoid triggering the leak-checker. This triggers in t5540 many times (basically every invocation of http-push). Signed-off-by: Jeff King --- http-push.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http-push.c b/http-push.c index 7196ffa525..f60b2ceba5 100644 --- a/http-push.c +++ b/http-push.c @@ -1983,5 +1983,7 @@ int cmd_main(int argc, const char **argv) request = next_request; } + refspec_clear(&rs); + return rc; } From patchwork Tue Sep 24 22:04:46 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811275 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 21B0242A8F for ; Tue, 24 Sep 2024 22:04:47 +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=1727215489; cv=none; b=IECyX3/h/YDrnfHGUmMG4Pa0dJ5xSji021TEy1bwN2+V4Tds/oivAx207RJMtvYFbI8aVXSQuep/C+Hovla/sVCt3MDWvewU3JXZIHlI6JA6e0vK/Efr8Kb5r3CUWxsrcIZMKXedLkHuoWXtnLFijy2423K3Nkl7EuNdU/cIYVg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215489; c=relaxed/simple; bh=c4vloYVRGtopetei8LpYCBm0sNSAKQnKKcvkzUPxG8o=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=XHFu4RuF/BnrUJ/rNa9Peu6kGlL1H5/BuWToipAsEuvMM3PUjnGsAxZ+DN2fKWudgYKPOaFL3fPRzlpIqlFEFadxDRCvVkSXRA3BwHdk6VQ1iYVgotcB+PdfCyzmAcqOE9BuOAGtH1d8U0Ncf3m/mKaqxSKA0rf2aAVXYJlBMac= 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 15574 invoked by uid 109); 24 Sep 2024 22:04:47 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:04:47 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18749 invoked by uid 111); 24 Sep 2024 22:04: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; Tue, 24 Sep 2024 18:04:46 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:04:46 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 20/28] http-push: free repo->url string Message-ID: <20240924220446.GT1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> Our repo->url string comes from str_end_url_with_slash(), which always allocates its output buffer. We should free it before exiting to avoid triggering the leak-checker. This can be seen by leak-checking t5540. Signed-off-by: Jeff King --- http-push.c | 1 + 1 file changed, 1 insertion(+) diff --git a/http-push.c b/http-push.c index f60b2ceba5..52c53928a9 100644 --- a/http-push.c +++ b/http-push.c @@ -1972,6 +1972,7 @@ int cmd_main(int argc, const char **argv) cleanup: if (info_ref_lock) unlock_remote(info_ref_lock); + free(repo->url); free(repo); http_cleanup(); From patchwork Tue Sep 24 22:05: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: 13811276 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 37BDC42A8F for ; Tue, 24 Sep 2024 22:05:52 +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=1727215553; cv=none; b=Ez1KfKVtNooVNUWpZbUsmIDtPSB9SQEyInsnR2gtiDjPzVoGlZ8njHUYP5knEi8fSdgo2NCo+mpOH0kHcTrbpGs8ZZQ9BItU1rJ5vrA2C41Dmh+iGzcZfBE7aS1cQyLWUkAtANegTd6Qdsi5sxTywdg0nc//SJ6ZaH/tFmEUFVc= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215553; c=relaxed/simple; bh=cgH51hyTMfwnu+WQCELbiEGAPdeLpUjQwuBIh3K0IRk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=HEyhkKLV0g3g/7fY5ETMffVMjjyBY3hNupEV+ERdkgv0HA2Qhfu2w7+mXRRjR1mVdci11gzNnb4WhFM3rxNWEG7S3n7vP1c2XEUTg5iyeD3P/f4yRtByHyptE9w6m6sQamH1dOrgxhJeiMJfqMzkg2LqQ/A8I4r/l3Eo8k7s9ig= 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 15582 invoked by uid 109); 24 Sep 2024 22:05:51 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:05:51 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18814 invoked by uid 111); 24 Sep 2024 22:05:50 -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; Tue, 24 Sep 2024 18:05:50 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:05:50 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 21/28] http-push: free curl header lists Message-ID: <20240924220550.GU1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> To pass headers to curl, we have to allocate a curl_slist linked list and then feed it to curl_easy_setopt(). But the header list is not copied by curl, and must remain valid until we are finished with the request. A few spots in http-push get this right, freeing the list after finishing the request, but many do not. In most cases the fix is simple: we set up the curl slot, start it, and then use run_active_slot() to take it to completion. After that, we don't need the headers anymore and can call curl_slist_free_all(). But one case is trickier: when we do a MOVE request, we start the request but don't immediately finish it. It's possible we could change this to be more like the other requests, but I didn't want to get into risky refactoring of this code. So we need to stick the header list into the request struct and remember to free it later. Curiously, the struct already has a headers field for this purpose! It goes all the way back to 58e60dd203 (Add support for pushing to a remote repository using HTTP/DAV, 2005-11-02), but it doesn't look like it was ever used. We can make use of it just by assigning our headers to it, and there is already code in finish_request() to clean it up. This fixes several leaks triggered by t5540. Signed-off-by: Jeff King --- http-push.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/http-push.c b/http-push.c index 52c53928a9..451f7d14bb 100644 --- a/http-push.c +++ b/http-push.c @@ -437,9 +437,11 @@ static void start_move(struct transfer_request *request) if (start_active_slot(slot)) { request->slot = slot; request->state = RUN_MOVE; + request->headers = dav_headers; } else { request->state = ABORTED; FREE_AND_NULL(request->url); + curl_slist_free_all(dav_headers); } } @@ -1398,6 +1400,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock) if (start_active_slot(slot)) { run_active_slot(slot); strbuf_release(&out_buffer.buf); + curl_slist_free_all(dav_headers); if (results.curl_result != CURLE_OK) { fprintf(stderr, "PUT error: curl result=%d, HTTP code=%ld\n", @@ -1407,6 +1410,7 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock) } } else { strbuf_release(&out_buffer.buf); + curl_slist_free_all(dav_headers); fprintf(stderr, "Unable to start PUT request\n"); return 0; } @@ -1516,6 +1520,7 @@ static void update_remote_info_refs(struct remote_lock *lock) results.curl_result, results.http_code); } } + curl_slist_free_all(dav_headers); } strbuf_release(&buffer.buf); } From patchwork Tue Sep 24 22:06:38 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811277 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 44F806E614 for ; Tue, 24 Sep 2024 22:06:39 +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=1727215601; cv=none; b=hKHebCCvAo6SihyKVa8xostpLUEC1q/oj7H7u621tp4rKjkMs0CVfEt16JcRIV3FJIy2qh71GOevCLqaR1WMVfVLXjhCKSv84NfXOcNeFxIGUDWSLAIDiOwroBt4M//09ThdMKK3U+qOaosP1LQIofwLY65/M44EEFO00WgFt0Q= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215601; c=relaxed/simple; bh=yBY+5pmQZU24WMj+cv0WL0yzePbHRc2BXA9rC6nGHWM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ERCFm5pvJAbPUGwgi9SFhnrKEvA0nMCCyL7YIt54UiQ+OPphkSjNmHqlb8JuuL8D8G6K3mbyh6KcP9/g7d5KH3DHR+fKgXoUnuvd/QRVTjtW0s9lEonskua7gDSZ5ihqFqB/mX/Xez76UVoU1SSeBzJdRXVNdGPoWgZJ0CHyS4s= 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 15588 invoked by uid 109); 24 Sep 2024 22:06:39 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:06:39 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18818 invoked by uid 111); 24 Sep 2024 22:06:38 -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; Tue, 24 Sep 2024 18:06:38 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:06:38 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 22/28] http-push: free transfer_request dest field Message-ID: <20240924220638.GV1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> When we issue a PUT request, we store the destination in the "dest" field by detaching from a strbuf. But we never free the result, causing a leak. We can address this in the release_request() function. But note that we also need to initialize it to NULL, as most other request types do not set it at all. Curiously there are _two_ functions to initialize a transfer_request struct. Adding the initialization only to add_fetch_request() seems to be enough for t5540, but I won't pretend to understand why. Rather than just adding "request->dest = NULL" in both spots, let's zero the whole struct. That addresses this problem, as well as any future ones (and it can't possibly hurt, as by definition we'd be hitting uninitialized memory previously). This fixes several leaks noticed by t5540. Signed-off-by: Jeff King --- http-push.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/http-push.c b/http-push.c index 451f7d14bb..9aa4d11ccd 100644 --- a/http-push.c +++ b/http-push.c @@ -514,6 +514,7 @@ static void release_request(struct transfer_request *request) } free(request->url); + free(request->dest); free(request); } @@ -651,11 +652,8 @@ static void add_fetch_request(struct object *obj) return; obj->flags |= FETCHING; - request = xmalloc(sizeof(*request)); + CALLOC_ARRAY(request, 1); request->obj = obj; - request->url = NULL; - request->lock = NULL; - request->headers = NULL; request->state = NEED_FETCH; request->next = request_queue_head; request_queue_head = request; @@ -687,11 +685,9 @@ static int add_send_request(struct object *obj, struct remote_lock *lock) } obj->flags |= PUSHING; - request = xmalloc(sizeof(*request)); + CALLOC_ARRAY(request, 1); request->obj = obj; - request->url = NULL; request->lock = lock; - request->headers = NULL; request->state = NEED_PUSH; request->next = request_queue_head; request_queue_head = request; From patchwork Tue Sep 24 22:08:49 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811278 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 7F520146A7A for ; Tue, 24 Sep 2024 22:08: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=1727215733; cv=none; b=lIGGJfaWfBg3WQAEJiRkBUvJgrPi9luBofbHzAXfCDO4HY1SolhEYRlOf2+NNCkdfsxLUXmH5sJa0OtrZaMPriozRYRHIXWQTSHB8svyVOSXqxFvaZBFQsQldH3frQw5PhzWJAl1AXYXCmY3JOpTOx3n9G4olFBRX2h0o77OLYw= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215733; c=relaxed/simple; bh=rwLyHuWsqVkH1+QthS0qbZpIfN9+w0p04tHr1gWVzPg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=nYdBCA63to+VCSB2xjaP3E9dBEQ7BUROi1+/XJeEb7r7nvtf2wQgRZ6YdkB/yKP358JIimCMOX6C1hAJnaWvI6qQQGwlKxaMybHStsJrqm1MqQq9IbPH61C++lycDEuLzYfIkVsfOxvtwSRnD5q9iL1Z1cPhVcUqyI7B8RRksEY= 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 15599 invoked by uid 109); 24 Sep 2024 22:08:50 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:08:50 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18836 invoked by uid 111); 24 Sep 2024 22:08:50 -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; Tue, 24 Sep 2024 18:08:50 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:08:49 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 23/28] http-push: free transfer_request strbuf Message-ID: <20240924220849.GW1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> When we issue a PUT, we initialize and fill a strbuf embedded in the transfer_request struct. But we never release this buffer, causing a leak. We can fix this by adding a strbuf_release() call to release_request(). If we stopped there, then non-PUT requests would try to release a zero-initialized strbuf. This works OK in practice, but we should try to follow the strbuf API more closely. So instead, we'll always initialize the strbuf when we create the transfer_request struct. That in turn means switching the strbuf_init() call in start_put() to a simple strbuf_grow(). This leak is triggered in t5540. Signed-off-by: Jeff King --- http-push.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/http-push.c b/http-push.c index 9aa4d11ccd..8acdb3f265 100644 --- a/http-push.c +++ b/http-push.c @@ -375,7 +375,7 @@ static void start_put(struct transfer_request *request) /* Set it up */ git_deflate_init(&stream, zlib_compression_level); size = git_deflate_bound(&stream, len + hdrlen); - strbuf_init(&request->buffer.buf, size); + strbuf_grow(&request->buffer.buf, size); request->buffer.posn = 0; /* Compress it */ @@ -515,6 +515,7 @@ static void release_request(struct transfer_request *request) free(request->url); free(request->dest); + strbuf_release(&request->buffer.buf); free(request); } @@ -655,6 +656,7 @@ static void add_fetch_request(struct object *obj) CALLOC_ARRAY(request, 1); request->obj = obj; request->state = NEED_FETCH; + strbuf_init(&request->buffer.buf, 0); request->next = request_queue_head; request_queue_head = request; @@ -689,6 +691,7 @@ static int add_send_request(struct object *obj, struct remote_lock *lock) request->obj = obj; request->lock = lock; request->state = NEED_PUSH; + strbuf_init(&request->buffer.buf, 0); request->next = request_queue_head; request_queue_head = request; From patchwork Tue Sep 24 22:09: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: 13811279 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 EEE6E13D89D for ; Tue, 24 Sep 2024 22:09: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=1727215780; cv=none; b=UzZsmlqnTLHJMar1lenH9HtRNw/LbIrsslnSR0SdfSYQCweRWFUUC0tEg9CM7FtvEgUyabV+xvwBWP3fyArO92g7BNYLwXUxaQ6GDKH2PaxJXrRqRpFOyoYaUfn/EB07o/1EjP/TI7ambK+RMcnrI+pxfWbiRK1SEsO+37t9E5g= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215780; c=relaxed/simple; bh=miXxgs9GyeQj2Xdgiz9jr3pUjnWnqHUfHhk7VrdzLAw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=sDxbWYrlPaET4tKAwzlaIlETtVWDhMThZnD2uCwt6IVE4oyfsizivq5+mf36PLKqRIDLJuHhpTd3a6sIlh06RYZROg2lHISunE+9DanWBSI7XVvEEELRiwzKcQb2p6HRli13wZOGCDevFyNyCUymZfeXM/PJrDPec+Va8EzuHz4= 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 15605 invoked by uid 109); 24 Sep 2024 22:09:38 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:09:38 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18840 invoked by uid 111); 24 Sep 2024 22:09:37 -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; Tue, 24 Sep 2024 18:09:37 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:09:37 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 24/28] http-push: free remote_ls_ctx.dentry_name Message-ID: <20240924220937.GX1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> The remote_ls_ctx struct has dentry_name string, which is filled in with a heap allocation in the handle_remote_ls_ctx() XML callback. After the XML parse is done in remote_ls(), we should free the string to avoid a leak. This fixes several leaks found by running t5540. Signed-off-by: Jeff King --- http-push.c | 1 + 1 file changed, 1 insertion(+) diff --git a/http-push.c b/http-push.c index 8acdb3f265..2e1c6851bb 100644 --- a/http-push.c +++ b/http-push.c @@ -1183,6 +1183,7 @@ static void remote_ls(const char *path, int flags, } free(ls.path); + free(ls.dentry_name); free(url); strbuf_release(&out_buffer.buf); strbuf_release(&in_buffer); From patchwork Tue Sep 24 22:09: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: 13811282 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 2AFDA12E1C2 for ; Tue, 24 Sep 2024 22:09: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=1727215797; cv=none; b=eA9mVrcciFEzuMyMkEHehNUNTOzxWN2gkfkBZG2DOZGkK12uu4S5ttAyAD8O7VQ94tMPMECUUP75QDJ7bnYThvUF8B2KOM++9RpMLJEFAJNKkTNeISN/7/NAJn2AsRdJgDdcDA7+xtVeRCsjMyMMsGbJM5+e3k8Io7kCuIg49jg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215797; c=relaxed/simple; bh=y7L5dFI6s5gyRGCtmhZRHU98xbfYXuLynF50+Y22aYY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=WsRVpU+yN+a6kBVzwDmDZaARJtCf+KQqn61Ae3jWGh9jhW9AwyGpgNyj+58xWpng2uBH9QVBz38QyyQmSVreiXIJ9ln8g/Jn9dNEPZZJRoTj7SQ7TT7LFyHEU8ScQVH1jxjZNb73tA1kHhdlBncF8R+svJ6cup9n5ogTzs1Upg8= 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 15611 invoked by uid 109); 24 Sep 2024 22:09:55 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:09:55 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18844 invoked by uid 111); 24 Sep 2024 22:09:54 -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; Tue, 24 Sep 2024 18:09:54 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:09:54 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 25/28] http-push: free xml_ctx.cdata after use Message-ID: <20240924220954.GY1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> When we ask libexpat to parse XML data, we sometimes set xml_cdata as a CharacterDataHandler callback. This fills in an allocated string in the xml_ctx struct which we never free, causing a leak. I won't pretend to understand the purpose of the field, but it looks like it is used by other callbacks during the parse. At any rate, we never look at it again after XML_Parse() returns, so we should be OK to free() it then. This fixes several leaks triggered by t5540. Signed-off-by: Jeff King --- http-push.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http-push.c b/http-push.c index 2e1c6851bb..1146d7c6fe 100644 --- a/http-push.c +++ b/http-push.c @@ -913,6 +913,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout) result = XML_Parse(parser, in_buffer.buf, in_buffer.len, 1); free(ctx.name); + free(ctx.cdata); if (result != XML_STATUS_OK) { fprintf(stderr, "XML error: %s\n", XML_ErrorString( @@ -1170,6 +1171,7 @@ static void remote_ls(const char *path, int flags, result = XML_Parse(parser, in_buffer.buf, in_buffer.len, 1); free(ctx.name); + free(ctx.cdata); if (result != XML_STATUS_OK) { fprintf(stderr, "XML error: %s\n", From patchwork Tue Sep 24 22:10:38 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811283 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 E79036E614 for ; Tue, 24 Sep 2024 22:10:39 +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=1727215841; cv=none; b=hCVxE19JwmtiDiZLiMAw4ZUijO6i0BqgrHUkpDGsB1AWTR4O7cDHg5PXCVB86O3G8TNN+ne8uWCeqengbpm9uFh210a3pRJkTsqqQPAI9BI+566p5fAyEDYkjpC6biAS4E7RUuoQ97j7GRyOqx9G1ofnahLl2jSeKQ3epldGWV8= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215841; c=relaxed/simple; bh=8ceUppMXa8nKrfI5M0LOZQmzMmDDkMVhyV+Y65MmwB4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=UkGX6LFxEohH5uOe7pxezw8ibPV1AVtm+TZKZeB6Z1De4jRANT9qpIZgqmieikiJD9E8m1n8kxwkvnGyeQ+t6X7J+PsSM6E0tKmgSGyjE0Ygt4322wq1gJdb5gccIddeyHRMHtqEmXyLj+7atoFBP69Gn55CnrNqEl4rGajyIW8= 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 15621 invoked by uid 109); 24 Sep 2024 22:10:39 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:10:39 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18909 invoked by uid 111); 24 Sep 2024 22:10:38 -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; Tue, 24 Sep 2024 18:10:38 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:10:38 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 26/28] http-push: clean up objects list Message-ID: <20240924221038.GZ1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> In http-push's get_delta(), we generate a list of pending objects by recursively processing trees and blobs, adding them to a linked list. And then we iterate over the list, adding a new request for each element. But since we iterate using the list head pointer, at the end it is NULL and all of the actual list structs have been leaked. We can fix this either by using a separate iterator and then calling object_list_free(), or by just freeing as we go. I picked the latter, just because it means we continue to shrink the list as we go, though I'm not sure it matters in practice (we call add_send_request() in the loop, but I don't think it ever looks at the global objects list itself). This fixes several leaks noticed in t5540. Signed-off-by: Jeff King --- http-push.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/http-push.c b/http-push.c index 1146d7c6fe..1cddd2fb37 100644 --- a/http-push.c +++ b/http-push.c @@ -1374,9 +1374,13 @@ static int get_delta(struct rev_info *revs, struct remote_lock *lock) } while (objects) { + struct object_list *next = objects->next; + if (!(objects->item->flags & UNINTERESTING)) count += add_send_request(objects->item, lock); - objects = objects->next; + + free(objects); + objects = next; } return count; From patchwork Tue Sep 24 22:11:13 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811284 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 54AAB6E614 for ; Tue, 24 Sep 2024 22:11:15 +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=1727215876; cv=none; b=N01jm21F2j0/5pb1Xvn75KxmI95yg7q5kjVypI0UIq34gxkoOZwmiDkOZqlY/7hUv3DS6odFAMpcrH0Qq/8rZPp/ne2hZIv4opVTHeWOXkVS7bLSY3ls12iNFo19gM65ujIrpb2m4BNXGyBXM/S6jDHLSwfrJ4w37ChHoIcBs0I= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215876; c=relaxed/simple; bh=uP7tLNkdvwbKUuNvwXSg41B/7dXBuLdAKqpYb4bhU5A=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ChSiLKYKbMYaIFg4F4Z5M/E0+dZiySNIVWyGKwl3n2Vo0NNNr34iAdua0IInHsgVnHkppCIduIKC8++oMYNqDfVA22mOoJpO7ntJpxFGh26S3WZhUlE4mopeHVCwJ0MtSuoOLjzHMz12AlmqK6tpzdrAspdoELCgTN9Y3jvFy0w= 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 15627 invoked by uid 109); 24 Sep 2024 22:11:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:11:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18915 invoked by uid 111); 24 Sep 2024 22:11:14 -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; Tue, 24 Sep 2024 18:11:14 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:11:13 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 27/28] http-push: clean up loose request when falling back to packed Message-ID: <20240924221113.GA1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> In http-push's finish_request(), if we fail a loose object request we may fall back to trying a packed request. But if we do so, we leave the http_loose_object_request in place, leaking it. We can fix this by always cleaning it up. Note that the obj_req pointer here (which we'll set to NULL) is a copy of the request->userData pointer, which will now point to freed memory. But that's OK. We'll either release the parent request struct entirely, or we'll convert it into a packed request, which will overwrite userData itself. This leak is found by t5540, but it's not quite leak-free yet. Signed-off-by: Jeff King --- http-push.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/http-push.c b/http-push.c index 1cddd2fb37..b36b1f9e35 100644 --- a/http-push.c +++ b/http-push.c @@ -582,9 +582,10 @@ static void finish_request(struct transfer_request *request) if (obj_req->rename == 0) request->obj->flags |= (LOCAL | REMOTE); + release_http_object_request(&obj_req); + /* Try fetching packed if necessary */ if (request->obj->flags & LOCAL) { - release_http_object_request(&obj_req); release_request(request); } else start_fetch_packed(request); From patchwork Tue Sep 24 22:12:39 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13811285 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 4B11713CFA5 for ; Tue, 24 Sep 2024 22:12:41 +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=1727215962; cv=none; b=BGHgrHO1VMe6tT3ACjgSr4R2abvx9A+HVH+MbsfYnvnRW6qf5XOmggBB6Sr7ulmuMo3inbRWsR54rP25suSN2pirKPIytu1gHzXvZzS8lplOBSKJ7eQsd5KWB3MKfaPIisyjYKbMuXW9BLjEBfkWEQORXhPZCqj2l3ZNa3Ixxkg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1727215962; c=relaxed/simple; bh=BJ2VvkRTSWAULhWXOP2d/ZtEQ/tzI8NtRvnRHG537F0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Dk+HgeRj+oZuwz28bGa5cyCVOuNoa+jaibtX0NdL0/HrtgU0Z6/S5oyg6tbRf6IALeIefyXqqAq0ztuxly9nV8Esex36h4tgRO0HbQp7ZNVOvvvo63/AW7b2KwDtw2q2fekzz6YJIr9tB36KEJSBvfDFT9/4l0hbHBOthkGXlR8= 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 15637 invoked by uid 109); 24 Sep 2024 22:12:40 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 24 Sep 2024 22:12:40 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18919 invoked by uid 111); 24 Sep 2024 22:12:40 -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; Tue, 24 Sep 2024 18:12:40 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 24 Sep 2024 18:12:39 -0400 From: Jeff King To: git@vger.kernel.org Cc: Patrick Steinhardt Subject: [PATCH 28/28] http-push: clean up local_refs at exit Message-ID: <20240924221239.GB1143820@coredump.intra.peff.net> References: <20240924214930.GA1143523@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: <20240924214930.GA1143523@coredump.intra.peff.net> We allocate a list of ref structs from get_local_heads() but never clean it up. We should do so before exiting to avoid complaints from the leak-checker. Note that we have to initialize it to NULL, because there's one code path that can jump to the cleanup label before we assign to it. Fixing this lets us mark t5540 as leak-free. Curiously building with SANITIZE=leak and gcc does not seem to find this problem, but switching to clang does. It seems like a fairly obvious leak, though. I was curious that the matching remote_refs did not have the same leak. But that is because we store the list in a global variable, so it's still reachable after we exit. Arguably we could treat it the same as future-proofing, but I didn't bother (now that the script is marked leak-free, anybody moving it to a stack variable will notice). Signed-off-by: Jeff King --- http-push.c | 3 ++- t/t5540-http-push-webdav.sh | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/http-push.c b/http-push.c index b36b1f9e35..aad89f2eab 100644 --- a/http-push.c +++ b/http-push.c @@ -1719,7 +1719,7 @@ int cmd_main(int argc, const char **argv) int rc = 0; int i; int new_refs; - struct ref *ref, *local_refs; + struct ref *ref, *local_refs = NULL; CALLOC_ARRAY(repo, 1); @@ -1997,6 +1997,7 @@ int cmd_main(int argc, const char **argv) } refspec_clear(&rs); + free_refs(local_refs); return rc; } diff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh index 37db3dec0c..27389b0908 100755 --- a/t/t5540-http-push-webdav.sh +++ b/t/t5540-http-push-webdav.sh @@ -10,6 +10,7 @@ This test runs various sanity checks on http-push.' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh if git http-push > /dev/null 2>&1 || [ $? -eq 128 ]