Skip to content

Commit 8da6f85

Browse files
fredroybakpaul
andauthored
[Simulation.Core] WorkerThread: fix use-after-free crash with ultra-short Task (#6116)
* add test illustrating the bug (crash) * fix the race use after free for task For an ultra-short task, a worker that's still in its doWork poll loop pops the task, runs it (m_task() runs the lambda + counter increment, microseconds), and the framework calls task->operator delete all before the main thread reaches setMainTaskStatus(task->getStatus()) * fix deleted includes --------- Co-authored-by: Paul Baksic <30337881+bakpaul@users.noreply.github.com>
1 parent ac40b51 commit 8da6f85

3 files changed

Lines changed: 118 additions & 2 deletions

File tree

Sofa/framework/Simulation/Core/src/sofa/simulation/task/WorkerThread.cpp

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -203,17 +203,27 @@ bool WorkerThread::pushTask(Task *task)
203203
return false;
204204
}
205205

206+
// Capture the task's Status* before the task becomes visible to workers.
207+
// Once m_tasks.push_back(task) runs and the lock is released, a worker
208+
// can pop the task, run it, and (if run() returns MemoryAlloc::Dynamic)
209+
// free it. Reading task->getStatus() after that point would dereference
210+
// freed memory. Status objects are owned by the caller of the dispatch
211+
// (e.g. a CpuTaskStatus on the originating frame) and are guaranteed to
212+
// outlive the workUntilDone() that follows the push, so the captured
213+
// pointer remains valid for the post-publish code below.
214+
Task::Status* statusForMain = nullptr;
206215
{
207216
simulation::ScopedLock lock(m_taskMutex);
208-
const int taskId = task->getStatus()->setBusy(true);
217+
statusForMain = task->getStatus();
218+
const int taskId = statusForMain->setBusy(true);
209219
task->m_id = taskId;
210220
m_tasks.push_back(task);
211221
}
212222

213223

214224
if (m_taskScheduler->testMainTaskStatus(nullptr))
215225
{
216-
m_taskScheduler->setMainTaskStatus(task->getStatus());
226+
m_taskScheduler->setMainTaskStatus(statusForMain);
217227
m_taskScheduler->wakeUpWorkers();
218228
}
219229

Sofa/framework/Simulation/Core/test/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ set(SOURCE_FILES
66
Colors_test.cpp
77
MappingGraph_test.cpp
88
ParallelForEach_test.cpp
9+
PushTaskRace_test.cpp
910
RequiredPlugin_test.cpp
1011
SceneCheckRegistry_test.cpp
1112
Simulation_test.cpp
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
/******************************************************************************
2+
* SOFA, Simulation Open-Framework Architecture *
3+
* (c) 2006 INRIA, USTL, UJF, CNRS, MGH *
4+
* *
5+
* This program is free software; you can redistribute it and/or modify it *
6+
* under the terms of the GNU Lesser General Public License as published by *
7+
* the Free Software Foundation; either version 2.1 of the License, or (at *
8+
* your option) any later version. *
9+
* *
10+
* This program is distributed in the hope that it will be useful, but WITHOUT *
11+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or *
12+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License *
13+
* for more details. *
14+
* *
15+
* You should have received a copy of the GNU Lesser General Public License *
16+
* along with this program. If not, see <http://www.gnu.org/licenses/>. *
17+
*******************************************************************************
18+
* Authors: The SOFA Team and external contributors (see Authors.txt) *
19+
* *
20+
* Contact information: contact@sofa-framework.org *
21+
******************************************************************************/
22+
#include <gtest/gtest.h>
23+
#include <sofa/simulation/task/DefaultTaskScheduler.h>
24+
#include <sofa/simulation/task/MainTaskSchedulerFactory.h>
25+
#include <sofa/simulation/task/CpuTask.h>
26+
#include <sofa/simulation/task/CpuTaskStatus.h>
27+
28+
#include <atomic>
29+
30+
namespace sofa
31+
{
32+
33+
// Reproduction for a use-after-free in WorkerThread::pushTask. The function
34+
// looks like:
35+
//
36+
// { lock; ...; m_tasks.push_back(task); } // task is now visible
37+
// if (m_taskScheduler->testMainTaskStatus(nullptr)) {
38+
// m_taskScheduler->setMainTaskStatus(task->getStatus()); // <-- UAF
39+
// ...
40+
// }
41+
//
42+
// Once m_tasks.push_back exposes the task to workers and the lock is released,
43+
// any worker can pop, run, and (for tasks that return MemoryAlloc::Dynamic
44+
// from run()) free the task. The post-lock task->getStatus() then reads the
45+
// vtable through freed memory.
46+
//
47+
// The race fires only when:
48+
// 1. testMainTaskStatus(nullptr) is true at the post-lock check, i.e.
49+
// this is the FIRST push of a workUntilDone cycle (m_mainTaskStatus
50+
// is set on the first push and cleared by workUntilDone), AND
51+
// 2. the task is short enough that a worker pops, runs, and frees it
52+
// before the main thread reaches the second task->getStatus() deref.
53+
//
54+
// Hence we tune the test for one task per workUntilDone cycle so every push
55+
// goes through the racy branch, and we run a large number of cycles to make
56+
// the race statistically certain.
57+
58+
namespace
59+
{
60+
61+
void singleShortLambdaCycle(simulation::TaskScheduler& scheduler,
62+
std::atomic<int>& counter)
63+
{
64+
simulation::CpuTaskStatus status;
65+
scheduler.addTask(status, [&counter]() {
66+
counter.fetch_add(1, std::memory_order_relaxed);
67+
});
68+
scheduler.workUntilDone(&status);
69+
}
70+
71+
} // namespace
72+
73+
// Stress the lambda overload of addTask with one short task per cycle.
74+
// Every push hits the racy branch in pushTask. If the use-after-free is
75+
// present, this test crashes non-deterministically on machines with
76+
// multiple worker threads. On the fixed code, it completes cleanly with
77+
// the expected counter total.
78+
TEST(PushTaskRace, ShortLambdaSingleTaskCycles)
79+
{
80+
constexpr int kCycles = 500000;
81+
82+
auto* scheduler = simulation::MainTaskSchedulerFactory::createInRegistry(
83+
simulation::DefaultTaskScheduler::name());
84+
ASSERT_NE(scheduler, nullptr);
85+
86+
// Force a real multi-threaded scheduler. The race only exists when
87+
// pushTask actually queues to a worker (>= 2 threads).
88+
scheduler->init(0);
89+
if (scheduler->getThreadCount() < 2)
90+
{
91+
GTEST_SKIP() << "scheduler has fewer than 2 threads; race cannot manifest";
92+
}
93+
94+
std::atomic<int> counter { 0 };
95+
for (int c = 0; c < kCycles; ++c)
96+
{
97+
singleShortLambdaCycle(*scheduler, counter);
98+
}
99+
100+
EXPECT_EQ(counter.load(std::memory_order_relaxed), kCycles);
101+
102+
scheduler->stop();
103+
}
104+
105+
} // namespace sofa

0 commit comments

Comments
 (0)