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

Loosen bounds check on CompositeByteBuf's maxNumComponents #8621

Merged
merged 2 commits into from Dec 5, 2018

Conversation

Projects
None yet
4 participants
@nicktrav
Copy link
Member

nicktrav commented Dec 4, 2018

Motivation:

In versions of Netty prior to 4.1.31.Final, a CompositeByteBuf could be
created with any size (including potentially nonsensical negative
values). This behavior changed in e7737b9, which introduced a bounds
check to only allow for a component size greater than one. This broke
some existing use cases that attempted to create a byte buf with a
single component.

Modifications:

Lower the bounds check on numComponents to include the single component
case, but still throw an exception for anything less than one.

Add unit tests for the case of numComponents being less than, equal to,
and greater than this lower bound.

Result:

Return to the behavior of 4.1.30.Final, allowing one component, but
still include an explicit check against a lower bound.

Note that while creating a CompositeByteBuf with a single component is
in some ways a contradiction of the term "composite", this patch caters
for existing uses while excluding the clearly nonsensical case of asking
for a CompositeByteBuf with zero or fewer components.

Fixes #8613.

Loosen bounds check on CompositeByteBuf's maxNumComponents
Motivation:

In versions of Netty prior to 4.1.31.Final, a CompositeByteBuf could be
created with any size (including potentially nonsensical negative
values). This behavior changed in e7737b9, which introduced a bounds
check to only allow for a component size greater than one. This broke
some existing use cases that attempted to create a byte buf with a
single component.

Modifications:

Lower the bounds check on numComponents to include the single component
case, but still throw an exception for anything less than one.

Add unit tests for the case of numComponents being less than, equal to,
and greater than this lower bound.

Result:

Return to the behavior of 4.1.30.Final, allowing one component, but
still include an explicit check against a lower bound.

Note that while creating a CompositeByteBuf with a single component is
in some ways a contradiction of the term "composite", this patch caters
for existing uses while excluding the clearly nonsensical case of asking
for a CompositeByteBuf with zero or fewer components.

Fixes #8613.
@netty-bot

This comment has been minimized.

Copy link

netty-bot commented Dec 4, 2018

Can one of the admins verify this patch?

@nicktrav nicktrav requested a review from normanmaurer Dec 4, 2018

@nicktrav

This comment has been minimized.

Copy link
Member Author

nicktrav commented Dec 4, 2018

cc: @njhill

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 4, 2018

@netty-bot test this please

@njhill

njhill approved these changes Dec 4, 2018

Copy link
Contributor

njhill left a comment

LGTM, thanks @nicktrav. I will defer to @normanmaurer on this but instead of adding a new test class you could consider adding the tests to the existing AbstractCompositeByteBufTest which already contains a bunch of similar ones.

private static void assertCompositeBufCreated(int expectedMaxComponents) {
CompositeByteBuf buf = new CompositeByteBuf(ALLOC, true, expectedMaxComponents);

assertNotNull(buf);

This comment has been minimized.

@njhill

njhill Dec 4, 2018

Contributor

no need for this check

This comment has been minimized.

@nicktrav

nicktrav Dec 4, 2018

Author Member

derp. of course. fixed.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 4, 2018

@nicktrav @njhill has a good point here... maybe do this :)

@nicktrav

This comment has been minimized.

Copy link
Member Author

nicktrav commented Dec 4, 2018

Moved the tests 👍 .

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 4, 2018

@netty-bot test this please

@normanmaurer normanmaurer merged commit d0d30f1 into netty:4.1 Dec 5, 2018

4 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
pull request validation (centos6-java9) Build finished.
Details

@normanmaurer normanmaurer added this to the 4.1.33.Final milestone Dec 5, 2018

@normanmaurer normanmaurer self-assigned this Dec 5, 2018

normanmaurer added a commit that referenced this pull request Dec 5, 2018

Loosen bounds check on CompositeByteBuf's maxNumComponents (#8621)
Motivation:

In versions of Netty prior to 4.1.31.Final, a CompositeByteBuf could be
created with any size (including potentially nonsensical negative
values). This behavior changed in e7737b9, which introduced a bounds
check to only allow for a component size greater than one. This broke
some existing use cases that attempted to create a byte buf with a
single component.

Modifications:

Lower the bounds check on numComponents to include the single component
case, but still throw an exception for anything less than one.

Add unit tests for the case of numComponents being less than, equal to,
and greater than this lower bound.

Result:

Return to the behavior of 4.1.30.Final, allowing one component, but
still include an explicit check against a lower bound.

Note that while creating a CompositeByteBuf with a single component is
in some ways a contradiction of the term "composite", this patch caters
for existing uses while excluding the clearly nonsensical case of asking
for a CompositeByteBuf with zero or fewer components.

Fixes #8613.
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Dec 5, 2018

@nicktrav thanks a lot buddy!

@nicktrav nicktrav deleted the nicktrav:nickt.composite-byte-buf branch Dec 5, 2018

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