Apply language preference to lazily discovered MUI files - #39
Merged
scudette merged 4 commits intoJun 12, 2026
Merged
Conversation
MUI files found next to message DLLs outside System32 were appended to the cache in directory order, after the constructor's sorting pass, so the message table of an arbitrary language won (cs-CZ for Windows Defender; before the first-wins guard, zh-TW). Keep the compiled preference regex on the resolver and sort lazily discovered lists in include_muis.
|
|
Moved sorting logic out of the hot path so the mui cache is always kept sorted during discovery stage instead of sorting on the fetch (which is very frequent). I also noticed poor locking so I added a refactor to ensure locks are always taken when we touch the cache maps. Also ran some staticcheck to clean up the code a bit. Upgraded depedencies to avoid some CVEs.
scudette
added a commit
to Velocidex/velociraptor
that referenced
this pull request
Jun 16, 2026
…ntly (#4877) `EventTable._Close()` has to release `mu` while it waits for the old query goroutines to drain, because they call `Tracer()` (which takes `mu`) on their way out. But that opens a window: a concurrent `StartQueries()`, for example `set_server_monitoring` racing the artifact-modification reload loop can enter, start its own queries, and then have its `self.wg` and `self.cancel` overwritten when the first caller re-acquires the lock. The overwritten generation keeps running but is untracked and can never be cancelled. Every race costs one leaked generation; we hit this in production and saw the same SERVER_EVENT artifact collecting 6 times in parallel after a burst of table reloads. The fix keeps the unlock-during-drain (it is required) and instead serializes updaters: a dedicated `update_mu` is held for the whole `StartQueries()`/`Close()` operation, so no second updater can enter the drain window, while `mu` stays free for the query goroutines' callbacks. Also fixes `Wait()` reading `self.wg` without the lock. The new test (`TestConcurrentUpdatesDoNotLeakQueries`) runs four concurrent updaters alternating between two artifact sets and then installs an empty table. If any generation was orphaned its queries can never be cancelled and the live-query counter never drains. It fails on master in ~10 seconds and passes with this change. Note that it is a logic race, not a data race - every write happens under `mu`, so the race detector alone does not catch it, the test does. Full server_monitoring suite passes with `-race -tags server_vql`. Full disclosure, same as my last PR (Velocidex/evtx#39): investigated and tested together with an AI assistant (Claude); I reviewed the analysis and the patch myself. This is the race I mentioned on Discord — it is what we were seeing at ThreatLight when spamming monitoring table updates. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Mike Cohen <mike@velocidex.com>
scudette
added a commit
to Velocidex/velociraptor
that referenced
this pull request
Jun 22, 2026
…ntly (#4877) `EventTable._Close()` has to release `mu` while it waits for the old query goroutines to drain, because they call `Tracer()` (which takes `mu`) on their way out. But that opens a window: a concurrent `StartQueries()`, for example `set_server_monitoring` racing the artifact-modification reload loop can enter, start its own queries, and then have its `self.wg` and `self.cancel` overwritten when the first caller re-acquires the lock. The overwritten generation keeps running but is untracked and can never be cancelled. Every race costs one leaked generation; we hit this in production and saw the same SERVER_EVENT artifact collecting 6 times in parallel after a burst of table reloads. The fix keeps the unlock-during-drain (it is required) and instead serializes updaters: a dedicated `update_mu` is held for the whole `StartQueries()`/`Close()` operation, so no second updater can enter the drain window, while `mu` stays free for the query goroutines' callbacks. Also fixes `Wait()` reading `self.wg` without the lock. The new test (`TestConcurrentUpdatesDoNotLeakQueries`) runs four concurrent updaters alternating between two artifact sets and then installs an empty table. If any generation was orphaned its queries can never be cancelled and the live-query counter never drains. It fails on master in ~10 seconds and passes with this change. Note that it is a logic race, not a data race - every write happens under `mu`, so the race detector alone does not catch it, the test does. Full server_monitoring suite passes with `-race -tags server_vql`. Full disclosure, same as my last PR (Velocidex/evtx#39): investigated and tested together with an AI assistant (Claude); I reviewed the analysis and the patch myself. This is the race I mentioned on Discord — it is what we were seeing at ThreatLight when spamming monitoring table updates. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Mike Cohen <mike@velocidex.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
sortMRUWithRegexpruns once in the constructor and only covers MUI files found under System32. Message DLLs that live elsewhere (for example Windows Defender'sPlatformdirectory, registered via the WINEVT publisher key) get their MUI files discovered lazily ininclude_muis, appended in directory order, and never sorted. With 22 Defender languages on disk, the first-wins merge then resolves every Defender message in cs-CZ (and before the first-wins guard was added, the last writer won, which was zh-TW). The system locale doesn't matter, andLangPreferenceRegeExphas no effect on these providers.This change:
lang_filter_re, compiled insortMRUWithRegexp),include_muis, so lazily discovered lists get the same treatment as the constructor pass,sortListWithPreference, shared by both paths.One small behaviour note: the old prepend loop reversed the relative order of multiple matching entries; the new helper keeps a stable partition (matching files first, original order preserved within each group).
Tests:
TestSortListWithPreferencecovers the helper,TestExpandLocationsPrefersLanguageForLazyMUIDirsreproduces the Defender layout (DLL outside System32 with cs-CZ / en-US / zh-TW MUI subdirs) and asserts en-US is expanded first. Both are windows-only and run in the existing windows.yaml workflow.GOOS=windows go build / test -care clean. Two pre-existing failures, not from this change:go vetreports unreachable code in evtx.go:1021, and the darwinTestCollectorneeds a built./dumpevtx.Verified two ways against the real Defender MUI set (22 languages, platform 4.18.26030.3011-0): by replaying the resolver merge, and by running a
dumpevtxbuild with this patch on the affected en-US machine against its live Defender operational log — 1225 events, all 631 Defender messages resolve to en-US, where the unpatched agent renders zh-TW for the same records.Reported in Velocidex/velociraptor#4876.
Full disclosure: the investigation and the testing were done together with an AI assistant (Claude); I reviewed the analysis and the patch myself. We have been running Velociraptor at ThreatLight for a long time and this came out of a real deployment issue (Defender alerts arriving in Traditional Chinese).