-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Explicitly tell Netty to not use unsafe #19786
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
Conversation
With the security permissions that we grant to Netty, Netty can not access unsafe (because it relies on having the runtime permission accessDeclaredMembers and the reflect permission suppressAccessChecks). Instead, we should just explicitly tell Netty to not use unsafe. This commit adds a flag to the default jvm.options to tell Netty to not look for unsafe.
|
Additionally, Netty looking for and not being able to access unsafe leads to log messages that might be scary to the end-user on startup: I've opened netty/netty#5624 to not log this message when unsafe is explicitly disabled, as we are configuring with this PR. |
|
LGTM |
|
no unsafe 😢 Out of curiosity ... do you guys have perf benchmarks you can report with and without unsafe to understand the impacts of unsafe? |
|
@Scottmitch that is actually a good thing. I am so happy we don't allow it to do that. Also it's not a regression it's been like this since 2.0 I guess |
|
my assumption is that performance is traded for safety/security/isolation/etc... this why I'm curious to see if benchmarking is available to confirm this suspicion. I'm not familiar with elastic and just followed this from a Netty issue ... I'm not claiming its a regression, good/bad in this context, and didn't mean to hijack the thread ... just curious :) |
|
@Scottmitch I tested the performance difference (after granting a lot of scary permissions 😇). For the performance metrics that we care about (indexing throughput, query latency, GC metrics, etc.), the benchmarks with unsafe were inline with the benchmarks with no unsafe, effectively no difference at all. For us, using unsafe provides zero benefit but has a huge cost. I did notice when I ran these benchmarks that there are several places where executing the privileged code is not handled properly within Netty. I'll open a PR against Netty to address. |
|
@jasontedor - thanks for following up ... looking forward to your PR :) |
|
@Scottmitch I opened netty/netty#5640 and netty/netty#5641. |
|
Sorry for being a little bit late to the party, but do you have pointers to specific issues caused by letting netty use |
|
I was a bit unclear in my previous post. For clarification: my question is just related to the transport client, and not a complete ES node. |
With the security permissions that we grant to Netty, Netty can not
access unsafe (because it relies on having the runtime permission
accessDeclaredMembers and the reflect permission
suppressAccessChecks). Instead, we should just explicitly tell Netty to
not use unsafe. This commit adds a flag to the default jvm.options to
tell Netty to not look for unsafe.
Relates #19562, relates netty/netty#5624