Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mitigate problems with curl on Windows 11 #6168

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Aug 20, 2024

Fixes #6120 by altering the call to curl. curl/curl#13845 can be mitigated by avoiding the use of --write-out so, having detected exit code 43 (which should never happen using the curl binary, as curl itself should normally know how to call its own functions!), a second call is made to call using --fail instead of --write-out. --fail was previously considered in #467 (comment). I've done some digging, and the semantics of --fail are good enough for what we're after, though using --write-out as in @samoht's f2ee0ec is superior in general because it conveys more information to the user when an error occurs (which was the point).

The bug in cURL is causing enough issues elsewhere that I imagine Microsoft will update the curl.exe binary shipped with Windows before long - if they don't, it's easy to extend the mitigation further to extract the actual http response code from the error message, as the format of that message has never altered, from a look at cURL's sources.

There are three reasons to prefer this slightly more complicated fix to just adding the requirement for cURL from Cygwin/MSYS2 as in #6142:

  1. We're getting away with the workaround of adding Cygwin's curl because Cygwin's SSL certificate doesn't contain the offending OIDS. If that were to change, the workaround would fail (because we have to download Cygwin's setup using C:\Windows\System32\curl.exe)
  2. Cygwin's curl is (correctly) built with the wrong SSL stack, so the workaround has the potential to create problems for some users (same issue as Cygwin git vs Git-for-Windows)
  3. The general direction of travel is supposed to be using fewer Cygwin/MSYS2 tools, so it's a shame to have to go in the opposite direction!

Running opam init --debug -vv on my Windows 11 laptop with this binary we can now see:

00:03.923  XSYS                   Downloading Cygwin setup checksums
Downloading Cygwin setup from cygwin.com
00:03.924  SYSTEM                 mkdir C:\Users\DRA\AppData\Local\Temp\opam-27848-4d3c3d
+ C:\Windows\system32\curl.exe "--write-out" "%{http_code}\\n" "--retry" "3" "--retry-delay" "2" "--user-agent" "opam/2.3.0~alpha~dev" "-L" "-o" "C:\\Users\\DRA\\AppData\\Local\\Temp\\opam-27848-4d3c3d\\sha512.sum.part" "--" "https://cygwin.com/sha512.sum"
- 200
-   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
-                                  Dload  Upload   Total   Spent    Left  Speed
100   291  100   291    0     0    666      0 --:--:-- --:--:-- --:--:--   670
00:04.413  SYSTEM                 rm C:\Users\DRA\AppData\Local\Temp\opam-DRA-27848\dl-27848-2260b8
00:04.413  SYSTEM                 mv C:\Users\DRA\AppData\Local\Temp\opam-27848-4d3c3d\sha512.sum.part -> C:\Users\DRA\AppData\Local\Temp\opam-27848-4d3c3d\sha512.sum
00:04.422  XSYS                   Downloading setup-x86_64.exe
00:04.422  SYSTEM                 mkdir C:\Devel\Roots\broken-curl
00:04.422  SYSTEM                 mkdir C:\Devel\Roots\broken-curl\.cygwin
+ C:\Windows\system32\curl.exe "--write-out" "%{http_code}\\n" "--retry" "3" "--retry-delay" "2" "--user-agent" "opam/2.3.0~alpha~dev" "-L" "-o" "C:\\Devel\\Roots\\broken-curl\\.cygwin\\setup-x86_64.exe.part" "--" "https://cygwin.com/setup-x86_64.exe"
- 200
-   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
-                                  Dload  Upload   Total   Spent    Left  Speed
100 1374k  100 1374k    0     0  1194k      0  0:00:01  0:00:01 --:--:-- 1196k

downloading Cygwin with no issues at all, but then mitigation kicking in for opam.ocaml.org:

00:44.717  PARALLEL               Next task in job 0: C:\Windows\system32\curl.exe --write-out %{http_code}\n --retry 3 --retry-delay 2 --user-agent opam/2.3.0~alpha~dev -L -o C:\Users\DRA\AppData\Local\Temp\opam-27848-a08201\index.tar.gz.part -- https://opam.ocaml.org/index.tar.gz
Processing  1/1: [default: http]
+ C:\Windows\system32\curl.exe "--write-out" "%{http_code}\\n" "--retry" "3" "--retry-delay" "2" "--user-agent" "opam/2.3.0~alpha~dev" "-L" "-o" "C:\\Users\\DRA\\AppData\\Local\\Temp\\opam-27848-a08201\\index.tar.gz.part" "--" "https://opam.ocaml.org/index.tar.gz"
- 000
-   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
-                                  Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
- curl: (43) A libcurl function was given a bad argument
00:45.092  PARALLEL               Collected task for job 0 (ret:43)
00:45.092  CURL                   Attempting to mitigate curl/curl#13845
00:45.093  PARALLEL               Next task in job 0: C:\Windows\system32\curl.exe --fail --retry 3 --retry-delay 2 --user-agent opam/2.3.0~alpha~dev -L -o C:\Users\DRA\AppData\Local\Temp\opam-27848-a08201\index.tar.gz.part -- https://opam.ocaml.org/index.tar.gz
+ C:\Windows\system32\curl.exe "--fail" "--retry" "3" "--retry-delay" "2" "--user-agent" "opam/2.3.0~alpha~dev" "-L" "-o" "C:\\Users\\DRA\\AppData\\Local\\Temp\\opam-27848-a08201\\index.tar.gz.part" "--" "https://opam.ocaml.org/index.tar.gz"
-   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
-                                  Dload  Upload   Total   Spent    Left  Speed
100 7223k  100 7223k    0     0  14.9M      0 --:--:-- --:--:-- --:--:-- 14.9M
00:45.610  PARALLEL               Collected task for job 0 (ret:0)
00:45.611  SYSTEM                 rm C:\Users\DRA\AppData\Local\Temp\opam-DRA-27848\dl-27848-5e0cc2
00:45.611  SYSTEM                 rm C:\Users\DRA\AppData\Local\Temp\opam-DRA-27848\dl-27848-f3db71
00:45.612  SYSTEM                 mv C:\Users\DRA\AppData\Local\Temp\opam-27848-a08201\index.tar.gz.part -> C:\Users\DRA\AppData\Local\Temp\opam-27848-a08201\index.tar.gz

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I added a commit (feel free to remove it if you disagree) to remove the with_curl_mitigation argument which seems to me like something we probably want to do everytime. The other places that use it do not seem to use the error code at all. EDIT: oops sorry i'm tired, nevermind ^^"

To me it looks good to go once rebased (I would also suggest squashing everything into one commit as the first commit makes more sense combined with the second)

@dra27
Copy link
Member Author

dra27 commented Aug 20, 2024

Thanks, @kit-ty-kate! FWIW, the point in two commits is entirely because the first commit makes sense on its own (and simplifies the diff of the actual diff in the second):

if foo then
  raise Bar;
baz ()

is functionally inferior to:

if foo then
  raise Bar
else
  baz ()

especially when considering an alteration to the first branch of the if which is what this PR is doing)

src/repository/opamDownload.ml Outdated Show resolved Hide resolved
src/repository/opamDownload.ml Show resolved Hide resolved
src/repository/opamDownload.ml Outdated Show resolved Hide resolved
src/repository/opamDownload.ml Outdated Show resolved Hide resolved
src/repository/opamDownload.ml Outdated Show resolved Hide resolved
src/repository/opamDownload.ml Outdated Show resolved Hide resolved
@dra27
Copy link
Member Author

dra27 commented Aug 21, 2024

Thanks, @rjbou - changes applied

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Mitigates a known issue in curl 8.8.0 especially affecting Windows
hosts. cf curl/curl#13845
@rjbou rjbou merged commit 4bf41fe into ocaml:master Aug 21, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error : [OPAM INIT] - Platform: Windows 11 HOME - Version: 23h2
3 participants