Repository: netty/netty
Issue: XmlFrameDecoder is corrupt
Labels: defect

Netty Version: 4.1.0.Beta4

Context:
When writing TCP client which exchanges XML messages with server I've encountered an error in XmlFrameDecoder. Given not full XML element, decoder will output not valid XML.

Steps to reproduce:
1. BUG - Decode invalid XML: `'<a><b/></'` - decoder will put it to output list and it will update readerIndex in input byteBuff
2. OK - Decode invalid XML: `'<a><b/>'` - decoder will not put it to output list nor it will update readerIndex in input byteBuff
3. OK - Decode valid XML: `'<a><b/></a>'` - decoder will put it to output list and it will update readerIndex in input byteBuff

Long story short:
XML decoder assumes that as soon as it finds `</` it can decrement opened brackets count. If closing bracket isn't in byteBuff this assumption will create a bug.


Issue comments:
Can you also test with 4.1.0.Beta6-SNAPSHOT just to be sure it is not fixed already ?

I've just checked newest commit on branch 4.1 (94f65ed7ff214de3fc9680d80a5ec2d2fe2aaf48) and the problem still exists. 

The problem is in line 115, below comment _'found </, decrementing openBracketsCount'_. IMHO additional validation should be added there to check if closing bracket '>' is available in ByteBuf. I'll have to fix it, my application is broken beacouse of this bug - I'll create pull request ASAP.

@normanmaurer BTW - last version of netty on Maven central is 4.1.0.Beta4, isn't this some oversight?

@tomaszc I can see Beta5 in maven central as well:
http://repo1.maven.org/maven2/io/netty/netty-all/4.1.0.Beta5/

@normanmaurer you're right, it is there, my mistake (morning without coffe ;) )

Meanwhile I've found additional error in XmlFrameDecoder:

StepsToReproduce:
Configure XmlFrameDecoder in your Channel Pipeline and send XML message with more than two root XML elements: `<a><b/><a><a><b/><a><a><b/><a>`. 
First decoder output will be proper: `<a><b/><a>`. 
Second decoder output will be corrupt: `<a><b/><a><a><b/><a>` instead of `<a><b/><a>`.

Cause:
In XmlFrameDecoder class, final part of decode(....) method is corrupt. When creating output frame (line 169) length parameter is invalid. It is based on input ByteBuff readerIndex, but it is not taken to account that it's value can be higher than 0. Therefore if XmlFrameDecoder will process byteBuff which readerIndex was already moved further, it's output will be corrupt.

Pull request with bugfix was created, hope it pass your verification


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec/src/main/java/io/netty/handler/codec/xml/XmlFrameDecoder.java b/codec/src/main/java/io/netty/handler/codec/xml/XmlFrameDecoder.java
index c4f70974350..4a8b262bf8a 100644
--- a/codec/src/main/java/io/netty/handler/codec/xml/XmlFrameDecoder.java
+++ b/codec/src/main/java/io/netty/handler/codec/xml/XmlFrameDecoder.java
@@ -111,8 +111,16 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
                 if (i < bufferLength - 1) {
                     final byte peekAheadByte = in.getByte(i + 1);
                     if (peekAheadByte == '/') {
-                        // found </, decrementing openBracketsCount
-                        openBracketsCount--;
+                        // found </, we must check if it is enclosed
+                        int peekFurtherAheadIndex = i + 2;
+                        while (peekFurtherAheadIndex <= bufferLength - 1) {
+                            //if we have </ and enclosing > we can decrement openBracketsCount
+                            if (in.getByte(peekFurtherAheadIndex) == '>') {
+                                openBracketsCount--;
+                                break;
+                            }
+                            peekFurtherAheadIndex++;
+                        }
                     } else if (isValidStartCharForXmlElement(peekAheadByte)) {
                         atLeastOneXmlElementFound = true;
                         // char after < is a valid xml element start char,
@@ -166,14 +174,15 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
         }
 
         final int readerIndex = in.readerIndex();
+        int xmlElementLength = length - readerIndex;
 
-        if (openBracketsCount == 0 && length > 0) {
-            if (length >= bufferLength) {
-                length = in.readableBytes();
+        if (openBracketsCount == 0 && xmlElementLength > 0) {
+            if (readerIndex + xmlElementLength >= bufferLength) {
+                xmlElementLength = in.readableBytes();
             }
             final ByteBuf frame =
-                    extractFrame(in, readerIndex + leadingWhiteSpaceCount, length - leadingWhiteSpaceCount);
-            in.skipBytes(length);
+                    extractFrame(in, readerIndex + leadingWhiteSpaceCount, xmlElementLength - leadingWhiteSpaceCount);
+            in.skipBytes(xmlElementLength);
             out.add(frame);
         }
     }


Test patch:
diff --git a/codec/src/test/java/io/netty/handler/codec/xml/XmlFrameDecoderTest.java b/codec/src/test/java/io/netty/handler/codec/xml/XmlFrameDecoderTest.java
index 62f6719fd7b..db3660accf8 100644
--- a/codec/src/test/java/io/netty/handler/codec/xml/XmlFrameDecoderTest.java
+++ b/codec/src/test/java/io/netty/handler/codec/xml/XmlFrameDecoderTest.java
@@ -101,6 +101,12 @@ public void testDecodeShortValidXmlWithLeadingWhitespace02AndTrailingGarbage() {
         testDecodeWithXml("  \n\r \t<xxx/>\ttrash", "<xxx/>", CorruptedFrameException.class);
     }
 
+    @Test
+    public void testDecodeInvalidXml() {
+        testDecodeWithXml("<a></", new Object[0]);
+        testDecodeWithXml("<a></a", new Object[0]);
+    }
+
     @Test
     public void testDecodeWithCDATABlock() {
         final String xml = "<book>" +
@@ -119,18 +125,21 @@ public void testDecodeWithCDATABlockContainingNestedUnbalancedXml() {
     }
 
     @Test
-    public void testDecodeWithTwoMessages() {
+    public void testDecodeWithMultipleMessages() {
         final String input = "<root xmlns=\"http://www.acme.com/acme\" status=\"loginok\" " +
                 "timestamp=\"1362410583776\"/>\n\n" +
                 "<root xmlns=\"http://www.acme.com/acme\" status=\"start\" time=\"0\" " +
                 "timestamp=\"1362410584794\">\n<child active=\"1\" status=\"started\" id=\"935449\" " +
-                "msgnr=\"2\"/>\n</root>";
+                "msgnr=\"2\"/>\n</root>" +
+                "<root xmlns=\"http://www.acme.com/acme\" status=\"logout\" timestamp=\"1362410584795\"/>";
         final String frame1 = "<root xmlns=\"http://www.acme.com/acme\" status=\"loginok\" " +
                 "timestamp=\"1362410583776\"/>";
         final String frame2 = "<root xmlns=\"http://www.acme.com/acme\" status=\"start\" time=\"0\" " +
                 "timestamp=\"1362410584794\">\n<child active=\"1\" status=\"started\" id=\"935449\" " +
                 "msgnr=\"2\"/>\n</root>";
-        testDecodeWithXml(input, frame1, frame2);
+        final String frame3 = "<root xmlns=\"http://www.acme.com/acme\" status=\"logout\" " +
+                "timestamp=\"1362410584795\"/>";
+        testDecodeWithXml(input, frame1, frame2, frame3);
     }
 
     @Test