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

Verify actually written checksum in VerifyingIndexOutput #13848

Merged
merged 1 commit into from Sep 30, 2015

Conversation

Projects
None yet
5 participants
@s1monw
Copy link
Contributor

commented Sep 29, 2015

today we don't verify that the actual checksum written to VerifyingIndexOutput
is the actual checksum we are expecting.

@dakrone

View changes

core/src/main/java/org/elasticsearch/index/store/Store.java Outdated
}

@Override
public void verify() throws IOException {
String footerChecksum = null;

This comment has been minimized.

Copy link
@dakrone

dakrone Sep 29, 2015

Member

I find it really confusing to have footerChecksum at the top-level that is a byte array and footerChecksum here that is a string, with only this. to indicate which one is being used.

@rmuir

View changes

core/src/main/java/org/elasticsearch/index/store/Store.java Outdated
long checksumIndex = writtenBytes - checksumPosition;
for (int i = 0;checksumIndex < footerChecksum.length && i<length; checksumIndex++, i++) { // checksum is written
footerChecksum[(int)checksumIndex] = b[offset+i];
}

This comment has been minimized.

Copy link
@rmuir

rmuir Sep 29, 2015

Contributor

Can we use a less complex loop structure?

This comment has been minimized.

Copy link
@mikemccand

mikemccand Sep 29, 2015

Contributor

And also catch here if there's an attempt to write beyond the checksum?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Sep 29, 2015

Member

What do you think of simplifying the last few lines with the following?

int checksumIndex = Math.toIntExact(writtenBytes - checksumPosition);
System.arraycopy(b, offset, footerChecksum, checksumIndex, length);
@mikemccand

View changes

core/src/main/java/org/elasticsearch/index/store/Store.java Outdated
final long index = writtenBytes - checksumPosition;
if (index < footerChecksum.length) {
footerChecksum[(int)index] = b;
}

This comment has been minimized.

Copy link
@mikemccand

mikemccand Sep 29, 2015

Contributor

Can we throw an exc in an else clause here? I.e., we should never try to write beyond the checksum...?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Sep 29, 2015

Member

Or use Math.toIntExact on writtenBytes - checksumPosition and do away with the extra if and else (less lines of code)?

int index = Math.toIntExact(writtenBytes - checksumPosition);
footerChecksum[index] = b;

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 29, 2015

Author Contributor

write past EOF 👯

This comment has been minimized.

Copy link
@mikemccand

mikemccand Sep 29, 2015

Contributor

write past EOF 👯

+1!

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2015

I pushed a new commit @jasontedor @dakrone @rmuir @mikemccand

offset += bytesToWrite;
length -= bytesToWrite;
writtenBytes += bytesToWrite;
if (writtenBytes + length > checksumPosition) {

This comment has been minimized.

Copy link
@rmuir

rmuir Sep 29, 2015

Contributor

do we think its worth it to optimize the last 8 bytes of the file, or should we just do the naive thing and handle the last 8 bytes case via a naive loop of writeByte() for each byte, so that the footer logic is only in one place?

This comment has been minimized.

Copy link
@mikemccand

mikemccand Sep 29, 2015

Contributor

should we just do the naive thing and handle the last 8 bytes case via a naive loop of writeByte() for each byte, so that the footer logic is only in one place?

+1

This comment has been minimized.

Copy link
@rmuir

rmuir Sep 30, 2015

Contributor

thanks for improving this, this part is easier to read now IMO.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2015

@mikemccand @rmuir pushed a new commit

if (index < footerChecksum.length) {
footerChecksum[index] = b;
} else {
verify(); // fail if we write more than expected

This comment has been minimized.

Copy link
@mikemccand

mikemccand Sep 30, 2015

Contributor

Maybe assert false here to be certain this verify() call threw exc?

This comment has been minimized.

Copy link
@rmuir

rmuir Sep 30, 2015

Contributor

Personally in code like this I would just throw AssertionError. it make code uglier in many cases to do this, but there are times where its better to be paranoid...

@mikemccand

View changes

core/src/test/java/org/elasticsearch/index/store/StoreTests.java Outdated
}
IOUtils.close(indexInput, verifyingOutput, dir);
}

This comment has been minimized.

Copy link
@mikemccand

mikemccand Sep 30, 2015

Contributor

Can we add another test case for the WritePastEOF case?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2015

@mikemccand next iteration pushed

@mikemccand

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2015

LGTM, thanks @s1monw!

Verify actually written checksum in VerifyingIndexOutput
today we don't verify that the actual checksum written to VerifyingIndexOutput
is the actual checksum we are expecting.

@s1monw s1monw force-pushed the s1monw:verify_written_checksum branch to c185a12 Sep 30, 2015

s1monw added a commit that referenced this pull request Sep 30, 2015

Merge pull request #13848 from s1monw/verify_written_checksum
Verify actually written checksum in VerifyingIndexOutput

@s1monw s1monw merged commit f526754 into elastic:master Sep 30, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@s1monw s1monw deleted the s1monw:verify_written_checksum branch Sep 30, 2015

s1monw added a commit that referenced this pull request Sep 30, 2015

Merge pull request #13848 from s1monw/verify_written_checksum
Verify actually written checksum in VerifyingIndexOutput

s1monw added a commit that referenced this pull request Sep 30, 2015

Merge pull request #13848 from s1monw/verify_written_checksum
Verify actually written checksum in VerifyingIndexOutput

s1monw added a commit to s1monw/elasticsearch that referenced this pull request Oct 4, 2015

Record all bytes of the checksum in VerifyingIndexOutput
The fix in elastic#13848 has an off by one issue where the first byte of the checksum
was never written. Unfortunately most tests shadowed the problem and the first
byte of the checksum seems to be very likely a 0 which causes only very rare
failures.

Relates to elastic#13896
Relates to elastic#13848

@s1monw s1monw added the v2.2.0 label Oct 4, 2015

s1monw added a commit that referenced this pull request Oct 5, 2015

Record all bytes of the checksum in VerifyingIndexOutput
The fix in #13848 has an off by one issue where the first byte of the checksum
was never written. Unfortunately most tests shadowed the problem and the first
byte of the checksum seems to be very likely a 0 which causes only very rare
failures.

Relates to #13896
Relates to #13848

s1monw added a commit that referenced this pull request Oct 5, 2015

Record all bytes of the checksum in VerifyingIndexOutput
The fix in #13848 has an off by one issue where the first byte of the checksum
was never written. Unfortunately most tests shadowed the problem and the first
byte of the checksum seems to be very likely a 0 which causes only very rare
failures.

Relates to #13896
Relates to #13848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.