diff --git a/indra/llmessage/llmessagetemplate.cpp b/indra/llmessage/llmessagetemplate.cpp index 730b72744a..c62fb5fcfa 100644 --- a/indra/llmessage/llmessagetemplate.cpp +++ b/indra/llmessage/llmessagetemplate.cpp @@ -59,22 +59,6 @@ void LLMsgVarData::addData(const void *data, S32 size, EMsgVariableType type, S3 } } -void LLMsgData::addDataFast(char *blockname, char *varname, const void *data, S32 size, EMsgVariableType type, S32 data_size) -{ - // remember that if the blocknumber is > 0 then the number is appended to the name - char *namep = (char *)blockname; - LLMsgBlkData* block_data = mMemberBlocks[namep]; - if (block_data->mBlockNumber) - { - namep += block_data->mBlockNumber; - block_data->addData(varname, data, size, type, data_size); - } - else - { - block_data->addData(varname, data, size, type, data_size); - } -} - // LLMessageVariable functions and friends std::ostream& operator<<(std::ostream& s, LLMessageVariable &msg) diff --git a/indra/llmessage/llmessagetemplate.h b/indra/llmessage/llmessagetemplate.h index f1ccb62153..f81c435715 100644 --- a/indra/llmessage/llmessagetemplate.h +++ b/indra/llmessage/llmessagetemplate.h @@ -93,6 +93,7 @@ class LLMsgVarDataMap const_iterator end() const { return mEntries.end(); } bool empty() const { return mEntries.empty(); } size_t size() const { return mEntries.size(); } + void clear() { mEntries.clear(); } // keeps capacity for reuse // Returns the entry for name, inserting an empty one keyed to name if // absent (std::map::operator[] semantics — repeated misses yield the @@ -149,6 +150,22 @@ class LLMsgBlkData } } + // Reset identity and drop variable data for pooled reuse, keeping the + // variable-map's capacity. Frees any heap payloads first: clearing the + // vector destroys the LLMsgVarData entries with their trivial destructor, + // which does not own mData. + void reinit(char *name, S32 blocknum) + { + for (LLMsgVarData& var : mMemberVarData) + { + var.deleteData(); + } + mMemberVarData.clear(); + mName = name; + mBlockNumber = blocknum; + mTotalSize = -1; + } + LLMsgVarData& addVariable(const char *name, EMsgVariableType type) { LLMsgVarData& entry = mMemberVarData[name]; @@ -169,31 +186,134 @@ class LLMsgBlkData S32 mTotalSize; }; +// One decoded or built message's block data. Each block name owns an ordered +// list of its repeat instances; groups are kept in first-seen order, which is +// the template's block order for both decode and build. +// +// This replaces an older std::map keyed by (canonical name pointer + repeat +// index) pointer arithmetic. String-table names are contiguous 64-byte rows, +// so name + i for i past ~64 walked into the adjacent name's storage; two +// block names within range plus enough repeats aliased keys, overwriting (and +// leaking) blocks and corrupting data. ImprovedTerseObjectUpdate-class +// messages reach ~180 repeats after zerocode expansion, so this was reachable +// from inbound traffic. Grouping by name sidesteps the synthesized key +// entirely and drops the per-block map node. class LLMsgData { public: - LLMsgData(const char *name) : mTotalSize(-1) + struct BlockGroup { - mName = (char *)name; + char* mName; // canonical (string-table) block name + std::vector mBlocks; // repeat instances in order; owned + }; + typedef std::vector msg_blk_data_map_t; + + LLMsgData(const char *name) : mName((char *)name), mTotalSize(-1) + { + mMemberBlocks.reserve(8); } ~LLMsgData() { - for_each(mMemberBlocks.begin(), mMemberBlocks.end(), DeletePairedPointer()); - mMemberBlocks.clear(); + for (BlockGroup& group : mMemberBlocks) + { + for (LLMsgBlkData* blockp : group.mBlocks) + { + delete blockp; + } + } + for (LLMsgBlkData* blockp : mBlockPool) + { + delete blockp; + } } + // Reset for reuse on the next message: recycle every block into the free + // pool and clear the groups, keeping all the vector capacity. Variable + // data on the recycled blocks is freed lazily by allocBlock() on reuse + // (or by the destructor), so a decode after warmup allocates nothing. + void reset(const char *name = nullptr) + { + for (BlockGroup& group : mMemberBlocks) + { + for (LLMsgBlkData* blockp : group.mBlocks) + { + mBlockPool.push_back(blockp); + } + } + mMemberBlocks.clear(); // keeps capacity + mName = (char *)name; + mTotalSize = -1; + } + + // Obtain a block for name+blocknum (from the pool, or freshly allocated) + // and append it as the next repeat of its group; returns it. + LLMsgBlkData* allocBlock(char *name, S32 blocknum) + { + LLMsgBlkData* blockp; + if (!mBlockPool.empty()) + { + blockp = mBlockPool.back(); + mBlockPool.pop_back(); + blockp->reinit(name, blocknum); + } + else + { + blockp = new LLMsgBlkData(name, blocknum); + } + getOrAddGroup(name).mBlocks.push_back(blockp); + return blockp; + } + + // Append blockp as the next repeat of its (canonical) block name. void addBlock(LLMsgBlkData *blockp) { - mMemberBlocks[blockp->mName] = blockp; + getOrAddGroup(blockp->mName).mBlocks.push_back(blockp); } - void addDataFast(char *blockname, char *varname, const void *data, S32 size, EMsgVariableType type, S32 data_size = -1); + bool empty() const { return mMemberBlocks.empty(); } -public: - typedef std::map msg_blk_data_map_t; - msg_blk_data_map_t mMemberBlocks; + // Repeat #blocknum of name, or nullptr if name or that index is absent. + LLMsgBlkData* getBlock(const char* name, S32 blocknum = 0) const + { + const BlockGroup* group = findGroup(name); + if (!group || blocknum < 0 || blocknum >= (S32)group->mBlocks.size()) + { + return nullptr; + } + return group->mBlocks[blocknum]; + } + + msg_blk_data_map_t mMemberBlocks; // groups in template order char *mName; S32 mTotalSize; + +private: + std::vector mBlockPool; // recycled blocks ready for reuse + + BlockGroup& getOrAddGroup(char* name) + { + for (BlockGroup& group : mMemberBlocks) + { + if (group.mName == name) + { + return group; + } + } + mMemberBlocks.push_back(BlockGroup{name, {}}); + return mMemberBlocks.back(); + } + + const BlockGroup* findGroup(const char* name) const + { + for (const BlockGroup& group : mMemberBlocks) + { + if (group.mName == name) + { + return &group; + } + } + return nullptr; + } }; // LLMessage* classes store the template of messages diff --git a/indra/llmessage/llsdmessagebuilder.cpp b/indra/llmessage/llsdmessagebuilder.cpp index 95ab1c1c34..c7a1fafee4 100644 --- a/indra/llmessage/llsdmessagebuilder.cpp +++ b/indra/llmessage/llsdmessagebuilder.cpp @@ -117,8 +117,11 @@ void LLSDMessageBuilder::addBinaryData( S32 size) { std::vector v; - v.resize(size); - memcpy(&(v[0]), reinterpret_cast(data), size); + if (size > 0) + { + v.resize(size); + memcpy(v.data(), reinterpret_cast(data), size); + } (*mCurrentBlock)[varname] = v; } @@ -239,41 +242,22 @@ U32 LLSDMessageBuilder::buildMessage(U8*, U32, U8) void LLSDMessageBuilder::copyFromMessageData(const LLMsgData& data) { - // copy the blocks - // counting variables used to encode multiple block info - S32 block_count = 0; - char* block_name = NULL; - - // loop through msg blocks to loop through variables, totalling up size - // data and filling the new (send) message - LLMsgData::msg_blk_data_map_t::const_iterator iter = - data.mMemberBlocks.begin(); - LLMsgData::msg_blk_data_map_t::const_iterator end = - data.mMemberBlocks.end(); - for(; iter != end; ++iter) + // walk each block group in template order, then each repeat in order, + // starting a fresh block per repeat and converting its variables to LLSD + for (const LLMsgData::BlockGroup& group : data.mMemberBlocks) { - const LLMsgBlkData* mbci = iter->second; - if(!mbci) continue; - - // do we need to encode a block code? - if (block_count == 0) + for (const LLMsgBlkData* mbci : group.mBlocks) { - block_count = mbci->mBlockNumber; - block_name = (char*)mbci->mName; - } - - // counting down mutliple blocks - block_count--; - - nextBlock(block_name); + nextBlock(group.mName); - // now loop through the variables - LLMsgBlkData::msg_var_data_map_t::const_iterator dit = mbci->mMemberVarData.begin(); - LLMsgBlkData::msg_var_data_map_t::const_iterator dend = mbci->mMemberVarData.end(); - - for(; dit != dend; ++dit) - { - const LLMsgVarData& mvci = *dit; + for (const LLMsgVarData& mvci : mbci->mMemberVarData) + { + // a placeholder variable that was never filled has no payload at + // all (null data pointer); there is nothing to convert + if (mvci.getSize() < 0) + { + continue; + } const char* varname = mvci.getName(); switch(mvci.getType()) @@ -284,14 +268,17 @@ void LLSDMessageBuilder::copyFromMessageData(const LLMsgData& data) case MVT_VARIABLE: { - const char end = ((const char*)mvci.getData())[mvci.getSize()-1]; // Ensure null terminated + // an empty field stores no payload at all (getData() may + // be null), so don't probe for a terminating NUL + const S32 size = mvci.getSize(); + const char end = size > 0 ? ((const char*)mvci.getData())[size-1] : 1; if (mvci.getDataSize() == 1 && end == 0) { addString(varname, (const char*)mvci.getData()); } else { - addBinaryData(varname, mvci.getData(), mvci.getSize()); + addBinaryData(varname, mvci.getData(), size); } break; } @@ -392,6 +379,7 @@ void LLSDMessageBuilder::copyFromMessageData(const LLMsgData& data) LL_WARNS() << "Unknown type in conversion of message to LLSD" << LL_ENDL; break; } + } } } } diff --git a/indra/llmessage/lltemplatemessagebuilder.cpp b/indra/llmessage/lltemplatemessagebuilder.cpp index 46c11251af..69d9ed9365 100644 --- a/indra/llmessage/lltemplatemessagebuilder.cpp +++ b/indra/llmessage/lltemplatemessagebuilder.cpp @@ -64,14 +64,21 @@ void LLTemplateMessageBuilder::newMessage(const char *name) mCurrentSendTotal = 0; - delete mCurrentSMessageData; - mCurrentSMessageData = NULL; - char* namep = (char*)name; if (mMessageTemplates.count(namep) > 0) { mCurrentSMessageTemplate = mMessageTemplates.find(name)->second; - mCurrentSMessageData = new LLMsgData(namep); + // reuse the working data set across messages; reset recycles blocks + // into its pool and keeps capacity, so steady-state sends (e.g. + // AgentUpdate every frame) allocate nothing. + if (!mCurrentSMessageData) + { + mCurrentSMessageData = new LLMsgData(namep); + } + else + { + mCurrentSMessageData->reset(namep); + } mCurrentSMessageName = namep; mCurrentSDataBlock = NULL; mCurrentSBlockName = NULL; @@ -90,8 +97,7 @@ void LLTemplateMessageBuilder::newMessage(const char *name) ++iter) { LLMessageBlock* ci = *iter; - LLMsgBlkData* tblockp = new LLMsgBlkData(ci->mName, 0); - mCurrentSMessageData->addBlock(tblockp); + mCurrentSMessageData->allocBlock(ci->mName, 0); } } else @@ -110,8 +116,13 @@ void LLTemplateMessageBuilder::clearMessage() mCurrentSMessageTemplate = NULL; - delete mCurrentSMessageData; - mCurrentSMessageData = NULL; + // keep mCurrentSMessageData (and its block pool) alive for reuse; reset + // recycles its blocks so nothing stale remains. Use is gated by + // mCurrentSMessageTemplate (now null) until the next newMessage(). + if (mCurrentSMessageData) + { + mCurrentSMessageData->reset(); + } mCurrentSMessageName = NULL; mCurrentSDataBlock = NULL; @@ -138,8 +149,9 @@ void LLTemplateMessageBuilder::nextBlock(const char* blockname) return; } - // ok, have we already set this block? - LLMsgBlkData* block_data = mCurrentSMessageData->mMemberBlocks[bnamep]; + // ok, have we already set this block? newMessage pre-added a placeholder + // for every template block, so getBlock(name, 0) is the group header. + LLMsgBlkData* block_data = mCurrentSMessageData->getBlock(bnamep, 0); if (block_data->mBlockNumber == 0) { // nope! set this as the current block @@ -171,22 +183,18 @@ void LLTemplateMessageBuilder::nextBlock(const char* blockname) // if the block is type MBT_MULTIPLE then we need a known number, - // make sure that we're not exceeding it + // make sure that we're not exceeding it (the header tracks the count) if ( (template_data->mType == MBT_MULTIPLE) - &&(mCurrentSDataBlock->mBlockNumber == template_data->mNumber)) + &&(block_data->mBlockNumber == template_data->mNumber)) { LL_ERRS() << "LLTemplateMessageBuilder::nextBlock called " - << mCurrentSDataBlock->mBlockNumber << " times for " << bnamep + << block_data->mBlockNumber << " times for " << bnamep << " exceeding " << template_data->mNumber << " specified in type MBT_MULTIPLE." << LL_ENDL; return; } - // ok, we can make a new one - // modify the name to avoid name collision by adding number to end - S32 count = block_data->mBlockNumber; - - // incrememt base name's count + // ok, we can make a new one; bump the count tracked on the header block_data->mBlockNumber++; if (block_data->mBlockNumber > MAX_BLOCKS) @@ -195,16 +203,10 @@ void LLTemplateMessageBuilder::nextBlock(const char* blockname) << "(limited to " << MAX_BLOCKS << ")" << LL_ENDL; } - // create new name - // Nota Bene: if things are working correctly, - // mCurrentMessageData->mMemberBlocks[blockname]->mBlockNumber == - // mCurrentDataBlock->mBlockNumber + 1 - - char *nbnamep = bnamep + count; - - mCurrentSDataBlock = new LLMsgBlkData(bnamep, count); - mCurrentSDataBlock->mName = nbnamep; - mCurrentSMessageData->mMemberBlocks[nbnamep] = mCurrentSDataBlock; + // append the new repeat under the canonical name; allocBlock pulls + // from the message's pool, groups it, and getBlock(name, i) recovers + // it by index + mCurrentSDataBlock = mCurrentSMessageData->allocBlock(bnamep, block_data->mBlockNumber - 1); // add placeholders for each of the variables for (LLMessageBlock::message_variable_map_t::const_iterator @@ -549,7 +551,8 @@ bool LLTemplateMessageBuilder::isMessageFull(const char* blockname) const max = MAX_BLOCKS; break; } - if(mCurrentSMessageData->mMemberBlocks[bnamep]->mBlockNumber >= max) + const LLMsgBlkData* header = mCurrentSMessageData->getBlock(bnamep, 0); + if(header && header->mBlockNumber >= max) { return true; } @@ -559,17 +562,14 @@ bool LLTemplateMessageBuilder::isMessageFull(const char* blockname) const static S32 buildBlock(U8* buffer, S32 buffer_size, const LLMessageBlock* template_data, LLMsgData* message_data) { S32 result = 0; - LLMsgData::msg_blk_data_map_t::const_iterator block_iter = message_data->mMemberBlocks.find(template_data->mName); - const LLMsgBlkData* mbci = block_iter->second; - - // ok, if this is the first block of a repeating pack, set - // block_count and, if it's type MBT_VARIABLE encode a byte - // for how many there are - S32 block_count = mbci->mBlockNumber; + // header (repeat 0) carries the repeat count; repeats are recovered by + // index via getBlock(name, i) + const LLMsgBlkData* header = message_data->getBlock(template_data->mName, 0); + S32 block_count = header ? header->mBlockNumber : 0; if (template_data->mType == MBT_VARIABLE) { // remember that mBlockNumber is a S32 - U8 temp_block_number = (U8)mbci->mBlockNumber; + U8 temp_block_number = (U8)block_count; if ((S32)(result + sizeof(U8)) < MAX_BUFFER_SIZE) { memcpy(&buffer[result], &temp_block_number, sizeof(U8)); @@ -589,15 +589,17 @@ static S32 buildBlock(U8* buffer, S32 buffer_size, const LLMessageBlock* templat if (block_count != template_data->mNumber) { // nope! need to fill it in all the way! - LL_ERRS() << "Block " << mbci->mName + LL_ERRS() << "Block " << template_data->mName << " is type MBT_MULTIPLE but only has data for " << block_count << " out of its " << template_data->mNumber << " blocks" << LL_ENDL; } } - while(block_count > 0) + for (S32 block_index = 0; block_index < block_count; ++block_index) { + const LLMsgBlkData* mbci = message_data->getBlock(template_data->mName, block_index); + // now loop through the variables for (LLMsgBlkData::msg_var_data_map_t::const_iterator iter = mbci->mMemberVarData.begin(); iter != mbci->mMemberVarData.end(); iter++) @@ -668,16 +670,6 @@ static S32 buildBlock(U8* buffer, S32 buffer_size, const LLMessageBlock* templat } } - --block_count; - - if (block_iter != message_data->mMemberBlocks.end()) - { - ++block_iter; - if (block_iter != message_data->mMemberBlocks.end()) - { - mbci = block_iter->second; - } - } } return result; @@ -767,42 +759,24 @@ U32 LLTemplateMessageBuilder::buildMessage( void LLTemplateMessageBuilder::copyFromMessageData(const LLMsgData& data) { - // copy the blocks - // counting variables used to encode multiple block info - S32 block_count = 0; - char *block_name = NULL; - - // loop through msg blocks to loop through variables, totalling up size - // data and filling the new (send) message - LLMsgData::msg_blk_data_map_t::const_iterator iter = - data.mMemberBlocks.begin(); - LLMsgData::msg_blk_data_map_t::const_iterator end = - data.mMemberBlocks.end(); - for(; iter != end; ++iter) + // walk each block group in template order, then each repeat in order, + // starting a fresh block per repeat and copying its variables + for (const LLMsgData::BlockGroup& group : data.mMemberBlocks) { - const LLMsgBlkData* mbci = iter->second; - if(!mbci) continue; - - // do we need to encode a block code? - if (block_count == 0) + for (const LLMsgBlkData* mbci : group.mBlocks) { - block_count = mbci->mBlockNumber; - block_name = (char *)mbci->mName; - } - - // counting down mutliple blocks - block_count--; + nextBlock(group.mName); - nextBlock(block_name); - - // now loop through the variables - LLMsgBlkData::msg_var_data_map_t::const_iterator dit = mbci->mMemberVarData.begin(); - LLMsgBlkData::msg_var_data_map_t::const_iterator dend = mbci->mMemberVarData.end(); - - for(; dit != dend; ++dit) - { - const LLMsgVarData& mvci = *dit; - addData(mvci.getName(), mvci.getData(), mvci.getType(), mvci.getSize()); + for (const LLMsgVarData& mvci : mbci->mMemberVarData) + { + // a placeholder variable that was never filled has no payload + // at all (null data pointer); there is nothing to copy + if (mvci.getSize() < 0) + { + continue; + } + addData(mvci.getName(), mvci.getData(), mvci.getType(), mvci.getSize()); + } } } } diff --git a/indra/llmessage/lltemplatemessagereader.cpp b/indra/llmessage/lltemplatemessagereader.cpp index 8c75204070..012f6bfea0 100644 --- a/indra/llmessage/lltemplatemessagereader.cpp +++ b/indra/llmessage/lltemplatemessagereader.cpp @@ -58,8 +58,13 @@ void LLTemplateMessageReader::clearMessage() { mReceiveSize = -1; mCurrentRMessageTemplate = nullptr; - delete mCurrentRMessageData; - mCurrentRMessageData = nullptr; + // Keep mCurrentRMessageData (and its block pool) alive for reuse; just + // recycle its blocks so nothing stale remains. mReceiveSize == -1 makes + // every getter bail until the next decode repopulates it. + if (mCurrentRMessageData) + { + mCurrentRMessageData->reset(); + } } S32 LLTemplateMessageReader::getData(const char *blockname, const char *varname, void *datap, S32 size, S32 blocknum, S32 max_size) @@ -77,26 +82,24 @@ S32 LLTemplateMessageReader::getData(const char *blockname, const char *varname, return 0; } - char *bnamep = (char *)blockname + blocknum; // this works because it's just a hash. The bnamep is never derefference char *vnamep = (char *)varname; - LLMsgData::msg_blk_data_map_t::const_iterator iter = mCurrentRMessageData->mMemberBlocks.find(bnamep); + LLMsgBlkData *msg_block_data = mCurrentRMessageData->getBlock(blockname, blocknum); - if (iter == mCurrentRMessageData->mMemberBlocks.end()) + if (!msg_block_data) { LL_ERRS() << "Block " << blockname << " #" << blocknum << " not in message " << mCurrentRMessageData->mName << LL_ENDL; return 0; } - LLMsgBlkData *msg_block_data = iter->second; LLMsgBlkData::msg_var_data_map_t &var_data_map = msg_block_data->mMemberVarData; 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; + << mCurrentRMessageData->mName<< " block " << blockname << LL_ENDL; return 0; } @@ -156,16 +159,8 @@ S32 LLTemplateMessageReader::getNumberOfBlocks(const char *blockname) return -1; } - char *bnamep = (char *)blockname; - - LLMsgData::msg_blk_data_map_t::const_iterator iter = mCurrentRMessageData->mMemberBlocks.find(bnamep); - - if (iter == mCurrentRMessageData->mMemberBlocks.end()) - { - return 0; - } - - return (iter->second)->mBlockNumber; + LLMsgBlkData* msg_data = mCurrentRMessageData->getBlock(blockname, 0); + return msg_data ? msg_data->mBlockNumber : 0; } S32 LLTemplateMessageReader::getSize(const char *blockname, const char *varname) @@ -183,34 +178,35 @@ S32 LLTemplateMessageReader::getSize(const char *blockname, const char *varname) return LL_MESSAGE_ERROR; } - char *bnamep = (char *)blockname; - - LLMsgData::msg_blk_data_map_t::const_iterator iter = mCurrentRMessageData->mMemberBlocks.find(bnamep); + LLMsgBlkData* msg_data = mCurrentRMessageData->getBlock(blockname, 0); - if (iter == mCurrentRMessageData->mMemberBlocks.end()) + if (!msg_data) { // don't crash - LL_INFOS() << "Block " << bnamep << " not in message " + LL_INFOS() << "Block " << blockname << " not in message " << mCurrentRMessageData->mName << LL_ENDL; return LL_BLOCK_NOT_IN_MESSAGE; } char *vnamep = (char *)varname; - LLMsgBlkData* msg_data = iter->second; LLMsgBlkData::msg_var_data_map_t::iterator vit = msg_data->mMemberVarData.find(vnamep); if (vit == msg_data->mMemberVarData.end()) { // don't crash LL_INFOS() << "Variable " << varname << " not in message " - << mCurrentRMessageData->mName << " block " << bnamep << LL_ENDL; + << mCurrentRMessageData->mName << " block " << blockname << LL_ENDL; return LL_VARIABLE_NOT_IN_BLOCK; } LLMsgVarData& vardata = *vit; - if (mCurrentRMessageTemplate->mMemberBlocks[bnamep]->mType != MBT_SINGLE) + // const pointer so the find-based getBlock() overload is chosen rather + // than the one that inserts into the template on a miss + const LLMessageTemplate* template_data = mCurrentRMessageTemplate; + const LLMessageBlock* template_block = template_data->getBlock((char*)blockname); + if (template_block && template_block->mType != MBT_SINGLE) { // This is a serious error - crash - LL_ERRS() << "Block " << bnamep << " isn't type MBT_SINGLE," + LL_ERRS() << "Block " << blockname << " isn't type MBT_SINGLE," " use getSize with blocknum argument!" << LL_ENDL; return LL_MESSAGE_ERROR; } @@ -233,25 +229,23 @@ S32 LLTemplateMessageReader::getSize(const char *blockname, S32 blocknum, const return LL_MESSAGE_ERROR; } - char *bnamep = (char *)blockname + blocknum; char *vnamep = (char *)varname; - LLMsgData::msg_blk_data_map_t::const_iterator iter = mCurrentRMessageData->mMemberBlocks.find(bnamep); + LLMsgBlkData* msg_data = mCurrentRMessageData->getBlock(blockname, blocknum); - if (iter == mCurrentRMessageData->mMemberBlocks.end()) + if (!msg_data) { // don't crash - LL_INFOS() << "Block " << bnamep << " not in message " + LL_INFOS() << "Block " << blockname << " #" << blocknum << " not in message " << mCurrentRMessageData->mName << LL_ENDL; return LL_BLOCK_NOT_IN_MESSAGE; } - LLMsgBlkData* msg_data = iter->second; LLMsgBlkData::msg_var_data_map_t::iterator vit = msg_data->mMemberVarData.find(vnamep); if (vit == msg_data->mMemberVarData.end()) { // don't crash LL_INFOS() << "Variable " << vnamep << " not in message " - << mCurrentRMessageData->mName << " block " << bnamep << LL_ENDL; + << mCurrentRMessageData->mName << " block " << blockname << LL_ENDL; return LL_VARIABLE_NOT_IN_BLOCK; } @@ -540,16 +534,23 @@ bool LLTemplateMessageReader::decodeData(const U8* buffer, const LLHost& sender, llassert( mReceiveSize >= 0 ); llassert( mCurrentRMessageTemplate); - llassert( !mCurrentRMessageData ); - delete mCurrentRMessageData; // just to make sure // The offset tells us how may bytes to skip after the end of the // message name. U8 offset = buffer[PHL_OFFSET]; S32 decode_pos = LL_PACKET_ID_SIZE + (S32)(mCurrentRMessageTemplate->mFrequency) + offset; - // create base working data set - mCurrentRMessageData = new LLMsgData(mCurrentRMessageTemplate->mName); + // Reuse the working data set across packets: it's reset (blocks recycled + // into its pool) rather than freed, so a steady-state decode does no heap + // allocation. + if (!mCurrentRMessageData) + { + mCurrentRMessageData = new LLMsgData(mCurrentRMessageTemplate->mName); + } + else + { + mCurrentRMessageData->reset(mCurrentRMessageTemplate->mName); + } // loop through the template building the data structure as we go LLMessageTemplate::message_block_map_t::const_iterator iter; @@ -604,20 +605,12 @@ bool LLTemplateMessageReader::decodeData(const U8* buffer, const LLHost& sender, // now loop through the block for (i = 0; i < repeat_number; i++) { - if (i) - { - // build new name to prevent collisions - // TODO: This should really change to a vector - cur_data_block = new LLMsgBlkData(mbci->mName, repeat_number); - cur_data_block->mName = mbci->mName + i; - } - else - { - cur_data_block = new LLMsgBlkData(mbci->mName, repeat_number); - } - - // add the block to the message - mCurrentRMessageData->addBlock(cur_data_block); + // Every repeat keeps the canonical block name; allocBlock groups + // them by name and preserves order, so getBlock(name, i) recovers + // repeat i. mBlockNumber carries the total repeat count (read by + // getNumberOfBlocks from repeat 0). Blocks come from the message's + // pool, so repeated decodes reuse them. + cur_data_block = mCurrentRMessageData->allocBlock(mbci->mName, repeat_number); // now read the variables for (LLMessageBlock::message_variable_map_t::const_iterator iter = @@ -714,7 +707,7 @@ bool LLTemplateMessageReader::decodeData(const U8* buffer, const LLHost& sender, } } - if (mCurrentRMessageData->mMemberBlocks.empty() + if (mCurrentRMessageData->empty() && !mCurrentRMessageTemplate->mMemberBlocks.empty()) { LL_DEBUGS() << "Empty message '" << mCurrentRMessageTemplate->mName << "' (no blocks)" << LL_ENDL; diff --git a/indra/llmessage/tests/llsdmessagebuilder_test.cpp b/indra/llmessage/tests/llsdmessagebuilder_test.cpp index 6804c3e29e..7dcc03edc1 100644 --- a/indra/llmessage/tests/llsdmessagebuilder_test.cpp +++ b/indra/llmessage/tests/llsdmessagebuilder_test.cpp @@ -833,5 +833,49 @@ namespace tut ensure_equals(output["Test0"][0]["Test0"].asString(), std::string("testing")); } + template<> template<> + void LLSDMessageBuilderTestObject::test<47>() + // an empty MVT_VARIABLE field stores no payload (getData() is null), + // so the legacy-to-llsd copy must not probe it for a terminating NUL + // and must yield an empty value instead of reading out of bounds + { + addValue(messageBlockData, (char*)"testBinData", nullptr, MVT_VARIABLE, 0, 1); + messageData->addBlock(messageBlockData); + LLSDMessageBuilder builder = defaultBuilder(); + + builder.copyFromMessageData(*messageData); + LLSD output = builder.getMessage(); + + ensure("Ensure empty variable copied from legacy to llsd is present", + output["testBlock"][0]["testBinData"].isDefined()); + ensure_equals("Ensure empty variable copied from legacy to llsd is empty", + output["testBlock"][0]["testBinData"].asBinary().size(), (size_t)0); + } + + template<> template<> + void LLSDMessageBuilderTestObject::test<48>() + // a placeholder variable in an opened block that was never filled + // (size -1) has no payload at all; the legacy-to-llsd copy must skip + // it rather than read through its null data pointer + { + LLMessageTemplate messageTemplate = defaultTemplate(); + LLMessageBlock* block = new LLMessageBlock(_PREHASH_Test0, MBT_VARIABLE); + block->addVariable(const_cast(_PREHASH_Test0), MVT_U8, 1); + block->addVariable(const_cast(_PREHASH_Test1), MVT_U8, 1); + messageTemplate.addBlock(block); + + LLTemplateMessageBuilder* builder = defaultTemplateBuilder(messageTemplate); + builder->addU8(_PREHASH_Test0, 7); // Test1 left as a placeholder + + LLSDMessageBuilder sd_builder; + sd_builder.copyFromMessageData(*builder->getCurrentMessage()); + LLSD output = sd_builder.getMessage(); + + ensure_equals("Ensure filled variable copied", + output["Test0"][0]["Test0"].asInteger(), 7); + ensure("Ensure unfilled placeholder variable not copied", + !output["Test0"][0].has("Test1")); + } + } diff --git a/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp b/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp index 074e15dcc2..e2bf97803e 100644 --- a/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp +++ b/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp @@ -118,7 +118,7 @@ namespace tut }; - typedef test_group LLTemplateMessageBuilderTestGroup; + typedef test_group LLTemplateMessageBuilderTestGroup; typedef LLTemplateMessageBuilderTestGroup::object LLTemplateMessageBuilderTestObject; LLTemplateMessageBuilderTestGroup templateMessageBuilderTestGroup("LLTemplateMessageBuilder"); @@ -1022,5 +1022,160 @@ namespace tut strlen(outBuffer), (size_t)254); delete reader; } + + template<> template<> + void LLTemplateMessageBuilderTestObject::test<48>() + // A variable block repeated far past 64 times, alongside a second + // block, all round-trip intact. The old data model keyed each repeat + // by (block-name pointer + repeat index), walking the string table; + // past ~64 repeats those keys ran into an adjacent block name and + // aliased, corrupting and leaking blocks. Grouping repeats by name + // removes the synthesized key. ImprovedTerseObjectUpdate-class + // messages reach this repeat count after zerocode expansion. + { + LLMessageTemplate messageTemplate = defaultTemplate(); + messageTemplate.addBlock(defaultBlock(MVT_U32, 4, MBT_VARIABLE)); // Test0 + messageTemplate.addBlock(createBlock(const_cast(_PREHASH_Test1), + MVT_U32, 4, MBT_SINGLE)); // Test1 + + const S32 NUM_BLOCKS = 200; + LLTemplateMessageBuilder* builder = defaultBuilder(messageTemplate); // nextBlock(Test0) + builder->addU32(_PREHASH_Test0, 0); + for (S32 i = 1; i < NUM_BLOCKS; ++i) + { + builder->nextBlock(_PREHASH_Test0); + builder->addU32(_PREHASH_Test0, (U32)i); + } + // createBlock always names the block's variable Test0, regardless of + // the block name, so the Test1 block's variable is Test0 too + builder->nextBlock(_PREHASH_Test1); + builder->addU32(_PREHASH_Test0, 0xdeadbeef); + + LLTemplateMessageReader* reader = setReader(messageTemplate, builder); + + ensure_equals("variable block count", + reader->getNumberOfBlocks(_PREHASH_Test0), NUM_BLOCKS); + for (S32 i = 0; i < NUM_BLOCKS; ++i) + { + U32 value = 0xffffffff; + reader->getU32(_PREHASH_Test0, _PREHASH_Test0, value, i); + ensure_equals("variable block repeat intact", value, (U32)i); + } + U32 single = 0; + reader->getU32(_PREHASH_Test1, _PREHASH_Test0, single); + ensure_equals("adjacent single block intact", single, (U32)0xdeadbeef); + delete reader; + } + + template<> template<> + void LLTemplateMessageBuilderTestObject::test<49>() + // Decode several messages through one reader to exercise the pooled + // LLMsgData reuse path: blocks recycle between decodes and no stale + // block or value leaks across messages of differing repeat counts. + { + LLMessageTemplate messageTemplate = defaultTemplate(); + messageTemplate.addBlock(defaultBlock(MVT_U32, 4, MBT_VARIABLE)); // Test0 + numberMap[1] = &messageTemplate; + + const U32 cap = 1024; + U8 bufA[cap]; + U8 bufB[cap]; + + // bufA: 5 repeats valued 100..104; bufB: 2 repeats valued 900..901 + U32 szA, szB; + { + LLTemplateMessageBuilder* b = defaultBuilder(messageTemplate); // nextBlock(Test0) + b->addU32(_PREHASH_Test0, 100); + for (S32 i = 1; i < 5; ++i) + { + b->nextBlock(_PREHASH_Test0); + b->addU32(_PREHASH_Test0, 100 + (U32)i); + } + memset(bufA, 0, LL_PACKET_ID_SIZE); + szA = b->buildMessage(bufA, cap, 0); + delete b; + } + { + LLTemplateMessageBuilder* b = defaultBuilder(messageTemplate); + b->addU32(_PREHASH_Test0, 900); + b->nextBlock(_PREHASH_Test0); + b->addU32(_PREHASH_Test0, 901); + memset(bufB, 0, LL_PACKET_ID_SIZE); + szB = b->buildMessage(bufB, cap, 0); + delete b; + } + + // alternate the two through one reader; reuse must not corrupt either + LLTemplateMessageReader reader(numberMap); + for (S32 pass = 0; pass < 4; ++pass) + { + bool useA = (pass % 2) == 0; + U8* buf = useA ? bufA : bufB; + U32 sz = useA ? szA : szB; + S32 count = useA ? 5 : 2; + U32 base = useA ? 100u : 900u; + + reader.validateMessage(buf, sz, LLHost()); + reader.readMessage(buf, LLHost()); + + ensure_equals("reuse block count", + reader.getNumberOfBlocks(_PREHASH_Test0), count); + for (S32 i = 0; i < count; ++i) + { + U32 v = 0xffffffff; + reader.getU32(_PREHASH_Test0, _PREHASH_Test0, v, i); + ensure_equals("reuse repeat value", v, base + (U32)i); + } + reader.clearMessage(); + } + } + + template<> template<> + void LLTemplateMessageBuilderTestObject::test<50>() + // Reuse one builder across messages to exercise the pooled LLMsgData + // on the send side: the second build must not inherit blocks or values + // from the first. + { + LLMessageTemplate messageTemplate = defaultTemplate(); + messageTemplate.addBlock(defaultBlock(MVT_U32, 4, MBT_VARIABLE)); // Test0 + nameMap[_PREHASH_TestMessage] = &messageTemplate; + numberMap[1] = &messageTemplate; + + LLTemplateMessageBuilder* builder = new LLTemplateMessageBuilder(nameMap); + + const U32 cap = 1024; + for (S32 pass = 0; pass < 4; ++pass) + { + S32 count = (pass % 2 == 0) ? 5 : 2; + U32 base = (pass % 2 == 0) ? 100u : 900u; + + builder->newMessage(_PREHASH_TestMessage); + builder->nextBlock(_PREHASH_Test0); + builder->addU32(_PREHASH_Test0, base); + for (S32 i = 1; i < count; ++i) + { + builder->nextBlock(_PREHASH_Test0); + builder->addU32(_PREHASH_Test0, base + (U32)i); + } + + U8 buf[cap]; + memset(buf, 0, LL_PACKET_ID_SIZE); + U32 sz = builder->buildMessage(buf, cap, 0); + + LLTemplateMessageReader reader(numberMap); + reader.validateMessage(buf, sz, LLHost()); + reader.readMessage(buf, LLHost()); + ensure_equals("builder reuse block count", + reader.getNumberOfBlocks(_PREHASH_Test0), count); + for (S32 i = 0; i < count; ++i) + { + U32 v = 0xffffffff; + reader.getU32(_PREHASH_Test0, _PREHASH_Test0, v, i); + ensure_equals("builder reuse value", v, base + (U32)i); + } + builder->clearMessage(); + } + delete builder; + } }