From c3043de6d2f68aefed79d025c228cd024c1a8720 Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 04:26:56 -0400 Subject: [PATCH 1/5] Message data: group block repeats by name instead of pointer-keying LLMsgData kept each decoded/built block in a std::map keyed by the block name's string-table pointer plus the repeat index (name + blocknum). 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 -- the long-standing "should really change to a vector" TODO. Give each block name an ordered vector of its repeat instances, with the groups themselves held in first-seen (template) order. getBlock(name, i) recovers repeat i by position, so no pointer outside a real object is ever formed and the per-repeat map node is gone. buildBlock() indexes repeats directly instead of walking ++map_iter and assuming the synthesized keys sort adjacently; both copyFromMessageData() paths iterate groups x repeats. The builder still tracks its instance count on the group header (mBlockNumber) to preserve the declared-but-empty placeholder semantics newMessage()/buildBlock() rely on. LLMsgBlkData stays heap-allocated and pointer-stable so the cached mCurrentSDataBlock / cur_data_block / mbci pointers can't dangle across a vector growth; pooling those objects is a separate follow-up. Drop dead LLMsgData::addDataFast (no callers, carried its own copy of the synthesized-pointer logic). Reader getSize() now uses the find-based const getBlock() template overload so a missing block can't insert into the template. Add a regression test: a variable block repeated 200 times alongside a second block, round-tripped, verifying every repeat and the neighbor are intact. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/llmessagetemplate.cpp | 16 --- indra/llmessage/llmessagetemplate.h | 78 +++++++++++-- indra/llmessage/llsdmessagebuilder.cpp | 40 ++----- indra/llmessage/lltemplatemessagebuilder.cpp | 104 ++++++------------ indra/llmessage/lltemplatemessagereader.cpp | 67 +++++------ .../tests/lltemplatemessagebuilder_test.cpp | 44 ++++++++ 6 files changed, 177 insertions(+), 172 deletions(-) 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..2d7845156e 100644 --- a/indra/llmessage/llmessagetemplate.h +++ b/indra/llmessage/llmessagetemplate.h @@ -169,31 +169,91 @@ 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; + } + } } + // 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: + 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..4dccc068dc 100644 --- a/indra/llmessage/llsdmessagebuilder.cpp +++ b/indra/llmessage/llsdmessagebuilder.cpp @@ -239,41 +239,16 @@ 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) + { const char* varname = mvci.getName(); switch(mvci.getType()) @@ -392,6 +367,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..559fb4a0d3 100644 --- a/indra/llmessage/lltemplatemessagebuilder.cpp +++ b/indra/llmessage/lltemplatemessagebuilder.cpp @@ -138,8 +138,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 +172,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 +192,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; addBlock groups it + // and getBlock(name, i) recovers it by index + mCurrentSDataBlock = new LLMsgBlkData(bnamep, block_data->mBlockNumber - 1); + mCurrentSMessageData->addBlock(mCurrentSDataBlock); // add placeholders for each of the variables for (LLMessageBlock::message_variable_map_t::const_iterator @@ -549,7 +540,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 +551,15 @@ 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); + const LLMsgBlkData* mbci = header; + 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)); @@ -596,8 +586,10 @@ static S32 buildBlock(U8* buffer, S32 buffer_size, const LLMessageBlock* templat } } - while(block_count > 0) + for (S32 block_index = 0; block_index < block_count; ++block_index) { + 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 +660,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 +749,18 @@ 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(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; - addData(mvci.getName(), mvci.getData(), mvci.getType(), mvci.getSize()); + for (const LLMsgVarData& mvci : mbci->mMemberVarData) + { + addData(mvci.getName(), mvci.getData(), mvci.getType(), mvci.getSize()); + } } } } diff --git a/indra/llmessage/lltemplatemessagereader.cpp b/indra/llmessage/lltemplatemessagereader.cpp index 8c75204070..a509e40d30 100644 --- a/indra/llmessage/lltemplatemessagereader.cpp +++ b/indra/llmessage/lltemplatemessagereader.cpp @@ -77,26 +77,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 +154,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 +173,35 @@ S32 LLTemplateMessageReader::getSize(const char *blockname, const char *varname) return LL_MESSAGE_ERROR; } - char *bnamep = (char *)blockname; + LLMsgBlkData* msg_data = mCurrentRMessageData->getBlock(blockname, 0); - LLMsgData::msg_blk_data_map_t::const_iterator iter = mCurrentRMessageData->mMemberBlocks.find(bnamep); - - 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 +224,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; } @@ -604,17 +593,11 @@ 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); - } + // Every repeat keeps the canonical block name; addBlock 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). + cur_data_block = new LLMsgBlkData(mbci->mName, repeat_number); // add the block to the message mCurrentRMessageData->addBlock(cur_data_block); @@ -714,7 +697,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/lltemplatemessagebuilder_test.cpp b/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp index 074e15dcc2..0f48c4dbf5 100644 --- a/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp +++ b/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp @@ -1022,5 +1022,49 @@ 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; + } } From a13afbb3b8c1b91d23311dd5cc526b5078e30274 Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 04:46:17 -0400 Subject: [PATCH 2/5] Message decode: pool and reuse the per-packet LLMsgData graph The template reader freed and reallocated its whole working data set every packet -- new LLMsgData plus a new LLMsgBlkData (each eagerly reserving a variable vector) per block repeat, all torn down again at clearMessage(). Under object-update load that is the hottest allocation path in the viewer (ImprovedTerseObjectUpdate-class messages carry ~180 repeats). Keep the reader's LLMsgData alive across packets and reset it in place instead. reset() recycles every block into an internal free pool and clears the groups while keeping all vector capacity; allocBlock() hands blocks back out from the pool (reinit clears the prior variable data, freeing any heap payloads). After warmup a steady-state decode performs no heap allocation. clearMessage() now recycles rather than deletes; every getter already gates on mReceiveSize == -1, so the reset-but-alive object is never read stale. LLMsgBlkData stays heap-allocated and pointer-stable, so cached mCurrentRMessageData / cur_data_block pointers within a decode are unaffected. Add a reuse-path test: alternate a 5-repeat and a 2-repeat message through one reader, verifying counts and values each pass so no stale block or value leaks across the differing repeat counts. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/llmessagetemplate.h | 60 ++++++++++++++++++ indra/llmessage/lltemplatemessagereader.cpp | 34 ++++++---- .../tests/lltemplatemessagebuilder_test.cpp | 63 +++++++++++++++++++ 3 files changed, 145 insertions(+), 12 deletions(-) diff --git a/indra/llmessage/llmessagetemplate.h b/indra/llmessage/llmessagetemplate.h index 2d7845156e..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]; @@ -204,6 +221,47 @@ class LLMsgData 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. @@ -230,6 +288,8 @@ class LLMsgData S32 mTotalSize; private: + std::vector mBlockPool; // recycled blocks ready for reuse + BlockGroup& getOrAddGroup(char* name) { for (BlockGroup& group : mMemberBlocks) diff --git a/indra/llmessage/lltemplatemessagereader.cpp b/indra/llmessage/lltemplatemessagereader.cpp index a509e40d30..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) @@ -529,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; @@ -593,14 +605,12 @@ bool LLTemplateMessageReader::decodeData(const U8* buffer, const LLHost& sender, // now loop through the block for (i = 0; i < repeat_number; i++) { - // Every repeat keeps the canonical block name; addBlock groups + // 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). - cur_data_block = new LLMsgBlkData(mbci->mName, repeat_number); - - // add the block to the message - mCurrentRMessageData->addBlock(cur_data_block); + // 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 = diff --git a/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp b/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp index 0f48c4dbf5..2771acf7e2 100644 --- a/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp +++ b/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp @@ -1066,5 +1066,68 @@ namespace tut 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(); + } + } } From 0427d50a71f66a8192966cefe4ef78ad2bb12cb7 Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 04:52:01 -0400 Subject: [PATCH 3/5] Message build: pool and reuse the outbound LLMsgData graph too LLTemplateMessageBuilder reallocated its working data set on every newMessage() -- a fresh LLMsgData plus one LLMsgBlkData per template block, freed again at clearMessage(). AgentUpdate and friends go out every frame, so this mirrors the inbound decode allocation churn. Reuse the builder's LLMsgData the same way the reader now does: reset it in place in newMessage() (and clearMessage()) instead of delete+new, and draw block instances from its pool via allocBlock(). Use is gated by mCurrentSMessageTemplate, which clearMessage() nulls, so the reset-but-alive object is never touched between messages. Add a test that drives one builder through alternating 5- and 2-block messages, round-tripping each, so a reused build can't inherit blocks or values from the previous one. Bump the group's max-tests headroom. Co-Authored-By: Claude Fable 5 Signed-off-by: Rye --- indra/llmessage/lltemplatemessagebuilder.cpp | 35 ++++++++----- .../tests/lltemplatemessagebuilder_test.cpp | 50 ++++++++++++++++++- 2 files changed, 72 insertions(+), 13 deletions(-) diff --git a/indra/llmessage/lltemplatemessagebuilder.cpp b/indra/llmessage/lltemplatemessagebuilder.cpp index 559fb4a0d3..c1956a11d6 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; @@ -192,10 +203,10 @@ void LLTemplateMessageBuilder::nextBlock(const char* blockname) << "(limited to " << MAX_BLOCKS << ")" << LL_ENDL; } - // append the new repeat under the canonical name; addBlock groups it - // and getBlock(name, i) recovers it by index - mCurrentSDataBlock = new LLMsgBlkData(bnamep, block_data->mBlockNumber - 1); - mCurrentSMessageData->addBlock(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 diff --git a/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp b/indra/llmessage/tests/lltemplatemessagebuilder_test.cpp index 2771acf7e2..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"); @@ -1129,5 +1129,53 @@ namespace tut 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; + } } From ef7be00606130cbe361ba7644058171918a3f30b Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 05:08:13 -0400 Subject: [PATCH 4/5] Message data: don't probe empty variable fields in the LLSD copy path An empty MVT_VARIABLE field stores no payload, and since the inline- storage change getData() returns null for it, so copyFromMessageData's unconditional read of the last byte (checking for a terminating NUL) became a null[-1] access; addBinaryData's &v[0] on a size-0 vector was likewise out of contract. Guard both on size, and have buildBlock's MBT_MULTIPLE underfill error name the block from the template instead of dereferencing a block-data pointer that can be null there. Co-Authored-By: Claude Fable 5 --- indra/llmessage/llsdmessagebuilder.cpp | 14 ++++++++++---- indra/llmessage/lltemplatemessagebuilder.cpp | 5 ++--- .../tests/llsdmessagebuilder_test.cpp | 19 +++++++++++++++++++ 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/indra/llmessage/llsdmessagebuilder.cpp b/indra/llmessage/llsdmessagebuilder.cpp index 4dccc068dc..550fb749ca 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; } @@ -259,14 +262,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; } diff --git a/indra/llmessage/lltemplatemessagebuilder.cpp b/indra/llmessage/lltemplatemessagebuilder.cpp index c1956a11d6..cc3f0750ef 100644 --- a/indra/llmessage/lltemplatemessagebuilder.cpp +++ b/indra/llmessage/lltemplatemessagebuilder.cpp @@ -565,7 +565,6 @@ static S32 buildBlock(U8* buffer, S32 buffer_size, const LLMessageBlock* templat // 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); - const LLMsgBlkData* mbci = header; S32 block_count = header ? header->mBlockNumber : 0; if (template_data->mType == MBT_VARIABLE) { @@ -590,7 +589,7 @@ 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; @@ -599,7 +598,7 @@ static S32 buildBlock(U8* buffer, S32 buffer_size, const LLMessageBlock* templat for (S32 block_index = 0; block_index < block_count; ++block_index) { - mbci = message_data->getBlock(template_data->mName, 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(); diff --git a/indra/llmessage/tests/llsdmessagebuilder_test.cpp b/indra/llmessage/tests/llsdmessagebuilder_test.cpp index 6804c3e29e..fa0562721b 100644 --- a/indra/llmessage/tests/llsdmessagebuilder_test.cpp +++ b/indra/llmessage/tests/llsdmessagebuilder_test.cpp @@ -833,5 +833,24 @@ 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); + } + } From 1ea1758cad007293283c33c527fff12709d1a32b Mon Sep 17 00:00:00 2001 From: Rye Date: Thu, 11 Jun 2026 05:27:59 -0400 Subject: [PATCH 5/5] Message data: skip never-filled placeholder variables when copying Builder-owned LLMsgData carries a size -1 placeholder for each variable of an opened block until addData() fills it, and a placeholder has no payload at all, so both copyFromMessageData() implementations would read through a null data pointer if handed such data. No production path does (every copyToBuilder source is reader-decoded data, which fills every variable), but the guard is one branch. Block-level walking is unchanged on purpose: pre-added header blocks were emitted by the old map walk too, and the hand-built data convention in the LLSD builder tests expects blocks with a zero repeat count to be copied. Co-Authored-By: Claude Fable 5 --- indra/llmessage/llsdmessagebuilder.cpp | 6 +++++ indra/llmessage/lltemplatemessagebuilder.cpp | 6 +++++ .../tests/llsdmessagebuilder_test.cpp | 25 +++++++++++++++++++ 3 files changed, 37 insertions(+) diff --git a/indra/llmessage/llsdmessagebuilder.cpp b/indra/llmessage/llsdmessagebuilder.cpp index 550fb749ca..c7a1fafee4 100644 --- a/indra/llmessage/llsdmessagebuilder.cpp +++ b/indra/llmessage/llsdmessagebuilder.cpp @@ -252,6 +252,12 @@ void LLSDMessageBuilder::copyFromMessageData(const LLMsgData& data) 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()) diff --git a/indra/llmessage/lltemplatemessagebuilder.cpp b/indra/llmessage/lltemplatemessagebuilder.cpp index cc3f0750ef..69d9ed9365 100644 --- a/indra/llmessage/lltemplatemessagebuilder.cpp +++ b/indra/llmessage/lltemplatemessagebuilder.cpp @@ -769,6 +769,12 @@ void LLTemplateMessageBuilder::copyFromMessageData(const LLMsgData& data) 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/tests/llsdmessagebuilder_test.cpp b/indra/llmessage/tests/llsdmessagebuilder_test.cpp index fa0562721b..7dcc03edc1 100644 --- a/indra/llmessage/tests/llsdmessagebuilder_test.cpp +++ b/indra/llmessage/tests/llsdmessagebuilder_test.cpp @@ -852,5 +852,30 @@ namespace tut 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")); + } + }