[crc64] Fix a subtle bug in CRC64 splice-by-8 implementation #627
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context: 9b88ce7 9b88ce7 implements a splice-by-8 version of the CRC64 algorithm which processes 8 bytes per loop iteration. However, the code had a subtle bug which resulted in the same CRC calculated for different strings and made some of Xamarin.Android tests to fail. The following strings yielded the same checksum value: * obj/Debug/lp/10/jl/bin/classes.jar * obj/Debug/lp/11/jl/bin/classes.jar * obj/Debug/lp/12/jl/bin/classes.jar The raeson for this was subtle (a stupid oversight on my part, really): the loop fetched values from the input array by using an index into the array: crc ^= (ulong)aptr[idx]; However, with `aptr` being declared as `byte* aptr` the indexing operation returned a single **byte** instead of the required 64-bit word (8 bytes) and, thus, on each iteration of the loop 7 bytes of the input arrays were ignored in calculation, thus causing the collisions. The fix is to cast `aptr` to `ulong*` and **then** index it, extracting the required 8 bytes. This commit also adds a test to check for this issue.
jonpryor
pushed a commit
that referenced
this pull request
Apr 22, 2020
Commit 9b88ce7 implemented a splice-by-8 version of the CRC64 algorithm which processes 8 bytes per loop iteration. However, the code had a subtle bug which resulted in the same CRC calculated for different strings and made some of the Xamarin.Android tests to fail. The following strings yielded the same checksum value: * `obj/Debug/lp/10/jl/bin/classes.jar` * `obj/Debug/lp/11/jl/bin/classes.jar` * `obj/Debug/lp/12/jl/bin/classes.jar` The reason for this was subtle (a stupid oversight on my part, really): the loop fetched values from the input array by using an index into the array: crc ^= (ulong)aptr[idx]; However, with `aptr` being declared as `byte* aptr` the indexing operation returned a single *byte* instead of the required 64-bit word (8 bytes) and, thus, on each iteration of the loop 7 bytes of the input arrays were ignored in calculation, thus causing the collisions. The fix is to cast `aptr` to `ulong*` and *then* index it, extracting the required 8 bytes. This commit also adds a test to check for this issue.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: 9b88ce7
9b88ce7 implements a splice-by-8 version of the CRC64 algorithm which
processes 8 bytes per loop iteration. However, the code had a subtle bug
which resulted in the same CRC calculated for different strings and made
some of Xamarin.Android tests to fail. The following strings yielded the
same checksum value:
The raeson for this was subtle (a stupid oversight on my part, really):
the loop fetched values from the input array by using an index into the
array:
However, with
aptrbeing declared asbyte* aptrthe indexingoperation returned a single byte instead of the required 64-bit
word (8 bytes) and, thus, on each iteration of the loop 7 bytes of the
input arrays were ignored in calculation, thus causing the collisions.
The fix is to cast
aptrtoulong*and then index it, extractingthe required 8 bytes.
This commit also adds a test to check for this issue.