Skip to content

Commit 4ecc7dc

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 05c5529 commit 4ecc7dc

File tree

3 files changed

+45
-5
lines changed

3 files changed

+45
-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
@@ -276,7 +276,7 @@ public Throwable getRootCause() {
276276
public void writeTo(StreamOutput out) throws IOException {
277277
out.writeOptionalString(this.getMessage());
278278
out.writeException(this.getCause());
279-
writeStackTraces(this, out);
279+
writeStackTraces(this, out, StreamOutput::writeException);
280280
out.writeMapOfLists(headers, StreamOutput::writeString, StreamOutput::writeString);
281281
out.writeMapOfLists(metadata, StreamOutput::writeString, StreamOutput::writeString);
282282
}
@@ -715,7 +715,8 @@ public static <T extends Throwable> T readStackTrace(T throwable, StreamInput in
715715
/**
716716
* Serializes the given exceptions stacktrace elements as well as it's suppressed exceptions to the given output stream.
717717
*/
718-
public static <T extends Throwable> T writeStackTraces(T throwable, StreamOutput out) throws IOException {
718+
public static <T extends Throwable> T writeStackTraces(T throwable, StreamOutput out,
719+
Writer<Throwable> exceptionWriter) throws IOException {
719720
StackTraceElement[] stackTrace = throwable.getStackTrace();
720721
out.writeVInt(stackTrace.length);
721722
for (StackTraceElement element : stackTrace) {
@@ -727,7 +728,7 @@ public static <T extends Throwable> T writeStackTraces(T throwable, StreamOutput
727728
Throwable[] suppressed = throwable.getSuppressed();
728729
out.writeVInt(suppressed.length);
729730
for (Throwable t : suppressed) {
730-
out.writeException(t);
731+
exceptionWriter.write(out, t);
731732
}
732733
return throwable;
733734
}

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
@@ -90,6 +90,7 @@
9090
public abstract class StreamOutput extends OutputStream {
9191

9292
private static final Map<TimeUnit, Byte> TIME_UNIT_BYTE_MAP;
93+
private static final int MAX_NESTED_EXCEPTION_LEVEL = 100;
9394

9495
static {
9596
final Map<TimeUnit, Byte> timeUnitByteMap = new EnumMap<>(TimeUnit.class);
@@ -910,8 +911,15 @@ public void writeOptionalWriteable(@Nullable Writeable writeable) throws IOExcep
910911
}
911912

912913
public void writeException(Throwable throwable) throws IOException {
914+
writeException(throwable, throwable, 0);
915+
}
916+
917+
private void writeException(Throwable rootException, Throwable throwable, int nestedLevel) throws IOException {
913918
if (throwable == null) {
914919
writeBoolean(false);
920+
} else if (nestedLevel > MAX_NESTED_EXCEPTION_LEVEL) {
921+
assert failOnTooManyNestedExceptions(rootException);
922+
writeException(new IllegalStateException("too many nested exceptions"));
915923
} else {
916924
writeBoolean(true);
917925
boolean writeCause = true;
@@ -1020,12 +1028,16 @@ public void writeException(Throwable throwable) throws IOException {
10201028
writeOptionalString(throwable.getMessage());
10211029
}
10221030
if (writeCause) {
1023-
writeException(throwable.getCause());
1031+
writeException(rootException, throwable.getCause(), nestedLevel + 1);
10241032
}
1025-
ElasticsearchException.writeStackTraces(throwable, this);
1033+
ElasticsearchException.writeStackTraces(throwable, this, (o, t) -> o.writeException(rootException, t, nestedLevel + 1));
10261034
}
10271035
}
10281036

1037+
boolean failOnTooManyNestedExceptions(Throwable throwable) {
1038+
throw new AssertionError("too many nested exceptions", throwable);
1039+
}
1040+
10291041
/**
10301042
* Writes a {@link NamedWriteable} to the current stream, by first writing its name and then the object itself
10311043
*/

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

Lines changed: 27 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.common.bytes.BytesArray;
@@ -49,12 +50,14 @@
4950
import java.util.stream.IntStream;
5051

5152
import static org.hamcrest.Matchers.closeTo;
53+
import static org.hamcrest.Matchers.containsString;
5254
import static org.hamcrest.Matchers.endsWith;
5355
import static org.hamcrest.Matchers.equalTo;
5456
import static org.hamcrest.Matchers.hasSize;
5557
import static org.hamcrest.Matchers.instanceOf;
5658
import static org.hamcrest.Matchers.is;
5759
import static org.hamcrest.Matchers.nullValue;
60+
import static org.hamcrest.Matchers.sameInstance;
5861

5962
/**
6063
* Tests for {@link BytesStreamOutput} paging behaviour.
@@ -850,4 +853,28 @@ public void testTimeValueSerialize() throws Exception {
850853
assertEqualityAfterSerialize(timeValue, 1 + out.bytes().length());
851854
}
852855

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

0 commit comments

Comments
 (0)