- Notifications
You must be signed in to change notification settings - Fork1.6k
Fix NPE race in NettyResponseFuture.cancel (#2042)#2088
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
HI@hyperxpro ... |
client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR fixes a race condition that could lead to a NullPointerException when cancelling a response by copying the channel reference to a local variable.
- Introduces a local variable "ch" to capture the channel in one atomic operation.
- Prevents a TOCTOU race condition by ensuring the local reference remains valid during subsequent operations.
73911eb intoAsyncHttpClient:mainUh oh!
There was an error while loading.Please reload this page.
Fixes#2042
This is a typical TOCTOU (time-of-check/time-of-use) racehttps://en.wikipedia.org/wiki/Time-of-check_to_time-of-use.
The NPE was occurring because the channel field could be set to null by another thread between the check and its use:
if (channel != null) { // time-of-check
Channels.setDiscard(channel); // time-of-use
Channels.silentlyCloseChannel(channel);
}
By copying channel into a local variable in one atomic read, we ensure that—even if another thread changes the field—the local reference remains valid.
P.S. It is hard to write a deterministic test that fails consistently, so this PR only includes the code fix.