Skip to content

Commit d25e968

Browse files
committed
Engine: Fix use after free when using removed clone threads
All threads are copied to m_threadsToStop, including clone threads that are removed later which can lead to use after free when the threadAboutToStop signal is emitted for these threads.
1 parent 60d4a7e commit d25e968

File tree

1 file changed

+9
-6
lines changed

1 file changed

+9
-6
lines changed

src/engine/internal/engine.cpp

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -321,31 +321,34 @@ void Engine::stop()
321321
// https://github.com/scratchfoundation/scratch-vm/blob/f1aa92fad79af17d9dd1c41eeeadca099339a9f1/src/engine/runtime.js#L2057-L2081
322322
if (m_activeThread) {
323323
stopThread(m_activeThread.get());
324-
// NOTE: The project should continue running even after "stop all" is called and the remaining threads should be stepped once.
325-
// The remaining threads can even start new threads which will ignore the "stop all" call and will "restart" the project.
326-
// This is probably a bug in the Scratch VM, but let's keep it here to keep it compatible.
327-
m_threadsToStop = m_threads;
328324

329325
// Remove threads owned by clones because clones are going to be deleted (#547)
330326
m_threads.erase(
331327
std::remove_if(
332328
m_threads.begin(),
333329
m_threads.end(),
334-
[](std::shared_ptr<Thread> thread) {
330+
[this](std::shared_ptr<Thread> thread) {
335331
assert(thread);
336332
Target *target = thread->target();
337333
assert(target);
338334

339335
if (!target->isStage()) {
340336
Sprite *sprite = static_cast<Sprite *>(target);
341337

342-
if (sprite->isClone())
338+
if (sprite->isClone()) {
339+
m_threadAboutToStop(thread.get());
343340
return true;
341+
}
344342
}
345343

346344
return false;
347345
}),
348346
m_threads.end());
347+
348+
// NOTE: The project should continue running even after "stop all" is called and the remaining threads should be stepped once.
349+
// The remaining threads can even start new threads which will ignore the "stop all" call and will "restart" the project.
350+
// This is probably a bug in the Scratch VM, but let's keep it here to keep it compatible.
351+
m_threadsToStop = m_threads;
349352
} else {
350353
// If there isn't any active thread, it means the project was stopped from the outside
351354
// In this case all threads should be removed and the project should be considered stopped

0 commit comments

Comments
 (0)