Fix batch of bugs found in deep code review by benhillis · Pull Request #40224 · microsoft/WSL

benhillis · 2026-04-17T14:41:08Z

Summary of the Pull Request

This PR fixes 12 confirmed bugs found during a comprehensive deep review of the entire WSL codebase. Issues already covered by PR #40197 are intentionally excluded.

PR Checklist

Closes: N/A (proactive code review)
Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
Tests: Added/updated if needed and all pass
Localization: All end user facing strings can be localized
Dev docs: Added/updated if needed
Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Bug fixes (not covered by #40197)

unittests.c: get_addr_info test entry pointed to GetSetIdTestEntry instead of GetAddrInfoTestEntry (copy-paste error — wrong test was running silently)
SocketChannel.h: %s format specifier in fmt-style LOG_ERROR on Linux; channel name was silently dropped from protocol error logs
p9file.cpp: readlinkat return stored as int instead of ssize_t per POSIX spec
configfile.cpp: ungetwc(WEOF) is undefined on some implementations; added guard
init.cpp: SIGCHLD race window — signal could be lost between signal(SIG_DFL) and sigprocmask(SIG_BLOCK); fixed by blocking first
util.cpp: Extracted duplicated signal skip list into SkipSignal() helper

Test improvements

NetworkTests.cpp: Added SO_RCVTIMEO timeout on accept() to prevent indefinite test hangs (resolves inline TODO)
DrvFsTests.cpp: Added LOG_IF_WIN32_BOOL_FALSE to cleanup operations to surface silent deletion failures

Script hardening

copy_and_build_tests.ps1: Replaced Invoke-Expression with call operator & to prevent command injection
test-setup.ps1: Route PostInstallCommand through bash -c for proper argument handling
deploy-to-vm.ps1: Prompt for password via Read-Host -AsSecureString when not provided

Resource management

WslConfigService.cs: Dispose FileSystemWatcher in destructor to prevent resource leak

Validation Steps Performed

FormatSource.ps1 passed on all C/C++ changes
Full Windows build verified (wslservice.exe, common.lib, configfile.lib all build clean)
Pre-existing build errors in wsltests (lambda capture in UnitTests.cpp) and wslserviceproxystub are unrelated

Bugs fixed (not covered by PR #40197): - unittests.c: Fix get_addr_info test entry pointing to wrong handler (GetSetIdTestEntry -> GetAddrInfoTestEntry) - SocketChannel.h: Fix %s format specifier in fmt-style LOG_ERROR on Linux; channel name was silently dropped from protocol error logs - p9file.cpp: Fix readlinkat return type (int -> ssize_t) to match POSIX specification - configfile.cpp: Guard ungetwc() call against WEOF to avoid undefined behavior on some implementations - init.cpp: Fix SIGCHLD race by blocking the signal before setting the handler, preventing a window where child exit could be lost - util.cpp: Extract duplicated signal skip list into SkipSignal() helper to ensure consistency between save and set handlers Test improvements: - NetworkTests.cpp: Add SO_RCVTIMEO timeout on accept() to prevent indefinite test hangs (resolves TODO) - DrvFsTests.cpp: Add LOG_IF_WIN32_BOOL_FALSE to cleanup operations to surface silent file/directory deletion failures Script hardening: - copy_and_build_tests.ps1: Replace Invoke-Expression with call operator to prevent command injection via interpolated variables - test-setup.ps1: Pass PostInstallCommand through bash -c for proper shell argument handling - deploy-to-vm.ps1: Prompt for password via Read-Host -AsSecureString when not provided, instead of creating empty SecureString Resource management: - WslConfigService.cs: Dispose FileSystemWatcher in destructor to prevent resource leak Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Full deep review covering 7 subsystems with 44 findings. 12 fixes on this branch, 10+ already in PR #40197, 4 deferred. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR applies a batch of targeted bug fixes and hardening changes across WSL’s Linux init/plan9 layers, shared infrastructure, test suites, and supporting PowerShell tooling.

Changes:

Fix correctness issues in Linux/shared code paths (protocol logging, config parsing edge case, POSIX return type, SIGCHLD race, duplicated signal skip logic).
Improve test reliability and debuggability (fix wrong Linux unit test entry, add host-side timeout attempt for a Windows networking test, surface cleanup failures in DrvFs tests).
Harden developer scripts and resource management (remove PowerShell Invoke-Expression, improve VM deploy password prompting, attempt to close FileSystemWatcher resources).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tools/test/test-setup.ps1	Run post-install commands via `bash -c` under `wsl.exe --exec` for better command handling.
tools/test/copy_and_build_tests.ps1	Replace `Invoke-Expression` with direct script invocation for safer execution.
tools/deploy/deploy-to-vm.ps1	Prompt for a password securely when none is provided.
test/windows/NetworkTests.cpp	Add a timeout-setting attempt intended to avoid indefinite `accept()` hangs.
test/windows/DrvFsTests.cpp	Log cleanup failures for reparse test artifacts to aid diagnosis.
test/linux/unit_tests/unittests.c	Fix `get_addr_info` test dispatch to the correct handler.
src/windows/wslsettings/Services/WslConfigService.cs	Dispose the `FileSystemWatcher` in the finalizer.
src/shared/inc/SocketChannel.h	Fix Linux fmt-style logging placeholder so the channel name is included.
src/shared/configfile/configfile.cpp	Guard against `ungetwc(WEOF)` undefined behavior.
src/linux/plan9/p9file.cpp	Use `ssize_t` for `readlinkat` result per POSIX.
src/linux/init/util.cpp	Deduplicate the signal skip list into a helper.
src/linux/init/init.cpp	Block SIGCHLD before resetting handler to close a race window.
code-review.md	Add a deep-review summary and tracking document for identified issues/fixes.

Copilot · 2026-04-17T14:46:23Z

+// Signals that cannot or should not be overridden by save/set handlers.
+static bool SkipSignal(unsigned int Signal)
+{
+    switch (Signal)
+    {
+    case SIGKILL:
+    case SIGSTOP:
+    case SIGCONT:
+    case SIGHUP:
+    case 32:
+    case 33:
+    case 34:
+        return true;


SkipSignal() hard-codes signal numbers 32/33/34, which are ABI-/libc-dependent (e.g., realtime signal ranges vary). Please replace these magic numbers with named constants/macros (such as SIGRTMIN/SIGRTMAX-derived values) or add a clear rationale tied to the specific environment assumptions.

Copilot · 2026-04-17T14:46:23Z

    ~WslConfigService()
    {
+        _wslConfigFileSystemWatcher?.Dispose();
        WslCoreConfigInterface.FreeWslConfig(_wslConfig);
        WslCoreConfigInterface.FreeWslConfig(_wslConfigDefaults);


Disposing the FileSystemWatcher only in the finalizer is non-deterministic and likely won’t run during normal app lifetime (this service is registered as a singleton). If the intent is to avoid handle/resource leaks, implement IDisposable and dispose the watcher (and native configs) when the app/host shuts down.

Copilot · 2026-04-17T14:46:23Z

+A comprehensive deep-dive review of the entire WSL codebase identified **44 issues** across
+7 major subsystems. **13 confirmed bugs** have been fixed on the `copilot/code-review-fixes`
+branch (plus 8 more are already addressed by open PR #40197). The remaining findings are
+documented below for future triage.


The executive summary says "13 confirmed bugs" fixed on this branch, but the PR description states 12. Please reconcile the counts (and/or clarify what the extra item is) so the documentation matches the PR scope.

Copilot · 2026-04-17T14:46:24Z

 $copyScriptCommand = $PSScriptRoot + "\copy_tests.ps1 -WslTestDirPath $WslTestDirPath -DistroName $DistroName"

 $cleanTestCommand = "rm -rf /data/test"
 $buildTestCommand = "cd /data/test; ./build_tests.sh; less /data/test/log/build_output"

 # clean test directory on linux side
 Write-Output "Cleaning unit tests at $DistroPath\rootfs\data\test"
 Run { wsl.exe --distribution $DistroName --user root --exec bash -c "$cleanTestCommand" }

 # call the logic in copy_tests.ps1
-Invoke-Expression $copyScriptCommand
+& (Join-Path $PSScriptRoot "copy_tests.ps1") -WslTestDirPath $WslTestDirPath -DistroName $DistroName


The new direct invocation of copy_tests.ps1 makes the earlier $copyScriptCommand construction unused. Consider removing the unused variable (and related string-building) to avoid confusion and keep StrictMode scripts easy to maintain.

Copilot · 2026-04-17T14:46:24Z

+            // Set a timeout on the listen socket to avoid an indefinite wait if the client never connects.
+            DWORD timeout = 10000;
+            VERIFY_ARE_NOT_EQUAL(setsockopt(listenSocket.get(), SOL_SOCKET, SO_RCVTIMEO, (char*)&timeout, sizeof(timeout)), SOCKET_ERROR);


Setting SO_RCVTIMEO does not apply to accept() on Winsock; accept can still block indefinitely here. To enforce a timeout, consider using select()/WSAPoll on the listening socket, or make it non-blocking and poll for FD_ACCEPT/readiness before calling accept.

Suggested change

// Set a timeout on the listen socket to avoid an indefinite wait if the client never connects.

DWORD timeout = 10000;

VERIFY_ARE_NOT_EQUAL(setsockopt(listenSocket.get(), SOL_SOCKET, SO_RCVTIMEO, (char*)&timeout, sizeof(timeout)), SOCKET_ERROR);

// Wait for the listening socket to become ready before calling accept(), since

// SO_RCVTIMEO does not apply to accept() on Winsock.

fd_set readSet;

FD_ZERO(&readSet);

FD_SET(listenSocket.get(), &readSet);

timeval timeout{};

timeout.tv_sec = 10;

timeout.tv_usec = 0;

const int selectResult = select(0, &readSet, nullptr, nullptr, &timeout);

VERIFY_ARE_EQUAL(selectResult, 1);

VERIFY_IS_TRUE(FD_ISSET(listenSocket.get(), &readSet));

Ben Hillis and others added 2 commits April 17, 2026 08:37

docs: add comprehensive code review report

9699624

Full deep review covering 7 subsystems with 44 findings. 12 fixes on this branch, 10+ already in PR #40197, 4 deferred. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 17, 2026 14:41

benhillis requested a review from a team as a code owner April 17, 2026 14:41

Copilot started reviewing on behalf of benhillis April 17, 2026 14:41 View session

benhillis closed this Apr 17, 2026

benhillis deleted the copilot/code-review-fixes branch April 17, 2026 14:43

Copilot AI reviewed Apr 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix batch of bugs found in deep code review#40224

Fix batch of bugs found in deep code review#40224
benhillis wants to merge 2 commits intomasterfrom
copilot/code-review-fixes

benhillis commented Apr 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-            // Set a timeout on the listen socket to avoid an indefinite wait if the client never connects.
-            DWORD timeout = 10000;
-            VERIFY_ARE_NOT_EQUAL(setsockopt(listenSocket.get(), SOL_SOCKET, SO_RCVTIMEO, (char*)&timeout, sizeof(timeout)), SOCKET_ERROR);
+            // Wait for the listening socket to become ready before calling accept(), since
+            // SO_RCVTIMEO does not apply to accept() on Winsock.
+            fd_set readSet;
+            FD_ZERO(&readSet);
+            FD_SET(listenSocket.get(), &readSet);
+            timeval timeout{};
+            timeout.tv_sec = 10;
+            timeout.tv_usec = 0;
+            const int selectResult = select(0, &readSet, nullptr, nullptr, &timeout);
+            VERIFY_ARE_EQUAL(selectResult, 1);
+            VERIFY_IS_TRUE(FD_ISSET(listenSocket.get(), &readSet));

Conversation

benhillis commented Apr 17, 2026

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Bug fixes (not covered by #40197)

Test improvements

Script hardening

Resource management

Validation Steps Performed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants