Repository navigation
Fix service crash when collecting a linux crash dump when maxCrashDumpCount is set to 0 - #13755
Merged
Merged
Conversation
…pCount is set to 0
Blue (OneBlue)
requested review from
Ben Hillis (benhillis),
Copilot,
Craig Loewen (craigloewen-msft) and
justus-camp-microsoft
November 20, 2025 21:30
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a service crash that occurs when collecting Linux crash dumps with wsl2.maxCrashDumpCount set to 0 and an empty crash dump folder. The root cause is that EnforceFileLimit attempts to access an empty map when called with Limit = 0, as the condition 0 < 0 fails to return early. The fix adds guard conditions at both call sites to skip EnforceFileLimit when the limit is 0 or negative, and adds defensive assertions within the function itself.
- Guard conditions added before
EnforceFileLimitcalls to prevent invocation whenMaxCrashDumpCount <= 0 - Added debug assertion in
EnforceFileLimitto document the precondition thatLimit > 0 - Removed dead code checking if unsigned
size_t Limit < 0
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/windows/service/exe/WslCoreVm.cpp | Added guard conditions in CollectCrashDumps and OnCrash to prevent calling EnforceFileLimit when MaxCrashDumpCount is 0 or negative |
| src/windows/common/wslutil.cpp | Added assertion requiring Limit > 0 and removed impossible Limit < 0 check from unsigned comparison |
| wsl::windows::common::wslutil::EnforceFileLimit(m_vmConfig.CrashDumpFolder.c_str(), m_vmConfig.MaxCrashDumpCount, pred); | ||
| if (m_vmConfig.MaxCrashDumpCount > 0) | ||
| { | ||
| wsl::windows::common::wslutil::EnforceFileLimit(m_vmConfig.CrashDumpFolder.c_str(), m_vmConfig.MaxCrashDumpCount, pred); |
There was a problem hiding this comment.
Can we push the 0 limit behavior into the EnforceFileLimit helper to just do nothing?
|
|
||
| void wsl::windows::common::wslutil::EnforceFileLimit(LPCWSTR Path, size_t Limit, const std::function<bool(const std::filesystem::directory_entry&)>& pred) | ||
| { | ||
| WI_ASSERT(Limit > 0); |
There was a problem hiding this comment.
In other words, change this assert to if (limit <= 0) return;
Ben Hillis (benhillis)
approved these changes
Nov 21, 2025
Ben Hillis (benhillis)
added a commit
that referenced
this pull request
Nov 26, 2025
* wslsettings: allow OOBE window to close with escape key (#13686) * wslsettings: allow OOBE windows to close with escape key * fix memory leak --------- Co-authored-by: Ben Hillis <[email protected]> * Localization change from build: 133610492 (#13704) Co-authored-by: WSL localization <[email protected]> * wslsettings: fix OOBE text truncation at 200% text scaling (#13693) * wslsettings: fix OOBE text truncation at 200% text scaling Add text scaling factor to window resize calculation and make hero image height responsive to text scaling. Increase minimum window size for better accessibility. Fix MAS 1.4.4 compliance for OOBE dialog. * pr feedback --------- Co-authored-by: Ben Hillis <[email protected]> * wslsettings: add underlines to links in about page (#13703) Co-authored-by: Ben Hillis <[email protected]> * .clang-format: add InsertBraces: true and minor fix to FormatSource.ps1 (#13712) Co-authored-by: Ben Hillis <[email protected]> * Localization change from build: 134015316 (#13731) Co-authored-by: WSL localization <[email protected]> * wslsettings: ensure selected setting is auto-expanded and selected (#13689) * wslsettings: ensure selected setting is auto-selected Implement keyboard focus management for SettingsExpander controls across settings pages. This resolves an accessibility issue reported internally. * add asserts --------- Co-authored-by: Ben Hillis <[email protected]> * build: fix minor compiler errors when building with VS2026 (#13744) * build: fix minor compiler errors when building with VS2026 * s * use VS2022 for clang format and cross compiling --------- Co-authored-by: Ben Hillis <[email protected]> * chore(distributions): Almalinux auto-update - 20251119 12:04:35 (#13743) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Mask systemd-networkd-wait-online.service during boot (#13611) * deps: update a number of NuGet packages to the latest available versions (#13728) Co-authored-by: Ben Hillis <[email protected]> * Notice change from build: 134267142 (#13751) Co-authored-by: WSL notice <[email protected]> * Add *.slnx to .gitignore (#13754) * Fix service crash when collecting a linux crash dump when maxCrashDumpCount is set to 0 (#13755) * Fix service crash when collecting a linux crash dump when maxCrashDumpCount is set to 0 * Move the check inside the function * cleanup: VirtioNetworking refactoring (#13760) * cleanup: update VirtioNetworking class to not rely on the WslCoreConfig struct * cleanup: simplify VirtioNetworking construction * remove old constructor and other cleanup * more minor cleanup * string cleanup in HandleVirtioModifyOpenPorts --------- Co-authored-by: Ben Hillis <[email protected]> * cleanup: switch from Microsoft::WRL::ComPtr to wil::com_ptr (#13767) * cleanup: switch from Microsoft::WRL::ComPtr to wil::com_ptr * reformat --------- Co-authored-by: Ben Hillis <[email protected]> * chore(distributions): Almalinux auto-update - 20251124 17:13:02 (#13780) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Notice change from build: 134527933 (#13782) Co-authored-by: WSL notice <[email protected]> * cleanup: VirtioNetworking refactoring to be more portable (#13783) * cleanup: VirtioNetworking refactoring to be more portable * more refactoring * make m_guestDeviceManager private --------- Co-authored-by: Ben Hillis <[email protected]> --------- Co-authored-by: Ben Hillis <[email protected]> Co-authored-by: Blue <[email protected]> Co-authored-by: WSL localization <[email protected]> Co-authored-by: AlmaLinux Autobot <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.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
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.
Summary of the Pull Request
This change fixes a potential service crash that can hit when a linux crash dump is collected when
wsl2.maxCrashDumpCountis set to zero and the crash folder doesn't contain any crash files.PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed