Repository navigation
Bound buffered SDK output capture across language surfaces - #1627
Safet Hrbinic (safmswork) wants to merge 1 commit into
Conversation
Retain at most 1 MiB per output stream while continuing to drain excess output. Report truncation through existing warnings and add regression coverage and SDK documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a2e7e097-c92f-44be-9d0f-1d6985c965cc
There was a problem hiding this comment.
🟡 Changes recommended
The supported V1 buffered execution APIs now discard output beyond 1 MiB per stream, breaking existing Rust, .NET, and Node callers that require complete responses even when workloads exit successfully.
1 open issue
What changed in this PR
This PR bounds buffered output capture across MXC’s Rust, Node, and .NET SDKs while continuing to drain excess output so workloads can finish.
Changes:
- Retain the first 1 MiB per stream and report truncation through existing warnings.
- Add regression coverage for capture limits, draining, read errors, and workload outcomes.
- Document the limits and add C ABI tests to the macOS build job.
| File | Description |
|---|---|
src/mxc-sdk/tests/streaming.rs |
Adds Seatbelt bounded-capture coverage. |
src/mxc-sdk/tests/streaming_bubblewrap.rs |
Adds Bubblewrap bounded-capture coverage. |
src/mxc-sdk/src/sandbox.rs |
Bounds Rust capture and adds warnings and unit tests. |
src/mxc-sdk/README.md |
Documents Rust capture limits and streaming alternatives. |
src/ffi/mxc_ffi/tests/ffi.rs |
Tests bounded capture and NUL conversion through the C ABI. |
src/ffi/mxc_ffi/src/lib.rs |
Documents C ABI capture limits. |
sdk/node/tests/unit/state-aware.test.ts |
Tests capture boundaries, draining, and read errors. |
sdk/node/tests/integration/isolation-session-state-aware.test.ts |
Adds lifecycle capture-limit integration coverage. |
sdk/node/src/v1/types.ts |
Documents captured output limits on result fields. |
sdk/node/src/v1/lifecycle.ts |
Bounds lifecycle capture and reports truncation. |
sdk/node/README.md |
Explains Node capture limits and streaming alternatives. |
sdk/dotnet/README.md |
Explains .NET capture limits and warning locations. |
sdk/dotnet/Microsoft.Mxc.Sdk/V1/MxcProcess.cs |
Bounds managed capture and exposes truncation warnings. |
sdk/dotnet/Microsoft.Mxc.Sdk/V1/ExecutionResult.cs |
Updates output and warning documentation. |
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/V1/MxcLifecycleE2ETests.cs |
Tests lifecycle capture limits and warnings. |
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/V1/MxcContainerIsolationSessionE2ETests.cs |
Tests native and managed buffered capture. |
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcProcessTests.cs |
Tests boundaries, draining, cancellation, and read failures. |
docs/development/build-and-test/ci-validation-infrastructure.md |
Describes capture-limit validation coverage. |
docs/api-reference/rust/v1/types.md |
Documents Rust result retention limits. |
docs/api-reference/node/v1/types.md |
Documents Node buffered output limits. |
docs/api-reference/dotnet/v1/types.md |
Documents .NET result and process capture limits. |
.github/workflows/Build.MacOS.Job.yml |
Adds the C ABI test suite to macOS validation. |
Review effort: Balanced
|
Copilot resolve the merge conflicts in this pull request |
There was a problem hiding this comment.
🟡 Changes recommended
The output-limit documentation still incorrectly promises an unconditional 1 MiB cap, omitting MicroVM’s separate 64 MiB combined stdout/stderr limit, overflow errors, and lack of streaming support.
1 open issue
1 resolved since last review
Review effort: Balanced
| /// Retains at most 1 MiB per stream before text conversion, drains excess | ||
| /// output, and reports truncation in `warnings_json_utf8`. |
1b3bc44 to
f2e7eaa
Compare
| let retained = read.min(MAX_CAPTURED_OUTPUT_BYTES - buf.len()); | ||
| buf.extend_from_slice(&chunk[..retained]); | ||
| truncated |= retained < read; |
|
I understand the concern about unbounded output consuming the caller’s memory. However, the demonstrated risk is memory exhaustion, not buffer corruption or a sandbox escape. That deserves consideration, but it does not by itself justify breaking the existing V1 API contract. These APIs previously returned complete output. Imposing a fixed 1 MiB limit means legitimate workloads can now complete successfully while returning incomplete results—for example, a truncated JSON document. A warning does not prevent existing callers from breaking, and the rationale for choosing 1 MiB has not been established. I would prefer to preserve V1 behavior and introduce bounded capture as an explicit choice, using a new versioned API where necessary. An unconditional V1 change needs a stronger explanation of the threat, why existing safeguards are insufficient, and why the compatibility cost is justified. Separately, the Node implementation allocates 2 MiB per execution even when no output is produced. Storage should grow with actual output, up to the chosen limit, rather than imposing that overhead on every caller. |
📖 Description
Problem: we have APIs that will launch an executable in a container, collect all the output the executable makes to STDOUT, then return that output once the executable is done. Some examples of these APIs:
Rust SDK: run , run_in_container , wait_with_output
Node SDK: run , runInContainer
C SDK: mxc_run_json
The problem is that there is currently no limits to the STDOUT output gathered. This has the potential to overrun the buffers and memory of the process who called this API to launch the container. This is a potential security risk.
The solution: retain at most 1 MiB per output stream while continuing to drain excess output. It's important to continue to drain the output even though we aren't giving it back to the API caller, so that STDOUT doesn't get full and cause the process in the container to wait excessively or even error out. If this output limit get's hit, we will report truncation through existing warnings.
This change also adds regression coverage and SDK documentation.
NOTE: A decision needs to be made here, whether to make the fix in the V1 of the APIs, or make a V2 and do the change there. Personally I'm leaning towards doing it in the V1 even though this could be considered a breaking change, but I think the security aspect overrides that. But I definitely would like to solicit other opinions here.
🔗 References
Bug 1628
🔍 Validation
✅ Checklist
feature-acceptedCargo.lock, thedependency-feed-checkcheck passes (see pull request builds)📋 Issue Type
Microsoft Reviewers: Open in CodeFlow