Copilot commented on code in PR #3661:
URL: https://github.com/apache/brpc/pull/3661#discussion_r4373555856
##########
src/butil/arena.cpp:
##########
@@ -64,7 +64,13 @@ void Arena::clear() {
}
void* Arena::allocate_new_block(size_t n) {
+ if (n > UINT32_MAX || n > SIZE_MAX - offsetof(Block, data)) {
+ return nullptr;
+ }
Block* b = (Block*)malloc(offsetof(Block, data) + n);
+ if (b == nullptr) {
+ return nullptr;
Review Comment:
The added tests reject oversized requests before reaching malloc, so they
would still pass if this null check were removed and the original crash
returned. Please add a deterministic allocation-failure test for a
representable isolated-block request, then verify nullptr is returned, existing
data is preserved, and subsequent allocations and clear() still work.
##########
test/CMakeLists.txt:
##########
@@ -88,6 +88,7 @@ file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/run_tests.sh
DESTINATION ${CMAKE_CURRENT_B
SET(TEST_BUTIL_SOURCES
${CMAKE_CURRENT_SOURCE_DIR}/abalist_unittest.cc
${CMAKE_CURRENT_SOURCE_DIR}/aligned_memory_unittest.cc
+ ${CMAKE_CURRENT_SOURCE_DIR}/arena_unittest.cc
Review Comment:
Please also register arena_unittest.cc in test/Makefile's TEST_BUTIL_SOURCES
list. That build uses an explicit source list (test/Makefile:54–156), not
discovery, so the Make-based test_butil and run_tests.sh workflow currently
omit both new regression tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
