Security: Fix unsafe COM pointer handling in settings menus and improve consistency

[coderabbitai[bot]]

Problem

A potential crash vulnerability was identified in DynamicCubemaps.cpp where a COM pointer (ID3D11Texture2D* tempTexture) is declared uninitialized and released unconditionally after a try/catch block. If an exception occurs before the pointer is assigned, tempTexture->Release() will dereference garbage memory, leading to undefined behavior or crashes.

Location: src/Features/DynamicCubemaps.cpp in the Dynamic Cubemap Creator export functionality

Root Cause

The pattern found:

ID3D11Texture2D* tempTexture;  // Uninitialized
try {
    DX::ThrowIfFailed(device->CreateTexture2D(&texDesc, subresourceData, &tempTexture));
    // ... other operations
} catch (const std::exception& e) {
    logger::error("...");
}
tempTexture->Release();  // Unsafe: may dereference garbage

Impact

• Security: Potential memory corruption or crash
• Reliability: Unpredictable behavior when exceptions occur during COM object creation
• User Experience: Application crashes during settings interaction

Scope for Investigation

This issue was discovered during PR #1409 review. A broader audit is needed to:

1. Settings Menus: Check all settings-related files (src/Features/*Settings*.cpp, UI code with ImGui) for similar patterns
2. COM Pointer Convention: Establish and document the preferred COM pointer handling approach for this codebase
3. Exception Safety: Review exception handling around COM object lifecycle management

Proposed Solutions

Immediate Fix

ID3D11Texture2D* tempTexture = nullptr;  // Initialize to nullptr
// ... try/catch block
if (tempTexture) {
    tempTexture->Release();
    tempTexture = nullptr;
}

Long-term Improvement

Consider adopting consistent COM smart pointer usage (e.g., Microsoft::WRL::ComPtr) to eliminate manual memory management:

ComPtr<ID3D11Texture2D> tempTexture;
DX::ThrowIfFailed(device->CreateTexture2D(&texDesc, subresourceData, tempTexture.GetAddressOf()));
// Automatic cleanup, exception-safe

Action Items

• [ ] Fix the immediate issue in DynamicCubemaps.cpp
• [ ] Audit settings menu files for similar patterns
• [ ] Document COM pointer handling conventions for the project
• [ ] Consider migrating to smart pointers for new COM code
• [ ] Add static analysis or linting rules to catch similar issues

References

• Source PR: https://github.com/doodlum/skyrim-community-shaders/pull/1409
• Comment: https://github.com/doodlum/skyrim-community-shaders/pull/1409#discussion_r2295465072
• Requester: @alandtse