Merge pull request #1949 from sldevel/xmlrpc-crash-fix

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

5 files changed, 267 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..d6f9a56c9d 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,171 @@ 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;
+    }
+    for (LLXMLNode* itemp = datap->getFirstChild().get(); itemp;
+         itemp = itemp->getNextSibling().get())
+    {
+        LLSD value;
+        if (!itemp->fromXMLRPCValue(value))
+        {
+            return false;
+        }
+        target.append(value);
+    }
+    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 a415e8983d..ba48b58f3b 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: