Skip to content

Commit

Permalink
Improve painless error wrapping
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rjernst committed Oct 14, 2023
1 parent e7a7a4e commit 61e719b
Show file tree
Hide file tree
Showing 6 changed files with 136 additions and 19 deletions.
Original file line number Diff line number Diff line change
@@ -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.
* <p>
* Only errors which should be sandboxed and not cause the node to crash are wrapped.
*/
class ErrorCauseWrapper extends ElasticsearchException {

private static final List<Class<? extends Error>> 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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ default ScriptException convertToScriptException(Throwable t, Map<String, List<S
scriptStack.add(element.toString());
}
}
ScriptException scriptException = new ScriptException("runtime error", t, scriptStack, getName(), PainlessScriptEngine.NAME, pos);
Throwable cause = ErrorCauseWrapper.maybeWrap(t);
ScriptException scriptException = new ScriptException("runtime error", cause, scriptStack, getName(), PainlessScriptEngine.NAME, pos);
for (Map.Entry<String, List<String>> entry : extraMetadata.entrySet()) {
scriptException.addMetadata(entry.getKey(), entry.getValue());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,8 @@ private static ScriptException convertToScriptException(String scriptSource, Thr
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.
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String, String> content = parser.mapStrings();
assertThat(content, hasEntry("type", "painless_error"));
assertThat(content, hasEntry("reason", "some error"));
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,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 {
Expand Down Expand Up @@ -212,11 +213,13 @@ public void testSecurityException() {

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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public class ScriptException extends ElasticsearchException {
* @throws NullPointerException if any parameters are {@code null} except pos.
*/
public ScriptException(String message, Throwable cause, List<String> scriptStack, String script, String lang, Position pos) {
super(Objects.requireNonNull(message), new CauseWrapper(Objects.requireNonNull(cause)));
super(Objects.requireNonNull(message), Objects.requireNonNull(cause));
this.scriptStack = Collections.unmodifiableList(Objects.requireNonNull(scriptStack));
this.script = Objects.requireNonNull(script);
this.lang = Objects.requireNonNull(lang);
Expand Down Expand Up @@ -206,18 +206,4 @@ public int hashCode() {
return Objects.hash(offset, start, end);
}
}

private static class CauseWrapper extends ElasticsearchException {
final Throwable t;
CauseWrapper(Throwable t) {
super(t.getMessage());
this.t = t;
}

public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.field("type", getExceptionName(t));
builder.field("reason", t.getMessage());
return builder;
}
}
}

0 comments on commit 61e719b

Please sign in to comment.