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

fix snappy crc32c checksum #10048

Merged
merged 1 commit into from Feb 27, 2020

Conversation

andyxning
Copy link
Contributor

Motivation:

Explain here the context, and why you're making that change.
What is the problem you're trying to solve.

Modification: The Snappy crc32c checksum produced by SnappyFrameEncoder maybe failed to be validated on other languages snappy decoder, such as golang/snappy.

Describe the modifications you've done.

Result:

Fixes #.

If there is no issue then describe the changes introduced by this PR.

This will make the 4-byte cast later after the mask operation. Because whether retaining the higher 4-7 bytes in a long java type will make difference in (checksum >> 15 | checksum << 17) + 0xa282ead8 result.

@netty-bot
Copy link

Can one of the admins verify this patch?

@andyxning
Copy link
Contributor Author

/cc @trustin @normanmaurer

@normanmaurer
Copy link
Member

@andyxning can you add a testcase ?

@andyxning
Copy link
Contributor Author

@normanmaurer Will do.

@normanmaurer
Copy link
Member

@andyxning please ping me once done.

@andyxning
Copy link
Contributor Author

@normanmaurer Update is done. PTAL.

@andyxning
Copy link
Contributor Author

@normanmaurer If this PR will be merged, when will the next release will be cut?

@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer
Copy link
Member

@andyxning can you please fix the checkstyle error ?:

20:51:33 [INFO] Starting audit...
20:51:33 [ERROR] /code/codec/src/main/java/io/netty/handler/codec/compression/Snappy.java:659:20: 'typecast' is not followed by whitespace. [WhitespaceAfter]
20:51:33 [ERROR] /code/codec/src/test/java/io/netty/handler/codec/compression/SnappyTest.java:244: Line is longer than 120 characters (found 129). [LineLength]
20:51:33 Audit done.

@andyxning
Copy link
Contributor Author

@normanmaurer Comments addressed. PTAL again.

@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer
Copy link
Member

@andyxning I just noticed you did not sign our ICLA yet... Can you do and let me know once done ?

https://netty.io/s/icla

@normanmaurer
Copy link
Member

@andyxning there are test failures... not sure if your change is incorrect or if the tests needs to be fixed ?

@andyxning
Copy link
Contributor Author

@normanmaurer CLA is Done. codes are updated.

@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer
Copy link
Member

@andyxning there are still failures

@johnou
Copy link
Contributor

johnou commented Feb 26, 2020

Test Result (4 failures / ±0)

io.netty.handler.codec.compression.SnappyFrameDecoderTest.testInvalidChecksumDoesNotThrowException
io.netty.handler.codec.compression.SnappyFrameEncoderTest.testStreamStartIsOnlyWrittenOnce
io.netty.handler.codec.compression.SnappyFrameEncoderTest.testSmallAmountOfDataIsUncompressed
io.netty.handler.codec.compression.SnappyTest.testCalculateChecksum

@andyxning
Copy link
Contributor Author

@netty-bot test this please

@andyxning
Copy link
Contributor Author

@normanmaurer Ut are resolved. PTAL. This is the last time that i will ping you for reviewing this PR, i promise. :-)

@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer normanmaurer merged commit a304d61 into netty:4.1 Feb 27, 2020
@normanmaurer
Copy link
Member

@andyxning thanks a lot!

@normanmaurer normanmaurer added this to the 4.1.46.Final milestone Feb 27, 2020
normanmaurer pushed a commit that referenced this pull request Feb 27, 2020
Motivation:

The Snappy crc32c checksum produced by SnappyFrameEncoder maybe failed to be validated on other languages snappy decoder, such as golang/snappy.

Modification: 

- make the 4-byte cast later after the mask operation. Because whether retaining the higher 4-7 bytes in a long java type will make difference in (checksum >> 15 | checksum << 17) + 0xa282ead8 result.

Result:

Checksum correctly calculated
@andyxning andyxning deleted the fix_snappy_crc32c_checksum branch February 27, 2020 08:24
@andyxning
Copy link
Contributor Author

@normanmaurer When will the next release will be cut and a new jar will be pushed to maven?

@martinfurmanski
Copy link
Contributor

Is this a breaking change?

@normanmaurer
Copy link
Member

@martinfurmanski I dont think so... Why you think it is ? To me it seems like we did not correctly calculate the checksum before.

@martinfurmanski
Copy link
Contributor

martinfurmanski commented Mar 2, 2020

@normanmaurer I didn't look at the details, but what I mean is if the prior patch version of netty is talking with this version, are they now calculating differently and can mismatch?

@normanmaurer
Copy link
Member

@martinfurmanski I didn't think about this... yes I think this could happen if the crc value overflows Integer.MAX_VALUE.

ihanyong pushed a commit to ihanyong/netty that referenced this pull request Jul 31, 2020
Motivation:

The Snappy crc32c checksum produced by SnappyFrameEncoder maybe failed to be validated on other languages snappy decoder, such as golang/snappy.

Modification: 

- make the 4-byte cast later after the mask operation. Because whether retaining the higher 4-7 bytes in a long java type will make difference in (checksum >> 15 | checksum << 17) + 0xa282ead8 result.

Result:

Checksum correctly calculated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants