Skip to content

Commit 4484d24

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 f03eb79 commit 4484d24

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
@@ -362,31 +362,34 @@ void Engine::stop()
362362
// https://github.com/scratchfoundation/scratch-vm/blob/f1aa92fad79af17d9dd1c41eeeadca099339a9f1/src/engine/runtime.js#L2057-L2081
363363
if (m_activeThread) {
364364
stopThread(m_activeThread.get());
365-
// NOTE: The project should continue running even after "stop all" is called and the remaining threads should be stepped once.
366-
// The remaining threads can even start new threads which will ignore the "stop all" call and will "restart" the project.
367-
// This is probably a bug in the Scratch VM, but let's keep it here to keep it compatible.
368-
m_threadsToStop = m_threads;
369365

370366
// Remove threads owned by clones because clones are going to be deleted (#547)
371367
m_threads.erase(
372368
std::remove_if(
373369
m_threads.begin(),
374370
m_threads.end(),
375-
[](std::shared_ptr<Thread> thread) {
371+
[this](std::shared_ptr<Thread> thread) {
376372
assert(thread);
377373
Target *target = thread->target();
378374
assert(target);
379375

380376
if (!target->isStage()) {
381377
Sprite *sprite = static_cast<Sprite *>(target);
382378

383-
if (sprite->isClone())
379+
if (sprite->isClone()) {
380+
m_threadAboutToStop(thread.get());
384381
return true;
382+
}
385383
}
386384

387385
return false;
388386
}),
389387
m_threads.end());
388+
389+
// NOTE: The project should continue running even after "stop all" is called and the remaining threads should be stepped once.
390+
// The remaining threads can even start new threads which will ignore the "stop all" call and will "restart" the project.
391+
// This is probably a bug in the Scratch VM, but let's keep it here to keep it compatible.
392+
m_threadsToStop = m_threads;
390393
} else {
391394
// If there isn't any active thread, it means the project was stopped from the outside
392395
// In this case all threads should be removed and the project should be considered stopped

0 commit comments

Comments
 (0)