From patchwork Tue Apr 2 20:05:17 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13614554 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 ECB3315AAA1 for ; Tue, 2 Apr 2024 20:05:18 +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=1712088321; cv=none; b=peRLeSvosHHhrjVdPID03Zpk4MlkQafvkYlPPtrGqe4oWSdGjUSbnxEana+wQH8lvwwDJlGSiqTCZnKRsHsRAmhKmIlstuvHs/khFdO/aZG/dUgW3fyQ5dQruU9PJKYnEXFT9sKo8MMSPJNLbtiQsjLYtOXTa5Gvk+Ro9AGc23I= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712088321; c=relaxed/simple; bh=VF0rIT3l/R8h3qVYoSelzUCIa16Zw7+3Snq4gfn2zbw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=USIcfBLTo582ZWuBgryQRhmTXYNKVKhbpx84w+b6PfBiNkLO4Zjuod12KUOFo7ipUL6lwuMHsAEGPBmHJtoseiXf8WWj4z5KIqwU7JWTV9t6LpB22JQD5731mZ98Om4Wm5dvB4RUpS2DKSAf2y2qItvz+/jKae1qlZa+B58vaxA= 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 7288 invoked by uid 109); 2 Apr 2024 20:05:18 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 02 Apr 2024 20:05:18 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1105 invoked by uid 111); 2 Apr 2024 20:05:19 -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, 02 Apr 2024 16:05:19 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 2 Apr 2024 16:05:17 -0400 From: Jeff King To: git@vger.kernel.org Cc: Daniel Stenberg Subject: [PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle Message-ID: <20240402200517.GA875182@coredump.intra.peff.net> References: <20240402200254.GA874754@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: <20240402200254.GA874754@coredump.intra.peff.net> In get_active_slot(), we return a CURL handle that may have been used before (reusing them is good because it lets curl reuse the same connection across many requests). We set a few curl options back to defaults that may have been modified by previous requests. We reset POSTFIELDS to NULL, but do not reset POSTFIELDSIZE (which defaults to "-1"). This usually doesn't matter because most POSTs will set both fields together anyway. But there is one exception: when handling a large request in remote-curl's post_rpc(), we don't set _either_, and instead set a READFUNCTION to stream data into libcurl. This can interact weirdly with a stale POSTFIELDSIZE setting, because curl will assume it should read only some set number of bytes from our READFUNCTION. However, it has worked in practice because we also manually set a "Transfer-Encoding: chunked" header, which libcurl uses as a clue to set the POSTFIELDSIZE to -1 itself. So everything works, but we're better off resetting the size manually for a few reasons: - there was a regression in curl 8.7.0 where the chunked header detection didn't kick in, causing any large HTTP requests made by Git to fail. This has since been fixed (but not yet released). In the issue, curl folks recommended setting it explicitly to -1: https://github.com/curl/curl/issues/13229#issuecomment-2029826058 and it indeed works around the regression. So even though it won't be strictly necessary after the fix there, this will help folks who end up using the affected libcurl versions. - it's consistent with what a new curl handle would look like. Since get_active_slot() may or may not return a used handle, this reduces the possibility of heisenbugs that only appear with certain request patterns. Note that the recommendation in the curl issue is to actually drop the manual Transfer-Encoding header. Modern libcurl will add the header itself when streaming from a READFUNCTION. However, that code wasn't added until 802aa5ae2 (HTTP: use chunked Transfer-Encoding for HTTP_POST if size unknown, 2019-07-22), which is in curl 7.66.0. We claim to support back to 7.19.5, so those older versions still need the manual header. Signed-off-by: Jeff King --- http.c | 1 + 1 file changed, 1 insertion(+) diff --git a/http.c b/http.c index e73b136e58..3d80bd6116 100644 --- a/http.c +++ b/http.c @@ -1452,6 +1452,7 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL); curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL); curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL); + curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, -1L); curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0); curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); From patchwork Tue Apr 2 20:06: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: 13614555 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 4F32315CD72 for ; Tue, 2 Apr 2024 20:06: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=1712088364; cv=none; b=tBKNT7Bh5eKnRwZ5Jhnrarn7ZB17thv6NnVIVyKsljMUt3eM+BaSWTkvhslsY38HwDfjynKUVv5zz0kpVRMJZDTmLluWrOvxeZKh9FqkcEljXiUcYx0B1fGCI25I5UF0jv7NsTF6uFh6SLeba2qZeandYDGZQ9SrjJ01ONb5k6w= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712088364; c=relaxed/simple; bh=p05KChR3sxLxXfYCqerzwB+SJeU2RZbMsaPxr73tYzU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=GuBzMWEPz70G3kwv7pwmxUzod3GoQT95xHfvRfUZMb5BBvd7/t5nkXNbsdTVTj32OV64Vr/M1zD51/ZEqrH8Vue5mlD6bXXrXlQa9Oaycm34JssbHvlEq9e69CPSeob32OKhuqs7GjJ/feuhOt4QRMSaOuiHTs/2a3rwP/3HQAE= 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 7296 invoked by uid 109); 2 Apr 2024 20:06:01 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 02 Apr 2024 20:06:01 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1113 invoked by uid 111); 2 Apr 2024 20:06: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, 02 Apr 2024 16:06:03 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 2 Apr 2024 16:06:00 -0400 From: Jeff King To: git@vger.kernel.org Cc: Daniel Stenberg Subject: [PATCH 2/2] INSTALL: bump libcurl version to 7.21.3 Message-ID: <20240402200600.GB875182@coredump.intra.peff.net> References: <20240402200254.GA874754@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: <20240402200254.GA874754@coredump.intra.peff.net> Our documentation claims we support curl versions back to 7.19.5. But we can no longer compile with that version since adding an unconditional use of CURLOPT_RESOLVE in 511cfd3bff (http: add custom hostname to IP address resolutions, 2022-05-16). That feature wasn't added to libcurl until 7.21.3. We could add #ifdefs to make this work back to 7.19.5. But given that nobody noticed the compilation failure in the intervening two years, it makes more sense to bump the version in the documentation to 7.21.3 (which is itself over 13 years old). We could perhaps go forward even more (which would let us drop some cruft from git-curl-compat.h), but this should be an obviously safe jump, and we can move forward later. Note that user-visible syntax for CURLOPT_RESOLVE has grown new features in subsequent curl versions. Our documentation mentions "+" and "-" entries, which require more recent versions than 7.21.3. We could perhaps clarify that in our docs, but it's probably not worth cluttering them with restrictions of ancient curl versions. Signed-off-by: Jeff King --- INSTALL | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/INSTALL b/INSTALL index c6fb240c91..2a46d04592 100644 --- a/INSTALL +++ b/INSTALL @@ -139,7 +139,7 @@ Issues of note: not need that functionality, use NO_CURL to build without it. - Git requires version "7.19.5" or later of "libcurl" to build + Git requires version "7.21.3" or later of "libcurl" to build without NO_CURL. This version requirement may be bumped in the future. From patchwork Fri Apr 5 20:04:51 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13619376 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 8F6D816EBFD for ; Fri, 5 Apr 2024 20:04:53 +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=1712347495; cv=none; b=q7NhwJOyVWRxR2yAfoX9JI1ao5JTvEP/PiWsQVD28xgYPk9sx/mogAldKufSparmeWP1Lge8lLgDwpu9Kzq1NWAD/6KalLHqvhvqhso/5uXiPNrHp0tCOtyKFL1164frlKaEBQAl8fKPsmQHqI+yk+KFWk8s+y5s0fL/4ag5YW4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712347495; c=relaxed/simple; bh=JLCLOfhtrGQU+J/EkcE77BasHE6lvjNDZCNjMhSmLwA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=l1D+NK8Q5uVufZS+4/DRuCdOZ3Xdx28A0qIa+QX3jDu9an8D56q44JvvEkHReDp5SFGBPmyucgqpjYcHsS2aCuErrXPZtGp/W02rvBvtfUUfD354r8dTM2U9H4tbwbkyQulck0vFa7Y9EV1Pnnn88+mM6Nzlvh6dnwVgzIjwGiw= 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 20164 invoked by uid 109); 5 Apr 2024 20:04:52 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Fri, 05 Apr 2024 20:04:52 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 32191 invoked by uid 111); 5 Apr 2024 20:04: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; Fri, 05 Apr 2024 16:04:54 -0400 Authentication-Results: peff.net; auth=none Date: Fri, 5 Apr 2024 16:04:51 -0400 From: Jeff King To: git@vger.kernel.org Cc: Daniel Stenberg , Junio C Hamano Subject: [PATCH 3/2] remote-curl: add Transfer-Encoding header only for older curl Message-ID: <20240405200451.GA3700566@coredump.intra.peff.net> References: <20240330000212.GA1261238@coredump.intra.peff.net> <2n7sn76-p413-5632-4o2s-o5n2p1rqnr5@unkk.fr> <20240402200254.GA874754@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: <20240402200254.GA874754@coredump.intra.peff.net> On Tue, Apr 02, 2024 at 04:02:54PM -0400, Jeff King wrote: > Ultimately the issue will be fixed by moving to a different version of > libcurl, but here's an easy workaround in the meantime, with a small doc > cleanup I found along the way. > > [1/2]: http: reset POSTFIELDSIZE when clearing curl handle > [2/2]: INSTALL: bump libcurl version to 7.21.3 Here's a final patch on top that gives us a workaround for the HTTP/2 half of this. It's a cleaned-up version of what I showed earlier. -- >8 -- Subject: remote-curl: add Transfer-Encoding header only for older curl As of curl 7.66.0, we don't need to manually specify a "chunked" Transfer-Encoding header. Instead, modern curl deduces the need for it in a POST that has a POSTFIELDSIZE of -1 and uses READFUNCTION rather than POSTFIELDS. That version is recent enough that we can't just drop the header; we need to do so conditionally. Since it's only a single line, it seems like the simplest thing would just be to keep setting it unconditionally (after all, the #ifdefs are much longer than the actual code). But there's another wrinkle: HTTP/2. Curl may choose to use HTTP/2 under the hood if the server supports it. And in that protocol, we do not use the chunked encoding for streaming at all. Most versions of curl handle this just fine by recognizing and removing the header. But there's a regression in curl 8.7.0 and 8.7.1 where it doesn't, and large requests over HTTP/2 are broken (which t5559 notices). That regression has since been fixed upstream, but not yet released. Make the setting of this header conditional, which will let Git work even with those buggy curl versions. And as a bonus, it serves as a reminder that we can eventually clean up the code as we bump the supported curl versions. Signed-off-by: Jeff King --- Manually tested against curl 8.7.0 (where skipping the header fixes t5559), as well as 7.65.0 (which continues to pass t5551, but would fail if we didn't set the header). git-curl-compat.h | 9 +++++++++ remote-curl.c | 3 +++ 2 files changed, 12 insertions(+) diff --git a/git-curl-compat.h b/git-curl-compat.h index fd96b3cdff..e1d0bdd273 100644 --- a/git-curl-compat.h +++ b/git-curl-compat.h @@ -126,6 +126,15 @@ #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS #endif +/** + * Versions before curl 7.66.0 (September 2019) required manually setting the + * transfer-encoding for a streaming POST; after that this is handled + * automatically. + */ +#if LIBCURL_VERSION_NUM < 0x074200 +#define GIT_CURL_NEED_TRANSFER_ENCODING_HEADER +#endif + /** * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0, * released in August 2022. diff --git a/remote-curl.c b/remote-curl.c index 31b02b8840..0b6d7815fd 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1,4 +1,5 @@ #include "git-compat-util.h" +#include "git-curl-compat.h" #include "config.h" #include "environment.h" #include "gettext.h" @@ -955,7 +956,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece /* The request body is large and the size cannot be predicted. * We must use chunked encoding to send it. */ +#ifdef GIT_CURL_NEED_TRANSFER_ENCODING_HEADER headers = curl_slist_append(headers, "Transfer-Encoding: chunked"); +#endif rpc->initial_buffer = 1; curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out); curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);