[WIP] [SPARK-19552] [BUILD] Upgrade Netty version to 4.1.8 final #16888
Conversation
|
Jenkins, retest this please |
|
Test build #72715 has finished for PR 16888 at commit
|
|
Shouldn't we use netty-4.0.44.Final rather than 4.1.x? |
|
BTW for Netty we shouldn't just bump to the highest version. We should use the maintenance branches. |
|
Updating to 4.0.44 should be fine (though, we got bit even by a maintenance update behavior change a while ago) but apparently that doesn't include the security fix in question. That said, it's not clear what the security issue is or whether it affects Spark -- can we start with that? Updating to 4.1.x is probably a good idea, eventually, even if it's not necessary, but it will require some care and testing. It sounds like this should be marked as CC @zsxwing |
|
Netty 4.1.1 and above has the fix in (a change to OpenSslEngine)- Netty 4.0.x does not, and the implementations of OpenSslEngine differs substantially between the releases (so a simple case of adding in the fix would be non-trivial and potentially lead to complications, this would be the path of least resistance) You can find more info on the CVE I'm concerned with at: https://www.versioneye.com/java/io.netty:netty-all/4.0.43.Final To be impacted it sounds as though we need to do several things - use Spark with Netty 4.1.0 and below (including the 4.0.x releases), have tcnative on our classpath and we specify to use the OpenSslEngine (there's a useful overview on why you'd want to do this here which mentions it being a drop-in replacement for the JDK classes and offers superior performance). Info on using tcnative here We don't include any of the netty/tcnative native libraries for Spark so I don't think we're impacted - but moving up to take on other fixes as well would be useful (and if it is so much faster we could see shuffle-intensive workloads speedup by upgrading and including natives later on). I'll set this as a WIP and look into why the tests fail. |
a-roberts
changed the title
|
OK, so there isn't actually a security case here (might update the JIRA accordingly). Updating to 4.1.x seems like a fine thing to try. If you can get it to work well, great. |
|
Are there specific benefits brought by updating to 4.1 of Netty? Netty is so core to Spark that any bug in it would be extremely difficult to debug (yes we have founds bugs in Netty and helped fix those in the past). So unless there are significant benefits, please do NOT update to 4.1. |
|
@a-roberts Spark doesn't use Netty's SSL handler, so it won't be a problem. |
|
I'll close this one then until there ever is a need to upgrade, cheers |
|
|
||
| @Override | ||
| public FileRegion retain() { | ||
| return this; |
There was a problem hiding this comment.
I believe this breaks the netty reference counting logic, I think instead it should be
return (TestFileRegion)super.retain();
|
|
||
| @Override | ||
| public FileRegion retain(int increment) { | ||
| return this; |
There was a problem hiding this comment.
Same as above, I think this should be
return (TestFileRegion)super.retain(increment);
|
taken over by #19884 ? |
|
Agree, this can be closed. |
What changes were proposed in this pull request?
We should use Netty 4.1.8 due to one security concern and other bug fixes - we'll need to change Spark code as well as the usual misc files (like pom.xml and deps files) to enable this
How was this patch tested?
Existing unit tests - this is a WIP as this introduces some new test failures and I have dummy implementations for the required methods I've added.