tls-eio: improve fuzz tests #463
Conversation
eio/tests/fuzz.ml
Outdated
| Some (To_client, Send 5); | ||
| ] | ||
| *) | ||
| if true then |
There was a problem hiding this comment.
sorry, I don't quite get this style of code? would you mind to explain the construct in more detail, and/or make it disappear. I've no clue in which cases the alternative should be executed (with manual code modification, likely).
There was a problem hiding this comment.
If you get a crash and you want to reproduce it manually for testing (e.g. to try variations) you can input it here directly. Keeping it in an if makes it easier to switch between the two modes, means it still gets type-checked when disabled, and you don't get bothered by unused variable errors from dune.
There was a problem hiding this comment.
Do you think it is possible to use "cmdliner" and have this "true" hidden behind a flag?
eio/tls_eio.ml
Outdated
| @@ -29,7 +29,7 @@ module Raw = struct | |||
| raise exn | |||
|
|
|||
| let write_t t cs = | |||
| try Flow.copy (Flow.cstruct_source [cs]) t.flow | |||
| try Flow.write t.flow [cs] | |||
There was a problem hiding this comment.
is this really part of improve fuzz test? or a performance improvement? or a semantic change?
There was a problem hiding this comment.
It at least deserves a separate commit with a slightly more descriptive commit message.
There was a problem hiding this comment.
It's mostly just a simplification. It could be an optimisation if the underlying flow implements write directly. Otherwise, write just falls back to doing a copy, as in the old code. I can remove it if you prefer.
There was a problem hiding this comment.
As said, a separate commit with a commit message that includes your above description would be sufficient for me. I've disliked myself in the past to sneak any library changes in "update tests" commits, not being able to properly track years later why something changed in one way or another.
|
My comment here - #460 (comment) - is applicable to this PR I believe. |
- The mock_socket has changed slightly. Previously, it would request a size to read, do the read, and then wait for another size for the next read, which would typically just be the EOF for that request. This made it harder for the fuzzer to reach useful states. Now you can do a single transmit with a single test action. - Only log a send when we actually send some data. - Allow the fuzzer to perform all the actions needed for the Tls handshake in one go instead of requiring it to find them by searching. This provides it a way to get to the interesting cases quickly.
talex5
force-pushed
the
improve-fuzzing
branch
from
b9794d7
to
2572ada
Compare
|
OK, I've now updated this PR to remove the change to
It's a bit tricky, as crowbar likes to parse the command-line options itself (in an at_exit handler!). I've now removed the code you objected to. It was just a convenience. @bikallem wrote:
There is no "send_close". You mean |
|
Thanks for your PR. |
CHANGES: * BREAKING: new opam package tls-lwt (formerly tls.lwt), in dune: (libraries tls.lwt) should now be libraries (tls-lwt) (mirleft/ocaml-tls#468 @hannesm, reported mirleft/ocaml-tls#449 by @mbacarella) * tls: update to mirage-crypto 0.11 API (mirleft/ocaml-tls#468 @hannesm) * tls: relax SignatureAlgorithms extension handling to allow OpenSSL interoperability tests with TLS 1.0 and TLS 1.1 (mirleft/ocaml-tls#469 @hannesm) * tls: remove Utils.filter_map and and Utils.option, use Stdlib instead (mirleft/ocaml-tls#455 @hannesm) * tls: do not globally open Utils (mirleft/ocaml-tls#455 @hannesm) * tls: export log source of Tracing module (mirleft/ocaml-tls#461 @bikallem) * tls: remove unused ciphersuites to reduce binary size (mirleft/ocaml-tls#467 @hannesm) * tls-lwt: do not catch out of memory exception (mirleft/ocaml-tls#469 @hannesm) * tls-eio: add fuzz testing using crowbar (mirleft/ocaml-tls#456 mirleft/ocaml-tls#463 @talex5) * tls-eio: update to eio 0.7 (mirleft/ocaml-tls#456 @talex5) * tls-eio: fix test for develop with vendoring (mirleft/ocaml-tls#462 @bikallem)
These are the improvements to the fuzz tests that were part of #459. I think it's worth merging these on their own, since that PR isn't ready.
The mock_socket has changed slightly. Previously, it would request a size to read, do the read, and then wait for another size for the next read, which would typically just be the EOF for that request. This made it harder for the fuzzer to reach useful states. Now you can do a single transmit with a single test action.
Simplified the write in Tls_eio slightly by using the newFlow.writeoperation.Only log a send when we actually send some data.
Allow the fuzzer to perform all the actions needed for the Tls handshake in one go instead of requiring it to find them by searching. This provides it a way to get to the interesting cases quickly.