Created attachment 149778 [details] crash backtrace Thread 1 received signal SIGSEGV, Segmentation fault. 0x00007ffa33edc1e0 in QWindowsPipeReader::waitForReadyRead () at C:\Packaging\KritaDepsBuild\ext_qt\s\qtbase\src\corelib\io/qwindowspipereader.cpp:317 Happened when rendering an animation to gif. Nightly 93e3db259c
By the way this is when testing under Application Verifier.
Created attachment 149928 [details] ASAN output Use-after free ASAN output from an ASAN+UBSAN build (mangled names, no line numbers)
Created attachment 149929 [details] ASAN output with line numbers
The root of evil seems to be this QApplication::processEvents() call [1]. This will be called from signals activated inside the blocking QProcess::waitForFinished() call. Inside processEvents(), if the child ffmpeg process has died, it will deleteLater() the pipe readers of the active QProcess. The deferred delete is also processed in processEvents(). When control returns to QProcess, it is now handling a deleted pipe reader, hence the UAF. [1]: https://invent.kde.org/graphics/krita/-/blob/0aa50565d3b1974338d9c25f39a5302a5be0a591/libs/ui/animation/KisFFMpegWrapper.cpp#L195
Hey Alvin, I'm not even sure why processEvents is necessary in that particular callback. I'll have to check out if removing that call has any regressions and report back my findings. If we really need to "process" events, then I think we simply want to call a non-blocking version...
Git commit abb211e6decfbcb2f5c95a261b95754a8b5b907c by Eoin O'Neill. Committed on 29/06/2022 at 23:31. Pushed by eoinoneill into branch 'master'. Hotfix: Attempt to prevent processEvents call when dealing with FFMPEG sub-process. This is a bandaid -- we need remove the need of "blocking" ffmpeg processes and instead opt for a futures based approach. M +5 -2 libs/ui/animation/KisFFMpegWrapper.cpp https://invent.kde.org/graphics/krita/commit/abb211e6decfbcb2f5c95a261b95754a8b5b907c
Alvin, I think I may have solved this issue, but I would like you to test it on Windows to see if the problem persists for you (I don't have a windows test environment atm) and if they do, please reopen the bug. Diving into the FFMPEGWrapper code, there are issues with how we are treading the subprocess. By having a "blocking" ffmpeg process, we are preventing the UI from updating. This is a bad design and we should be approaching this with a "Futures" based approach (I.E. schedule a task to occur after process has concluded.) At some point I plan to redo parts of how our exporter works, which would be a good time to change this approach.
Thanks for trying to fix it. Honestly I think the change may not fix the issue -- it seems QProcess::state is set to NotRunning only after QProcess has detected the process has died, so the condition may still be true if the process has just died. But I'll need to do a new ASAN build and test it out to tell. (I could also test with Application Verifier, but I feel it is a bit less reliable than ASAN, and it's also quite slow...) (In reply to Eoin O'Neill from comment #7) > Diving into the FFMPEGWrapper code, there are issues with how we are > treading the subprocess. By having a "blocking" ffmpeg process, we are > preventing the UI from updating. This is a bad design and we should be > approaching this with a "Futures" based approach (I.E. schedule a task to > occur after process has concluded.) The biggest issue is the waitForFinished call. I wonder if it can instead start a nested QEventLoop, and rely on the QProcess::finished signal, perhaps also with a timeout QTimer to exit the event loop? This may even remove the need to call processEvents to update UI. Running it on another thread or using QFuture can also work, but that may be a bit overkill.
> Honestly I think the change may not fix the issue Ah sorry, I do take that back. Your fix does work because it sets the process state and call deleteLater, instead of deleting the pipe reader immediately. Skipping processEvents will prevent triggering the deferred delete and thus prevent the UAF. Thanks again :)
Yeah no worries. :) I agree with you that the whole thing needs a bit of a work -- but for now I think this will have to do since 5.1 is coming soon. I have an interest in taking another crack at the animation exporter once I get the audio branch in a nicer place, so it will be high(ish) on my todo list. We have a few bugs in the exporter that could be resolved or made streamlined in general. I will set a reminder in my email client to take a look at this specific bug report again in a few months just so I don't lose track of it. :) Cheers! Eoin