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

Check for integer overflow in Bytes.extend #934

Merged
merged 3 commits into from Dec 19, 2016

Conversation

Projects
None yet
2 participants
@yallop
Member

yallop commented Nov 26, 2016

Current behaviour:

# Bytes.extend "abc" max_int max_int;;
- : bytes = "\000"

With this fix:

# Bytes.extend "abc" max_int max_int;;
Exception: Invalid_argument "Bytes.extend".
Show outdated Hide outdated Changes
@@ -128,6 +128,9 @@ Next version (4.05.0):
### Bug fixes
- GPR#934: check for integer overflow in Bytes.extend
(Jeremy Yallop)

This comment has been minimized.

@gasche

gasche Nov 26, 2016

Member

Sorting entries by MPR first, GPR second, this should go as the very last item of the category.

@gasche

gasche Nov 26, 2016

Member

Sorting entries by MPR first, GPR second, this should go as the very last item of the category.

Show outdated Hide outdated stdlib/bytes.ml
let len = length s + left in
if len < 0 then invalid_arg "Bytes.extend" else
let len = len + right in
if len < 0 then invalid_arg "Bytes.extend" else

This comment has been minimized.

@gasche

gasche Nov 26, 2016

Member

I find the shadowing used here not very readable. You could name the first variable len_left, or declare let (++) a b = (* check for overflow *) let c = a + b in if c < 0 then invalid_arg "Bytes.extend" else c and let len = length s ++ left ++ right.

@gasche

gasche Nov 26, 2016

Member

I find the shadowing used here not very readable. You could name the first variable len_left, or declare let (++) a b = (* check for overflow *) let c = a + b in if c < 0 then invalid_arg "Bytes.extend" else c and let len = length s ++ left ++ right.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Nov 28, 2016

Member

Thanks for the comments, @gasche. I've updated the patch.

I think this may need a bit more care before merging, though, since it rejects

Bytes.extend "abc" (-4) 4

which was previously accepted, and had reasonable behaviour. I'll come back to this shortly.

Member

yallop commented Nov 28, 2016

Thanks for the comments, @gasche. I've updated the patch.

I think this may need a bit more care before merging, though, since it rejects

Bytes.extend "abc" (-4) 4

which was previously accepted, and had reasonable behaviour. I'll come back to this shortly.

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Dec 19, 2016

Member

I've updated the overflow check to take care with negative inputs (31f8776) and added a testsuite for Bytes.extend that includes tests both for existing behaviour and for the overflow check (5de7108).

Member

yallop commented Dec 19, 2016

I've updated the overflow check to take care with negative inputs (31f8776) and added a testsuite for Bytes.extend that includes tests both for existing behaviour and for the overflow check (5de7108).

@gasche gasche merged commit e51cbef into ocaml:trunk Dec 19, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 19, 2016

Member

I'm partial to patches that come with testsuite support. Merged.

Member

gasche commented Dec 19, 2016

I'm partial to patches that come with testsuite support. Merged.

@yallop yallop deleted the yallop:bytes-extend-overflow branch Dec 19, 2016

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #934 from yallop/bytes-extend-overflow
Check for integer overflow in Bytes.extend
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment