From 61737261742cdb684dc1704a8484ec235064f907 Mon Sep 17 00:00:00 2001 From: Rye Date: Mon, 12 Oct 2026 14:42:00 -0400 Subject: [PATCH 1/5] LLBufferArray::splitAfter adds no empty segment after a segment's last byte splitAfter meant to leave a segment whole when asked to split it after its last byte, but tested for the address one past it, which getSegment never returns. So it split anyway and inserted the zero-length segment its comment says it avoids. LLBufferStreamBuf::sync had come to rely on that. It splits after the last byte written and erases the segment after the split, the part of the put area the stream did not use. When a write had filled its segment, the segment it erased was the empty one, so it erased nothing. With the split fixed it would erase whatever came next, another channel's data included, on every flush and on the stream's destruction. It now trims only a put area with something left over. llbuffer's tests check that a split after the last byte adds nothing, and io's that a flush after filling the put segment keeps the segment after it. Each fails without its half of the fix. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/llbuffer.cpp | 2 +- indra/llmessage/llbufferstream.cpp | 12 +++++++----- indra/llmessage/tests/io_test.cpp | 22 ++++++++++++++++++++++ indra/llmessage/tests/llbuffer_test.cpp | 17 +++++++++++++++++ 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/indra/llmessage/llbuffer.cpp b/indra/llmessage/llbuffer.cpp index 3a4b493b264..7a74ad87388 100644 --- a/indra/llmessage/llbuffer.cpp +++ b/indra/llmessage/llbuffer.cpp @@ -363,7 +363,7 @@ LLBufferArray::segment_iterator_t LLBufferArray::splitAfter(U8* address) // We have the location and the segment. U8* base = (*it).data(); S32 size = (*it).size(); - if(address == (base + size)) + if(address == (base + size - 1)) { // No need to split, since this is the last byte of the // segment. We do not want to have zero length segments, since diff --git a/indra/llmessage/llbufferstream.cpp b/indra/llmessage/llbufferstream.cpp index cb5a4c1824c..f7137b084a8 100644 --- a/indra/llmessage/llbufferstream.cpp +++ b/indra/llmessage/llbufferstream.cpp @@ -212,15 +212,17 @@ int LLBufferStreamBuf::sync() address = mBuffer->seek(mChannels.out(), address, -1); if(address) { - LLBufferArray::segment_iterator_t it; - it = mBuffer->splitAfter(address); LLBufferArray::segment_iterator_t end = mBuffer->endSegment(); + LLBufferArray::segment_iterator_t it = mBuffer->getSegment(address); if(it != end) { - ++it; - if(it != end) + // Give back what the put area did not use. A write that + // filled its segment left nothing over, and the segment + // after that one is not the put area's to erase. + if(address != ((*it).data() + (*it).size() - 1)) { - mBuffer->eraseSegment(it); + it = mBuffer->splitAfter(address); + mBuffer->eraseSegment(++it); } return_value = 0; } diff --git a/indra/llmessage/tests/io_test.cpp b/indra/llmessage/tests/io_test.cpp index b41653f8321..e373ad559c8 100644 --- a/indra/llmessage/tests/io_test.cpp +++ b/indra/llmessage/tests/io_test.cpp @@ -418,6 +418,28 @@ namespace CHECK(actual == expected); } + TEST_CASE_METHOD(buffer_and_stream_data, "buffer_stream: flush after filling the put segment keeps the segment after it", "[buffer_stream]") + { + LLChannelDescriptors ch = mBuffer.nextChannel(); + LLBufferStream str(ch, &mBuffer); + + // The first character makes the put area, a segment of the stream's own size. + str << 'x'; + LLBufferArray::segment_iterator_t it = mBuffer.beginSegment(); + REQUIRE(it != mBuffer.endSegment()); + const S32 segment_size = (*it).size(); + + // Fill it to its last byte, and put a segment on another channel after it. + str << std::string(segment_size - 1, 'x'); + const char TAIL[] = "tail"; + const S32 tail_len = static_cast(strlen(TAIL)); + mBuffer.append(ch.in(), (U8*)TAIL, tail_len); + str.flush(); + + CHECK(mBuffer.countAfter(ch.out(), NULL) == segment_size); + CHECK(mBuffer.countAfter(ch.in(), NULL) == tail_len); + } + TEST_CASE_METHOD(buffer_and_stream_data, "buffer_stream: seekp on the stream matches seekp on an ostringstream", "[buffer_stream]") { LLChannelDescriptors ch = mBuffer.nextChannel(); diff --git a/indra/llmessage/tests/llbuffer_test.cpp b/indra/llmessage/tests/llbuffer_test.cpp index d60392b85f9..aff9cf4259f 100644 --- a/indra/llmessage/tests/llbuffer_test.cpp +++ b/indra/llmessage/tests/llbuffer_test.cpp @@ -235,6 +235,23 @@ namespace CHECK(str == std::string(" is a Virtual World")); } + TEST_CASE("buffer: splitAfter the last byte of a segment adds no empty segment", "[buffer]") + { + const char array[] = "SecondLife"; + S32 len = static_cast(strlen(array)); + LLBufferArray bufferArray; + bufferArray.append(0, (U8*)array, len); + + char buf[255]; + S32 len1 = len; + U8* last = bufferArray.readAfter(0, 0, (U8*)buf, len1); + REQUIRE(len1 == len); + LLBufferArray::segment_iterator_t iterator = bufferArray.splitAfter(last); + REQUIRE(iterator != bufferArray.endSegment()); + CHECK((*iterator).size() == len); + CHECK(++iterator == bufferArray.endSegment()); + } + TEST_CASE("buffer: eraseSegment removes the segment makeSegment made", "[buffer]") { LLBufferArray bufferArray; From a685bc11a7b3546f0133b1dec649bf3028d3f604 Mon Sep 17 00:00:00 2001 From: Rye Date: Mon, 12 Oct 2026 14:43:46 -0400 Subject: [PATCH 2/5] BufferArrayStreamBuf seeks to the end of its data, not its last byte seekoff clamped a read position to the last byte, so a seek to the end left the next read returning that byte rather than EOF, and a putback there putting back the byte before it. It also took an offset from the end the wrong way, so seekg(-2, end) went two bytes past the end and was clamped back, and it let a seek before the start wrap round as a size_t. Over an empty BufferArray every seek failed, the end being one byte before the start. A seek from the current position moved the read position even when the seek then failed. Both directions now work out where a seek lands the same way: from the start, the current position or the end, adding the offset; before the start fails and changes nothing, past the end is the end. A read position at the end lies just past the last byte, in that byte's block. Nothing in the viewer seeks one of these streams, so this changes no behaviour there. bufferstream's tests check the end, an offset from it, a write from it, a seek before the start, and an empty BufferArray. The seek to the end now puts back the '!' that the TUT test's message said it would, where the check had expected EOF. Co-Authored-By: Claude Opus 5.5 --- indra/llcorehttp/bufferstream.cpp | 110 ++++++++++--------- indra/llcorehttp/tests/bufferstream_test.cpp | 40 ++++++- 2 files changed, 96 insertions(+), 54 deletions(-) diff --git a/indra/llcorehttp/bufferstream.cpp b/indra/llcorehttp/bufferstream.cpp index ea92f2f71dc..905c02128b6 100644 --- a/indra/llcorehttp/bufferstream.cpp +++ b/indra/llcorehttp/bufferstream.cpp @@ -185,83 +185,95 @@ std::streamsize BufferArrayStreamBuf::xsputn(const char * src, std::streamsize c } -std::streampos BufferArrayStreamBuf::seekoff(std::streamoff off, - std::ios_base::seekdir way, - std::ios_base::openmode which) +namespace { - std::streampos ret(-1); - - if (! mBufferArray) + // Where a seek of off from way lands in size bytes, from cur for a seek + // from the current position. Before the start is -1, and past the end + // is the end. + std::streamoff seek_target(std::streamoff off, std::ios_base::seekdir way, size_t cur, size_t size) { - return ret; - } - - if (std::ios_base::in == which) - { - size_t pos(0); - + std::streamoff base(0); switch (way) { case std::ios_base::beg: - pos = off; break; case std::ios_base::cur: - pos = mReadCurPos += off; + base = std::streamoff(cur); break; case std::ios_base::end: - pos = mBufferArray->mLen - off; + base = std::streamoff(size); break; default: - return ret; + return -1; } - if (pos >= mBufferArray->size()) + const std::streamoff target(base + off); + if (target < 0) { - pos = (std::max)(size_t(0), mBufferArray->size() - 1); + return -1; } - size_t ba_offset(0); - int block(mBufferArray->findBlock(pos, &ba_offset)); - if (block < 0) - return ret; - const char * start(NULL), * end(NULL); - if (! mBufferArray->getBlockStartEnd(block, &start, &end)) - return ret; - mReadCurBlock = block; - mReadBegin = start; - mReadCur = start + ba_offset; - mReadEnd = end; - ret = mReadCurPos = pos; + return (std::min)(target, std::streamoff(size)); } - else if (std::ios_base::out == which) - { - size_t pos(0); +} - switch (way) - { - case std::ios_base::beg: - pos = off; - break; - case std::ios_base::cur: - pos = mWriteCurPos += off; - break; +std::streampos BufferArrayStreamBuf::seekoff(std::streamoff off, + std::ios_base::seekdir way, + std::ios_base::openmode which) +{ + std::streampos ret(-1); - case std::ios_base::end: - pos = mBufferArray->mLen - off; - break; + if (! mBufferArray) + { + return ret; + } - default: + if (std::ios_base::in == which) + { + const size_t size(mBufferArray->size()); + const std::streamoff target(seek_target(off, way, mReadCurPos, size)); + if (target < 0) + { return ret; } + const size_t pos(target); - if (pos > mBufferArray->size()) + if (0 == size) + { + // Nothing to read: where a new stream buffer starts + mReadCurBlock = -1; + mReadBegin = mReadCur = mReadEnd = NULL; + } + else + { + // The end lies just past the last byte, in that byte's block, + // so that a read there finds nothing and a putback finds the byte. + const bool at_end(pos == size); + size_t ba_offset(0); + int block(mBufferArray->findBlock(at_end ? pos - 1 : pos, &ba_offset)); + if (block < 0) + return ret; + const char * start(NULL), * end(NULL); + if (! mBufferArray->getBlockStartEnd(block, &start, &end)) + return ret; + mReadCurBlock = block; + mReadBegin = start; + mReadCur = start + ba_offset + (at_end ? 1 : 0); + mReadEnd = end; + } + ret = mReadCurPos = pos; + } + else if (std::ios_base::out == which) + { + const std::streamoff target(seek_target(off, way, mWriteCurPos, mBufferArray->size())); + if (target < 0) { - pos = mBufferArray->size(); + return ret; } - ret = mWriteCurPos = pos; + ret = mWriteCurPos = size_t(target); } return ret; diff --git a/indra/llcorehttp/tests/bufferstream_test.cpp b/indra/llcorehttp/tests/bufferstream_test.cpp index 2536fe790c0..60ea9e3fba1 100644 --- a/indra/llcorehttp/tests/bufferstream_test.cpp +++ b/indra/llcorehttp/tests/bufferstream_test.cpp @@ -114,6 +114,21 @@ TEST_CASE("BufferStream Tests: BufferArrayStream holds a reference on an empty B } +TEST_CASE("BufferStream Tests: BufferArrayStreamBuf over an empty BufferArray seeks to its start and reads nothing", "[BufferStream_Tests]") +{ + // create a new ref counted BufferArray with implicit reference + BufferArray * ba = new BufferArray; + BufferArrayStreamBuf bsb(ba); + ba->release(); + ba = NULL; + + CHECK(bsb.seekoff(0, std::ios_base::beg, std::ios_base::in) == std::streampos(0)); + CHECK(bsb.seekoff(0, std::ios_base::end, std::ios_base::in) == std::streampos(0)); + CHECK(tst_traits_t::eof() == bsb.underflow()); + CHECK(tst_traits_t::eof() == bsb.pbackfail(tst_traits_t::eof())); +} + + TEST_CASE("BufferStream Tests: BufferArrayStreamBuf reads and appends and seeks a BufferArray", "[BufferStream_Tests]") { // create a new ref counted BufferArray with implicit reference @@ -152,11 +167,26 @@ TEST_CASE("BufferStream Tests: BufferArrayStreamBuf reads and appends and seeks CHECK(bsb->seekoff(4, std::ios_base::cur, std::ios_base::in) == std::streampos(4)); UNSCOPED_INFO("showmanyc() picks up seekoff"); CHECK(bsb->showmanyc() == (c_len + 5 - 4)); - CHECK(bsb->seekoff(0, std::ios_base::end, std::ios_base::in) == std::streampos(c_len + 4)); - // The seek to the end leaves the read position on the last character, the '!', - // so pbackfail() refuses the '!' and puts back the 'a' before it - CHECK(tst_traits_t::eof() == bsb->pbackfail('!')); - CHECK(bsb->pbackfail('a') == 'a'); + UNSCOPED_INFO("the end is past the last character"); + CHECK(bsb->seekoff(0, std::ios_base::end, std::ios_base::in) == std::streampos(c_len + 5)); + CHECK(tst_traits_t::eof() == bsb->underflow()); + CHECK(bsb->showmanyc() == 0); + // so the last character, the '!', is the one that puts back there + CHECK(tst_traits_t::eof() == bsb->pbackfail('a')); + CHECK(bsb->pbackfail('!') == '!'); + UNSCOPED_INFO("an offset from the end counts back from it"); + CHECK(bsb->seekoff(-2, std::ios_base::end, std::ios_base::in) == std::streampos(c_len + 3)); + CHECK(bsb->underflow() == 'a'); + UNSCOPED_INFO("past the end is the end"); + CHECK(bsb->seekoff(1, std::ios_base::end, std::ios_base::in) == std::streampos(c_len + 5)); + UNSCOPED_INFO("before the start fails and leaves the position"); + CHECK(bsb->seekoff(-1, std::ios_base::beg, std::ios_base::in) == std::streampos(-1)); + CHECK(bsb->seekoff(0, std::ios_base::cur, std::ios_base::in) == std::streampos(c_len + 5)); + UNSCOPED_INFO("a write from the end overwrites the last character"); + CHECK(bsb->seekoff(-1, std::ios_base::end, std::ios_base::out) == std::streampos(c_len + 4)); + CHECK(bsb->xsputn("?", 1) == 1); + CHECK(bsb->seekoff(-1, std::ios_base::end, std::ios_base::in) == std::streampos(c_len + 4)); + CHECK(bsb->underflow() == '?'); // the stream buffer is not ref counted; deleting it drops the BufferArray's last reference delete bsb; From 35218b7f5c945ce1c0682b8577685a1bc210a1f1 Mon Sep 17 00:00:00 2001 From: Rye Date: Mon, 12 Oct 2026 14:45:29 -0400 Subject: [PATCH 3/5] HTTP retries back off at most five seconds by default again HTTP_RETRY_BACKOFF_MAX_DEFAULT is commented "5 sec" and was 5E6L when MAINT-7495 set it, to stop the viewer retrying for too long. A 2024 change that wrote these constants as integers gave it as 50000006UL, fifty seconds. A request that sets no ceiling of its own has backed off since as if it had none: with the default five retries, after 1, 2, 4, 8 and 16 seconds instead of 1, 2, 4, 5 and 5. HttpOptions takes the same default, clamped on its way into a request to the 20-second limit, which is still above every one of those delays. It is 5 seconds again. Two static_asserts hold the defaults in order and within that limit; the second fails on the old value. Co-Authored-By: Claude Opus 5.5 --- indra/llcorehttp/_httpinternal.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/indra/llcorehttp/_httpinternal.h b/indra/llcorehttp/_httpinternal.h index 2a191fa5507..c5f72dd30c8 100644 --- a/indra/llcorehttp/_httpinternal.h +++ b/indra/llcorehttp/_httpinternal.h @@ -131,8 +131,12 @@ constexpr int HTTP_RETRY_COUNT_DEFAULT = 5; constexpr int HTTP_RETRY_COUNT_MIN = 0; constexpr int HTTP_RETRY_COUNT_MAX = 100; constexpr HttpTime HTTP_RETRY_BACKOFF_MIN_DEFAULT = 1000000UL; // 1 sec -constexpr HttpTime HTTP_RETRY_BACKOFF_MAX_DEFAULT = 50000006UL; // 5 sec +constexpr HttpTime HTTP_RETRY_BACKOFF_MAX_DEFAULT = 5000000UL; // 5 sec constexpr HttpTime HTTP_RETRY_BACKOFF_MAX = 20000000UL; // 20 sec +static_assert(HTTP_RETRY_BACKOFF_MIN_DEFAULT <= HTTP_RETRY_BACKOFF_MAX_DEFAULT, + "the default backoff range must not be empty"); +static_assert(HTTP_RETRY_BACKOFF_MAX_DEFAULT <= HTTP_RETRY_BACKOFF_MAX, + "the default backoff must lie within the limit options are clamped to"); constexpr int HTTP_REDIRECTS_DEFAULT = 10; From ef08613dc77cee5abc9fa9761bc0b85af3036ee1 Mon Sep 17 00:00:00 2001 From: Rye Date: Mon, 12 Oct 2026 14:48:20 -0400 Subject: [PATCH 4/5] The ascii data packers' binary unpacks read no further than their line LLDataPackerAsciiBuffer's and LLDataPackerAsciiFile's unpackBinaryData took the size a line gave, capped only by the caller's buffer, and read that many bytes three characters at a time from the line, a 512-byte copy on the stack, with no regard to where the line ended. A line that claimed more bytes than it held was read on past its terminator and past the copy, and the unpack reported success with the size it claimed. A line whose size did not parse left out_size as it was, which LLTextureAnim, among others, leaves uninitialised. unpackBinaryDataFixed read the caller's count from its line the same way. Both packers now share one unpack that stops where the line does. A line with no size fails with out_size 0; one that holds fewer bytes than it claims gives what it holds, fails, and says how many bytes it gave in out_size. unpackBinaryDataFixed fails likewise when its line is short. What a well-formed line unpacks to is unchanged. lldatapacker's tests give each a line that claims more than it holds, and one with no size. They fail on the old code, which reported success for every one of them. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/lldatapacker.cpp | 146 ++++++++++---------- indra/llmessage/tests/lldatapacker_test.cpp | 52 +++++++ 2 files changed, 123 insertions(+), 75 deletions(-) diff --git a/indra/llmessage/lldatapacker.cpp b/indra/llmessage/lldatapacker.cpp index a54ea591178..6bb22ea4742 100644 --- a/indra/llmessage/lldatapacker.cpp +++ b/indra/llmessage/lldatapacker.cpp @@ -45,6 +45,63 @@ const S32 DP_BUFSIZE = 512; static char DUMMY_BUFFER[128]; /*Flawfinder: ignore*/ +// Reads up to count bytes, each written "%02x ", from text, a line +// getValueStr read, and returns how many it read. It stops where the line +// does rather than reading on past its terminator. +static S32 unpack_hex_bytes(const char* text, U8* value, S32 count) +{ + const size_t len = strlen(text); /* Flawfinder: ignore */ + S32 i = 0; + for (size_t pos = 0; i < count && pos + 2 <= len; pos += 3, ++i) + { + unsigned int val; + if (sscanf(text + pos, "%2x", &val) != 1) + { + break; + } + value[i] = (U8)val; + } + return i; +} + +// Unpacks a line packBinaryData wrote, its size as "%010d " and then its +// bytes, into value, which holds value_size of them. out_size is the size +// the line gives, or what was read of it when the line holds less. +static bool unpack_ascii_binary_data(const char* valuestr, U8* value, S32 value_size, S32& out_size, const char* name) +{ + if (value_size < 0) + { + LL_WARNS() << "unpackBinaryData passed negative buffer size for " << name << ", aborting!" << LL_ENDL; + out_size = 0; + return false; + } + + S32 size = 0; + if (sscanf(valuestr, "%010d", &size) != 1 || size < 0) + { + LL_WARNS() << "unpackBinaryData unpacked invalid size for " << name << ", aborting!" << LL_ENDL; + out_size = 0; + return false; + } + + // The bytes follow the ten digits of the size and a space. + const char* bytes = valuestr + llmin(strlen(valuestr), (size_t)11); /* Flawfinder: ignore */ + S32 max_bytes = llmin(size, value_size); + if (max_bytes != size) + { + LL_WARNS() << "unpackBinaryData: buffer too small for " << name << ", truncating!" << LL_ENDL; + } + S32 read = unpack_hex_bytes(bytes, value, max_bytes); + if (read < max_bytes) + { + LL_WARNS() << "unpackBinaryData: " << name << " holds " << read << " of its " << size << " bytes, aborting!" << LL_ENDL; + out_size = read; + return false; + } + out_size = size; + return true; +} + LLDataPacker::LLDataPacker() : mPassFlags(0), mWriteEnabled(false) { } @@ -852,39 +909,13 @@ bool LLDataPackerAsciiBuffer::packBinaryData(const U8 *value, S32 size, const ch bool LLDataPackerAsciiBuffer::unpackBinaryData(U8 *value, S32 value_size, S32 &out_size, const char *name) { - bool success = true; char valuestr[DP_BUFSIZE]; /* Flawfinder: ignore */ if (!getValueStr(name, valuestr, DP_BUFSIZE)) { out_size = 0; return false; } - - if (value_size < 0) - { - LL_WARNS() << "LLDataPackerBinaryBuffer::unpackBinaryData passed negative buffer size, aborting!" << LL_ENDL; - out_size = 0; - return false; - } - - char *cur_pos = &valuestr[0]; - sscanf(valuestr,"%010d", &out_size); - cur_pos += 11; - - S32 max_bytes = llmin(out_size, value_size); - if (max_bytes != out_size) - { - LL_WARNS() << "LLDataPackerAsciiBuffer::unpackBinaryData: buffer too small for data, truncating!" << LL_ENDL; - } - S32 i; - for (i = 0; i < max_bytes; i++) - { - S32 val; - sscanf(cur_pos,"%02x", &val); - value[i] = val; - cur_pos += 3; - } - return success; + return unpack_ascii_binary_data(valuestr, value, value_size, out_size, name); } @@ -937,24 +968,19 @@ bool LLDataPackerAsciiBuffer::packBinaryDataFixed(const U8 *value, S32 size, con bool LLDataPackerAsciiBuffer::unpackBinaryDataFixed(U8 *value, S32 size, const char *name) { - bool success = true; char valuestr[DP_BUFSIZE]; /* Flawfinder: ignore */ if (!getValueStr(name, valuestr, DP_BUFSIZE)) { return false; } - char *cur_pos = &valuestr[0]; - - S32 i; - for (i = 0; i < size; i++) + S32 read = unpack_hex_bytes(valuestr, value, size); + if (read < size) { - S32 val; - sscanf(cur_pos,"%02x", &val); - value[i] = val; - cur_pos += 3; + LL_WARNS() << "unpackBinaryDataFixed: " << name << " holds " << read << " of its " << size << " bytes, aborting!" << LL_ENDL; + return false; } - return success; + return true; } @@ -1677,38 +1703,13 @@ bool LLDataPackerAsciiFile::packBinaryData(const U8 *value, S32 size, const char bool LLDataPackerAsciiFile::unpackBinaryData(U8 *value, S32 value_size, S32 &out_size, const char *name) { - char valuestr[DP_BUFSIZE]; /*Flawfinder: ignore*/ + char valuestr[DP_BUFSIZE]; /* Flawfinder: ignore */ if (!getValueStr(name, valuestr, DP_BUFSIZE)) { out_size = 0; return false; } - - if (value_size < 0) - { - LL_WARNS() << "LLDataPackerBinaryBuffer::unpackBinaryData passed negative buffer size, aborting!" << LL_ENDL; - out_size = 0; - return false; - } - - char *cur_pos = &valuestr[0]; - sscanf(valuestr,"%010d", &out_size); - cur_pos += 11; - - S32 max_bytes = llmin(out_size, value_size); - if (max_bytes != out_size) - { - LL_WARNS() << "LLDataPackerAsciiBuffer::unpackBinaryData: buffer too small for data, truncating!" << LL_ENDL; - } - S32 i; - for (i = 0; i < max_bytes; i++) - { - S32 val; - sscanf(cur_pos,"%02x", &val); - value[i] = val; - cur_pos += 3; - } - return true; + return unpack_ascii_binary_data(valuestr, value, value_size, out_size, name); } @@ -1743,24 +1744,19 @@ bool LLDataPackerAsciiFile::packBinaryDataFixed(const U8 *value, S32 size, const bool LLDataPackerAsciiFile::unpackBinaryDataFixed(U8 *value, S32 size, const char *name) { - bool success = true; - char valuestr[DP_BUFSIZE]; /*Flawfinder: ignore*/ + char valuestr[DP_BUFSIZE]; /* Flawfinder: ignore */ if (!getValueStr(name, valuestr, DP_BUFSIZE)) { return false; } - char *cur_pos = &valuestr[0]; - - S32 i; - for (i = 0; i < size; i++) + S32 read = unpack_hex_bytes(valuestr, value, size); + if (read < size) { - S32 val; - sscanf(cur_pos,"%02x", &val); - value[i] = val; - cur_pos += 3; + LL_WARNS() << "unpackBinaryDataFixed: " << name << " holds " << read << " of its " << size << " bytes, aborting!" << LL_ENDL; + return false; } - return success; + return true; } diff --git a/indra/llmessage/tests/lldatapacker_test.cpp b/indra/llmessage/tests/lldatapacker_test.cpp index 573ce8eef25..a07d9514b06 100644 --- a/indra/llmessage/tests/lldatapacker_test.cpp +++ b/indra/llmessage/tests/lldatapacker_test.cpp @@ -642,4 +642,56 @@ namespace UNSCOPED_INFO("the refused read leaves its argument alone"); CHECK(value == 11); } + + // A line of binary data that claims more bytes than it holds gives what + // it holds and fails, rather than reading on past the end of the line. + TEST_CASE("datapacker_test: LLDataPackerAsciiBuffer unpackBinaryData reads no further than its line", "[datapacker_test]") + { + char text[] = "0000000100 01 02 \n"; + LLDataPackerAsciiBuffer reader(text, sizeof(text)); + U8 value[128] = {}; + S32 size = -1; + CHECK_FALSE(reader.unpackBinaryData(value, sizeof(value), size, "data")); + CHECK(size == 2); + CHECK(value[0] == 1); + CHECK(value[1] == 2); + CHECK(value[2] == 0); + } + + TEST_CASE("datapacker_test: LLDataPackerAsciiBuffer unpackBinaryDataFixed reads no further than its line", "[datapacker_test]") + { + char text[] = "01 02 \n"; + LLDataPackerAsciiBuffer reader(text, sizeof(text)); + U8 value[4] = {}; + CHECK_FALSE(reader.unpackBinaryDataFixed(value, sizeof(value), "data")); + CHECK(value[0] == 1); + CHECK(value[1] == 2); + CHECK(value[2] == 0); + } + + TEST_CASE("datapacker_test: LLDataPackerAsciiBuffer unpackBinaryData refuses a line with no size", "[datapacker_test]") + { + char text[] = "lots 01 02 \n"; + LLDataPackerAsciiBuffer reader(text, sizeof(text)); + U8 value[4] = {}; + S32 size = -1; + CHECK_FALSE(reader.unpackBinaryData(value, sizeof(value), size, "data")); + CHECK(size == 0); + } + + TEST_CASE("datapacker_test: LLDataPackerAsciiFile unpackBinaryData and unpackBinaryDataFixed read no further than their lines", "[datapacker_test]") + { + std::istringstream istr("data\t0000000100 01 02 \nfixed\t01 02 \n"); + LLDataPackerAsciiFile reader(istr, 2); + U8 value[128] = {}; + S32 size = -1; + CHECK_FALSE(reader.unpackBinaryData(value, sizeof(value), size, "data")); + CHECK(size == 2); + CHECK(value[2] == 0); + + U8 fixed[4] = {}; + CHECK_FALSE(reader.unpackBinaryDataFixed(fixed, sizeof(fixed), "fixed")); + CHECK(fixed[1] == 2); + CHECK(fixed[2] == 0); + } } From 30e3fafc3385c1cbfc50ea7c49724de4816144cf Mon Sep 17 00:00:00 2001 From: Rye Date: Mon, 12 Oct 2026 14:51:38 -0400 Subject: [PATCH 5/5] LLHTTPRegistrar::buildAllServices deletes the nodes addNode refuses addNode refuses a node for a path that already has one and leaves it with its caller, but said nothing, and buildAllServices passed it a node fresh from a factory and forgot it. Building the services into a root that has some of them already, as building twice does, leaked a node a path. addNode now returns whether it took the node, and buildAllServices deletes one it refused. llhttpnode's tests check the refusal through the return value, and that a second buildAllServices into the same root leaves as many nodes alive as the first: it registers a node type that counts itself. Both fail without the fix. Not changed: a second wildcard under one parent, named differently from the first, replaces it rather than being refused, and the first is leaked. Refusing it would change which node answers such a path. Co-Authored-By: Claude Opus 5.5 --- indra/llmessage/llhttpnode.cpp | 11 +++++--- indra/llmessage/llhttpnode.h | 6 +++-- indra/llmessage/tests/llhttpnode_stub.cpp | 2 +- indra/llmessage/tests/llhttpnode_test.cpp | 32 +++++++++++++++++++---- 4 files changed, 40 insertions(+), 11 deletions(-) diff --git a/indra/llmessage/llhttpnode.cpp b/indra/llmessage/llhttpnode.cpp index 65bdfaff7e3..28da469ba7b 100644 --- a/indra/llmessage/llhttpnode.cpp +++ b/indra/llmessage/llhttpnode.cpp @@ -271,7 +271,7 @@ const LLHTTPNode* LLHTTPNode::traverse( -void LLHTTPNode::addNode(const std::string& path, LLHTTPNode* nodeToAdd) +bool LLHTTPNode::addNode(const std::string& path, LLHTTPNode* nodeToAdd) { typedef boost::tokenizer< boost::char_separator > tokenizer; boost::char_separator sep("/", "", boost::drop_empty_tokens); @@ -291,7 +291,7 @@ void LLHTTPNode::addNode(const std::string& path, LLHTTPNode* nodeToAdd) { LL_WARNS() << "LLHTTPNode::addNode: already a node that handles " << path << LL_ENDL; - return; + return false; } while (true) @@ -334,6 +334,7 @@ void LLHTTPNode::addNode(const std::string& path, LLHTTPNode* nodeToAdd) if (lastOne) break; node = nextNode; } + return true; } static void append_node_paths(LLSD& result, @@ -466,7 +467,11 @@ void LLHTTPRegistrar::buildAllServices(LLHTTPNode& root) LL_DEBUGS("AppInit") << "LLHTTPRegistrar::buildAllServices adding node for path " << i->first << LL_ENDL; - root.addNode(i->first, i->second->build()); + LLHTTPNode* node = i->second->build(); + if (!root.addNode(i->first, node)) + { + delete node; + } } } diff --git a/indra/llmessage/llhttpnode.h b/indra/llmessage/llhttpnode.h index 203e83e29aa..bbd4771474c 100644 --- a/indra/llmessage/llhttpnode.h +++ b/indra/llmessage/llhttpnode.h @@ -222,11 +222,13 @@ class LLHTTPNode Usually, child nodes are add through LLHTTPRegistration, not by calling this interface directly. - The added node will be now owned by the parent node. + The added node will be now owned by the parent node. A path + that already has a node is refused: addNode returns false, and + the node it was given is still the caller's. */ //@{ - virtual void addNode(const std::string& path, LLHTTPNode* nodeToAdd); + virtual bool addNode(const std::string& path, LLHTTPNode* nodeToAdd); LLSD allNodePaths() const; ///< Returns an arrary of node paths at and under this node diff --git a/indra/llmessage/tests/llhttpnode_stub.cpp b/indra/llmessage/tests/llhttpnode_stub.cpp index d896084021a..bb9c00245f8 100644 --- a/indra/llmessage/tests/llhttpnode_stub.cpp +++ b/indra/llmessage/tests/llhttpnode_stub.cpp @@ -61,7 +61,7 @@ LLHTTPNode* LLHTTPNode::getChild(const std::string& name, LLSD& context) const { bool LLHTTPNode::handles(const LLSD& remainder, LLSD& context) const { return false; } bool LLHTTPNode::validate(const std::string& name, LLSD& context) const { return false; } const LLHTTPNode* LLHTTPNode::traverse(const std::string& path, LLSD& context) const { return NULL; } -void LLHTTPNode::addNode(const std::string& path, LLHTTPNode* nodeToAdd) { } +bool LLHTTPNode::addNode(const std::string& path, LLHTTPNode* nodeToAdd) { return false; } LLSD LLHTTPNode::allNodePaths() const { return LLSD(); } const LLHTTPNode* LLHTTPNode::rootNode() const { return NULL; } const LLHTTPNode* LLHTTPNode::findNode(const std::string& name) const { return NULL; } diff --git a/indra/llmessage/tests/llhttpnode_test.cpp b/indra/llmessage/tests/llhttpnode_test.cpp index f9bf4db5d08..78e724e0775 100644 --- a/indra/llmessage/tests/llhttpnode_test.cpp +++ b/indra/llmessage/tests/llhttpnode_test.cpp @@ -223,11 +223,10 @@ namespace mRoot.addNode("greek/alpha", alphaNode); mRoot.addNode("greek/beta", betaNode); - mRoot.addNode("greek/gamma", gammaNode); - mRoot.addNode("greek/gamma", gamma2Node.get()); - const bool refused = gamma2Node->rootNode() == gamma2Node.get(); - CHECK(refused); - if (!refused) + CHECK(mRoot.addNode("greek/gamma", gammaNode)); + const bool taken = mRoot.addNode("greek/gamma", gamma2Node.get()); + CHECK_FALSE(taken); + if (taken) { (void)gamma2Node.release(); // mRoot took it, and deletes it } @@ -373,6 +372,29 @@ namespace } } + // Counts the nodes alive, to show that one buildAllServices refuses is deleted. + struct CountedNode : public LLHTTPNode + { + CountedNode() { ++sLive; } + ~CountedNode() override { --sLive; } + + static int sLive; + }; + int CountedNode::sLive = 0; + + TEST_CASE_METHOD(HTTPNodeTestData, "http node: buildAllServices deletes a node whose path is taken", "[http_node]") + { + static LLHTTPRegistration counted("/test/registrar/counted"); + + LLHTTPRegistrar::buildAllServices(mRoot); + const int live = CountedNode::sLive; + CHECK(live > 0); + + // Every path is taken now, so every node built is refused. + LLHTTPRegistrar::buildAllServices(mRoot); + CHECK(CountedNode::sLive == live); + } + TEST_CASE_METHOD(HTTPNodeTestData, "http node: web/server/api lists the registered paths", "[http_node]") { // test introspection