DRTVWR-476, SL-12204: Fix crash in Marketplace Listings.

The observed crash was due to sharing a stateful global resource (the global
LLMessageSystem instance) between different tasks. Specifically, a coroutine
sets its mMessageReader one way, expecting that value to persist until it's
done with message parsing, but another coroutine sneaks in at a suspension
point and sets it differently.

Introduce LockMessageReader and LockMessageChecker classes, which must be
instantiated by a consumer of the resource. The constructor of each locks a
coroutine-aware mutex, so that for the lifetime of the lock object no other
coroutine can instantiate another.

Refactor the code so that LLMessageSystem::mMessageReader can only be modified
by LockMessageReader, not by direct assignment. mMessageReader is now an
instance of LLMessageReaderPointer, which supports dereferencing and
comparison but not assignment. Only LockMessageReader can change its value.

LockMessageReader addresses the use case in which the specific mMessageReader
value need only persist for the duration of a single method call. Add an
instance in LLMessageHandlerBridge::post().

LockMessageChecker is a subclass of LockMessageReader: both lock the same
mutex. LockMessageChecker addresses the use case in which the specific
mMessageReader value must persist across multiple method calls. Modify the
methods in question to require a LockMessageChecker instance. Provide
LockMessageChecker forwarding methods to facilitate calling the underlying
LLMessageSystem methods via the LockMessageChecker instance.

Add LockMessageChecker instances to LLAppViewer::idleNetwork(), a couple cases
in idle_startup() and LLMessageSystem::establishBidirectionalTrust().

1 files changed, 25 insertions, 16 deletions

diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp
index 6ef4025ab1..da62bb12e8 100644
--- a/indra/llmessage/message.cpp
+++ b/indra/llmessage/message.cpp
@@ -117,8 +117,8 @@ void LLMessageHandlerBridge::post(LLHTTPNode::ResponsePtr response,
 	gMessageSystem->mLastSender = LLHost(input["sender"].asString());
 	gMessageSystem->mPacketsIn += 1;
 	gMessageSystem->mLLSDMessageReader->setMessage(namePtr, input["body"]);
-	gMessageSystem->mMessageReader = gMessageSystem->mLLSDMessageReader;
-	
+	LockMessageReader rdr(gMessageSystem->mMessageReader, gMessageSystem->mLLSDMessageReader);
+
 	if(gMessageSystem->callHandler(namePtr, false, gMessageSystem))
 	{
 		response->result(LLSD());
@@ -189,7 +189,7 @@ void LLMessageSystem::init()
 	mTimingCallbackData = NULL;
 
 	mMessageBuilder = NULL;
-	mMessageReader = NULL;
+	LockMessageReader(mMessageReader, NULL);
 }
 
 // Read file and build message templates
@@ -230,7 +230,6 @@ LLMessageSystem::LLMessageSystem(const std::string& filename, U32 port,
 
 	mTemplateMessageReader = new LLTemplateMessageReader(mMessageNumbers);
 	mLLSDMessageReader = new LLSDMessageReader();
-	mMessageReader = NULL;
 
 	// initialize various bits of net info
 	mSocket = 0;
@@ -330,7 +329,6 @@ LLMessageSystem::~LLMessageSystem()
 	
 	delete mTemplateMessageReader;
 	mTemplateMessageReader = NULL;
-	mMessageReader = NULL;
 
 	delete mTemplateMessageBuilder;
 	mTemplateMessageBuilder = NULL;
@@ -480,11 +478,12 @@ LLCircuitData* LLMessageSystem::findCircuit(const LLHost& host,
 }
 
 // Returns TRUE if a valid, on-circuit message has been received.
-BOOL LLMessageSystem::checkMessages( S64 frame_count )
+// Requiring a non-const LockMessageChecker reference ensures that
+// mMessageReader has been set to mTemplateMessageReader.
+BOOL LLMessageSystem::checkMessages(LockMessageChecker&, S64 frame_count )
 {
 	// Pump 
 	BOOL	valid_packet = FALSE;
-	mMessageReader = mTemplateMessageReader;
 
 	LLTransferTargetVFile::updateQueue();
 	
@@ -748,7 +747,7 @@ S32	LLMessageSystem::getReceiveBytes() const
 }
 
 
-void LLMessageSystem::processAcks(F32 collect_time)
+void LLMessageSystem::processAcks(LockMessageChecker&, F32 collect_time)
 {
 	F64Seconds mt_sec = getMessageTimeSeconds();
 	{
@@ -2062,8 +2061,9 @@ void LLMessageSystem::dispatch(
 		return;
 	}
 	// enable this for output of message names
-	//LL_INFOS("Messaging") << "< \"" << msg_name << "\"" << LL_ENDL;
-	//LL_DEBUGS() << "data: " << LLSDNotationStreamer(message) << LL_ENDL;	   
+	LL_DEBUGS("Messaging") << "< \"" << msg_name << "\"" << LL_ENDL;
+	LL_DEBUGS("Messaging") << "context: " << context << LL_ENDL;
+	LL_DEBUGS("Messaging") << "message: " << message << LL_ENDL;	   
 
 	handler->post(responsep, context, message);
 }
@@ -3268,6 +3268,8 @@ void null_message_callback(LLMessageSystem *msg, void **data)
 // up, and then sending auth messages.
 void LLMessageSystem::establishBidirectionalTrust(const LLHost &host, S64 frame_count )
 {
+	LockMessageChecker lmc(this);
+
 	std::string shared_secret = get_shared_secret();
 	if(shared_secret.empty())
 	{
@@ -3287,7 +3289,7 @@ void LLMessageSystem::establishBidirectionalTrust(const LLHost &host, S64 frame_
 		addU8Fast(_PREHASH_PingID, 0);
 		addU32Fast(_PREHASH_OldestUnacked, 0);
 		sendMessage(host);
-		if (checkMessages( frame_count ))
+		if (lmc.checkMessages( frame_count ))
 		{
 			if (isMessageFast(_PREHASH_CompletePingCheck) &&
 			    (getSender() == host))
@@ -3295,7 +3297,7 @@ void LLMessageSystem::establishBidirectionalTrust(const LLHost &host, S64 frame_
 				break;
 			}
 		}
-		processAcks();
+		lmc.processAcks();
 		ms_sleep(1);
 	}
 
@@ -3314,8 +3316,8 @@ void LLMessageSystem::establishBidirectionalTrust(const LLHost &host, S64 frame_
 		cdp = mCircuitInfo.findCircuit(host);
 		if(!cdp) break; // no circuit anymore, no point continuing.
 		if(cdp->getTrusted()) break; // circuit is trusted.
-		checkMessages(frame_count);
-		processAcks();
+		lmc.checkMessages(frame_count);
+		lmc.processAcks();
 		ms_sleep(1);
 	}
 }
@@ -3973,11 +3975,18 @@ void LLMessageSystem::setTimeDecodesSpamThreshold( F32 seconds )
 	LLMessageReader::setTimeDecodesSpamThreshold(seconds);
 }
 
+LockMessageChecker::LockMessageChecker(LLMessageSystem* msgsystem):
+    // for the lifespan of this LockMessageChecker instance, use
+    // LLTemplateMessageReader as msgsystem's mMessageReader
+    LockMessageReader(msgsystem->mMessageReader, msgsystem->mTemplateMessageReader),
+    mMessageSystem(msgsystem)
+{}
+
 // HACK! babbage: return true if message rxed via either UDP or HTTP
 // TODO: babbage: move gServicePump in to LLMessageSystem?
-bool LLMessageSystem::checkAllMessages(S64 frame_count, LLPumpIO* http_pump)
+bool LLMessageSystem::checkAllMessages(LockMessageChecker& lmc, S64 frame_count, LLPumpIO* http_pump)
 {
-	if(checkMessages(frame_count))
+	if(lmc.checkMessages(frame_count))
 	{
 		return true;
 	}