Make static final buffers unreleasable and read-only (#11802)

Motivation: Fix #11792 and avoid potential corruption issues caused by writable shared buffers. TL;DR: `HttpObjectEncoder` sends a duplicate of its static final `ByteBuf` field to encode HTTP chunks, such as `0\r\n\r\n`. This reused buffer is neither read-only nor unexpandable, and thus anyone in the pipeline can modify or expand it. As a result, `SslHandler` can sometimes expand it and append stuff to it, causing unexpected data corruption. Modification: Most of the static final buffers in the project are made unreleasable and read-only by wrapping them in `Unpooled.unreleasableBuffer(...).asReadOnly()`. Buffers that were excluded: - Unpooled.EMPTY_BUFFER - EmptyByteBuf instances are not readable/writable/releasable. Wrapping this buffer in `Unpooled.unreleasableBuffer(...).asReadOnly()` would change the instance type, since it's a public API it could be a potential breaking change. - ReplayingDecoderByteBuf.EMPTY_BUFFER - since it uses Unpooled.EMPTY_BUFFER and not releasable too. Excluded buffers are marked with `SuppressWarnings` to skip check added in netty/netty-build#12. Result: Fixes #11792.
netty · Nov 3, 2021 · 14f04b5 · 14f04b5
1 parent 5caecec
commit 14f04b5
Show file tree

Hide file tree

Showing 12 changed files with 33 additions and 29 deletions.
diff --git a/buffer/src/main/java/io/netty/buffer/Unpooled.java b/buffer/src/main/java/io/netty/buffer/Unpooled.java
@@ -88,6 +88,7 @@ public final class Unpooled {
     /**
      * A buffer whose capacity is {@code 0}.
      */
+    @SuppressWarnings("checkstyle:StaticFinalBuffer")  // EmptyByteBuf is not writeable or readable.
     public static final ByteBuf EMPTY_BUFFER = ALLOC.buffer(0, 0);
 
     static {

diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java
@@ -51,9 +51,10 @@ public abstract class HttpObjectEncoder<H extends HttpMessage> extends MessageTo
     static final int CRLF_SHORT = (CR << 8) | LF;
     private static final int ZERO_CRLF_MEDIUM = ('0' << 16) | CRLF_SHORT;
     private static final byte[] ZERO_CRLF_CRLF = { '0', CR, LF, CR, LF };
-    private static final ByteBuf CRLF_BUF = unreleasableBuffer(directBuffer(2).writeByte(CR).writeByte(LF));
-    private static final ByteBuf ZERO_CRLF_CRLF_BUF = unreleasableBuffer(directBuffer(ZERO_CRLF_CRLF.length)
-            .writeBytes(ZERO_CRLF_CRLF));
+    private static final ByteBuf CRLF_BUF = unreleasableBuffer(
+            directBuffer(2).writeByte(CR).writeByte(LF)).asReadOnly();
+    private static final ByteBuf ZERO_CRLF_CRLF_BUF = unreleasableBuffer(
+            directBuffer(ZERO_CRLF_CRLF.length).writeBytes(ZERO_CRLF_CRLF)).asReadOnly();
     private static final float HEADERS_WEIGHT_NEW = 1 / 5f;
     private static final float HEADERS_WEIGHT_HISTORICAL = 1 - HEADERS_WEIGHT_NEW;
     private static final float TRAILERS_WEIGHT_NEW = HEADERS_WEIGHT_NEW;

diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket00FrameEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket00FrameEncoder.java
@@ -32,11 +32,11 @@
 @Sharable
 public class WebSocket00FrameEncoder extends MessageToMessageEncoder<WebSocketFrame> implements WebSocketFrameEncoder {
     private static final ByteBuf _0X00 = Unpooled.unreleasableBuffer(
-            Unpooled.directBuffer(1, 1).writeByte((byte) 0x00));
+            Unpooled.directBuffer(1, 1).writeByte((byte) 0x00)).asReadOnly();
     private static final ByteBuf _0XFF = Unpooled.unreleasableBuffer(
-            Unpooled.directBuffer(1, 1).writeByte((byte) 0xFF));
+            Unpooled.directBuffer(1, 1).writeByte((byte) 0xFF)).asReadOnly();
     private static final ByteBuf _0XFF_0X00 = Unpooled.unreleasableBuffer(
-            Unpooled.directBuffer(2, 2).writeByte((byte) 0xFF).writeByte((byte) 0x00));
+            Unpooled.directBuffer(2, 2).writeByte((byte) 0xFF).writeByte((byte) 0x00)).asReadOnly();
 
     @Override
     protected void encode(ChannelHandlerContext ctx, WebSocketFrame msg, List<Object> out) throws Exception {

diff --git a/...-http2/src/main/java/io/netty/handler/codec/http2/CleartextHttp2ServerUpgradeHandler.java b/...-http2/src/main/java/io/netty/handler/codec/http2/CleartextHttp2ServerUpgradeHandler.java
@@ -37,7 +37,7 @@
  */
 @UnstableApi
 public final class CleartextHttp2ServerUpgradeHandler extends ByteToMessageDecoder {
-    private static final ByteBuf CONNECTION_PREFACE = unreleasableBuffer(connectionPrefaceBuf());
+    private static final ByteBuf CONNECTION_PREFACE = unreleasableBuffer(connectionPrefaceBuf()).asReadOnly();
 
     private final HttpServerCodec httpServerCodec;
     private final HttpServerUpgradeHandler httpServerUpgradeHandler;

diff --git a/codec-smtp/src/main/java/io/netty/handler/codec/smtp/SmtpRequestEncoder.java b/codec-smtp/src/main/java/io/netty/handler/codec/smtp/SmtpRequestEncoder.java
@@ -34,7 +34,7 @@ public final class SmtpRequestEncoder extends MessageToMessageEncoder<Object> {
     private static final int CRLF_SHORT = ('\r' << 8) | '\n';
     private static final byte SP = ' ';
     private static final ByteBuf DOT_CRLF_BUFFER = Unpooled.unreleasableBuffer(
-            Unpooled.directBuffer(3).writeByte('.').writeByte('\r').writeByte('\n'));
+            Unpooled.directBuffer(3).writeByte('.').writeByte('\r').writeByte('\n')).asReadOnly();
 
     private boolean contentExpected;
 

diff --git a/codec/src/test/java/io/netty/handler/codec/compression/AbstractDecoderTest.java b/codec/src/test/java/io/netty/handler/codec/compression/AbstractDecoderTest.java
@@ -31,13 +31,10 @@
 @TestInstance(TestInstance.Lifecycle.PER_CLASS)
 public abstract class AbstractDecoderTest extends AbstractCompressionTest {
 
-    protected static final ByteBuf WRAPPED_BYTES_SMALL;
-    protected static final ByteBuf WRAPPED_BYTES_LARGE;
-
-    static {
-        WRAPPED_BYTES_SMALL = Unpooled.wrappedBuffer(BYTES_SMALL);
-        WRAPPED_BYTES_LARGE = Unpooled.wrappedBuffer(BYTES_LARGE);
-    }
+    protected static final ByteBuf WRAPPED_BYTES_SMALL = Unpooled.unreleasableBuffer(
+            Unpooled.wrappedBuffer(BYTES_SMALL)).asReadOnly();
+    protected static final ByteBuf WRAPPED_BYTES_LARGE = Unpooled.unreleasableBuffer(
+            Unpooled.wrappedBuffer(BYTES_LARGE)).asReadOnly();
 
     protected EmbeddedChannel channel;
 
@@ -86,19 +83,19 @@ public ByteBuf[] largeData() {
     @ParameterizedTest
     @MethodSource("smallData")
     public void testDecompressionOfSmallChunkOfData(ByteBuf data) throws Exception {
-        testDecompression(WRAPPED_BYTES_SMALL, data);
+        testDecompression(WRAPPED_BYTES_SMALL.duplicate(), data);
     }
 
     @ParameterizedTest
     @MethodSource("largeData")
     public void testDecompressionOfLargeChunkOfData(ByteBuf data) throws Exception {
-        testDecompression(WRAPPED_BYTES_LARGE, data);
+        testDecompression(WRAPPED_BYTES_LARGE.duplicate(), data);
     }
 
     @ParameterizedTest
     @MethodSource("largeData")
     public void testDecompressionOfBatchedFlowOfData(ByteBuf data) throws Exception {
-        testDecompressionOfBatchedFlow(WRAPPED_BYTES_LARGE, data);
+        testDecompressionOfBatchedFlow(WRAPPED_BYTES_LARGE.duplicate(), data);
     }
 
     protected void testDecompression(final ByteBuf expected, final ByteBuf data) throws Exception {

diff --git a/codec/src/test/java/io/netty/handler/codec/compression/BrotliDecoderTest.java b/codec/src/test/java/io/netty/handler/codec/compression/BrotliDecoderTest.java
@@ -41,8 +41,6 @@ public class BrotliDecoderTest {
     private static final Random RANDOM;
     private static final byte[] BYTES_SMALL = new byte[256];
     private static final byte[] BYTES_LARGE = new byte[256 * 1024];
-    private static final ByteBuf WRAPPED_BYTES_SMALL;
-    private static final ByteBuf WRAPPED_BYTES_LARGE;
     private static final byte[] COMPRESSED_BYTES_SMALL;
     private static final byte[] COMPRESSED_BYTES_LARGE;
 
@@ -52,15 +50,18 @@ public class BrotliDecoderTest {
             RANDOM = new Random();
             fillArrayWithCompressibleData(BYTES_SMALL);
             fillArrayWithCompressibleData(BYTES_LARGE);
-            WRAPPED_BYTES_SMALL = Unpooled.wrappedBuffer(BYTES_SMALL);
-            WRAPPED_BYTES_LARGE = Unpooled.wrappedBuffer(BYTES_LARGE);
             COMPRESSED_BYTES_SMALL = compress(BYTES_SMALL);
             COMPRESSED_BYTES_LARGE = compress(BYTES_LARGE);
         } catch (Throwable throwable) {
             throw new ExceptionInInitializerError(throwable);
         }
     }
 
+    private static final ByteBuf WRAPPED_BYTES_SMALL = Unpooled.unreleasableBuffer(
+            Unpooled.wrappedBuffer(BYTES_SMALL)).asReadOnly();
+    private static final ByteBuf WRAPPED_BYTES_LARGE = Unpooled.unreleasableBuffer(
+            Unpooled.wrappedBuffer(BYTES_LARGE)).asReadOnly();
+
     static boolean isNotSupported() {
         return PlatformDependent.isOsx() && "aarch_64".equals(PlatformDependent.normalizedArch());
     }
@@ -111,13 +112,13 @@ public static ByteBuf[] largeData() {
     @ParameterizedTest
     @MethodSource("smallData")
     public void testDecompressionOfSmallChunkOfData(ByteBuf data) {
-        testDecompression(WRAPPED_BYTES_SMALL, data);
+        testDecompression(WRAPPED_BYTES_SMALL.duplicate(), data);
     }
 
     @ParameterizedTest
     @MethodSource("largeData")
     public void testDecompressionOfLargeChunkOfData(ByteBuf data) {
-        testDecompression(WRAPPED_BYTES_LARGE, data);
+        testDecompression(WRAPPED_BYTES_LARGE.duplicate(), data);
     }
 
     @ParameterizedTest

diff --git a/.../src/main/java/io/netty/example/http2/helloworld/frame/server/HelloWorldHttp2Handler.java b/.../src/main/java/io/netty/example/http2/helloworld/frame/server/HelloWorldHttp2Handler.java
@@ -42,7 +42,8 @@
 @Sharable
 public class HelloWorldHttp2Handler implements ChannelHandler {
 
-    static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(copiedBuffer("Hello World", CharsetUtil.UTF_8));
+    static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(
+            copiedBuffer("Hello World", CharsetUtil.UTF_8)).asReadOnly();
 
     @Override
     public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {

diff --git a/.../main/java/io/netty/example/http2/helloworld/multiplex/server/HelloWorldHttp2Handler.java b/.../main/java/io/netty/example/http2/helloworld/multiplex/server/HelloWorldHttp2Handler.java
@@ -40,7 +40,8 @@
 @Sharable
 public class HelloWorldHttp2Handler implements ChannelHandler {
 
-    static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(copiedBuffer("Hello World", CharsetUtil.UTF_8));
+    static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(
+            copiedBuffer("Hello World", CharsetUtil.UTF_8)).asReadOnly();
 
     @Override
     public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {

diff --git a/example/src/main/java/io/netty/example/http2/helloworld/server/HelloWorldHttp2Handler.java b/example/src/main/java/io/netty/example/http2/helloworld/server/HelloWorldHttp2Handler.java
@@ -42,7 +42,8 @@
  */
 public final class HelloWorldHttp2Handler extends Http2ConnectionHandler implements Http2FrameListener {
 
-    static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(copiedBuffer("Hello World", CharsetUtil.UTF_8));
+    static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(
+            copiedBuffer("Hello World", CharsetUtil.UTF_8)).asReadOnly();
 
     HelloWorldHttp2Handler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
                            Http2Settings initialSettings) {

diff --git a/pom.xml b/pom.xml
@@ -444,7 +444,7 @@
     <netty.dev.tools.directory>${project.build.directory}/dev-tools</netty.dev.tools.directory>
     <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
     <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
-    <netty.build.version>29</netty.build.version>
+    <netty.build.version>30</netty.build.version>
     <jboss.marshalling.version>1.4.11.Final</jboss.marshalling.version>
     <jetty.alpnAgent.version>2.0.10</jetty.alpnAgent.version>
     <jetty.alpnAgent.path>"${settings.localRepository}"/org/mortbay/jetty/alpn/jetty-alpn-agent/${jetty.alpnAgent.version}/jetty-alpn-agent-${jetty.alpnAgent.version}.jar</jetty.alpnAgent.path>

diff --git a/testsuite-http2/src/main/java/io/netty/testsuite/http2/HelloWorldHttp2Handler.java b/testsuite-http2/src/main/java/io/netty/testsuite/http2/HelloWorldHttp2Handler.java
@@ -42,7 +42,8 @@
  */
 public final class HelloWorldHttp2Handler extends Http2ConnectionHandler implements Http2FrameListener {
 
-    static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(copiedBuffer("Hello World", CharsetUtil.UTF_8));
+    static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(
+            copiedBuffer("Hello World", CharsetUtil.UTF_8)).asReadOnly();
 
     HelloWorldHttp2Handler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
                            Http2Settings initialSettings) {