You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by vanzin <gi...@git.apache.org> on 2018/09/26 17:28:34 UTC
[GitHub] spark pull request #22557: [SPARK-25535][core] Work around bad error handlin...
GitHub user vanzin opened a pull request:
https://github.com/apache/spark/pull/22557
[SPARK-25535][core] Work around bad error handling in commons-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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vanzin/spark SPARK-25535
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22557.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #22557
----
commit fe3f06bbc5db00acf893fd6c4f70f93ba1648d11
Author: Marcelo Vanzin <va...@...>
Date: 2018-09-26T00:22:11Z
[SPARK-25535][core] Work around bad error handling in commons-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.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22557
**[Test build #96720 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96720/testReport)** for PR 22557 at commit [`e6e5382`](https://github.com/apache/spark/commit/e6e538282d408b5a87d7fd81e1c2344e41854f5c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96720/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3550/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22557
**[Test build #96720 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96720/testReport)** for PR 22557 at commit [`e6e5382`](https://github.com/apache/spark/commit/e6e538282d408b5a87d7fd81e1c2344e41854f5c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22557: [SPARK-25535][core] Work around bad error handlin...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22557#discussion_r221067971
--- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/AuthEngine.java ---
@@ -241,29 +249,52 @@ private SecretKeySpec generateKey(String kdf, int iterations, byte[] salt, int k
return new SecretKeySpec(key.getEncoded(), conf.keyAlgorithm());
}
- private byte[] doCipherOp(CryptoCipher cipher, byte[] in, boolean isFinal)
+ private byte[] doCipherOp(int mode, byte[] in, boolean isFinal)
throws GeneralSecurityException {
+ CryptoCipher cipher;
+ switch (mode) {
+ case Cipher.ENCRYPT_MODE:
+ cipher = encryptor;
+ break;
+ case Cipher.DECRYPT_MODE:
+ cipher = decryptor;
+ break;
+ default:
+ throw new IllegalArgumentException(String.valueOf(mode));
+ }
+
Preconditions.checkState(cipher != null);
- int scale = 1;
- while (true) {
- int size = in.length * scale;
- byte[] buffer = new byte[size];
- try {
- int outSize = isFinal ? cipher.doFinal(in, 0, in.length, buffer, 0)
- : cipher.update(in, 0, in.length, buffer, 0);
- if (outSize != buffer.length) {
- byte[] output = new byte[outSize];
- System.arraycopy(buffer, 0, output, 0, output.length);
- return output;
- } else {
- return buffer;
+ try {
+ int scale = 1;
+ while (true) {
+ int size = in.length * scale;
+ byte[] buffer = new byte[size];
+ try {
+ int outSize = isFinal ? cipher.doFinal(in, 0, in.length, buffer, 0)
+ : cipher.update(in, 0, in.length, buffer, 0);
+ if (outSize != buffer.length) {
+ byte[] output = new byte[outSize];
+ System.arraycopy(buffer, 0, output, 0, output.length);
+ return output;
+ } else {
+ return buffer;
+ }
+ } catch (ShortBufferException e) {
+ // Try again with a bigger buffer.
+ scale *= 2;
}
- } catch (ShortBufferException e) {
- // Try again with a bigger buffer.
- scale *= 2;
}
+ } catch (InternalError ie) {
+ // SPARK-25535. The commons-cryto library will throw InternalError if something goes wrong,
+ // and leave bad state behind in the Java wrappers, so it's not safe to use them afterwards.
+ if (mode == Cipher.ENCRYPT_MODE) {
+ this.encryptor = null;
+ } else {
+ this.decryptor = null;
--- End diff --
Actually the check is already there I'll just add a message to it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96639/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22557
**[Test build #96637 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96637/testReport)** for PR 22557 at commit [`fe3f06b`](https://github.com/apache/spark/commit/fe3f06bbc5db00acf893fd6c4f70f93ba1648d11).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` trait BaseErrorHandler extends Closeable `
* ` class ErrorHandlingReadableChannel(`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22557: [SPARK-25535][core] Work around bad error handlin...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22557#discussion_r221067787
--- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/AuthEngine.java ---
@@ -241,29 +249,52 @@ private SecretKeySpec generateKey(String kdf, int iterations, byte[] salt, int k
return new SecretKeySpec(key.getEncoded(), conf.keyAlgorithm());
}
- private byte[] doCipherOp(CryptoCipher cipher, byte[] in, boolean isFinal)
+ private byte[] doCipherOp(int mode, byte[] in, boolean isFinal)
throws GeneralSecurityException {
+ CryptoCipher cipher;
+ switch (mode) {
+ case Cipher.ENCRYPT_MODE:
+ cipher = encryptor;
+ break;
+ case Cipher.DECRYPT_MODE:
+ cipher = decryptor;
+ break;
+ default:
+ throw new IllegalArgumentException(String.valueOf(mode));
+ }
+
Preconditions.checkState(cipher != null);
- int scale = 1;
- while (true) {
- int size = in.length * scale;
- byte[] buffer = new byte[size];
- try {
- int outSize = isFinal ? cipher.doFinal(in, 0, in.length, buffer, 0)
- : cipher.update(in, 0, in.length, buffer, 0);
- if (outSize != buffer.length) {
- byte[] output = new byte[outSize];
- System.arraycopy(buffer, 0, output, 0, output.length);
- return output;
- } else {
- return buffer;
+ try {
+ int scale = 1;
+ while (true) {
+ int size = in.length * scale;
+ byte[] buffer = new byte[size];
+ try {
+ int outSize = isFinal ? cipher.doFinal(in, 0, in.length, buffer, 0)
+ : cipher.update(in, 0, in.length, buffer, 0);
+ if (outSize != buffer.length) {
+ byte[] output = new byte[outSize];
+ System.arraycopy(buffer, 0, output, 0, output.length);
+ return output;
+ } else {
+ return buffer;
+ }
+ } catch (ShortBufferException e) {
+ // Try again with a bigger buffer.
+ scale *= 2;
}
- } catch (ShortBufferException e) {
- // Try again with a bigger buffer.
- scale *= 2;
}
+ } catch (InternalError ie) {
+ // SPARK-25535. The commons-cryto library will throw InternalError if something goes wrong,
+ // and leave bad state behind in the Java wrappers, so it's not safe to use them afterwards.
+ if (mode == Cipher.ENCRYPT_MODE) {
+ this.encryptor = null;
+ } else {
+ this.decryptor = null;
--- End diff --
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22557
@squito
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22557
**[Test build #96639 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96639/testReport)** for PR 22557 at commit [`4606ed9`](https://github.com/apache/spark/commit/4606ed938a6ec9925dd6fdabd6c90a7f0d2fec71).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22557
Ping
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3494/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22557
> unrelated, but shouldn't EncryptedMessage.transferTo() not keep looping if target.write() doesn't accept all the data?
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...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22557
**[Test build #96637 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96637/testReport)** for PR 22557 at commit [`fe3f06b`](https://github.com/apache/spark/commit/fe3f06bbc5db00acf893fd6c4f70f93ba1648d11).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22557
**[Test build #96714 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96714/testReport)** for PR 22557 at commit [`e6e5382`](https://github.com/apache/spark/commit/e6e538282d408b5a87d7fd81e1c2344e41854f5c).
* This patch **fails Java style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22557
**[Test build #96714 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96714/testReport)** for PR 22557 at commit [`e6e5382`](https://github.com/apache/spark/commit/e6e538282d408b5a87d7fd81e1c2344e41854f5c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96637/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3549/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22557
**[Test build #96639 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96639/testReport)** for PR 22557 at commit [`4606ed9`](https://github.com/apache/spark/commit/4606ed938a6ec9925dd6fdabd6c90a7f0d2fec71).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22557: [SPARK-25535][core] Work around bad error handlin...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/22557#discussion_r221056816
--- Diff: core/src/main/scala/org/apache/spark/security/CryptoStreamUtils.scala ---
@@ -157,6 +165,111 @@ private[spark] object CryptoStreamUtils extends Logging {
}
+ /**
+ * SPARK-25535. The commons-cryto library will throw InternalError if something goes
+ * wrong, and leave bad state behind in the Java wrappers, so it's not safe to use them
+ * afterwards. This wrapper detects that situation and avoids further calls into the
+ * commons-crypto code, while still allowing the underlying streams to be closed.
+ *
+ * This should be removed once CRYPTO-141 is fixed (and Spark upgrades its commons-crypto
+ * dependency).
+ */
+ trait BaseErrorHandler extends Closeable {
+
+ private var closed = false
+
+ protected def cipherStream: Closeable
+ protected def wrapped: Closeable
--- End diff --
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)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22557: [SPARK-25535][core] Work around bad error handlin...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/22557#discussion_r221056128
--- Diff: core/src/test/scala/org/apache/spark/security/CryptoStreamUtilsSuite.scala ---
@@ -164,6 +167,34 @@ class CryptoStreamUtilsSuite extends SparkFunSuite {
}
}
+ test("error handling wrapper") {
+ val wrapped = mock(classOf[ReadableByteChannel])
+ val decrypted = mock(classOf[ReadableByteChannel])
+ val errorHandler = new CryptoStreamUtils.ErrorHandlingReadableChannel(decrypted, wrapped)
+
+ when(decrypted.read(any(classOf[ByteBuffer])))
+ .thenThrow(new IOException())
+ .thenThrow(new InternalError())
+ .thenReturn(1)
+
+ val out = ByteBuffer.allocate(1)
+ intercept[IOException] {
+ errorHandler.read(out)
+ }
+ intercept[InternalError] {
+ errorHandler.read(out)
+ }
+ intercept[IOException] {
+ errorHandler.read(out)
+ }
--- End diff --
since you're throwing a mock IOException above, this would be a little more clear if here you checked the message too
```scala
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)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3493/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/22557
merged to master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22557: [SPARK-25535][core] Work around bad error handlin...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22557
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/22557
lgtm
will leave for a day before mergning
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22557
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22557: [SPARK-25535][core] Work around bad error handlin...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/22557#discussion_r221058039
--- Diff: common/network-common/src/main/java/org/apache/spark/network/crypto/AuthEngine.java ---
@@ -241,29 +249,52 @@ private SecretKeySpec generateKey(String kdf, int iterations, byte[] salt, int k
return new SecretKeySpec(key.getEncoded(), conf.keyAlgorithm());
}
- private byte[] doCipherOp(CryptoCipher cipher, byte[] in, boolean isFinal)
+ private byte[] doCipherOp(int mode, byte[] in, boolean isFinal)
throws GeneralSecurityException {
+ CryptoCipher cipher;
+ switch (mode) {
+ case Cipher.ENCRYPT_MODE:
+ cipher = encryptor;
+ break;
+ case Cipher.DECRYPT_MODE:
+ cipher = decryptor;
+ break;
+ default:
+ throw new IllegalArgumentException(String.valueOf(mode));
+ }
+
Preconditions.checkState(cipher != null);
- int scale = 1;
- while (true) {
- int size = in.length * scale;
- byte[] buffer = new byte[size];
- try {
- int outSize = isFinal ? cipher.doFinal(in, 0, in.length, buffer, 0)
- : cipher.update(in, 0, in.length, buffer, 0);
- if (outSize != buffer.length) {
- byte[] output = new byte[outSize];
- System.arraycopy(buffer, 0, output, 0, output.length);
- return output;
- } else {
- return buffer;
+ try {
+ int scale = 1;
+ while (true) {
+ int size = in.length * scale;
+ byte[] buffer = new byte[size];
+ try {
+ int outSize = isFinal ? cipher.doFinal(in, 0, in.length, buffer, 0)
+ : cipher.update(in, 0, in.length, buffer, 0);
+ if (outSize != buffer.length) {
+ byte[] output = new byte[outSize];
+ System.arraycopy(buffer, 0, output, 0, output.length);
+ return output;
+ } else {
+ return buffer;
+ }
+ } catch (ShortBufferException e) {
+ // Try again with a bigger buffer.
+ scale *= 2;
}
- } catch (ShortBufferException e) {
- // Try again with a bigger buffer.
- scale *= 2;
}
+ } catch (InternalError ie) {
+ // SPARK-25535. The commons-cryto library will throw InternalError if something goes wrong,
+ // and leave bad state behind in the Java wrappers, so it's not safe to use them afterwards.
+ if (mode == Cipher.ENCRYPT_MODE) {
+ this.encryptor = null;
+ } else {
+ this.decryptor = null;
--- End diff --
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22557: [SPARK-25535][core] Work around bad error handling in co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22557
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96714/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org