From f1c062c46fe3caafd20ccc182cfb9d72d099a474 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 17 Oct 2023 07:59:13 -0600 Subject: [PATCH 1/2] Improve painless error wrapping (#100872) Painless sandboxes some errors from Java for which it can recover. These errors are wrapped within a ScriptException. However, retaining the error as a cause can be confusing when walking the error chain. This commit wraps the error so that the real error type does not appear, but maintains the same error message in xcontent serialized form. --- docs/changelog/100872.yaml | 5 ++ .../painless/ErrorCauseWrapper.java | 50 ++++++++++++ .../painless/PainlessScript.java | 10 ++- .../painless/PainlessScriptEngine.java | 3 +- .../painless/BasicStatementTests.java | 4 +- .../painless/ErrorCauseWrapperTests.java | 76 +++++++++++++++++++ .../elasticsearch/painless/FunctionTests.java | 9 +-- .../painless/WhenThingsGoWrongTests.java | 23 +++--- 8 files changed, 161 insertions(+), 19 deletions(-) create mode 100644 docs/changelog/100872.yaml create mode 100644 modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java create mode 100644 modules/lang-painless/src/test/java/org/elasticsearch/painless/ErrorCauseWrapperTests.java diff --git a/docs/changelog/100872.yaml b/docs/changelog/100872.yaml new file mode 100644 index 0000000000000..9877afa28982e --- /dev/null +++ b/docs/changelog/100872.yaml @@ -0,0 +1,5 @@ +pr: 100872 +summary: Improve painless error wrapping +area: Infra/Scripting +type: bug +issues: [] diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java new file mode 100644 index 0000000000000..aeaf44bfd014c --- /dev/null +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java @@ -0,0 +1,50 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.painless; + +import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.xcontent.XContentBuilder; + +import java.io.IOException; +import java.util.List; + +/** + * A wrapper for Error which hides the underlying Error from the exception cause chain. + *

+ * Only errors which should be sandboxed and not cause the node to crash are wrapped. + */ +class ErrorCauseWrapper extends ElasticsearchException { + + private static final List> wrappedErrors = List.of( + PainlessError.class, + OutOfMemoryError.class, + StackOverflowError.class, + LinkageError.class + ); + + final Throwable realCause; + + private ErrorCauseWrapper(Throwable realCause) { + super(realCause.getMessage()); + this.realCause = realCause; + } + + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.field("type", getExceptionName(realCause)); + builder.field("reason", realCause.getMessage()); + return builder; + } + + static Throwable maybeWrap(Throwable t) { + if (wrappedErrors.contains(t.getClass())) { + return new ErrorCauseWrapper(t); + } + return t; + } +} diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScript.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScript.java index 2bd9acf5bae0c..c56cd397ecf07 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScript.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScript.java @@ -82,7 +82,15 @@ default ScriptException convertToScriptException(Throwable t, Map> entry : extraMetadata.entrySet()) { scriptException.addMetadata(entry.getKey(), entry.getValue()); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java index 27e02a4a7c30c..a40b096894db8 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java @@ -492,7 +492,8 @@ private ScriptException convertToScriptException(String scriptSource, Throwable break; } } - throw new ScriptException("compile error", t, scriptStack, scriptSource, PainlessScriptEngine.NAME, pos); + Throwable cause = ErrorCauseWrapper.maybeWrap(t); + throw new ScriptException("compile error", cause, scriptStack, scriptSource, PainlessScriptEngine.NAME, pos); } // very simple heuristic: +/- 25 chars. can be improved later. diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java index 26dcbd6533b47..f0ce9a61d0db7 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java @@ -671,8 +671,8 @@ public void testNoLoopCounterInForEach() { int total = (int) exec("int total = 0; for (int value : params['values']) total += value; return total", params, false); assertEquals(total, 20000000); - PainlessError pe = expectScriptThrows( - PainlessError.class, + var pe = expectScriptThrows( + ErrorCauseWrapper.class, () -> exec( "int total = 0; for (int value = 0; value < params['values'].length; ++value) total += value; return total", params, diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ErrorCauseWrapperTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ErrorCauseWrapperTests.java new file mode 100644 index 0000000000000..d2206b3522fc4 --- /dev/null +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ErrorCauseWrapperTests.java @@ -0,0 +1,76 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.painless; + +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xcontent.ToXContent; +import org.elasticsearch.xcontent.XContentFactory; +import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xcontent.json.JsonXContent; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.Map; + +import static org.hamcrest.Matchers.hasEntry; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; + +public class ErrorCauseWrapperTests extends ESTestCase { + + ErrorCauseWrapper assertWraps(Error realError) { + var e = ErrorCauseWrapper.maybeWrap(realError); + assertThat(e.getCause(), nullValue()); + assertThat(e, instanceOf(ErrorCauseWrapper.class)); + var wrapper = (ErrorCauseWrapper) e; + assertThat(wrapper.realCause, is(realError)); + return wrapper; + } + + public void testOutOfMemoryError() { + assertWraps(new OutOfMemoryError("oom")); + } + + public void testStackOverflowError() { + assertWraps(new StackOverflowError("soe")); + } + + public void testLinkageError() { + assertWraps(new LinkageError("le")); + } + + public void testPainlessError() { + assertWraps(new PainlessError("pe")); + } + + public void testNotWrapped() { + var realError = new AssertionError("not wrapped"); + var e = ErrorCauseWrapper.maybeWrap(realError); + assertThat(e, is(realError)); + } + + public void testXContent() throws IOException { + var e = assertWraps(new PainlessError("some error")); + + var output = new ByteArrayOutputStream(); + var builder = XContentFactory.jsonBuilder(output); + builder.startObject(); + e.toXContent(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + builder.flush(); + + try (var parser = JsonXContent.jsonXContent.createParser(XContentParserConfiguration.EMPTY, output.toByteArray())) { + Map content = parser.mapStrings(); + assertThat(content, hasEntry("type", "painless_error")); + assertThat(content, hasEntry("reason", "some error")); + } + + } +} diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionTests.java index 23af1adb93124..ebd3cfc156361 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.painless; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; public class FunctionTests extends ScriptTestCase { @@ -75,11 +76,9 @@ public void testBadCastFromMethod() { } public void testInfiniteLoop() { - Error expected = expectScriptThrows(PainlessError.class, () -> { exec("void test() {boolean x = true; while (x) {}} test()"); }); - assertThat( - expected.getMessage(), - containsString("The maximum number of statements that can be executed in a loop has been reached.") - ); + var e = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("void test() {boolean x = true; while (x) {}} test()"); }); + assertThat(e.realCause.getClass(), equalTo(PainlessError.class)); + assertThat(e.getMessage(), containsString("The maximum number of statements that can be executed in a loop has been reached.")); } public void testReturnVoid() { diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index e68e2f6ba15cd..0dbd62dd0ccc4 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -18,6 +18,7 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; public class WhenThingsGoWrongTests extends ScriptTestCase { @@ -121,34 +122,34 @@ public void testBogusParameter() { } public void testInfiniteLoops() { - PainlessError expected = expectScriptThrows(PainlessError.class, () -> { exec("boolean x = true; while (x) {}"); }); + var expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("boolean x = true; while (x) {}"); }); assertTrue(expected.getMessage().contains("The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectScriptThrows(PainlessError.class, () -> { exec("while (true) {int y = 5;}"); }); + expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("while (true) {int y = 5;}"); }); assertTrue(expected.getMessage().contains("The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectScriptThrows(PainlessError.class, () -> { exec("while (true) { boolean x = true; while (x) {} }"); }); + expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("while (true) { boolean x = true; while (x) {} }"); }); assertTrue(expected.getMessage().contains("The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectScriptThrows(PainlessError.class, () -> { + expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("while (true) { boolean x = false; while (x) {} }"); fail("should have hit PainlessError"); }); assertTrue(expected.getMessage().contains("The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectScriptThrows(PainlessError.class, () -> { + expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("boolean x = true; for (;x;) {}"); fail("should have hit PainlessError"); }); assertTrue(expected.getMessage().contains("The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectScriptThrows(PainlessError.class, () -> { + expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("for (;;) {int x = 5;}"); fail("should have hit PainlessError"); }); assertTrue(expected.getMessage().contains("The maximum number of statements that can be executed in a loop has been reached.")); - expected = expectScriptThrows(PainlessError.class, () -> { + expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("def x = true; do {int y = 5;} while (x)"); fail("should have hit PainlessError"); }); @@ -165,7 +166,7 @@ public void testLoopLimits() { // right below limit: ok exec("for (int x = 0; x < 999999; ++x) {}"); - PainlessError expected = expectScriptThrows(PainlessError.class, () -> { exec("for (int x = 0; x < 1000000; ++x) {}"); }); + var expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("for (int x = 0; x < 1000000; ++x) {}"); }); assertTrue(expected.getMessage().contains("The maximum number of statements that can be executed in a loop has been reached.")); } @@ -211,11 +212,13 @@ public void testBadBoxingCast() { public void testOutOfMemoryError() { assumeTrue("test only happens to work for sure on oracle jre", Constants.JAVA_VENDOR.startsWith("Oracle")); - expectScriptThrows(OutOfMemoryError.class, () -> { exec("int[] x = new int[Integer.MAX_VALUE - 1];"); }); + var e = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("int[] x = new int[Integer.MAX_VALUE - 1];"); }); + assertThat(e.realCause.getClass(), equalTo(OutOfMemoryError.class)); } public void testStackOverflowError() { - expectScriptThrows(StackOverflowError.class, () -> { exec("void recurse(int x, int y) {recurse(x, y)} recurse(1, 2);"); }); + var e = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("void recurse(int x, int y) {recurse(x, y)} recurse(1, 2);"); }); + assertThat(e.realCause.getClass(), equalTo(StackOverflowError.class)); } public void testCanNotOverrideRegexEnabled() { From 2cb212d3c51103523d9ffb60072c4607d1ed4c1e Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 20 Nov 2023 14:53:39 -0800 Subject: [PATCH 2/2] fix compile --- .../painless/ErrorCauseWrapper.java | 2 +- .../painless/BasicStatementTests.java | 2 +- .../painless/ErrorCauseWrapperTests.java | 19 ++++++++++--------- .../elasticsearch/painless/FunctionTests.java | 5 ++++- .../painless/WhenThingsGoWrongTests.java | 11 +++++++---- 5 files changed, 23 insertions(+), 16 deletions(-) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java index aeaf44bfd014c..29cc98d1405e4 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ErrorCauseWrapper.java @@ -21,7 +21,7 @@ */ class ErrorCauseWrapper extends ElasticsearchException { - private static final List> wrappedErrors = List.of( + private static final List> wrappedErrors = org.elasticsearch.core.List.of( PainlessError.class, OutOfMemoryError.class, StackOverflowError.class, diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java index f0ce9a61d0db7..7f2dd914843f0 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java @@ -671,7 +671,7 @@ public void testNoLoopCounterInForEach() { int total = (int) exec("int total = 0; for (int value : params['values']) total += value; return total", params, false); assertEquals(total, 20000000); - var pe = expectScriptThrows( + ErrorCauseWrapper pe = expectScriptThrows( ErrorCauseWrapper.class, () -> exec( "int total = 0; for (int value = 0; value < params['values'].length; ++value) total += value; return total", diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ErrorCauseWrapperTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ErrorCauseWrapperTests.java index d2206b3522fc4..1c3a49491954d 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ErrorCauseWrapperTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ErrorCauseWrapperTests.java @@ -10,8 +10,9 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.ToXContent; +import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; -import org.elasticsearch.xcontent.XContentParserConfiguration; +import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.xcontent.json.JsonXContent; import java.io.ByteArrayOutputStream; @@ -26,10 +27,10 @@ public class ErrorCauseWrapperTests extends ESTestCase { ErrorCauseWrapper assertWraps(Error realError) { - var e = ErrorCauseWrapper.maybeWrap(realError); + Throwable e = ErrorCauseWrapper.maybeWrap(realError); assertThat(e.getCause(), nullValue()); assertThat(e, instanceOf(ErrorCauseWrapper.class)); - var wrapper = (ErrorCauseWrapper) e; + ErrorCauseWrapper wrapper = (ErrorCauseWrapper) e; assertThat(wrapper.realCause, is(realError)); return wrapper; } @@ -51,22 +52,22 @@ public void testPainlessError() { } public void testNotWrapped() { - var realError = new AssertionError("not wrapped"); - var e = ErrorCauseWrapper.maybeWrap(realError); + AssertionError realError = new AssertionError("not wrapped"); + Throwable e = ErrorCauseWrapper.maybeWrap(realError); assertThat(e, is(realError)); } public void testXContent() throws IOException { - var e = assertWraps(new PainlessError("some error")); + ErrorCauseWrapper e = assertWraps(new PainlessError("some error")); - var output = new ByteArrayOutputStream(); - var builder = XContentFactory.jsonBuilder(output); + ByteArrayOutputStream output = new ByteArrayOutputStream(); + XContentBuilder builder = XContentFactory.jsonBuilder(output); builder.startObject(); e.toXContent(builder, ToXContent.EMPTY_PARAMS); builder.endObject(); builder.flush(); - try (var parser = JsonXContent.jsonXContent.createParser(XContentParserConfiguration.EMPTY, output.toByteArray())) { + try (XContentParser parser = createParser(JsonXContent.jsonXContent, output.toByteArray())) { Map content = parser.mapStrings(); assertThat(content, hasEntry("type", "painless_error")); assertThat(content, hasEntry("reason", "some error")); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionTests.java index ebd3cfc156361..6a4ec00d340a5 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/FunctionTests.java @@ -76,7 +76,10 @@ public void testBadCastFromMethod() { } public void testInfiniteLoop() { - var e = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("void test() {boolean x = true; while (x) {}} test()"); }); + ErrorCauseWrapper e = expectScriptThrows( + ErrorCauseWrapper.class, + () -> { exec("void test() {boolean x = true; while (x) {}} test()"); } + ); assertThat(e.realCause.getClass(), equalTo(PainlessError.class)); assertThat(e.getMessage(), containsString("The maximum number of statements that can be executed in a loop has been reached.")); } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index 0dbd62dd0ccc4..f709649d0e338 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -122,7 +122,7 @@ public void testBogusParameter() { } public void testInfiniteLoops() { - var expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("boolean x = true; while (x) {}"); }); + ErrorCauseWrapper expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("boolean x = true; while (x) {}"); }); assertTrue(expected.getMessage().contains("The maximum number of statements that can be executed in a loop has been reached.")); expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("while (true) {int y = 5;}"); }); @@ -166,7 +166,7 @@ public void testLoopLimits() { // right below limit: ok exec("for (int x = 0; x < 999999; ++x) {}"); - var expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("for (int x = 0; x < 1000000; ++x) {}"); }); + ErrorCauseWrapper expected = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("for (int x = 0; x < 1000000; ++x) {}"); }); assertTrue(expected.getMessage().contains("The maximum number of statements that can be executed in a loop has been reached.")); } @@ -212,12 +212,15 @@ public void testBadBoxingCast() { public void testOutOfMemoryError() { assumeTrue("test only happens to work for sure on oracle jre", Constants.JAVA_VENDOR.startsWith("Oracle")); - var e = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("int[] x = new int[Integer.MAX_VALUE - 1];"); }); + ErrorCauseWrapper e = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("int[] x = new int[Integer.MAX_VALUE - 1];"); }); assertThat(e.realCause.getClass(), equalTo(OutOfMemoryError.class)); } public void testStackOverflowError() { - var e = expectScriptThrows(ErrorCauseWrapper.class, () -> { exec("void recurse(int x, int y) {recurse(x, y)} recurse(1, 2);"); }); + ErrorCauseWrapper e = expectScriptThrows( + ErrorCauseWrapper.class, + () -> { exec("void recurse(int x, int y) {recurse(x, y)} recurse(1, 2);"); } + ); assertThat(e.realCause.getClass(), equalTo(StackOverflowError.class)); }