From 6b613682ba450c5f7c6d528881ba028d03bf3448 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 15 Oct 2020 20:41:29 +0200 Subject: [PATCH] Ensure we don't leak the ClassLoader in the backtrace (#10691) Motivation: We have a few classes in which we store and reuse static instances of various exceptions. When doing so it is important to also override fillInStacktrace() and so prevent the leak of the ClassLoader in the internal backtrace field. Modifications: - Add overrides of fillInStracktrace when needed - Move ThrowableUtil usage in the static methods Result: Fixes https://github.com/netty/netty/pull/10686 --- .../codec/spdy/SpdyProtocolException.java | 29 ++++++++-- .../codec/spdy/SpdySessionHandler.java | 9 ++-- .../handler/codec/http2/HpackDecoder.java | 33 ++++++------ .../codec/http2/HpackHuffmanDecoder.java | 5 +- .../handler/codec/http2/Http2Exception.java | 31 +++++++++-- .../netty/util/concurrent/DefaultPromise.java | 22 +++++++- .../netty/resolver/dns/DnsResolveContext.java | 53 +++++++++++-------- .../io/netty/channel/ChannelException.java | 30 +++++++++-- .../ThreadPerChannelEventLoopGroup.java | 5 +- 9 files changed, 155 insertions(+), 62 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyProtocolException.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyProtocolException.java index 477599af13..92b4613aee 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyProtocolException.java +++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyProtocolException.java @@ -17,6 +17,7 @@ package io.netty.handler.codec.spdy; import io.netty.util.internal.PlatformDependent; import io.netty.util.internal.SuppressJava6Requirement; +import io.netty.util.internal.ThrowableUtil; public class SpdyProtocolException extends Exception { @@ -48,11 +49,14 @@ public class SpdyProtocolException extends Exception { super(cause); } - static SpdyProtocolException newStatic(String message) { + static SpdyProtocolException newStatic(String message, Class clazz, String method) { + final SpdyProtocolException exception; if (PlatformDependent.javaVersion() >= 7) { - return new SpdyProtocolException(message, true); + exception = new StacklessSpdyProtocolException(message, true); + } else { + exception = new StacklessSpdyProtocolException(message); } - return new SpdyProtocolException(message); + return ThrowableUtil.unknownStackTrace(exception, clazz, method); } @SuppressJava6Requirement(reason = "uses Java 7+ Exception.(String, Throwable, boolean, boolean)" + @@ -61,4 +65,23 @@ public class SpdyProtocolException extends Exception { super(message, null, false, true); assert shared; } + + private static final class StacklessSpdyProtocolException extends SpdyProtocolException { + private static final long serialVersionUID = -6302754207557485099L; + + StacklessSpdyProtocolException(String message) { + super(message); + } + + StacklessSpdyProtocolException(String message, boolean shared) { + super(message, shared); + } + + // Override fillInStackTrace() so we not populate the backtrace via a native call and so leak the + // Classloader. + @Override + public Throwable fillInStackTrace() { + return this; + } + } } diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java index e3e00cf5ab..08a0b5c5c1 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java +++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java @@ -21,7 +21,6 @@ import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelPromise; import io.netty.util.internal.ObjectUtil; -import io.netty.util.internal.ThrowableUtil; import java.util.concurrent.atomic.AtomicInteger; @@ -34,10 +33,10 @@ import static io.netty.util.internal.ObjectUtil.checkPositiveOrZero; */ public class SpdySessionHandler extends ChannelDuplexHandler { - private static final SpdyProtocolException PROTOCOL_EXCEPTION = ThrowableUtil.unknownStackTrace( - SpdyProtocolException.newStatic(null), SpdySessionHandler.class, "handleOutboundMessage(...)"); - private static final SpdyProtocolException STREAM_CLOSED = ThrowableUtil.unknownStackTrace( - SpdyProtocolException.newStatic("Stream closed"), SpdySessionHandler.class, "removeStream(...)"); + private static final SpdyProtocolException PROTOCOL_EXCEPTION = + SpdyProtocolException.newStatic(null, SpdySessionHandler.class, "handleOutboundMessage(...)"); + private static final SpdyProtocolException STREAM_CLOSED = + SpdyProtocolException.newStatic("Stream closed", SpdySessionHandler.class, "removeStream(...)"); private static final int DEFAULT_WINDOW_SIZE = 64 * 1024; // 64 KB default initial window size private int initialSendWindowSize = DEFAULT_WINDOW_SIZE; diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java index 6070bc5f98..f7210d6c59 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java @@ -49,35 +49,34 @@ import static io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName.getPseu import static io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName.hasPseudoHeaderFormat; import static io.netty.util.AsciiString.EMPTY_STRING; import static io.netty.util.internal.ObjectUtil.checkPositive; -import static io.netty.util.internal.ThrowableUtil.unknownStackTrace; final class HpackDecoder { - private static final Http2Exception DECODE_ULE_128_DECOMPRESSION_EXCEPTION = unknownStackTrace( + private static final Http2Exception DECODE_ULE_128_DECOMPRESSION_EXCEPTION = Http2Exception.newStatic(COMPRESSION_ERROR, "HPACK - decompression failure", - Http2Exception.ShutdownHint.HARD_SHUTDOWN), HpackDecoder.class, + Http2Exception.ShutdownHint.HARD_SHUTDOWN, HpackDecoder.class, "decodeULE128(..)"); - private static final Http2Exception DECODE_ULE_128_TO_LONG_DECOMPRESSION_EXCEPTION = unknownStackTrace( + private static final Http2Exception DECODE_ULE_128_TO_LONG_DECOMPRESSION_EXCEPTION = Http2Exception.newStatic(COMPRESSION_ERROR, "HPACK - long overflow", - Http2Exception.ShutdownHint.HARD_SHUTDOWN), HpackDecoder.class, "decodeULE128(..)"); - private static final Http2Exception DECODE_ULE_128_TO_INT_DECOMPRESSION_EXCEPTION = unknownStackTrace( + Http2Exception.ShutdownHint.HARD_SHUTDOWN, HpackDecoder.class, "decodeULE128(..)"); + private static final Http2Exception DECODE_ULE_128_TO_INT_DECOMPRESSION_EXCEPTION = Http2Exception.newStatic(COMPRESSION_ERROR, "HPACK - int overflow", - Http2Exception.ShutdownHint.HARD_SHUTDOWN), HpackDecoder.class, "decodeULE128ToInt(..)"); - private static final Http2Exception DECODE_ILLEGAL_INDEX_VALUE = unknownStackTrace( + Http2Exception.ShutdownHint.HARD_SHUTDOWN, HpackDecoder.class, "decodeULE128ToInt(..)"); + private static final Http2Exception DECODE_ILLEGAL_INDEX_VALUE = Http2Exception.newStatic(COMPRESSION_ERROR, "HPACK - illegal index value", - Http2Exception.ShutdownHint.HARD_SHUTDOWN), HpackDecoder.class, "decode(..)"); - private static final Http2Exception INDEX_HEADER_ILLEGAL_INDEX_VALUE = unknownStackTrace( + Http2Exception.ShutdownHint.HARD_SHUTDOWN, HpackDecoder.class, "decode(..)"); + private static final Http2Exception INDEX_HEADER_ILLEGAL_INDEX_VALUE = Http2Exception.newStatic(COMPRESSION_ERROR, "HPACK - illegal index value", - Http2Exception.ShutdownHint.HARD_SHUTDOWN), HpackDecoder.class, "indexHeader(..)"); - private static final Http2Exception READ_NAME_ILLEGAL_INDEX_VALUE = unknownStackTrace( + Http2Exception.ShutdownHint.HARD_SHUTDOWN, HpackDecoder.class, "indexHeader(..)"); + private static final Http2Exception READ_NAME_ILLEGAL_INDEX_VALUE = Http2Exception.newStatic(COMPRESSION_ERROR, "HPACK - illegal index value", - Http2Exception.ShutdownHint.HARD_SHUTDOWN), HpackDecoder.class, "readName(..)"); - private static final Http2Exception INVALID_MAX_DYNAMIC_TABLE_SIZE = unknownStackTrace( + Http2Exception.ShutdownHint.HARD_SHUTDOWN, HpackDecoder.class, "readName(..)"); + private static final Http2Exception INVALID_MAX_DYNAMIC_TABLE_SIZE = Http2Exception.newStatic(COMPRESSION_ERROR, "HPACK - invalid max dynamic table size", - Http2Exception.ShutdownHint.HARD_SHUTDOWN), HpackDecoder.class, + Http2Exception.ShutdownHint.HARD_SHUTDOWN, HpackDecoder.class, "setDynamicTableSize(..)"); - private static final Http2Exception MAX_DYNAMIC_TABLE_SIZE_CHANGE_REQUIRED = unknownStackTrace( + private static final Http2Exception MAX_DYNAMIC_TABLE_SIZE_CHANGE_REQUIRED = Http2Exception.newStatic(COMPRESSION_ERROR, "HPACK - max dynamic table size change required", - Http2Exception.ShutdownHint.HARD_SHUTDOWN), HpackDecoder.class, "decode(..)"); + Http2Exception.ShutdownHint.HARD_SHUTDOWN, HpackDecoder.class, "decode(..)"); private static final byte READ_HEADER_REPRESENTATION = 0; private static final byte READ_MAX_DYNAMIC_TABLE_SIZE = 1; private static final byte READ_INDEXED_HEADER = 2; diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanDecoder.java index cc6b351f12..c3270f0948 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanDecoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanDecoder.java @@ -34,7 +34,6 @@ package io.netty.handler.codec.http2; import io.netty.buffer.ByteBuf; import io.netty.util.AsciiString; import io.netty.util.ByteProcessor; -import io.netty.util.internal.ThrowableUtil; import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR; @@ -4665,9 +4664,9 @@ final class HpackHuffmanDecoder implements ByteProcessor { HUFFMAN_FAIL << 8, }; - private static final Http2Exception BAD_ENCODING = ThrowableUtil.unknownStackTrace( + private static final Http2Exception BAD_ENCODING = Http2Exception.newStatic(COMPRESSION_ERROR, "HPACK - Bad Encoding", - Http2Exception.ShutdownHint.HARD_SHUTDOWN), HpackHuffmanDecoder.class, "decode(..)"); + Http2Exception.ShutdownHint.HARD_SHUTDOWN, HpackHuffmanDecoder.class, "decode(..)"); private byte[] dest; private int k; diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Exception.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Exception.java index cedfe0ba95..1a40c3085d 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Exception.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Exception.java @@ -17,6 +17,7 @@ package io.netty.handler.codec.http2; import io.netty.util.internal.PlatformDependent; import io.netty.util.internal.SuppressJava6Requirement; +import io.netty.util.internal.ThrowableUtil; import io.netty.util.internal.UnstableApi; import java.util.ArrayList; @@ -64,11 +65,15 @@ public class Http2Exception extends Exception { this.shutdownHint = checkNotNull(shutdownHint, "shutdownHint"); } - static Http2Exception newStatic(Http2Error error, String message, ShutdownHint shutdownHint) { + static Http2Exception newStatic(Http2Error error, String message, ShutdownHint shutdownHint, + Class clazz, String method) { + final Http2Exception exception; if (PlatformDependent.javaVersion() >= 7) { - return new Http2Exception(error, message, shutdownHint, true); + exception = new StacklessHttp2Exception(error, message, shutdownHint, true); + } else { + exception = new StacklessHttp2Exception(error, message, shutdownHint); } - return new Http2Exception(error, message, shutdownHint); + return ThrowableUtil.unknownStackTrace(exception, clazz, method); } @SuppressJava6Requirement(reason = "uses Java 7+ Exception.(String, Throwable, boolean, boolean)" + @@ -305,4 +310,24 @@ public class Http2Exception extends Exception { return exceptions.iterator(); } } + + private static final class StacklessHttp2Exception extends Http2Exception { + + private static final long serialVersionUID = 1077888485687219443L; + + StacklessHttp2Exception(Http2Error error, String message, ShutdownHint shutdownHint) { + super(error, message, shutdownHint); + } + + StacklessHttp2Exception(Http2Error error, String message, ShutdownHint shutdownHint, boolean shared) { + super(error, message, shutdownHint, shared); + } + + // Override fillInStackTrace() so we not populate the backtrace via a native call and so leak the + // Classloader. + @Override + public Throwable fillInStackTrace() { + return this; + } + } } diff --git a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java index bc7a09b54d..1e6f1a2dcb 100644 --- a/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java +++ b/common/src/main/java/io/netty/util/concurrent/DefaultPromise.java @@ -43,8 +43,8 @@ public class DefaultPromise extends AbstractFuture implements Promise { AtomicReferenceFieldUpdater.newUpdater(DefaultPromise.class, Object.class, "result"); private static final Object SUCCESS = new Object(); private static final Object UNCANCELLABLE = new Object(); - private static final CauseHolder CANCELLATION_CAUSE_HOLDER = new CauseHolder(ThrowableUtil.unknownStackTrace( - new CancellationException(), DefaultPromise.class, "cancel(...)")); + private static final CauseHolder CANCELLATION_CAUSE_HOLDER = new CauseHolder( + StacklessCancellationException.newInstance(DefaultPromise.class, "cancel(...)")); private static final StackTraceElement[] CANCELLATION_STACK = CANCELLATION_CAUSE_HOLDER.cause.getStackTrace(); private volatile Object result; @@ -843,4 +843,22 @@ public class DefaultPromise extends AbstractFuture implements Promise { rejectedExecutionLogger.error("Failed to submit a listener notification task. Event loop shut down?", t); } } + + private static final class StacklessCancellationException extends CancellationException { + + private static final long serialVersionUID = -2974906711413716191L; + + private StacklessCancellationException() { } + + // Override fillInStackTrace() so we not populate the backtrace via a native call and so leak the + // Classloader. + @Override + public Throwable fillInStackTrace() { + return this; + } + + static StacklessCancellationException newInstance(Class clazz, String method) { + return ThrowableUtil.unknownStackTrace(new StacklessCancellationException(), clazz, method); + } + } } diff --git a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java index 3cde0e0083..b7cc4d21f3 100644 --- a/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java +++ b/resolver-dns/src/main/java/io/netty/resolver/dns/DnsResolveContext.java @@ -62,26 +62,21 @@ import static java.lang.Math.min; abstract class DnsResolveContext { - private static final RuntimeException NXDOMAIN_QUERY_FAILED_EXCEPTION = ThrowableUtil.unknownStackTrace( - DnsResolveContextException.newStatic("No answer found and NXDOMAIN response code returned"), - DnsResolveContext.class, - "onResponse(..)"); - private static final RuntimeException CNAME_NOT_FOUND_QUERY_FAILED_EXCEPTION = ThrowableUtil.unknownStackTrace( - DnsResolveContextException.newStatic("No matching CNAME record found"), - DnsResolveContext.class, - "onResponseCNAME(..)"); - private static final RuntimeException NO_MATCHING_RECORD_QUERY_FAILED_EXCEPTION = ThrowableUtil.unknownStackTrace( - DnsResolveContextException.newStatic("No matching record type found"), - DnsResolveContext.class, - "onResponseAorAAAA(..)"); - private static final RuntimeException UNRECOGNIZED_TYPE_QUERY_FAILED_EXCEPTION = ThrowableUtil.unknownStackTrace( - new RuntimeException("Response type was unrecognized"), - DnsResolveContext.class, - "onResponse(..)"); - private static final RuntimeException NAME_SERVERS_EXHAUSTED_EXCEPTION = ThrowableUtil.unknownStackTrace( - DnsResolveContextException.newStatic("No name servers returned an answer"), - DnsResolveContext.class, - "tryToFinishResolve(..)"); + private static final RuntimeException NXDOMAIN_QUERY_FAILED_EXCEPTION = + DnsResolveContextException.newStatic("No answer found and NXDOMAIN response code returned", + DnsResolveContext.class, "onResponse(..)"); + private static final RuntimeException CNAME_NOT_FOUND_QUERY_FAILED_EXCEPTION = + DnsResolveContextException.newStatic("No matching CNAME record found", + DnsResolveContext.class, "onResponseCNAME(..)"); + private static final RuntimeException NO_MATCHING_RECORD_QUERY_FAILED_EXCEPTION = + DnsResolveContextException.newStatic("No matching record type found", + DnsResolveContext.class, "onResponseAorAAAA(..)"); + private static final RuntimeException UNRECOGNIZED_TYPE_QUERY_FAILED_EXCEPTION = + DnsResolveContextException.newStatic("Response type was unrecognized", + DnsResolveContext.class, "onResponse(..)"); + private static final RuntimeException NAME_SERVERS_EXHAUSTED_EXCEPTION = + DnsResolveContextException.newStatic("No name servers returned an answer", + DnsResolveContext.class, "tryToFinishResolve(..)"); final DnsNameResolver parent; private final Promise originalPromise; @@ -118,6 +113,8 @@ abstract class DnsResolveContext { static final class DnsResolveContextException extends RuntimeException { + private static final long serialVersionUID = 1209303419266433003L; + private DnsResolveContextException(String message) { super(message); } @@ -129,11 +126,21 @@ abstract class DnsResolveContext { assert shared; } - static DnsResolveContextException newStatic(String message) { + // Override fillInStackTrace() so we not populate the backtrace via a native call and so leak the + // Classloader. + @Override + public Throwable fillInStackTrace() { + return this; + } + + static DnsResolveContextException newStatic(String message, Class clazz, String method) { + final DnsResolveContextException exception; if (PlatformDependent.javaVersion() >= 7) { - return new DnsResolveContextException(message, true); + exception = new DnsResolveContextException(message, true); + } else { + exception = new DnsResolveContextException(message); } - return new DnsResolveContextException(message); + return ThrowableUtil.unknownStackTrace(exception, clazz, method); } } diff --git a/transport/src/main/java/io/netty/channel/ChannelException.java b/transport/src/main/java/io/netty/channel/ChannelException.java index 32f4642202..dd70829727 100644 --- a/transport/src/main/java/io/netty/channel/ChannelException.java +++ b/transport/src/main/java/io/netty/channel/ChannelException.java @@ -17,6 +17,7 @@ package io.netty.channel; import io.netty.util.internal.PlatformDependent; import io.netty.util.internal.SuppressJava6Requirement; +import io.netty.util.internal.ThrowableUtil; import io.netty.util.internal.UnstableApi; /** @@ -61,10 +62,33 @@ public class ChannelException extends RuntimeException { assert shared; } - static ChannelException newStatic(String message, Throwable cause) { + static ChannelException newStatic(String message, Class clazz, String method) { + ChannelException exception; if (PlatformDependent.javaVersion() >= 7) { - return new ChannelException(message, cause, true); + exception = new StacklessChannelException(message, null, true); + } else { + exception = new StacklessChannelException(message, null); + } + return ThrowableUtil.unknownStackTrace(exception, clazz, method); + } + + private static final class StacklessChannelException extends ChannelException { + private static final long serialVersionUID = -6384642137753538579L; + + StacklessChannelException(String message, Throwable cause) { + super(message, cause); + } + + StacklessChannelException(String message, Throwable cause, boolean shared) { + super(message, cause, shared); + } + + // Override fillInStackTrace() so we not populate the backtrace via a native call and so leak the + // Classloader. + + @Override + public Throwable fillInStackTrace() { + return this; } - return new ChannelException(message, cause); } } diff --git a/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoopGroup.java b/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoopGroup.java index a3a95a6309..f60844f59a 100644 --- a/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoopGroup.java +++ b/transport/src/main/java/io/netty/channel/ThreadPerChannelEventLoopGroup.java @@ -29,7 +29,6 @@ import io.netty.util.internal.EmptyArrays; import io.netty.util.internal.ObjectUtil; import io.netty.util.internal.PlatformDependent; import io.netty.util.internal.ReadOnlyIterator; -import io.netty.util.internal.ThrowableUtil; import java.util.Collections; import java.util.Iterator; @@ -132,8 +131,8 @@ public class ThreadPerChannelEventLoopGroup extends AbstractEventExecutorGroup i this.maxChannels = maxChannels; this.executor = executor; - tooManyChannels = ThrowableUtil.unknownStackTrace( - ChannelException.newStatic("too many channels (max: " + maxChannels + ')', null), + tooManyChannels = + ChannelException.newStatic("too many channels (max: " + maxChannels + ')', ThreadPerChannelEventLoopGroup.class, "nextChild()"); }