| Message ID | cover-0.5-00000000000-20210721T220402Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drop support for ancient curl | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > This series simplifies the http code by dropping support for curl > versions older than 7.19.4, released in March 2009. Thanks. Will take a look and may have some comments, but I'd prefer to see an Ack from Peff as well. > Jeff King (3): > http: drop support for curl < 7.11.1 > http: drop support for curl < 7.16.0 > http: drop support for curl < 7.19.4 > > Ævar Arnfjörð Bjarmason (2): > http: drop support for curl < 7.19.3 and < 7.16.4 (again) > http: rename CURLOPT_FILE to CURLOPT_WRITEDATA > > http-push.c | 29 +-------- > http-walker.c | 14 +---- > http.c | 169 ++------------------------------------------------ > http.h | 46 -------------- > imap-send.c | 4 -- > remote-curl.c | 11 +--- > 6 files changed, 10 insertions(+), 263 deletions(-) > > Range-diff against v1: > 1: 8793735cc2c ! 1: dcbb6f95652 http: drop support for curl < 7.11.1 > @@ Metadata > ## Commit message ## > http: drop support for curl < 7.11.1 > > - Recent versions of Git will not build with curl older than > - 7.11.1 due to (at least) two issues: > + Drop support for this ancient version of curl and simplify the code by > + allowing us get rid of some "#ifdef"'s. > + > + Git will not build with vanilla curl older than 7.11.1 due to (at > + least) two issues: > > - our use of CURLOPT_POSTFIELDSIZE in 37ee680d9b > (http.postbuffer: allow full range of ssize_t values, > @@ Commit message > obvious benefit is that we'll have fewer conditional bits > cluttering the code. > > - But more importantly, we're doing a disservice to users to > - pretend that Git works with old versions. It's clear that > - nobody is testing modern Git with such old versions of curl > - (we've had 3 released versions with the CURLPROTO issue > - without a report of anyone seeing the breakage in the wild). > - And there are a lot of subtle ways we could be getting this > - wrong (for instance, curl prior to 7.17.0 did not copy > - string arguments to curl_easy_setopt(), which means that > - using an old copy of curl could produce use-after-free > - bugs that are not present with more recent versions). > - > This patch drops all #ifdefs that reference older versions > (note that curl's preprocessor macros are in hex, so we're > looking for 070b01, not 071101). > > + Signed-off-by: Jeff King <peff@peff.net> > + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > + > ## http.c ## > @@ > static struct trace_key trace_curl = TRACE_KEY_INIT(CURL); > 2: 15638cd1856 ! 2: 1c9f3bc031b http: drop support for curl < 7.16.0 > @@ Metadata > ## Commit message ## > http: drop support for curl < 7.16.0 > > - As discussed in the previous commit, Git is not well-tested > - with old versions of curl (and in fact since v2.12.0 does > - not even compile with versions older than 7.19.4). Let's > - stop pretending we support curl that old and drop any > - now-obslete #ifdefs. > + In the last commit we dropped support for curl < 7.11.1, let's > + continue that and drop support for versions older than 7.16.0. This > + allows us to get rid of some now-obsolete #ifdefs. > > - Choosing 7.16.0 is a somewhat arbitrary cutoff, but: > + Choosing 7.16.0 is a somewhat arbitrary cutoff: > > - 1. it came out in October of 2006, over 10 years ago. > - Besides being a nice round number, it's a common > - end-of-life support period, even for conservative > + 1. It came out in October of 2006, almost 15 years ago. > + Besides being a nice round number, around 10 years is > + a common end-of-life support period, even for conservative > distributions. > > - 2. that version introduced the curl_multi interface, which > + 2. That version introduced the curl_multi interface, which > gives us a lot of bang for the buck in removing #ifdefs > > + RHEL 5 came with curl 7.15.5[1] (released in August 2006). RHEL 5's > + extended life cycle program ended on 2020-11-30[1]. RHEL 6 comes with > + curl 7.19.7 (released in November 2009), and RHEL 7 comes with > + 7.29.0 (released in February 2013). > + > + 1. http://lore.kernel.org/git/873e1f31-2a96-5b72-2f20-a5816cad1b51@jupiterrise.com > + > + Signed-off-by: Jeff King <peff@peff.net> > + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > + > ## http-push.c ## > @@ http-push.c: static void curl_setup_http(CURL *curl, const char *url, > curl_easy_setopt(curl, CURLOPT_INFILE, buffer); > @@ http.h: void finish_all_active_slots(void); > void http_init(struct remote *remote, const char *url, > int proactive_auth); > > + ## imap-send.c ## > +@@ imap-send.c: static int curl_append_msgs_to_imap(struct imap_server_conf *server, > + if (cred.username) { > + if (res == CURLE_OK) > + credential_approve(&cred); > +-#if LIBCURL_VERSION_NUM >= 0x070d01 > + else if (res == CURLE_LOGIN_DENIED) > +-#else > +- else > +-#endif > + credential_reject(&cred); > + } > + > + > ## remote-curl.c ## > @@ remote-curl.c: static size_t rpc_out(void *ptr, size_t eltsize, > return avail; > 3: 335046de7bc ! 3: faae88b7fec http: drop support for curl < 7.19.4 > @@ Metadata > ## Commit message ## > http: drop support for curl < 7.19.4 > > - Since v2.12.0, Git does not compile with versions of curl > - older than 7.19.4. That version of curl is about 8 years > - old. This means it may still be used in some distributions > - with long-running support periods. But the fact that we > - haven't received a single bug report about the compile-time > - breakage implies that nobody cares about building recent > - releases on such platforms. > + In the last commit we dropped support for curl < 7.16.0, let's > + continue that and drop support for versions older than 7.19.4. This > + allows us to simplify the code by getting rid of some "#ifdef"'s. > > - As discussed in the previous two commits, this cleans up the > - code and gives a more realistic signal to users about which > - versions of Git are actually tested (in particular, this > - moves us past the potential use-after-free issues with curl > - older than 7.17.0). > + Git was broken with vanilla curl < 7.19.4 from v2.12.0 until > + v2.15.0. Compiling with it was broken by using CURLPROTO_* outside any > + "#ifdef" in aeae4db174 (http: create function to get curl allowed > + protocols, 2016-12-14), and fixed in v2.15.0 in f18777ba6ef (http: fix > + handling of missing CURLPROTO_*, 2017-08-11). > + > + It's unclear how much anyone was impacted by that in practice, since > + as noted in [1] RHEL versions using curl older than that still > + compiled, because RedHat backported some features. Perhaps other > + vendors did the same. > + > + Still, it's one datapoint indicating that it wasn't in active use at > + the time. That (the v2.12.0 release) was in Feb 24, 2017, with v2.15.0 > + on Oct 30, 2017, it's now mid-2021. > + > + 1. http://lore.kernel.org/git/c8a2716d-76ac-735c-57f9-175ca3acbcb0@jupiterrise.com; > + followed-up by f18777ba6ef (http: fix handling of missing CURLPROTO_*, > + 2017-08-11) > + > + Signed-off-by: Jeff King <peff@peff.net> > + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > ## http.c ## > @@ http.c: static int min_curl_sessions = 1; > @@ http.c: static void var_override(const char **var, char *value) > } > > static void init_curl_proxy_auth(CURL *result) > +@@ http.c: void setup_curl_trace(CURL *handle) > + curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL); > + } > + > +-#ifdef CURLPROTO_HTTP > + static long get_curl_allowed_protocols(int from_user) > + { > + long allowed_protocols = 0; > +@@ http.c: static long get_curl_allowed_protocols(int from_user) > + > + return allowed_protocols; > + } > +-#endif > + > + #if LIBCURL_VERSION_NUM >=0x072f00 > + static int get_curl_http_version_opt(const char *version_string, long *opt) > @@ http.c: static CURL *get_curl_handle(void) > } > > @@ http.c: static CURL *get_curl_handle(void) > -#if LIBCURL_VERSION_NUM >= 0x071301 > curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL); > -#elif LIBCURL_VERSION_NUM >= 0x071101 > - curl_easy_setopt(result, CURLOPT_POST301, 1); > +- curl_easy_setopt(result, CURLOPT_POST301, 1); > -#endif > -#ifdef CURLPROTO_HTTP > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, > 4: e049f37357a < -: ----------- http: #error on too-old curl > -: ----------- > 4: 9a30e92520c http: drop support for curl < 7.19.3 and < 7.16.4 (again) > -: ----------- > 5: 64e510b4a6b http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
On 2021-07-21 at 22:22:11, Ævar Arnfjörð Bjarmason wrote: > This series simplifies the http code by dropping support for curl > versions older than 7.19.4, released in March 2009. > > This was last discussed on-list in 2017: > http://lore.kernel.org/git/20170809120024.7phdjzjv54uv5dpz@sigill.intra.peff.net > > My reading of why it didn't get integrated at the time was: > > - The original commit messages are opinionated about git not working > on these versions anyway, as noted in the original thread that's > only true of vanilla curl, but anyone impacted by these issues at > the time was probably using e.g. RHEL, which had backports that > confused the issue. > > - While in 2017 these versions were already ancient, RHEL 5 (released > in 2007) was still seeing some notable production use. > > It finally got "we really mean it now" EOL'd in late 2020 when > extended life-cycle support ended (see > https://access.redhat.com/support/policy/updates/errata). RHEL 6 > does not have a libcurl affected by these changes. > > - It ended with a patch to "error on too-old curl", i.e. to make > compiling on versions older than 7.19.4 an error. I've ejected that > per the discussion about backports confusing that issue. I'm in favor of this series. I'm actually in favor of dropping support for RHEL 6 as well, since there is nobody providing public security support for it, and therefore nobody but people paying Red Hat (that is, not this project) can be expected to safely run it. I also think ten years is about the reasonable maximum lifetime of software. So, with or without those changes, this seems like a good approach to me.
On 22/07/21 05.22, Ævar Arnfjörð Bjarmason wrote: > This series simplifies the http code by dropping support for curl > versions older than 7.19.4, released in March 2009. But INSTALL says: > - "libcurl" library is used by git-http-fetch, git-fetch, and, if > the curl version >= 7.34.0, for git-imap-send. You might also > want the "curl" executable for debugging purposes. If you do not > use http:// or https:// repositories, and do not want to put > patches into an IMAP mailbox, you do not have to have them > (use NO_CURL). I think it's worth mentioning minimal required curl version (7.19.4) there.
On Wed, Jul 21 2021, brian m. carlson wrote: > [[PGP Signed Part:Undecided]] > On 2021-07-21 at 22:22:11, Ævar Arnfjörð Bjarmason wrote: >> This series simplifies the http code by dropping support for curl >> versions older than 7.19.4, released in March 2009. >> >> This was last discussed on-list in 2017: >> http://lore.kernel.org/git/20170809120024.7phdjzjv54uv5dpz@sigill.intra.peff.net >> >> My reading of why it didn't get integrated at the time was: >> >> - The original commit messages are opinionated about git not working >> on these versions anyway, as noted in the original thread that's >> only true of vanilla curl, but anyone impacted by these issues at >> the time was probably using e.g. RHEL, which had backports that >> confused the issue. >> >> - While in 2017 these versions were already ancient, RHEL 5 (released >> in 2007) was still seeing some notable production use. >> >> It finally got "we really mean it now" EOL'd in late 2020 when >> extended life-cycle support ended (see >> https://access.redhat.com/support/policy/updates/errata). RHEL 6 >> does not have a libcurl affected by these changes. >> >> - It ended with a patch to "error on too-old curl", i.e. to make >> compiling on versions older than 7.19.4 an error. I've ejected that >> per the discussion about backports confusing that issue. > > I'm in favor of this series. I'm actually in favor of dropping support > for RHEL 6 as well, since there is nobody providing public security > support for it, and therefore nobody but people paying Red Hat (that is, > not this project) can be expected to safely run it. I also think ten > years is about the reasonable maximum lifetime of software. > > So, with or without those changes, this seems like a good approach to > me. I'll clarify this along with other fixes in a re-roll, but I think our policy shouldn't have anything to do with upstream promises of support, but merely the trade-off of how easy it is for us to support old software & how likely it is that people use it in practice along with git. So as an example we still say we support Perl 5.8, which is ridiculously ancient as far as any notion of upstream security support goes (and as an aside, does have real DoS issues exposed by e.g. the gitweb we ship). But while we could probably bump that to something more modern nowadays in practice we're not a mostly-Perl project, so I haven't found it to be worth it to bump it when working on the relevant code. I'm only using RHEL 5 as a shorthand for a system that's usually the most ancient thing people want to build new gits with in practice. It's just not the case that you can't run RHEL 5 or even RHEL 4 "safely" even today. Upstream has just abandoned it, but that doesn't mean users in the wild have. There's also CentOS, not everyone cares about IBM corporate support policies. E.g. in practice at a past-job I've had to build git using system libcurl in a mixed environment which (and I forget the details) included mostly today's equivalent of RHEL 8 and 7, but there was some system using RHEL 5 in a closet somewhere still using puppet automation. Why? Because (and I forget the details, but this example will do) because it needed to operate some proprietary dongle requiring a RHEL 5 kernel driver that its vendor had since abandoned. There were plans to move away from it, but that was maybe 1-2 years away at the time. Meanwhile I had to build a git across the fleet, and it would be a hassle to need to ship my own libcurl just because this project wanted to have paternalistic version dependency policies. I mean, if it's a matter of supporting that version being painful then fair enough. I had some comments in the 2017 thread (or something linked from it) about needing to package your dependencies not being *that* big a deal. Hence this series, I think on balance the improvement in maintainability of the http code makes it worth it. But let's not justify it with a user not being able to run such software securely, in my example those ancient boxes were externally firewalled, and in any case any practical security issues were probably with some vendor's admin interface on them, not whatever ancient kernel they had. On the other hand there's surely people who are running RHEL 5 today who are running insecure setup, but let's not make it our job to force them to move by virtue of being overly annoying about dependency version requirements. We should have the view that git's critical infrastructure and we should be wary of breaking things. It would also just be counter-productive, the result would probably be that the ancient box wouldn't get an upgraded git, and would still have preventable CVE's in git itself present (e.g. the gitmodules RCE).
On 2021-07-22 at 07:09:59, Ævar Arnfjörð Bjarmason wrote: > I'll clarify this along with other fixes in a re-roll, but I think our > policy shouldn't have anything to do with upstream promises of support, > but merely the trade-off of how easy it is for us to support old > software & how likely it is that people use it in practice along with > git. I don't think I agree. We should try to support major operating systems well provided we can adequately be expected to test on them, and that means that they should have publicly available security support. In other words, a developer on the relevant operating system should be able to test on that OS without paying ongoing money for the privilege of doing so securely. Once an operating system is no longer supported security-wise, we should no longer support it, either, since we can't be expected to test or develop on it securely. Nobody could responsibly run such an image on a CI system or test with it on an Internet-connected computer, so we should no longer consider it worthy of our support. > So as an example we still say we support Perl 5.8, which is ridiculously > ancient as far as any notion of upstream security support goes (and as > an aside, does have real DoS issues exposed by e.g. the gitweb we ship). > > But while we could probably bump that to something more modern nowadays > in practice we're not a mostly-Perl project, so I haven't found it to be > worth it to bump it when working on the relevant code. I've actually argued in favor of bumping the version to 5.14 a long time ago. I can send a patch for that. It has a bunch of nice new features we could take advantage of. > I'm only using RHEL 5 as a shorthand for a system that's usually the > most ancient thing people want to build new gits with in practice. > > It's just not the case that you can't run RHEL 5 or even RHEL 4 "safely" > even today. Upstream has just abandoned it, but that doesn't mean users > in the wild have. There's also CentOS, not everyone cares about IBM > corporate support policies. Yes, and CentOS has dropped support earlier than Red Hat has. Just because users want to run new versions of Git on systems that should long ago have been abandoned[0] does not mean we should take the burden of maintaining that code for them. Since they have the source code, they can build and maintain Git on those old systems and apply any necessary patches. If this becomes burdensome, then perhaps the cost of maintaining the system will be an incentive to replace it with a secure system. I am unconvinced that we should make it easier for people to run insecure operating systems because they pose a hazard to the Internet when connected to it. Just because it is behind some firewall doesn't mean that it cannot be compromised, and once it is, it can then become a source of spam and abuse. This is not an idle thought experiment; it does practically happen with great frequency on the Internet today. An unsupported system might be acceptable if it has no network connectivity at all, but then it would not need a newer version of Git. It is not that I have not experienced such load-bearing obsolete systems before: I have, and I have done my best to support them. But I've also been happy to be clear to management and/or customers about what that means in terms of costs and that we were taking a real, substantial risk, and been clear what the consequences were. In no situation, however, did I try to convince outside parties that my obsolete OS was deserving of someone else's maintenance burden or argue that the system should not be replaced as soon as possible. > We should have the view that git's critical infrastructure and we should > be wary of breaking things. It would also just be counter-productive, > the result would probably be that the ancient box wouldn't get an > upgraded git, and would still have preventable CVE's in git itself > present (e.g. the gitmodules RCE). Considering that the machine already has multiple CVEs, probably including root code execution vulnerabilities, I'm not sure how much worse we could make it. It's already trivial to compromise with or without a newer version of Git. [0] I should point out that ten years of support is already extremely generous.
On Thu, Jul 22 2021, brian m. carlson wrote: > [[PGP Signed Part:Undecided]] > On 2021-07-22 at 07:09:59, Ævar Arnfjörð Bjarmason wrote: >> I'll clarify this along with other fixes in a re-roll, but I think our >> policy shouldn't have anything to do with upstream promises of support, >> but merely the trade-off of how easy it is for us to support old >> software & how likely it is that people use it in practice along with >> git. > > I don't think I agree. We should try to support major operating systems > well provided we can adequately be expected to test on them, and that > means that they should have publicly available security support. In > other words, a developer on the relevant operating system should be able > to test on that OS without paying ongoing money for the privilege of doing > so securely. Doesn't drawing that line in the sand for Linux distributions by implication leave out support for Windows, OSX and any other proprietary system? You need to pay for security and other updates for those from day one. > Once an operating system is no longer supported security-wise, we should > no longer support it, either, since we can't be expected to test or > develop on it securely. Nobody could responsibly run such an image on > a CI system or test with it on an Internet-connected computer, so we > should no longer consider it worthy of our support. Yes, I do think we disagree. I just think we should focus narrowly on whether it's a hassle for us to support older libcurl, whether some version of it is packaged with an old OS that's known to be in wide use or not is ultimately just a useful heuristic. >> So as an example we still say we support Perl 5.8, which is ridiculously >> ancient as far as any notion of upstream security support goes (and as >> an aside, does have real DoS issues exposed by e.g. the gitweb we ship). >> >> But while we could probably bump that to something more modern nowadays >> in practice we're not a mostly-Perl project, so I haven't found it to be >> worth it to bump it when working on the relevant code. > > I've actually argued in favor of bumping the version to 5.14 a long time > ago. I can send a patch for that. It has a bunch of nice new features > we could take advantage of. Sure, I'm not opposed. Just noting the in-tree nicer features for us v.s. more aggressive versioning policy for packagers and users (not that Perl 5.14 is aggressive). >> I'm only using RHEL 5 as a shorthand for a system that's usually the >> most ancient thing people want to build new gits with in practice. >> >> It's just not the case that you can't run RHEL 5 or even RHEL 4 "safely" >> even today. Upstream has just abandoned it, but that doesn't mean users >> in the wild have. There's also CentOS, not everyone cares about IBM >> corporate support policies. > > Yes, and CentOS has dropped support earlier than Red Hat has. > > Just because users want to run new versions of Git on systems that > should long ago have been abandoned[0] does not mean we should take the > burden of maintaining that code for them. Since they have the source > code, they can build and maintain Git on those old systems and apply > any necessary patches. If this becomes burdensome, then perhaps the > cost of maintaining the system will be an incentive to replace it with a > secure system. > > I am unconvinced that we should make it easier for people to run > insecure operating systems because they pose a hazard to the Internet > when connected to it. Just because it is behind some firewall doesn't > mean that it cannot be compromised, and once it is, it can then become > a source of spam and abuse. This is not an idle thought experiment; it > does practically happen with great frequency on the Internet today. An > unsupported system might be acceptable if it has no network connectivity > at all, but then it would not need a newer version of Git. Aren't you assuming that any network connectivity is equal to connectivity to the open internet? In any case, I think the notion that we should make git slightly more painful to use on these systems as a distant proxy variable to forcing OS upgrades is several levels away from where I think we should be drawing the line, which is closer to "is it painful in-tree?" and "is someone sending us patches to make it work?" etc.
On Thu, Jul 22, 2021 at 12:22:11AM +0200, Ævar Arnfjörð Bjarmason wrote: > This series is a re-roll of patches found in Peff's GitHub repo at > jk/no-ancient-curl, which were already-rebased versions of those > patches. His original on-list version had his Signed-off-by, but the > range-diff is against that branch, hence the addition of > Signed-off-by in the range-diff. Heh, OK. It's a little surprising to see random junk pulled out of my GitHub repo, but in this case I was holding onto them with the intent of eventually resending after more time passed. So I'm happy to see these cleaned up and posted. I think what's on that branch should be good-ish, in the sense that I've been rebasing it forward as part of my daily routine, and it's part of the build that I use day-to-day. Though apparently I never applied the CURLOPT_POST301 fix. :-/ I know my S-o-b was on the originals to the list, but just to make clear: I am fine with using them on the rebased versions you grabbed. > I then added a couple of patches on top, one is based on my comments > on the v1 http://lore.kernel.org/git/871sokhoi9.fsf@gmail.com, > i.e. the CURLAUTH_DIGEST_IE and CURLOPT_USE_SSL flags are also > version-based, and we can drop support for curls that don't have them. Seems reasonable. > I then renamed the ancient CURLOPT_FILE alias to > CURLOPT_WRITEDATA. Incidentally that's how I remembered to dig up this > series, i.e. I tried to search for "CURLOPT_FILE" in API documentation > while reading our HTTP code, but had a hard time finding it, turns out > we were using a very ancient synonym for the preferred name. This seemed weirdly familiar. Looks like it was part of a series last year, but the trickier parts built on top merited a re-roll that never came: https://lore.kernel.org/git/20201013191729.2524700-2-smcallis@google.com/ > Jeff King (3): > http: drop support for curl < 7.11.1 > http: drop support for curl < 7.16.0 > http: drop support for curl < 7.19.4 > > Ævar Arnfjörð Bjarmason (2): > http: drop support for curl < 7.19.3 and < 7.16.4 (again) > http: rename CURLOPT_FILE to CURLOPT_WRITEDATA So modulo the commit message tweaks that Junio suggested, this all looks fine. I actually think my original "#error on too-old curl" is still reasonable. Yes, people whose distro has backported all of these features could possibly still use it. But in that case they likely know what's going on and can rip out the #error. It seems much more likely to me that it _won't_ work, and they'll get confused by obscure errors when they try to use an old curl. But I don't feel too stronlgy about it either way. -Peff
Jeff King <peff@peff.net> writes: > On Thu, Jul 22, 2021 at 12:22:11AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> This series is a re-roll of patches found in Peff's GitHub repo at >> jk/no-ancient-curl, which were already-rebased versions of those >> patches. His original on-list version had his Signed-off-by, but the >> range-diff is against that branch, hence the addition of >> Signed-off-by in the range-diff. > > Heh, OK. It's a little surprising to see random junk pulled out of my > GitHub repo, but in this case I was holding onto them with the intent of > eventually resending after more time passed. > > So I'm happy to see these cleaned up and posted. I think what's on that > branch should be good-ish, in the sense that I've been rebasing it > forward as part of my daily routine, and it's part of the build that I > use day-to-day. Though apparently I never applied the CURLOPT_POST301 > fix. :-/ Thanks. > I know my S-o-b was on the originals to the list, but just to make > clear: I am fine with using them on the rebased versions you grabbed. Good. S-o-b is merely "I can let the project use it" and does not say "I agree this is (still) relevant in the context of the code this is being submitted to", so the above note is very much appreciated. > So modulo the commit message tweaks that Junio suggested, this all looks > fine. I actually think my original "#error on too-old curl" is still > reasonable. Yes, people whose distro has backported all of these > features could possibly still use it. But in that case they likely know > what's going on and can rip out the #error. It seems much more likely > to me that it _won't_ work, and they'll get confused by obscure errors > when they try to use an old curl. > > But I don't feel too stronlgy about it either way. Me neither. Those who are vanilla would not be helped by having it, as their build would fail if their cURL is too old anyway even without it. Those who backported would have a build that may or may not work, but diagnosing it is part of the job of backporting their cURL anyway. So in practice, I think "#error if you are older than X" primarily would serve documentation purposes (which may be worth doing, but requirements listed in INSTALL would probably be a better alternative anyway). Thanks.
On July 23, 2021 12:21 PM, Junio C Hamano wrote: >Jeff King <peff@peff.net> writes: > >> On Thu, Jul 22, 2021 at 12:22:11AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >>> This series is a re-roll of patches found in Peff's GitHub repo at >>> jk/no-ancient-curl, which were already-rebased versions of those >>> patches. His original on-list version had his Signed-off-by, but the >>> range-diff is against that branch, hence the addition of >>> Signed-off-by in the range-diff. >> >> Heh, OK. It's a little surprising to see random junk pulled out of my >> GitHub repo, but in this case I was holding onto them with the intent >> of eventually resending after more time passed. >> >> So I'm happy to see these cleaned up and posted. I think what's on >> that branch should be good-ish, in the sense that I've been rebasing >> it forward as part of my daily routine, and it's part of the build >> that I use day-to-day. Though apparently I never applied the >> CURLOPT_POST301 fix. :-/ > >Thanks. > >> I know my S-o-b was on the originals to the list, but just to make >> clear: I am fine with using them on the rebased versions you grabbed. > >Good. S-o-b is merely "I can let the project use it" and does not say "I agree this is (still) relevant in the context of the code this is being >submitted to", so the above note is very much appreciated. > >> So modulo the commit message tweaks that Junio suggested, this all >> looks fine. I actually think my original "#error on too-old curl" is >> still reasonable. Yes, people whose distro has backported all of these >> features could possibly still use it. But in that case they likely >> know what's going on and can rip out the #error. It seems much more >> likely to me that it _won't_ work, and they'll get confused by obscure >> errors when they try to use an old curl. >> >> But I don't feel too stronlgy about it either way. > >Me neither. Those who are vanilla would not be helped by having it, as their build would fail if their cURL is too old anyway even without >it. Those who backported would have a build that may or may not work, but diagnosing it is part of the job of backporting their cURL >anyway. So in practice, I think "#error if you are older than X" primarily would serve documentation purposes (which may be worth doing, >but requirements listed in INSTALL would probably be a better alternative anyway). This is probably a red-herring, but from what I am observing, the curl 7.70 version is required for OpenSSL 3.0.0. Once we move there, which my team is working on, the near recent older version of curl could also be problematic and incompatible. This is rather unpleasant because the current standard libcurl on our platform is 7.65, which is too old to be compatible anyway, so we're going to have to put out a separate libcurl build. Curl seems to need to be closer to the bleeding edge to retain imminent compatibility. -Randall
On Fri, Jul 23, 2021 at 09:21:11AM -0700, Junio C Hamano wrote: > > So modulo the commit message tweaks that Junio suggested, this all looks > > fine. I actually think my original "#error on too-old curl" is still > > reasonable. Yes, people whose distro has backported all of these > > features could possibly still use it. But in that case they likely know > > what's going on and can rip out the #error. It seems much more likely > > to me that it _won't_ work, and they'll get confused by obscure errors > > when they try to use an old curl. > > > > But I don't feel too stronlgy about it either way. > > Me neither. Those who are vanilla would not be helped by having it, > as their build would fail if their cURL is too old anyway even > without it. Those who backported would have a build that may or may > not work, but diagnosing it is part of the job of backporting their > cURL anyway. So in practice, I think "#error if you are older than > X" primarily would serve documentation purposes (which may be worth > doing, but requirements listed in INSTALL would probably be a better > alternative anyway). Yeah, it is purely for documentation to help people who are confused by a broken build. But I agree that INSTALL is probably reasonable there (not to mention that the whole point is that these versions are so old hardly anybody should be using them anyway). So let's forget about the #error thing, then. -Peff
This series simplifies the http code by dropping support for curl versions older than 7.19.4, released in March 2009. This was last discussed on-list in 2017: http://lore.kernel.org/git/20170809120024.7phdjzjv54uv5dpz@sigill.intra.peff.net My reading of why it didn't get integrated at the time was: - The original commit messages are opinionated about git not working on these versions anyway, as noted in the original thread that's only true of vanilla curl, but anyone impacted by these issues at the time was probably using e.g. RHEL, which had backports that confused the issue. - While in 2017 these versions were already ancient, RHEL 5 (released in 2007) was still seeing some notable production use. It finally got "we really mean it now" EOL'd in late 2020 when extended life-cycle support ended (see https://access.redhat.com/support/policy/updates/errata). RHEL 6 does not have a libcurl affected by these changes. - It ended with a patch to "error on too-old curl", i.e. to make compiling on versions older than 7.19.4 an error. I've ejected that per the discussion about backports confusing that issue. This series is a re-roll of patches found in Peff's GitHub repo at jk/no-ancient-curl, which were already-rebased versions of those patches. His original on-list version had his Signed-off-by, but the range-diff is against that branch, hence the addition of Signed-off-by in the range-diff. Peff's original 3/4 had a subtle bug in keeping the "CURLOPT_POST301" branch of an ifdef/elif, spotted by Mischa POSLAWSKY, a fix for that is squashed in here. See https://lore.kernel.org/git/20170810123641.GG2363@shiar.net/ I then added a couple of patches on top, one is based on my comments on the v1 http://lore.kernel.org/git/871sokhoi9.fsf@gmail.com, i.e. the CURLAUTH_DIGEST_IE and CURLOPT_USE_SSL flags are also version-based, and we can drop support for curls that don't have them. I then renamed the ancient CURLOPT_FILE alias to CURLOPT_WRITEDATA. Incidentally that's how I remembered to dig up this series, i.e. I tried to search for "CURLOPT_FILE" in API documentation while reading our HTTP code, but had a hard time finding it, turns out we were using a very ancient synonym for the preferred name. Jeff King (3): http: drop support for curl < 7.11.1 http: drop support for curl < 7.16.0 http: drop support for curl < 7.19.4 Ævar Arnfjörð Bjarmason (2): http: drop support for curl < 7.19.3 and < 7.16.4 (again) http: rename CURLOPT_FILE to CURLOPT_WRITEDATA http-push.c | 29 +-------- http-walker.c | 14 +---- http.c | 169 ++------------------------------------------------ http.h | 46 -------------- imap-send.c | 4 -- remote-curl.c | 11 +--- 6 files changed, 10 insertions(+), 263 deletions(-) Range-diff against v1: 1: 8793735cc2c ! 1: dcbb6f95652 http: drop support for curl < 7.11.1 @@ Metadata ## Commit message ## http: drop support for curl < 7.11.1 - Recent versions of Git will not build with curl older than - 7.11.1 due to (at least) two issues: + Drop support for this ancient version of curl and simplify the code by + allowing us get rid of some "#ifdef"'s. + + Git will not build with vanilla curl older than 7.11.1 due to (at + least) two issues: - our use of CURLOPT_POSTFIELDSIZE in 37ee680d9b (http.postbuffer: allow full range of ssize_t values, @@ Commit message obvious benefit is that we'll have fewer conditional bits cluttering the code. - But more importantly, we're doing a disservice to users to - pretend that Git works with old versions. It's clear that - nobody is testing modern Git with such old versions of curl - (we've had 3 released versions with the CURLPROTO issue - without a report of anyone seeing the breakage in the wild). - And there are a lot of subtle ways we could be getting this - wrong (for instance, curl prior to 7.17.0 did not copy - string arguments to curl_easy_setopt(), which means that - using an old copy of curl could produce use-after-free - bugs that are not present with more recent versions). - This patch drops all #ifdefs that reference older versions (note that curl's preprocessor macros are in hex, so we're looking for 070b01, not 071101). + Signed-off-by: Jeff King <peff@peff.net> + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> + ## http.c ## @@ static struct trace_key trace_curl = TRACE_KEY_INIT(CURL); 2: 15638cd1856 ! 2: 1c9f3bc031b http: drop support for curl < 7.16.0 @@ Metadata ## Commit message ## http: drop support for curl < 7.16.0 - As discussed in the previous commit, Git is not well-tested - with old versions of curl (and in fact since v2.12.0 does - not even compile with versions older than 7.19.4). Let's - stop pretending we support curl that old and drop any - now-obslete #ifdefs. + In the last commit we dropped support for curl < 7.11.1, let's + continue that and drop support for versions older than 7.16.0. This + allows us to get rid of some now-obsolete #ifdefs. - Choosing 7.16.0 is a somewhat arbitrary cutoff, but: + Choosing 7.16.0 is a somewhat arbitrary cutoff: - 1. it came out in October of 2006, over 10 years ago. - Besides being a nice round number, it's a common - end-of-life support period, even for conservative + 1. It came out in October of 2006, almost 15 years ago. + Besides being a nice round number, around 10 years is + a common end-of-life support period, even for conservative distributions. - 2. that version introduced the curl_multi interface, which + 2. That version introduced the curl_multi interface, which gives us a lot of bang for the buck in removing #ifdefs + RHEL 5 came with curl 7.15.5[1] (released in August 2006). RHEL 5's + extended life cycle program ended on 2020-11-30[1]. RHEL 6 comes with + curl 7.19.7 (released in November 2009), and RHEL 7 comes with + 7.29.0 (released in February 2013). + + 1. http://lore.kernel.org/git/873e1f31-2a96-5b72-2f20-a5816cad1b51@jupiterrise.com + + Signed-off-by: Jeff King <peff@peff.net> + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> + ## http-push.c ## @@ http-push.c: static void curl_setup_http(CURL *curl, const char *url, curl_easy_setopt(curl, CURLOPT_INFILE, buffer); @@ http.h: void finish_all_active_slots(void); void http_init(struct remote *remote, const char *url, int proactive_auth); + ## imap-send.c ## +@@ imap-send.c: static int curl_append_msgs_to_imap(struct imap_server_conf *server, + if (cred.username) { + if (res == CURLE_OK) + credential_approve(&cred); +-#if LIBCURL_VERSION_NUM >= 0x070d01 + else if (res == CURLE_LOGIN_DENIED) +-#else +- else +-#endif + credential_reject(&cred); + } + + ## remote-curl.c ## @@ remote-curl.c: static size_t rpc_out(void *ptr, size_t eltsize, return avail; 3: 335046de7bc ! 3: faae88b7fec http: drop support for curl < 7.19.4 @@ Metadata ## Commit message ## http: drop support for curl < 7.19.4 - Since v2.12.0, Git does not compile with versions of curl - older than 7.19.4. That version of curl is about 8 years - old. This means it may still be used in some distributions - with long-running support periods. But the fact that we - haven't received a single bug report about the compile-time - breakage implies that nobody cares about building recent - releases on such platforms. + In the last commit we dropped support for curl < 7.16.0, let's + continue that and drop support for versions older than 7.19.4. This + allows us to simplify the code by getting rid of some "#ifdef"'s. - As discussed in the previous two commits, this cleans up the - code and gives a more realistic signal to users about which - versions of Git are actually tested (in particular, this - moves us past the potential use-after-free issues with curl - older than 7.17.0). + Git was broken with vanilla curl < 7.19.4 from v2.12.0 until + v2.15.0. Compiling with it was broken by using CURLPROTO_* outside any + "#ifdef" in aeae4db174 (http: create function to get curl allowed + protocols, 2016-12-14), and fixed in v2.15.0 in f18777ba6ef (http: fix + handling of missing CURLPROTO_*, 2017-08-11). + + It's unclear how much anyone was impacted by that in practice, since + as noted in [1] RHEL versions using curl older than that still + compiled, because RedHat backported some features. Perhaps other + vendors did the same. + + Still, it's one datapoint indicating that it wasn't in active use at + the time. That (the v2.12.0 release) was in Feb 24, 2017, with v2.15.0 + on Oct 30, 2017, it's now mid-2021. + + 1. http://lore.kernel.org/git/c8a2716d-76ac-735c-57f9-175ca3acbcb0@jupiterrise.com; + followed-up by f18777ba6ef (http: fix handling of missing CURLPROTO_*, + 2017-08-11) + + Signed-off-by: Jeff King <peff@peff.net> + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## http.c ## @@ http.c: static int min_curl_sessions = 1; @@ http.c: static void var_override(const char **var, char *value) } static void init_curl_proxy_auth(CURL *result) +@@ http.c: void setup_curl_trace(CURL *handle) + curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL); + } + +-#ifdef CURLPROTO_HTTP + static long get_curl_allowed_protocols(int from_user) + { + long allowed_protocols = 0; +@@ http.c: static long get_curl_allowed_protocols(int from_user) + + return allowed_protocols; + } +-#endif + + #if LIBCURL_VERSION_NUM >=0x072f00 + static int get_curl_http_version_opt(const char *version_string, long *opt) @@ http.c: static CURL *get_curl_handle(void) } @@ http.c: static CURL *get_curl_handle(void) -#if LIBCURL_VERSION_NUM >= 0x071301 curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL); -#elif LIBCURL_VERSION_NUM >= 0x071101 - curl_easy_setopt(result, CURLOPT_POST301, 1); +- curl_easy_setopt(result, CURLOPT_POST301, 1); -#endif -#ifdef CURLPROTO_HTTP curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, 4: e049f37357a < -: ----------- http: #error on too-old curl -: ----------- > 4: 9a30e92520c http: drop support for curl < 7.19.3 and < 7.16.4 (again) -: ----------- > 5: 64e510b4a6b http: rename CURLOPT_FILE to CURLOPT_WRITEDATA