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

Update fuzz tests to released crowbar API #244

Merged
merged 1 commit into from Mar 25, 2019
Merged

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Mar 25, 2019

In #237 (comment), @hannesm noted that some cstruct functions can be used to access memory outside of the original cstruct (but inside the original buffer), which seems a bit surprising. I noticed that the fuzz tests actually check for the safer behaviour, so I tried running them to see why they were passing. It turns out that they have bit-rotted and no longer compile.

This PR updates the tests to use the released crowbar API. I ran afl-fuzz on fuzz.exe, after a few minutes it reported three failures:

sub 0 204
sub: FAIL

When given the input:

    [#1 [44943; 0; 0]; 0; 204]
    
the test failed:

    check false

That is:

utop # let whole = Cstruct.create 44943 in
       let empty = Cstruct.sub whole 0 0 in
       Cstruct.sub empty 0 204;;
- : t = {buffer = <abstr>; off = 0; len = 204}
add_len: FAIL

When given the input:

    [#1 [60159; 8; 0]; 32768]
    
the test failed:

    check false
set_len: FAIL

When given the input:

    [#1 [61056; 0; 0]; 512]
    
the test failed:

    check false

I'm not sure what we expect the correct behaviour to be here, but either the tests or the code should be changed.

@hannesm
Copy link
Member

hannesm commented Mar 25, 2019

NB: in mirage/mirage-tcpip#394 i removed various set_len calls. IMHO set_len and add_len should be removed, and sub be fixed. thanks!

@dinosaure
Copy link
Member

Same as @hannesm, sub should be fixed. However, we need to see if mirage-tcpip relies on this behavior.

@hannesm hannesm mentioned this pull request Mar 25, 2019
@avsm avsm merged commit 9c4d782 into mirage:master Mar 25, 2019
@avsm
Copy link
Member

avsm commented Mar 25, 2019

thanks!

talex5 added a commit that referenced this pull request Apr 9, 2019
@hannesm hannesm mentioned this pull request Apr 18, 2019
avsm added a commit to avsm/opam-repository that referenced this pull request Apr 19, 2019
…uct-sexp and cstruct-lwt (5.0.0)

CHANGES:

**Security**: This release tightens bounds checks to ensure
that data outside a given view (but still inside the underlying
buffer) cannot be accessed.

- `sub` does more checks (mirage/ocaml-cstruct#244 mirage/ocaml-cstruct#245 @hannesm @talex5 review by @dinosaure)
- `add_len` and `set_len` are now deprecated and will be removed
  in a future release. (mirage/ocaml-cstruct#251 @hannesm)
- do not add user-provided data for bounds checks
  (mirage/ocaml-cstruct#253 @hannesm, report and review by @talex5)
- improve CI to add fuzzing (mirage/ocaml-cstruct#255 mirage/ocaml-cstruct#252 @avsm @yomimono @talex5)

**Remove Unix dependency**: cstruct now uses the new `bigarray-compat`
library instead of Bigarray directly, to avoid a dependency on Unix
when using OCaml compilers less than 4.06.0.  This will break downstream
libraries that do not have a direct dependency on `Bigarray`.  Simply
fix it in your library by adding a `bigarray` dependency in your dune
file. (mirage/ocaml-cstruct#247 @TheLortex)

**Capability module**: To improve the safety of future code with stronger type
checking, this release introduces a new `Cstruct_cap` module which makes the
underlying Cstruct an abstract type instead of a record. In return for this
extra abstraction, the module can enforce read-only, write only, and read/write
buffers by tracking them as phantom type variables.  Although this library
shares an implementation internally with classic `Cstruct`, it is a significant
revision and so we will be gradually migrating to it.  Feedback on it is
welcome! (mirage/ocaml-cstruct#237 @dinosaure and many excited reviewers)

**Ppx compare functions**: A new `compare_X` function is generated for
`cenum` declarations. This respects custom ids supplied in the cenum
declaration and so is more robust than polymorphic compare (mirage/ocaml-cstruct#248 @emillon)

The CI has also been switched over to both Azure Pipelines and Drone in
addition to Travis, and as a result the tests all run on Windows, macOS,
various Linux distributions, on x86 and arm64 machines, and runs AFL
fuzz tests on the Drone cloud (mirage/ocaml-cstruct#255 @avsm).
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

4 participants