From 15f5f85f3520de85b7793b404d39fb6840b8dfec Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 22 Jan 2024 10:38:33 +1100 Subject: [PATCH] Limit nesting depth in Exception XContent (#103741) Poor constructed exceptions may contain cycles between causes and/or supressed exceptions. This commit changes the toXContent rendering of exceptions to limit the nesting depth in order to avoid generating infinite nested JSON (which actually leads to a `StackOverflowError`) --- docs/changelog/103741.yaml | 5 ++ .../painless/ErrorCauseWrapper.java | 2 +- .../elasticsearch/ElasticsearchException.java | 44 ++++++++++++++---- .../search/SearchPhaseExecutionException.java | 16 +++++-- .../ElasticsearchExceptionTests.java | 46 +++++++++++++++++++ 5 files changed, 100 insertions(+), 13 deletions(-) create mode 100644 docs/changelog/103741.yaml diff --git a/docs/changelog/103741.yaml b/docs/changelog/103741.yaml new file mode 100644 index 0000000000000..6771ddd329f46 --- /dev/null +++ b/docs/changelog/103741.yaml @@ -0,0 +1,5 @@ +pr: 103741 +summary: Limit nesting depth in Exception XContent +area: Infra/Resiliency +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 index 308d6223c666e..c1e1012eb3381 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 @@ -36,7 +36,7 @@ private ErrorCauseWrapper(Throwable realCause) { this.realCause = realCause; } - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + protected XContentBuilder toXContent(XContentBuilder builder, Params params, int nestedLevel) throws IOException { builder.field("type", getExceptionName(realCause)); builder.field("reason", realCause.getMessage()); return builder; diff --git a/server/src/main/java/org/elasticsearch/ElasticsearchException.java b/server/src/main/java/org/elasticsearch/ElasticsearchException.java index 50a5f7420847b..0674bbc67505a 100644 --- a/server/src/main/java/org/elasticsearch/ElasticsearchException.java +++ b/server/src/main/java/org/elasticsearch/ElasticsearchException.java @@ -342,12 +342,20 @@ public static int getId(Class exception) { } @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return toXContent(builder, params, 0); + } + + /** + * Equivalent to {@link org.elasticsearch.xcontent.ToXContent#toXContent(XContentBuilder, Params)} except that it limits nesting depth + * so that it can avoid stackoverflow errors. + */ + protected XContentBuilder toXContent(XContentBuilder builder, Params params, int nestedLevel) throws IOException { Throwable ex = ExceptionsHelper.unwrapCause(this); if (ex != this) { - generateThrowableXContent(builder, params, this); + generateThrowableXContent(builder, params, this, nestedLevel); } else { - innerToXContent(builder, params, this, getExceptionName(), getMessage(), headers, metadata, getCause()); + innerToXContent(builder, params, this, getExceptionName(), getMessage(), headers, metadata, getCause(), nestedLevel); } return builder; } @@ -360,8 +368,17 @@ protected static void innerToXContent( String message, Map> headers, Map> metadata, - Throwable cause + Throwable cause, + int nestedLevel ) throws IOException { + + if (nestedLevel > MAX_NESTED_EXCEPTION_LEVEL) { + var terminalException = new IllegalStateException("too many nested exceptions"); + builder.field(TYPE, getExceptionName(terminalException)); + builder.field(REASON, terminalException.getMessage()); + return; + } + builder.field(TYPE, type); builder.field(REASON, message); @@ -377,7 +394,7 @@ protected static void innerToXContent( if (cause != null) { builder.field(CAUSED_BY); builder.startObject(); - generateThrowableXContent(builder, params, cause); + generateThrowableXContent(builder, params, cause, nestedLevel + 1); builder.endObject(); } } @@ -399,7 +416,7 @@ protected static void innerToXContent( builder.startArray(SUPPRESSED.getPreferredName()); for (Throwable suppressed : allSuppressed) { builder.startObject(); - generateThrowableXContent(builder, params, suppressed); + generateThrowableXContent(builder, params, suppressed, nestedLevel + 1); builder.endObject(); } builder.endArray(); @@ -552,18 +569,27 @@ public static ElasticsearchException innerFromXContent(XContentParser parser, bo /** * Static toXContent helper method that renders {@link org.elasticsearch.ElasticsearchException} or {@link Throwable} instances * as XContent, delegating the rendering to {@link #toXContent(XContentBuilder, Params)} - * or {@link #innerToXContent(XContentBuilder, Params, Throwable, String, String, Map, Map, Throwable)}. + * or {@link #innerToXContent(XContentBuilder, Params, Throwable, String, String, Map, Map, Throwable, int)}. * * This method is usually used when the {@link Throwable} is rendered as a part of another XContent object, and its result can * be parsed back using the {@link #fromXContent(XContentParser)} method. */ public static void generateThrowableXContent(XContentBuilder builder, Params params, Throwable t) throws IOException { + generateThrowableXContent(builder, params, t, 0); + } + + /** + * Equivalent to {@link #generateThrowableXContent(XContentBuilder, Params, Throwable)} but limits nesting depth + * so that it can avoid stackoverflow errors. + */ + protected static void generateThrowableXContent(XContentBuilder builder, Params params, Throwable t, int nestedLevel) + throws IOException { t = ExceptionsHelper.unwrapCause(t); if (t instanceof ElasticsearchException) { - ((ElasticsearchException) t).toXContent(builder, params); + ((ElasticsearchException) t).toXContent(builder, params, nestedLevel); } else { - innerToXContent(builder, params, t, getExceptionName(t), t.getMessage(), emptyMap(), emptyMap(), t.getCause()); + innerToXContent(builder, params, t, getExceptionName(t), t.getMessage(), emptyMap(), emptyMap(), t.getCause(), nestedLevel); } } diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java b/server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java index 8347dc8e3dc80..022c3304bc865 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchPhaseExecutionException.java @@ -129,15 +129,25 @@ protected void metadataToXContent(XContentBuilder builder, Params params) throws } @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + protected XContentBuilder toXContent(XContentBuilder builder, Params params, int nestedLevel) throws IOException { Throwable ex = ExceptionsHelper.unwrapCause(this); if (ex != this) { - generateThrowableXContent(builder, params, this); + generateThrowableXContent(builder, params, this, nestedLevel); } else { // We don't have a cause when all shards failed, but we do have shards failures so we can "guess" a cause // (see {@link #getCause()}). Here, we use super.getCause() because we don't want the guessed exception to // be rendered twice (one in the "cause" field, one in "failed_shards") - innerToXContent(builder, params, this, getExceptionName(), getMessage(), getHeaders(), getMetadata(), super.getCause()); + innerToXContent( + builder, + params, + this, + getExceptionName(), + getMessage(), + getHeaders(), + getMetadata(), + super.getCause(), + nestedLevel + ); } return builder; } diff --git a/server/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java b/server/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java index 6400eef6f9e34..2268c95312716 100644 --- a/server/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java +++ b/server/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java @@ -43,6 +43,7 @@ import org.elasticsearch.search.internal.ShardSearchContextId; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.transport.RemoteTransportException; +import org.elasticsearch.xcontent.ObjectPath; import org.elasticsearch.xcontent.ToXContent; import org.elasticsearch.xcontent.XContent; import org.elasticsearch.xcontent.XContentBuilder; @@ -67,8 +68,11 @@ import static org.hamcrest.CoreMatchers.hasItem; import static org.hamcrest.CoreMatchers.hasItems; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.startsWith; public class ElasticsearchExceptionTests extends ESTestCase { @@ -724,6 +728,48 @@ public void testToXContentWithHeadersAndMetadata() throws IOException { ); } + @SuppressWarnings("unchecked") + public void testToXContentWithObjectCycles() throws Exception { + ElasticsearchException root = new ElasticsearchException("root exception"); + + ElasticsearchException suppressed1 = new ElasticsearchException("suppressed#1", root); + + ElasticsearchException suppressed2 = new ElasticsearchException("suppressed#2"); + ElasticsearchException suppressed3 = new ElasticsearchException("suppressed#3"); + suppressed3.addSuppressed(suppressed2); + suppressed2.addSuppressed(suppressed3); + + root.addSuppressed(suppressed1); + root.addSuppressed(suppressed2); + root.addSuppressed(suppressed3); + + // Because we support up to 100 nested exceptions, this JSON ends up very long. + // Rather than assert the full content, we check that + // (a) it generated successfully (no StackOverflowErrors) + BytesReference xContent = XContentHelper.toXContent(root, XContentType.JSON, randomBoolean()); + // (b) it's valid JSON + final Map map = XContentHelper.convertToMap(xContent, false, XContentType.JSON).v2(); + // (c) it contains the right content + assertThat(ObjectPath.eval("type", map), equalTo("exception")); + assertThat(ObjectPath.eval("reason", map), equalTo("root exception")); + assertThat(ObjectPath.eval("suppressed.0.reason", map), equalTo("suppressed#1")); + assertThat(ObjectPath.eval("suppressed.0.caused_by.reason", map), equalTo("root exception")); + assertThat(ObjectPath.eval("suppressed.0.caused_by.suppressed.0.reason", map), equalTo("suppressed#1")); + assertThat(ObjectPath.eval("suppressed.1.reason", map), equalTo("suppressed#2")); + assertThat(ObjectPath.eval("suppressed.1.suppressed.0.reason", map), equalTo("suppressed#3")); + assertThat(ObjectPath.eval("suppressed.1.suppressed.0.suppressed.0.reason", map), equalTo("suppressed#2")); + assertThat(ObjectPath.eval("suppressed.2.reason", map), equalTo("suppressed#3")); + assertThat(ObjectPath.eval("suppressed.2.suppressed.0.reason", map), equalTo("suppressed#2")); + assertThat(ObjectPath.eval("suppressed.2.suppressed.0.suppressed.0.reason", map), equalTo("suppressed#3")); + + String tailExceptionPath = ".suppressed.0.caused_by".repeat(50).substring(1) + ".suppressed.0"; + final Object tailException = ObjectPath.eval(tailExceptionPath, map); + assertThat(tailException, not(nullValue())); + assertThat(tailException, instanceOf(Map.class)); + assertThat((Map) tailException, hasEntry("reason", "too many nested exceptions")); + assertThat((Map) tailException, hasEntry("type", "illegal_state_exception")); + } + public void testFromXContent() throws IOException { final XContent xContent = randomFrom(XContentType.values()).xContent(); XContentBuilder builder = XContentBuilder.builder(xContent)