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
Possible Race in JdkSslEngine #4832
Comments
@carl-mastrangelo I'm a bit confused for this one as the same SslHandler instance should only be called from the same Thread. So how could this be a race ? |
Oops, forgot the header of the second one: It is actually being hit from another thread. |
And it's the same SslHandler ? Are you sure ? Also is this the full stacktrace ? |
Which is spawned from:
The second thread (T44) is:
which is spawned by
|
@carl-mastrangelo sorry for all the question but how can I see that it is the same instance of SslHandler ? From the stacktraces it is always called because of |
It may be that the description of this bug is wrong, and the bug is somewhere else. The memory address is the same in both cases, and looks to be triggered by a write followed by a read on separate threads. |
@carl-mastrangelo hmm... both traces have |
In the top most stack trace, it has the header:
Which is why I suspect it is a RaW hazard. |
I looked into this again when rerunning the race detector. I see the issue clearly now: The Buffer being modified is Unpooled.EMPTY_BUFFER. The empty buffer has a nioBuffer attached which is concurrently modified in two places by the SSLEngine. I think this is benign, but it adds noise to the race detector output, and could possibly be used to frighten small shildren. Is there any reason EMPTY_BUFFER has to be backed by an actual nioBuffer? Could it work without one? Side note: is it even safe to call SSLEngine concurrently? Isn't SSL data inherently linear and dependent on previous data? |
It needs to be backed by one as SSLEngine works on ByteBuffer.
|
Would it make more sense for the SSL Handler to just make's its own empty buffer, since the SSLEngine is going to be modifying it? |
How is it modifing it? Its empty with a limit and capacity of 0. there is techical no way to modify it
|
The call to |
Alternatively, Buffer and ByteBuffer are not final. It would be easy to make a "empty" noop Buffer to just ignore all calls and have that be the empty buffer. |
These are not final but constructor is package private so nope this is not possible
|
I think it is fine as it is... @carl-mastrangelo please reopen if you not agree |
Following from my previously filed issues, the conflicting thread access stack traces:
and
Hazarding a guess looks like it might be SslHandler.singleBuffer
cc: @nmittler & @normanmaurer
The text was updated successfully, but these errors were encountered: