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

Reject negative destination offsets in 'blit' #160

Merged
merged 2 commits into from Jul 11, 2017
Merged

Conversation

yallop
Copy link
Member

@yallop yallop commented Jul 10, 2017

Before:

# Cstruct.blit (Cstruct.create 0xf) 0 (Cstruct.create 0xf) (-0xf) 0xf;;
*** Error in `/home/jeremy/.opam/4.04.2/bin/ocamlrun': free(): invalid next size (fast): 0x0000561b7e0b0470 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x70bcb)[0x7f10fe2f9bcb]
/lib/x86_64-linux-gnu/libc.so.6(+0x76f96)[0x7f10fe2fff96]
[...]

After:

# Cstruct.blit (Cstruct.create 0xf) 0 (Cstruct.create 0xf) (-0xf) 0xf;;
Exception:
Invalid_argument
 "Cstruct.blit src=[0,15](15) dst=[0,15](15) dst-off=-15 len=15".

@avsm avsm requested a review from talex5 July 10, 2017 22:12
@hannesm
Copy link
Member

hannesm commented Jul 10, 2017

this looks good to me -- there's a small inconsistency though: blit_from_string, blit_from_bytes, and blit_to_bytes do a dstoff < 0 check, but in the first conditional, calling to the err_blit_X_src.

I agree that the solution in here is correct, and maybe we should adapt it to the other blit_* functions as well (it's mainly cosmetic AFAICT)!?

@talex5
Copy link
Contributor

talex5 commented Jul 11, 2017

LGTM, but can we get some AFL fuzz tests for this library? I suspect it would find all of these bugs in seconds.

(I can add some crowbar tests if you want)

@samoht
Copy link
Member

samoht commented Jul 11, 2017

(I can add some crowbar tests if you want)

That's a great idea!

talex5 pushed a commit to talex5/ocaml-cstruct that referenced this pull request Jul 11, 2017
When running the tests individually, it:

- Found mirage#160 in 17s.
- Found an overflow in of_bigarray in 1s.
- Found an overflow in sub in 1s.
talex5 pushed a commit to talex5/ocaml-cstruct that referenced this pull request Jul 11, 2017
When running the tests individually, it:

- Found mirage#160 in 17s.
- Found an overflow in of_bigarray in 1s.
- Found an overflow in sub in 1s.
talex5 pushed a commit to talex5/ocaml-cstruct that referenced this pull request Jul 11, 2017
When running the tests individually, it:

- Found mirage#160 in 17s.
- Found an overflow in of_bigarray in 1s.
- Found an overflow in sub in 1s.
talex5 pushed a commit to talex5/ocaml-cstruct that referenced this pull request Jul 11, 2017
When running the tests individually, it:

- Found mirage#160 in 17s.
- Found an overflow in of_bigarray in 1s.
- Found an overflow in sub in 1s.
talex5 pushed a commit to talex5/ocaml-cstruct that referenced this pull request Jul 11, 2017
When running the tests individually, it:

- Found mirage#160 in 17s.
- Found an overflow in of_bigarray in 1s.
- Found an overflow in sub in 1s.
@talex5 talex5 mentioned this pull request Jul 11, 2017
@talex5
Copy link
Contributor

talex5 commented Jul 11, 2017

(fuzzing added in #164)

@avsm avsm mentioned this pull request Jul 11, 2017
@avsm avsm merged commit c1efac1 into mirage:master Jul 11, 2017
@yallop yallop deleted the blit-bounds branch July 11, 2017 22:26
avsm added a commit to mirage/opam-repository that referenced this pull request Jul 12, 2017
--

- Fix arithmetic overflow in `Cstruct.lenv` and `copyv` (mirage/ocaml-cstruct#159 by @yallop)
- Reject negative destination offsets in `blit` (mirage/ocaml-cstruct#160 by @yallop)
- Add AFL fuzz tests using Crowbar, which independently discovered
  mirage/ocaml-cstruct#160 and also an overflow in `of_bigarray` and `sub`, now bith
  fixed (mirage/ocaml-cstruct#164 by @talex5)
- Improve performance of several allocation functions by eliminating an
  unnecessary buffer zero step (mirage/ocaml-cstruct#158 by @hannesm)
- Compile the source tree with stricter flags, including dead variable
  detection and deprecation warnings (mirage/ocaml-cstruct#157 by @samoht)
- Bump the required minimum OCaml version up to 4.03.0 (due to mirage/ocaml-cstruct#157).
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

5 participants