From 714a0c2d9c90f34ff54dedee40bff95d6a3aff8c Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 00:43:33 -0400 Subject: [PATCH 1/7] LLPacketRing: stop clobbering buffered packets on empty receives, restore asserts bufferInboundPacket() received straight into the ring slot at mHeadIndex before knowing whether a packet had actually arrived. When the ring is full, that slot holds the oldest *unread* packet, and the would-block receive that terminates every drainSocket() pass reset its size to 0 and desynced the byte/packet accounting. Since the drain path runs exactly when message processing falls behind and the ring pins at its cap, every drain under sustained overload corrupted one buffered packet and leaked its byte count. Receive into a scratch buffer and only commit the slot once we have a real packet, the same way the SOCKS branch already works. This was the single root cause behind all three asserts disabled in cbf99f68d7 (they were raw assert(), so they only ever fired in Debug). With the clobbering gone they are true invariants again; restore them as llassert with comments on why each holds. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/llpacketring.cpp | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/indra/llmessage/llpacketring.cpp b/indra/llmessage/llpacketring.cpp index 15f0d18b0e..d49556cf14 100644 --- a/indra/llmessage/llpacketring.cpp +++ b/indra/llmessage/llpacketring.cpp @@ -194,7 +194,8 @@ S32 LLPacketRing::receiveOrDropPacket(S32 socket, char *datap, bool drop) S32 LLPacketRing::receiveOrDropBufferedPacket(char *datap, bool drop) { - //llassert(mNumBufferedPackets > 0); + // receivePacket() only routes here when packets are buffered. + llassert(mNumBufferedPackets > 0); S32 packet_size = 0; @@ -209,7 +210,9 @@ S32 LLPacketRing::receiveOrDropBufferedPacket(char *datap, bool drop) mNumBufferedBytes -= packet_size; if (mNumBufferedPackets == 0) { - //llassert(mNumBufferedBytes == 0); + // Byte accounting nets to zero once the ring drains. Held now that + // bufferInboundPacket no longer clobbers a live slot on empty receives. + llassert(mNumBufferedBytes == 0); } if (!drop) @@ -220,7 +223,10 @@ S32 LLPacketRing::receiveOrDropBufferedPacket(char *datap, bool drop) } else { - //llassert(false); assertion disabled due to 0 size packets from server???? + // Unreachable: bufferInboundPacket only commits a slot for a real + // (size > 0) packet, so a buffered packet never reads back as 0. + // Assert in debug; fall through returning 0 in release. + llassert(false); } } else @@ -279,12 +285,21 @@ S32 LLPacketRing::bufferInboundPacket(S32 socket) } else { - packet->init(socket); - packet_size = packet->getSize(); + // Receive into a scratch buffer first rather than straight into the + // ring slot. When the ring is full, mPacketRing[mHeadIndex] is the + // oldest *unread* packet; a would-block or zero-length datagram makes + // receive_packet() return <= 0, and receiving directly into the slot + // would clobber that unread packet and desync the byte/packet + // accounting. Only commit the slot once we know we have a real + // packet. (The SOCKS branch above already works this way.) + char buffer[NET_BUFFER_SIZE]; /* Flawfinder: ignore */ + packet_size = receive_packet(socket, buffer); if (packet_size > 0) { mActualBytesIn += packet_size; + packet->init(buffer, packet_size, ::get_sender()); + mHeadIndex = (mHeadIndex + 1) % (S16)(mPacketRing.size()); if (mNumBufferedPackets < MAX_BUFFER_RING_SIZE) { From 6e392ac49e64589503a85e17c80d9a59eb4891ef Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 00:43:52 -0400 Subject: [PATCH 2/7] LLMessageSystem: fix zerocode overflow recovery, ack-space math, receive stats - zeroCodeExpand() overflow case 3 reset outptr to the buffer start and kept decoding, handing a scrambled packet to the parser. Discard the malformed packet instead, mirroring overflow cases 1 and 2. - sendMessage() computed the appended-ack budget with unsigned math, so an over-MTU payload (ChildAgentUpdate, SendXferPacket) wrapped to a huge space_left and got acks appended despite having no room. Signed math yields a negative count and skips the append. - Record the message number in the receive-count list. It was never set (TODO from the babbage era), so dumpReceiveCounts() -- the diagnostic that fires on message storms -- has always attributed nothing. - operator<< probed mMessageNumbers with operator[], permanently inserting a null entry for every missing id it touched. Use find(). - Log packets at exactly the minimum valid size (>= vs >). - Fix the big-endian MVT_S16Array swizzle iterating n % 2 elements instead of n / 2. Inert on little-endian builds. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/message.cpp | 57 ++++++++++++++++++++----------------- indra/llmessage/message.h | 2 +- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index 876feffc79..9851e05acd 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -534,7 +534,7 @@ bool LLMessageSystem::checkMessages(LockMessageChecker&, S64 frame_count, // If you want to dump all received packets into Alchemy.log, uncomment this //dumpPacketToLog(); - if(mTrueReceiveSize && receive_size > (S32) LL_MINIMUM_VALID_PACKET_SIZE && !faked_message) + if(mTrueReceiveSize && receive_size >= (S32) LL_MINIMUM_VALID_PACKET_SIZE && !faked_message) { #define LOCALHOST_ADDR 16777343 LLMessageLog::log(mLastSender, LLHost(LOCALHOST_ADDR, mPort), buffer, mTrueReceiveSize); @@ -1248,8 +1248,9 @@ S32 LLMessageSystem::sendMessage(const LLHost &host) mReliablePacketsOut++; } - // tack packet acks onto the end of this message - S32 space_left = (MTUBYTES - buffer_length) / sizeof(TPACKETID); // space left for packet ids + // tack packet acks onto the end of this message; signed math so an + // over-MTU payload yields a negative count instead of wrapping huge + S32 space_left = (MTUBYTES - (S32)buffer_length) / (S32)sizeof(TPACKETID); // space left for packet ids S32 ack_count = (S32)cdp->mAcks.size(); bool is_ack_appended = false; std::vector acks; @@ -1369,8 +1370,8 @@ void LLMessageSystem::logMsgFromInvalidCircuit( const LLHost& host, bool recv_re } else { - // TODO: babbage: work out if we need these - // mMessageCountList[mNumMessageCounts].mMessageNum = mCurrentRMessageTemplate->mMessageNumber; + const LLMessageTemplate* mt = mTemplateMessageReader->getTemplate(); + mMessageCountList[mNumMessageCounts].mMessageNum = mt ? mt->mMessageNumber : 0; mMessageCountList[mNumMessageCounts].mMessageBytes = mMessageReader->getMessageSize(); mMessageCountList[mNumMessageCounts].mInvalid = true; mNumMessageCounts++; @@ -1420,9 +1421,8 @@ void LLMessageSystem::logTrustedMsgFromUntrustedCircuit( const LLHost& host ) } else { - // TODO: babbage: work out if we need these - //mMessageCountList[mNumMessageCounts].mMessageNum - // = mCurrentRMessageTemplate->mMessageNumber; + const LLMessageTemplate* mt = mTemplateMessageReader->getTemplate(); + mMessageCountList[mNumMessageCounts].mMessageNum = mt ? mt->mMessageNumber : 0; mMessageCountList[mNumMessageCounts].mMessageBytes = mMessageReader->getMessageSize(); mMessageCountList[mNumMessageCounts].mInvalid = true; @@ -1438,8 +1438,8 @@ void LLMessageSystem::logValidMsg(LLCircuitData *cdp, const LLHost& host, bool r } else { - // TODO: babbage: work out if we need these - //mMessageCountList[mNumMessageCounts].mMessageNum = mCurrentRMessageTemplate->mMessageNumber; + const LLMessageTemplate* mt = mTemplateMessageReader->getTemplate(); + mMessageCountList[mNumMessageCounts].mMessageNum = mt ? mt->mMessageNumber : 0; mMessageCountList[mNumMessageCounts].mMessageBytes = mMessageReader->getMessageSize(); mMessageCountList[mNumMessageCounts].mInvalid = false; mNumMessageCounts++; @@ -1727,7 +1727,6 @@ void LLMessageSystem::setMaxMessageCounts(const S32 num) std::ostream& operator<<(std::ostream& s, LLMessageSystem &msg) { - U32 i; if (msg.mbError) { s << "Message system not correctly initialized"; @@ -1737,26 +1736,29 @@ std::ostream& operator<<(std::ostream& s, LLMessageSystem &msg) s << "Message system open on port " << msg.mPort << " and socket " << msg.mSocket << "\n"; // s << "Message template file " << msg.mName << " loaded\n"; - s << "\nHigh frequency messages:\n"; - - for (i = 1; msg.mMessageNumbers[i] && (i < 255); i++) + // use find() rather than operator[] so dumping doesn't pollute the + // number map with null entries for every probed id + auto dump_band = [&s, &msg](U32 first, U32 last) { - s << *(msg.mMessageNumbers[i]); - } + for (U32 i = first; i < last; i++) + { + auto iter = msg.mMessageNumbers.find(i); + if (iter == msg.mMessageNumbers.end() || !iter->second) + { + break; + } + s << *(iter->second); + } + }; - s << "\nMedium frequency messages:\n"; + s << "\nHigh frequency messages:\n"; + dump_band(1, 255); - for (i = (255 << 8) + 1; msg.mMessageNumbers[i] && (i < (255 << 8) + 255); i++) - { - s << *msg.mMessageNumbers[i]; - } + s << "\nMedium frequency messages:\n"; + dump_band((255 << 8) + 1, (255 << 8) + 255); s << "\nLow frequency messages:\n"; - - for (i = (0xFFFF0000) + 1; msg.mMessageNumbers[i] && (i < 0xFFFFFFFF); i++) - { - s << *msg.mMessageNumbers[i]; - } + dump_band(0xFFFF0000 + 1, 0xFFFFFFFF); } return s; } @@ -2929,7 +2931,10 @@ S32 LLMessageSystem::zeroCodeExpand(U8** data, S32* data_size) { LL_WARNS("Messaging") << "attempt to write past reasonable encoded buffer size 3" << LL_ENDL; callExceptionFunc(MX_WROTE_PAST_BUFFER_SIZE); + // discard the malformed packet instead of continuing to + // decode into a scrambled buffer (mirrors cases 1 and 2) outptr = mEncodedRecvBuffer; + break; } memset(outptr,0,(*inptr) - 1); outptr += ((*inptr) - 1); diff --git a/indra/llmessage/message.h b/indra/llmessage/message.h index 83d867b04c..3461bd9cc4 100644 --- a/indra/llmessage/message.h +++ b/indra/llmessage/message.h @@ -1136,7 +1136,7 @@ static inline void *htolememcpy(void *vs, const void *vct, EMsgVariableType type LL_ERRS() << "Size argument passed to htolememcpy doesn't match swizzle type size" << LL_ENDL; } #ifdef LL_BIG_ENDIAN - length = n % 2; + length = (S32)(n / 2); for (i = 1; i < length; i++) { htolememcpy(s + i*2, ct + i*2, MVT_S16, 2); From bf313ca38ab915bcd9c148c7f302cf2330cef2d0 Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 00:44:05 -0400 Subject: [PATCH 3/7] LLTemplateMessageBuilder: never write through the caller's buffer on truncation When more than 255 bytes were stuffed into a Variable-1 field, addData() NUL-terminated the payload by casting away const and writing into the caller's buffer -- undefined behavior through std::string::c_str() (both addString overloads route here) and a straight crash for a string literal. Clamp the size, let addData() copy the payload, then terminate the *stored* copy. Wire bytes and stored bytes are unchanged; only the caller mutation is gone. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/lltemplatemessagebuilder.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/indra/llmessage/lltemplatemessagebuilder.cpp b/indra/llmessage/lltemplatemessagebuilder.cpp index 758d6d343f..94b3a04dcc 100644 --- a/indra/llmessage/lltemplatemessagebuilder.cpp +++ b/indra/llmessage/lltemplatemessagebuilder.cpp @@ -313,6 +313,7 @@ void LLTemplateMessageBuilder::addData(const char *varname, const void *data, EM if (var_data->getType() == MVT_VARIABLE) { // Variable 1 can only store 255 bytes, make sure our data is smaller + bool truncated = false; if ((var_data->getSize() == 1) && (size > 255)) { @@ -320,12 +321,19 @@ void LLTemplateMessageBuilder::addData(const char *varname, const void *data, EM << "attempted to stuff more than 255 bytes in " << "(" << size << "). Clamping size and truncating data." << LL_ENDL; size = 255; - char *truncate = (char *)data; - truncate[254] = 0; // array size is 255 but the last element index is 254 + truncated = true; } // no correct size for MVT_VARIABLE, instead we need to tell how many bytes the size will be encoded as mCurrentSDataBlock->addData(vnamep, data, size, type, var_data->getSize()); + if (truncated) + { + // NUL-terminate the *stored* copy so a truncated string field is + // still a valid C string. The caller's buffer is const and must + // not be written through (it may be std::string::c_str() or a + // literal); last element index of the 255 stored bytes is 254. + static_cast(mCurrentSDataBlock->mMemberVarData[vnamep].getData())[254] = 0; + } mCurrentSendTotal += size; } else From 8d092c1264ea3c6c091fe1a7d97849a714473b9c Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 00:44:26 -0400 Subject: [PATCH 4/7] Message decode: bound variable-length fields, cut per-variable heap traffic Hardening: - decodeData() trusted the wire-claimed size of MVT_VARIABLE fields. A malformed or hostile packet could drive the copy up to 64KB past the receive buffer via a 2-byte length, or hand new U8[] a negative size via the 4-byte form. Clamp the claimed size to the bytes remaining in the packet and log it as ran-off-end. Regression test included (test<46>, verified to fail against the unclamped decoder). Allocation overhaul -- previously every variable of every block of every inbound packet cost a map-node insert plus a new U8[] + copy, all freed again at clearMessage(): - LLMsgVarData stores payloads up to 24 bytes inline, sized to cover every fixed wire type (largest is LLVector3d), so only long Variable fields heap-allocate. getData() computes the pointer so vector growth cannot dangle into a moved-from element. - Replace the per-block LLIndexedVector (vector + std::map index) with an insertion-ordered flat vector scanned by prehashed name pointer: same API and iteration order, zero per-entry node allocations. operator[] inserts keyed by name so repeated misses return the same entry, and the reader's getSize() overloads query with find() so lookups never insert at all. - decodeData() takes the variable ref returned by addVariable() instead of re-finding it by name for addData(); reader getData() does one lookup instead of find() + operator[]. test<47> covers the Variable-1 truncation fix from the previous commit: caller buffer untouched, stored copy clamped and NUL-terminated. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/llmessagetemplate.cpp | 10 ++- indra/llmessage/llmessagetemplate.h | 79 +++++++++++++++++-- indra/llmessage/llsdmessagebuilder.cpp | 4 +- indra/llmessage/lltemplatemessagereader.cpp | 43 ++++++---- .../tests/lltemplatemessagebuilder_test.cpp | 55 +++++++++++++ 5 files changed, 166 insertions(+), 25 deletions(-) diff --git a/indra/llmessage/llmessagetemplate.cpp b/indra/llmessage/llmessagetemplate.cpp index 6a0a8e1b91..cdbde86d0f 100644 --- a/indra/llmessage/llmessagetemplate.cpp +++ b/indra/llmessage/llmessagetemplate.cpp @@ -46,8 +46,14 @@ void LLMsgVarData::addData(const void *data, S32 size, EMsgVariableType type, S3 if(size) { delete[] mData; // Delete it if it already exists - mData = new U8[size]; - htolememcpy(mData, data, mType, size); + mData = nullptr; + U8* dest = mInlineData; + if (size > INLINE_DATA_SIZE) + { + mData = new U8[size]; + dest = mData; + } + htolememcpy(dest, data, mType, size); } } diff --git a/indra/llmessage/llmessagetemplate.h b/indra/llmessage/llmessagetemplate.h index c5e2bf47e7..f1ccb62153 100644 --- a/indra/llmessage/llmessagetemplate.h +++ b/indra/llmessage/llmessagetemplate.h @@ -55,18 +55,82 @@ class LLMsgVarData char *getName() const { return mName; } S32 getSize() const { return mSize; } - void *getData() { return (void*)mData; } - const void *getData() const { return (const void*)mData; } + void *getData() { return mData ? (void*)mData : (mSize > 0 ? (void*)mInlineData : nullptr); } + const void *getData() const { return mData ? (const void*)mData : (mSize > 0 ? (const void*)mInlineData : nullptr); } S32 getDataSize() const { return mDataSize; } EMsgVariableType getType() const { return mType; } protected: + // Payloads up to this size are stored in mInlineData instead of a heap + // block, sized to fit the largest fixed wire type (LLVector3d, 24 bytes) + // so only long Variable fields allocate. + static constexpr S32 INLINE_DATA_SIZE = 24; + char *mName; S32 mSize; S32 mDataSize; - U8 *mData; + U8 *mData; // heap storage, only used when mSize > INLINE_DATA_SIZE EMsgVariableType mType; + U8 mInlineData[INLINE_DATA_SIZE]; +}; + +// Insertion-ordered flat map from prehashed name pointer to variable data. +// One instance exists per block per decoded/built message, with at most a +// few dozen entries, so a linear pointer-compare scan wins over a node-based +// map and avoids its per-entry allocations on the packet decode path. +class LLMsgVarDataMap +{ +public: + typedef std::vector::iterator iterator; + typedef std::vector::const_iterator const_iterator; + + LLMsgVarDataMap() { mEntries.reserve(8); } + + iterator begin() { return mEntries.begin(); } + const_iterator begin() const { return mEntries.begin(); } + iterator end() { return mEntries.end(); } + const_iterator end() const { return mEntries.end(); } + bool empty() const { return mEntries.empty(); } + size_t size() const { return mEntries.size(); } + + // Returns the entry for name, inserting an empty one keyed to name if + // absent (std::map::operator[] semantics — repeated misses yield the + // same entry, never duplicates). An inserted entry has size -1 until + // someone calls addData on it. + LLMsgVarData& operator[](const char* name) + { + for (LLMsgVarData& entry : mEntries) + { + if (entry.getName() == name) + { + return entry; + } + } + return mEntries.emplace_back(name, MVT_U8); + } + + iterator find(const char* name) + { + iterator it = mEntries.begin(); + const iterator it_end = mEntries.end(); + for (; it != it_end; ++it) + { + if (it->getName() == name) + { + break; + } + } + return it; + } + + const_iterator find(const char* name) const + { + return const_cast(this)->find(name); + } + +private: + std::vector mEntries; }; class LLMsgBlkData @@ -85,10 +149,11 @@ class LLMsgBlkData } } - void addVariable(const char *name, EMsgVariableType type) + LLMsgVarData& addVariable(const char *name, EMsgVariableType type) { - LLMsgVarData tmp(name,type); - mMemberVarData[name] = tmp; + LLMsgVarData& entry = mMemberVarData[name]; + entry = LLMsgVarData(name, type); + return entry; } void addData(char *name, const void *data, S32 size, EMsgVariableType type, S32 data_size = -1) @@ -98,7 +163,7 @@ class LLMsgBlkData } S32 mBlockNumber; - typedef LLIndexedVector msg_var_data_map_t; + typedef LLMsgVarDataMap msg_var_data_map_t; msg_var_data_map_t mMemberVarData; char *mName; S32 mTotalSize; diff --git a/indra/llmessage/llsdmessagebuilder.cpp b/indra/llmessage/llsdmessagebuilder.cpp index 096abaa2ea..bfc5710a9d 100644 --- a/indra/llmessage/llsdmessagebuilder.cpp +++ b/indra/llmessage/llsdmessagebuilder.cpp @@ -41,8 +41,8 @@ namespace { - // The message variable storage is a raw `U8[]` buffer (allocated as - // `new U8[size]` in LLMsgVarData), so reading it as the variable's + // The message variable storage is a raw `U8[]` buffer (inline or + // heap-allocated in LLMsgVarData), so reading it as the variable's // logical type via `*(T*)mvci.getData()` is strict-aliasing UB. memcpy // through a trivially-copyable local stays inside the rules and lowers // to the same load on every supported toolchain. diff --git a/indra/llmessage/lltemplatemessagereader.cpp b/indra/llmessage/lltemplatemessagereader.cpp index 84b5373ba4..268b809917 100644 --- a/indra/llmessage/lltemplatemessagereader.cpp +++ b/indra/llmessage/lltemplatemessagereader.cpp @@ -92,14 +92,15 @@ void LLTemplateMessageReader::getData(const char *blockname, const char *varname LLMsgBlkData *msg_block_data = iter->second; LLMsgBlkData::msg_var_data_map_t &var_data_map = msg_block_data->mMemberVarData; - if (var_data_map.find(vnamep) == var_data_map.end()) + LLMsgBlkData::msg_var_data_map_t::iterator vit = var_data_map.find(vnamep); + if (vit == var_data_map.end()) { LL_ERRS() << "Variable "<< vnamep << " not in message " << mCurrentRMessageData->mName<< " block " << bnamep << LL_ENDL; return; } - LLMsgVarData& vardata = msg_block_data->mMemberVarData[vnamep]; + LLMsgVarData& vardata = *vit; if (size && size != vardata.getSize()) { @@ -194,15 +195,17 @@ S32 LLTemplateMessageReader::getSize(const char *blockname, const char *varname) char *vnamep = (char *)varname; LLMsgBlkData* msg_data = iter->second; - LLMsgVarData& vardata = msg_data->mMemberVarData[vnamep]; + LLMsgBlkData::msg_var_data_map_t::iterator vit = msg_data->mMemberVarData.find(vnamep); - if (!vardata.getName()) + if (vit == msg_data->mMemberVarData.end()) { // don't crash LL_INFOS() << "Variable " << varname << " not in message " << mCurrentRMessageData->mName << " block " << bnamep << LL_ENDL; return LL_VARIABLE_NOT_IN_BLOCK; } + LLMsgVarData& vardata = *vit; + if (mCurrentRMessageTemplate->mMemberBlocks[bnamep]->mType != MBT_SINGLE) { // This is a serious error - crash LL_ERRS() << "Block " << bnamep << " isn't type MBT_SINGLE," @@ -241,15 +244,17 @@ S32 LLTemplateMessageReader::getSize(const char *blockname, S32 blocknum, const } LLMsgBlkData* msg_data = iter->second; - LLMsgVarData& vardata = msg_data->mMemberVarData[vnamep]; + LLMsgBlkData::msg_var_data_map_t::iterator vit = msg_data->mMemberVarData.find(vnamep); - if (!vardata.getName()) + if (vit == msg_data->mMemberVarData.end()) { // don't crash LL_INFOS() << "Variable " << vnamep << " not in message " << mCurrentRMessageData->mName << " block " << bnamep << LL_ENDL; return LL_VARIABLE_NOT_IN_BLOCK; } + LLMsgVarData& vardata = *vit; + return vardata.getSize(); } @@ -617,7 +622,8 @@ bool LLTemplateMessageReader::decodeData(const U8* buffer, const LLHost& sender, // ok, build out the variables // add variable block - cur_data_block->addVariable(mvci.getName(), mvci.getType()); + LLMsgVarData& vardata = + cur_data_block->addVariable(mvci.getName(), mvci.getType()); // what type of variable? if (mvci.getType() == MVT_VARIABLE) @@ -658,7 +664,18 @@ bool LLTemplateMessageReader::decodeData(const U8* buffer, const LLHost& sender, } decode_pos += data_size; - cur_data_block->addData(mvci.getName(), &buffer[decode_pos], tsize, mvci.getType()); + // Bound the wire-claimed length by the bytes actually + // remaining in the packet, so a malformed length field + // can't drive an out-of-bounds read. + if (tsize > (U32)llmax(mReceiveSize - decode_pos, 0)) + { + if (!custom) + logRanOffEndOfPacket(sender, decode_pos, (S32)tsize); + + tsize = 0; + } + + vardata.addData(&buffer[decode_pos], tsize, mvci.getType()); decode_pos += tsize; } else @@ -673,15 +690,13 @@ bool LLTemplateMessageReader::decodeData(const U8* buffer, const LLHost& sender, // default to 0s. U32 size = mvci.getSize(); std::vector data(size, 0); - cur_data_block->addData(mvci.getName(), &(data[0]), - size, mvci.getType()); + vardata.addData(&(data[0]), size, mvci.getType()); } else { - cur_data_block->addData(mvci.getName(), - &buffer[decode_pos], - mvci.getSize(), - mvci.getType()); + vardata.addData(&buffer[decode_pos], + mvci.getSize(), + mvci.getType()); } decode_pos += mvci.getSize(); } diff --git a/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp b/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp index ddf1c3dbcc..074e15dcc2 100644 --- a/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp +++ b/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp @@ -967,5 +967,60 @@ namespace tut ensure_equals("Ensure unchanged buffer ", strlen(outBuffer), 0); delete reader; } + + template<> template<> + void LLTemplateMessageBuilderTestObject::test<46>() + // variable-length field whose on-wire size claims more bytes than + // the message contains -> read clamps to 0 length instead of + // running off the end of the buffer + { + // build a message with one 1-byte-length variable field + LLMessageTemplate messageTemplate = defaultTemplate(); + messageTemplate.addBlock(defaultBlock(MVT_VARIABLE, 1, MBT_SINGLE)); + LLTemplateMessageBuilder* builder = defaultBuilder(messageTemplate); + builder->addString(_PREHASH_Test0, "hello"); + const U32 bufferSize = 1024; + U8 buffer[bufferSize]; + memset(buffer, 0xaa, bufferSize); + memset(buffer, 0, LL_PACKET_ID_SIZE); + U32 builtSize = builder->buildMessage(buffer, bufferSize, 0); + delete builder; + + // corrupt the length byte (located after the 6-byte header and + // 1-byte high-frequency message number) so the field claims to + // extend far past the end of the message + buffer[LL_PACKET_ID_SIZE + 1] = 255; + + numberMap[1] = &messageTemplate; + LLTemplateMessageReader* reader = + new LLTemplateMessageReader(numberMap); + reader->validateMessage(buffer, builtSize, LLHost()); + reader->readMessage(buffer, LLHost()); + ensure_equals("Ensure malformed length clamped to empty", + reader->getSize(_PREHASH_Test0, _PREHASH_Test0), 0); + delete reader; + } + + template<> template<> + void LLTemplateMessageBuilderTestObject::test<47>() + // stuffing an oversized string into a Variable-1 field truncates the + // stored copy without writing into the caller's buffer + { + LLMessageTemplate messageTemplate = defaultTemplate(); + messageTemplate.addBlock(defaultBlock(MVT_VARIABLE, 1, MBT_SINGLE)); + LLTemplateMessageBuilder* builder = defaultBuilder(messageTemplate); + std::string in(300, 'x'); + builder->addString(_PREHASH_Test0, in); + ensure_equals("Ensure caller buffer untouched", in[254], 'x'); + ensure_equals("Ensure caller size untouched", in.size(), (size_t)300); + + // round-trip: stored copy is clamped to 255 bytes and NUL-terminated + LLTemplateMessageReader* reader = setReader(messageTemplate, builder); + char outBuffer[1024]; + reader->getString(_PREHASH_Test0, _PREHASH_Test0, 1024, outBuffer); + ensure_equals("Ensure stored string truncated and terminated", + strlen(outBuffer), (size_t)254); + delete reader; + } } From 7c231dff17f280eac8a635ef805f8eb16b3e2424 Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 01:06:11 -0400 Subject: [PATCH 5/7] Message strings: terminate at copied length; account outbound compression getData() now returns the number of bytes it copied, and both getString() overloads NUL-terminate at exactly that length. The std::string overload no longer zero-fills a 1501-byte stack buffer on every chat/name/string read, and the char* overload no longer leaves the span between the copied data and buffer_size-1 uninitialized, where an unterminated wire string could expose stale stack contents to the caller's strlen. Reinstate the outbound zerocode compression stats (TODO'd out since the babbage era, leaving summarizeLogs reporting zeros) by accounting them in LLMessageSystem::sendMessage(), which owns the counters and can detect the buffer swap compressMessage() performs only when compression wins. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/lltemplatemessagebuilder.cpp | 10 ++----- indra/llmessage/lltemplatemessagereader.cpp | 28 ++++++++++++-------- indra/llmessage/lltemplatemessagereader.h | 5 ++-- indra/llmessage/message.cpp | 7 +++++ 4 files changed, 29 insertions(+), 21 deletions(-) diff --git a/indra/llmessage/lltemplatemessagebuilder.cpp b/indra/llmessage/lltemplatemessagebuilder.cpp index 94b3a04dcc..758352228c 100644 --- a/indra/llmessage/lltemplatemessagebuilder.cpp +++ b/indra/llmessage/lltemplatemessagebuilder.cpp @@ -566,18 +566,12 @@ static S32 zero_code(U8 **data, U32 *data_size) if (net_gain < 0) { - // TODO: babbage: reinstate stat collecting... - //mCompressedPacketsOut++; - //mUncompressedBytesOut += *data_size; - + // Compression stats are accounted by LLMessageSystem::sendMessage(), + // which detects this buffer swap. *data = encodedSendBuffer; *data_size += net_gain; encodedSendBuffer[0] |= LL_ZERO_CODE_FLAG; // set the head bit to indicate zero coding - - //mCompressedBytesOut += *data_size; - } - //mTotalBytesOut += *data_size; return(net_gain); } diff --git a/indra/llmessage/lltemplatemessagereader.cpp b/indra/llmessage/lltemplatemessagereader.cpp index 268b809917..c215272c16 100644 --- a/indra/llmessage/lltemplatemessagereader.cpp +++ b/indra/llmessage/lltemplatemessagereader.cpp @@ -62,19 +62,19 @@ void LLTemplateMessageReader::clearMessage() mCurrentRMessageData = nullptr; } -void LLTemplateMessageReader::getData(const char *blockname, const char *varname, void *datap, S32 size, S32 blocknum, S32 max_size) +S32 LLTemplateMessageReader::getData(const char *blockname, const char *varname, void *datap, S32 size, S32 blocknum, S32 max_size) { // is there a message ready to go? if (mReceiveSize == -1) { LL_ERRS() << "No message waiting for decode 2!" << LL_ENDL; - return; + return 0; } if (!mCurrentRMessageData) { LL_ERRS() << "Invalid mCurrentMessageData in getData!" << LL_ENDL; - return; + return 0; } char *bnamep = (char *)blockname + blocknum; // this works because it's just a hash. The bnamep is never derefference @@ -86,7 +86,7 @@ void LLTemplateMessageReader::getData(const char *blockname, const char *varname { LL_ERRS() << "Block " << blockname << " #" << blocknum << " not in message " << mCurrentRMessageData->mName << LL_ENDL; - return; + return 0; } LLMsgBlkData *msg_block_data = iter->second; @@ -97,7 +97,7 @@ void LLTemplateMessageReader::getData(const char *blockname, const char *varname { LL_ERRS() << "Variable "<< vnamep << " not in message " << mCurrentRMessageData->mName<< " block " << bnamep << LL_ENDL; - return; + return 0; } LLMsgVarData& vardata = *vit; @@ -109,7 +109,7 @@ void LLTemplateMessageReader::getData(const char *blockname, const char *varname << " is size " << vardata.getSize() << " but copying into buffer of size " << size << LL_ENDL; - return; + return 0; } @@ -126,6 +126,7 @@ void LLTemplateMessageReader::getData(const char *blockname, const char *varname { memcpy(datap, vardata.getData(), vardata_size); } + return llmax(vardata_size, 0); // -1 means the variable was never filled } else { @@ -136,6 +137,7 @@ void LLTemplateMessageReader::getData(const char *blockname, const char *varname << LL_ENDL; memcpy(datap, vardata.getData(), max_size); + return max_size; } } @@ -419,15 +421,19 @@ inline void LLTemplateMessageReader::getIPPort(const char *block, const char *va inline void LLTemplateMessageReader::getString(const char *block, const char *var, S32 buffer_size, char *s, S32 blocknum ) { s[0] = '\0'; - getData(block, var, s, 0, blocknum, buffer_size); - s[buffer_size - 1] = '\0'; + S32 data_size = getData(block, var, s, 0, blocknum, buffer_size); + // terminate at the copied length so an unterminated wire string can't + // expose whatever follows it in the caller's buffer + s[llclamp(data_size, 0, buffer_size - 1)] = '\0'; } inline void LLTemplateMessageReader::getString(const char *block, const char *var, std::string& outstr, S32 blocknum ) { - char s[MTUBYTES + 1]= {0}; // every element is initialized with 0 - getData(block, var, s, 0, blocknum, MTUBYTES); - s[MTUBYTES] = '\0'; + // wire strings normally carry their own NUL; terminating at the copied + // length covers the ones that don't, without zero-filling 1.5KB per call + char s[MTUBYTES + 1]; + S32 data_size = getData(block, var, s, 0, blocknum, MTUBYTES); + s[llclamp(data_size, 0, MTUBYTES)] = '\0'; outstr = s; } diff --git a/indra/llmessage/lltemplatemessagereader.h b/indra/llmessage/lltemplatemessagereader.h index cf02208583..d4dab9a7aa 100644 --- a/indra/llmessage/lltemplatemessagereader.h +++ b/indra/llmessage/lltemplatemessagereader.h @@ -111,8 +111,9 @@ class LLTemplateMessageReader : public LLMessageReader private: - void getData(const char *blockname, const char *varname, void *datap, - S32 size = 0, S32 blocknum = 0, S32 max_size = S32_MAX); + // returns the number of bytes copied into datap + S32 getData(const char *blockname, const char *varname, void *datap, + S32 size = 0, S32 blocknum = 0, S32 max_size = S32_MAX); bool decodeTemplate(const U8* buffer, S32 buffer_size, // inputs LLMessageTemplate** msg_template, bool custom = false); // outputs diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index 9851e05acd..b186526703 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -1221,6 +1221,13 @@ S32 LLMessageSystem::sendMessage(const LLHost &host) U8 * buf_ptr = (U8 *)mSendBuffer; U32 buffer_length = mSendSize; mMessageBuilder->compressMessage(buf_ptr, buffer_length); + if (buf_ptr != (U8 *)mSendBuffer) + { + // compressMessage() only swaps buffers when zerocoding won + mCompressedPacketsOut++; + mUncompressedBytesOut += mSendSize; + mCompressedBytesOut += buffer_length; + } if (buffer_length > 1500) { From 5541ce25b5c1a5ef73d320739d0973217c43d1ed Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 01:06:25 -0400 Subject: [PATCH 6/7] Don't end the SOCKS drain on a runt wrapper; fill packet-id gaps across the wrap A SOCKS datagram no larger than its header made bufferInboundPacket() report 0, which drainSocket() reads as "socket empty", stranding any packets still queued behind it. Discard the runt but report its raw size so the drain keeps going; the existing accounting counts it as received-but-dropped. checkPacketInID()'s gap filler compared packet ids with plain <, so a small forward gap spanning the 24-bit wrap (expecting 0xFFFFF8, got 0x000002) was misread as an out-of-order arrival: log spam plus a resync that never marked the skipped ids as potentially lost. Use the modular forward distance instead; the fill loop already wraps. A genuinely old id yields a huge modular distance and still takes the out-of-order path. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/llcircuit.cpp | 5 ++++- indra/llmessage/llpacketring.cpp | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/indra/llmessage/llcircuit.cpp b/indra/llmessage/llcircuit.cpp index caefdad358..62ea547562 100644 --- a/indra/llmessage/llcircuit.cpp +++ b/indra/llmessage/llcircuit.cpp @@ -729,7 +729,10 @@ void LLCircuitData::checkPacketInID(TPACKETID id, bool receive_resent) U64Microseconds time = LLMessageSystem::getMessageTimeUsecs(); TPACKETID index = mPacketsInID; S32 gap_count = 0; - if ((index < id) && ((id - index) < 16)) + // Modular forward distance, so a small gap that spans the 24-bit + // packet-id wrap fills in normally; a genuinely old id yields a + // huge forward distance and still takes the out-of-order path. + if (LLModularMath::subtract(id, index) < 16) { while (index != id) { diff --git a/indra/llmessage/llpacketring.cpp b/indra/llmessage/llpacketring.cpp index d49556cf14..7fdd0e8b25 100644 --- a/indra/llmessage/llpacketring.cpp +++ b/indra/llmessage/llpacketring.cpp @@ -279,7 +279,10 @@ S32 LLPacketRing::bufferInboundPacket(S32 socket) } else { - packet_size = 0; + // Runt SOCKS wrapper (no payload past the header): discard + // it, but keep the raw size so drainSocket() keeps draining + // instead of misreading one bad datagram as an empty socket. + // The drain accounting counts it as received-but-dropped. } } } From a07fa99aa50ae3357822a5d177d378f6c557806c Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 01:23:04 -0400 Subject: [PATCH 7/7] Review: consume packet after malformed length; swizzle by caller-declared type When a variable-length field claims more bytes than remain, advance decode_pos to the end of the packet so subsequent fields zero-fill -- consistent with the other ran-off-end paths -- rather than parsing the malformed field's payload as later fields. LLMsgVarData::addData() now swizzles with the caller-declared type instead of mType, which is still the default MVT_U8 when an entry is created without addVariable() (pre-existing; identical under the old LLIndexedVector). No effect on little-endian builds. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/llmessagetemplate.cpp | 4 +++- indra/llmessage/lltemplatemessagereader.cpp | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/indra/llmessage/llmessagetemplate.cpp b/indra/llmessage/llmessagetemplate.cpp index cdbde86d0f..730b72744a 100644 --- a/indra/llmessage/llmessagetemplate.cpp +++ b/indra/llmessage/llmessagetemplate.cpp @@ -53,7 +53,9 @@ void LLMsgVarData::addData(const void *data, S32 size, EMsgVariableType type, S3 mData = new U8[size]; dest = mData; } - htolememcpy(dest, data, mType, size); + // swizzle by the caller-declared layout of the incoming data; mType + // may still be the default if this entry skipped addVariable() + htolememcpy(dest, data, type, size); } } diff --git a/indra/llmessage/lltemplatemessagereader.cpp b/indra/llmessage/lltemplatemessagereader.cpp index c215272c16..8c75204070 100644 --- a/indra/llmessage/lltemplatemessagereader.cpp +++ b/indra/llmessage/lltemplatemessagereader.cpp @@ -678,6 +678,10 @@ bool LLTemplateMessageReader::decodeData(const U8* buffer, const LLHost& sender, if (!custom) logRanOffEndOfPacket(sender, decode_pos, (S32)tsize); + // consume the rest of the packet so later fields + // zero-fill (like the other ran-off-end paths) + // instead of parsing the malformed field's payload + decode_pos = mReceiveSize; tsize = 0; }