Fix for crash in XMLRPC reply decoding on login with large inventories

Commit 2ea5ac0c43e3e28d2b1774f5367d099271a1da32 introduced a crash bug
due to the recursive construction of the XMLTreeNode wrapper class.
The constructor of the said class typically recurses twice as many times
as there are entries in the user's inventory list.

This commit:
- Moves the fromXMLRPCValue() method and its helper functions from the LLSD
  class/module to the LLXMLNode class, where it belongs, thus making
  LLSD::TreeNode (which was a wrapper class to avoid making llcommon
  dependant on llxml, which is still the case after this commit) totally
  moot; the fromXMLRPCValue() call is now done directly on the LLXMLNode.
- Moves the XML and XMLRPC decoding code out of the HTTP coroutine
  LLXMLRPCTransaction::Handler (coroutines got an even smaller and fixed
  stack), and into LLXMLRPCTransaction::Impl::process().
- Removes XMLTreeNode entirely, fixing the crash as a result.

5 files changed, 269 insertions, 252 deletions

diff --git a/indra/llcommon/llsd.cpp b/indra/llcommon/llsd.cpp
index 2bbe06e72f..77fe545c3f 100644
--- a/indra/llcommon/llsd.cpp
+++ b/indra/llcommon/llsd.cpp
@@ -1018,153 +1018,6 @@ const LLSD::String& LLSD::asStringRef() const { return safe(impl).asStringRef();
 
 LLSD::String LLSD::asXMLRPCValue() const { return "<value>" + safe(impl).asXMLRPCValue() + "</value>"; }
 
-static bool inline check(bool condition, const char* warning_message)
-{
-    if (!condition)
-    {
-        LL_WARNS() << warning_message << LL_ENDL;
-    }
-
-    return condition;
-}
-
-static bool parseXMLRPCArrayValue(LLSD& target, LLSD::TreeNode* node)
-{
-    LLSD::TreeNode* data = node->getFirstChild();
-    if (!check(data, "No array inner XML element (<data> expected)") ||
-        !check(data->hasName("data"), "Invalid array inner XML element (<data> expected)") ||
-        !check(!data->getNextSibling(), "Multiple array inner XML elements (single <data> expected)"))
-        return false;
-
-    for (LLSD::TreeNode* item = data->getFirstChild(); item; item = item->getNextSibling())
-    {
-        LLSD value;
-        if (!value.fromXMLRPCValue(item))
-            return false;
-
-        target.append(value);
-    }
-
-    return true;
-}
-
-static bool parseXMLRPCStructValue(LLSD& target, LLSD::TreeNode* node)
-{
-    for (LLSD::TreeNode* item = node->getFirstChild(); item; item = item->getNextSibling())
-    {
-        if (!check(item->hasName("member"), "Invalid struct inner XML element (<member> expected)"))
-            return false;
-
-        std::string name;
-        LLSD value;
-        for (LLSD::TreeNode* subitem = item->getFirstChild(); subitem; subitem = subitem->getNextSibling())
-        {
-            if (subitem->hasName("name"))
-            {
-                name = LLStringFn::xml_decode(subitem->getTextContents());
-            }
-            else if (!value.fromXMLRPCValue(subitem))
-            {
-                return false;
-            }
-        }
-        if (!check(!name.empty(), "Empty struct member name"))
-            return false;
-
-        target.insert(name, value);
-    }
-
-    return true;
-}
-
-bool LLSD::fromXMLRPCValue(TreeNode* node)
-{
-    clear();
-
-    llassert(node);
-    if (!node)
-        return false;
-
-    if (!check(node->hasName("value"), "Invalid XML element (<value> expected)"))
-        return false;
-
-    TreeNode* inner = node->getFirstChild();
-    if (!inner)
-    {
-        check(false, "No inner XML element (value type expected)");
-        // Value with no type qualifier is treated as string
-        assign(LLStringFn::xml_decode(node->getTextContents()));
-        return true;
-    }
-
-    if (!check(!inner->getNextSibling(), "Multiple inner XML elements (single expected)"))
-        return false;
-
-    if (inner->hasName("string"))
-    {
-        assign(LLStringFn::xml_decode(inner->getTextContents()));
-        return true;
-    }
-
-    if (inner->hasName("int") || inner->hasName("i4"))
-    {
-        assign(std::stoi(inner->getTextContents()));
-        return true;
-    }
-
-    if (inner->hasName("double"))
-    {
-        assign(std::stod(inner->getTextContents()));
-        return true;
-    }
-
-    if (inner->hasName("boolean"))
-    {
-        assign(!!std::stoi(inner->getTextContents()));
-        return true;
-    }
-
-    if (inner->hasName("dateTime.iso8601"))
-    {
-        assign(Date(inner->getTextContents()));
-        return true;
-    }
-
-    if (inner->hasName("base64"))
-    {
-        std::string decoded = LLBase64::decodeAsString(inner->getTextContents());
-        Binary binary(decoded.size());
-        memcpy(binary.data(), decoded.data(), decoded.size());
-        assign(binary);
-        return true;
-    }
-
-    if (inner->hasName("array"))
-    {
-        if (!parseXMLRPCArrayValue(*this, inner))
-        {
-            clear();
-            return false;
-        }
-        return true;
-    }
-
-    if (inner->hasName("struct"))
-    {
-        if (!parseXMLRPCStructValue(*this, inner))
-        {
-            clear();
-            return false;
-        }
-        return true;
-    }
-
-    check(false, "Unknown inner XML element (known value type expected)");
-    // Value with unknown type qualifier is treated as string
-    assign(LLStringFn::xml_decode(inner->getTextContents()));
-    return true;
-}
-
 // const char * helpers
 LLSD::LLSD(const char* v) : impl(0)     { ALLOC_LLSD_OBJECT;    assign(v); }
 void LLSD::assign(const char* v)
diff --git a/indra/llcommon/llsd.h b/indra/llcommon/llsd.h
index 781e8d58e9..d2b3548831 100644
--- a/indra/llcommon/llsd.h
+++ b/indra/llcommon/llsd.h
@@ -290,16 +290,6 @@ public:
         // See http://xmlrpc.com/spec.md
         String asXMLRPCValue() const;
 
-        struct TreeNode
-        {
-            virtual bool hasName(const String& name) const = 0;
-            virtual String getTextContents() const = 0;
-            virtual TreeNode* getFirstChild() const = 0;
-            virtual TreeNode* getNextSibling() const = 0;
-        };
-
-        bool fromXMLRPCValue(TreeNode* node);
-
         operator Boolean() const    { return asBoolean(); }
         operator Integer() const    { return asInteger(); }
         operator Real() const       { return asReal(); }
diff --git a/indra/llxml/llxmlnode.cpp b/indra/llxml/llxmlnode.cpp
index 7f6b8093fc..8eb4556828 100644
--- a/indra/llxml/llxmlnode.cpp
+++ b/indra/llxml/llxmlnode.cpp
@@ -38,7 +38,9 @@
 #include "v3math.h"
 #include "v3dmath.h"
 #include "v4math.h"
+#include "llbase64.h"
 #include "llquaternion.h"
+#include "llsd.h"
 #include "llstring.h"
 #include "lluuid.h"
 #include "lldir.h"
@@ -3263,3 +3265,173 @@ S32 LLXMLNode::getLineNumber()
 {
     return mLineNumber;
 }
+
+bool LLXMLNode::parseXmlRpcArrayValue(LLSD& target)
+{
+    LLXMLNode* datap = getFirstChild().get();
+    if (!datap)
+    {
+        LL_WARNS() << "No inner XML element." << LL_ENDL;
+        return false;
+    }
+    if (!datap->hasName("data"))
+    {
+        LL_WARNS() << "No inner XML element (<data> expected, got: "
+                   << datap->mName->mString << ")" << LL_ENDL;
+        return false;
+    }
+    if (datap->getNextSibling().get())
+    {
+        LL_WARNS() << "Multiple inner XML elements (single <data> expected)"
+                   << LL_ENDL;
+        return false;
+    }
+    U32 i = 0;
+    for (LLXMLNode* itemp = datap->getFirstChild().get(); itemp;
+         itemp = itemp->getNextSibling().get())
+    {
+        LLSD value;
+        if (!itemp->fromXMLRPCValue(value))
+        {
+            return false;
+        }
+        target.append(value);
+        ++i;
+    }
+    return true;
+}
+
+bool LLXMLNode::parseXmlRpcStructValue(LLSD& target)
+{
+    std::string name;
+    LLSD value;
+    for (LLXMLNode* itemp = getFirstChild().get(); itemp;
+         itemp = itemp->getNextSibling().get())
+    {
+        if (!itemp->hasName("member"))
+        {
+            LL_WARNS() << "Invalid inner XML element (<member> expected, got: <"
+                       << itemp->mName->mString << ">" << LL_ENDL;
+            return false;
+        }
+        name.clear();
+        value.clear();
+        for (LLXMLNode* chilp = itemp->getFirstChild().get(); chilp;
+             chilp = chilp->getNextSibling().get())
+        {
+            if (chilp->hasName("name"))
+            {
+                name = LLStringFn::xml_decode(chilp->getTextContents());
+            }
+            else if (!chilp->fromXMLRPCValue(value))
+            {
+                return false;
+            }
+        }
+        if (name.empty())
+        {
+            LL_WARNS() << "Empty struct member name" << LL_ENDL;
+            return false;
+        }
+        target.insert(name, value);
+    }
+    return true;
+}
+
+bool LLXMLNode::fromXMLRPCValue(LLSD& target)
+{
+    target.clear();
+
+    if (!hasName("value"))
+    {
+        LL_WARNS() << "Invalid XML element (<value> expected), got: <"
+                   << mName->mString << ">" << LL_ENDL;
+        return false;
+    }
+
+    LLXMLNode* childp = getFirstChild().get();
+    if (!childp)
+    {
+        LL_WARNS() << "No inner XML element (value type expected)" << LL_ENDL;
+        // Value with no type qualifier is treated as string
+        target.assign(LLStringFn::xml_decode(getTextContents()));
+        return true;
+    }
+
+    if (childp->getNextSibling())
+    {
+        LL_WARNS() << "Multiple inner XML elements (single expected)"
+                   << LL_ENDL;
+        return false;
+    }
+
+    if (childp->hasName("string"))
+    {
+        target.assign(LLStringFn::xml_decode(childp->getTextContents()));
+        return true;
+    }
+
+    if (childp->hasName("int") || childp->hasName("i4"))
+    {
+        target.assign(std::stoi(childp->getTextContents()));
+        return true;
+    }
+
+    if (childp->hasName("double"))
+    {
+        target.assign(std::stod(childp->getTextContents()));
+        return true;
+    }
+
+    if (childp->hasName("boolean"))
+    {
+        target.assign(std::stoi(childp->getTextContents()) != 0);
+        return true;
+    }
+
+    if (childp->hasName("dateTime.iso8601"))
+    {
+        target.assign(LLSD::Date(childp->getTextContents()));
+        return true;
+    }
+
+    if (childp->hasName("base64"))
+    {
+        std::string decoded =
+            LLBase64::decodeAsString(inner->getTextContents());
+        size_t size = decoded.size();
+        LLSD::Binary binary(size);
+        if (size)
+        {
+            memcpy((void*)binary.data(), (void*)decoded.data(), size);
+        }
+        target.assign(binary);
+        return true;
+    }
+
+    if (childp->hasName("array"))
+    {
+        if (!childp->parseXmlRpcArrayValue(target))
+        {
+            target.clear();
+            return false;
+        }
+        return true;
+    }
+
+    if (childp->hasName("struct"))
+    {
+        if (!childp->parseXmlRpcStructValue(target))
+        {
+            target.clear();
+            return false;
+        }
+        return true;
+    }
+
+    LL_WARNS() << "Unknown inner XML element (known value type expected)"
+               << LL_ENDL;
+    // Value with unknown type qualifier is treated as string
+    target.assign(LLStringFn::xml_decode(childp->getTextContents()));
+    return true;
+}
diff --git a/indra/llxml/llxmlnode.h b/indra/llxml/llxmlnode.h
index b8f9e1ff69..3769ec8293 100644
--- a/indra/llxml/llxmlnode.h
+++ b/indra/llxml/llxmlnode.h
@@ -50,6 +50,7 @@ class LLVector3d;
 class LLQuaternion;
 class LLColor4;
 class LLColor4U;
+class LLSD;
 
 
 struct CompareAttributes
@@ -284,12 +285,18 @@ public:
     void setAttributes(ValueType type, U32 precision, Encoding encoding, U32 length);
 //  void appendValue(const std::string& value); // Unused
 
+    bool fromXMLRPCValue(LLSD& target);
+
     // Unit Testing
     void createUnitTest(S32 max_num_children);
     bool performUnitTest(std::string &error_buffer);
 
 protected:
     bool removeChild(LLXMLNode* child);
+    bool isFullyDefault();
+
+    bool parseXmlRpcArrayValue(LLSD& target);
+    bool parseXmlRpcStructValue(LLSD& target);
 
 public:
     std::string mID;                // The ID attribute of this node
@@ -328,8 +335,6 @@ protected:
     static const char *skipNonWhitespace(const char *str);
     static const char *parseInteger(const char *str, U64 *dest, bool *is_negative, U32 precision, Encoding encoding);
     static const char *parseFloat(const char *str, F64 *dest, U32 precision, Encoding encoding);
-
-    bool isFullyDefault();
 };
 
 #endif // LL_LLXMLNODE
diff --git a/indra/newview/llxmlrpctransaction.cpp b/indra/newview/llxmlrpctransaction.cpp
index 55622fb6b7..9dedb685db 100644
--- a/indra/newview/llxmlrpctransaction.cpp
+++ b/indra/newview/llxmlrpctransaction.cpp
@@ -68,17 +68,13 @@ class LLXMLRPCTransaction::Handler : public LLCore::HttpHandler
 {
 public:
     Handler(LLCore::HttpRequest::ptr_t &request, LLXMLRPCTransaction::Impl *impl);
-    virtual ~Handler();
 
-    virtual void onCompleted(LLCore::HttpHandle handle, LLCore::HttpResponse * response);
+    void onCompleted(LLCore::HttpHandle handle,
+                     LLCore::HttpResponse* response) override;
 
     typedef std::shared_ptr<LLXMLRPCTransaction::Handler> ptr_t;
 
 private:
-
-    bool parseResponse(LLXMLNodePtr root);
-    bool parseValue(LLSD& target, LLXMLNodePtr source);
-
     LLXMLRPCTransaction::Impl *mImpl;
     LLCore::HttpRequest::ptr_t mRequest;
 };
@@ -104,6 +100,8 @@ public:
 
     std::string         mResponseText;
     LLSD                mResponseData;
+	bool                mHasResponse;
+	bool                mResponseParsed;
 
     std::string         mCertStore;
     LLSD                mErrorCertData;
@@ -120,6 +118,10 @@ public:
 
     void setStatus(EStatus code, const std::string& message = "", const std::string& uri = "");
     void setHttpStatus(const LLCore::HttpStatus &status);
+
+private:
+    bool parseResponse(LLXMLNodePtr root);
+    bool parseValue(LLSD& target, LLXMLNodePtr source);
 };
 
 LLXMLRPCTransaction::Handler::Handler(LLCore::HttpRequest::ptr_t &request,
@@ -129,10 +131,6 @@ LLXMLRPCTransaction::Handler::Handler(LLCore::HttpRequest::ptr_t &request,
 {
 }
 
-LLXMLRPCTransaction::Handler::~Handler()
-{
-}
-
 void LLXMLRPCTransaction::Handler::onCompleted(LLCore::HttpHandle handle,
     LLCore::HttpResponse * response)
 {
@@ -159,7 +157,6 @@ void LLXMLRPCTransaction::Handler::onCompleted(LLCore::HttpHandle handle,
         return;
     }
 
-    mImpl->setStatus(LLXMLRPCTransaction::StatusComplete);
     mImpl->mTransferStats = response->getTransferStats();
 
     // The contents of a buffer array are potentially noncontiguous, so we
@@ -169,88 +166,12 @@ void LLXMLRPCTransaction::Handler::onCompleted(LLCore::HttpHandle handle,
 
     body->read(0, mImpl->mResponseText.data(), body->size());
 
-    LLXMLNodePtr root;
-    if (!LLXMLNode::parseBuffer(mImpl->mResponseText.data(), body->size(), root, nullptr))
-    {
-        LL_WARNS() << "Failed parsing XML response; request URI: " << mImpl->mURI << LL_ENDL;
-        return;
-    }
-
-    if (!parseResponse(root))
-        return;
-
-    LL_INFOS() << "XML response parsed successfully; request URI: " << mImpl->mURI << LL_ENDL;
-}
-
-struct XMLTreeNode final : public LLSD::TreeNode
-{
-    XMLTreeNode(const LLXMLNodePtr impl)
-        : mImpl(impl)
-        , mFirstChild(impl ? create(impl->getFirstChild()) : nullptr)
-        , mNextSibling(impl ? create(impl->getNextSibling()) : nullptr)
-    {
-    }
-
-    static XMLTreeNode* create(LLXMLNodePtr node) { return node ? new XMLTreeNode(node) : nullptr; }
-
-    virtual bool hasName(const LLSD::String& name) const override { return mImpl && mImpl->hasName(name); }
-    virtual LLSD::String getTextContents() const override { return mImpl ? mImpl->getTextContents() : LLStringUtil::null; }
-    virtual TreeNode* getFirstChild() const override { return mFirstChild.get(); }
-    virtual TreeNode* getNextSibling() const override { return mNextSibling.get(); }
-
-private:
-    const LLXMLNodePtr mImpl;
-    const std::shared_ptr<XMLTreeNode> mFirstChild;
-    const std::shared_ptr<XMLTreeNode> mNextSibling;
-};
-
-bool LLXMLRPCTransaction::Handler::parseResponse(LLXMLNodePtr root)
-{
-    // We have alreasy checked in LLXMLNode::parseBuffer()
-    // that root contains exactly one child
-    if (!root->hasName("methodResponse"))
-    {
-        LL_WARNS() << "Invalid root element in XML response; request URI: " << mImpl->mURI << LL_ENDL;
-        return false;
-    }
-
-    LLXMLNodePtr first = root->getFirstChild();
-    LLXMLNodePtr second = first->getFirstChild();
-    if (!first->getNextSibling() && second && !second->getNextSibling())
-    {
-        if (first->hasName("fault"))
-        {
-            LLSD fault;
-            if (parseValue(fault, second) &&
-                fault.isMap() && fault.has("faultCode") && fault.has("faultString"))
-            {
-                LL_WARNS() << "Request failed;"
-                    << " faultCode: '" << fault.get("faultCode").asString() << "',"
-                    << " faultString: '" << fault.get("faultString").asString() << "',"
-                    << " request URI: " << mImpl->mURI << LL_ENDL;
-                return false;
-            }
-        }
-        else if (first->hasName("params") &&
-            second->hasName("param") && !second->getNextSibling())
-        {
-            LLXMLNodePtr third = second->getFirstChild();
-            if (third && !third->getNextSibling() && parseValue(mImpl->mResponseData, third))
-            {
-                return true;
-            }
-        }
-    }
-
-    LL_WARNS() << "Invalid response format; request URI: " << mImpl->mURI << LL_ENDL;
-
-    return false;
-}
-
-bool LLXMLRPCTransaction::Handler::parseValue(LLSD& target, LLXMLNodePtr source)
-{
-    XMLTreeNode tn(source);
-    return target.fromXMLRPCValue(&tn);
+    // We do not do the parsing in the HTTP coroutine, since it could exhaust
+    // the coroutine stack in extreme cases. Instead, we flag the data buffer
+    // as ready, and let mImpl decode it in its process() method, on the main
+    // coroutine. HB
+    mImpl->mHasResponse = true;
+    mImpl->setStatus(LLXMLRPCTransaction::StatusComplete);
 }
 
 //=========================================================================
@@ -265,6 +186,8 @@ LLXMLRPCTransaction::Impl::Impl
     : mHttpRequest()
     , mStatus(LLXMLRPCTransaction::StatusNotStarted)
     , mURI(uri)
+    , mHasResponse(false)
+    , mResponseParsed(false)
 {
     LLCore::HttpOptions::ptr_t httpOpts;
     LLCore::HttpHeaders::ptr_t httpHeaders;
@@ -327,6 +250,57 @@ LLXMLRPCTransaction::Impl::Impl
         mURI, body.get(), httpOpts, httpHeaders, mHandler);
 }
 
+bool LLXMLRPCTransaction::Impl::parseResponse(LLXMLNodePtr root)
+{
+    // We have already checked in LLXMLNode::parseBuffer() that root contains
+    // exactly one child.
+    if (!root->hasName("methodResponse"))
+    {
+        LL_WARNS() << "Invalid root element in XML response; request URI: "
+                   << mURI << LL_ENDL;
+        return false;
+    }
+
+    LLXMLNodePtr first = root->getFirstChild();
+    LLXMLNodePtr second = first->getFirstChild();
+    if (first && !first->getNextSibling() && second &&
+        !second->getNextSibling())
+    {
+        if (first->hasName("fault"))
+        {
+            LLSD fault;
+            if (parseValue(fault, second) && fault.isMap() &&
+                fault.has("faultCode") && fault.has("faultString"))
+            {
+                LL_WARNS() << "Request failed. faultCode: '"
+                        << fault.get("faultCode").asString()
+                        << "', faultString: '"
+                        << fault.get("faultString").asString()
+                        << "', request URI: " << mURI << LL_ENDL;
+                return false;
+            }
+        }
+        else if (first->hasName("params") &&
+                 second->hasName("param") && !second->getNextSibling())
+        {
+            LLXMLNodePtr third = second->getFirstChild();
+            if (third && !third->getNextSibling() &&
+                parseValue(mResponseData, third))
+            {
+                return true;
+            }
+        }
+    }
+
+    LL_WARNS() << "Invalid response format; request URI: " << mURI << LL_ENDL;
+    return false;
+}
+
+bool LLXMLRPCTransaction::Impl::parseValue(LLSD& target, LLXMLNodePtr src)
+{
+    return src->fromXMLRPCValue(target);
+}
+
 bool LLXMLRPCTransaction::Impl::process()
 {
     if (!mPostH || !mHttpRequest)
@@ -335,6 +309,29 @@ bool LLXMLRPCTransaction::Impl::process()
         return true; //failed, quit.
     }
 
+    // Parse the response when we have one and it has not yet been parsed. HB
+    if (mHasResponse && !mResponseParsed)
+    {
+        LLXMLNodePtr root;
+        if (!LLXMLNode::parseBuffer(mResponseText.data(), mResponseText.size(),
+                                    root, nullptr))
+        {
+ 	        LL_WARNS() << "Failed parsing XML in response; request URI: "
+                       << mURI << LL_ENDL;
+        }
+        else if (parseResponse(root))
+        {
+            LL_INFOS() << "XMLRPC response parsed successfully; request URI: "
+                       << mURI << LL_ENDL;
+        }
+        else
+        {
+            LL_WARNS() << "XMLRPC response parsing failed; request URI: "
+                       << mURI << LL_ENDL;
+        }
+        mResponseParsed = true;
+    }
+
     switch (mStatus)
     {
         case LLXMLRPCTransaction::StatusComplete: