Fix: Critical dangling pointer bug in NexusBridge::nxmFilesAvailable#2288
Fix: Critical dangling pointer bug in NexusBridge::nxmFilesAvailable#2288DLST316 wants to merge 4 commits intoModOrganizer2:masterfrom
Conversation
|
Thanks for pointing that out, unfortunately your fix introduces other issues. In particular, you're assuming that there is a single receiver to the signal, where in typical Qt fashion, many objects can connect to the signal, hence making it impossible to de-allocate the lists since the order of the slots is not known. This signal is meant to be used by MO2 plugins, so the SLOT are not within MO2 code. Right now, I don't think any (main) plugin uses that otherwise we would have many crash reports for this. A proper fix would be to modify the interface to not pass pointers to the signal but actual object, or to change this to use callbacks rather than signals/slots. |
|
@Holt59 Thank you for the detailed guidance! This is very helpful. The proposed solution of modifying the interface makes perfect sense. I would like to try implementing this proper fix myself. I will close this current PR, as the approach here is not the right direction, and will open a new one if I can create a working solution based on your suggestion. Since I'm still learning the codebase, it might take me some time, so please don't feel the need to wait on me if this is a high-priority issue. I'll continue to follow the project. Thanks again! |
The Problem
I discovered a critical dangling pointer bug in the
NexusBridge::nxmFilesAvailablefunction. The original code stored the address of a stack-allocated variable (temp) in a list. When the function returned, this created a dangling pointer, leading to a high risk of application crashes or memory corruption when thefilesAvailablesignal was processed.My Solution
To fix this immediate and critical crash risk, I have modified the function to allocate the
ModRepositoryFileInfoobjects on the heap usingnew. This ensures that the pointers passed via thefilesAvailablesignal remain valid after the function returns, preventing the crash.This is a minimal, targeted fix designed to resolve the most severe aspect of the bug without altering the existing
IModRepositoryBridgeinterface.Known Issues & Discussion
I understand that this solution is not perfect and introduces a new challenge: memory management. The
newly allocated objects are not currently deallocated, which will result in a memory leak.The ideal, long-term solution would likely involve changing the
IModRepositoryBridgeinterface to pass a list of objects by value (e.g.,QList<ModRepositoryFileInfo>) instead of pointers. However, as I am not deeply familiar with the overall architecture of this large project, I was hesitant to make such a significant change to a core interface.I am submitting this PR to solve the immediate crash risk and to open a discussion with the project maintainers. I would greatly appreciate your feedback and guidance on the best path forward.
Specifically, I would like to ask:
filesAvailablesignal? I would be happy to add the necessary memory deallocation logic (e.g.,qDeleteAll) there as a follow-up commit.Thank you for your time and for maintaining this great project!