BUG-2838: Trim leading whitespaces from incoming NETCONF messages. 27/16927/5
authorGwenael Lambrouin <gwenael.lambrouin@b-com.com>
Thu, 21 May 2015 13:32:35 +0000 (15:32 +0200)
committerGerrit Code Review <gerrit@opendaylight.org>
Tue, 26 May 2015 11:54:11 +0000 (11:54 +0000)
Some network devices (such as Cisco routers) send RPC replies with a leading
newline before the XML declaration. The OpenDaylight controller does not accept
those replies: the XML parsing fails.

This patch fixes the NETCONF messages before they are sent to the XML parser:
it removes all the spurious characters at the beginning of the messages.

Change-Id: Ibc6eb6dc5bad6252a3c9bed73d3db83814aff501
Signed-off-by: Gwenael Lambrouin <gwenael.lambrouin@b-com.com>
Signed-off-by: Maros Marsalek <mmarsale@cisco.com>
opendaylight/netconf/netconf-netty-util/src/main/java/org/opendaylight/controller/netconf/nettyutil/handler/NetconfXMLToMessageDecoder.java
opendaylight/netconf/netconf-netty-util/src/test/java/org/opendaylight/controller/netconf/nettyutil/handler/NetconfXMLToMessageDecoderTest.java

index 1dc8e0233b4b44241eb4e05eb218bb0eb5ae90d1..5ae9bb4b1ca8dab36a29ef9925ed1e74ea3ec889 100644 (file)
@@ -10,6 +10,7 @@ package org.opendaylight.controller.netconf.nettyutil.handler;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufInputStream;
 import io.netty.buffer.ByteBufUtil;
+import io.netty.buffer.Unpooled;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.handler.codec.ByteToMessageDecoder;
 import java.io.IOException;
@@ -30,9 +31,56 @@ public final class NetconfXMLToMessageDecoder extends ByteToMessageDecoder {
                 LOG.trace("Received to decode: {}", ByteBufUtil.hexDump(in));
             }
 
+            /* According to the XML 1.0 specifications, when there is an XML declaration
+             * at the beginning of an XML document, it is invalid to have
+             * white spaces before that declaration (reminder: a XML declaration looks like:
+             * <?xml version="1.0" encoding="UTF-8"?>). In contrast, when there is no XML declaration,
+             * it is valid to have white spaces at the beginning of the document.
+             *
+             * When they send a NETCONF message, several NETCONF servers start with a new line (either
+             * LF or CRLF), presumably to improve readability in interactive sessions with a human being.
+             * Some NETCONF servers send an XML declaration, some others do not.
+             *
+             * If a server starts a NETCONF message with white spaces and follows with an XML
+             * declaration, XmlUtil.readXmlToDocument() will fail because this is invalid XML.
+             * But in the spirit of the "NETCONF over SSH" RFC 4742 and to improve interoperability, we want
+             * to accept those messages.
+             *
+             * To do this, the following code strips the leading bytes before the start of the XML messages.
+             */
+
+            // Skip all leading whitespaces by moving the reader index to the first non whitespace character
+            while (in.isReadable()) {
+                if (!isWhitespace(in.readByte())) {
+                    // return reader index to the first non whitespace character
+                    in.readerIndex(in.readerIndex() - 1);
+                    break;
+                }
+            }
+
+            // Warn about leading whitespaces
+            if (in.readerIndex() != 0 && LOG.isWarnEnabled()) {
+                final byte[] strippedBytes = new byte[in.readerIndex()];
+                in.getBytes(0, strippedBytes, 0, in.readerIndex());
+                LOG.warn("XML message with unwanted leading bytes detected. Discarded the {} leading byte(s): '{}'",
+                        in.readerIndex(), ByteBufUtil.hexDump(Unpooled.wrappedBuffer(strippedBytes)));
+            }
+        }
+        if (in.isReadable()) {
             out.add(new NetconfMessage(XmlUtil.readXmlToDocument(new ByteBufInputStream(in))));
         } else {
             LOG.debug("No more content in incoming buffer.");
         }
     }
+
+    /**
+     * Check whether a byte is whitespace/control character. Considered whitespace characters: <br/>
+     * SPACE, \t, \n, \v, \r, \f
+     *
+     * @param b byte to check
+     * @return true if the byte is a whitespace/control character
+     */
+    private static boolean isWhitespace(final byte b) {
+        return b <= 0x0d && b >= 0x09 || b == 0x20;
+    }
 }
index f85a38769f1f5b451690f63c718d4804c7767719..aaa92e8050d332e3c2b1c8f7b78f62d23114cc55 100644 (file)
@@ -14,6 +14,7 @@ import com.google.common.collect.Lists;
 import io.netty.buffer.Unpooled;
 import java.util.ArrayList;
 import org.junit.Test;
+import org.xml.sax.SAXParseException;
 
 public class NetconfXMLToMessageDecoderTest {
 
@@ -30,4 +31,61 @@ public class NetconfXMLToMessageDecoderTest {
         new NetconfXMLToMessageDecoder().decode(null, Unpooled.wrappedBuffer("<msg/>".getBytes()), out);
         assertEquals(1, out.size());
     }
-}
\ No newline at end of file
+
+    @Test
+    public void testDecodeWithLeadingLFAndXmlDecl() throws Exception {
+        /* Test that we accept XML documents with a line feed (0x0a) before the
+         * XML declaration in the XML prologue.
+         * A leading LF is the case reported in BUG-2838.
+         */
+        final ArrayList<Object> out = Lists.newArrayList();
+        new NetconfXMLToMessageDecoder().decode(null, Unpooled.wrappedBuffer("\n<?xml version=\"1.0\" encoding=\"UTF-8\"?><msg/>".getBytes()), out);
+        assertEquals(1, out.size());
+    }
+
+    @Test
+    public void testDecodeWithLeadingCRLFAndXmlDecl() throws Exception {
+        /* Test that we accept XML documents with both a carriage return and
+         * line feed (0x0d 0x0a) before the XML declaration in the XML prologue.
+         * Leading CRLF can be seen with some Cisco routers
+         * (eg CSR1000V running IOS 15.4(1)S)
+         */
+        final ArrayList<Object> out = Lists.newArrayList();
+        new NetconfXMLToMessageDecoder().decode(null, Unpooled.wrappedBuffer("\r\n<?xml version=\"1.0\" encoding=\"UTF-8\"?><msg/>".getBytes()), out);
+        assertEquals(1, out.size());
+    }
+
+    @Test(expected=SAXParseException.class)
+    public void testDecodeGibberish() throws Exception {
+        /* Test that we reject inputs where we cannot find the xml start '<' character */
+        final ArrayList<Object> out = Lists.newArrayList();
+        new NetconfXMLToMessageDecoder().decode(null, Unpooled.wrappedBuffer("\r\n?xml version>".getBytes()), out);
+        assertEquals(1, out.size());
+    }
+
+    @Test
+    public void testDecodeOnlyWhitespaces() throws Exception {
+        /* Test that we handle properly a bunch of whitespaces.
+         */
+        final ArrayList<Object> out = Lists.newArrayList();
+        new NetconfXMLToMessageDecoder().decode(null, Unpooled.wrappedBuffer("\r\n".getBytes()), out);
+        assertEquals(0, out.size());
+    }
+
+    @Test
+    public void testDecodeWithAllWhitespaces() throws Exception {
+        /* Test that every whitespace we want to skip is actually skipped.
+         */
+
+        final ArrayList<Object> out = Lists.newArrayList();
+        byte whitespaces[] = {' ', '\t', '\n', '\r', '\f', 0x0b /* vertical tab */};
+        new NetconfXMLToMessageDecoder().decode(
+                null,
+                Unpooled.copiedBuffer(
+                        Unpooled.wrappedBuffer(whitespaces),
+                        Unpooled.wrappedBuffer("<?xml version=\"1.0\" encoding=\"UTF-8\"?><msg/>".getBytes())),
+                out);
+        assertEquals(1, out.size());
+    }
+}
+