Fix various protocol parsing issues

[Blue (OneBlue)]

Summary of the Pull Request

This change solves 3 parsing bugs:

1. Reject 'clientId' strings whose length extend passed the end of the message in Plan9's TGetLock / RGetLock handler

2. Reject truncated Plan9 TRead requests

3. Switch string arrays to using the ArrayFromSpan format instead of null separated strings. The previous implementation could read passed the message length in multiple places

This change also add a bit of logic in the plan9 entrypoint to run it ad-hoc, which allows to run unit tests on the plan9 server

PR Checklist

• Closes: Link to issue #xxx
• 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

Validation Steps Performed

[Copilot]

🟡 Changes recommended

Address the truncated TRead logging failure and correct the ineffective Rgetlock regression test.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens Plan 9 protocol parsing and migrates process string arrays to bounded, length-prefixed serialization.

Changes:

• Adds validation for truncated and oversized Plan 9 requests.
• Adds TCP Plan 9 test infrastructure and regression tests.
• Updates process-message serialization and parsing.

File summaries

File	Summary
test/windows/Plan9ProtocolTests.cpp	Adds protocol regression tests; one test does not exercise Rgetlock parsing, with minor wording nits.
test/windows/CMakeLists.txt	Registers the new tests.
src/windows/service/exe/WslCoreInstance.cpp	Uses the updated process-message layout.
src/windows/service/exe/LxssInstance.cpp	Updates process-message field access.
src/windows/service/exe/LxssCreateProcess.h	Implements length-prefixed serialization.
src/windows/service/exe/LxssCreateProcess.cpp	Removes obsolete serialization logic.
src/shared/inc/lxinitshared.h	Updates process-message structures.
src/linux/plan9/p9log.h	Bounds lock-string logging.
src/linux/plan9/p9handler.cpp	Validates message sizes and lock strings; logging can still terminate on truncated TRead requests.
src/linux/init/util.cpp	Parses process argument arrays.
src/linux/init/plan9.h	Extends Plan 9 server parameters.
src/linux/init/plan9.cpp	Adds TCP server binding.
src/linux/init/init.cpp	Parses revised process messages and bind options.
src/linux/init/config.h	Updates environment-block APIs.
src/linux/init/config.cpp	Parses environment arrays safely.

Review details

Suppressed comments (3)

test/windows/Plan9ProtocolTests.cpp:271

• This packet cannot exercise Rgetlock parsing: HandleMessage has no Rgetlock case and returns ENOTSUP regardless of the payload, so the assertion would pass even if the malformed client-id handling were broken. Test the actual Tgetlock path, or explicitly enable verbose logging and assert that this malformed response-shaped packet does not terminate the server.

        SendMessageAndExpectResponse(
            server.client.get(), MakePlan9Message(Plan9MessageType::Rgetlock, std::move(payload)), Plan9MessageType::Rlerror);

test/windows/Plan9ProtocolTests.cpp:210

• The comment says “return returns,” which is grammatically incorrect; please remove the duplicate verb.

    // Send a truncated tread and validate that the server return returns EINVAL.

test/windows/Plan9ProtocolTests.cpp:224

• “Passed” is the wrong word here; the client ID extends past the end of the message.

    // Validate that the server rejects client ids that extend passed the end of the message.

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Verbose Plan 9 logging can fail-fast on truncated TRead requests before validation returns an error response.

Review details

Suppressed comments (1)

src/linux/plan9/p9handler.cpp:1257

• The new minimum-size check runs after LogMessage(). With verbose Plan9 logging enabled, TraceLogMessage parses Tread using fail-fast U64()/U32() calls, so this truncated request can terminate the server before it reaches the Rlerror path that the check is intended to provide. Move logging after this validation (or make every logging field read fallible) so malformed messages are rejected consistently regardless of log level.

        const auto messageSize = reader.U32(); // message size, already validated
        auto messageType = reader.U8();
        const auto messageTag = reader.U16();
        const SpanWriter errorWriter{response.Writer};

        LX_INT error{};

        if (const auto minimumMessageSize = GetMessageSize(static_cast<MessageType>(messageType));
            minimumMessageSize != 0 && messageSize < minimumMessageSize)

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

🔵 Needs a closer look

The tests use a fixed port and do not actually cover the truncated client-ID parsing path.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

test/windows/Plan9ProtocolTests.cpp:29

• Using a fixed port makes these integration tests nondeterministically fail whenever another process or parallel test already owns 1234. Bind an ephemeral/dynamically selected port and report the actual port to the client test instead of hard-coding one.
  test/windows/Plan9ProtocolTests.cpp:278
• This appends all 4096 bytes declared by the length field, so TryString() succeeds and the test only exercises the separate > 40 limit. It does not cover the claimed truncated-clientId case where the declared length extends past the message; add a request whose length exceeds the remaining payload while retaining a separate max-length test.

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Three moderate issues remain unresolved: socket-path validation, failure-safe malformed-message logging, and hard-coded test-port allocation.

Review details

Suppressed comments (3)

src/linux/init/init.cpp:3386

• --socket-path is accepted together with --bind, even though that path is only meaningful for the supplied server-fd mode. RunPlan9Server still retains the value and may unlink it during shutdown, so reject this combination (or clear the path) instead of silently accepting an invalid and potentially destructive configuration.

    if (BindPort.has_value() == static_cast<bool>(ServerFd) ||
        (BindPort.has_value() && (*BindPort <= 0 || *BindPort > std::numeric_limits<uint16_t>::max())))

src/linux/plan9/p9handler.cpp:1262

• This logging call still runs outside the try block, and TraceLogMessage parses many variable-length fields with SpanReader::String(), whose bounds failure is FAIL_FAST() (for example a truncated Tversion). Thus a malformed message with a truncated string can terminate the Plan 9 server before it returns Rlerror, even though the handler is intended to reject malformed requests. Move logging inside the exception boundary or make every logger parse failure-safe before logging untrusted messages.

            LogMessage(reader.Span());

test/windows/Plan9ProtocolTests.cpp:29

• This test always binds 127.0.0.1:1234, so it will fail whenever another process or concurrently running test owns that port. Please allocate a test-specific free port (and pass/observe that value) instead of relying on a hard-coded port.

    constexpr uint16_t c_plan9Port = 1234;

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved moderate findings affect request logging and test reliability, and lock-handler coverage is incomplete.

Review details

Suppressed comments (4)

src/linux/plan9/p9handler.cpp:750

• The new regression test only exercises HandleGetLock, while this change also adds the same truncated-string path to HandleLock. Add a malformed Tlock request assertion here so the second handler is protected from regressing independently.

        const auto clientId = reader.TryString();
        if (!clientId.Success)
        {
            return LX_EINVAL;
        }

src/linux/plan9/p9handler.cpp:1264

• LogMessage still parses every variable-length field with SpanReader::String(), whose bounds failure calls FAIL_FAST() rather than throwing. A malformed request such as a truncated Tversion can satisfy the new fixed minimum-size check, reach this call, and abort the Plan 9 server before it can return Rlerror; make the logging parser bounds-safe (or otherwise validate dynamic fields before logging).

                LogMessage(reader.Span());

test/windows/Plan9ProtocolTests.cpp:29

• This test always binds the distro's loopback port 1234, so it fails whenever another process or concurrently running test already owns that port. Please allocate an available port (for example, bind ephemerally and report the actual port) or otherwise reserve a collision-free endpoint for the test.

    constexpr uint16_t c_plan9Port = 1234;

test/windows/Plan9ProtocolTests.cpp:138

• The bound port marker is printed immediately after the guest bind, but the WSL2 loopback relay can take time to expose that port to the host. Existing host-to-guest tests retry connect for this race (for example, test/windows/NetworkTests.cpp:1864-1881); making one connection attempt here can produce intermittent WSAECONNREFUSED failures in CI. Retry the connection with a bounded timeout after the marker.

        THROW_LAST_ERROR_IF(connect(server.client.get(), reinterpret_cast<SOCKADDR*>(&address), sizeof(address)) == SOCKET_ERROR);

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Moderate issues remain around process-array coverage and collision-free test-port allocation.

Review details

Suppressed comments (4)

src/linux/init/config.cpp:1595

• The function signature no longer accepts Common, but the routine header below still documents Common as its argument and omits the new buffer and offset parameters. Please update the parameter documentation so callers can understand which message regions each offset refers to.

EnvironmentBlock ConfigCreateEnvironmentBlock(
    gsl::span<gsl::byte> Buffer,
    unsigned int EnvironmentOffset,
    unsigned int NtEnvironmentOffset,
    unsigned int NtPathOffset,
    const wsl::linux::WslDistributionConfig& Config)

src/linux/init/init.cpp:927

• This templated parser now receives a Message pointer, but its routine header still documents only Parsed, Buffer, and Config. That leaves the new source of the offsets and fixed fields undocumented; please add the Message parameter and update the description of Buffer accordingly.

template <typename TMessage>
int CreateProcessParseCommon(PCREATE_PROCESS_PARSED_COMMON Parsed, gsl::span<gsl::byte> Buffer, const TMessage* Message, const wsl::linux::WslDistributionConfig& Config)

src/windows/service/exe/LxssCreateProcess.h:116

• The new array encoding is used for every normal and utility-VM process launch, but the added tests exercise only Plan 9 messages and do not cover creating/parsing an LX_INIT_CREATE_PROCESS message with command-line and environment entries. A serialization mismatch here would break all process launches while existing end-to-end launch tests may not identify which array is malformed; please add a focused round-trip/integration case covering multiple and empty arrays.

        message.WriteStringArray(message->CommandLineOffset, commandLine.data(), commandLine.size());
        message.WriteStringArray(message->EnvironmentOffset, environment.data(), environment.size());
        message.WriteStringArray(message->NtEnvironmentOffset, ntEnvironment.data(), ntEnvironment.size());

test/windows/Plan9ProtocolTests.cpp:29

• This test always binds the ad-hoc server to guest port 1234. Any process already using that port makes bind() fail, so the test then waits for a startup line that can never be emitted and fails nondeterministically on shared or busy test environments. Please use an ephemeral/allocated port (and report the actual bound port to the test) or otherwise reserve a collision-free port for each test instance.

    constexpr uint16_t c_plan9Port = 1234;

• Files reviewed: 16/16 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The protocol tests use a fixed port, causing failures when that port is already occupied.

Review details

Suppressed comments (1)

test/windows/Plan9ProtocolTests.cpp:29

• This test hard-codes 127.0.0.1:1234, so it fails whenever another process or concurrently running WSL test owns that port. Please allocate an ephemeral port (and report/use the port returned by bind) or otherwise choose a free port so the protocol tests do not depend on the host environment.

    constexpr uint16_t c_plan9Port = 1234;

• Files reviewed: 16/16 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Blue (OneBlue)]

• This test hard-codes 127.0.0.1:1234, so it fails whenever another process or concurrently running WSL test owns that port. Please allocate an ephemeral port (and report/use the port returned by bind) or otherwise choose a free port so the protocol tests do not depend on the host environment.

That's fine, the tests do this in many places

[David Bennett (dkbennett)]

this looks a lot simpler than the former code!