Fix critical null pointer dereference in Win32Window::WndProc #292
+688
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideFixes a critical null pointer dereference in Win32Window::WndProc by adding a safe GetThisFromHandle call with a null check and optional debug logging, and also introduces a generated Gradle configuration cache HTML report file under android/build/reports/problems/. Sequence diagram for updated Win32Window::WndProc message handlingsequenceDiagram
participant OS as OS_Windows
participant WndProc as Win32Window_WndProc
participant Win32Window as Win32Window_instance
participant DefProc as DefWindowProc
OS->>WndProc: WndProc(window, message, wparam, lparam)
alt message == WM_NCCREATE
WndProc->>WndProc: get CREATESTRUCT from lparam
WndProc->>Win32Window: construct instance (lpCreateParams)
WndProc->>WndProc: EnableFullDpiSupportIfAvailable(window)
WndProc->>Win32Window: that.window_handle_ = window
end
WndProc->>WndProc: that = GetThisFromHandle(window)
alt that != nullptr
WndProc->>Win32Window: MessageHandler(window, message, wparam, lparam)
Win32Window-->>WndProc: LRESULT
WndProc-->>OS: LRESULT
else that == nullptr (early message before WM_NCCREATE)
opt DEBUG_EARLY_MESSAGES defined
WndProc->>WndProc: format debug_msg with message and window
WndProc->>WndProc: OutputDebugString(debug_msg)
end
WndProc->>DefProc: DefWindowProc(window, message, wparam, lparam)
DefProc-->>WndProc: LRESULT
WndProc-->>OS: LRESULT
end
Class diagram for Win32Window and WndProc changesclassDiagram
class Win32Window {
+HWND window_handle_
+static LRESULT CALLBACK WndProc(HWND window, UINT message, WPARAM wparam, LPARAM lparam)
+LRESULT MessageHandler(HWND window, UINT message, WPARAM wparam, LPARAM lparam)
}
class Helpers {
+static Win32Window* GetThisFromHandle(HWND window)
+static void EnableFullDpiSupportIfAvailable(HWND window)
}
Win32Window ..> Helpers : uses
Win32Window ..> Win32Window : GetThisFromHandle returns this
Win32Window : WndProc
Win32Window : MessageHandler
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The Gradle problems-report.html file under android/build/reports appears to be a generated artifact; consider removing it from version control and adding it to .gitignore to avoid bloating the repo with build outputs.
- In Win32Window::WndProc, the new debug logging block uses OutputDebugString directly; if the project has an existing logging or tracing utility, it would be better to route this through that for consistency and configurability.
- There is a minor style issue in win32_window.cpp around the new null-check block (extra blank line and trailing whitespace after the closing brace); aligning this with the surrounding formatting would keep the file consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Gradle problems-report.html file under android/build/reports appears to be a generated artifact; consider removing it from version control and adding it to .gitignore to avoid bloating the repo with build outputs.
- In Win32Window::WndProc, the new debug logging block uses OutputDebugString directly; if the project has an existing logging or tracing utility, it would be better to route this through that for consistency and configurability.
- There is a minor style issue in win32_window.cpp around the new null-check block (extra blank line and trailing whitespace after the closing brace); aligning this with the surrounding formatting would keep the file consistent.
## Individual Comments
### Comment 1
<location> `windows/runner/win32_window.cpp:171-173` </location>
<code_context>
- } else if (Win32Window* that = GetThisFromHandle(window)) {
+ }
+
+ Win32Window* that = GetThisFromHandle(window);
+ if (that) {
return that->MessageHandler(window, message, wparam, lparam);
+ } else {
+ // Optional: debug message for messages arriving before WM_NCCREATE
</code_context>
<issue_to_address>
**issue (bug_risk):** WM_NCCREATE will now also go through MessageHandler, which changes previous behavior.
Previously, `WM_NCCREATE` exited before reaching `MessageHandler`, but now it also flows through `GetThisFromHandle(window)` and into `MessageHandler` (if the handle is registered). If this behavior change isn’t desired, consider keeping the `else` or explicitly bypassing `WM_NCCREATE` to match the old behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
close #289
Description:
This PR addresses a critical null pointer dereference in the Win32Window::WndProc function that could crash the application in rare circumstances.
Issue:
The WndProc function calls GetThisFromHandle(window) and immediately dereferences the returned pointer:
else if (Win32Window* that = GetThisFromHandle(window)) {
return that->MessageHandler(window, message, wparam, lparam);
}
If a Windows message arrives before WM_NCCREATE, GWLP_USERDATA is not yet initialized, causing GetThisFromHandle(window) to return nullptr.
Dereferencing this pointer results in undefined behavior or a crash.
Solution:
Added a null check before calling MessageHandler:
Win32Window* that = GetThisFromHandle(window);
if (that) {
return that->MessageHandler(window, message, wparam, lparam);
}
Messages received before WM_NCCREATE now safely fall through to DefWindowProc, preventing crashes.
Optional debug logging was added for early messages (guarded by #ifdef DEBUG_EARLY_MESSAGES) to assist with debugging if needed.
Impact:
Fixes crashes caused by early messages on some systems.
No behavior change for normal message handling after WM_NCCREATE.
Tested:
Compiled and ran the application on Windows.
Verified no crash occurs with messages arriving before and after WM_NCCREATE.
Summary by Sourcery
Prevent Win32 window message handler from dereferencing a null window instance before initialization and add generated Gradle configuration cache problem report artifact.
Bug Fixes:
Enhancements: