Utf8FrameValidator must release buffer when validation fails (#9909)

Motivation: Utf8FrameValidator must release the input buffer if the validation fails to ensure no memory leak happens Modifications: - Catch exception, release frame and rethrow - Adjust unit test Result: Fixes #9906
netty · Dec 27, 2019 · 06a5173 · 06a5173
1 parent bc026ef
commit 06a5173
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 31 deletions.
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/Utf8FrameValidator.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/Utf8FrameValidator.java
@@ -35,42 +35,47 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
         if (msg instanceof WebSocketFrame) {
             WebSocketFrame frame = (WebSocketFrame) msg;
 
-            // Processing for possible fragmented messages for text and binary
-            // frames
-            if (((WebSocketFrame) msg).isFinalFragment()) {
-                // Final frame of the sequence. Apparently ping frames are
-                // allowed in the middle of a fragmented message
-                if (!(frame instanceof PingWebSocketFrame)) {
-                    fragmentedFramesCount = 0;
+            try {
+                // Processing for possible fragmented messages for text and binary
+                // frames
+                if (((WebSocketFrame) msg).isFinalFragment()) {
+                    // Final frame of the sequence. Apparently ping frames are
+                    // allowed in the middle of a fragmented message
+                    if (!(frame instanceof PingWebSocketFrame)) {
+                        fragmentedFramesCount = 0;
 
-                    // Check text for UTF8 correctness
-                    if ((frame instanceof TextWebSocketFrame) ||
-                            (utf8Validator != null && utf8Validator.isChecking())) {
-                        // Check UTF-8 correctness for this payload
-                        checkUTF8String(frame.content());
+                        // Check text for UTF8 correctness
+                        if ((frame instanceof TextWebSocketFrame) ||
+                                (utf8Validator != null && utf8Validator.isChecking())) {
+                            // Check UTF-8 correctness for this payload
+                            checkUTF8String(frame.content());
 
-                        // This does a second check to make sure UTF-8
-                        // correctness for entire text message
-                        utf8Validator.finish();
-                    }
-                }
-            } else {
-                // Not final frame so we can expect more frames in the
-                // fragmented sequence
-                if (fragmentedFramesCount == 0) {
-                    // First text or binary frame for a fragmented set
-                    if (frame instanceof TextWebSocketFrame) {
-                        checkUTF8String(frame.content());
+                            // This does a second check to make sure UTF-8
+                            // correctness for entire text message
+                            utf8Validator.finish();
+                        }
                     }
                 } else {
-                    // Subsequent frames - only check if init frame is text
-                    if (utf8Validator != null && utf8Validator.isChecking()) {
-                        checkUTF8String(frame.content());
+                    // Not final frame so we can expect more frames in the
+                    // fragmented sequence
+                    if (fragmentedFramesCount == 0) {
+                        // First text or binary frame for a fragmented set
+                        if (frame instanceof TextWebSocketFrame) {
+                            checkUTF8String(frame.content());
+                        }
+                    } else {
+                        // Subsequent frames - only check if init frame is text
+                        if (utf8Validator != null && utf8Validator.isChecking()) {
+                            checkUTF8String(frame.content());
+                        }
                     }
-                }
 
-                // Increment counter
-                fragmentedFramesCount++;
+                    // Increment counter
+                    fragmentedFramesCount++;
+                }
+            } catch (CorruptedWebSocketFrameException e) {
+                frame.release();
+                throw e;
             }
         }
 

diff --git a/...src/test/java/io/netty/handler/codec/http/websocketx/WebSocketUtf8FrameValidatorTest.java b/...src/test/java/io/netty/handler/codec/http/websocketx/WebSocketUtf8FrameValidatorTest.java
@@ -36,8 +36,9 @@ public void testCorruptedFrameExceptionInCheck() {
 
     private void assertCorruptedFrameExceptionHandling(byte[] data) {
         EmbeddedChannel channel = new EmbeddedChannel(new Utf8FrameValidator());
+        TextWebSocketFrame frame = new TextWebSocketFrame(Unpooled.copiedBuffer(data));
         try {
-            channel.writeInbound(new TextWebSocketFrame(Unpooled.copiedBuffer(data)));
+            channel.writeInbound(frame);
             Assert.fail();
         } catch (CorruptedFrameException e) {
             // expected exception
@@ -51,5 +52,6 @@ private void assertCorruptedFrameExceptionHandling(byte[] data) {
             buf.release();
         }
         Assert.assertNull(channel.readOutbound());
+        Assert.assertEquals(0, frame.refCnt());
     }
 }