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

Improve EOF handling for exec files #397

Merged
merged 1 commit into from Apr 17, 2016

Conversation

Projects
None yet
3 participants
@marchof
Member

marchof commented Apr 15, 2016

As proposed here we should not ignore EOFExceptions in case exec files are truncated somewhere in the middle: https://groups.google.com/d/msg/jacoco/qby-WuQp194/iFnx2fAHBAAJ

@marchof marchof self-assigned this Apr 15, 2016

@marchof marchof added this to the 0.7.7 milestone Apr 15, 2016

marchof added a commit that referenced this pull request Apr 15, 2016

GitHub #397: Improve EOF handling for exec files.
A EOFException should be reported for truncated exec files.

marchof added a commit that referenced this pull request Apr 15, 2016

GitHub #397: Improve EOF handling for exec files.
A EOFException should be reported for truncated exec files.
@bjkail

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Apr 15, 2016

Contributor

The change to ExecutionDataReader.readBlock (and the ExecutionDataWriter constants) break the API in a non-internal package, so I suggest to revert those changes. ExecutionDataReader.read can safely cast to byte after checking for -1.

Contributor

bjkail commented Apr 15, 2016

The change to ExecutionDataReader.readBlock (and the ExecutionDataWriter constants) break the API in a non-internal package, so I suggest to revert those changes. ExecutionDataReader.read can safely cast to byte after checking for -1.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Apr 15, 2016

Member

Sharp eye, thank you @bjkail

Indeed - cast to byte after check for -1 is safe - quoting Javadoc as a proof:

Reads the next byte of data from the input stream. The value byte is returned as an int in the range 0 to 255. If no byte is available because the end of the stream has been reached, the value -1 is returned.

I reworked code a bit to restore API compatibility. Please check, if all fine, then I will squash commits and push it into master.

Member

Godin commented Apr 15, 2016

Sharp eye, thank you @bjkail

Indeed - cast to byte after check for -1 is safe - quoting Javadoc as a proof:

Reads the next byte of data from the input stream. The value byte is returned as an int in the range 0 to 255. If no byte is available because the end of the stream has been reached, the value -1 is returned.

I reworked code a bit to restore API compatibility. Please check, if all fine, then I will squash commits and push it into master.

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Apr 15, 2016

Member

Also maybe we should reclassify this as "bug fix" but not as "non-functional change", at least this is a behavioural change since truncated files won't be silently accepted as it was before.

Member

Godin commented Apr 15, 2016

Also maybe we should reclassify this as "bug fix" but not as "non-functional change", at least this is a behavioural change since truncated files won't be silently accepted as it was before.

@bjkail

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Apr 15, 2016

Contributor

Looks good to me now, thanks.

Contributor

bjkail commented Apr 15, 2016

Looks good to me now, thanks.

marchof added a commit that referenced this pull request Apr 17, 2016

GitHub #397: Improve EOF handling for exec files.
A EOFException should be reported for truncated exec files.
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Apr 17, 2016

Member

@bjkail @Godin Agree on all comments, Thx! I updated the commit.

Member

marchof commented Apr 17, 2016

@bjkail @Godin Agree on all comments, Thx! I updated the commit.

GitHub #397: Improve EOF handling for exec files.
A EOFException should be reported for truncated exec files.

@Godin Godin merged commit db83743 into master Apr 17, 2016

2 checks passed

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

@Godin Godin deleted the issue-397 branch Apr 17, 2016

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017

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