orkaudio: Fix leak in ConfigManager #112
Conversation
|
Fixed by removing Seems like this is not required, delete takes care of that release itself. Before After
|
|
Crashes on |
…to fix-leak-config-manager * 'fix-leak-config-manager' of github.com:voiceip/oreka: orkaudio: Fix leak in Opus Header Write (#110)
|
Not exactly sure, but this looked like a case of shadow variable. Changing it to class reference, both resolves the leak & seg fault. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
orkbasecxx/ConfigManager.cpp (1)
176-179: Improve error handling to ensure proper cleanup.The current implementation exits the program on failure without proper cleanup. This could lead to resource leaks.
Consider using RAII or ensuring cleanup on all exit paths:
void ConfigManager::Initialize() { struct Cleanup { XercesDOMParser** parser; bool* failed; Cleanup(XercesDOMParser** p, bool* f) : parser(p), failed(f) {} ~Cleanup() { if (*failed) { delete *parser; *parser = nullptr; XMLPlatformUtils::Terminate(); } } } cleanup(&m_parser, &failed); // ... rest of the function ... if (failed) { throw std::runtime_error("Configuration initialization failed"); } }
🧹 Nitpick comments (1)
orkbasecxx/ConfigManager.cpp (1)
113-119: LGTM! Memory management looks correct.The change to make m_parser a member variable and removing doc->release() is the right approach, as the parser owns the document's memory. This fixes both the memory leak and segmentation fault mentioned in the PR objectives.
Consider initializing m_parser in the constructor's initialization list and adding nullptr checks:
class ConfigManager { private: XercesDOMParser* m_parser; public: ConfigManager() : m_parser(nullptr) {} // ... rest of the class };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
orkbasecxx/ConfigManager.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Image
- GitHub Check: Build Orkweb & Orktrack
| @@ -100,7 +100,7 @@ void ConfigManager::Initialize() | |||
| } | |||
| } | |||
|
|
|||
| XMLPlatformUtils::Initialize(); | |||
| XMLPlatformUtils::Initialize(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add corresponding XMLPlatformUtils::Terminate() call.
While XMLPlatformUtils::Initialize() is now correctly active, there's no matching Terminate() call. This could lead to resource leaks.
Consider adding cleanup in the destructor:
ConfigManager::~ConfigManager() {
if (m_parser) {
delete m_parser;
m_parser = nullptr;
}
XMLPlatformUtils::Terminate();
}
WalkthroughThe pull request updates the Changes
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Leak Info
Attempted Fix
Throws this exception when run without debug mode
Summary by CodeRabbit