Harden LLMessageSystem UDP paths, fix packet ring corruption, cut per-packet allocations

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<width>(id, index) < 16)
             {
                 while (index != id)
                 {

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);
     }
 }
 

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<LLMsgVarData>::iterator iterator;
+    typedef std::vector<LLMsgVarData>::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<LLMsgVarDataMap*>(this)->find(name);
+    }
+
+private:
+    std::vector<LLMsgVarData> 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<LLMsgVarData, const char *, 8> msg_var_data_map_t;
+    typedef LLMsgVarDataMap msg_var_data_map_t;
     msg_var_data_map_t                  mMemberVarData;
     char                                *mName;
     S32                                 mTotalSize;

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)
             {

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.

indra/llmessage/lltemplatemessagebuilder.cpp

@@ -313,19 +313,27 @@ 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))
         {
             LL_WARNS() << "Field " << varname << " is a Variable 1 but program "
                    << "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<U8*>(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);
 }