fix: use-after-free in CommandQueue delete methods#419
fix: use-after-free in CommandQueue delete methods#419mfazekas wants to merge 1 commit intorive-app:mainfrom
Conversation
Remove premature listener deletion from all 6 delete methods (deleteFile, deleteArtboard, deleteViewModelInstance, deleteImage, deleteFont, deleteAudio). The C++ CommandQueue still holds raw pointers to listeners after deletion, causing EXC_BAD_ACCESS when processMessages() invokes callbacks through dangling pointers. Listeners are already cleaned up in dealloc.
|
Good catch! They are cleaned in dealloc. The initial implementation is in an attempt to immediately free up memory once a Rive type has been "deleted" via command queue. I wonder if there's a way to have that without deferring until a command queue is dealloc'd. In our case, a command queue won't get deallocated until a Worker is deinit'd, and it's theoretically possible for someone to allocated 100s of files, which would allocate 100s of (C++) listeners, even if those files are deleted incrementally until the Worker is finally deinit'd. The listeners themselves are quite small, so it's in the interest of good faith cleanup. |
|
We might be able to leverage the callbacks the command queue gives us for when a file is deleted, rather than doing it early as we currently do, if we want to clean up as types are deleted. |
Added another PR, that still frees the listeners, from command queue callbacks: #420 |
Fixes #418
Summary
Fixes EXC_BAD_ACCESS crash in
processMessages()caused by premature listener deletion in CommandQueue delete methods. When a resource (e.g. ViewModelInstance) was deleted, its C++ listener was immediatelydeleted, but the C++ CommandQueue still held a raw pointer to it — ifprocessMessages()ran before the queue processed the delete command, it would invoke callbacks through a dangling pointer.The fix removes the immediate
delete listenerfrom all 6 affected methods (deleteFile,deleteArtboard,deleteViewModelInstance,deleteImage,deleteFont,deleteAudio). Listeners are already cleaned up indealloc, and the__weakobserver pattern ensures callbacks on orphaned listeners are safe no-ops.Test plan
lists_demo.rivteardown test — crashed on iteration 1 without fix