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

prev should be set to null when setting cursor to head in scavengeSome #6180

Closed
wants to merge 1 commit into from
Closed

prev should be set to null when setting cursor to head in scavengeSome #6180

wants to merge 1 commit into from

Conversation

zsxwing
Copy link
Contributor

@zsxwing zsxwing commented Jan 6, 2017

Set prev to null when setting cursor to head in scavengeSome to fix #6153.

Motivation:

scavengeSome() has a corner case: when setting cursor to head, this.prev may point to the tail of the WeakOrderQueue linked list. Then it's possible that the following while loop will link the tail to the head, and cause endless loop.

I made a reproducer in zsxwing@36522e7 . The unit test will just run forever. Unfortunately, I cannot change it to a unit test because it needs to add some codes to scavengeSome to control the execution flow.

Modification:

Set prev to null when setting cursor to head in scavengeSome.

Result:

Fixes #6153.

@zsxwing
Copy link
Contributor Author

zsxwing commented Jan 6, 2017

@normanmaurer PTAL

@normanmaurer
Copy link
Member

@zsxwing awesome!

Can you please sign the ICLA and update the community message :

http://netty.io/s/icla
http://netty.io/wiki/writing-a-commit-message.html

Let me know once done

@zsxwing
Copy link
Contributor Author

zsxwing commented Jan 6, 2017

Already signed it.

@zsxwing
Copy link
Contributor Author

zsxwing commented Jan 6, 2017

Updated the commit message.

Motivation:

`scavengeSome()` has a corner case: when setting `cursor` to `head`, `this.prev` may point to the tail of the  `WeakOrderQueue` linked list. Then it's possible that the following while loop will link the tail to the head, and cause endless loop.

I made a reproducer in zsxwing@36522e7 . The unit test will just run forever. Unfortunately, I cannot change it to a unit test because it needs to add some codes to `scavengeSome` to control the execution flow.

Modification:

Set `prev` to null when setting `cursor` to `head` in `scavengeSome`

Result:

Fixes #6153.
@normanmaurer normanmaurer self-requested a review January 7, 2017 19:47
@normanmaurer normanmaurer self-assigned this Jan 7, 2017
@normanmaurer normanmaurer added this to the 4.0.43.Final milestone Jan 7, 2017
@normanmaurer
Copy link
Member

Cherry-picked into 4.1 (2457f38) and 4.0 (735b83b)

@zsxwing thanks and good catch!

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

Successfully merging this pull request may close these issues.

Potential endless loop in Recycler.Stack.scavengeSome
2 participants