[SPARK-25535][core] Work around bad error handling in commons-crypto. #22557
Conversation
The commons-crypto library does some questionable error handling internally, which can lead to JVM crashes if some call into native code fails and cleans up state it should not. While the library is not fixed, this change adds some workarounds in Spark code so that when an error is detected in the commons-crypto side, Spark avoids calling into the library further. Tested with existing and added unit tests.
|
Test build #96637 has finished for PR 22557 at commit
|
|
Test build #96639 has finished for PR 22557 at commit
|
squito
left a comment
squito
left a comment
There was a problem hiding this comment.
unrelated, but shouldn't EncryptedMessage.transferTo() not keep looping if target.write() doesn't accept all the data?
| } | ||
| intercept[IOException] { | ||
| errorHandler.read(out) | ||
| } |
There was a problem hiding this comment.
since you're throwing a mock IOException above, this would be a little more clear if here you checked the message too
assert(intercept[IOException] {
errorHandler.read(out)
}.getMessage() === ("Cipher stream is closed."))(you can also use assertThrows if you're not looking at the exception at all, though doesn't really matter)
| private var closed = false | ||
|
|
||
| protected def cipherStream: Closeable | ||
| protected def wrapped: Closeable |
There was a problem hiding this comment.
its a little confusing that there are two closeables. I'd add a comment here that wrapped is what the cipherStream is already wrapping, and we want to make sure it gets closed if the cipherStream has an internal error.
(when I see a variable named wrapped, I assumed it was what this was wrapping, not what the cipherStream was wrapping)
| if (mode == Cipher.ENCRYPT_MODE) { | ||
| this.encryptor = null; | ||
| } else { | ||
| this.decryptor = null; |
There was a problem hiding this comment.
any particular reason to set these to null, rather than having an isValid flag in here? you'd get an NPE if you ever tried to use the TranportCipher after this -- you are protecting against that in the wrapping code, but seems you could do it here.
There was a problem hiding this comment.
I can add a check that the ciphers are not null, but no need to have a separate flag. These need to be set to null so that close knows not to try to mess with them.
There was a problem hiding this comment.
Actually the check is already there I'll just add a message to it.
|
Test build #96714 has finished for PR 22557 at commit
|
|
retest this please |
|
Test build #96720 has finished for PR 22557 at commit
|
Yes I noticed that too, but separate change. Hopefully I can fix other stuff in commons-crypto that would allow more of that code to be cleaned up... |
|
lgtm will leave for a day before mergning |
|
Ping |
|
merged to master |
The commons-crypto library does some questionable error handling internally, which can lead to JVM crashes if some call into native code fails and cleans up state it should not. While the library is not fixed, this change adds some workarounds in Spark code so that when an error is detected in the commons-crypto side, Spark avoids calling into the library further. Tested with existing and added unit tests. Closes apache#22557 from vanzin/SPARK-25535. Authored-by: Marcelo Vanzin <[email protected]> Signed-off-by: Imran Rashid <[email protected]>
|
Hi, All. |
|
+1 |
|
Sure. |
…mons-crypto. The commons-crypto library does some questionable error handling internally, which can lead to JVM crashes if some call into native code fails and cleans up state it should not. While the library is not fixed, this change adds some workarounds in Spark code so that when an error is detected in the commons-crypto side, Spark avoids calling into the library further. Tested with existing and added unit tests. Closes apache#22557 from vanzin/SPARK-25535. Authored-by: Marcelo Vanzin <[email protected]> Signed-off-by: Imran Rashid <[email protected]> (cherry picked from commit 3eee9e0)
5 participants
The commons-crypto library does some questionable error handling internally,
which can lead to JVM crashes if some call into native code fails and cleans
up state it should not.
While the library is not fixed, this change adds some workarounds in Spark code
so that when an error is detected in the commons-crypto side, Spark avoids
calling into the library further.
Tested with existing and added unit tests.