From 90a5f40bf4754fe922251e9547ce2a7645e26ba7 Mon Sep 17 00:00:00 2001 From: adpa-ms <> Date: Sun, 16 Aug 2026 20:21:31 -0700 Subject: [PATCH 1/6] Point the engine's error doc at the entry point this crate actually has The doc comment on the SDK error linked `crate::spawn_sandbox`, which does not exist in `mxc_engine` -- that is `mxc-sdk`'s name for the wrapper. The engine's own streaming entry point is `spawn`. Rustdoc resolves intra-doc links against the crate being documented, so this fails `cargo rustdoc -p mxc_engine -- -D warnings`. Pre-dates the surrounding change; corrected here because this branch is the next thing to touch the file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1 --- src/core/mxc_engine/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/mxc_engine/src/error.rs b/src/core/mxc_engine/src/error.rs index 02a3ffddc..9aad9c090 100644 --- a/src/core/mxc_engine/src/error.rs +++ b/src/core/mxc_engine/src/error.rs @@ -70,7 +70,7 @@ 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)). #[derive(Debug, Clone, PartialEq, Eq)] pub struct Error { /// The closed error code. From 72d320ecd3d55f510c73964375767cf0f3d6509e Mon Sep 17 00:00:00 2001 From: adpa-ms <> Date: Sun, 16 Aug 2026 20:21:54 -0700 Subject: [PATCH 2/6] Carry the failing API's detail through the SDK error type The public SDK error is a facade over the foundation crate's error, and it copied only the code and message -- so the operation, platform status and remediation the backend had already produced were dropped at that boundary. Every in-process caller lost the diagnosis: the Rust SDK, the C ABI over it, and the C# binding beyond that. A caller could see "backend_error: The provision was not found." with nothing to say which call failed or why. The three carry flat on Error rather than nested behind a sub-struct, matching the wire envelope, the C ABI and the C# binding. One failure then reads the same whichever of the four surfaces a caller is holding, which is worth more than making an invariant hold by construction on exactly one of them. The half of that invariant which is real is documented instead: a native code only ever appears alongside the operation it belongs to, because a status with no call to attribute it to is not something a producer can express. A remediation carries no such coupling -- it is an actionable hint, and nothing about a hint requires an API call to have been in flight. Display renders the operation and status in brackets, so a consumer that only logs the error keeps the diagnosis rather than silently losing it. A remediation with no operation renders as plain code and message, not as an empty bracket. Error is #[non_exhaustive], as both the wire envelope and the internal error it facades already are. Adding that attribute after the fact is a breaking change and removing it is not -- measured, not assumed: a downstream crate compiled against a non-exhaustive type still builds after the attribute is removed, while adding it fails with E0639 and E0004. So the choice belongs here, while nothing yet consumes the surface. The crate documentation gains a worked example of reading the detail, and says what a caller needs to know: that the operation and status are absent for a failure raised before any API call was reached, and that a native code never appears without one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1 --- src/core/mxc-sdk/src/lib.rs | 30 +++++- src/core/mxc_engine/src/error.rs | 173 ++++++++++++++++++++++++++++++- 2 files changed, 199 insertions(+), 4 deletions(-) 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 9aad9c090..7ce917eba 100644 --- a/src/core/mxc_engine/src/error.rs +++ b/src/core/mxc_engine/src/error.rs @@ -71,17 +71,62 @@ impl From for ErrorCode { /// An error returned by the SDK's fallible operations /// ([`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`. Only ever present + /// alongside [`operation`](Self::operation): a status with no call to + /// attribute it to is not something a producer can express. + 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 +134,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" + ); + } +} From 768617fc183439ef418ddee06884909c0d47211f Mon Sep 17 00:00:00 2001 From: adpa-ms <> Date: Sun, 16 Aug 2026 20:22:27 -0700 Subject: [PATCH 3/6] Give the C ABI one shape for why a call failed Each failing surface carried a bare message: the run result, the state-aware result, and the out-parameter of the two entry points that hand back a live handle. A binding could report that a call failed but not which call, nor with what platform status -- detail the backend had already produced and the SDK now carries. All three now carry an MxcErrorDetail. One struct means a binding learns the same things from any surface and frees them all the same way, and it is the shape the experimental opt-in rung will extend rather than reshape a second time. Each field crosses independently, and the module header says exactly which couplings are real: a native code is non-null only when the operation is, because a status with no call to attribute it to is not something a producer can express, while a remediation carries no such coupling at all. An operation with no status is a supported shape, pinned by absent_optional_fields_stay_null_rather_than_empty, so the contract does not claim the three either all cross or all stay null. The out-parameter changes from an owned string to caller-provided storage for one detail. That moves a responsibility: the error owns heap strings with no destructor, so a caller passing null would leak every one of them. finish_spawn frees the detail itself in that case rather than dropping a struct of raw pointers on the floor. The contract requires storage holding no live detail, and says why the callee cannot simply free what was there: uninitialised storage holds no pointers it could release, and nothing tells the two cases apart at runtime. Initialisation uses a write rather than an assignment to say exactly that. The module header promises that every entry point wraps its body in catch_unwind, so a panic becomes a status code rather than an unwind across the C ABI, where unwinding is undefined behaviour. Two did not: the new mxc_error_detail_free, and mxc_version. The second is benign -- its version is a compile-time constant and its only fallible step is discharged with unwrap_or_default, so it cannot unwind -- but a blanket claim with a silent exception is worse than either a smaller claim or no exception. Both are wrapped now, so the rule is exceptionless and grep-checkable at 23 of 23, rather than something a reader has to re-derive by working out which bodies can panic. mxc_version's fallback returns an empty static string rather than null, so its documented "valid for the lifetime of the process and must not be freed" contract holds on that unreachable path too. The build script's header stops claiming the generated bindings are checked in and diffed. They are gitignored and regenerated -- as the same file already says nine lines further down -- and the header now names both callers that regenerate them rather than only the gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1 --- src/ffi/mxc_ffi/build.rs | 14 +- src/ffi/mxc_ffi/src/error_detail.rs | 252 ++++++++++++++++++++++++++++ src/ffi/mxc_ffi/src/lib.rs | 64 +++++-- src/ffi/mxc_ffi/src/state_aware.rs | 98 +++++++---- src/ffi/mxc_ffi/src/streaming.rs | 137 ++++++++++----- src/ffi/mxc_ffi/tests/ffi.rs | 10 +- 6 files changed, 470 insertions(+), 105 deletions(-) create mode 100644 src/ffi/mxc_ffi/src/error_detail.rs 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..2d64c12a9 --- /dev/null +++ b/src/ffi/mxc_ffi/src/error_detail.rs @@ -0,0 +1,252 @@ +// 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. +//! +//! ## Invariant +//! +//! `native_code_utf8` is non-null only when `operation_utf8` is — a status with +//! no call to attribute it to is not something a producer can express. +//! `remediation_utf8` carries no such coupling: it is populated from its own +//! field on the SDK error, independently of the other two. + +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 unless + /// `operation_utf8` is non-null. + pub native_code_utf8: *mut c_char, + /// An actionable hint, when the failure carries one. Null otherwise — this + /// field does not depend on `operation_utf8`. + 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, + /// which is what upholds "a native code implies an operation". + #[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..0681fac12 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) }; } diff --git a/src/ffi/mxc_ffi/src/state_aware.rs b/src/ffi/mxc_ffi/src/state_aware.rs index 6e838e5e4..77b03d7d7 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) } @@ -232,10 +262,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 +301,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 +326,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] From 11da3e7e0ffc798cd6bfb21cbc99b8ce328e68ba Mon Sep 17 00:00:00 2001 From: adpa-ms <> Date: Sun, 16 Aug 2026 20:22:50 -0700 Subject: [PATCH 4/6] Surface the failing API call to C# callers MxcException carried a code and a message, so the managed binding lost the same diagnosis the layers beneath it had just been taught to carry. It now exposes Operation, NativeCode and Remediation, and ToString appends the operation and status so a caller that only logs the exception keeps them. The three are documented rather than enforced where enforcement would be a lie: a native code is non-null only when an operation is, because the native layer cannot produce a status with no call to attribute it to. A remediation carries no such coupling. The five-argument constructor stays internal, which keeps the first implication true by construction rather than by convention -- a public overload taking three independent nullable strings would let a caller build the state the documentation says cannot exist, and ToString would then silently drop the status. NativeError.ToException is the one place the native detail becomes managed, and it marshals each field independently: null and the empty string stay distinct, because the native contract distinguishes "the API supplied nothing" from "it supplied an empty value". Both callers release the native detail in a finally block, so a throw during marshalling or exception construction cannot strand the strings it owns. NativeErrorTests covers that marshalling step, which nothing pinned before: transposing operation_utf8 and native_code_utf8 left the whole suite green, because the tests either side of it drive the managed exception directly or the all-null detail a library-raised failure produces. Every value in the new tests is distinct so a transposition fails. The tests fabricate the struct and free their own allocations, and say why -- a reviewer read those frees as evidence that product code must release marshalled strings by hand, when the opposite holds: the test allocated that memory so the test frees it, while a real detail goes back to the native allocator, through mxc_error_detail_free when it stands alone or the owning result's free function when it is embedded. The codegen gate additionally asserts mxc_error_detail_free, and checks that every Rust source csbindgen reads is also declared as an MSBuild input -- the lists having drifted once already, which is how an incremental C# build can compile against stale declarations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1 --- scripts/check-dotnet-bindings-codegen.js | 126 +++++++++++++- .../Microsoft.Mxc.Sdk.Tests.csproj | 2 + .../MxcExceptionTests.cs | 126 ++++++++++++++ .../NativeErrorTests.cs | 158 ++++++++++++++++++ .../Microsoft.Mxc.Sdk.csproj | 2 +- sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs | 74 +++++++- sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs | 18 +- sdk/dotnet/Microsoft.Mxc.Sdk/MxcSandbox.cs | 19 ++- .../Microsoft.Mxc.Sdk/Native/NativeError.cs | 49 ++++++ 9 files changed, 554 insertions(+), 20 deletions(-) create mode 100644 sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcExceptionTests.cs create mode 100644 sdk/dotnet/Microsoft.Mxc.Sdk.Tests/NativeErrorTests.cs create mode 100644 sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeError.cs 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..dd589a432 100644 --- a/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs +++ b/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs @@ -8,18 +8,90 @@ 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. Both are +/// for failures raised before any API call was reached — +/// a malformed policy, say. holds an actionable hint +/// whenever the failure has one. +/// +/// +/// is non-null only when is: a +/// status with no call to attribute it to is not something the native layer can +/// express. carries no such coupling. +/// +/// 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. + /// unless is set. + /// + 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. + /// + /// + /// Deliberately : keeping the overload internal is + /// what makes the documented implication — a native code implies an + /// operation — hold by construction rather than by convention. A public + /// overload taking three independent nullable strings would let a caller + /// build the state the documentation says cannot exist, and + /// would then silently drop the status. + /// + 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); +} From fade1d684a9a71abac4756244d4ed247bc64e890 Mon Sep 17 00:00:00 2001 From: adpa-ms <> Date: Sun, 16 Aug 2026 20:23:04 -0700 Subject: [PATCH 5/6] Document the structured error surface across both SDKs Repository convention requires a public SDK API change to update the crate documentation and the SDK README together. The Rust crate docs gained the worked example when the error type changed; the two READMEs had not caught up, and the architecture notes still described the C ABI's old result shape. The Rust SDK README gains a "Diagnosing a failure" section: which entry points return an Error, that the live Sandbox handle is the deliberate exception returning io::Result, how to read the detail, and that a native code only ever appears alongside the operation it belongs to while a remediation does not. It also states that Error is #[non_exhaustive], so a caller builds one with Error::new rather than by literal. The C# README documents the same three properties on MxcException and the same coupling, so a reader arriving from either binding is told the same thing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02c36d36-fb3d-446f-9108-4e4a01b01af1 --- .github/copilot-instructions.md | 2 +- sdk/dotnet/README.md | 19 ++++++++++++++ src/core/mxc-sdk/README.md | 46 +++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) 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/sdk/dotnet/README.md b/sdk/dotnet/README.md index 790c251e7..90fbc0fb4 100644 --- a/sdk/dotnet/README.md +++ b/sdk/dotnet/README.md @@ -36,6 +36,25 @@ 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 — a malformed policy, say — and +`NativeCode` is non-null only when `Operation` is. `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/src/core/mxc-sdk/README.md b/src/core/mxc-sdk/README.md index 94c990991..1aed91152 100644 --- a/src/core/mxc-sdk/README.md +++ b/src/core/mxc-sdk/README.md @@ -53,6 +53,52 @@ 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. A native +code only ever appears alongside the operation it belongs to, and an API that +names the call it failed in without supplying a status is a normal, tested +shape. [`Error::remediation`] carries no such coupling: it 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 From c8a8ec074a749c9e16128ff6915a95d2d2162852 Mon Sep 17 00:00:00 2001 From: adpa-ms Date: Tue, 18 Aug 2026 15:43:08 -0700 Subject: [PATCH 6/6] Drop the field-coupling claims and prove the error detail crosses The docs stated that `nativeCode` and `remediation` never appear without `operation`. Nothing enforced it, and this change had already decoupled `remediation` on the flattened public error, so the statements were false wherever they still appeared. Removed rather than enforced: the coupling was never wanted. That reaches the TypeScript SDK and both wire docs because they carried the same claim. `from_sdk_error` and the `alloc_cstring` non-null guarantee had no tests. The guarantee is load-bearing: a null message means success, so a failure that allocated null would read as one. --- docs/isolation-session/state-aware-rust.md | 3 +- .../mxc-state-aware-sandbox-api.md | 9 +-- sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs | 24 +----- sdk/dotnet/README.md | 3 +- sdk/node/README.md | 2 +- sdk/node/src/errors.ts | 7 +- .../isolation-session-state-aware.test.ts | 3 +- .../isolation_session/common/src/error.rs | 10 +-- src/core/mxc-sdk/README.md | 7 +- src/core/mxc_engine/src/error.rs | 5 +- src/core/wxc_common/src/mxc_error.rs | 6 +- src/ffi/mxc_ffi/src/error_detail.rs | 17 ++-- src/ffi/mxc_ffi/src/lib.rs | 81 +++++++++++++++++++ src/ffi/mxc_ffi/src/state_aware.rs | 39 +++++++++ 14 files changed, 152 insertions(+), 64 deletions(-) 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/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs b/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs index dd589a432..8483096de 100644 --- a/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs +++ b/sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs @@ -9,18 +9,10 @@ namespace Microsoft.Mxc.Sdk; /// carries the human-readable detail. /// /// -/// /// When the failure came from an underlying platform API, -/// names the call and carries its status. Both are -/// for failures raised before any API call was reached — -/// a malformed policy, say. holds an actionable hint -/// whenever the failure has one. -/// -/// -/// is non-null only when is: a -/// status with no call to attribute it to is not something the native layer can -/// express. carries no such coupling. -/// +/// names the call and carries its status. +/// holds an actionable hint whenever the failure has +/// one. /// public sealed class MxcException : Exception { @@ -36,7 +28,7 @@ public sealed class MxcException : Exception /// /// The underlying platform status, for example 0x80070490. - /// unless is set. + /// when the failure carries none. /// public string? NativeCode { get; } @@ -56,14 +48,6 @@ public MxcException(ErrorCode code, string message) /// Create an exception carrying the failing API call alongside the code and /// message. /// - /// - /// Deliberately : keeping the overload internal is - /// what makes the documented implication — a native code implies an - /// operation — hold by construction rather than by convention. A public - /// overload taking three independent nullable strings would let a caller - /// build the state the documentation says cannot exist, and - /// would then silently drop the status. - /// internal MxcException( ErrorCode code, string message, diff --git a/sdk/dotnet/README.md b/sdk/dotnet/README.md index 90fbc0fb4..e91e020fe 100644 --- a/sdk/dotnet/README.md +++ b/sdk/dotnet/README.md @@ -40,8 +40,7 @@ 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 — a malformed policy, say — and -`NativeCode` is non-null only when `Operation` is. `ToString()` appends the +`null` for failures raised before any API call. `ToString()` appends the operation and status, so logging the exception alone keeps the diagnosis: ```csharp 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 1aed91152..8b62c307e 100644 --- a/src/core/mxc-sdk/README.md +++ b/src/core/mxc-sdk/README.md @@ -79,11 +79,10 @@ if let Some(hint) = &error.remediation { [`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. A native -code only ever appears alongside the operation it belongs to, and an API that +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`] carries no such coupling: it is present whenever -the failure has an actionable hint. +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. diff --git a/src/core/mxc_engine/src/error.rs b/src/core/mxc_engine/src/error.rs index 7ce917eba..990ddd718 100644 --- a/src/core/mxc_engine/src/error.rs +++ b/src/core/mxc_engine/src/error.rs @@ -91,9 +91,8 @@ pub struct Error { /// 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`. Only ever present - /// alongside [`operation`](Self::operation): a status with no call to - /// attribute it to is not something a producer can express. + /// 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, 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/src/error_detail.rs b/src/ffi/mxc_ffi/src/error_detail.rs index 2d64c12a9..ad4ce38b1 100644 --- a/src/ffi/mxc_ffi/src/error_detail.rs +++ b/src/ffi/mxc_ffi/src/error_detail.rs @@ -10,12 +10,9 @@ //! failed and with what platform status* from any of them, and frees them all //! the same way. //! -//! ## Invariant +//! ## Ownership //! -//! `native_code_utf8` is non-null only when `operation_utf8` is — a status with -//! no call to attribute it to is not something a producer can express. -//! `remediation_utf8` carries no such coupling: it is populated from its own -//! field on the SDK error, independently of the other two. +//! Every non-null field is owned by the caller and must be released. use std::ffi::c_char; use std::panic::catch_unwind; @@ -39,11 +36,10 @@ pub struct MxcErrorDetail { /// 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 unless - /// `operation_utf8` is non-null. + /// 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 — this - /// field does not depend on `operation_utf8`. + /// An actionable hint, when the failure carries one. Null otherwise. pub remediation_utf8: *mut c_char, } @@ -185,8 +181,7 @@ mod tests { detail.free_strings(); } - /// A failure raised before any API call carries a message and nothing else, - /// which is what upholds "a native code implies an operation". + /// 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"); diff --git a/src/ffi/mxc_ffi/src/lib.rs b/src/ffi/mxc_ffi/src/lib.rs index 0681fac12..82b65ac0d 100644 --- a/src/ffi/mxc_ffi/src/lib.rs +++ b/src/ffi/mxc_ffi/src/lib.rs @@ -488,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 77b03d7d7..7882dba61 100644 --- a/src/ffi/mxc_ffi/src/state_aware.rs +++ b/src/ffi/mxc_ffi/src/state_aware.rs @@ -254,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(