Skip to content

Commit 0bc95b0

Browse files
committed
Avoid StackOverflowError if write circular reference exception (#54147)
We should never write a circular reference exception as we will fail a node with StackOverflowError. However, we have one in #53589. I tried but failed to find its location. With this commit, we will avoid StackOverflowError in production and detect circular exceptions in tests. Closes #53589
1 parent 62c1e3d commit 0bc95b0

File tree

3 files changed

+46
-5
lines changed

3 files changed

+46
-5
lines changed

server/src/main/java/org/elasticsearch/ElasticsearchException.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ public Throwable getRootCause() {
286286
public void writeTo(StreamOutput out) throws IOException {
287287
out.writeOptionalString(this.getMessage());
288288
out.writeException(this.getCause());
289-
writeStackTraces(this, out);
289+
writeStackTraces(this, out, StreamOutput::writeException);
290290
if (out.getVersion().onOrAfter(Version.V_5_3_0)) {
291291
out.writeMapOfLists(headers, StreamOutput::writeString, StreamOutput::writeString);
292292
out.writeMapOfLists(metadata, StreamOutput::writeString, StreamOutput::writeString);
@@ -728,7 +728,8 @@ public static <T extends Throwable> T readStackTrace(T throwable, StreamInput in
728728
/**
729729
* Serializes the given exceptions stacktrace elements as well as it's suppressed exceptions to the given output stream.
730730
*/
731-
public static <T extends Throwable> T writeStackTraces(T throwable, StreamOutput out) throws IOException {
731+
public static <T extends Throwable> T writeStackTraces(T throwable, StreamOutput out,
732+
Writer<Throwable> exceptionWriter) throws IOException {
732733
StackTraceElement[] stackTrace = throwable.getStackTrace();
733734
out.writeVInt(stackTrace.length);
734735
for (StackTraceElement element : stackTrace) {
@@ -740,7 +741,7 @@ public static <T extends Throwable> T writeStackTraces(T throwable, StreamOutput
740741
Throwable[] suppressed = throwable.getSuppressed();
741742
out.writeVInt(suppressed.length);
742743
for (Throwable t : suppressed) {
743-
out.writeException(t);
744+
exceptionWriter.write(out, t);
744745
}
745746
return throwable;
746747
}

server/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@
8686
public abstract class StreamOutput extends OutputStream {
8787

8888
private static final Map<TimeUnit, Byte> TIME_UNIT_BYTE_MAP;
89+
private static final int MAX_NESTED_EXCEPTION_LEVEL = 100;
8990

9091
static {
9192
final Map<TimeUnit, Byte> timeUnitByteMap = new EnumMap<>(TimeUnit.class);
@@ -858,8 +859,15 @@ public void writeOptionalWriteable(@Nullable Writeable writeable) throws IOExcep
858859
}
859860

860861
public void writeException(Throwable throwable) throws IOException {
862+
writeException(throwable, throwable, 0);
863+
}
864+
865+
private void writeException(Throwable rootException, Throwable throwable, int nestedLevel) throws IOException {
861866
if (throwable == null) {
862867
writeBoolean(false);
868+
} else if (nestedLevel > MAX_NESTED_EXCEPTION_LEVEL) {
869+
assert failOnTooManyNestedExceptions(rootException);
870+
writeException(new IllegalStateException("too many nested exceptions"));
863871
} else {
864872
writeBoolean(true);
865873
boolean writeCause = true;
@@ -982,12 +990,16 @@ public void writeException(Throwable throwable) throws IOException {
982990
writeOptionalString(throwable.getMessage());
983991
}
984992
if (writeCause) {
985-
writeException(throwable.getCause());
993+
writeException(rootException, throwable.getCause(), nestedLevel + 1);
986994
}
987-
ElasticsearchException.writeStackTraces(throwable, this);
995+
ElasticsearchException.writeStackTraces(throwable, this, (o, t) -> o.writeException(rootException, t, nestedLevel + 1));
988996
}
989997
}
990998

999+
boolean failOnTooManyNestedExceptions(Throwable throwable) {
1000+
throw new AssertionError("too many nested exceptions", throwable);
1001+
}
1002+
9911003
/**
9921004
* Writes a {@link NamedWriteable} to the current stream, by first writing its name and then the object itself
9931005
*/

server/src/test/java/org/elasticsearch/common/io/stream/BytesStreamsTests.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
package org.elasticsearch.common.io.stream;
2121

22+
import org.apache.lucene.store.AlreadyClosedException;
2223
import org.apache.lucene.util.BytesRef;
2324
import org.apache.lucene.util.Constants;
2425
import org.elasticsearch.Version;
@@ -48,10 +49,13 @@
4849
import java.util.stream.IntStream;
4950

5051
import static org.hamcrest.Matchers.closeTo;
52+
import static org.hamcrest.Matchers.containsString;
5153
import static org.hamcrest.Matchers.endsWith;
5254
import static org.hamcrest.Matchers.equalTo;
5355
import static org.hamcrest.Matchers.hasSize;
56+
import static org.hamcrest.Matchers.instanceOf;
5457
import static org.hamcrest.Matchers.is;
58+
import static org.hamcrest.Matchers.sameInstance;
5559

5660
/**
5761
* Tests for {@link BytesStreamOutput} paging behaviour.
@@ -848,4 +852,28 @@ public void testTimeValueSerialize() throws Exception {
848852
assertEqualityAfterSerialize(timeValue, 1 + out.bytes().length());
849853
}
850854

855+
public void testWriteCircularReferenceException() throws IOException {
856+
IOException rootEx = new IOException("disk broken");
857+
AlreadyClosedException ace = new AlreadyClosedException("closed", rootEx);
858+
rootEx.addSuppressed(ace); // circular reference
859+
860+
BytesStreamOutput testOut = new BytesStreamOutput();
861+
AssertionError error = expectThrows(AssertionError.class, () -> testOut.writeException(rootEx));
862+
assertThat(error.getMessage(), containsString("too many nested exceptions"));
863+
assertThat(error.getCause(), equalTo(rootEx));
864+
865+
BytesStreamOutput prodOut = new BytesStreamOutput() {
866+
@Override
867+
boolean failOnTooManyNestedExceptions(Throwable throwable) {
868+
assertThat(throwable, sameInstance(rootEx));
869+
return true;
870+
}
871+
};
872+
prodOut.writeException(rootEx);
873+
StreamInput in = prodOut.bytes().streamInput();
874+
Exception newEx = in.readException();
875+
assertThat(newEx, instanceOf(IOException.class));
876+
assertThat(newEx.getMessage(), equalTo("disk broken"));
877+
assertArrayEquals(newEx.getStackTrace(), rootEx.getStackTrace());
878+
}
851879
}

0 commit comments

Comments
 (0)