SL-13986 Validate buffer size to avoid SIGBUS crash on sscanf

3 files changed, 125 insertions, 82 deletions

diff --git a/indra/llinventory/lllandmark.cpp b/indra/llinventory/lllandmark.cpp
index 4c6075d6b5..789716b449 100644
--- a/indra/llinventory/lllandmark.cpp
+++ b/indra/llinventory/lllandmark.cpp
@@ -103,60 +103,100 @@ LLVector3 LLLandmark::getRegionPos() const
 
 
 // static
-LLLandmark* LLLandmark::constructFromString(const char *buffer)
+LLLandmark* LLLandmark::constructFromString(const char *buffer, const S32 buffer_size)
 {
-	const char* cur = buffer;
 	S32 chars_read = 0;
+	S32 chars_read_total = 0;
 	S32 count = 0;
 	U32 version = 0;
 
+    bool bad_block = false;
+    LLLandmark* result = NULL;
+
 	// read version 
-	count = sscanf( cur, "Landmark version %u\n%n", &version, &chars_read );
-	if(count != 1)
-	{
-		goto error;
-	}
+	count = sscanf( buffer, "Landmark version %u\n%n", &version, &chars_read );
 
-	if(version == 1)
-	{
-		LLVector3d pos;
-		cur += chars_read;
-		// read position
-		count = sscanf( cur, "position %lf %lf %lf\n%n", pos.mdV+VX, pos.mdV+VY, pos.mdV+VZ, &chars_read );
-		if( count != 3 )
-		{
-			goto error;
-		}
-		cur += chars_read;
-		// LL_INFOS() << "Landmark read: " << pos << LL_ENDL;
-		
-		return new LLLandmark(pos);
-	}
-	else if(version == 2)
-	{
-		// *NOTE: Changing the buffer size will require changing the
-		// scanf call below.
-		char region_id_str[MAX_STRING];	/* Flawfinder: ignore */
-		LLVector3 pos;
-		cur += chars_read;
-		count = sscanf(	/* Flawfinder: ignore */
-			cur,
-			"region_id %254s\n%n",
-			region_id_str, &chars_read);
-		if(count != 1) goto error;
-		cur += chars_read;
-		count = sscanf(cur, "local_pos %f %f %f\n%n", pos.mV+VX, pos.mV+VY, pos.mV+VZ, &chars_read);
-		if(count != 3) goto error;
-		cur += chars_read;
-		LLLandmark* lm = new LLLandmark;
-		lm->mRegionID.set(region_id_str);
-		lm->mRegionPos = pos;
-		return lm;
-	}
+    if (count != 1)
+    {
+        bad_block = true;
+    }
+
+    chars_read_total += chars_read;
+
+    if (chars_read_total >= buffer_size)
+    {
+        // either file was truncated or data in file was damaged
+        bad_block = true;
+    }
+
+    if (!bad_block)
+    {
+        switch (version)
+        {
+            case 1:
+            {
+                LLVector3d pos;
+                // read position
+                count = sscanf(buffer + chars_read_total, "position %lf %lf %lf\n%n", pos.mdV + VX, pos.mdV + VY, pos.mdV + VZ, &chars_read);
+                if (count != 3)
+                {
+                    bad_block = true;
+                }
+                else
+                {
+                    LL_DEBUGS("Landmark") << "Landmark read: " << pos << LL_ENDL;
+                    result = new LLLandmark(pos);
+                }
+                break;
+            }
+            case 2:
+            {
+                // *NOTE: Changing the buffer size will require changing the
+                // scanf call below.
+                char region_id_str[MAX_STRING];
+                LLVector3 pos;
+                count = sscanf( buffer + chars_read_total,
+                                "region_id %254s\n%n",
+                                region_id_str,
+                                &chars_read);
+                if (count != 1)
+                {
+                    bad_block = true;
+                }
+                chars_read_total += chars_read;
+                if (chars_read_total >= buffer_size)
+                {
+                    bad_block = true;
+                }
+                if (!bad_block)
+                {
+                    count = sscanf(buffer + chars_read_total, "local_pos %f %f %f\n%n", pos.mV + VX, pos.mV + VY, pos.mV + VZ, &chars_read);
+                    if (count != 3)
+                    {
+                        bad_block = true;
+                    }
+                    else
+                    {
+                        result = new LLLandmark;
+                        result->mRegionID.set(region_id_str);
+                        result->mRegionPos = pos;
+                    }
+                }
+                break;
+            }
+            default:
+            {
+                LL_INFOS("Landmark") << "Encountered Unknown landmark version " << version << LL_ENDL;
+                break;
+            }
+        }
+    }
 
- error:
-	LL_INFOS() << "Bad Landmark Asset: bad _DATA_ block." << LL_ENDL;
-	return NULL;
+    if (bad_block)
+    {
+        LL_INFOS("Landmark") << "Bad Landmark Asset: bad _DATA_ block." << LL_ENDL;
+    }
+    return result;
 }
 
 
@@ -176,7 +216,7 @@ void LLLandmark::requestRegionHandle(
 	if(region_id.isNull())
 	{
 		// don't bother with checking - it's 0.
-		LL_DEBUGS() << "requestRegionHandle: null" << LL_ENDL;
+		LL_DEBUGS("Landmark") << "requestRegionHandle: null" << LL_ENDL;
 		if(callback)
 		{
 			const U64 U64_ZERO = 0;
@@ -187,7 +227,7 @@ void LLLandmark::requestRegionHandle(
 	{
 		if(region_id == mLocalRegion.first)
 		{
-			LL_DEBUGS() << "requestRegionHandle: local" << LL_ENDL;
+			LL_DEBUGS("Landmark") << "requestRegionHandle: local" << LL_ENDL;
 			if(callback)
 			{
 				callback(region_id, mLocalRegion.second);
@@ -198,13 +238,13 @@ void LLLandmark::requestRegionHandle(
 			region_map_t::iterator it = mRegions.find(region_id);
 			if(it == mRegions.end())
 			{
-				LL_DEBUGS() << "requestRegionHandle: upstream" << LL_ENDL;
+				LL_DEBUGS("Landmark") << "requestRegionHandle: upstream" << LL_ENDL;
 				if(callback)
 				{
 					region_callback_map_t::value_type vt(region_id, callback);
 					sRegionCallbackMap.insert(vt);
 				}
-				LL_DEBUGS() << "Landmark requesting information about: "
+				LL_DEBUGS("Landmark") << "Landmark requesting information about: "
 						 << region_id << LL_ENDL;
 				msg->newMessage("RegionHandleRequest");
 				msg->nextBlock("RequestBlock");
@@ -214,7 +254,7 @@ void LLLandmark::requestRegionHandle(
 			else if(callback)
 			{
 				// we have the answer locally - just call the callack.
-				LL_DEBUGS() << "requestRegionHandle: ready" << LL_ENDL;
+				LL_DEBUGS("Landmark") << "requestRegionHandle: ready" << LL_ENDL;
 				callback(region_id, (*it).second.mRegionHandle);
 			}
 		}
diff --git a/indra/llinventory/lllandmark.h b/indra/llinventory/lllandmark.h
index 92923ea6fb..be34113a90 100644
--- a/indra/llinventory/lllandmark.h
+++ b/indra/llinventory/lllandmark.h
@@ -60,7 +60,7 @@ public:
 
 	// constructs a new LLLandmark from a string
 	// return NULL if there's an error
-	static LLLandmark* constructFromString(const char *buffer);
+	static LLLandmark* constructFromString(const char *buffer, const S32 buffer_size);
 
 	// register callbacks that this class handles
 	static void registerCallbacks(LLMessageSystem* msg);
diff --git a/indra/newview/lllandmarklist.cpp b/indra/newview/lllandmarklist.cpp
index c58540914e..a790e513a2 100644
--- a/indra/newview/lllandmarklist.cpp
+++ b/indra/newview/lllandmarklist.cpp
@@ -109,36 +109,39 @@ void LLLandmarkList::processGetAssetReply(
 		LLVFile file(vfs, uuid, type);
 		S32 file_length = file.getSize();
 
-		std::vector<char> buffer(file_length + 1);
-		file.read( (U8*)&buffer[0], file_length);
-		buffer[ file_length ] = 0;
-
-		LLLandmark* landmark = LLLandmark::constructFromString(&buffer[0]);
-		if (landmark)
-		{
-			gLandmarkList.mList[ uuid ] = landmark;
-			gLandmarkList.mRequestedList.erase(uuid);
-			
-			LLVector3d pos;
-			if(!landmark->getGlobalPos(pos))
-			{
-				LLUUID region_id;
-				if(landmark->getRegionID(region_id))
-				{
-					LLLandmark::requestRegionHandle(
-						gMessageSystem,
-						gAgent.getRegionHost(),
-						region_id,
-						boost::bind(&LLLandmarkList::onRegionHandle, &gLandmarkList, uuid));
-				}
-
-				// the callback will be called when we get the region handle.
-			}
-			else
-			{
-				gLandmarkList.makeCallbacks(uuid);
-			}
-		}
+        if (file_length > 0)
+        {
+            std::vector<char> buffer(file_length + 1);
+            file.read((U8*)&buffer[0], file_length);
+            buffer[file_length] = 0;
+
+            LLLandmark* landmark = LLLandmark::constructFromString(&buffer[0], buffer.size());
+            if (landmark)
+            {
+                gLandmarkList.mList[uuid] = landmark;
+                gLandmarkList.mRequestedList.erase(uuid);
+
+                LLVector3d pos;
+                if (!landmark->getGlobalPos(pos))
+                {
+                    LLUUID region_id;
+                    if (landmark->getRegionID(region_id))
+                    {
+                        LLLandmark::requestRegionHandle(
+                            gMessageSystem,
+                            gAgent.getRegionHost(),
+                            region_id,
+                            boost::bind(&LLLandmarkList::onRegionHandle, &gLandmarkList, uuid));
+                    }
+
+                    // the callback will be called when we get the region handle.
+                }
+                else
+                {
+                    gLandmarkList.makeCallbacks(uuid);
+                }
+            }
+        }
 	}
 	else
 	{