[BugFix] Fix heap-use-after-free of ThreadPool bvar #42351

banmoy · 2024-03-08T11:22:02Z

Why I'm doing:

Fix #42319.
The problem is introduced by #40171. The pr adds bvar members in ThreadPool, and each thread will update the bvars. bvar uses thread-local to improve performance, and will do some work (delete_thread_exit_helper) in __nptl_deallocate_tsd before the pthread exits. It will access bvar members here.

      #5 0x2075252f in bvar::detail::AgentGroup<bvar::detail::AgentCombiner<long, long, bvar::detail::AddTo<long> >::Agent>::_destroy_tls_blocks() (/root/starrocks/be/ut_build_ASAN/test/starrocks_test+0x2075252f)
      #6 0x25a5c0a2 in butil::detail::delete_thread_exit_helper(void*) (/root/starrocks/be/ut_build_ASAN/test/starrocks_test+0x25a5c0a2)
      #7 0x7fe5fdb57ca1 in __nptl_deallocate_tsd (/lib64/libpthread.so.0+0x7ca1)
      #8 0x7fe5fdb57eb2 in start_thread (/lib64/libpthread.so.0+0x7eb2)

But ThreadPool does not join the thread before destructing the pool itself, so it's possible that the bvar members are destroyed before delete_thread_exit_helper finishes which will lead to heap-use-after-free.

What I'm doing:

There are two ways to fix the problem

the threadpool joins the thread to ensure the thread totally exits, and then destructs the pool itself
replace bvar with CoreLocalCounter to ensure the thread will not access threadpool members out of ThreadPool::dispatch_thread

The 1 is more elegant, but more complicated because the threadpool is dynamic, and need a way to join threads (only join the threads in the destructor is not enough). Currently use 2 to fix the problem. According to the test, updating bvar takes about 10ns constantly as the number of threads increase, and CoreLocalCounter takes about 20ns constantly. I think it's acceptable to use CoreLocalCounter in this scenario.

What type of PR is this:

Does this PR entail a change in behavior?

Yes, this PR will result in a change in behavior.
No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

Interface/UI changes: syntax, type conversion, expression evaluation, display information
Parameter changes: default values, similar parameters but with different default values
Policy changes: use new policy to replace old one, functionality automatically enabled
Feature removed
Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

I have added test cases for my bug fix or my new feature
This pr needs user documentation (for new or modified features or behaviors)
- I have added documentation for my new feature or new function
This is a backport pr

Bugfix cherry-pick branch check:

sduzh · 2024-03-08T17:21:13Z

be/src/util/threadpool.cpp

-        _total_executed_tasks << 1;
-        _total_pending_time_ns << start_time.GetDeltaSince(task.submit_time).ToNanoseconds();
-        _total_execute_time_ns << finish_time.GetDeltaSince(start_time).ToNanoseconds();
+        _total_executed_tasks.fetch_add(1);


Suggested change

_total_executed_tasks.fetch_add(1);

_total_executed_tasks.fetch_add(1, std::memory_order_relaxed);

sduzh · 2024-03-08T17:21:21Z

be/src/util/threadpool.cpp

-        _total_pending_time_ns << start_time.GetDeltaSince(task.submit_time).ToNanoseconds();
-        _total_execute_time_ns << finish_time.GetDeltaSince(start_time).ToNanoseconds();
+        _total_executed_tasks.fetch_add(1);
+        _total_pending_time_ns.fetch_add(start_time.GetDeltaSince(task.submit_time).ToNanoseconds());


sduzh · 2024-03-08T17:21:27Z

be/src/util/threadpool.cpp

-        _total_execute_time_ns << finish_time.GetDeltaSince(start_time).ToNanoseconds();
+        _total_executed_tasks.fetch_add(1);
+        _total_pending_time_ns.fetch_add(start_time.GetDeltaSince(task.submit_time).ToNanoseconds());
+        _total_execute_time_ns.fetch_add(finish_time.GetDeltaSince(start_time).ToNanoseconds());


sduzh · 2024-03-08T17:21:45Z

be/src/util/threadpool.h

@@ -249,14 +248,11 @@ class ThreadPool {

    int max_threads() const { return _max_threads.load(std::memory_order_acquire); }

-    // Use bvar as the counter, and should not be called frequently.
-    int64_t total_executed_tasks() const { return _total_executed_tasks.get_value(); }
+    int64_t total_executed_tasks() const { return _total_executed_tasks; }


Suggested change

int64_t total_executed_tasks() const { return _total_executed_tasks; }

int64_t total_executed_tasks() const { return _total_executed_tasks.load(std::memory_order_relaxed); }

sduzh · 2024-03-08T17:21:52Z

be/src/util/threadpool.h


-    // Use bvar as the counter, and should not be called frequently.
-    int64_t total_pending_time_ns() const { return _total_pending_time_ns.get_value(); }
+    int64_t total_pending_time_ns() const { return _total_pending_time_ns; }


sduzh · 2024-03-08T17:21:57Z

be/src/util/threadpool.h


-    // Use bvar as the counter, and should not be called frequently.
-    int64_t total_execute_time_ns() const { return _total_execute_time_ns.get_value(); }
+    int64_t total_execute_time_ns() const { return _total_execute_time_ns; }


sduzh · 2024-03-11T02:02:02Z

be/src/util/threadpool.h

@@ -249,14 +248,11 @@ class ThreadPool {

    int max_threads() const { return _max_threads.load(std::memory_order_acquire); }

-    // Use bvar as the counter, and should not be called frequently.
-    int64_t total_executed_tasks() const { return _total_executed_tasks.get_value(); }
+    int64_t total_executed_tasks() const { return _total_executed_tasks.load(); }


Suggested change

int64_t total_executed_tasks() const { return _total_executed_tasks.load(); }

int64_t total_executed_tasks() const { return _total_executed_tasks.load(std::memory_order_relaxed); }

sduzh · 2024-03-11T02:02:09Z

be/src/util/threadpool.h


-    // Use bvar as the counter, and should not be called frequently.
-    int64_t total_pending_time_ns() const { return _total_pending_time_ns.get_value(); }
+    int64_t total_pending_time_ns() const { return _total_pending_time_ns.load(); }


sduzh · 2024-03-11T02:02:14Z

be/src/util/threadpool.h


-    // Use bvar as the counter, and should not be called frequently.
-    int64_t total_execute_time_ns() const { return _total_execute_time_ns.get_value(); }
+    int64_t total_execute_time_ns() const { return _total_execute_time_ns.load(); }


sduzh · 2024-03-11T02:05:45Z

Is the performance impact of using std::atomic acceptable? Has there been any comparison done with CoreLocalCounter?

Signed-off-by: PengFei Li <[email protected]>

banmoy · 2024-03-11T16:28:54Z

@sduzh Thanks for your advice. I add CoreLocalCounterTest.test_perf to compare the update performance among bvar, CoreLocalCounter and atomic with different threads. The results are as following

the performance of both bvar and CoreLocalCounter remains low as the number of threads increases, but the performance of atomic decreases as the number of threads increases
the cost of bvar is about 10ns, and the cost of CoreLocalCounter is about 20ns

As a result, I think using CoreLocalCounter to replace bvar is acceptable although it has ~10ns overhead than bvar

#thread	1	2	3	4	5	6	7	8	9	10	11	12	13	14	15	16	17	18	19	20	21	22	23	24
bvar	9	9	9	9	9	9	9	9	9	9	9	9	9	9	9	9	10	10	10	10	10	10	10	10
CoreLocalCounter	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19	19
atomic	6	11	34	54	68	81	90	109	113	149	153	173	181	201	211	250	234	255	270	291	291	313	369	335

github-actions · 2024-03-12T03:02:19Z

[FE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

github-actions · 2024-03-12T03:10:38Z

[BE Incremental Coverage Report]

✅ pass : 6 / 6 (100.00%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	be/src/util/threadpool.h	3	3	100.00%	[]
🔵	be/src/util/threadpool.cpp	3	3	100.00%	[]

Signed-off-by: PengFei Li <[email protected]>

) (#42682) Signed-off-by: PengFei Li <[email protected]>

) (#42683) Signed-off-by: PengFei Li <[email protected]>

mergify bot assigned banmoy Mar 8, 2024

banmoy marked this pull request as ready for review March 8, 2024 15:13

banmoy requested a review from a team as a code owner March 8, 2024 15:13

banmoy changed the title ~~[BugFix] Fix bvar heap-use-after-free in ThreadPool~~ [BugFix] Fix heap-use-after-free of ThreadPool bvar Mar 8, 2024

sduzh reviewed Mar 8, 2024

View reviewed changes

banmoy force-pushed the fix_asan branch from 2d5cf72 to 9d5108d Compare March 10, 2024 11:28

sduzh reviewed Mar 11, 2024

View reviewed changes

banmoy marked this pull request as draft March 11, 2024 09:58

banmoy force-pushed the fix_asan branch 2 times, most recently from a26ac03 to 20c300b Compare March 11, 2024 15:27

banmoy added 4 commits March 11, 2024 23:29

[BugFix] Fix bvar heap-use-after-free in ThreadPool

b7a2644

Signed-off-by: PengFei Li <[email protected]>

Fix comment

fac0111

Signed-off-by: PengFei Li <[email protected]>

Add CoreLocalCounterTest

f6e67e1

Signed-off-by: PengFei Li <[email protected]>

Use CoreLocalCounter

085ad7a

Signed-off-by: PengFei Li <[email protected]>

banmoy force-pushed the fix_asan branch from 20c300b to 085ad7a Compare March 11, 2024 15:29

banmoy marked this pull request as ready for review March 11, 2024 16:30

sduzh approved these changes Mar 12, 2024

View reviewed changes

sduzh requested review from kevincai and imay March 12, 2024 03:05

sduzh enabled auto-merge (squash) March 12, 2024 03:05

kevincai approved these changes Mar 12, 2024

View reviewed changes

sduzh merged commit edb5a63 into StarRocks:main Mar 12, 2024
47 checks passed

banmoy added a commit to banmoy/starrocks that referenced this pull request Mar 15, 2024

[BugFix] Fix heap-use-after-free of ThreadPool bvar (StarRocks#42351)

888260e

Signed-off-by: PengFei Li <[email protected]>

banmoy added a commit to banmoy/starrocks that referenced this pull request Mar 15, 2024

[BugFix] Fix heap-use-after-free of ThreadPool bvar (StarRocks#42351)

d446f9d

Signed-off-by: PengFei Li <[email protected]>

This was referenced Mar 15, 2024

[Enhancement] Add thread pool metrics (backport #40171) (backport #42351) #42682

Merged

[Enhancement] Add thread pool metrics (backport #40171) (backport #42351) #42683

Merged

banmoy added a commit to banmoy/starrocks that referenced this pull request Mar 15, 2024

[BugFix] Fix heap-use-after-free of ThreadPool bvar (StarRocks#42351)

f2e0011

Signed-off-by: PengFei Li <[email protected]>

banmoy added a commit to banmoy/starrocks that referenced this pull request Mar 15, 2024

[BugFix] Fix heap-use-after-free of ThreadPool bvar (StarRocks#42351)

a2be93e

Signed-off-by: PengFei Li <[email protected]>

banmoy added a commit to banmoy/starrocks that referenced this pull request Mar 15, 2024

[BugFix] Fix heap-use-after-free of ThreadPool bvar (StarRocks#42351)

a4cb703

Signed-off-by: PengFei Li <[email protected]>

banmoy added a commit to banmoy/starrocks that referenced this pull request Mar 15, 2024

[BugFix] Fix heap-use-after-free of ThreadPool bvar (StarRocks#42351)

3757742

Signed-off-by: PengFei Li <[email protected]>

banmoy added a commit to banmoy/starrocks that referenced this pull request Mar 15, 2024

[BugFix] Fix heap-use-after-free of ThreadPool bvar (StarRocks#42351)

63fe174

Signed-off-by: PengFei Li <[email protected]>

banmoy added a commit to banmoy/starrocks that referenced this pull request Mar 15, 2024

[BugFix] Fix heap-use-after-free of ThreadPool bvar (StarRocks#42351)

e701bf9

Signed-off-by: PengFei Li <[email protected]>

wyb pushed a commit that referenced this pull request Mar 15, 2024

[Enhancement] Add thread pool metrics (backport #40171) (backport #42351

6bd3670

) (#42682) Signed-off-by: PengFei Li <[email protected]>

wyb pushed a commit that referenced this pull request Mar 15, 2024

[Enhancement] Add thread pool metrics (backport #40171) (backport #42351

47b8d4e

) (#42683) Signed-off-by: PengFei Li <[email protected]>

github-actions bot added 3.1-merged 3.2-merged labels Mar 15, 2024

before-Sunrise mentioned this pull request Apr 16, 2024

[BugFix] fix be exit probelm #44162

Merged

24 tasks

mergify bot mentioned this pull request Apr 17, 2024

[BugFix] fix be exit probelm (backport #44162) #44216

Merged

42 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BugFix] Fix heap-use-after-free of ThreadPool bvar #42351

[BugFix] Fix heap-use-after-free of ThreadPool bvar #42351

banmoy commented Mar 8, 2024 •

edited

Loading

sduzh Mar 8, 2024

sduzh Mar 8, 2024

sduzh Mar 8, 2024

sduzh Mar 8, 2024

sduzh Mar 8, 2024

sduzh Mar 8, 2024

sduzh Mar 11, 2024

sduzh Mar 11, 2024

sduzh Mar 11, 2024

sduzh commented Mar 11, 2024

banmoy commented Mar 11, 2024 •

edited

Loading

github-actions bot commented Mar 12, 2024

github-actions bot commented Mar 12, 2024

	_total_executed_tasks.fetch_add(1);
	_total_executed_tasks.fetch_add(1, std::memory_order_relaxed);

	int64_t total_executed_tasks() const { return _total_executed_tasks; }
	int64_t total_executed_tasks() const { return _total_executed_tasks.load(std::memory_order_relaxed); }

[BugFix] Fix heap-use-after-free of ThreadPool bvar #42351

[BugFix] Fix heap-use-after-free of ThreadPool bvar #42351

Conversation

banmoy commented Mar 8, 2024 • edited Loading

Why I'm doing:

What I'm doing:

What type of PR is this:

Checklist:

Bugfix cherry-pick branch check:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sduzh commented Mar 11, 2024

banmoy commented Mar 11, 2024 • edited Loading

github-actions bot commented Mar 12, 2024

[FE Incremental Coverage Report]

github-actions bot commented Mar 12, 2024

[BE Incremental Coverage Report]

file detail

banmoy commented Mar 8, 2024 •

edited

Loading

banmoy commented Mar 11, 2024 •

edited

Loading