Asynchronous WUA methods - #17
Conversation
…l) require a non-NULL IUnknown* callback argument. Passing NULL (VT_NULL) makes them fail with DISP_E_TYPEMISMATCH (0x80020005), so I fixed the functions
Co-authored-by: Copilot <copilot@github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a universal no-op COM callback (noopCallback) to prevent DISP_E_TYPEMISMATCH errors when calling asynchronous Windows Update Agent (WUA) methods, and adds GetIsCompleted methods to track job status. Feedback highlights a memory leak in newNoopCallback where new instances are continuously appended to a global slice; implementing a shared singleton is recommended to resolve this and fix the corresponding test. Additionally, a test query for ole.IID_IDispatch in TestNoopCallback_QueryInterface should be updated to a supported callback IID since IDispatch is not supported by the custom callback implementation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
The changes are fine—no memory leaks. I tested them on a Windows 11 22H2 machine and upgraded to 25H2 without any issues. |
- Replace per-call allocation + keepAlive slice with sync.Once singleton - Fix olecallback_other.go stub to match new function signature - Fix TestNoopCallback_QueryInterface to use proper noopCallback object - Clean up stale IDispatch comments in olecallback.go Change-Id: I660e3afeb48e38ec4ad225e69c48365b83192da1 Co-developed-by: Qoder <noreply@qoder.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17 +/- ##
==========================================
+ Coverage 44.70% 46.07% +1.36%
==========================================
Files 35 35
Lines 1436 1452 +16
==========================================
+ Hits 642 669 +27
+ Misses 566 562 -4
+ Partials 228 221 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors the asynchronous Windows Update Agent (WUA) callback mechanism by replacing the previous 7-entry IDispatch vtable with a 4-entry IUnknown-derived vtable, which correctly matches the expected layout of WUA callback interfaces and prevents vtable corruption. It also aggregates the COM Free-Threaded Marshaler (FTM) to make the callback agile across apartments, and introduces GetIsCompleted methods to query the live completion status of download and installation jobs. The review feedback highlights the need to restore safety checks in ncQueryInterface for null pointers (iid == 0 and ppvObject == 0) to prevent potential nil pointer dereferences, and suggests adding corresponding test cases to verify this robust behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func ncQueryInterface(this, iid, ppvObject uintptr) uintptr { | ||
| if ppvObject == 0 { | ||
| return hrEPointer | ||
| } | ||
| guid := (*ole.GUID)(unsafe.Pointer(iid)) | ||
| out := (*uintptr)(unsafe.Pointer(ppvObject)) | ||
| if iid == 0 { | ||
| *out = 0 | ||
| return hrENoInterface | ||
| p := (*noopCallback)(unsafe.Pointer(this)) |
There was a problem hiding this comment.
The ncQueryInterface function is missing safety checks for iid == 0 and ppvObject == 0. If iid is 0, casting it to *ole.GUID and calling ole.IsEqualGUID will cause a nil pointer dereference and panic. Additionally, if ppvObject is 0, standard COM behavior requires returning E_POINTER (0x80004003). Returning early with these checks restores the robustness of the previous implementation.
| func ncQueryInterface(this, iid, ppvObject uintptr) uintptr { | |
| if ppvObject == 0 { | |
| return hrEPointer | |
| } | |
| guid := (*ole.GUID)(unsafe.Pointer(iid)) | |
| out := (*uintptr)(unsafe.Pointer(ppvObject)) | |
| if iid == 0 { | |
| *out = 0 | |
| return hrENoInterface | |
| p := (*noopCallback)(unsafe.Pointer(this)) | |
| func ncQueryInterface(this, iid, ppvObject uintptr) uintptr { | |
| if ppvObject == 0 { | |
| return uintptr(0x80004003) // E_POINTER | |
| } | |
| out := (*uintptr)(unsafe.Pointer(ppvObject)) | |
| if iid == 0 { | |
| *out = 0 | |
| return hrENoInterface | |
| } | |
| guid := (*ole.GUID)(unsafe.Pointer(iid)) | |
| p := (*noopCallback)(unsafe.Pointer(this)) |
| // An unsupported IID must fail with E_NOINTERFACE and clear the out pointer. | ||
| unsupported := &ole.GUID{Data1: 0xdeadbeef, Data2: 0x1234, Data3: 0x5678} | ||
| out := uintptr(0xfff) | ||
| hr := ncQueryInterface(this, uintptr(unsafe.Pointer(unsupported)), uintptr(unsafe.Pointer(&out))) | ||
| if hr != hrENoInterface { | ||
| t.Errorf("QueryInterface(unsupported) = 0x%x, want E_NOINTERFACE", hr) | ||
| } | ||
| if out != 0 { | ||
| t.Errorf("QueryInterface(unsupported) out = 0x%x, want 0", out) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add test cases to verify the robustness of ncQueryInterface when handling iid == 0 (nil IID) and ppvObject == 0 (nil output pointer). This ensures that the safety checks behave as expected and prevents future regressions.
// An unsupported IID must fail with E_NOINTERFACE and clear the out pointer.
unsupported := &ole.GUID{Data1: 0xdeadbeef, Data2: 0x1234, Data3: 0x5678}
out := uintptr(0xfff)
hr := ncQueryInterface(this, uintptr(unsafe.Pointer(unsupported)), uintptr(unsafe.Pointer(&out)))
if hr != hrENoInterface {
t.Errorf("QueryInterface(unsupported) = 0x%x, want E_NOINTERFACE", hr)
}
if out != 0 {
t.Errorf("QueryInterface(unsupported) out = 0x%x, want 0", out)
}
// Safety checks: iid == 0 or ppvObject == 0
if hr := ncQueryInterface(this, uintptr(unsafe.Pointer(ole.IID_IUnknown)), 0); hr != 0x80004003 {
t.Errorf("QueryInterface with nil ppvObject = 0x%x, want E_POINTER (0x80004003)", hr)
}
out = uintptr(0xfff)
if hr := ncQueryInterface(this, 0, uintptr(unsafe.Pointer(&out))); hr != hrENoInterface {
t.Errorf("QueryInterface with nil iid = 0x%x, want E_NOINTERFACE", hr)
}
if out != 0 {
t.Errorf("QueryInterface with nil iid out = 0x%x, want 0", out)
}
}- Return E_POINTER when ppvObject == 0 - Return E_NOINTERFACE when iid == 0 (prevents nil deref on GUID cast) - Add corresponding test cases for both edge cases - Move guid/out dereference after null checks for safety Change-Id: I616164500ed2cf2c1761620bb2fb9ed4fd6a7070 Co-developed-by: Qoder <noreply@qoder.com>
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces GetIsCompleted methods to both IDownloadJob and IInstallationJob to retrieve the live completion status of asynchronous operations. Additionally, it refactors the no-op callback mechanism used in asynchronous Windows Update Agent (WUA) calls. The previous IDispatch implementation was replaced with a 4-entry IUnknown vtable layout (noopCallback) to prevent memory corruption and crashes during callback invocation. It also aggregates the COM Free-Threaded Marshaler (FTM) to ensure the callback is agile across different apartments, and adds comprehensive unit tests for the new callback behavior. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
@wisper-sabrina Thanks for your contributions. I take care of the broken ci. |
2 participants
The asynchronous WUA methods (BeginSearch, BeginDownload, BeginInstall) require a non-NULL IUnknown* callback argument. Passing NULL (VT_NULL) makes them fail with DISP_E_TYPEMISMATCH (0x80020005), so I fixed the functions