stats: fix a bug where empty StatName cannot initialize storage correctly#45348
stats: fix a bug where empty StatName cannot initialize storage correctly#45348wbpcode wants to merge 3 commits into
Conversation
…ctly Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors StatName by moving serialization helper methods to SymbolTable::Encoding and adding a test for empty StatName storage. The review feedback recommends keeping dataAsStringView() private to avoid external misuse, using a friend class declaration instead, and removing a redundant private: access specifier.
There was a problem hiding this comment.
Pull request overview
Fixes a crash in StatNameStorage(StatName, SymbolTable&) when passed a default-constructed StatName (whose internal pointer is null). The previous path used StatName::copyToMemBlock, which called absl::MakeSpan(nullptr, 1) and then memcpy'd from null. The fix routes the storage copy through SymbolTable::Encoding::appendToMemBlock, which already handles null-backed StatName by emitting a single zero byte. The two now-unused StatName member helpers are replaced by a new Encoding::appendStatNameDataTo helper used by SymbolTable::join, requiring dataAsStringView() to become public.
Changes:
- Replace
StatName::copyToMemBlock/appendDataToMemBlockwithEncoding::appendToMemBlock/appendStatNameDataToso null-backedStatNamestorage init is safe. - Make
StatName::dataAsStringView()public so the new Encoding helper can use it; expand the comment onStatName::size()to clarify null-storage semantics. - Add a unit test constructing
StatNameStoragefrom a default-constructedStatName.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| source/common/stats/symbol_table.h | Removes copyToMemBlock/appendDataToMemBlock, declares Encoding::appendStatNameDataTo, makes dataAsStringView() public, updates size() comment. |
| source/common/stats/symbol_table.cc | Implements appendStatNameDataTo; routes StatNameStorage(StatName, …) through appendToMemBlock and join() through appendStatNameDataTo. |
| test/common/stats/symbol_table_impl_test.cc | Adds StorageFromEmptyStatName regression test. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: code <wbphub@gmail.com>
| /** | ||
| * Copies the entire StatName representation into a MemBlockBuilder, including | ||
| * the length metadata at the beginning. The MemBlockBuilder must not have | ||
| * any other data in it. | ||
| * | ||
| * @param mem_block_builder the builder to receive the storage. | ||
| */ | ||
| void copyToMemBlock(MemBlockBuilder<uint8_t>& mem_block_builder) { | ||
| ASSERT(mem_block_builder.size() == 0); | ||
| mem_block_builder.appendData(absl::MakeSpan(size_and_data_, size())); | ||
| } |
There was a problem hiding this comment.
This method doesn't take the special case where size_and_data_ be null take into account. At that case the size() will return 1, but the size_and_data_ is null, then crash. The Encoding::appendToMemBlock have handled that correctly. So we remove this method and reuse the Encoding::appendToMemBlock.
There was a problem hiding this comment.
Can we just document this and add an ASSERT? This is a pretty big change for a corner-case, especially un-inlining a performance critical function.
| /** | ||
| * Appends the data portion of the StatName representation into a | ||
| * MemBlockBuilder, excluding the length metadata. This is appropriate for | ||
| * join(), where several stat-names are combined, and we only need the | ||
| * aggregated length metadata. | ||
| * | ||
| * @param mem_block_builder the builder to receive the storage. | ||
| */ | ||
| void appendDataToMemBlock(MemBlockBuilder<uint8_t>& storage) { | ||
| auto sv = dataAsStringView(); | ||
| storage.appendData(absl::MakeSpan(reinterpret_cast<const uint8_t*>(sv.data()), sv.size())); | ||
| } |
There was a problem hiding this comment.
To keep consistence, we move this logic to Encoding as a helper method.
|
/retest |
Commit Message: stats: fix a bug where empty StatName cannot initialize storage correctly
Additional Description:
In the previous implementation, if we use StatName that constructed with default constructor (data_and_size_ be nullptr) to construct StatNameStorage, it will crash Envoy.
This is because
copyToMemBlockmethod of StatName not handle the case where data_and_size_ be nullptr correctly. Now, this PR unified related calling to Encoding's helper methods.Risk Level: low.
Testing: unit.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.