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

Test correct condition for exceed_limit error #1654

Merged
merged 1 commit into from Oct 15, 2018

Conversation

Projects
None yet
3 participants
@rnewson
Member

rnewson commented Oct 15, 2018

Previously we were testing if Pos + TotalBytes exceeded the pread
limit. This is the wrong logic entirely. We are trying to prevent an
attempted call to file:pread/3 where the third parameter, the number
of bytes to read, is a very large number (due to a corruption
elsewhere, say). Instead we throw exceed_limit as soon as a file gets
above a certain size.

I switched this to an if statement to make it clear that the "read
past EOF" and "try to read too many bytes" checks are quite distinct
from each other.

Test correct condition for exceed_limit error
Previously we were testing if Pos + TotalBytes exceeded the pread
limit. This is the wrong logic entirely. We are trying to prevent an
attempted call to file:pread/3 where the third parameter, the number
of bytes to read, is a very large number (due to a corruption
elsewhere, say). Instead we throw exceed_limit as soon as a file gets
above a certain size.

I switched this to an if statement to make it clear that the "read
past EOF" and "try to read too many bytes" checks are quite distinct
from each other.

@rnewson rnewson requested review from davisp and eiri Oct 15, 2018

@davisp

davisp approved these changes Oct 15, 2018

+1

@eiri

eiri approved these changes Oct 15, 2018

I believe this is correct fix and I agree that it is much easier to read with if

@rnewson rnewson merged commit 638554b into master Oct 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rnewson rnewson deleted the fix_exceed_limit branch Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment