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/llmessagetemplate.cpp b/indra/llmessage/llmessagetemplate.cpp index 6a0a8e1b91..730b72744a 100644 --- a/indra/llmessage/llmessagetemplate.cpp +++ b/indra/llmessage/llmessagetemplate.cpp @@ -46,8 +46,16 @@ 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; + } + // 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/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/llpacketring.cpp b/indra/llmessage/llpacketring.cpp index 15f0d18b0e..7fdd0e8b25 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 @@ -273,18 +279,30 @@ 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. } } } 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) { 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/lltemplatemessagebuilder.cpp b/indra/llmessage/lltemplatemessagebuilder.cpp index 758d6d343f..758352228c 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 @@ -558,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 84b5373ba4..8c75204070 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,20 +86,21 @@ 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; 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; + return 0; } - LLMsgVarData& vardata = msg_block_data->mMemberVarData[vnamep]; + LLMsgVarData& vardata = *vit; if (size && size != vardata.getSize()) { @@ -108,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; } @@ -125,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 { @@ -135,6 +137,7 @@ void LLTemplateMessageReader::getData(const char *blockname, const char *varname << LL_ENDL; memcpy(datap, vardata.getData(), max_size); + return max_size; } } @@ -194,15 +197,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 +246,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(); } @@ -414,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; } @@ -617,7 +628,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 +670,22 @@ 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); + + // 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; + } + + vardata.addData(&buffer[decode_pos], tsize, mvci.getType()); decode_pos += tsize; } else @@ -673,15 +700,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/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 876feffc79..b186526703 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); @@ -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) { @@ -1248,8 +1255,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 +1377,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 +1428,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 +1445,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 +1734,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 +1743,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 +2938,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); 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; + } }