Copilot commented on code in PR #3361:
URL: https://github.com/apache/brpc/pull/3361#discussion_r3485501406
##########
test/thread_key_unittest.cpp:
##########
@@ -230,9 +231,12 @@ void* ThreadKeyFunc(void* arg) {
auto thread_key_arg = (ThreadKeyArg*)arg;
auto thread_keys = thread_key_arg->thread_keys;
std::vector<int> expects(thread_keys.size(), 0);
+ std::vector<std::unique_ptr<int>> owned_data;
+ owned_data.reserve(thread_keys.size());
for (auto key : thread_keys) {
EXPECT_TRUE(butil::thread_getspecific(*key) == NULL);
- EXPECT_EQ(0, butil::thread_setspecific(*key, new int(0)));
+ owned_data.emplace_back(new int(0));
+ EXPECT_EQ(0, butil::thread_setspecific(*key, owned_data.back().get()));
EXPECT_EQ(*(static_cast<int*>(butil::thread_getspecific(*key))), 0);
Review Comment:
`owned_data` owns the same `int*` that is also registered as thread-specific
data for keys created with a destructor (see thread_key_create(..., [](void*
data){ delete ... })). When the thread exits, `owned_data` frees the pointers
first, and then the TLS destructor frees them again, causing a double-free
under ASan/LSan. Release ownership after a successful thread_setspecific() so
cleanup is handled only by the key destructor.
##########
test/thread_key_unittest.cpp:
##########
@@ -262,14 +266,17 @@ TEST(ThreadLocalTest, thread_key_multi_thread) {
g_stopped = false;
g_deleted = false;
std::vector<ThreadKey*> thread_keys;
+ std::vector<std::unique_ptr<int>> owned_data;
int key_num = 20480;
+ owned_data.reserve(key_num);
for (int i = 0; i < key_num; ++i) {
thread_keys.push_back(new ThreadKey());
ASSERT_EQ(0, butil::thread_key_create(*thread_keys.back(), [](void*
data) {
delete static_cast<int*>(data);
}));
ASSERT_TRUE(butil::thread_getspecific(*thread_keys.back()) == NULL);
- ASSERT_EQ(0, butil::thread_setspecific(*thread_keys.back(), new
int(0)));
+ owned_data.emplace_back(new int(0));
+ ASSERT_EQ(0, butil::thread_setspecific(*thread_keys.back(),
owned_data.back().get()));
ASSERT_EQ(*(static_cast<int*>(butil::thread_getspecific(*thread_keys.back()))),
0);
Review Comment:
The per-key destructor deletes the thread-specific value, but `owned_data`
will also delete the same pointer at scope exit unless ownership is released.
This is a double-free when thread_key_delete triggers the destructor (and also
when worker threads exit). After successfully setting the value, release the
unique_ptr so the key destructor becomes the sole owner.
##########
src/brpc/span.cpp:
##########
@@ -337,14 +337,11 @@ void Span::ResetServerSpanName(const std::string&
full_method_name) {
void Span::submit(int64_t cpuwide_us) {
// Note: this method is not called for client-side spans.
EndAsParent();
- SpanContainer* container = new(std::nothrow)
SpanContainer(shared_from_this());
// If memory allocation fails, the server span will not be submitted for
persistence.
// The server span will be destroyed later when its shared_ptr refcount
drops to zero
// Child spans (held in _client_list) will also be destroyed when
// their refcounts reach zero.
- if (container) {
- container->submit(cpuwide_us);
- }
+ (new SpanContainer(shared_from_this()))->submit(cpuwide_us);
Review Comment:
The comment says span submission should be skipped if allocation fails, but
the new code uses throwing `new` and unconditionally dereferences the result.
Under OOM (or with exceptions disabled), this can terminate the process instead
of safely skipping persistence as documented. Use nothrow allocation and keep
the null-check, or update the comment/behavior consistently.
##########
test/object_pool_unittest.cpp:
##########
@@ -184,9 +184,13 @@ TEST_F(ObjectPoolTest, get_int) {
tm.stop();
printf("get a int takes %.1fns\n", tm.n_elapsed()/(double)N);
+ std::vector<std::unique_ptr<int>> new_ints;
Review Comment:
This test now uses std::unique_ptr but the file does not include <memory>.
It may compile only due to transitive includes (e.g. via gtest), which is
fragile across toolchains and header changes. Add an explicit `#include
<memory>` at file scope.
--
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]
