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

make Cohttp_lwt_unix.default_ctx lazy #1027

Merged
merged 4 commits into from May 3, 2024
Merged

make Cohttp_lwt_unix.default_ctx lazy #1027

merged 4 commits into from May 3, 2024

Conversation

akuhlens
Copy link
Contributor

@akuhlens akuhlens commented Apr 3, 2024

The oss tool semgrep uses cohttp for some of its network requests. We recently got a bug report that showed that even if you didn't do any network requests if you don't have CA certificates installed on the machine you get the following backtrace.

root@1eb4df820b50:/usr/local/src# semgrep --json --skip-unknown-extensions --disable-version-check --metrics=off --no-rewrite-rule-ids -f var-in-href.yaml 
Fatal error: exception Failure("ca-certs: no trust anchor file found, looked into /etc/ssl/certs/ca-certificates.crt, /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem, /etc/ssl/ca-bundle.pem.\nPlease report an issue at https://github.com/mirage/ca-certs, including:\n- the output of uname -s\n- the distribution you use\n- the location of default trust anchors (if known)\n")
Raised at Stdlib.failwith in file "stdlib.ml", line 29, characters 17-33
Called from CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 31, characters 17-27
Re-raised at CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 36, characters 4-11
Called from Conduit_lwt_unix.default_ctx in file "src/conduit-lwt-unix/conduit_lwt_unix.ml", line 158, characters 26-79
Called from CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 31, characters 17-27
Re-raised at CamlinternalLazy.force_lazy_block in file "camlinternalLazy.ml", line 36, characters 4-11
Called from Cohttp_lwt_unix__Net.default_ctx in file "cohttp-lwt-unix/src/net.ml", line 33, characters 10-49

This change makes the default_ctx value lazy so that it isn't initialized when the library is loaded. Which allows us to control when it is initialized. This is the same pattern that the conduit library uses for there default_ctx.

Test Plan

I have tested this change by:

  • running the test suites and
  • pinning the version of cohttp that semgrep is using to this branch and showing that similar calls to semgrep no longer raise this error.

@mseri
Copy link
Collaborator

mseri commented Apr 4, 2024

There is still a failure

File "cohttp-mirage/src/net.ml", line 1:
Error: The implementation cohttp-mirage/src/net.pp.ml
       does not match the interface cohttp-mirage/src/.cohttp_mirage.objs/byte/cohttp_mirage__Net.cmi:
        ... ... ... ... In module Make:
       Values do not match:
         val default_ctx : ctx
       is not included in
         val default_ctx : ctx lazy_t
       The type ctx is not compatible with the type ctx lazy_t
       File "cohttp-mirage/src/net.mli", line 11, characters 10-47:
         Expected declaration
       File "cohttp-mirage/src/net.ml", line 20, characters 6-17:
         Actual declaration
(cd _build/default && /home/opam/.opam/5.0/bin/ocamlc.opt -w @1..3@5..28@30..39@43@46..47@49..57@61..62-40 -strict-sequence -strict-formats -short-paths -keep-locs -g -bin-annot -I cohttp-mirage/src/.cohttp_mirage.objs/byte -I /home/opam/.opam/5.0/lib/angstrom -I /home/opam/.opam/5.0/lib/asn1-combinators -I /home/opam/.opam/5.0/lib/astring -I /home/opam/.opam/5.0/lib/base64 -I /home/opam/.opam/5.0/lib/bigstringaf -I /home/opam/.opam/5.0/lib/ca-certs-nss -I /home/opam/.opam/5.0/lib/conduit -I /home/opam/.opam/5.0/lib/conduit-lwt -I /home/opam/.opam/5.0/lib/conduit-mirage -I /home/opam/.opam/5.0/lib/cstruct -I /home/opam/.opam/5.0/lib/dns -I /home/opam/.opam/5.0/lib/dns-client -I /home/opam/.opam/5.0/lib/dns-client-mirage -I /home/opam/.opam/5.0/lib/dns/cache -I /home/opam/.opam/5.0/lib/domain-name -I /home/opam/.opam/5.0/lib/duration -I /home/opam/.opam/5.0/lib/eqaf -I /home/opam/.opam/5.0/lib/eqaf/bigstring -I /home/opam/.opam/5.0/lib/eqaf/cstruct -I /home/opam/.opam/5.0/lib/fmt -I /home/opam/.opam/5.0/lib/gmap -I /home/opam/.opam/5.0/lib/happy-eyeballs -I /home/opam/.opam/5.0/lib/hkdf -I /home/opam/.opam/5.0/lib/io-page -I /home/opam/.opam/5.0/lib/ipaddr -I /home/opam/.opam/5.0/lib/ipaddr-sexp -I /home/opam/.opam/5.0/lib/logs -I /home/opam/.opam/5.0/lib/lru -I /home/opam/.opam/5.0/lib/lwt -I /home/opam/.opam/5.0/lib/macaddr -I /home/opam/.opam/5.0/lib/magic-mime -I /home/opam/.opam/5.0/lib/metrics -I /home/opam/.opam/5.0/lib/mirage-channel -I /home/opam/.opam/5.0/lib/mirage-clock -I /home/opam/.opam/5.0/lib/mirage-crypto -I /home/opam/.opam/5.0/lib/mirage-crypto-ec -I /home/opam/.opam/5.0/lib/mirage-crypto-pk -I /home/opam/.opam/5.0/lib/mirage-crypto-rng -I /home/opam/.opam/5.0/lib/mirage-flow -I /home/opam/.opam/5.0/lib/mirage-flow-combinators -I /home/opam/.opam/5.0/lib/mirage-kv -I /home/opam/.opam/5.0/lib/mirage-random -I /home/opam/.opam/5.0/lib/mirage-time -I /home/opam/.opam/5.0/lib/optint -I /home/opam/.opam/5.0/lib/parsexp -I /home/opam/.opam/5.0/lib/pbkdf -I /home/opam/.opam/5.0/lib/ppx_sexp_conv/runtime-lib -I /home/opam/.opam/5.0/lib/psq -I /home/opam/.opam/5.0/lib/ptime -I /home/opam/.opam/5.0/lib/randomconv -I /home/opam/.opam/5.0/lib/re -I /home/opam/.opam/5.0/lib/seq -I /home/opam/.opam/5.0/lib/sexplib -I /home/opam/.opam/5.0/lib/sexplib0 -I /home/opam/.opam/5.0/lib/stringext -I /home/opam/.opam/5.0/lib/tcpip -I /home/opam/.opam/5.0/lib/tls -I /home/opam/.opam/5.0/lib/tls-mirage -I /home/opam/.opam/5.0/lib/uri -I /home/opam/.opam/5.0/lib/uri-sexp -I /home/opam/.opam/5.0/lib/uri/services -I /home/opam/.opam/5.0/lib/vchan -I /home/opam/.opam/5.0/lib/x509 -I /home/opam/.opam/5.0/lib/xenstore -I /home/opam/.opam/5.0/lib/xenstore/client -I /home/opam/.opam/5.0/lib/zarith -I cohttp-lwt/src/.cohttp_lwt.objs/byte -I cohttp/src/.cohttp.objs/byte -I http/src/.http.objs/byte -I http/src/bytebuffer/.http_bytebuffer.objs/byte -intf-suffix .ml -no-alias-deps -opaque -open Cohttp_mirage__ -o cohttp-mirage/src/.cohttp_mirage.objs/byte/cohttp_mirage__Net.cmo -c -impl cohttp-mirage/src/net.pp.ml)
File "cohttp-mirage/src/net.ml", line 1:
Error: The implementation cohttp-mirage/src/net.pp.ml
       does not match the interface cohttp-mirage/src/.cohttp_mirage.objs/byte/cohttp_mirage__Net.cmi:
        ... ... ... ... In module Make:
       Values do not match:
         val default_ctx : ctx
       is not included in
         val default_ctx : ctx lazy_t
       The type ctx is not compatible with the type ctx lazy_t
       File "cohttp-mirage/src/net.mli", line 11, characters 10-47:
         Expected declaration
       File "cohttp-mirage/src/net.ml", line 20, characters 6-17:
         Actual declaration

@akuhlens
Copy link
Contributor Author

akuhlens commented Apr 5, 2024

Are the rest of the failures expected? I am not sure how to debug the one that isn't marked experimental.

@akuhlens
Copy link
Contributor Author

I think this is good to go now!

@akuhlens
Copy link
Contributor Author

Hi @mseri, is this something that would be accepted? Or should we try to fix this problem internally?

@mseri
Copy link
Collaborator

mseri commented Apr 24, 2024

I don't see why not. I wanted to see if anybody had an opinion about it and if we could do it differently, but I don't think I will have time for doing that soon. I'll have a proper look in the weekend, sorry for the delay

@akuhlens
Copy link
Contributor Author

akuhlens commented May 2, 2024

Hi @mseri, did you get a chance to look at this this last weekend?

@mseri
Copy link
Collaborator

mseri commented May 3, 2024

Looks good to me

@mseri
Copy link
Collaborator

mseri commented May 3, 2024

I am going to merge but please @mefyl have a look as well if you have time

@mseri mseri merged commit c2b2271 into mirage:master May 3, 2024
15 of 19 checks passed
@mefyl
Copy link
Contributor

mefyl commented May 3, 2024

It looks like the problem and the solution here are the Unix backend equivalent of this issue in the jsoo backend, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants