Rewrite windowsupdate package to fix COM memory leaks by directionless · Pull Request #2615 · kolide/launcher

directionless · 2026-03-05T04:34:34Z

The Windows Update COM interop code had severe memory leaks caused by missing IUnknown.Release() and VARIANT.Clear() calls throughout the object lifecycle. The intermediate oleconv package made this worse by consuming VARIANT pointers before callers could clean them up, turning every property access into a leak.

This commit rewrites the package following the patterns already established in ee/wmi/wmi.go:

Add olehelpers.go with centralized property/method accessors that defer VARIANT.Clear() before extracting Go values, ensuring every COM call site releases native memory correctly.
Release the IUnknown ref from CreateObject after QueryInterface (CreateObject returns refcount=1, QueryInterface bumps to 2, so the IUnknown ref must be released to avoid a leak).
Defer IDispatch.Release() in every toXxx conversion function so intermediate dispatch pointers don't outlive their scope.
Add explicit Release() methods on types that retain an IDispatch for subsequent method calls (IUpdateSession, IUpdate).
Delete pkg/windows/oleconv entirely — it had no other consumers and its abstraction actively prevented correct resource management.
Move package from pkg/windows/windowsupdate to ee/windowsupdate.
Add Windows integration tests exercising the full COM lifecycle (session creation, searcher, history queries).
Add BenchmarkQueryHistory that measures non-Go memory growth per iteration using ee/tables/ci, with a 64 KiB/op threshold that will fail CI if native memory leaks regress.
Add RequireNonGolangMemoryBelowThreshold to ee/tables/ci for reusable native memory leak detection in benchmarks.
Add ee/windowsupdate to the test-bench-tables Makefile target.

See ee/windowsupdate/COM_LEAK_ANALYSIS-2026-04.md for the full root cause analysis and design rationale.

The Windows Update COM interop code had severe memory leaks caused by missing IUnknown.Release() and VARIANT.Clear() calls throughout the object lifecycle. The intermediate oleconv package made this worse by consuming VARIANT pointers before callers could clean them up, turning every property access into a leak. This commit rewrites the package following the patterns already established in ee/wmi/wmi.go: - Add olehelpers.go with centralized property/method accessors that defer VARIANT.Clear() before extracting Go values, ensuring every COM call site releases native memory correctly. - Release the IUnknown ref from CreateObject after QueryInterface (CreateObject returns refcount=1, QueryInterface bumps to 2, so the IUnknown ref must be released to avoid a leak). - Defer IDispatch.Release() in every toXxx conversion function so intermediate dispatch pointers don't outlive their scope. - Add explicit Release() methods on types that retain an IDispatch for subsequent method calls (IUpdateSession, IUpdate). - Delete pkg/windows/oleconv entirely — it had no other consumers and its abstraction actively prevented correct resource management. - Move package from pkg/windows/windowsupdate to ee/windowsupdate. - Add Windows integration tests exercising the full COM lifecycle (session creation, searcher, history queries). - Add BenchmarkQueryHistory that measures non-Go memory growth per iteration using ee/tables/ci, with a 64 KiB/op threshold that will fail CI if native memory leaks regress. - Add RequireNonGolangMemoryBelowThreshold to ee/tables/ci for reusable native memory leak detection in benchmarks. - Add ee/windowsupdate to the test-bench-tables Makefile target. See ee/windowsupdate/COM_LEAK_ANALYSIS-2026-04.md for the full root cause analysis and design rationale.

RebeccaMahany · 2026-03-19T16:02:07Z

+on process exit. See `ee/tables/windowsupdatetable/windowsupdate.go` and
+`cmd/launcher/query_windowsupdates_windows.go`.


Might be clearer to link to the PR, for if/when that workaround goes away.

Suggested change

on process exit. See `ee/tables/windowsupdatetable/windowsupdate.go` and

`cmd/launcher/query_windowsupdates_windows.go`.

on process exit. See https://github.com/kolide/launcher/pull/2185.

RebeccaMahany · 2026-03-19T16:11:49Z

+func toICategories(categoriesDisp *ole.IDispatch) ([]*ICategory, error) {
+	count, err := getPropertyInt32(categoriesDisp, "Count")
+	if err != nil {
+		return nil, fmt.Errorf("Count: %w", err)


Nitpick (applies to all the wrapped errors in this package) for slightly more verbosity + adhering to error string capitalization standards --

Suggested change

return nil, fmt.Errorf("Count: %w", err)

return nil, fmt.Errorf("getting Count property: %w", err)

RebeccaMahany · 2026-03-19T16:14:04Z

+		return nil, fmt.Errorf("Order: %w", err)
+	}
+
+	// Parent is commented out to avoid infinite recursion (Parent -> Category -> Parent ...)


Suggested change

// Parent is commented out to avoid infinite recursion (Parent -> Category -> Parent ...)

// Parent is omitted to avoid infinite recursion (Parent -> Category -> Parent ...)

RebeccaMahany · 2026-03-19T16:14:17Z

+		return nil, fmt.Errorf("Type: %w", err)
+	}
+
+	// Updates is commented out to avoid pulling the full update tree per category.


Suggested change

// Updates is commented out to avoid pulling the full update tree per category.

// Updates is omitted to avoid pulling the full update tree per category.

RebeccaMahany · 2026-03-19T16:17:26Z

+	if disp == nil {
+		return nil, nil
+	}


Do we need this nil check elsewhere? We don't have it for e.g. toISearchResult, toICategories, toICategory

RebeccaMahany · 2026-03-19T16:25:02Z

+//nolint:unused
+func toIUpdateCollection(updates []*IUpdate) (*ole.IDispatch, error) {


Should we remove?

RebeccaMahany · 2026-03-19T16:32:32Z

+	if identityDisp, err := getPropertyDispatch(updateDisp, "Identity"); err != nil {
+		return nil, err
+	} else if identityDisp != nil {
+		// toIUpdateIdentity calls Release() on identityDisp internally


As I'm reading the PR, I am thinking it might be nice to make it clearer via function names when the function is handling calling Release versus when it's the caller's responsibility. Maybe a pattern like toIUpdateIdentityWithRelease?

Post-review, this is the biggest comment I have -- it's hard to track exactly when a disp should be released or when it's already been handled, and since we can panic on double-release it feels important to document more strongly

RebeccaMahany · 2026-03-19T16:38:00Z

 	ServiceID                           string
 }

 func toIUpdateSearcher(updateSearcherDisp *ole.IDispatch) (*IUpdateSearcher, error) {


When will this disp get released?

RebeccaMahany · 2026-03-19T16:38:55Z

+	searchResultDisp, err := callMethodDispatch(iUpdateSearcher.disp, "Search", criteria)
 	if err != nil {
-		return nil, fmt.Errorf("calling Search: %w", err)
+		return nil, fmt.Errorf("Search: %w", err)


As with the others -- I would revert all the changes to the error strings in this file to conform to error string casing standards:

Suggested change

return nil, fmt.Errorf("Search: %w", err)

return nil, fmt.Errorf("calling Search: %w", err)

RebeccaMahany · 2026-03-19T16:42:05Z

+// Release()/Clear() calls would show up. The test fails if per-op
+// native growth exceeds the threshold.


Since we aren't failing the benchmark anymore --

Suggested change

// Release()/Clear() calls would show up. The test fails if per-op

// native growth exceeds the threshold.

// Release()/Clear() calls would show up.

directionless force-pushed the seph/windows-memory branch 2 times, most recently from 31ac81a to f56854c Compare March 5, 2026 22:11

directionless marked this pull request as ready for review March 5, 2026 22:59

This was referenced Mar 6, 2026

windowsupdate tests #2621

Merged

Add kolide_toml table #2619

Merged

directionless added 2 commits March 19, 2026 08:58

fix

74d2614

directionless force-pushed the seph/windows-memory branch from 0ccbaa4 to 74d2614 Compare March 19, 2026 13:00

fix words

bca2750

RebeccaMahany reviewed Mar 19, 2026

View reviewed changes

RebeccaMahany mentioned this pull request Mar 30, 2026

Simple TUF based downloader #2600

Merged

Navigation Menu

Uh oh!

Rewrite windowsupdate package to fix COM memory leaks - #2615

Rewrite windowsupdate package to fix COM memory leaks#2615
directionless wants to merge 3 commits into
kolide:mainfrom
directionless:seph/windows-memory

directionless commented Mar 5, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

RebeccaMahany Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		on process exit. See `ee/tables/windowsupdatetable/windowsupdate.go` and
		`cmd/launcher/query_windowsupdates_windows.go`.

	on process exit. See `ee/tables/windowsupdatetable/windowsupdate.go` and
	`cmd/launcher/query_windowsupdates_windows.go`.
	on process exit. See https://github.com/kolide/launcher/pull/2185.

	return nil, fmt.Errorf("Count: %w", err)
	return nil, fmt.Errorf("getting Count property: %w", err)

	// Parent is commented out to avoid infinite recursion (Parent -> Category -> Parent ...)
	// Parent is omitted to avoid infinite recursion (Parent -> Category -> Parent ...)

	// Updates is commented out to avoid pulling the full update tree per category.
	// Updates is omitted to avoid pulling the full update tree per category.

		//nolint:unused
		func toIUpdateCollection(updates []IUpdate) (ole.IDispatch, error) {

	return nil, fmt.Errorf("Search: %w", err)
	return nil, fmt.Errorf("calling Search: %w", err)

		// Release()/Clear() calls would show up. The test fails if per-op
		// native growth exceeds the threshold.

Uh oh!

Conversation

directionless commented Mar 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants