Fix race condition in the ListenWithoutBindIsTracked test case - #41352
Blue (OneBlue) wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the VerifyListenWithoutBindIsTracked test logic in the WSL Windows network test suite to eliminate a race between host-side bind() retries and the guest-side implicit port binding/tracking, making the test less flaky while still validating that the listen-without-bind scenario becomes reachable from the host.
Changes:
- Keep the guest-side listening socket alive across multiple host connection attempts by accepting connections in a loop.
- Replace the host-side “retry bind until it gets blocked” probe with a “retry connect until it succeeds” probe to avoid competing for the same port.
- Explicitly target loopback (
INADDR_LOOPBACK) for the host connection probe.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The new code introduces concrete correctness issues (Winsock error retrieval and varargs format/type mismatches) that should be fixed to avoid undefined behavior and misleading failures in tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
test/windows/NetworkTests.cpp:2290
- LogInfo uses printf-style formatting; assignedPort and ntohs(...) should be cast to unsigned int to match %u and avoid varargs type-mismatch UB.
LogInfo(
"connect() to port %u failed with %d; local source port is %u",
assignedPort,
connectError,
ntohs(localAddr.sin_port));
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
| wil::unique_socket sock(socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)); | ||
| THROW_LAST_ERROR_IF(!sock); | ||
|
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate findings remain unresolved.
Review details
Suppressed comments (2)
test/windows/NetworkTests.cpp:2311
- Both
%uarguments in this new diagnostic call receive values that undergo integral promotion toint(uint16_tandntohs(...)), while%urequiresunsigned int. This is undefined behavior and can produce misleading retry diagnostics; cast both arguments explicitly, matching the cast in the precedingLogInfocall.
"connect() to port %u failed with %d; local source port is %u",
assignedPort,
connectError,
ntohs(localAddr.sin_port));
test/windows/NetworkTests.cpp:2292
connect()succeeding only proves that some listener accepted127.0.0.1:assignedPort; it does not prove the connection reached this guest. An unrelated host listener on the randomly selected port, or the localhost relay accepting before its guest-side connection is established, can make this retry pass even when the GNS mapping is missing or broken. Have the guest send a unique marker afteraccept()and require the host to receive it (with a timeout) before treating the attempt as successful.
if (connect(sock.get(), reinterpret_cast<SOCKADDR*>(&addr), sizeof(addr)) == SOCKET_ERROR)
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The retry can succeed against an unrelated Windows service without confirming the guest listener accepted the connection.
Review details
Suppressed comments (1)
test/windows/NetworkTests.cpp:2292
- A successful connect here does not prove that the connection reached the guest listener: if another Windows service happens to own this ephemeral port, the retry will pass even though GNS never installed the mapping. This test should send a sentinel and have the Perl listener acknowledge it (or otherwise verify data/acceptance in the guest) before treating the retry as success.
if (connect(sock.get(), reinterpret_cast<SOCKADDR*>(&addr), sizeof(addr)) == SOCKET_ERROR)
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary of the Pull Request
This change solves a race condition in the tests: We're waiting for GNS to perform a port binding, but we're also calling bind() on the same port on the host. If the GNS bind comes right while we hold the bound socket, it will fail and the test will fail.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed