fix: implement singleton noop IDispatch callback for async WUA methods#16
Conversation
The asynchronous WUA methods (BeginSearch, BeginDownload, BeginInstall, BeginUninstall) require a non-NULL IUnknown* callback argument. Passing NULL (VT_NULL) causes DISP_E_TYPEMISMATCH (0x80020005). This commit introduces a minimal IDispatch singleton (newNoopDispatch) whose Invoke does nothing. Key design decisions: - Global singleton via sync.Once to avoid unbounded memory growth - atomic.AddInt32 for thread-safe COM reference counting - Disable go vet unsafeptr check in CI (required by syscall.NewCallback) Change-Id: I44dc095451daab5b44cee9d13a8c86776b009ecd Co-developed-by: Qoder <noreply@qoder.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16 +/- ##
==========================================
+ Coverage 43.89% 45.12% +1.23%
==========================================
Files 34 35 +1
Lines 1392 1436 +44
==========================================
+ Hits 611 648 +37
- Misses 562 565 +3
- Partials 219 223 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 minimal, stateless IDispatch COM callback implementation (newNoopDispatch) to replace nil arguments in asynchronous Windows Update Agent (WUA) methods (BeginDownload, BeginInstall, BeginUninstall, and BeginSearch), resolving DISP_E_TYPEMISMATCH errors. The review feedback highlights several critical and medium-severity improvements: addressing potential compilation failures on non-Windows platforms by adding a stub file or applying build tags consistently; initializing the pVarResult out-parameter to VT_EMPTY in ncInvoke to prevent potential memory corruption or crashes; and improving safety and code clarity by directly referencing the globalNoop singleton instead of performing unsafe conversions from the this pointer, along with adding defensive checks for null pointers.
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.
- Update go.mod minimum version from 1.23 to 1.24 - Update CI matrix from [1.23, 1.24, 1.25] to [1.24, 1.25, 1.26] - Update lint job Go version to 1.24 Change-Id: I90c990b7f42adefd15d4e5890a2a361d959b2c47 Co-developed-by: Qoder <noreply@qoder.com>
ed54222 to
8bd1d23
Compare
- Use globalNoop singleton directly instead of unsafe this-pointer casts - Add defensive null check for iid in ncQueryInterface - Initialize pVarResult to VT_EMPTY in ncInvoke per COM contract - Add olecallback_other.go stub for non-Windows compilation Change-Id: Ic6fe7ef0ad19930acf724a90ea9ee51993c695d3 Co-developed-by: Qoder <noreply@qoder.com>
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request updates the Go version to 1.24 and introduces a minimal, stateless, no-op IDispatch callback implementation (newNoopDispatch) to prevent DISP_E_TYPEMISMATCH errors during asynchronous Windows Update Agent (WUA) operations. The review feedback focuses on improving COM compliance and code safety within the callback implementation. Specifically, it is recommended to return E_POINTER when the output pointer is null, cast the 'this' pointer to access the reference counter instead of referencing the global singleton directly, and use the go-ole package's VARIANT type and VT_EMPTY constant instead of raw pointer dereferencing with magic numbers.
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.
| const ( | ||
| hrSOK = uintptr(0x00000000) | ||
| hrENoInterface = uintptr(0x80004002) | ||
| hrENotImpl = uintptr(0x80004001) | ||
| ) |
There was a problem hiding this comment.
To comply with COM standards, QueryInterface should return E_POINTER (0x80004003) if the output pointer argument ppvObject is null. Let's define hrEPointer to support this check.
| const ( | |
| hrSOK = uintptr(0x00000000) | |
| hrENoInterface = uintptr(0x80004002) | |
| hrENotImpl = uintptr(0x80004001) | |
| ) | |
| const ( | |
| hrSOK = uintptr(0x00000000) | |
| hrEPointer = uintptr(0x80004003) | |
| hrENoInterface = uintptr(0x80004002) | |
| hrENotImpl = uintptr(0x80004001) | |
| ) |
| func ncQueryInterface(this, iid, ppvObject uintptr) uintptr { | ||
| if iid == 0 { | ||
| if ppvObject != 0 { | ||
| *(*uintptr)(unsafe.Pointer(ppvObject)) = 0 | ||
| } | ||
| return hrENoInterface | ||
| } | ||
| guid := (*ole.GUID)(unsafe.Pointer(iid)) | ||
| out := (*uintptr)(unsafe.Pointer(ppvObject)) | ||
| if ole.IsEqualGUID(guid, ole.IID_IUnknown) || ole.IsEqualGUID(guid, ole.IID_IDispatch) { | ||
| atomic.AddInt32(&globalNoop.ref, 1) | ||
| if out != nil { | ||
| *out = this | ||
| } | ||
| return hrSOK | ||
| } | ||
| if out != nil { | ||
| *out = 0 | ||
| } | ||
| return hrENoInterface | ||
| } |
There was a problem hiding this comment.
In COM, QueryInterface must return E_POINTER if ppvObject is null. Additionally, instead of referencing the global globalNoop singleton directly, it is more idiomatic and robust to cast the this pointer to *noopCallback to access the instance's reference counter.
func ncQueryInterface(this, iid, ppvObject uintptr) uintptr {
if ppvObject == 0 {
return hrEPointer
}
if iid == 0 {
*(*uintptr)(unsafe.Pointer(ppvObject)) = 0
return hrENoInterface
}
guid := (*ole.GUID)(unsafe.Pointer(iid))
out := (*uintptr)(unsafe.Pointer(ppvObject))
if ole.IsEqualGUID(guid, ole.IID_IUnknown) || ole.IsEqualGUID(guid, ole.IID_IDispatch) {
p := (*noopCallback)(unsafe.Pointer(this))
atomic.AddInt32(&p.ref, 1)
*out = this
return hrSOK
}
*out = 0
return hrENoInterface
}| func ncAddRef(this uintptr) uintptr { | ||
| return uintptr(uint32(atomic.AddInt32(&globalNoop.ref, 1))) | ||
| } | ||
|
|
||
| func ncRelease(this uintptr) uintptr { | ||
| // Singleton object: it is never actually freed even if the count reaches | ||
| // zero. We still maintain the counter so the value returned to the COM | ||
| // caller is meaningful. | ||
| return uintptr(uint32(atomic.AddInt32(&globalNoop.ref, -1))) | ||
| } |
There was a problem hiding this comment.
Instead of referencing the global globalNoop singleton directly, cast the this pointer to *noopCallback to access the instance's reference counter. This is more idiomatic for COM method implementations.
func ncAddRef(this uintptr) uintptr {
p := (*noopCallback)(unsafe.Pointer(this))
return uintptr(uint32(atomic.AddInt32(&p.ref, 1)))
}
func ncRelease(this uintptr) uintptr {
// Singleton object: it is never actually freed even if the count reaches
// zero. We still maintain the counter so the value returned to the COM
// caller is meaningful.
p := (*noopCallback)(unsafe.Pointer(this))
return uintptr(uint32(atomic.AddInt32(&p.ref, -1)))
}| func ncInvoke(this, dispIdMember, riid, lcid, wFlags, pDispParams, pVarResult, pExcepInfo, puArgErr uintptr) uintptr { | ||
| if pVarResult != 0 { | ||
| *(*uint16)(unsafe.Pointer(pVarResult)) = 0 // VT_EMPTY | ||
| } | ||
| return hrSOK | ||
| } |
There was a problem hiding this comment.
Instead of using raw pointer dereferencing with magic numbers (*(*uint16)(unsafe.Pointer(pVarResult)) = 0), cast pVarResult to *ole.VARIANT and use ole.VT_EMPTY from the go-ole package. This is much cleaner and less error-prone.
| func ncInvoke(this, dispIdMember, riid, lcid, wFlags, pDispParams, pVarResult, pExcepInfo, puArgErr uintptr) uintptr { | |
| if pVarResult != 0 { | |
| *(*uint16)(unsafe.Pointer(pVarResult)) = 0 // VT_EMPTY | |
| } | |
| return hrSOK | |
| } | |
| func ncInvoke(this, dispIdMember, riid, lcid, wFlags, pDispParams, pVarResult, pExcepInfo, puArgErr uintptr) uintptr { | |
| if pVarResult != 0 { | |
| v := (*ole.VARIANT)(unsafe.Pointer(pVarResult)) | |
| v.VT = ole.VT_EMPTY | |
| } | |
| return hrSOK | |
| } |
- Add hrEPointer (0x80004003) and return it when ppvObject is null - Simplify ncQueryInterface by removing redundant nil checks after early E_POINTER guard - Use (*ole.VARIANT).VT = ole.VT_EMPTY instead of raw uint16 cast Change-Id: Ibc97c62942a7aec7bd87c7bedb18eb9939472d29 Co-developed-by: Qoder <noreply@qoder.com>
1 participant
relate to #15
The asynchronous WUA methods (BeginSearch, BeginDownload, BeginInstall, BeginUninstall) require a non-NULL IUnknown* callback argument. Passing NULL (VT_NULL) causes DISP_E_TYPEMISMATCH (0x80020005).
This commit introduces a minimal IDispatch singleton (newNoopDispatch) whose Invoke does nothing. Key design decisions:
Change-Id: I44dc095451daab5b44cee9d13a8c86776b009ecd
Co-developed-by: Qoder noreply@qoder.com