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

Avoid possible comparison contract violation #9883

Merged
merged 2 commits into from Dec 19, 2019

Conversation

@gerdriesselmann
Copy link
Contributor

gerdriesselmann commented Dec 13, 2019

The current implementation would violate comparison contract for two cookies C1 and C2 with same path length, since C1 < C2 and C2 < C1. Returning 0 (equality) does not since C1 == C2 and C2 == C1. See #9881

Motivation:

The current implementation causes IllegalArgumetExceptions to be thrown on Java 11.

Modification:

Return equality instead of less than on same path length.

Result:

Fixes #9881.

The current implementation would violate comparison contract for two cookies C1 and C2 with same path length, since C1 < C2 and C2 < C1. Returning 0 (equality) does not since C1 == C2 and C2 == C1. See #9881
@netty-bot

This comment has been minimized.

Copy link

netty-bot commented Dec 13, 2019

Can one of the admins verify this patch?

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 13, 2019

@gerdriesselmann can't you add a test case that validates 0 is returned in this case ?

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 13, 2019

@gerdriesselmann also please sign our icla if you didn't already: https://netty.io/s/icla

@gerdriesselmann

This comment has been minimized.

Copy link
Contributor Author

gerdriesselmann commented Dec 13, 2019

@normanmaurer icla was signed.

Regarding a test: since this is a private class of ClientCookieEncoder, I have no real idea how to write a test against it.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 13, 2019

@gerdriesselmann make it package-private :)

@gerdriesselmann

This comment has been minimized.

Copy link
Contributor Author

gerdriesselmann commented Dec 13, 2019

@normanmaurer To be honest, I'm doing Scala, but am rather alienated by the Java language and its tooling in general and Maven especially. I tried to execute the existing ClientCookieEncoderTest within IntelliJ, but can't compile against Java 11, and get "Unrecognized option: --illegal-access=deny" against Java 8. Additionally, as soon as I'm changing something (like adding a test case), a CheckStyle plugin jumps in and stops me from progressing. I can't even figure out what exactly it complains about...

Sorry to be of no further help here...

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 19, 2019

@netty-bot test this please

@normanmaurer normanmaurer added this to the 4.1.45.Final milestone Dec 19, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 19, 2019

@gerdriesselmann I will add a unit test as followup once this is in. Thanks!

@normanmaurer normanmaurer merged commit f995426 into netty:4.1 Dec 19, 2019
3 checks passed
3 checks passed
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 19, 2019

@gerdriesselmann merged... thanks a lot.

normanmaurer added a commit that referenced this pull request Dec 19, 2019
Motivation:

The current implementation causes IllegalArgumetExceptions to be thrown on Java 11.

The current implementation would violate comparison contract for two cookies C1 and C2 with same path length, since C1 < C2 and C2 < C1. Returning 0 (equality) does not since C1 == C2 and C2 == C1. See #9881

Modification:

Return equality instead of less than on same path length.

Result:

Fixes #9881.
normanmaurer added a commit that referenced this pull request Dec 19, 2019
Motivation:

#9883 added a bug-fix for the Comparator in ClientCookieEncoder but did not add a testcase.

Modifications:

- Add testcase
- Simplify code

Result:

Include a test to ensure we not regress.
normanmaurer added a commit that referenced this pull request Dec 20, 2019
Motivation:

#9883 added a bug-fix for the Comparator in ClientCookieEncoder but did not add a testcase.

Modifications:

- Add testcase
- Simplify code

Result:

Include a test to ensure we not regress.
normanmaurer added a commit that referenced this pull request Dec 20, 2019
Motivation:

#9883 added a bug-fix for the Comparator in ClientCookieEncoder but did not add a testcase.

Modifications:

- Add testcase
- Simplify code

Result:

Include a test to ensure we not regress.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.