diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 3a1ad851a..e04ed70f3 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -278,7 +278,7 @@ The workspace is organized into six top-level directories under `src/`: - `mxc_engine` is the **single execution engine** — the one home for "given an `ExecutionRequest`, run it". It owns: run-to-completion backend selection (`run` / `resolve_runner`, covering **all** backends, incl. the Windows ProcessContainer BaseContainer/AppContainer BFS/DACL fallback tiers via `appcontainer_common::dispatcher::dispatch_with_fallback`, and every experimental backend, feature-gated); streaming (`spawn` → `Box`); state-aware lifecycle dispatch (`run_state_aware`, including Windows Sandbox and IsolationSession); host probing (`platform_support` / `PlatformSupport`); and config building (`build_request` / `build_request_with_containment`, `SandboxPolicy` + sections, `available_tools_policy`/`user_profile_policy`/`temporary_files_policy`). It depends on the backend crates (cfg-split: appcontainer/windows_sandbox lifecycle/isolation_session/wslc/nanvix on Windows, bubblewrap/lxc/nanvix on Linux, seatbelt on macOS) so it can't live in `wxc_common`. Both the executor binaries and `mxc-sdk` call into it. `ResolvedRunner` carries the boxed runner plus (Windows only) the optional `DaclManager` guard, so `wxc-exec` can park the guard for its signal handler. - `mxc-sdk` is the **public Rust SDK** — a thin facade over `mxc_engine`. Build a `SandboxRequest` with `build_request`, then either `run(request)` (run-to-completion; returns an `Output` with the `WaitOutcome`, captured `stdout`/`stderr`, warnings, and optional structured output metadata) or `spawn_sandbox(request)` (returns a `Sandbox` handle for live bidirectional stdio — `take_stdin`/`take_stdout`/`take_stderr`, `kill()`, `wait()` returning a `WaitOutcome` (`Exited(i32)` / `TimedOut`) as `io::Result`, `output_metadata()` after terminal completion, or `wait_with_output()`). It re-exports the engine's config-building surface (`build_request`, `build_request_with_containment` + `Containment`/`WslcSection`, `mxc_sdk::policy::{SandboxPolicy sections}`, discovery helpers) and `platform_support`; `mod sandbox` (wrapping the engine's `SandboxProcess` in `Sandbox`) is its only local module. No pty is ever allocated. Streaming supports Seatbelt (macOS), Bubblewrap (Linux), Windows ProcessContainer (AppContainer + BaseContainer), and WSLC (Windows, experimental — needs the crate's `wslc` feature plus `SandboxRequest::set_experimental(true)`; no stdin and `id() == 0`, since the WSLC SDK exposes neither); other backends return `ErrorCode::UnsupportedContainment`. - The lower-level execution surface lives in `wxc_common::sandbox_process`: the `SandboxBackend` trait (`validate` + `spawn(request, logger, StdioMode) -> Box` + a `diagnose_exit` hook) and the generic `Runner` adapter that bridges any `SandboxBackend` to the run-to-completion `ScriptRunner` (via `spawn(StdioMode::Inherit)` then `wait()`). `SandboxProcess::output_metadata()` carries backend-produced structured outputs after terminal teardown without writing to process-global stdio. `StdioMode::Pipes` hands the caller live stdin/stdout/stderr (what the `mxc-sdk` streaming path uses); `StdioMode::Inherit` lets the child inherit the host's stdio (what the executor binaries use, preserving the TTY under a pty). `SandboxBackend` is implemented for Seatbelt, Bubblewrap, Windows ProcessContainer, and WSLC (on `wslc_common::WSLContainerRunner` itself, which shares one container lifecycle — `start_container` — between its streaming `SandboxBackend` and run-to-completion `ScriptRunner` impls, differing only in where the WSLC SDK's output callbacks write). -- `mxc_ffi` (`ffi/mxc_ffi`, `crate-type = ["cdylib", "staticlib", "lib"]`) is a flat, panic-safe **C ABI over `mxc-sdk`** for language bindings. `mxc_run(policyJson, command, out)` runs a sandbox to completion, filling a `#[repr(C)] MxcRunResult` (status + exit_code + timed_out + owned stdout/stderr/error/output-metadata C strings); every entry point is `catch_unwind`-wrapped so a panic becomes a status code, never an unwind. Its `build.rs` runs **csbindgen** to generate the C# P/Invoke (`sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeMethods.g.cs`), gated behind the crate's **`dotnetsdk`** feature (off by default, so the whole-workspace backend build matrix doesn't compile csbindgen). The generated file is **not committed** (gitignored); the C# csproj regenerates it at build time and `scripts/check-dotnet-bindings-codegen.js` runs the codegen in CI and asserts the expected entry points are produced. The C ABI is **not a stable external contract** (native + binding are co-versioned and generated together; see the crate docs). It exposes three surfaces: **run-to-completion** (`mxc_run`), **streaming** (`mxc_spawn` → opaque `MxcSandbox` handle; `mxc_stream_read`/`write`/`flush`, `mxc_sandbox_take_stdin`/`stdout`/`stderr`, `mxc_sandbox_id`/`try_wait`/`wait`/`kill`/`output_metadata_json`/`free`, in `src/streaming.rs`), and the **state-aware lifecycle** (`mxc_state_aware` for the envelope phases + `mxc_state_aware_exec` returning a live streaming handle, in `src/state_aware.rs`). All three `.rs` files are csbindgen inputs in `build.rs`; the `MXC_STATUS_*` space already reserves the state-aware phase codes. +- `mxc_ffi` (`ffi/mxc_ffi`, `crate-type = ["cdylib", "staticlib", "lib"]`) is a flat, panic-safe **C ABI over `mxc-sdk`** for language bindings. `mxc_run(policyJson, command, out)` runs a sandbox to completion, filling a `#[repr(C)] MxcRunResult` (status + exit_code + timed_out, owned stdout/stderr/output-metadata C strings, and an `MxcErrorDetail` carrying the failure message plus the failing API call and its platform status); every entry point is `catch_unwind`-wrapped so a panic becomes a status code, never an unwind. Its `build.rs` runs **csbindgen** to generate the C# P/Invoke (`sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeMethods.g.cs`), gated behind the crate's **`dotnetsdk`** feature (off by default, so the whole-workspace backend build matrix doesn't compile csbindgen). The generated file is **not committed** (gitignored); the C# csproj regenerates it at build time and `scripts/check-dotnet-bindings-codegen.js` runs the codegen in CI and asserts the expected entry points are produced. The C ABI is **not a stable external contract** (native + binding are co-versioned and generated together; see the crate docs). It exposes three surfaces: **run-to-completion** (`mxc_run`), **streaming** (`mxc_spawn` → opaque `MxcSandbox` handle; `mxc_stream_read`/`write`/`flush`, `mxc_sandbox_take_stdin`/`stdout`/`stderr`, `mxc_sandbox_id`/`try_wait`/`wait`/`kill`/`output_metadata_json`/`free`, in `src/streaming.rs`), and the **state-aware lifecycle** (`mxc_state_aware` for the envelope phases + `mxc_state_aware_exec` returning a live streaming handle, in `src/state_aware.rs`). All four `.rs` files are csbindgen inputs in `build.rs` (the shared `MxcErrorDetail` lives in `src/error_detail.rs`); the `MXC_STATUS_*` space already reserves the state-aware phase codes. - `mxc_pty` is the shared pty bridge used by the LXC backend (`lxc_common::lxc_bindings::attach_run`) so the inner shell sees a real TTY and host stdio is streamed live. (Seatbelt and Bubblewrap no longer use it: they spawn directly and let the child inherit the host's stdio — a TTY when the executor binary runs under a pty — via `SandboxBackend::spawn(StdioMode::Inherit)`.) - `learning_mode_core` is the **cross-platform learning-mode / captureDenials model + output emitter**: `DeniedResource` (+ `ResourceType`/`AccessType`), `DenialSummary`, the `DenialAnalyzer` decode trait, and `emit` — which writes the on-disk denials deliverable as a **single JSON document** `{ "denials": [...], "summary": {...} }` (`write_document` / `DenialsDocument`) and defines the serializable `DenialsOutputPointer`. It carries no OS-specific code (must not depend on any `backends/*` crate); the Windows ETL decoder implementing `DenialAnalyzer` lives in `backends/learning_mode/windows`. When `processContainer.captureDenials` is set, native PSEC/V2 seals and decodes a managed ETL locally, while guarded WPR relogs its host-wide source into a process-scoped ETL before analysis; both routes write the same canonical JSON through shared output plumbing and return neutral `wxc_common` metadata. Explicit `retainEtl` preserves the native sealed trace or the guarded process-scoped relogged trace after a terminal wait; abandonment discards it. `wxc-exec` serializes the metadata as the one-line stderr pointer at the CLI boundary; Rust/C#/FFI callers receive it programmatically. Each denial's `resource` field holds the file path or the AppContainer capability name; capability denials resolve their capability SID to a friendly name via `backends/learning_mode/windows`'s `capability_names` (well-known `S-1-15-3-…` SID → policy name; custom hashed SIDs fall back to the SID string). - `mxc_build_common` is a build-time helper crate — all Windows binary crates use it in their `build.rs` to embed VersionInfo (ProductName, FileDescription, copyright, version+commit). When adding a new Windows binary crate, add `mxc_build_common` as a build-dependency and call `mxc_build_common::embed_version_info()` from `build.rs` diff --git a/docs/isolation-session/state-aware-rust.md b/docs/isolation-session/state-aware-rust.md index 598c9ac95..3621c6604 100644 --- a/docs/isolation-session/state-aware-rust.md +++ b/docs/isolation-session/state-aware-rust.md @@ -402,8 +402,7 @@ that is the API's own message, passed through verbatim. | MXC-internal failure (relay threads, console handles) | — | — | — | | `Policy` and the MXC-side `malformed_*` rejections | — | — | — | -**Invariant:** `nativeCode` implies `operation`, and `remediation` implies `operation`. -`operation` marks that an API operation was in flight; neither refinement appears alone. +**Invariant:** `operation` marks that an API operation was in flight. `operation` is the interface-qualified member name — for example `IsoSessionOps.StopSessionAsync`. It is deliberately low-cardinality and free of call diff --git a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md index 7310d333f..541ace5ab 100644 --- a/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md +++ b/docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md @@ -816,11 +816,10 @@ logic on `code` first. **Stability.** Unlike `code`, which is a closed and versioned enum, the *values* of `operation` and `nativeCode` are **best-effort diagnostics and may change without a schema version bump**. They are derived from the underlying platform API — for IsolationSession, from the projected WinRT class and method names — which MXC does not own and cannot version. Consumers should aggregate on them for telemetry and log them for diagnosis, but branch program logic on `code`, and should not treat a particular `operation` value as a guarantee. (MXC's own end-to-end tests do pin exact values; that is deliberate — they verify MXC's mapping, and move with it in the same change.) -**Invariant:** `nativeCode` implies `operation`, and `remediation` implies `operation`. -`operation` marks that an API operation was in flight; the other two refine it, and -neither ever appears alone. A failure MXC raises before or outside any API call — a -malformed request or id, a policy rejection, or an internal failure of MXC's own -machinery — carries only `code` and `message`. +**Invariant:** `operation` marks that an API operation was in flight. A failure +MXC raises before or outside any API call — a malformed request or id, a policy +rejection, or an internal failure of MXC's own machinery — carries only `code` +and `message`. **Which fields earn a place here.** A named top-level field is for a **backend-neutral** concept: `operation`, `nativeCode` and `remediation` all apply equally to a Windows diff --git a/scripts/check-dotnet-bindings-codegen.js b/scripts/check-dotnet-bindings-codegen.js index 3e174f786..60706a3cc 100644 --- a/scripts/check-dotnet-bindings-codegen.js +++ b/scripts/check-dotnet-bindings-codegen.js @@ -26,12 +26,18 @@ const generated = join( "NativeMethods.g.cs" ); -// The extern entry points the C# SDK P/Invokes; the generated file must expose -// each one. Keep in sync with the `#[no_mangle] extern "C"` fns in -// src/ffi/mxc_ffi/src/lib.rs. +// A smoke-test subset of the extern entry points the C# SDK P/Invokes — not the +// whole set, which is larger. The compiler is the real backstop: csbindgen emits +// a binding only for a fn carrying `#[no_mangle]` or `#[export_name]`, and takes +// the `EntryPoint` from whichever of the two determines the export. So renaming +// a fn, removing it, or dropping that attribute makes the generated method +// change or disappear, and the hand-written call sites stop compiling. +// +// Keep in sync with the `#[no_mangle] extern "C"` fns in src/ffi/mxc_ffi/src/. const REQUIRED_ENTRY_POINTS = [ "mxc_run", "mxc_run_result_free", + "mxc_error_detail_free", "mxc_string_free", "mxc_version", ]; @@ -72,6 +78,118 @@ if (missing.length > 0) { process.exit(1); } +// The same set of Rust sources is named twice: once for csbindgen to read, and +// once as the MSBuild target's incremental `Inputs`. MSBuild skips the target +// when its output is newer than every declared input, so a file missing from +// the second list means an incremental C# build can compile against stale +// declarations — and cargo's own `rerun-if-changed` never gets consulted, +// because the target never runs. A clean CI build always regenerates and so +// cannot catch it. +// +// This is not hypothetical: adding `error_detail.rs` to build.rs without adding +// it to the csproj is exactly how the lists drifted once already. +const CRATE_REL_ROOT = "ffi/mxc_ffi"; +const buildRs = readFileSync( + join(repoRoot, "src", "ffi", "mxc_ffi", "build.rs"), + "utf8" +); +const csproj = readFileSync( + join( + repoRoot, + "sdk", + "dotnet", + "Microsoft.Mxc.Sdk", + "Microsoft.Mxc.Sdk.csproj" + ), + "utf8" +); + +// Enumerate every call site, then parse each one — rather than matching only +// the shape we expect. A regex that skips what it cannot read would let an +// unrecognised-but-live input (a trailing comma, a `const` argument, a call +// split across lines) pass unnoticed while the four literal calls keep the +// zero-call guard quiet. Anything unparseable fails the gate instead. +const callSites = [...buildRs.matchAll(/\.input_extern_file\s*\(/g)]; +if (callSites.length === 0) { + console.error( + "ERROR: found no `.input_extern_file(...)` calls in src/ffi/mxc_ffi/build.rs.\n" + + " The parity check cannot be trusted; has the codegen setup changed?" + ); + process.exit(1); +} + +const csbindgenInputs = []; +const unparseable = []; +for (const site of callSites) { + const rest = buildRs.slice(site.index + site[0].length); + // Only a plain string literal, optionally followed by a trailing comma and + // whitespace, is understood. Anything else is reported, not skipped. + const literal = rest.match(/^\s*"([^"]+)"\s*,?\s*\)/); + if (literal) { + csbindgenInputs.push(literal[1]); + } else { + unparseable.push(rest.split("\n")[0].trim().slice(0, 60)); + } +} +if (unparseable.length > 0) { + console.error( + "ERROR: could not read the argument of some `.input_extern_file(...)` call(s)\n" + + "in src/ffi/mxc_ffi/build.rs, so this gate cannot prove the MSBuild inputs\n" + + "cover them:\n" + + unparseable.map((s) => ` - .input_extern_file(${s}`).join("\n") + + "\n\nUse a plain string literal, or teach this check the new form." + ); + process.exit(1); +} + +// Scope to the owning target's opening tag — attributes live there, and +// matching only `` cannot bleed into child elements. The lookahead +// finds `Name` wherever it sits among the attributes: requiring it first would +// make legal, behaviour-preserving XML reordering look like a missing target. +// `\b` keeps `` from matching. +const target = csproj.match( + /]*\bName="GenerateNativeBindings")[^>]*>/ +); +if (!target) { + console.error( + "ERROR: could not find the `GenerateNativeBindings` target in the csproj." + ); + process.exit(1); +} +const inputsAttr = target[0].match(/Inputs="([^"]*)"/); +if (!inputsAttr) { + console.error( + "ERROR: the `GenerateNativeBindings` target declares no `Inputs` attribute,\n" + + " so MSBuild cannot know when to regenerate the bindings." + ); + process.exit(1); +} + +// build.rs paths are crate-relative ("src/lib.rs"); the csproj spells them from +// the repo's src dir ("$(MxcSrcDir)/ffi/mxc_ffi/src/lib.rs"). Compare on the +// crate-rooted tail so the check tolerates how the csproj roots itself but +// still rejects the same filename under a different crate. +const declaredInputs = inputsAttr[1] + .split(";") + .map((p) => p.trim().replace(/\\/g, "/")) + .filter(Boolean); +const notDeclared = csbindgenInputs.filter((rel) => { + const expected = `${CRATE_REL_ROOT}/${rel}`; + return !declaredInputs.some((declared) => declared.endsWith(expected)); +}); +if (notDeclared.length > 0) { + console.error( + "ERROR: Rust source(s) read by csbindgen are missing from the C# project's\n" + + "MSBuild `Inputs`, so an incremental build can skip regeneration and use\n" + + "stale bindings:\n" + + notDeclared.map((p) => ` - ${CRATE_REL_ROOT}/${p}`).join("\n") + + "\n\nAdd each to the GenerateNativeBindings `Inputs` in\n" + + " sdk/dotnet/Microsoft.Mxc.Sdk/Microsoft.Mxc.Sdk.csproj" + ); + process.exit(1); +} + console.log( - `C# bindings codegen OK: generated with ${REQUIRED_ENTRY_POINTS.length} expected entry points` + `C# bindings codegen OK: generated with ${REQUIRED_ENTRY_POINTS.length} expected entry points; ` + + `${csbindgenInputs.length} csbindgen source(s) all declared as MSBuild inputs` ); diff --git a/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/Microsoft.Mxc.Sdk.Tests.csproj b/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/Microsoft.Mxc.Sdk.Tests.csproj index 3124ae63a..c90f16e20 100644 --- a/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/Microsoft.Mxc.Sdk.Tests.csproj +++ b/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/Microsoft.Mxc.Sdk.Tests.csproj @@ -6,6 +6,8 @@ enable false true + + true diff --git a/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcExceptionTests.cs b/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcExceptionTests.cs new file mode 100644 index 000000000..519d913e8 --- /dev/null +++ b/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcExceptionTests.cs @@ -0,0 +1,126 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Linq; +using System.Reflection; +using Microsoft.Mxc.Sdk; +using Xunit; + +namespace Microsoft.Mxc.Sdk.Tests; + +/// +/// The structured failure detail: how it is carried, and how it renders. +/// +/// +/// The populated path is exercised here directly rather than through a native +/// call, because the native layer only fills these fields when a real platform +/// API fails — which needs a prepared host and a backend. What the native tests +/// below pin is the other half: that an error with no API behind it marshals as +/// rather than as an empty string. +/// +public class MxcExceptionTests +{ + [Fact] + public void OnlyTheCodeAndMessageConstructorIsPublic() + { + // The invariant fix is a single `internal` modifier on the structured + // constructor, and this assembly can see internals — so every other test + // here compiles whether that modifier says `internal` or `public`. + // Without this check, widening it back would restore the original defect + // (a NativeCode with no Operation, which ToString silently drops) with a + // completely green suite. + var publicConstructors = typeof(MxcException) + .GetConstructors(BindingFlags.Public | BindingFlags.Instance); + + var signatures = publicConstructors + .Select(c => string.Join(", ", c.GetParameters().Select(p => p.ParameterType.Name))) + .ToArray(); + + Assert.Equal( + new[] { $"{nameof(ErrorCode)}, {nameof(String)}" }, + signatures); + } + + [Fact] + public void ApiDetail_IsCarriedAlongsideTheCodeAndMessage() + { + var ex = new MxcException( + ErrorCode.BackendError, + "The provision was not found.", + "IsoSessionOps.StopSessionAsync", + "0x80070490", + "Provision the session first."); + + Assert.Equal(ErrorCode.BackendError, ex.Code); + Assert.Equal("The provision was not found.", ex.Message); + Assert.Equal("IsoSessionOps.StopSessionAsync", ex.Operation); + Assert.Equal("0x80070490", ex.NativeCode); + Assert.Equal("Provision the session first.", ex.Remediation); + } + + [Fact] + public void WithoutApiDetail_TheCallFieldsAreNull() + { + var ex = new MxcException(ErrorCode.MalformedRequest, "bad json"); + + Assert.Null(ex.Operation); + Assert.Null(ex.NativeCode); + Assert.Null(ex.Remediation); + } + + [Fact] + public void ToString_KeepsTheOperationAndStatusVisible() + { + var full = new MxcException( + ErrorCode.BackendError, + "The provision was not found.", + "IsoSessionOps.StopSessionAsync", + "0x80070490", + null); + Assert.Equal( + "BackendError: The provision was not found. [IsoSessionOps.StopSessionAsync 0x80070490]", + full.ToString()); + + // An operation with no status renders without a dangling separator. + var operationOnly = new MxcException( + ErrorCode.BackendError, "nope", "Iface.Call", null, null); + Assert.Equal("BackendError: nope [Iface.Call]", operationOnly.ToString()); + + // No detail at all renders exactly as it did before this surface existed. + var bare = new MxcException(ErrorCode.MalformedRequest, "bad"); + Assert.Equal("MalformedRequest: bad", bare.ToString()); + } + + [Fact] + public void NativeFailureWithNoApiCall_MarshalsAbsentFieldsAsNull() + { + // A version-less policy is rejected by the native parser before any + // backend API is reached, so the detail crosses with a message and + // nothing else. Null rather than empty is the point: it is how a caller + // tells "the API supplied no status" from "it supplied an empty one". + var policy = new SandboxPolicy { Version = string.Empty }; + + var ex = Assert.Throws(() => MxcSandbox.Run(policy, "echo hi")); + + Assert.Equal(ErrorCode.MalformedRequest, ex.Code); + Assert.False(string.IsNullOrEmpty(ex.Message)); + Assert.Null(ex.Operation); + Assert.Null(ex.NativeCode); + Assert.Null(ex.Remediation); + } + + [Fact] + public void NativeSpawnFailure_AlsoMarshalsThroughTheSharedShape() + { + // The streaming entry point fills a caller-provided detail rather than + // returning one inside a result struct; this pins that the same shape + // reaches the caller by that route too. + var policy = new SandboxPolicy { Version = string.Empty }; + + var ex = Assert.Throws(() => MxcSandbox.Spawn(policy, "echo hi")); + + Assert.Equal(ErrorCode.MalformedRequest, ex.Code); + Assert.False(string.IsNullOrEmpty(ex.Message)); + Assert.Null(ex.Operation); + } +} diff --git a/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/NativeErrorTests.cs b/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/NativeErrorTests.cs new file mode 100644 index 000000000..c3aa1995a --- /dev/null +++ b/sdk/dotnet/Microsoft.Mxc.Sdk.Tests/NativeErrorTests.cs @@ -0,0 +1,158 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Runtime.InteropServices; +using Microsoft.Mxc.Sdk; +using Microsoft.Mxc.Sdk.Native; +using Xunit; + +namespace Microsoft.Mxc.Sdk.Tests; + +/// +/// The marshalling step itself: a populated native detail becoming an +/// . +/// +/// +/// covers the halves either side of this one — +/// the managed exception's own behaviour, and the all-null detail that a +/// library-raised failure produces. Neither reaches +/// NativeError.ToException with the call fields filled, because the +/// native layer only fills them when a real platform API fails, which needs a +/// prepared host and a backend. So nothing pinned the field-to-property +/// mapping: transposing operation_utf8 and native_code_utf8 in +/// the marshalling left the whole suite green. Every value below is distinct +/// so that a transposition fails. +/// +public unsafe class NativeErrorTests +{ + /// + /// Allocate a native UTF-8 string, keeping distinct + /// from the empty string — the distinction the native contract rests on. + /// + private static byte* Utf8(string? value) => + value is null ? null : (byte*)Marshal.StringToCoTaskMemUTF8(value); + + private static MxcErrorDetail Detail( + string? message, + string? operation = null, + string? nativeCode = null, + string? remediation = null) => + new() + { + message_utf8 = Utf8(message), + operation_utf8 = Utf8(operation), + native_code_utf8 = Utf8(nativeCode), + remediation_utf8 = Utf8(remediation), + }; + + /// + /// Releases what allocated. Freeing a null pointer is a + /// no-op, so every field can be released unconditionally. + /// + /// + /// Test-owned memory: it was allocated here, so it is freed here. Product + /// code never does this — a real detail is allocated by the native layer and + /// released back to it, by mxc_error_detail_free when the detail + /// stands alone or by the owning result's free function when it is embedded + /// in one, because only the allocator that produced a pointer can free it. + /// + private static void Free(MxcErrorDetail detail) + { + Marshal.FreeCoTaskMem((IntPtr)detail.message_utf8); + Marshal.FreeCoTaskMem((IntPtr)detail.operation_utf8); + Marshal.FreeCoTaskMem((IntPtr)detail.native_code_utf8); + Marshal.FreeCoTaskMem((IntPtr)detail.remediation_utf8); + } + + [Fact] + public void EachNativeFieldLandsOnItsOwnProperty() + { + var detail = Detail( + "The provision was not found.", + "IsoSessionOps.StopSessionAsync", + "0x80070490", + "Provision the session first."); + + try + { + var ex = NativeError.ToException( + (int)ErrorCode.BackendError, detail, "unused fallback"); + + Assert.Equal(ErrorCode.BackendError, ex.Code); + Assert.Equal("The provision was not found.", ex.Message); + Assert.Equal("IsoSessionOps.StopSessionAsync", ex.Operation); + Assert.Equal("0x80070490", ex.NativeCode); + Assert.Equal("Provision the session first.", ex.Remediation); + } + finally + { + Free(detail); + } + } + + [Fact] + public void AnAbsentFieldBecomesNullAndAnEmptyOneStaysEmpty() + { + // The native layer uses null for "the API supplied nothing here" and + // reserves the empty string for "it supplied an empty value". Marshalling + // must not collapse the two, or a caller cannot tell them apart. + var detail = Detail("boom", "Iface.Call", nativeCode: string.Empty); + + try + { + var ex = NativeError.ToException( + (int)ErrorCode.BackendError, detail, "unused fallback"); + + Assert.Equal("Iface.Call", ex.Operation); + Assert.Equal(string.Empty, ex.NativeCode); + Assert.Null(ex.Remediation); + } + finally + { + Free(detail); + } + } + + [Fact] + public void AMessagelessDetailFallsBackRatherThanThrowingOrReportingNothing() + { + // Should not happen on a failure, but a null message must still leave + // the caller with something actionable rather than an empty exception. + var detail = Detail(null, "Iface.Call"); + + try + { + var ex = NativeError.ToException( + (int)ErrorCode.BackendError, detail, "the fallback"); + + Assert.Equal("the fallback", ex.Message); + Assert.Equal("Iface.Call", ex.Operation); + } + finally + { + Free(detail); + } + } + + [Fact] + public void TheStatusCodeCrossesAsTheErrorCode() + { + var detail = Detail("bad json"); + + try + { + var ex = NativeError.ToException( + (int)ErrorCode.MalformedRequest, detail, "unused fallback"); + + Assert.Equal(ErrorCode.MalformedRequest, ex.Code); + Assert.Null(ex.Operation); + Assert.Null(ex.NativeCode); + Assert.Null(ex.Remediation); + } + finally + { + Free(detail); + } + } +} diff --git a/sdk/dotnet/Microsoft.Mxc.Sdk/Microsoft.Mxc.Sdk.csproj b/sdk/dotnet/Microsoft.Mxc.Sdk/Microsoft.Mxc.Sdk.csproj index c33b1fd7f..8096643c3 100644 --- a/sdk/dotnet/Microsoft.Mxc.Sdk/Microsoft.Mxc.Sdk.csproj +++ b/sdk/dotnet/Microsoft.Mxc.Sdk/Microsoft.Mxc.Sdk.csproj @@ -36,7 +36,7 @@ diff --git a/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs b/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs index 3df094655..8483096de 100644 --- a/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs +++ b/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs @@ -8,18 +8,74 @@ namespace Microsoft.Mxc.Sdk; /// from the native layer; /// carries the human-readable detail. /// +/// +/// When the failure came from an underlying platform API, +/// names the call and carries its status. +/// holds an actionable hint whenever the failure has +/// one. +/// public sealed class MxcException : Exception { /// The typed error code. public ErrorCode Code { get; } + /// + /// The API call that failed, namespaced by its interface and free of call + /// parameters, so it can be grouped in telemetry. + /// when no API call was in flight. + /// + public string? Operation { get; } + + /// + /// The underlying platform status, for example 0x80070490. + /// when the failure carries none. + /// + public string? NativeCode { get; } + + /// + /// An actionable hint for the caller, when the failure carries one. + /// otherwise. + /// + public string? Remediation { get; } + /// Create an exception with the given code and message. public MxcException(ErrorCode code, string message) + : this(code, message, null, null, null) + { + } + + /// + /// Create an exception carrying the failing API call alongside the code and + /// message. + /// + internal MxcException( + ErrorCode code, + string message, + string? operation, + string? nativeCode, + string? remediation) : base(message) { Code = code; + Operation = operation; + NativeCode = nativeCode; + Remediation = remediation; } /// - public override string ToString() => $"{Code}: {Message}"; + /// + /// Appends the operation and status when present, so a caller that only + /// logs the exception keeps the diagnosis rather than losing it. + /// + public override string ToString() + { + if (Operation is null) + { + return $"{Code}: {Message}"; + } + + return NativeCode is null + ? $"{Code}: {Message} [{Operation}]" + : $"{Code}: {Message} [{Operation} {NativeCode}]"; + } } diff --git a/sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs b/sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs index e724d2966..32be52804 100644 --- a/sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs +++ b/sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs @@ -125,16 +125,21 @@ public static MxcSandboxProcess ExecInSandbox(SandboxId id, string command) fixed (byte* requestPtr = requestBuf) { NativeSandbox* handle = null; - byte* error = null; + MxcErrorDetail error = default; var status = NativeMethods.mxc_state_aware_exec(requestPtr, &handle, &error); if (status != (int)ErrorCode.Success) { - var message = PtrToString(error) ?? "unknown error"; - if (error is not null) + // See MxcSandbox.Spawn: the release belongs in `finally` so a throw + // during marshalling or exception construction cannot strand the + // detail's strings. + try { - NativeMethods.mxc_string_free(error); + throw NativeError.ToException(status, error, "unknown error"); + } + finally + { + NativeMethods.mxc_error_detail_free(&error); } - throw new MxcException((ErrorCode)status, message); } return new MxcSandboxProcess(MxcSandboxHandle.FromRaw(handle)); } @@ -250,8 +255,7 @@ private static void SetBackendConfig(JsonObject envelope, string phase, string k { if (status != (int)ErrorCode.Success) { - var message = PtrToString(result.error_utf8) ?? "unknown error"; - throw new MxcException((ErrorCode)status, message); + throw NativeError.ToException(status, result.error, "unknown error"); } var responseJson = PtrToString(result.response_json_utf8) ?? "{}"; var root = JsonNode.Parse(responseJson) as JsonObject; diff --git a/sdk/dotnet/Microsoft.Mxc.Sdk/MxcSandbox.cs b/sdk/dotnet/Microsoft.Mxc.Sdk/MxcSandbox.cs index 6c5b50f07..4ee04e0f5 100644 --- a/sdk/dotnet/Microsoft.Mxc.Sdk/MxcSandbox.cs +++ b/sdk/dotnet/Microsoft.Mxc.Sdk/MxcSandbox.cs @@ -72,8 +72,7 @@ public static RunResult Run(SandboxPolicy policy, string command) { if (status != (int)ErrorCode.Success) { - var message = PtrToString(result.error_utf8) ?? "unknown error"; - throw new MxcException((ErrorCode)status, message); + throw NativeError.ToException(status, result.error, "unknown error"); } return new RunResult @@ -132,16 +131,22 @@ public static MxcSandboxProcess Spawn(SandboxPolicy policy, string command) fixed (byte* commandPtr = commandBuf) { NativeSandbox* handle = null; - byte* error = null; + MxcErrorDetail error = default; var status = NativeMethods.mxc_spawn(policyPtr, commandPtr, &handle, &error); if (status != (int)ErrorCode.Success) { - var message = PtrToString(error) ?? "unknown error"; - if (error is not null) + // `finally`, not a straight-line free: marshalling the strings or + // allocating the exception can throw, and on that path the detail + // would never be released. Ownership has to be discharged however + // we leave this block. + try { - NativeMethods.mxc_string_free(error); + throw NativeError.ToException(status, error, "unknown error"); + } + finally + { + NativeMethods.mxc_error_detail_free(&error); } - throw new MxcException((ErrorCode)status, message); } return new MxcSandboxProcess(MxcSandboxHandle.FromRaw(handle), policy.TimeoutMs); } diff --git a/sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeError.cs b/sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeError.cs new file mode 100644 index 000000000..de55b2367 --- /dev/null +++ b/sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeError.cs @@ -0,0 +1,49 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Runtime.InteropServices; + +namespace Microsoft.Mxc.Sdk.Native; + +/// +/// Turns the native failure detail into an . +/// +/// +/// Shared rather than repeated per call site so every entry point reports a +/// failure the same way. Each field is carried across independently: an +/// operation with no status is a supported and tested shape, so this preserves +/// whatever the native layer supplied per field rather than treating the three +/// as a unit. Absence stays distinguishable from the empty string. +/// +internal static unsafe class NativeError +{ + /// + /// Build an exception from a native detail. + /// + /// The native status code. + /// The detail the native layer filled. + /// + /// Used when the native layer supplied no message — which should not happen + /// on a failure, but leaves the caller with something actionable if it does. + /// + internal static MxcException ToException( + int status, + MxcErrorDetail detail, + string fallbackMessage) + { + return new MxcException( + (ErrorCode)status, + ToStringOrNull(detail.message_utf8) ?? fallbackMessage, + ToStringOrNull(detail.operation_utf8), + ToStringOrNull(detail.native_code_utf8), + ToStringOrNull(detail.remediation_utf8)); + } + + /// + /// Marshal a native UTF-8 string, mapping a null pointer to + /// rather than to the empty string — the native layer + /// uses null to mean "the API supplied nothing here". + /// + internal static string? ToStringOrNull(byte* p) => + p is null ? null : Marshal.PtrToStringUTF8((IntPtr)p); +} diff --git a/sdk/dotnet/README.md b/sdk/dotnet/README.md index 790c251e7..e91e020fe 100644 --- a/sdk/dotnet/README.md +++ b/sdk/dotnet/README.md @@ -36,6 +36,24 @@ catch (MxcException ex) } ``` +When the failure came from an underlying platform API, `MxcException` also +carries which call failed and why: `Operation` names the call and `NativeCode` +carries its status (e.g. `0x80070490`), with `Remediation` holding an +actionable hint whenever the failure has one. `Operation` and `NativeCode` are +`null` for failures raised before any API call. `ToString()` appends the +operation and status, so logging the exception alone keeps the diagnosis: + +```csharp +catch (MxcException ex) when (ex.Operation is not null) +{ + Console.Error.WriteLine($"{ex.Operation} failed with {ex.NativeCode}: {ex.Message}"); + if (ex.Remediation is not null) + { + Console.Error.WriteLine($" try: {ex.Remediation}"); + } +} +``` + `MxcSandbox.RunAsync(policy, command)` offloads the blocking native call to the thread pool. `MxcSandbox.NativeVersion` returns the loaded `mxc_ffi` version. Optional feature outputs are returned through `RunResult.OutputMetadata`; for diff --git a/sdk/node/README.md b/sdk/node/README.md index 1c45f652b..531232f39 100644 --- a/sdk/node/README.md +++ b/sdk/node/README.md @@ -285,7 +285,7 @@ try { } ``` -`operation`, `nativeCode` and `remediation` are optional and travel together: `nativeCode` and `remediation` never appear without `operation`. A failure MXC raises before reaching the backend — a malformed request or id, or a policy rejection — carries only `code` and `message`. +`operation`, `nativeCode` and `remediation` are optional. A failure MXC raises before reaching the backend — a malformed request or id, or a policy rejection — carries only `code` and `message`. These three are currently populated only by **IsolationSession state-aware** operations. Windows Sandbox has no semantic error channel to derive them from, and the one-shot surface folds the same detail into `message` instead, so they are uniformly absent there — always treat them as optional. diff --git a/sdk/node/src/errors.ts b/sdk/node/src/errors.ts index b89e5667f..dd1b5c285 100644 --- a/sdk/node/src/errors.ts +++ b/sdk/node/src/errors.ts @@ -27,10 +27,9 @@ export type ErrorCode = * error envelope — `operation`, `nativeCode` and `remediation` sit alongside * `code` and `message`, not nested inside `details`. * - * **Invariant:** `nativeCode` implies `operation`, and `remediation` implies - * `operation`. `operation` marks that an underlying API call was in flight; - * the other two refine it, and neither appears on its own. A failure MXC - * raises before or outside any API call carries only `code` and `message`. + * **Invariant:** `operation` marks that an underlying API call was in flight. + * A failure MXC raises before or outside any API call carries only `code` and + * `message`. * * The invariant is guaranteed by the executor, which cannot construct a * violating envelope. This interface mirrors the flat wire shape rather than diff --git a/sdk/node/tests/integration/isolation-session-state-aware.test.ts b/sdk/node/tests/integration/isolation-session-state-aware.test.ts index 72d1aedeb..7bad27ee2 100644 --- a/sdk/node/tests/integration/isolation-session-state-aware.test.ts +++ b/sdk/node/tests/integration/isolation-session-state-aware.test.ts @@ -258,8 +258,7 @@ describe('IsolationSession state-aware policy validation', { skip: policyValidat // Full chain, negative case. An oversized appId is rejected by MXC's // own validation, before any IsolationSession API call is made. The // structured failure fields describe an API operation that was in flight; - // none was, so they must reach the caller absent rather than empty — - // `nativeCode` and `remediation` never appear without `operation`. + // none was, so they must reach the caller absent rather than empty. // // The canonical network acknowledgment is supplied so the only thing wrong // with this request is the appId; that keeps the assertion on the message diff --git a/src/backends/isolation_session/common/src/error.rs b/src/backends/isolation_session/common/src/error.rs index 6a6e86d94..e63ea1518 100644 --- a/src/backends/isolation_session/common/src/error.rs +++ b/src/backends/isolation_session/common/src/error.rs @@ -75,10 +75,8 @@ const NO_API_MESSAGE: &str = "the IsolationSession API reported a failure withou /// /// `operation` is always present — this type only describes failures where an /// API call was in flight. `code` is absent only when the status could not be -/// read; `remediation` only when the API supplied one. That is what upholds -/// the `MxcError` invariant that `nativeCode` and `remediation` never appear -/// without `operation`. `message` is likewise never empty — see -/// [`IsoApiFailure::new`]. +/// read; `remediation` only when the API supplied one. `message` is likewise +/// never empty — see [`IsoApiFailure::new`]. #[derive(Debug, Clone, PartialEq, Eq)] pub(super) struct IsoApiFailure { /// Interface-qualified operation, e.g. `IsoSessionOps.AddUserAsync`. @@ -694,8 +692,8 @@ mod tests { assert_eq!(mapped.remediation(), None); } - /// `nativeCode` implies `operation`, and `remediation` implies - /// `operation`. Neither may ever appear alone. + /// Every variant of this type describes a failure with an API call in + /// flight, so each carries an `operation`. #[test] fn every_variant_upholds_the_field_invariant() { let com = windows_core::Error::from_hresult(windows_core::HRESULT(0x80004005_u32 as i32)); diff --git a/src/core/mxc-sdk/README.md b/src/core/mxc-sdk/README.md index 94c990991..8b62c307e 100644 --- a/src/core/mxc-sdk/README.md +++ b/src/core/mxc-sdk/README.md @@ -53,6 +53,51 @@ Filesystem-policy discovery helpers (ports of the SDK's `policy.ts`) are also available to feed a policy: [`available_tools_policy`] (PATH + tool/SDK env dirs), [`user_profile_policy`], and [`temporary_files_policy`]. +## Diagnosing a failure + +Every fallible **entry point** — [`build_request`], +[`build_request_with_containment`], [`run`], [`spawn_sandbox`], +[`exec_sandbox`], [`run_state_aware_json`] — returns an [`Error`] carrying a +closed [`ErrorCode`] and a message, plus, when the failure came from an +underlying platform API, the call that failed and its status. + +The live [`Sandbox`] handle is the deliberate exception: `wait`, `try_wait`, +`wait_with_output` and `kill` return [`std::io::Result`], mirroring +[`std::process::Child`]. An `Err` from those is an actual OS wait or signal +failure — a timeout is [`WaitOutcome::TimedOut`], not an error. + +```rust,no_run +# fn report(error: mxc_sdk::Error) { +if let Some(operation) = &error.operation { + eprintln!("{operation} failed with {:?}", error.native_code); +} +if let Some(hint) = &error.remediation { + eprintln!(" try: {hint}"); +} +# } +``` + +[`Error::operation`] and [`Error::native_code`] are **absent** for a failure +raised before any API call was reached — a malformed policy, say — so their +presence tells you which side of the boundary the failure came from. An API that +names the call it failed in without supplying a status is a normal, tested +shape. [`Error::remediation`] is present whenever the failure has an actionable +hint. + +[`Error`] is `#[non_exhaustive]` — read its fields freely, but build one with +[`Error::new`] rather than by literal, so a field added later costs you nothing. + +`Display` appends the operation — and the status when there is one — to the +message, so a consumer that only logs the error does not silently lose them: + +```text +backend_error: The provision was not found. [IsoSessionOps.StopSessionAsync 0x80070490] +``` + +The same three fields cross the C ABI (`mxc_ffi`) and surface on the C# SDK's +`MxcException` as `Operation` / `NativeCode` / `Remediation`, so a diagnosis +made here reads the same from every binding. + ## Discovering host backends Two read-only probes answer "what can I run here?" — for two different diff --git a/src/core/mxc-sdk/src/lib.rs b/src/core/mxc-sdk/src/lib.rs index 62aef0b10..c6c0fc7fe 100644 --- a/src/core/mxc-sdk/src/lib.rs +++ b/src/core/mxc-sdk/src/lib.rs @@ -61,6 +61,28 @@ //! LXC) return an [`Error`] with [`ErrorCode::UnsupportedContainment`]; drive //! the standalone executor binaries for those. //! +//! # Diagnosing a failure +//! +//! [`Error`] carries a closed [`ErrorCode`] and a message, and — when the +//! failure came from an underlying platform API — the call that failed and its +//! status: +//! +//! ```no_run +//! # fn demo(error: mxc_sdk::Error) { +//! if let Some(operation) = &error.operation { +//! eprintln!("{operation} failed with {:?}", error.native_code); +//! } +//! if let Some(hint) = &error.remediation { +//! eprintln!(" try: {hint}"); +//! } +//! # } +//! ``` +//! +//! [`Error::operation`] and [`Error::native_code`] are absent for a failure +//! raised before any API call was reached — a malformed policy, say — and a +//! native code only ever appears alongside the operation it belongs to. +//! `Display` renders both, so logging the error alone does not lose them. +//! //! ```no_run //! use mxc_sdk::{build_request_with_containment, run, Containment, SandboxPolicy, WslcSection}; //! @@ -150,9 +172,11 @@ pub fn spawn_sandbox(request: SandboxRequest) -> Result { /// [`Error`]), or when waiting on the child fails at the OS level. pub fn run(request: SandboxRequest) -> Result { let sandbox = spawn_sandbox(request)?; - sandbox.wait_with_output().map_err(|e| Error { - code: ErrorCode::BackendError, - message: format!("waiting for the sandbox to complete failed: {e}"), + sandbox.wait_with_output().map_err(|e| { + Error::new( + ErrorCode::BackendError, + format!("waiting for the sandbox to complete failed: {e}"), + ) }) } diff --git a/src/core/mxc_engine/src/error.rs b/src/core/mxc_engine/src/error.rs index 02a3ffddc..990ddd718 100644 --- a/src/core/mxc_engine/src/error.rs +++ b/src/core/mxc_engine/src/error.rs @@ -70,18 +70,62 @@ impl From for ErrorCode { } /// An error returned by the SDK's fallible operations -/// ([`build_request`](crate::build_request) / [`spawn_sandbox`](crate::spawn_sandbox)). +/// ([`build_request`](crate::build_request) / [`spawn`](crate::spawn)). +/// +/// The detail fields sit flat on the error, the same way the wire format, the +/// C ABI and the C# SDK carry them — one failure reads the same whichever of +/// the four surfaces a caller is holding. +/// +/// Marked `#[non_exhaustive]`, as both the wire envelope and the internal error +/// this facades already are: read the fields, and build one with +/// [`Error::new`] rather than by literal, so a later field costs a downstream +/// crate nothing. #[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] pub struct Error { /// The closed error code. pub code: ErrorCode, /// A human-readable message. pub message: String, + /// The API call that failed, namespaced by its interface and free of call + /// parameters, so it can be grouped in telemetry. Absent when the failure + /// was raised before any API call. + pub operation: Option, + /// The underlying platform status, e.g. `0x80070490`. Absent when the + /// failure carries none. + pub native_code: Option, + /// An actionable "how to fix it" hint, when the failure carries one. + pub remediation: Option, } +impl Error { + /// An error with no API detail — the shape for failures raised before any + /// API call was made. + pub fn new(code: ErrorCode, message: impl Into) -> Self { + Self { + code, + message: message.into(), + operation: None, + native_code: None, + remediation: None, + } + } +} + +/// Renders `code: message`, then the failing call and its status in brackets +/// when present — so a consumer that only logs the error does not silently lose +/// the diagnosis. Mirrors the internal type's rendering. impl std::fmt::Display for Error { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}: {}", self.code, self.message) + write!(f, "{}: {}", self.code, self.message)?; + if let Some(operation) = &self.operation { + write!(f, " [{operation}")?; + if let Some(native_code) = &self.native_code { + write!(f, " {native_code}")?; + } + write!(f, "]")?; + } + Ok(()) } } @@ -89,9 +133,135 @@ impl std::error::Error for Error {} impl From for Error { fn from(error: MxcError) -> Self { + let (operation, native_code, remediation) = match error.api_failure { + Some(failure) => { + let failure = *failure; + ( + Some(failure.operation), + failure.native_code, + failure.remediation, + ) + } + None => (None, None, None), + }; Self { code: error.code.into(), message: error.message, + operation, + native_code, + remediation, } } } + +#[cfg(test)] +mod tests { + use super::*; + use wxc_common::mxc_error::{ApiFailure as InnerFailure, MxcError}; + + /// The conversion carries the API detail across, rather than keeping only + /// the code and message. + /// + /// The regression this pins: the facade used to copy `code`/`message` and + /// drop `api_failure` on the floor, so every in-process caller — Rust SDK, + /// FFI, and C# alike — lost the operation and platform status that the + /// backend had already produced. + #[test] + fn the_conversion_preserves_the_api_detail() { + let inner = MxcError::backend_error("The provision was not found.").with_api_failure( + InnerFailure::new("IsoSessionOps.StopSessionAsync") + .with_native_code("0x80070490") + .with_remediation("Provision the session first."), + ); + + let error = Error::from(inner); + + assert_eq!(error.code, ErrorCode::BackendError); + assert_eq!(error.message, "The provision was not found."); + assert_eq!( + error.operation.as_deref(), + Some("IsoSessionOps.StopSessionAsync") + ); + assert_eq!(error.native_code.as_deref(), Some("0x80070490")); + assert_eq!( + error.remediation.as_deref(), + Some("Provision the session first.") + ); + } + + /// A failure that names only its operation leaves the optional halves + /// absent rather than empty — `None` and `Some("")` are different answers + /// to "did the API supply a status?". + #[test] + fn an_operation_without_a_status_leaves_the_rest_absent() { + let error = Error::from( + MxcError::backend_error("boom").with_api_failure(InnerFailure::new("Iface.Call")), + ); + + assert_eq!(error.operation.as_deref(), Some("Iface.Call")); + assert_eq!(error.native_code, None); + assert_eq!(error.remediation, None); + } + + /// An error raised before any API call carries no detail at all, whether it + /// came through the conversion or was constructed directly. + #[test] + fn an_error_raised_before_any_api_call_carries_no_detail() { + let converted = Error::from(MxcError::malformed_request("bad json")); + + assert_eq!(converted.operation, None); + assert_eq!(converted.native_code, None); + assert_eq!(converted.remediation, None); + + let constructed = Error::new(ErrorCode::BackendError, "x"); + + assert_eq!(constructed.operation, None); + assert_eq!(constructed.native_code, None); + assert_eq!(constructed.remediation, None); + } + + /// The flat shape admits a remediation with no operation. `Display` renders + /// only the operation and status, so that combination has to come out as + /// plain `code: message` rather than as an empty bracket. + #[test] + fn a_remediation_without_an_operation_renders_without_brackets() { + let error = Error { + remediation: Some("Supply a supported policy.".into()), + ..Error::new(ErrorCode::PolicyValidation, "unsupported policy") + }; + + assert_eq!(error.to_string(), "policy_validation: unsupported policy"); + } + + /// Rendering keeps the operation and status visible, so a consumer that + /// only logs `{e}` does not lose the diagnosis. + #[test] + fn display_keeps_the_operation_and_status_visible() { + let with_status = Error::from( + MxcError::backend_error("The provision was not found.").with_api_failure( + InnerFailure::new("IsoSessionOps.StopSessionAsync").with_native_code("0x80070490"), + ), + ); + assert_eq!( + with_status.to_string(), + "backend_error: The provision was not found. \ + [IsoSessionOps.StopSessionAsync 0x80070490]" + ); + + // No status: the brackets carry the operation alone rather than a + // dangling separator. + let without_status = Error::from( + MxcError::backend_error("nope").with_api_failure(InnerFailure::new("Iface.Call")), + ); + assert_eq!( + without_status.to_string(), + "backend_error: nope [Iface.Call]" + ); + + // No detail at all: unchanged from before this change. + assert_eq!( + Error::from(MxcError::malformed_request("bad")).to_string(), + "malformed_request: bad" + ); + } +} diff --git a/src/core/wxc_common/src/mxc_error.rs b/src/core/wxc_common/src/mxc_error.rs index 03a4e0467..8ae35e966 100644 --- a/src/core/wxc_common/src/mxc_error.rs +++ b/src/core/wxc_common/src/mxc_error.rs @@ -505,10 +505,8 @@ mod tests { assert_eq!(err.remediation(), Some("Re-provision the sandbox.")); } - /// `native_code` and `remediation` live inside `ApiFailure`, so the - /// normal construction path cannot set them without an `operation` — - /// the envelope invariant holds by construction rather than by - /// convention. + /// `native_code` and `remediation` live inside `ApiFailure`, so the normal + /// construction path cannot set them without an `operation`. #[test] fn structured_detail_always_carries_an_operation() { let err = MxcError::backend_error("boom") diff --git a/src/ffi/mxc_ffi/build.rs b/src/ffi/mxc_ffi/build.rs index 7585939e1..e8134dba5 100644 --- a/src/ffi/mxc_ffi/build.rs +++ b/src/ffi/mxc_ffi/build.rs @@ -2,17 +2,20 @@ // Licensed under the MIT License. //! Generates the C# P/Invoke layer for the C# SDK from this crate's `extern -//! "C"` surface, using csbindgen. The generated file is checked in (a CI drift -//! gate regenerates and diffs it). +//! "C"` surface, using csbindgen. The generated file is gitignored rather than +//! committed, so it is regenerated rather than diffed. //! //! Code generation is gated behind the **`dotnetsdk`** cargo feature so that //! normal builds — including the whole-workspace backend build matrix — do -//! **not** compile csbindgen or write into the source tree. Only the drift gate -//! (`scripts/check-dotnet-bindings-codegen.js`, which builds with -//! `--features dotnetsdk`) regenerates the committed file. +//! **not** compile csbindgen or write into the source tree. Two callers pass +//! that feature: the C# csproj's `GenerateNativeBindings` target, which keeps +//! the bindings current for each C# compile, and +//! `scripts/check-dotnet-bindings-codegen.js`, which regenerates and asserts +//! the expected entry points are produced. fn main() { println!("cargo:rerun-if-changed=src/lib.rs"); + println!("cargo:rerun-if-changed=src/error_detail.rs"); println!("cargo:rerun-if-changed=src/streaming.rs"); println!("cargo:rerun-if-changed=src/state_aware.rs"); println!("cargo:rerun-if-changed=build.rs"); @@ -43,6 +46,7 @@ fn generate_csharp_bindings() { if let Err(e) = csbindgen::Builder::default() .input_extern_file("src/lib.rs") + .input_extern_file("src/error_detail.rs") .input_extern_file("src/streaming.rs") .input_extern_file("src/state_aware.rs") .csharp_dll_name("mxc_ffi") diff --git a/src/ffi/mxc_ffi/src/error_detail.rs b/src/ffi/mxc_ffi/src/error_detail.rs new file mode 100644 index 000000000..ad4ce38b1 --- /dev/null +++ b/src/ffi/mxc_ffi/src/error_detail.rs @@ -0,0 +1,247 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! The structured failure detail carried across the C ABI. +//! +//! One shape for every failing surface in this library, rather than a bare +//! message per surface: the run-to-completion result, the state-aware result, +//! and the out-parameter of the two entry points that hand back a live handle +//! all carry an [`MxcErrorDetail`]. A binding therefore learns *which API call +//! failed and with what platform status* from any of them, and frees them all +//! the same way. +//! +//! ## Ownership +//! +//! Every non-null field is owned by the caller and must be released. + +use std::ffi::c_char; +use std::panic::catch_unwind; +use std::ptr; + +use mxc_sdk::Error; + +use crate::{alloc_cstring, free_cstr}; + +/// Why a call failed: the message, plus the failing API call when one was in +/// flight. +/// +/// Every non-null field is owned by the caller and must be released — by +/// [`mxc_error_detail_free`] when the detail stands alone, or by the owning +/// result's free function when it is embedded in one. +#[repr(C)] +pub struct MxcErrorDetail { + /// Human-readable message (UTF-8, NUL-terminated), or null on success. + pub message_utf8: *mut c_char, + /// The API call that failed, namespaced by its interface and free of call + /// parameters, so it can be grouped in telemetry. Null when the failure was + /// raised before any API call, as a malformed request is. + pub operation_utf8: *mut c_char, + /// The underlying platform status, e.g. `0x80070490`. Null when the failure + /// carries none. + pub native_code_utf8: *mut c_char, + /// An actionable hint, when the failure carries one. Null otherwise. + pub remediation_utf8: *mut c_char, +} + +impl MxcErrorDetail { + /// The success shape: every field null. + pub(crate) fn none() -> Self { + Self { + message_utf8: ptr::null_mut(), + operation_utf8: ptr::null_mut(), + native_code_utf8: ptr::null_mut(), + remediation_utf8: ptr::null_mut(), + } + } + + /// A message with no API detail — for failures this library raises itself, + /// such as a policy that will not parse. + pub(crate) fn from_message(message: impl Into) -> Self { + Self { + message_utf8: alloc_cstring(message.into().as_bytes()), + ..Self::none() + } + } + + /// The full detail from an SDK error, carrying the API call across when the + /// error names one. + pub(crate) fn from_error(error: &Error) -> Self { + Self { + message_utf8: alloc_cstring(error.message.as_bytes()), + operation_utf8: opt_cstring(error.operation.as_deref()), + native_code_utf8: opt_cstring(error.native_code.as_deref()), + remediation_utf8: opt_cstring(error.remediation.as_deref()), + } + } + + /// Free every owned string, resetting each to null. Idempotent, so a + /// double free is a no-op rather than a fault. + pub(crate) fn free_strings(&mut self) { + free_cstr(&mut self.message_utf8); + free_cstr(&mut self.operation_utf8); + free_cstr(&mut self.native_code_utf8); + free_cstr(&mut self.remediation_utf8); + } +} + +/// Allocate an optional string, mapping absence to null. Absence and the empty +/// string stay distinguishable, which is the whole point: `None` means the API +/// supplied no status, `Some("")` would mean it supplied an empty one. +fn opt_cstring(value: Option<&str>) -> *mut c_char { + match value { + Some(text) => alloc_cstring(text.as_bytes()), + None => ptr::null_mut(), + } +} + +/// Free the strings owned by a standalone [`MxcErrorDetail`] — the shape the +/// out-parameter entry points fill. +/// +/// Does **not** free the struct itself: that storage belongs to the caller. +/// Passing null is a no-op, and calling it twice is safe. +/// +/// # Safety +/// `detail` must be null, or a valid pointer to an `MxcErrorDetail` this +/// library filled and nobody has freed by other means. +#[no_mangle] +pub unsafe extern "C" fn mxc_error_detail_free(detail: *mut MxcErrorDetail) { + if detail.is_null() { + return; + } + let _ = catch_unwind(|| { + // SAFETY: non-null per the check above, and valid per the caller contract. + unsafe { (*detail).free_strings() }; + }); +} + +#[cfg(test)] +mod tests { + use super::*; + use mxc_sdk::ErrorCode; + use std::ffi::CStr; + + /// Read an owned C string back, or `None` when the pointer is null. Absence + /// and emptiness must stay distinguishable. + fn read(p: *mut c_char) -> Option { + if p.is_null() { + None + } else { + // SAFETY: non-null per the check, and produced by `alloc_cstring`. + Some(unsafe { CStr::from_ptr(p) }.to_string_lossy().into_owned()) + } + } + + fn sdk_error_with_detail() -> Error { + let mut error = Error::new(ErrorCode::BackendError, "The provision was not found."); + error.operation = Some("IsoSessionOps.StopSessionAsync".into()); + error.native_code = Some("0x80070490".into()); + error.remediation = Some("Provision the session first.".into()); + error + } + + /// The whole detail crosses the boundary, not just the message. + /// + /// The regression this pins: the C ABI used to expose a bare `error_utf8`, + /// so a binding could report *that* a call failed but never *which* call or + /// with what platform status. + #[test] + fn an_sdk_error_carries_its_api_detail_across() { + let mut detail = MxcErrorDetail::from_error(&sdk_error_with_detail()); + + assert_eq!( + read(detail.message_utf8).as_deref(), + Some("The provision was not found.") + ); + assert_eq!( + read(detail.operation_utf8).as_deref(), + Some("IsoSessionOps.StopSessionAsync") + ); + assert_eq!(read(detail.native_code_utf8).as_deref(), Some("0x80070490")); + assert_eq!( + read(detail.remediation_utf8).as_deref(), + Some("Provision the session first.") + ); + + detail.free_strings(); + } + + /// An operation with no status leaves the optional halves NULL rather than + /// empty: `None` and `Some("")` are different answers to "did the API + /// supply a status?", and a binding maps null to `null`, not `""`. + #[test] + fn absent_optional_fields_stay_null_rather_than_empty() { + let mut error = Error::new(ErrorCode::BackendError, "boom"); + error.operation = Some("Iface.Call".into()); + let mut detail = MxcErrorDetail::from_error(&error); + + assert_eq!(read(detail.operation_utf8).as_deref(), Some("Iface.Call")); + assert!(detail.native_code_utf8.is_null()); + assert!(detail.remediation_utf8.is_null()); + + detail.free_strings(); + } + + /// A failure raised before any API call carries a message and nothing else. + #[test] + fn a_library_raised_failure_carries_only_a_message() { + let mut detail = MxcErrorDetail::from_message("policy JSON pointer is null"); + + assert_eq!( + read(detail.message_utf8).as_deref(), + Some("policy JSON pointer is null") + ); + assert!(detail.operation_utf8.is_null()); + assert!(detail.native_code_utf8.is_null()); + assert!(detail.remediation_utf8.is_null()); + + detail.free_strings(); + } + + /// An error with no API detail at all still produces a message-only detail, + /// so the success shape and the "no detail" shape stay distinguishable. + #[test] + fn an_error_without_api_detail_leaves_the_call_fields_null() { + let error = Error::new(ErrorCode::MalformedRequest, "bad json"); + let mut detail = MxcErrorDetail::from_error(&error); + + assert_eq!(read(detail.message_utf8).as_deref(), Some("bad json")); + assert!(detail.operation_utf8.is_null()); + + detail.free_strings(); + } + + /// Success is all-null, so a caller can test any field to see there was no + /// failure. + #[test] + fn the_success_shape_is_entirely_null() { + let detail = MxcErrorDetail::none(); + assert!(detail.message_utf8.is_null()); + assert!(detail.operation_utf8.is_null()); + assert!(detail.native_code_utf8.is_null()); + assert!(detail.remediation_utf8.is_null()); + } + + /// Freeing twice is a no-op rather than a double free, because each field + /// is nulled as it is released. + #[test] + fn freeing_is_idempotent() { + let mut detail = MxcErrorDetail::from_error(&sdk_error_with_detail()); + + // SAFETY: a valid, filled detail this test owns. + unsafe { mxc_error_detail_free(&mut detail) }; + assert!(detail.message_utf8.is_null()); + assert!(detail.operation_utf8.is_null()); + + // SAFETY: the same detail, already freed — must not fault. + unsafe { mxc_error_detail_free(&mut detail) }; + assert!(detail.message_utf8.is_null()); + } + + /// Freeing a null pointer is tolerated, matching the other `*_free` entry + /// points in this library. + #[test] + fn freeing_null_is_tolerated() { + // SAFETY: null is explicitly part of the contract. + unsafe { mxc_error_detail_free(ptr::null_mut()) }; + } +} diff --git a/src/ffi/mxc_ffi/src/lib.rs b/src/ffi/mxc_ffi/src/lib.rs index b66e9d86b..82b65ac0d 100644 --- a/src/ffi/mxc_ffi/src/lib.rs +++ b/src/ffi/mxc_ffi/src/lib.rs @@ -24,6 +24,12 @@ //! caller must free every non-null out-string via [`mxc_run_result_free`] //! (which frees a whole [`MxcRunResult`]) or [`mxc_string_free`]. The pointer //! returned by [`mxc_version`] is static and must **not** be freed. +//! - **Failures** carry an [`MxcErrorDetail`]: the message, plus the API call +//! that failed and its platform status when one was in flight. It is embedded +//! in the result structs (freed with them) and filled through the `out_error` +//! parameter of the handle-returning entry points (freed with +//! [`mxc_error_detail_free`]). A null field means the layer below supplied +//! nothing there — never an empty string. //! - **Never unwinds**: every entry point wraps its body in //! [`std::panic::catch_unwind`]; a panic becomes a status code //! ([`MXC_STATUS_PANIC`]), never an unwind across the boundary. @@ -51,8 +57,10 @@ use std::sync::OnceLock; use mxc_sdk::{build_request, run, ErrorCode, SandboxPolicy, WaitOutcome}; +mod error_detail; mod state_aware; mod streaming; +pub use error_detail::*; pub use state_aware::*; pub use streaming::*; @@ -122,11 +130,12 @@ pub(crate) fn status_from_error_code(code: ErrorCode) -> i32 { /// /// On success (`status == 0`), `exit_code` / `timed_out` describe how the /// process finished and `stdout_utf8` / `stderr_utf8` carry its captured output -/// (`error_utf8` is null). On failure, `error_utf8` carries a human-readable -/// message and the output fields are null. +/// (every field of `error` is null). On failure, `error` carries the message +/// and, when an API call was in flight, which call failed and with what +/// platform status; the output fields are null. /// -/// All non-null `*_utf8` pointers are owned by the caller and must be released -/// with [`mxc_run_result_free`]. +/// All non-null pointers — including those inside `error` — are owned by the +/// caller and must be released with [`mxc_run_result_free`]. #[repr(C)] pub struct MxcRunResult { /// `0` on success; otherwise one of the `MXC_STATUS_*` codes. @@ -139,8 +148,8 @@ pub struct MxcRunResult { pub stdout_utf8: *mut c_char, /// Captured stderr (UTF-8, NUL-terminated), or null. pub stderr_utf8: *mut c_char, - /// Error message (UTF-8, NUL-terminated) when `status != 0`, else null. - pub error_utf8: *mut c_char, + /// Why the call failed, when `status != 0`; all-null otherwise. + pub error: MxcErrorDetail, /// Structured output metadata JSON (UTF-8, NUL-terminated), or null. pub output_metadata_json_utf8: *mut c_char, /// Security warnings raised during the run, as a JSON array of strings @@ -161,16 +170,26 @@ impl MxcRunResult { timed_out: 0, stdout_utf8: ptr::null_mut(), stderr_utf8: ptr::null_mut(), - error_utf8: ptr::null_mut(), + error: MxcErrorDetail::none(), output_metadata_json_utf8: ptr::null_mut(), warnings_json_utf8: ptr::null_mut(), } } + /// A failure this library raised itself, with no API call behind it. fn error(status: i32, message: impl Into) -> Self { Self { status, - error_utf8: alloc_cstring(message.into().as_bytes()), + error: MxcErrorDetail::from_message(message), + ..Self::empty() + } + } + + /// A failure from the SDK, carrying its API detail across. + fn from_sdk_error(error: &mxc_sdk::Error) -> Self { + Self { + status: status_from_error_code(error.code), + error: MxcErrorDetail::from_error(error), ..Self::empty() } } @@ -179,7 +198,7 @@ impl MxcRunResult { fn free_strings(&mut self) { free_cstr(&mut self.stdout_utf8); free_cstr(&mut self.stderr_utf8); - free_cstr(&mut self.error_utf8); + self.error.free_strings(); free_cstr(&mut self.output_metadata_json_utf8); free_cstr(&mut self.warnings_json_utf8); } @@ -296,7 +315,7 @@ fn run_inner(policy_json_utf8: *const c_char, command_utf8: *const c_char) -> Mx let mut request = match build_request(&policy, None) { Ok(r) => r, - Err(e) => return MxcRunResult::error(status_from_error_code(e.code), e.message), + Err(e) => return MxcRunResult::from_sdk_error(&e), }; request.set_script(command); @@ -341,14 +360,14 @@ fn run_inner(policy_json_utf8: *const c_char, command_utf8: *const c_char) -> Mx timed_out, stdout_utf8: alloc_cstring(&output.stdout), stderr_utf8: alloc_cstring(&output.stderr), - error_utf8: ptr::null_mut(), + error: MxcErrorDetail::none(), output_metadata_json_utf8: output_metadata_json .map_or(ptr::null_mut(), |json| alloc_cstring(&json)), warnings_json_utf8: warnings_json .map_or(ptr::null_mut(), |json| alloc_cstring(&json)), } } - Err(e) => MxcRunResult::error(status_from_error_code(e.code), e.message), + Err(e) => MxcRunResult::from_sdk_error(&e), } } @@ -392,12 +411,21 @@ pub unsafe extern "C" fn mxc_string_free(s: *mut c_char) { /// /// The pointer is valid for the lifetime of the process and must **not** be /// freed. +/// +/// The body cannot panic today — the version is a compile-time constant and the +/// only fallible step is discharged with `unwrap_or_default` — but it is wrapped +/// anyway so the module's "every entry point" rule holds with no exception for a +/// reader to rediscover. The fallback is an empty **static** string rather than +/// null, so the pointer contract above holds on that path too. #[no_mangle] pub extern "C" fn mxc_version() -> *const c_char { static VERSION: OnceLock = OnceLock::new(); - VERSION - .get_or_init(|| CString::new(env!("CARGO_PKG_VERSION")).unwrap_or_default()) - .as_ptr() + catch_unwind(|| { + VERSION + .get_or_init(|| CString::new(env!("CARGO_PKG_VERSION")).unwrap_or_default()) + .as_ptr() + }) + .unwrap_or(c"".as_ptr()) } #[cfg(test)] @@ -419,18 +447,18 @@ mod tests { fn malformed_policy_json_reports_malformed_request() { let mut out = run_with("{ not json", Some("echo hi")); assert_eq!(out.status, MXC_STATUS_MALFORMED_REQUEST); - assert!(!out.error_utf8.is_null()); + assert!(!out.error.message_utf8.is_null()); assert!(out.stdout_utf8.is_null()); // SAFETY: `out` was filled by `mxc_run`. unsafe { mxc_run_result_free(&mut out) }; - assert!(out.error_utf8.is_null()); + assert!(out.error.message_utf8.is_null()); } #[test] fn null_command_reports_null_argument() { let mut out = run_with(r#"{"version":"0.7.0-alpha"}"#, None); assert_eq!(out.status, MXC_STATUS_NULL_ARGUMENT); - assert!(!out.error_utf8.is_null()); + assert!(!out.error.message_utf8.is_null()); unsafe { mxc_run_result_free(&mut out) }; } @@ -460,4 +488,85 @@ mod tests { mxc_string_free(ptr::null_mut()); } } + + /// `alloc_cstring` sanitizes interior NULs before `CString::new`, which is + /// that call's only failure mode, so it never returns null. Callers rely on + /// this: a null `message_utf8` means success, so a failure that allocated + /// null would read as one. + #[test] + fn alloc_cstring_never_returns_null() { + for input in [ + &b""[..], + b"plain", + b"interior\0nul", + b"\0leading", + b"trailing\0", + b"\0\0\0", + &[0xff, 0xfe, 0x00, 0x41][..], + ] { + let mut p = alloc_cstring(input); + assert!(!p.is_null(), "returned null for {input:?}"); + free_cstr(&mut p); + } + } + + /// A failure from the SDK reaches the caller with its API detail intact, + /// and the code maps to the matching `MXC_STATUS_*`. + #[test] + fn from_sdk_error_carries_the_api_detail() { + let mut error = + mxc_sdk::Error::new(ErrorCode::BackendError, "The provision was not found."); + error.operation = Some("IsoSessionOps.StopSessionAsync".to_string()); + error.native_code = Some("0x80070490".to_string()); + error.remediation = Some("Re-provision the sandbox.".to_string()); + + let mut result = MxcRunResult::from_sdk_error(&error); + assert_eq!(result.status, MXC_STATUS_BACKEND_ERROR); + + // SAFETY: every pointer was produced by `alloc_cstring` just above. + unsafe { + assert_eq!( + CStr::from_ptr(result.error.message_utf8).to_str().unwrap(), + "The provision was not found." + ); + assert_eq!( + CStr::from_ptr(result.error.operation_utf8) + .to_str() + .unwrap(), + "IsoSessionOps.StopSessionAsync" + ); + assert_eq!( + CStr::from_ptr(result.error.native_code_utf8) + .to_str() + .unwrap(), + "0x80070490" + ); + assert_eq!( + CStr::from_ptr(result.error.remediation_utf8) + .to_str() + .unwrap(), + "Re-provision the sandbox." + ); + } + + result.free_strings(); + } + + /// An empty version parses as JSON but fails `build_request`, which is the + /// arm that carries an SDK error rather than a message this library wrote. + /// The message is asserted because the JSON-parse arm returns the same + /// status. + #[test] + fn a_failing_build_request_reports_the_sdk_error() { + let mut out = run_with(r#"{"version":""}"#, Some("echo hi")); + assert_eq!(out.status, MXC_STATUS_MALFORMED_REQUEST); + // SAFETY: `out` was filled by `mxc_run`. + let message = unsafe { CStr::from_ptr(out.error.message_utf8) } + .to_str() + .unwrap() + .to_string(); + assert_eq!(message, "Policy version is required"); + // SAFETY: `out` was filled by `mxc_run`. + unsafe { mxc_run_result_free(&mut out) }; + } } diff --git a/src/ffi/mxc_ffi/src/state_aware.rs b/src/ffi/mxc_ffi/src/state_aware.rs index 6e838e5e4..7882dba61 100644 --- a/src/ffi/mxc_ffi/src/state_aware.rs +++ b/src/ffi/mxc_ffi/src/state_aware.rs @@ -26,24 +26,25 @@ use mxc_sdk::{exec_sandbox, run_state_aware_json}; use crate::streaming::MxcSandbox; use crate::{ - alloc_cstring, cstr_to_str, free_cstr, status_from_error_code, MXC_STATUS_INVALID_UTF8, - MXC_STATUS_NULL_ARGUMENT, MXC_STATUS_PANIC, MXC_STATUS_SUCCESS, + alloc_cstring, cstr_to_str, free_cstr, status_from_error_code, MxcErrorDetail, + MXC_STATUS_INVALID_UTF8, MXC_STATUS_NULL_ARGUMENT, MXC_STATUS_PANIC, MXC_STATUS_SUCCESS, }; /// The result of an [`mxc_state_aware`] call. /// /// On success (`status == 0`), `response_json_utf8` holds the response-envelope -/// JSON (`error_utf8` is null). On failure, `error_utf8` holds a human-readable -/// message and `response_json_utf8` is null. Both non-null pointers are owned by -/// the caller and released with [`mxc_state_aware_result_free`]. +/// JSON (every field of `error` is null). On failure, `error` carries the +/// message and, when an API call was in flight, which call failed and with what +/// platform status; `response_json_utf8` is null. All non-null pointers are +/// owned by the caller and released with [`mxc_state_aware_result_free`]. #[repr(C)] pub struct MxcStateAwareResult { /// `0` on success; otherwise one of the `MXC_STATUS_*` codes. pub status: i32, /// The response-envelope JSON (UTF-8, NUL-terminated) on success, else null. pub response_json_utf8: *mut c_char, - /// Error message (UTF-8, NUL-terminated) when `status != 0`, else null. - pub error_utf8: *mut c_char, + /// Why the call failed, when `status != 0`; all-null otherwise. + pub error: MxcErrorDetail, } impl MxcStateAwareResult { @@ -52,21 +53,31 @@ impl MxcStateAwareResult { Self { status: MXC_STATUS_SUCCESS, response_json_utf8: ptr::null_mut(), - error_utf8: ptr::null_mut(), + error: MxcErrorDetail::none(), } } + /// A failure this library raised itself, with no API call behind it. fn error(status: i32, message: impl Into) -> Self { Self { status, response_json_utf8: ptr::null_mut(), - error_utf8: alloc_cstring(message.into().as_bytes()), + error: MxcErrorDetail::from_message(message), + } + } + + /// A failure from the SDK, carrying its API detail across. + fn from_sdk_error(error: &mxc_sdk::Error) -> Self { + Self { + status: status_from_error_code(error.code), + response_json_utf8: ptr::null_mut(), + error: MxcErrorDetail::from_error(error), } } fn free_strings(&mut self) { free_cstr(&mut self.response_json_utf8); - free_cstr(&mut self.error_utf8); + self.error.free_strings(); } } @@ -127,9 +138,9 @@ fn state_aware_inner(request_json_utf8: *const c_char, dry_run: bool) -> MxcStat Ok(response_json) => MxcStateAwareResult { status: MXC_STATUS_SUCCESS, response_json_utf8: alloc_cstring(response_json.as_bytes()), - error_utf8: ptr::null_mut(), + error: MxcErrorDetail::none(), }, - Err(e) => MxcStateAwareResult::error(status_from_error_code(e.code), e.message), + Err(e) => MxcStateAwareResult::from_sdk_error(&e), } } @@ -156,27 +167,39 @@ pub unsafe extern "C" fn mxc_state_aware_result_free(r: *mut MxcStateAwareResult /// spawns the process, and on success writes an opaque /// [`MxcSandbox`](crate::MxcSandbox) handle to `*out_handle` (drive it with the /// `mxc_stream_*` / `mxc_sandbox_*` externs, free it with `mxc_sandbox_free`). -/// On failure returns the status code and, if `out_error` is non-null, writes an -/// owned UTF-8 error string to `*out_error`; `*out_handle` is set to null. +/// On failure returns the status code and, if `out_error` is non-null, fills it +/// with the message plus the failing API call when there was one (release it +/// with [`mxc_error_detail_free`](crate::mxc_error_detail_free)); +/// `*out_handle` is set to null. /// /// # Safety /// - `request_json_utf8` must be null or a valid NUL-terminated UTF-8 C string. -/// - `out_handle` must be non-null and point to writable pointer-sized storage; -/// on success the caller owns `*out_handle` and frees it with `mxc_sandbox_free`. -/// - `out_error` must be null or point to writable pointer-sized storage. +/// - `out_handle` must be non-null and point to writable pointer-sized storage +/// holding **no live handle** — it is overwritten with null before anything +/// else, and `mxc_sandbox_free` is the handle's only destructor, so free an +/// existing one before reusing its storage. On success the caller owns +/// `*out_handle` and frees it with `mxc_sandbox_free`. +/// - `out_error` must be null, or point to writable storage for one +/// [`MxcErrorDetail`] that holds **no live detail**: either fresh or +/// uninitialised storage, or storage already released with +/// [`mxc_error_detail_free`](crate::mxc_error_detail_free). This function +/// overwrites that storage without freeing what was there, so handing it a +/// populated detail leaks that detail's strings. #[no_mangle] pub unsafe extern "C" fn mxc_state_aware_exec( request_json_utf8: *const c_char, out_handle: *mut *mut MxcSandbox, - out_error: *mut *mut c_char, + out_error: *mut MxcErrorDetail, ) -> i32 { if !out_handle.is_null() { // SAFETY: caller-guaranteed writable pointer-sized storage. unsafe { *out_handle = ptr::null_mut() }; } if !out_error.is_null() { - // SAFETY: caller-guaranteed writable pointer-sized storage. - unsafe { *out_error = ptr::null_mut() }; + // `write` rather than assignment, for the reason given on `mxc_spawn`: + // the storage may be uninitialised, and nothing here is dropped. + // SAFETY: caller-guaranteed writable storage for one detail. + unsafe { ptr::write(out_error, MxcErrorDetail::none()) }; } if out_handle.is_null() { return MXC_STATUS_NULL_ARGUMENT; @@ -189,22 +212,29 @@ pub unsafe extern "C" fn mxc_state_aware_exec( None if request_json_utf8.is_null() => { return Err(( MXC_STATUS_NULL_ARGUMENT, - "request JSON pointer is null".to_string(), + MxcErrorDetail::from_message("request JSON pointer is null"), )) } None => { return Err(( MXC_STATUS_INVALID_UTF8, - "request JSON is not UTF-8".to_string(), + MxcErrorDetail::from_message("request JSON is not UTF-8"), )) } }; - exec_sandbox(request_json).map_err(|e| (status_from_error_code(e.code), e.message)) + exec_sandbox(request_json).map_err(|e| { + ( + status_from_error_code(e.code), + MxcErrorDetail::from_error(&e), + ) + }) })) - .unwrap_or(Err(( - MXC_STATUS_PANIC, - "the mxc engine panicked".to_string(), - ))); + .unwrap_or_else(|_| { + Err(( + MXC_STATUS_PANIC, + MxcErrorDetail::from_message("the mxc engine panicked"), + )) + }); // SAFETY: `out_handle` non-null (checked), `out_error` null or writable. unsafe { crate::streaming::finish_spawn(outcome, out_handle, out_error) } @@ -224,6 +254,45 @@ mod tests { out } + /// The state-aware result carries the same API detail as the + /// run-to-completion one, and maps the code the same way. + #[test] + fn from_sdk_error_carries_the_api_detail() { + let mut error = + mxc_sdk::Error::new(crate::ErrorCode::StaleId, "The provision was not found."); + error.operation = Some("IsoSessionOps.StopSessionAsync".to_string()); + error.native_code = Some("0x80070490".to_string()); + error.remediation = Some("Re-provision the sandbox.".to_string()); + + let mut result = MxcStateAwareResult::from_sdk_error(&error); + assert_eq!(result.status, crate::MXC_STATUS_STALE_ID); + assert!(result.response_json_utf8.is_null()); + + // SAFETY: every pointer was produced by `alloc_cstring` just above. + unsafe { + assert_eq!( + std::ffi::CStr::from_ptr(result.error.operation_utf8) + .to_str() + .unwrap(), + "IsoSessionOps.StopSessionAsync" + ); + assert_eq!( + std::ffi::CStr::from_ptr(result.error.native_code_utf8) + .to_str() + .unwrap(), + "0x80070490" + ); + assert_eq!( + std::ffi::CStr::from_ptr(result.error.remediation_utf8) + .to_str() + .unwrap(), + "Re-provision the sandbox." + ); + } + + result.error.free_strings(); + } + #[test] fn one_shot_config_is_malformed_request() { let mut out = call( @@ -232,10 +301,10 @@ mod tests { ); assert_eq!(out.status, crate::MXC_STATUS_MALFORMED_REQUEST); assert!(out.response_json_utf8.is_null()); - assert!(!out.error_utf8.is_null()); + assert!(!out.error.message_utf8.is_null()); // SAFETY: filled by `mxc_state_aware`. unsafe { mxc_state_aware_result_free(&mut out) }; - assert!(out.error_utf8.is_null()); + assert!(out.error.message_utf8.is_null()); } #[test] @@ -271,7 +340,7 @@ mod tests { // SAFETY: null request is explicitly handled; valid out pointer. let status = unsafe { mxc_state_aware(ptr::null(), 0, &mut out) }; assert_eq!(status, MXC_STATUS_NULL_ARGUMENT); - assert!(!out.error_utf8.is_null()); + assert!(!out.error.message_utf8.is_null()); // SAFETY: filled by `mxc_state_aware`. unsafe { mxc_state_aware_result_free(&mut out) }; } @@ -296,13 +365,13 @@ mod tests { fn exec_non_exec_phase_reports_error_and_null_handle() { let j = CString::new(r#"{"phase":"provision","containment":"isolation_session"}"#).unwrap(); let mut handle: *mut MxcSandbox = ptr::null_mut(); - let mut err: *mut c_char = ptr::null_mut(); + let mut err = MxcErrorDetail::none(); // SAFETY: valid string and out pointers. let status = unsafe { mxc_state_aware_exec(j.as_ptr(), &mut handle, &mut err) }; assert_eq!(status, crate::MXC_STATUS_MALFORMED_REQUEST); assert!(handle.is_null()); - assert!(!err.is_null()); - // SAFETY: `err` was allocated by `mxc_state_aware_exec`. - unsafe { crate::mxc_string_free(err) }; + assert!(!err.message_utf8.is_null()); + // SAFETY: `err` was filled by `mxc_state_aware_exec` and not yet freed. + unsafe { crate::mxc_error_detail_free(&mut err) }; } } diff --git a/src/ffi/mxc_ffi/src/streaming.rs b/src/ffi/mxc_ffi/src/streaming.rs index 3bc0ead5d..723e38db4 100644 --- a/src/ffi/mxc_ffi/src/streaming.rs +++ b/src/ffi/mxc_ffi/src/streaming.rs @@ -57,7 +57,7 @@ use std::ptr; use mxc_sdk::{build_request, spawn_sandbox, Sandbox, SandboxPolicy, WaitOutcome}; use crate::{ - alloc_cstring, cstr_to_str, status_from_error_code, MXC_STATUS_BACKEND_ERROR, + alloc_cstring, cstr_to_str, status_from_error_code, MxcErrorDetail, MXC_STATUS_BACKEND_ERROR, MXC_STATUS_INVALID_UTF8, MXC_STATUS_MALFORMED_REQUEST, MXC_STATUS_NULL_ARGUMENT, MXC_STATUS_PANIC, MXC_STATUS_SUCCESS, }; @@ -103,23 +103,34 @@ pub struct MxcWriteStream { /// Parses `policy_json_utf8` as a `SandboxPolicy`, sets `command_utf8` as the /// command to run, and spawns the process with piped stdio. On success writes /// the handle to `*out_handle` and returns [`MXC_STATUS_SUCCESS`]. On failure -/// returns the status code and, if `out_error` is non-null, writes an owned -/// UTF-8 error string to `*out_error` (free it with -/// [`mxc_string_free`](crate::mxc_string_free)); `*out_handle` is set to null. +/// returns the status code and, if `out_error` is non-null, fills it with the +/// message plus the failing API call when there was one (release it with +/// [`mxc_error_detail_free`](crate::mxc_error_detail_free)); `*out_handle` is +/// set to null. /// /// # Safety /// - `policy_json_utf8` / `command_utf8` must be null or valid NUL-terminated /// UTF-8 C strings. -/// - `out_handle` must be non-null and point to writable pointer-sized storage; -/// on success the caller owns `*out_handle` and must free it with -/// [`mxc_sandbox_free`]. -/// - `out_error` must be null or point to writable pointer-sized storage. +/// - `out_handle` must be non-null and point to writable pointer-sized storage +/// holding **no live handle**: this function overwrites it with null before +/// doing anything else, so a handle already stored there is stranded, and +/// [`mxc_sandbox_free`] is its only destructor. Free an existing handle +/// before reusing its storage. On success the caller owns `*out_handle` and +/// must free it with [`mxc_sandbox_free`]. +/// - `out_error` must be null, or point to writable storage for one +/// [`MxcErrorDetail`] that holds **no live detail**: either fresh or +/// uninitialised storage, or storage already released with +/// [`mxc_error_detail_free`](crate::mxc_error_detail_free). This function +/// overwrites that storage without freeing what was there, so handing it a +/// populated detail leaks that detail's strings. It cannot do otherwise: +/// uninitialised storage holds no pointers it could safely release, and +/// nothing distinguishes the two cases at runtime. #[no_mangle] pub unsafe extern "C" fn mxc_spawn( policy_json_utf8: *const c_char, command_utf8: *const c_char, out_handle: *mut *mut MxcSandbox, - out_error: *mut *mut c_char, + out_error: *mut MxcErrorDetail, ) -> i32 { // Initialise out-params defensively so a partial/failed call never leaves // stale pointers behind. @@ -128,8 +139,13 @@ pub unsafe extern "C" fn mxc_spawn( unsafe { *out_handle = ptr::null_mut() }; } if !out_error.is_null() { - // SAFETY: caller-guaranteed writable pointer-sized storage. - unsafe { *out_error = ptr::null_mut() }; + // `write` rather than assignment: the storage may be uninitialised, and + // assigning would be a claim that a valid value is being overwritten. + // Nothing is dropped either way -- the type owns raw pointers and has no + // destructor -- which is exactly why the contract above requires the + // caller to hand over storage holding no live detail. + // SAFETY: caller-guaranteed writable storage for one detail. + unsafe { ptr::write(out_error, MxcErrorDetail::none()) }; } if out_handle.is_null() { return MXC_STATUS_NULL_ARGUMENT; @@ -138,10 +154,12 @@ pub unsafe extern "C" fn mxc_spawn( let outcome = catch_unwind(AssertUnwindSafe(|| { spawn_inner(policy_json_utf8, command_utf8) })) - .unwrap_or(Err(( - MXC_STATUS_PANIC, - "the mxc engine panicked".to_string(), - ))); + .unwrap_or_else(|_| { + Err(( + MXC_STATUS_PANIC, + MxcErrorDetail::from_message("the mxc engine panicked"), + )) + }); // SAFETY: `out_handle` non-null (checked), `out_error` null or writable. unsafe { finish_spawn(outcome, out_handle, out_error) } @@ -149,16 +167,16 @@ pub unsafe extern "C" fn mxc_spawn( /// Shared tail of the handle-returning spawn entry points ([`mxc_spawn`] and /// `mxc_state_aware_exec`): on success box the [`Sandbox`] into an -/// [`MxcSandbox`] handle and write it to `*out_handle`; on failure write the -/// message to `*out_error` (when non-null) and return the status. +/// [`MxcSandbox`] handle and write it to `*out_handle`; on failure hand the +/// detail to `*out_error` (when non-null) and return the status. /// /// # Safety /// `out_handle` must be non-null and writable; `out_error` must be null or -/// writable. Both are pointer-sized. +/// point to writable storage for one [`MxcErrorDetail`]. pub(crate) unsafe fn finish_spawn( - outcome: Result, + outcome: Result, out_handle: *mut *mut MxcSandbox, - out_error: *mut *mut c_char, + out_error: *mut MxcErrorDetail, ) -> i32 { match outcome { Ok(sandbox) => { @@ -167,10 +185,17 @@ pub(crate) unsafe fn finish_spawn( unsafe { *out_handle = Box::into_raw(boxed) }; MXC_STATUS_SUCCESS } - Err((status, message)) => { - if !out_error.is_null() { - // SAFETY: `out_error` non-null and writable per the caller contract. - unsafe { *out_error = alloc_cstring(message.as_bytes()) }; + Err((status, mut detail)) => { + if out_error.is_null() { + // The caller does not want the detail, but it already owns heap + // strings — dropping the struct would leak every one of them, + // because raw pointers have no destructor. + detail.free_strings(); + } else { + // SAFETY: `out_error` non-null and writable per the caller contract, + // and initialised to an all-null detail by every caller before this + // point -- so nothing live is overwritten here. + unsafe { *out_error = detail }; } status } @@ -181,38 +206,59 @@ pub(crate) unsafe fn finish_spawn( fn spawn_inner( policy_json_utf8: *const c_char, command_utf8: *const c_char, -) -> Result { +) -> Result { // SAFETY: caller contract on `mxc_spawn`; borrowed only within scope. let policy_json = match unsafe { cstr_to_str(policy_json_utf8) } { Some(s) => s, None if policy_json_utf8.is_null() => { return Err(( MXC_STATUS_NULL_ARGUMENT, - "policy JSON pointer is null".into(), + MxcErrorDetail::from_message("policy JSON pointer is null"), + )) + } + None => { + return Err(( + MXC_STATUS_INVALID_UTF8, + MxcErrorDetail::from_message("policy JSON is not UTF-8"), )) } - None => return Err((MXC_STATUS_INVALID_UTF8, "policy JSON is not UTF-8".into())), }; let command = match unsafe { cstr_to_str(command_utf8) } { Some(s) => s, None if command_utf8.is_null() => { - return Err((MXC_STATUS_NULL_ARGUMENT, "command pointer is null".into())) + return Err(( + MXC_STATUS_NULL_ARGUMENT, + MxcErrorDetail::from_message("command pointer is null"), + )) + } + None => { + return Err(( + MXC_STATUS_INVALID_UTF8, + MxcErrorDetail::from_message("command is not UTF-8"), + )) } - None => return Err((MXC_STATUS_INVALID_UTF8, "command is not UTF-8".into())), }; let policy: SandboxPolicy = serde_json::from_str(policy_json).map_err(|e| { ( MXC_STATUS_MALFORMED_REQUEST, - format!("failed to parse policy JSON: {e}"), + MxcErrorDetail::from_message(format!("failed to parse policy JSON: {e}")), ) })?; - let mut request = - build_request(&policy, None).map_err(|e| (status_from_error_code(e.code), e.message))?; + let mut request = build_request(&policy, None).map_err(sdk_error_detail)?; request.set_script(command); - spawn_sandbox(request).map_err(|e| (status_from_error_code(e.code), e.message)) + spawn_sandbox(request).map_err(sdk_error_detail) +} + +/// Map an SDK error onto the status + detail pair the spawn chain carries, so +/// the failing API call survives instead of being flattened to a message. +fn sdk_error_detail(error: mxc_sdk::Error) -> (i32, MxcErrorDetail) { + ( + status_from_error_code(error.code), + MxcErrorDetail::from_error(&error), + ) } // --------------------------------------------------------------------------- @@ -633,14 +679,17 @@ mod tests { fn spawn_null_policy_reports_null_argument() { let command = CString::new("echo hi").unwrap(); let mut handle: *mut MxcSandbox = ptr::null_mut(); - let mut err: *mut c_char = ptr::null_mut(); + let mut err = MxcErrorDetail::none(); // SAFETY: null policy pointer is explicitly handled. let status = unsafe { mxc_spawn(ptr::null(), command.as_ptr(), &mut handle, &mut err) }; assert_eq!(status, MXC_STATUS_NULL_ARGUMENT); assert!(handle.is_null()); - assert!(!err.is_null(), "an error message should be provided"); - // SAFETY: `err` was allocated by `mxc_spawn`. - unsafe { crate::mxc_string_free(err) }; + assert!( + !err.message_utf8.is_null(), + "an error message should be provided" + ); + // SAFETY: `err` was filled by `mxc_spawn` and not yet freed. + unsafe { crate::mxc_error_detail_free(&mut err) }; } #[test] @@ -648,14 +697,14 @@ mod tests { let policy = CString::new("{ not json").unwrap(); let command = CString::new("echo hi").unwrap(); let mut handle: *mut MxcSandbox = ptr::null_mut(); - let mut err: *mut c_char = ptr::null_mut(); + let mut err = MxcErrorDetail::none(); // SAFETY: valid strings and valid out pointers. let status = unsafe { mxc_spawn(policy.as_ptr(), command.as_ptr(), &mut handle, &mut err) }; assert_eq!(status, MXC_STATUS_MALFORMED_REQUEST); assert!(handle.is_null()); - assert!(!err.is_null()); - // SAFETY: `err` was allocated by `mxc_spawn`. - unsafe { crate::mxc_string_free(err) }; + assert!(!err.message_utf8.is_null()); + // SAFETY: `err` was filled by `mxc_spawn` and not yet freed. + unsafe { crate::mxc_error_detail_free(&mut err) }; } #[test] @@ -745,7 +794,7 @@ mod tests { let command = CString::new("echo mxc_stream_ok").unwrap(); let mut handle: *mut MxcSandbox = ptr::null_mut(); - let mut err: *mut c_char = ptr::null_mut(); + let mut err = MxcErrorDetail::none(); // SAFETY: valid strings and out pointers. let status = unsafe { mxc_spawn(policy.as_ptr(), command.as_ptr(), &mut handle, &mut err) }; assert_eq!(status, MXC_STATUS_SUCCESS, "spawn failed (status {status})"); @@ -801,7 +850,7 @@ mod tests { .unwrap(); let mut handle: *mut MxcSandbox = ptr::null_mut(); - let mut err: *mut c_char = ptr::null_mut(); + let mut err = MxcErrorDetail::none(); // SAFETY: valid strings and out pointers. let status = unsafe { mxc_spawn(policy.as_ptr(), command.as_ptr(), &mut handle, &mut err) }; assert_eq!(status, MXC_STATUS_SUCCESS, "spawn failed (status {status})"); @@ -862,7 +911,7 @@ mod tests { CString::new("C:\\Windows\\System32\\cmd.exe /v:on /c set /p x= & echo done").unwrap(); let mut handle: *mut MxcSandbox = ptr::null_mut(); - let mut err: *mut c_char = ptr::null_mut(); + let mut err = MxcErrorDetail::none(); // SAFETY: valid strings and out pointers. let status = unsafe { mxc_spawn(policy.as_ptr(), command.as_ptr(), &mut handle, &mut err) }; assert_eq!(status, MXC_STATUS_SUCCESS, "spawn failed (status {status})"); diff --git a/src/ffi/mxc_ffi/tests/ffi.rs b/src/ffi/mxc_ffi/tests/ffi.rs index 133daf182..18af6c361 100644 --- a/src/ffi/mxc_ffi/tests/ffi.rs +++ b/src/ffi/mxc_ffi/tests/ffi.rs @@ -26,15 +26,17 @@ fn extern_run_rejects_malformed_policy() { assert_eq!(status, mxc_ffi::MXC_STATUS_MALFORMED_REQUEST); assert_eq!(out.status, status); - assert!(!out.error_utf8.is_null()); - // SAFETY: `error_utf8` is a valid C string filled by `mxc_run`. - let msg = unsafe { CStr::from_ptr(out.error_utf8) }.to_str().unwrap(); + assert!(!out.error.message_utf8.is_null()); + // SAFETY: the message is a valid C string filled by `mxc_run`. + let msg = unsafe { CStr::from_ptr(out.error.message_utf8) } + .to_str() + .unwrap(); assert!(msg.contains("policy"), "unexpected message: {msg}"); assert!(out.stdout_utf8.is_null()); // SAFETY: `out` was filled by `mxc_run`; frees its owned strings. unsafe { mxc_run_result_free(&mut out) }; - assert!(out.error_utf8.is_null()); + assert!(out.error.message_utf8.is_null()); } #[test]