Skip to content

Commit

Permalink
Harden the parsing of the result code from curl
Browse files Browse the repository at this point in the history
stderr and stdout can become interleaved, certainly on Windows, which
results in the http result code becoming lost.

Change puts a newline both before and after the code. However, the
interleaving means that a blank line may appear _after_ the result code.
Therefore, the logic now attempts to parse the first non-blank line from
the end of the output from curl.
  • Loading branch information
dra27 committed Jun 2, 2024
1 parent c4cf981 commit b02478b
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 16 deletions.
1 change: 1 addition & 0 deletions master_changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ users)

## Repository
* Fix download URLs containing invalid characters on Windows (e.g. the ? character in `?full_index=1`) [#5921 @dra27]
* Harden the parsing of the output from curl - the progress meter can become interleaved with the status code on Windows [#5984 @dra27]

## Lock

Expand Down
25 changes: 15 additions & 10 deletions src/repository/opamDownload.ml
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,15 @@ let user_agent =

let curl_args = [
CString "--write-out", None;
CString "%%{http_code}\\n", None; (* 6.5 13-Mar-2000 *)
CString "\\n%%{http_code}\\n", None; (* 6.5 13-Mar-2000 *)
CString "--retry", None; CIdent "retry", None; (* 7.12.3 20-Dec-2004 *)
CString "--retry-delay", None; CString "2", None; (* 7.12.3 20-Dec-2004 *)
CString "--compressed",
Some (FIdent (OpamFilter.ident_of_string "compress")); (* 7.10 1-Oct-2002 *)
CString "--user-agent", None; user_agent, None; (* 4.5.1 12-Jun-1998 *)
CString "-L", None; (* 4.9 7-Oct-1998 *)
CString "-o", None; CIdent "out", None; (* 2.3 21-Aug-1997 *)
CString "--no-progress-meter", None; (* 7.67.0 6-Nov-2019 *)
CString "--location", None; (* 4.9 7-Oct-1998 *)
CString "--output", None; CIdent "out", None; (* 2.3 21-Aug-1997 *)
CString "--", None; (* End list of options; 5.0 1-Dec-1998 *)
CIdent "url", None;
]
Expand Down Expand Up @@ -108,13 +109,17 @@ let tool_return url ret =
Printf.sprintf "curl: empty response while downloading %s"
(OpamUrl.to_string url))
| l ->
let code = List.hd (List.rev l) in
let num = try int_of_string code with Failure _ -> 999 in
if num >= 400 then
fail (Some ("curl error code " ^ code),
Printf.sprintf "curl: code %s while downloading %s"
code (OpamUrl.to_string url))
else Done ()
let l = List.rev l in
try
let code = List.find ((<>) "") l in
let num = try int_of_string code with Failure _ -> 999 in
if num >= 400 then
fail (Some ("curl error code " ^ code),
Printf.sprintf "curl: code %s while downloading %s"
code (OpamUrl.to_string url))
else Done ()
with Not_found ->
fail (Some "Curl failed", "No output from the curl command")

let download_command ~compress ?checksum ~url ~dst () =
let cmd, args =
Expand Down
8 changes: 4 additions & 4 deletions tests/reftests/download.test
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ The following actions will be performed:

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
Processing 1/1: [foo.1: http]
+ curl "--write-out" "%{http_code}\n" "--retry" "3" "--retry-delay" "2" "--user-agent" "opam/current" "-L" "-o" "${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part" "--" "https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz"
+ curl "--write-out" "\n%{http_code}\n" "--retry" "3" "--retry-delay" "2" "--user-agent" "opam/current" "-L" "-o" "${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part" "--" "https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz"
Processing 1/1:
+ tar "xfz" "${OPAMTMP}/v1.0.0.tar.gz" "-C" "${OPAMTMP}"
Done.
Expand Down Expand Up @@ -107,10 +107,10 @@ The following actions will be performed:

<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
Processing 1/1: [foo.1: http]
+ curl "--write-out" "%{http_code}\n" "--retry" "3" "--retry-delay" "2" "--user-agent" "opam/current" "-L" "-o" "${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part" "--" "https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz"
[ERROR] Failed to get sources of foo.1: curl error code ***The curl is a lie*** [args: --write-out %{http_code}\n --retry 3 --retry-delay 2 --user-agent opam/current -L -o ${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part -- https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz]
+ curl "--write-out" "\n%{http_code}\n" "--retry" "3" "--retry-delay" "2" "--user-agent" "opam/current" "-L" "-o" "${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part" "--" "https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz"
[ERROR] Failed to get sources of foo.1: curl error code ***The curl is a lie*** [args: --write-out \n%{http_code}\n --retry 3 --retry-delay 2 --user-agent opam/current -L -o ${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part -- https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz]

OpamSolution.Fetch_fail("https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz (curl: code ***The curl is a lie*** [args: --write-out %{http_code}\n --retry 3 --retry-delay 2 --user-agent opam/current -L -o ${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part -- https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz] while downloading https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz)")
OpamSolution.Fetch_fail("https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz (curl: code ***The curl is a lie*** [args: --write-out \n%{http_code}\n --retry 3 --retry-delay 2 --user-agent opam/current -L -o ${BASEDIR}/OPAM/download/.opam-switch/sources/foo.1/v1.0.0.tar.gz.part -- https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz] while downloading https://github.com/UnixJunkie/get_line/archive/v1.0.0.tar.gz)")


<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
Expand Down
4 changes: 2 additions & 2 deletions tests/reftests/swhid.unix.test
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ The following actions will be performed:
<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
[ERROR] Failed to get sources of snappy-ko.2: Download command failed

OpamSolution.Fetch_fail("https://fake.exe/url.tar.gz (Download command failed: \"curl --write-out %{http_code}\\n --retry 3 --retry-delay 2 --user-agent opam/current -L -o ${BASEDIR}/OPAM/fallback/.opam-switch/sources/snappy-ko.2/url.tar.gz.part -- https://fake.exe/url.tar.gz\" exited with code 6)")
OpamSolution.Fetch_fail("https://fake.exe/url.tar.gz (Download command failed: \"curl --write-out \n%{http_code}\\n --retry 3 --retry-delay 2 --user-agent opam/current -L -o ${BASEDIR}/OPAM/fallback/.opam-switch/sources/snappy-ko.2/url.tar.gz.part -- https://fake.exe/url.tar.gz\" exited with code 6)")


<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
Expand Down Expand Up @@ -68,7 +68,7 @@ The following actions will be performed:
Processing 1/3: [snappy-swhid-dir.2: http]
[ERROR] Failed to get sources of snappy-swhid-dir.2: Download command failed

OpamSolution.Fetch_fail("https://fake.exe/url.tar.gz (Download command failed: \"curl --write-out %{http_code}\\n --retry 3 --retry-delay 2 --user-agent opam/current -L -o ${BASEDIR}/OPAM/fallback/.opam-switch/sources/snappy-swhid-dir.2/url.tar.gz.part -- https://fake.exe/url.tar.gz\" exited with code 6)")
OpamSolution.Fetch_fail("https://fake.exe/url.tar.gz (Download command failed: \"curl --write-out \n%{http_code}\\n --retry 3 --retry-delay 2 --user-agent opam/current -L -o ${BASEDIR}/OPAM/fallback/.opam-switch/sources/snappy-swhid-dir.2/url.tar.gz.part -- https://fake.exe/url.tar.gz\" exited with code 6)")


<><> Error report <><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
Expand Down

0 comments on commit b02478b

Please sign in to comment.