From 0e503ccd97ca5287a14d3147b0deaa99892ccd6f Mon Sep 17 00:00:00 2001
From: Joanna Hulboj <joanna.hulboj@isode.com>
Date: Sat, 18 Mar 2017 18:52:56 +0000
Subject: Modified XMLBeautifier to handle split payloads

When receiving network data we were processing it in chunks. Sometimes
one XML message got split across multiple chunks. XMLBeautifier was
stateless and would create a new parser for every single chunk. This was
causing multi-chunk messages to be truncated in the beautified output.
The change I made in XMLBeautifier allows it to maintain the state.

Test-Information:

Tested unger Windows 10 and CentOS.

Unit tests pass OK.

Change-Id: Idad2a8e0248ed3cbe2b47a12718b909e56ac1279

diff --git a/Sluift/ElementConvertors/DOMElementConvertor.cpp b/Sluift/ElementConvertors/DOMElementConvertor.cpp
index b957686..72474bb 100644
--- a/Sluift/ElementConvertors/DOMElementConvertor.cpp
+++ b/Sluift/ElementConvertors/DOMElementConvertor.cpp
@@ -185,7 +185,7 @@ boost::optional<std::string> DOMElementConvertor::convertToLua(
 
     // Parse the payload again
     ParserClient parserClient(L);
-    std::shared_ptr<XMLParser> parser(parsers.createXMLParser(&parserClient));
+    std::shared_ptr<XMLParser> parser(std::move(parsers.createXMLParser(&parserClient)));
     bool result = parser->parse(serializedPayload);
     assert(result);
 
diff --git a/Swift/Controllers/Settings/XMLSettingsProvider.cpp b/Swift/Controllers/Settings/XMLSettingsProvider.cpp
index 4dfb8bd..2573af0 100644
--- a/Swift/Controllers/Settings/XMLSettingsProvider.cpp
+++ b/Swift/Controllers/Settings/XMLSettingsProvider.cpp
@@ -18,14 +18,13 @@ namespace Swift {
 XMLSettingsProvider::XMLSettingsProvider(const std::string& xmlConfig) : level_(0) {
     if (!xmlConfig.empty()) {
         PlatformXMLParserFactory factory;
-        XMLParser* parser = factory.createXMLParser(this);
+        auto parser = factory.createXMLParser(this);
         if (parser->parse(xmlConfig)) {
             SWIFT_LOG(debug) << "Found and parsed system config" << std::endl;
         }
         else {
             SWIFT_LOG(debug) << "Found invalid system config" << std::endl;
         }
-        delete parser;
     }
     else {
         SWIFT_LOG(debug) << "No system config found" << std::endl;
diff --git a/Swiften/Client/ClientXMLTracer.cpp b/Swiften/Client/ClientXMLTracer.cpp
index 4852aa1..e205f41 100644
--- a/Swiften/Client/ClientXMLTracer.cpp
+++ b/Swiften/Client/ClientXMLTracer.cpp
@@ -14,23 +14,19 @@
 
 namespace Swift {
 
-ClientXMLTracer::ClientXMLTracer(CoreClient* client, bool bosh) : bosh(bosh) {
+ClientXMLTracer::ClientXMLTracer(CoreClient* client, bool bosh) : bosh_(bosh) {
 #ifdef SWIFTEN_PLATFORM_WIN32
-    beautifier = new XMLBeautifier(true, false);
+    beautifier_ = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
 #else
-    beautifier = new XMLBeautifier(true, true);
+    beautifier_ = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, true));
 #endif
-    onDataReadConnection = client->onDataRead.connect(boost::bind(&ClientXMLTracer::printData, this, '<', _1));
-    onDataWrittenConnection = client->onDataWritten.connect(boost::bind(&ClientXMLTracer::printData, this, '>', _1));
-}
-
-ClientXMLTracer::~ClientXMLTracer() {
-    delete beautifier;
+    onDataReadConnection_ = client->onDataRead.connect(boost::bind(&ClientXMLTracer::printData, this, '<', _1));
+    onDataWrittenConnection_ = client->onDataWritten.connect(boost::bind(&ClientXMLTracer::printData, this, '>', _1));
 }
 
 void ClientXMLTracer::printData(char direction, const SafeByteArray& data) {
-    printLine(direction);
-    if (bosh) {
+    if (bosh_) {
+        printLine(direction);
         std::string line = byteArrayToString(ByteArray(data.begin(), data.end()));
 // Disabled because it swallows bits of XML (namespaces, if I recall)
 //        size_t endOfHTTP = line.find("\r\n\r\n");
@@ -42,7 +38,16 @@ void ClientXMLTracer::printData(char direction, const SafeByteArray& data) {
 //        }
     }
     else {
-        std::cerr << beautifier->beautify(byteArrayToString(ByteArray(data.begin(), data.end()))) << std::endl;
+        const auto& str = beautifier_->beautify(byteArrayToString(ByteArray(data.begin(), data.end())));
+
+        if (beautifier_->wasReset()) {
+            printLine(direction);
+        }
+        std::cerr << str;
+        if (beautifier_->getLevel() <= 1) {
+            std::cerr << std::endl;
+        }
+
     }
 }
 
diff --git a/Swiften/Client/ClientXMLTracer.h b/Swiften/Client/ClientXMLTracer.h
index ad28fe2..d3aeb0c 100644
--- a/Swiften/Client/ClientXMLTracer.h
+++ b/Swiften/Client/ClientXMLTracer.h
@@ -6,6 +6,8 @@
 
 #pragma once
 
+#include <memory>
+
 #include <Swiften/Base/API.h>
 #include <Swiften/Base/SafeByteArray.h>
 #include <Swiften/Client/CoreClient.h>
@@ -15,16 +17,15 @@ namespace Swift {
     class SWIFTEN_API ClientXMLTracer {
         public:
             ClientXMLTracer(CoreClient* client, bool bosh = false);
-            ~ClientXMLTracer();
 
         private:
             void printData(char direction, const SafeByteArray& data);
             void printLine(char c);
 
         private:
-            XMLBeautifier *beautifier;
-            bool bosh;
-            boost::signals2::scoped_connection onDataReadConnection;
-            boost::signals2::scoped_connection onDataWrittenConnection;
+            std::unique_ptr<XMLBeautifier> beautifier_;
+            bool bosh_;
+            boost::signals2::scoped_connection onDataReadConnection_;
+            boost::signals2::scoped_connection onDataWrittenConnection_;
     };
 }
diff --git a/Swiften/Client/UnitTest/XMLBeautifierTest.cpp b/Swiften/Client/UnitTest/XMLBeautifierTest.cpp
new file mode 100644
index 0000000..0188634
--- /dev/null
+++ b/Swiften/Client/UnitTest/XMLBeautifierTest.cpp
@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2010-2017 Isode Limited.
+ * All rights reserved.
+ * See the COPYING file for more information.
+ */
+
+#include "gtest/gtest.h"
+#include <Swiften/Client/XMLBeautifier.h>
+#include <iostream>
+
+using namespace Swift;
+
+namespace {
+    const static std::string FULL_FORMATTED_OUTPUT("<list>\n <el>aqq</el>\n <el>bzz</el>\n</list>");
+}
+
+TEST(XMLBeautifierTest, testBeautify) {
+    auto beautifier = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
+
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, beautifier->beautify("<list><el>aqq</el><el>bzz</el></list>"));
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, beautifier->beautify("<list><el>aqq</el><el>bzz</el></list>"));
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+}
+
+TEST(XMLBeautifierTest, testBeautifyMultipleChunks) {
+    auto beautifier = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
+
+    auto result = beautifier->beautify("<list><el>aqq</el>");
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(1, beautifier->getLevel());
+
+    result += beautifier->beautify("<el>bzz</el></list>");
+    ASSERT_FALSE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, result);
+}
+
+TEST(XMLBeautifierTest, testBeautifyMultipleChunksMiddleElement) {
+    auto beautifier = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
+
+    auto result = beautifier->beautify("<l");
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+
+    result += beautifier->beautify("ist><el>aqq</el><el>bzz</el></list>");
+    ASSERT_FALSE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, result);
+}
+
+TEST(XMLBeautifierTest, testBeautifyInvalidMultipleChunks) {
+    auto beautifier = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
+
+    ASSERT_EQ(std::string("<list>\n <el>aqq"), beautifier->beautify("<list><el>aqq<"));
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(2, beautifier->getLevel());
+
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, beautifier->beautify("<list><el>aqq</el><el>bzz</el></list>"));
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+}
diff --git a/Swiften/Client/XMLBeautifier.cpp b/Swiften/Client/XMLBeautifier.cpp
index e2cd58e..97a228f 100644
--- a/Swiften/Client/XMLBeautifier.cpp
+++ b/Swiften/Client/XMLBeautifier.cpp
@@ -14,35 +14,53 @@
 
 #include <sstream>
 #include <stack>
+#include <iostream>
 
 #include <Swiften/Base/Log.h>
 #include <Swiften/Parser/PlatformXMLParserFactory.h>
 
 namespace Swift {
 
-XMLBeautifier::XMLBeautifier(bool indention, bool coloring) : doIndention(indention), doColoring(coloring), intLevel(0), parser(nullptr), lastWasStepDown(false) {
-    factory = new PlatformXMLParserFactory();
+XMLBeautifier::XMLBeautifier(bool indention, bool coloring) : doIndention_(indention), doColoring_(coloring), factory_(new PlatformXMLParserFactory()) {
 }
 
-XMLBeautifier::~XMLBeautifier() {
-    delete factory;
-}
 
 std::string XMLBeautifier::beautify(const std::string &text) {
-    parser = factory->createXMLParser(this);
-    intLevel = 0;
-    buffer.str(std::string());
-    parser->parse(text);
-    delete parser;
-    return buffer.str();
+    wasReset_ = false;
+    if (!parser_) {
+        reset();
+    }
+    buffer_.str(std::string());
+    if (!parser_->parse(text)) {
+        reset();
+        parser_->parse(text);
+    }
+    return buffer_.str();
+}
+
+bool XMLBeautifier::wasReset() const {
+    return wasReset_;
+}
+
+int XMLBeautifier::getLevel() const {
+    return intLevel_;
 }
 
 void XMLBeautifier::indent() {
-    for (int i = 0; i < intLevel; ++i) {
-        buffer << " ";
+    for (int i = 0; i < intLevel_; ++i) {
+        buffer_ << " ";
     }
 }
 
+void XMLBeautifier::reset() {
+    parser_ = factory_->createXMLParser(this);
+    intLevel_ = 0;
+    lastWasStepDown_ = false;
+    std::stack<std::string>().swap(parentNSs_);
+    buffer_.str(std::string());
+    wasReset_ = true;
+}
+
 // all bold but reset
 // static const char colorBlue[] = "\x1b[01;34m";
 static const char colorCyan[] = "\x1b[01;36m";
@@ -86,47 +104,47 @@ std::string XMLBeautifier::styleValue(const std::string& text) const {
 }
 
 void XMLBeautifier::handleStartElement(const std::string& element, const std::string& ns, const AttributeMap& attributes) {
-    if (doIndention) {
-        if (intLevel) buffer << std::endl;
+    if (doIndention_) {
+        if (intLevel_) buffer_ << std::endl;
     }
     indent();
-    buffer << "<" << (doColoring ? styleTag(element) : element);
-    if (!ns.empty() && (!parentNSs.empty() && parentNSs.top() != ns)) {
-        buffer << " ";
-        buffer << (doColoring ? styleAttribute("xmlns") : "xmlns");
-        buffer << "=";
-        buffer << "\"" << (doColoring ? styleNamespace(ns) : ns) << "\"";
+    buffer_ << "<" << (doColoring_ ? styleTag(element) : element);
+    if (!ns.empty() && (!parentNSs_.empty() && parentNSs_.top() != ns)) {
+        buffer_ << " ";
+        buffer_ << (doColoring_ ? styleAttribute("xmlns") : "xmlns");
+        buffer_ << "=";
+        buffer_ << "\"" << (doColoring_ ? styleNamespace(ns) : ns) << "\"";
     }
     if (!attributes.getEntries().empty()) {
         for (const auto& entry : attributes.getEntries()) {
-            buffer << " ";
-            buffer << (doColoring ? styleAttribute(entry.getAttribute().getName()) : entry.getAttribute().getName());
-            buffer << "=";
-            buffer << "\"" << (doColoring ? styleValue(entry.getValue()) : entry.getValue()) << "\"";
+            buffer_ << " ";
+            buffer_ << (doColoring_ ? styleAttribute(entry.getAttribute().getName()) : entry.getAttribute().getName());
+            buffer_ << "=";
+            buffer_ << "\"" << (doColoring_ ? styleValue(entry.getValue()) : entry.getValue()) << "\"";
         }
     }
-    buffer << ">";
-    ++intLevel;
-    lastWasStepDown = false;
-    parentNSs.push(ns);
+    buffer_ << ">";
+    ++intLevel_;
+    lastWasStepDown_ = false;
+    parentNSs_.push(ns);
 }
 
 void XMLBeautifier::handleEndElement(const std::string& element, const std::string& /* ns */) {
-    --intLevel;
-    parentNSs.pop();
-    if (/*hadCDATA.top() ||*/ lastWasStepDown) {
-        if (doIndention) {
-            buffer << std::endl;
+    --intLevel_;
+    parentNSs_.pop();
+    if (/*hadCDATA.top() ||*/ lastWasStepDown_) {
+        if (doIndention_) {
+            buffer_ << std::endl;
         }
         indent();
     }
-    buffer << "</" << (doColoring ? styleTag(element) : element) << ">";
-    lastWasStepDown = true;
+    buffer_ << "</" << (doColoring_ ? styleTag(element) : element) << ">";
+    lastWasStepDown_ = true;
 }
 
 void XMLBeautifier::handleCharacterData(const std::string& data) {
-    buffer << data;
-    lastWasStepDown = false;
+    buffer_ << data;
+    lastWasStepDown_ = false;
 }
 
 }
diff --git a/Swiften/Client/XMLBeautifier.h b/Swiften/Client/XMLBeautifier.h
index 8da935a..7458211 100644
--- a/Swiften/Client/XMLBeautifier.h
+++ b/Swiften/Client/XMLBeautifier.h
@@ -12,6 +12,7 @@
 
 #pragma once
 
+#include <memory>
 #include <sstream>
 #include <stack>
 #include <string>
@@ -27,10 +28,12 @@ namespace Swift {
 
 class SWIFTEN_API XMLBeautifier : public XMLParserClient {
 public:
+
     XMLBeautifier(bool indention, bool coloring);
-    virtual ~XMLBeautifier();
 
     std::string beautify(const std::string&);
+    bool wasReset() const;
+    int getLevel() const;
 
 private:
     void handleStartElement(const std::string& element, const std::string& ns, const AttributeMap& attributes);
@@ -39,6 +42,7 @@ private:
 
 private:
     void indent();
+    void reset();
 
 private:
     std::string styleTag(const std::string& text) const;
@@ -47,16 +51,16 @@ private:
     std::string styleValue(const std::string& text) const;
 
 private:
-    bool doIndention;
-    bool doColoring;
+    const bool doIndention_;
+    const bool doColoring_;
 
-    int intLevel;
-    std::string inputBuffer;
-    std::stringstream buffer;
-    XMLParserFactory* factory;
-    XMLParser* parser;
+    std::unique_ptr<XMLParserFactory> factory_;
+    std::unique_ptr<XMLParser> parser_;
 
-    bool lastWasStepDown;
-    std::stack<std::string> parentNSs;
+    bool wasReset_ = true;
+    int intLevel_ = 0;
+    bool lastWasStepDown_ = false;
+    std::stringstream buffer_;
+    std::stack<std::string> parentNSs_;
 };
 }
diff --git a/Swiften/Parser/BOSHBodyExtractor.cpp b/Swiften/Parser/BOSHBodyExtractor.cpp
index c45d338..803f16a 100644
--- a/Swiften/Parser/BOSHBodyExtractor.cpp
+++ b/Swiften/Parser/BOSHBodyExtractor.cpp
@@ -126,7 +126,7 @@ BOSHBodyExtractor::BOSHBodyExtractor(XMLParserFactory* parserFactory, const Byte
 
     // Parse the body element
     BOSHBodyParserClient parserClient(this);
-    std::shared_ptr<XMLParser> parser(parserFactory->createXMLParser(&parserClient));
+    std::shared_ptr<XMLParser> parser(std::move(parserFactory->createXMLParser(&parserClient)));
     if (!parser->parse(std::string(
             reinterpret_cast<const char*>(vecptr(data)),
             boost::numeric_cast<size_t>(std::distance(data.begin(), i))))) {
diff --git a/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h b/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h
index 2c1ff8e..dcdbffa 100644
--- a/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h
+++ b/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h
@@ -22,10 +22,6 @@ namespace Swift {
                 xmlParser = PlatformXMLParserFactory().createXMLParser(this);
             }
 
-            ~PayloadsParserTester() {
-                delete xmlParser;
-            }
-
             bool parse(const std::string& data) {
                 return xmlParser->parse(data);
             }
@@ -60,7 +56,7 @@ namespace Swift {
             }
 
         private:
-            XMLParser* xmlParser;
+            std::unique_ptr<XMLParser> xmlParser;
             FullPayloadParserFactoryCollection factories;
             std::shared_ptr<PayloadParser> payloadParser;
             int level;
diff --git a/Swiften/Parser/PlatformXMLParserFactory.cpp b/Swiften/Parser/PlatformXMLParserFactory.cpp
index 87f70d1..97e1c9e 100644
--- a/Swiften/Parser/PlatformXMLParserFactory.cpp
+++ b/Swiften/Parser/PlatformXMLParserFactory.cpp
@@ -20,11 +20,11 @@ namespace Swift {
 PlatformXMLParserFactory::PlatformXMLParserFactory() {
 }
 
-XMLParser* PlatformXMLParserFactory::createXMLParser(XMLParserClient* client) {
+std::unique_ptr<XMLParser> PlatformXMLParserFactory::createXMLParser(XMLParserClient* client) {
 #ifdef HAVE_LIBXML
-    return new LibXMLParser(client);
+    return std::unique_ptr<XMLParser>(new LibXMLParser(client));
 #else
-    return new ExpatParser(client);
+    return std::unique_ptr<XMLParser>(new ExpatParser(client));
 #endif
 }
 
diff --git a/Swiften/Parser/PlatformXMLParserFactory.h b/Swiften/Parser/PlatformXMLParserFactory.h
index 82b8573..fa3ca19 100644
--- a/Swiften/Parser/PlatformXMLParserFactory.h
+++ b/Swiften/Parser/PlatformXMLParserFactory.h
@@ -14,6 +14,6 @@ namespace Swift {
         public:
             PlatformXMLParserFactory();
 
-            virtual XMLParser* createXMLParser(XMLParserClient*);
+            virtual std::unique_ptr<XMLParser> createXMLParser(XMLParserClient*);
     };
 }
diff --git a/Swiften/Parser/UnitTest/ParserTester.h b/Swiften/Parser/UnitTest/ParserTester.h
index a98eb51..aa01d40 100644
--- a/Swiften/Parser/UnitTest/ParserTester.h
+++ b/Swiften/Parser/UnitTest/ParserTester.h
@@ -16,12 +16,7 @@ namespace Swift {
     template<typename ParserType>
     class ParserTester : public XMLParserClient {
         public:
-            ParserTester(ParserType* parser) : parser_(parser) {
-                xmlParser_ = PlatformXMLParserFactory().createXMLParser(this);
-            }
-
-            ~ParserTester() {
-                delete xmlParser_;
+            ParserTester(ParserType* parser) : xmlParser_(PlatformXMLParserFactory().createXMLParser(this)), parser_(parser) {
             }
 
             bool parse(const std::string& data) {
@@ -41,7 +36,7 @@ namespace Swift {
             }
 
         private:
-            XMLParser* xmlParser_;
+            std::unique_ptr<XMLParser> xmlParser_;
             ParserType* parser_;
     };
 }
diff --git a/Swiften/Parser/XMLParserFactory.h b/Swiften/Parser/XMLParserFactory.h
index 091f45b..595512b 100644
--- a/Swiften/Parser/XMLParserFactory.h
+++ b/Swiften/Parser/XMLParserFactory.h
@@ -6,6 +6,8 @@
 
 #pragma once
 
+#include <memory>
+
 #include <Swiften/Base/API.h>
 
 namespace Swift {
@@ -16,6 +18,6 @@ namespace Swift {
         public:
             virtual ~XMLParserFactory();
 
-            virtual XMLParser* createXMLParser(XMLParserClient*) = 0;
+            virtual std::unique_ptr<XMLParser> createXMLParser(XMLParserClient*) = 0;
     };
 }
diff --git a/Swiften/Parser/XMPPParser.cpp b/Swiften/Parser/XMPPParser.cpp
index 2b45a12..d2a5a98 100644
--- a/Swiften/Parser/XMPPParser.cpp
+++ b/Swiften/Parser/XMPPParser.cpp
@@ -60,7 +60,6 @@ XMPPParser::XMPPParser(
 
 XMPPParser::~XMPPParser() {
     delete currentElementParser_;
-    delete xmlParser_;
 }
 
 bool XMPPParser::parse(const std::string& data) {
diff --git a/Swiften/Parser/XMPPParser.h b/Swiften/Parser/XMPPParser.h
index 09fae38..6595b94 100644
--- a/Swiften/Parser/XMPPParser.h
+++ b/Swiften/Parser/XMPPParser.h
@@ -42,7 +42,7 @@ namespace Swift {
             ElementParser* createElementParser(const std::string& element, const std::string& xmlns);
 
         private:
-            XMLParser* xmlParser_;
+            std::unique_ptr<XMLParser> xmlParser_;
             XMPPParserClient* client_;
             PayloadParserFactoryCollection* payloadParserFactories_;
             enum Level {
diff --git a/Swiften/SConscript b/Swiften/SConscript
index 2182132..c77d80f 100644
--- a/Swiften/SConscript
+++ b/Swiften/SConscript
@@ -382,6 +382,7 @@ if env["SCONS_STAGE"] == "build" :
             File("Client/UnitTest/NickResolverTest.cpp"),
             File("Client/UnitTest/ClientBlockListManagerTest.cpp"),
             File("Client/UnitTest/BlockListImplTest.cpp"),
+            File("Client/UnitTest/XMLBeautifierTest.cpp"),
             File("Compress/UnitTest/ZLibCompressorTest.cpp"),
             File("Compress/UnitTest/ZLibDecompressorTest.cpp"),
             File("Component/UnitTest/ComponentHandshakeGeneratorTest.cpp"),
-- 
cgit v0.10.2-6-g49f6