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

Explicitly tell Netty to not use unsafe #19786

Merged
merged 1 commit into from
Aug 3, 2016
Merged

Explicitly tell Netty to not use unsafe #19786

merged 1 commit into from
Aug 3, 2016

Conversation

jasontedor
Copy link
Member

@jasontedor jasontedor commented Aug 3, 2016

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

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.
@jasontedor jasontedor added >enhancement review :Distributed/Network Http and internode communication implementations v5.0.0-beta1 labels Aug 3, 2016
@jasontedor
Copy link
Member Author

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:

[2016-08-03 12:59:34,765][INFO ][io.netty.util.internal.PlatformDependent] Your platform does not provide complete low-level API for accessing direct buffers reliably. Unless explicitly requested, heap buffer will always be preferred to avoid potential system unsuitability.

I've opened netty/netty#5624 to not log this message when unsafe is explicitly disabled, as we are configuring with this PR.

@dakrone
Copy link
Member

dakrone commented Aug 3, 2016

LGTM

@jasontedor jasontedor merged commit eb6da69 into elastic:master Aug 3, 2016
@jasontedor jasontedor mentioned this pull request Aug 3, 2016
5 tasks
@jasontedor jasontedor deleted the netty-no-unsafe branch August 3, 2016 19:26
@Scottmitch
Copy link

no unsafe 😢

Out of curiosity ... do you guys have perf benchmarks you can report with and without unsafe to understand the impacts of unsafe?

@s1monw
Copy link
Contributor

s1monw commented Aug 3, 2016

@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

@Scottmitch
Copy link

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 :)

@jasontedor
Copy link
Member Author

@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.

@Scottmitch
Copy link

@jasontedor - thanks for following up ... looking forward to your PR :)

@jasontedor
Copy link
Member Author

jasontedor commented Aug 5, 2016

@Scottmitch I opened netty/netty#5640 and netty/netty#5641.

@slovdahl
Copy link

Sorry for being a little bit late to the party, but do you have pointers to specific issues caused by letting netty use sun.misc.Unsafe? I'm wondering because I have another transitive dependency on netty in my environment that seems to depend on having a working sun.misc.Unsafe.

@slovdahl
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >enhancement v5.0.0-alpha5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants