1 files changed, 127 insertions, 5 deletions

diff --git a/indra/llmessage/message.h b/indra/llmessage/message.h
index 0af5a1b96d..52dbf871db 100644
--- a/indra/llmessage/message.h
+++ b/indra/llmessage/message.h
@@ -61,6 +61,8 @@
 #include "llstoredmessage.h"
 #include "boost/function.hpp"
 #include "llpounceable.h"
+#include "llcoros.h"
+#include LLCOROS_MUTEX_HEADER
 
 const U32 MESSAGE_MAX_STRINGS_LENGTH = 64;
 const U32 MESSAGE_NUMBER_OF_HASH_BUCKETS = 8192;
@@ -199,6 +201,91 @@ public:
 	virtual void complete(const LLHost& host, const LLUUID& agent) const = 0;
 };
 
+/**
+ * SL-12204: We've observed crashes when consumer code sets
+ * LLMessageSystem::mMessageReader, assuming that all subsequent processing of
+ * the current message will use the same mMessageReader value -- only to have
+ * a different coroutine sneak in and replace mMessageReader before
+ * completion. This is a limitation of sharing a stateful global resource for
+ * message parsing; instead code receiving a new message should instantiate a
+ * (trivially constructed) local message parser and use that.
+ *
+ * Until then, when one coroutine sets a particular LLMessageReader subclass
+ * as the current message reader, ensure that no other coroutine can replace
+ * it until the first coroutine has finished with its message.
+ *
+ * This is achieved with two helper classes. LLMessageSystem::mMessageReader
+ * is now an LLMessageReaderPointer instance, which can efficiently compare or
+ * dereference its contained LLMessageReader* but which cannot be directly
+ * assigned. To change the value of LLMessageReaderPointer, you must
+ * instantiate LockMessageReader with the LLMessageReader* you wish to make
+ * current. mMessageReader will have that value for the lifetime of the
+ * LockMessageReader instance, then revert to nullptr. Moreover, as its name
+ * implies, LockMessageReader locks the mutex in LLMessageReaderPointer so
+ * that any other coroutine instantiating LockMessageReader will block until
+ * the first coroutine has destroyed its instance.
+ */
+class LLMessageReaderPointer
+{
+public:
+    LLMessageReaderPointer(): mPtr(nullptr) {}
+    // It is essential that comparison and dereferencing must be fast, which
+    // is why we don't check for nullptr when dereferencing.
+    LLMessageReader* operator->() const { return mPtr; }
+    bool operator==(const LLMessageReader* other) const { return mPtr == other; }
+    bool operator!=(const LLMessageReader* other) const { return ! (*this == other); }
+private:
+    // Only LockMessageReader can set mPtr.
+    friend class LockMessageReader;
+    LLMessageReader* mPtr;
+    LLCoros::Mutex mMutex;
+};
+
+/**
+ * To set mMessageReader to nullptr:
+ *
+ * @code
+ * // use an anonymous instance that is destroyed immediately
+ * LockMessageReader(gMessageSystem->mMessageReader, nullptr);
+ * @endcode
+ *
+ * Why do we still require going through LockMessageReader at all? Because it
+ * would be Bad if any coroutine set mMessageReader to nullptr while another
+ * coroutine was still parsing a message.
+ */
+class LockMessageReader
+{
+public:
+    LockMessageReader(LLMessageReaderPointer& var, LLMessageReader* instance):
+        mVar(var.mPtr),
+        mLock(var.mMutex)
+    {
+        mVar = instance;
+    }
+    // Some compilers reportedly fail to suppress generating implicit copy
+    // operations even though we have a move-only LockType data member.
+    LockMessageReader(const LockMessageReader&) = delete;
+    LockMessageReader& operator=(const LockMessageReader&) = delete;
+    ~LockMessageReader()
+    {
+        mVar = nullptr;
+    }
+private:
+    // capture a reference to LLMessageReaderPointer::mPtr
+    decltype(LLMessageReaderPointer::mPtr)& mVar;
+    // while holding a lock on LLMessageReaderPointer::mMutex
+    LLCoros::LockType mLock;
+};
+
+/**
+ * LockMessageReader is great as long as you only need mMessageReader locked
+ * during a single LLMessageSystem function call. However, empirically the
+ * sequence from checkAllMessages() through processAcks() need mMessageReader
+ * locked to LLTemplateMessageReader. Enforce that by making them require an
+ * instance of LockMessageChecker.
+ */
+class LockMessageChecker;
+
 class LLMessageSystem : public LLMessageSenderInterface
 {
  private:
@@ -331,8 +418,8 @@ public:
 	bool addCircuitCode(U32 code, const LLUUID& session_id);
 
 	BOOL	poll(F32 seconds); // Number of seconds that we want to block waiting for data, returns if data was received
-	BOOL	checkMessages( S64 frame_count = 0 );
-	void	processAcks(F32 collect_time = 0.f);
+	BOOL	checkMessages(LockMessageChecker&, S64 frame_count = 0 );
+	void	processAcks(LockMessageChecker&, F32 collect_time = 0.f);
 
 	BOOL	isMessageFast(const char *msg);
 	BOOL	isMessage(const char *msg)
@@ -730,7 +817,7 @@ public:
 		const LLSD& data);
 
 	// Check UDP messages and pump http_pump to receive HTTP messages.
-	bool checkAllMessages(S64 frame_count, LLPumpIO* http_pump);
+	bool checkAllMessages(LockMessageChecker&, S64 frame_count, LLPumpIO* http_pump);
 
 	// Moved to allow access from LLTemplateMessageDispatcher
 	void clearReceiveState();
@@ -817,12 +904,13 @@ private:
 	LLMessageBuilder* mMessageBuilder;
 	LLTemplateMessageBuilder* mTemplateMessageBuilder;
 	LLSDMessageBuilder* mLLSDMessageBuilder;
-	LLMessageReader* mMessageReader;
+	LLMessageReaderPointer mMessageReader;
 	LLTemplateMessageReader* mTemplateMessageReader;
 	LLSDMessageReader* mLLSDMessageReader;
 
 	friend class LLMessageHandlerBridge;
-	
+	friend class LockMessageChecker;
+
 	bool callHandler(const char *name, bool trustedSource,
 					 LLMessageSystem* msg);
 
@@ -835,6 +923,40 @@ private:
 // external hook into messaging system
 extern LLPounceable<LLMessageSystem*, LLPounceableStatic> gMessageSystem;
 
+// Implementation of LockMessageChecker depends on definition of
+// LLMessageSystem, hence must follow it.
+class LockMessageChecker: public LockMessageReader
+{
+public:
+    LockMessageChecker(LLMessageSystem* msgsystem);
+
+    // For convenience, provide forwarding wrappers so you can call (e.g.)
+    // checkAllMessages() on your LockMessageChecker instance instead of
+    // passing the instance to LLMessageSystem::checkAllMessages(). Use
+    // perfect forwarding to avoid having to maintain these wrappers in sync
+    // with the target methods.
+    template <typename... ARGS>
+    bool checkAllMessages(ARGS&&... args)
+    {
+        return mMessageSystem->checkAllMessages(*this, std::forward<ARGS>(args)...);
+    }
+
+    template <typename... ARGS>
+    bool checkMessages(ARGS&&... args)
+    {
+        return mMessageSystem->checkMessages(*this, std::forward<ARGS>(args)...);
+    }
+
+    template <typename... ARGS>
+    void processAcks(ARGS&&... args)
+    {
+        return mMessageSystem->processAcks(*this, std::forward<ARGS>(args)...);
+    }
+
+private:
+    LLMessageSystem* mMessageSystem;
+};
+
 // Must specific overall system version, which is used to determine
 // if a patch is available in the message template checksum verification.
 // Return true if able to initialize system.