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

Add the miou implementation #503

Merged
merged 11 commits into from
Aug 21, 2024
Merged

Add the miou implementation #503

merged 11 commits into from
Aug 21, 2024

Conversation

dinosaure
Copy link
Contributor

#443 rebased with string. /cc @hannesm feel free to remove the execution of the fuzzer which can take a long time.

tls-miou-unix.opam Outdated Show resolved Hide resolved
tls-miou-unix.opam Outdated Show resolved Hide resolved
tls-miou-unix.opam Outdated Show resolved Hide resolved
@hannesm
Copy link
Member

hannesm commented Aug 20, 2024

looks great apart from minor comments.

@@ -0,0 +1,12 @@
(test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure that you want to execute this executable every time, it takes a very long time.

Copy link
Member

Choose a reason for hiding this comment

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

how long is "very long"? And what are the options? marking it as executable? then we can't attach it to a package without a public_name...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how long is "very long"?

2mins on my (good) machine.

And what are the options? marking it as executable? then we can't attach it to a package without a public_name...

We can set it as a simple executable if the test bother you too much (because it takes to much time).

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine leaving it as test. I usually develop in ocaml 4 land where I don't have miou anyways, and I prefer to have tests that are useful enabled and running (otherwise we hit the "what was the last time that feedback.exe worked" again).

@dinosaure dinosaure force-pushed the miou-string-2 branch 2 times, most recently from ced7d6e to 01200a3 Compare August 21, 2024 12:05
@hannesm
Copy link
Member

hannesm commented Aug 21, 2024

This looks fine to merge, but:

When given the input:


    ["\203\171\240\014\003\191\176Q\174TZ\212\134\127&\171\029\0315}\1720I\207\132\156\172\238:gy\157\214\180\152\003\204T\161kT\016\224$O\224\183@2\140\142\004\146\155\022]\177\185i\163\229\218u;";
     [[[#1 client-to-server;
        ">8\127%\158\206\218\161u\131T\141\\&\217\178\016>q\028\1402)"]];
      ([#3 noop; #0 close]) => [[[#1 client-to-server;
                                  "\229g\229,`yr\172\252\217\209\026\243\006\223\030\207\n!\175>\174R\185{1\172\255O\215\025\194$7\159f\221\166dA\191\253\173<^\135/!\154\169\132H\202f\024\015\173x?U\135\232Fr"]];
                                ([#0 close; #1 shutdown-recv]) => [[[#1 client-to-server;
                                                                    "\244,#gnm\028\244\241(\144\131v\006\157Z\236\191\243\252\020*\027$\168y\235\154\174\128\031=~\251\235\180\192\184\163\243\170\222\173\171\140\166{\170G\152\135vIa\149\227\194\rD\180C\135L\136"];
                                                                    [?; ?]];
                                                                   ([#0 close;
                                                                    #1 shutdown-recv]) => 
                                                                   [[[?; ?]];
                                                                    (
                                                                    [?; ?]) => 
                                                                    [[[?; ?]];
                                                                    ([?; ?]) => ?]]]]]]
    
the test failed:


    [ERROR] Unexpected result: Ok [] & Error
                                     Unix.Unix_error(Unix.EMFILE, "socket", "")

so is there by chance somewhere a socket leak? or can we reduce the number of sockets being opened?

@dinosaure
Copy link
Contributor Author

so is there by chance somewhere a socket leak? or can we reduce the number of sockets being opened?

I'm currently on it, due to the random path produced by the fuzzer and taken by the state machine, it's not sure that all sockets are correctly closed. I will ping you back and I will fix that.

@hannesm
Copy link
Member

hannesm commented Aug 21, 2024

Thanks, I rebased on the main branch and added 6328427. This works nicely on my laptop :)

@hannesm hannesm merged commit f2ce295 into mirleft:main Aug 21, 2024
1 check was pending
hannesm added a commit to hannesm/opam-repository that referenced this pull request Aug 21, 2024
CHANGES:

* API breaking change: remove usage of Cstruct.t inside of TLS, use bytes
  and string instead (mirleft/ocaml-tls#497 by @art-w, @hannesm, @dinosaure, @reynir)
  Performance is up to 3x improved (bandwidth), 2x improvement for handshake/s
  on an Intel Core(TM) i7-5600U CPU @ 2.60GHz
* FEATURE: add tls-miou-unix package, which adds miou support for TLS
  (mirleft/ocaml-tls#494 mirleft/ocaml-tls#503 @dinosaure)
* FEATURE: tls-lwt and tls-async: allow TLS over an existing connection
  `Tls_lwt.client_of_channels : Tls.Config.client -> ?host:[`host] Domain_name.t -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t`
  and
  `Tls_lwt.server_of_channels : Tls.Config.server -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t`
  (mirleft/ocaml-tls#499 @art-w @MisterDA)
* API breaking changes: revise errors - reduce the polymorphic variant
  in size, align it with RFC specified errors, be in parts more precise
  about errors, in other parts skip data (mirleft/ocaml-tls#505, @hannesm - fixes mirleft/ocaml-tls#491)
  NB: if you relied on a specific error constructor, please open an issue
* Remove unused constructors from Packet.{alert_type, compression_methods,
  client_certificate_type, extension_type} (mirleft/ocaml-tls#505, @hannesm)
  NB: if you relied on specific constructors, please open an issue
* API breaking change: Tls.Config.{server,client} now return a result
  type instead of raising an exception (mirleft/ocaml-tls#502, @hannesm, fixes mirleft/ocaml-tls#411)
* FEATURE: add bench/speed.exe, a benchmark for bandwidth (for different
  ciphersuites) and handshakes (different key exchanges and private keys)
  (mirleft/ocaml-tls#500 @hannesm @dinosaure @reynir)
* BUGFIX: tests/feedback.exe update with TLS 1.3 semantics, run as test
  (mirleft/ocaml-tls#501, @hannesm - reported by @dinosaure)
hannesm added a commit to hannesm/opam-repository that referenced this pull request Aug 22, 2024
CHANGES:

* API breaking change: remove usage of Cstruct.t inside of TLS, use bytes
  and string instead (mirleft/ocaml-tls#497 by @art-w, @hannesm, @dinosaure, @reynir)
  Performance is up to 3x improved (bandwidth), 2x improvement for handshake/s
  on an Intel Core(TM) i7-5600U CPU @ 2.60GHz
* FEATURE: add tls-miou-unix package, which adds miou support for TLS
  (mirleft/ocaml-tls#494 mirleft/ocaml-tls#503 @dinosaure)
* FEATURE: tls-lwt and tls-async: allow TLS over an existing connection
  `Tls_lwt.client_of_channels : Tls.Config.client -> ?host:[`host] Domain_name.t -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t`
  and
  `Tls_lwt.server_of_channels : Tls.Config.server -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t`
  (mirleft/ocaml-tls#499 @art-w @MisterDA)
* API breaking changes: revise errors - reduce the polymorphic variant
  in size, align it with RFC specified errors, be in parts more precise
  about errors, in other parts skip data (mirleft/ocaml-tls#505, @hannesm - fixes mirleft/ocaml-tls#491)
  NB: if you relied on a specific error constructor, please open an issue
* Remove unused constructors from Packet.{alert_type, compression_methods,
  client_certificate_type, extension_type} (mirleft/ocaml-tls#505, @hannesm)
  NB: if you relied on specific constructors, please open an issue
* API breaking change: Tls.Config.{server,client} now return a result
  type instead of raising an exception (mirleft/ocaml-tls#502, @hannesm, fixes mirleft/ocaml-tls#411)
* FEATURE: add bench/speed.exe, a benchmark for bandwidth (for different
  ciphersuites) and handshakes (different key exchanges and private keys)
  (mirleft/ocaml-tls#500 @hannesm @dinosaure @reynir)
* BUGFIX: tests/feedback.exe update with TLS 1.3 semantics, run as test
  (mirleft/ocaml-tls#501, @hannesm - reported by @dinosaure)
@dinosaure dinosaure deleted the miou-string-2 branch August 22, 2024 09:02
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
CHANGES:

* API breaking change: remove usage of Cstruct.t inside of TLS, use bytes
  and string instead (mirleft/ocaml-tls#497 by @art-w, @hannesm, @dinosaure, @reynir)
  Performance is up to 3x improved (bandwidth), 2x improvement for handshake/s
  on an Intel Core(TM) i7-5600U CPU @ 2.60GHz
* FEATURE: add tls-miou-unix package, which adds miou support for TLS
  (mirleft/ocaml-tls#494 mirleft/ocaml-tls#503 @dinosaure)
* FEATURE: tls-lwt and tls-async: allow TLS over an existing connection
  `Tls_lwt.client_of_channels : Tls.Config.client -> ?host:[`host] Domain_name.t -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t`
  and
  `Tls_lwt.server_of_channels : Tls.Config.server -> Lwt_io.input_channel * Lwt_io.output_channel -> t Lwt.t`
  (mirleft/ocaml-tls#499 @art-w @MisterDA)
* API breaking changes: revise errors - reduce the polymorphic variant
  in size, align it with RFC specified errors, be in parts more precise
  about errors, in other parts skip data (mirleft/ocaml-tls#505, @hannesm - fixes mirleft/ocaml-tls#491)
  NB: if you relied on a specific error constructor, please open an issue
* Remove unused constructors from Packet.{alert_type, compression_methods,
  client_certificate_type, extension_type} (mirleft/ocaml-tls#505, @hannesm)
  NB: if you relied on specific constructors, please open an issue
* API breaking change: Tls.Config.{server,client} now return a result
  type instead of raising an exception (mirleft/ocaml-tls#502, @hannesm, fixes mirleft/ocaml-tls#411)
* FEATURE: add bench/speed.exe, a benchmark for bandwidth (for different
  ciphersuites) and handshakes (different key exchanges and private keys)
  (mirleft/ocaml-tls#500 @hannesm @dinosaure @reynir)
* BUGFIX: tests/feedback.exe update with TLS 1.3 semantics, run as test
  (mirleft/ocaml-tls#501, @hannesm - reported by @dinosaure)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants