diff --git a/AGENTS.md b/AGENTS.md index 3a567aa6e4..4a52ed34e8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,7 +73,7 @@ libdatadog is integrated into many runtimes and languages via FFI, and runs in D - Bubble errors up to the library caller with detail — prefer structured error enums (e.g. `thiserror`) over opaque strings. - Stay free of global effects unless a feature requires them: no spawning threads, no globals, no reading environment variables behind the caller's back. - Care about performance, especially memory allocations on hot paths. -- Panics across FFI boundaries are undefined behavior. FFI entry points must catch unwinds (e.g. `std::panic::catch_unwind`) and convert them into error returns rather than letting them propagate into the caller's runtime. +- A panic that reaches an `extern "C"` boundary aborts the host process. FFI entry points must catch unwinds (e.g. `std::panic::catch_unwind`) and convert them into error returns rather than letting them propagate into the caller's runtime. Whether a release artifact gets panic containment is decided by `builder` alone, through its `catch_panic` feature (a default), which propagates to the `catch_panic` feature in `libdd-profiling-ffi/Cargo.toml`. Projects building their own flavor with `builder`'s default features off ask for `catch_panic` explicitly; leaving it out yields abort-on-panic semantics. The FFI examples are what verify containment is on for our own release process. - The C FFI does **not** offer C ABI backward-compatibility guarantees: callers (Datadog SDKs) pin to specific libdatadog versions, so `#[repr(C)]` layouts, function signatures, and enum variants may change between releases. ### Cryptography diff --git a/builder/Cargo.toml b/builder/Cargo.toml index 57001b5550..20bd8e8331 100644 --- a/builder/Cargo.toml +++ b/builder/Cargo.toml @@ -20,6 +20,7 @@ default = [ "ddsketch", "ffe", "shared-runtime", + "catch_panic", ] crashtracker = [] profiling = [] @@ -35,11 +36,11 @@ ddsketch = [] ffe = [] shared-runtime = [] otel-thread-ctx = [] +catch_panic = [] regex-lite = ["libdd-common/regex-lite"] [lib] bench = false -test = false doctest = false [dependencies] diff --git a/builder/src/bin/release.rs b/builder/src/bin/release.rs index 33f7b23c30..c25f11604a 100644 --- a/builder/src/bin/release.rs +++ b/builder/src/bin/release.rs @@ -9,6 +9,7 @@ use builder::builder::Builder; use builder::common::Common; #[cfg(feature = "crashtracker")] use builder::crashtracker::CrashTracker; +use builder::features::{profiling_features, Selection}; #[cfg(feature = "profiling")] use builder::profiling::Profiling; use builder::utils::project_root; @@ -54,34 +55,7 @@ pub fn main() { host.clone() }; - #[allow(clippy::vec_init_then_push)] - let features = { - #[allow(unused_mut)] - let mut f: Vec = vec![]; - #[cfg(feature = "telemetry")] - f.push("ddtelemetry-ffi".to_string()); - #[cfg(feature = "data-pipeline")] - f.push("data-pipeline-ffi".to_string()); - #[cfg(feature = "data-pipeline-compression")] - f.push("data-pipeline-compression".to_string()); - #[cfg(feature = "crashtracker")] - f.push("crashtracker-ffi".to_string()); - #[cfg(feature = "symbolizer")] - f.push("symbolizer".to_string()); - #[cfg(feature = "library-config")] - f.push("datadog-library-config-ffi".to_string()); - #[cfg(feature = "log")] - f.push("datadog-log-ffi".to_string()); - #[cfg(feature = "ddsketch")] - f.push("ddsketch-ffi".to_string()); - #[cfg(feature = "ffe")] - f.push("libdd-ffe-ffi".to_string()); - #[cfg(feature = "shared-runtime")] - f.push("shared-runtime".to_string()); - #[cfg(feature = "otel-thread-ctx")] - f.push("otel-thread-ctx-ffi".to_string()); - f - }; + let features = profiling_features(&Selection::from_cargo_features()); let mut builder = Builder::new( source_path.to_str().unwrap(), diff --git a/builder/src/builder.rs b/builder/src/builder.rs index bce396f102..7ef6472ab5 100644 --- a/builder/src/builder.rs +++ b/builder/src/builder.rs @@ -40,7 +40,7 @@ use crate::utils::{file_replace, project_root}; /// Ok(()) /// } /// } -/// let mut builder = Builder::new("source", "target", "arch", "features", "profile", "version"); +/// let mut builder = Builder::new("source", "target", "arch", "profile", "features", "version"); /// let core = Box::new(Core { /// version: builder.version.clone(), /// }); diff --git a/builder/src/features.rs b/builder/src/features.rs new file mode 100644 index 0000000000..a05f21e0fa --- /dev/null +++ b/builder/src/features.rs @@ -0,0 +1,345 @@ +// Copyright 2026-Present Datadog, Inc. https://www.datadoghq.com/ +// SPDX-License-Identifier: Apache-2.0 + +//! Maps `builder`'s own Cargo features onto the `libdd-profiling-ffi` feature list used to +//! produce the combined artifact. +//! +//! Data plus a pure function rather than `#[cfg]` attributes, so that combinations no CI job +//! builds can still be asserted in unit tests. + +/// What a release build should contain. Selected through `builder`'s Cargo features. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct Selection { + pub telemetry: bool, + pub data_pipeline: bool, + pub data_pipeline_compression: bool, + pub crashtracker: bool, + pub symbolizer: bool, + pub library_config: bool, + pub log: bool, + pub ddsketch: bool, + pub ffe: bool, + pub shared_runtime: bool, + pub otel_thread_ctx: bool, + /// Return panics at the FFI boundary as errors instead of aborting the process. A + /// `builder` default; projects opting out of those defaults must ask for it explicitly. + pub catch_panic: bool, +} + +impl Selection { + /// Reads the selection from `builder`'s own compiled Cargo features. + pub fn from_cargo_features() -> Self { + Self { + telemetry: cfg!(feature = "telemetry"), + data_pipeline: cfg!(feature = "data-pipeline"), + data_pipeline_compression: cfg!(feature = "data-pipeline-compression"), + crashtracker: cfg!(feature = "crashtracker"), + symbolizer: cfg!(feature = "symbolizer"), + library_config: cfg!(feature = "library-config"), + log: cfg!(feature = "log"), + ddsketch: cfg!(feature = "ddsketch"), + ffe: cfg!(feature = "ffe"), + shared_runtime: cfg!(feature = "shared-runtime"), + otel_thread_ctx: cfg!(feature = "otel-thread-ctx"), + catch_panic: cfg!(feature = "catch_panic"), + } + } +} + +/// Needed whatever else is selected. `ddcommon-ffi` is here because it is a *default* of +/// `libdd-profiling-ffi`, which `--no-default-features` discards. +const ALWAYS: &[&str] = &["cbindgen", "ddcommon-ffi"]; + +/// The complete `--features` list for `cargo rustc -p libdd-profiling-ffi`. +/// +/// Callers **must** also pass `--no-default-features`: the returned list is exhaustive, so +/// nothing is inherited. Inheriting defaults is how `catch_panic` went missing from the +/// shipped artifact in APMSP-3830 without any build failing. +pub fn profiling_features(selection: &Selection) -> Vec { + let mut features: Vec<&str> = ALWAYS.to_vec(); + + if selection.catch_panic { + features.push("catch_panic"); + } + + if selection.telemetry { + features.push("ddtelemetry-ffi"); + } + if selection.data_pipeline { + features.push("data-pipeline-ffi"); + } + if selection.data_pipeline_compression { + features.push("data-pipeline-compression"); + } + if selection.crashtracker { + features.extend([ + "crashtracker-ffi", + "crashtracker-collector", + "crashtracker-receiver", + "demangler", + ]); + } + if selection.symbolizer { + features.push("symbolizer"); + } + if selection.library_config { + features.push("datadog-library-config-ffi"); + } + if selection.log { + features.push("datadog-log-ffi"); + } + if selection.ddsketch { + features.push("ddsketch-ffi"); + } + if selection.ffe { + features.push("libdd-ffe-ffi"); + } + if selection.shared_runtime { + features.push("shared-runtime"); + } + if selection.otel_thread_ctx { + features.push("otel-thread-ctx-ffi"); + } + + features.into_iter().map(str::to_string).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use std::path::{Path, PathBuf}; + use toml::Value; + + /// Every module-selecting flag, so the exhaustive test below cannot silently stop + /// covering a field that someone adds to [`Selection`]. + const MODULE_SETTERS: &[fn(&mut Selection)] = &[ + |s| s.telemetry = true, + |s| s.data_pipeline = true, + |s| s.data_pipeline_compression = true, + |s| s.crashtracker = true, + |s| s.symbolizer = true, + |s| s.library_config = true, + |s| s.log = true, + |s| s.ddsketch = true, + |s| s.ffe = true, + |s| s.shared_runtime = true, + |s| s.otel_thread_ctx = true, + ]; + + fn selection_from_bits(bits: u32) -> Selection { + let mut selection = Selection::default(); + for (index, set) in MODULE_SETTERS.iter().enumerate() { + if bits & (1 << index) != 0 { + set(&mut selection); + } + } + selection + } + + /// APMSP-3830 was the feature being present for some builds and absent for others, so + /// cover every combination a project can ask for, not just the one we release. + #[test] + fn catch_panic_survives_every_module_combination() { + for bits in 0..(1u32 << MODULE_SETTERS.len()) { + let mut selection = selection_from_bits(bits); + selection.catch_panic = true; + assert!( + profiling_features(&selection).contains(&"catch_panic".to_string()), + "catch_panic missing despite being selected, for {selection:?}" + ); + } + } + + /// The converse: not asking for it never sneaks it in, whatever else is selected. + #[test] + fn catch_panic_is_never_implied_by_another_feature() { + for bits in 0..(1u32 << MODULE_SETTERS.len()) { + let selection = selection_from_bits(bits); + assert!( + !profiling_features(&selection).contains(&"catch_panic".to_string()), + "catch_panic emitted without being selected, for {selection:?}" + ); + } + } + + /// Exhaustive struct literal on purpose, no `..Default::default()`: adding a field to + /// [`Selection`] breaks this until it also gets a branch and a `MODULE_SETTERS` entry. + #[test] + fn all_fields_set_emits_every_module_feature() { + let everything = Selection { + telemetry: true, + data_pipeline: true, + data_pipeline_compression: true, + crashtracker: true, + symbolizer: true, + library_config: true, + log: true, + ddsketch: true, + ffe: true, + shared_runtime: true, + otel_thread_ctx: true, + catch_panic: true, + }; + + assert_eq!( + profiling_features(&everything), + vec![ + "cbindgen", + "ddcommon-ffi", + "catch_panic", + "ddtelemetry-ffi", + "data-pipeline-ffi", + "data-pipeline-compression", + "crashtracker-ffi", + "crashtracker-collector", + "crashtracker-receiver", + "demangler", + "symbolizer", + "datadog-library-config-ffi", + "datadog-log-ffi", + "ddsketch-ffi", + "libdd-ffe-ffi", + "shared-runtime", + "otel-thread-ctx-ffi", + ] + ); + } + + /// Nothing is inherited under `--no-default-features`, so still-needed defaults must be + /// listed explicitly. + #[test] + fn always_lists_features_that_no_longer_come_from_defaults() { + for bits in 0..(1u32 << MODULE_SETTERS.len()) { + let features = profiling_features(&selection_from_bits(bits)); + for required in ALWAYS { + assert!( + features.contains(&required.to_string()), + "{required} missing for bits {bits:#b}" + ); + } + } + } + + #[test] + fn crashtracker_pulls_in_its_sub_features() { + let selection = Selection { + crashtracker: true, + ..Default::default() + }; + let features = profiling_features(&selection); + for expected in [ + "crashtracker-ffi", + "crashtracker-collector", + "crashtracker-receiver", + "demangler", + ] { + assert!(features.contains(&expected.to_string()), "{expected}"); + } + } + + #[test] + fn no_module_selected_emits_no_module_features() { + let features = profiling_features(&Selection::default()); + assert_eq!(features, vec!["cbindgen", "ddcommon-ffi"]); + } + + #[test] + fn emitted_list_has_no_duplicates() { + for bits in 0..(1u32 << MODULE_SETTERS.len()) { + let features = profiling_features(&selection_from_bits(bits)); + let mut sorted = features.clone(); + sorted.sort(); + sorted.dedup(); + assert_eq!(sorted.len(), features.len(), "duplicates for {bits:#b}"); + } + } + + // The tests above only see a hand-built `Selection`. These two cover the ends of the + // chain: `catch_panic` in builder's `default`, and the fan-out in libdd-profiling-ffi. + // Neither is reachable from examples/ffi/trace_exporter.c. + + fn workspace_root() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .expect("builder/ has a parent") + .to_path_buf() + } + + fn manifest(path: &Path) -> Value { + fs::read_to_string(path) + .unwrap_or_else(|e| panic!("reading {}: {e}", path.display())) + .parse() + .unwrap_or_else(|e| panic!("parsing {}: {e}", path.display())) + } + + fn declares_feature(manifest: &Value, name: &str) -> bool { + manifest + .get("features") + .and_then(Value::as_table) + .is_some_and(|features| features.contains_key(name)) + } + + /// Entries of a `[features]` list, empty when the feature is absent or has none. + fn feature<'a>(manifest: &'a Value, name: &str) -> Vec<&'a str> { + manifest + .get("features") + .and_then(Value::as_table) + .and_then(|features| features.get(name)) + .and_then(Value::as_array) + .map(|entries| entries.iter().filter_map(Value::as_str).collect()) + .unwrap_or_default() + } + + #[test] + fn builder_defaults_to_panic_containment() { + let builder = manifest(&workspace_root().join("builder/Cargo.toml")); + + assert!( + feature(&builder, "default").contains(&"catch_panic"), + "builder's default must include catch_panic, or the artifact aborts the host \ + process on a Rust panic" + ); + } + + #[test] + fn every_containment_capable_sub_crate_is_fanned_out() { + let root = workspace_root(); + let aggregator_dir = root.join("libdd-profiling-ffi"); + let aggregator = manifest(&aggregator_dir.join("Cargo.toml")); + let fan_out = feature(&aggregator, "catch_panic"); + + let dependencies = aggregator + .get("dependencies") + .and_then(Value::as_table) + .expect("libdd-profiling-ffi declares [dependencies]"); + + let mut found = 0; + for (name, spec) in dependencies { + if spec.get("optional").and_then(Value::as_bool) != Some(true) { + continue; + } + let Some(path) = spec.get("path").and_then(Value::as_str) else { + continue; + }; + let sub_crate = manifest(&aggregator_dir.join(path).join("Cargo.toml")); + if !declares_feature(&sub_crate, "catch_panic") { + continue; + } + + found += 1; + let entry = format!("{name}?/catch_panic"); + assert!( + fan_out.contains(&entry.as_str()), + "{name} has a catch_panic feature that libdd-profiling-ffi's catch_panic does \ + not enable; add \"{entry}\"" + ); + } + + assert!( + found > 0, + "no containment-capable optional dependency found, so this test is no longer \ + checking anything" + ); + } +} diff --git a/builder/src/lib.rs b/builder/src/lib.rs index 3151e7c408..e9a465e769 100644 --- a/builder/src/lib.rs +++ b/builder/src/lib.rs @@ -6,6 +6,7 @@ pub mod builder; pub mod common; #[cfg(feature = "crashtracker")] pub mod crashtracker; +pub mod features; pub mod module; pub mod utils; diff --git a/builder/src/profiling.rs b/builder/src/profiling.rs index 1e22132974..c79461a67a 100644 --- a/builder/src/profiling.rs +++ b/builder/src/profiling.rs @@ -8,7 +8,6 @@ use anyhow::Result; use serde::Deserialize; use std::ffi::OsStr; use std::fs; -use std::ops::Add; use std::path::{Path, PathBuf}; use std::process::Command; use std::rc::Rc; @@ -132,17 +131,14 @@ impl Profiling { impl Module for Profiling { fn build(&self) -> Result<()> { - let features = self.features.to_string() + "," + "cbindgen"; - #[cfg(feature = "crashtracker")] - let features = features.add(",crashtracker-collector,crashtracker-receiver,demangler"); - // Using rustc instead of build in order to overcome issues with LTO optimization. let mut cargo_args = vec![ "rustc", "-p", CRATE_FOLDER, + "--no-default-features", "--features", - &features, + &self.features, "--target", &self.arch, ]; diff --git a/examples/ffi/trace_exporter.c b/examples/ffi/trace_exporter.c index 38277aabf6..3fe7a90902 100644 --- a/examples/ffi/trace_exporter.c +++ b/examples/ffi/trace_exporter.c @@ -58,6 +58,29 @@ int log_init(const char* log_path) { return 0; } +int test_error_on_panic(void) { + ddog_TracerTraceChunks *chunks = NULL; + ddog_TraceExporterError *panic_err = ddog_tracer_trace_chunks_new(SIZE_MAX, &chunks); + + if (panic_err == NULL) { + fprintf(stderr, "capacity overflow unexpectedly succeeded\n"); + if (chunks != NULL) { ddog_tracer_trace_chunks_free(chunks); } + return 1; + } + if (panic_err->code != DDOG_TRACE_EXPORTER_ERROR_CODE_PANIC) { + fprintf(stderr, "capacity overflow returned error code %d instead of panic\n", + panic_err->code); + ddog_trace_exporter_error_free(panic_err); + return 1; + } + + // Containment worked: the panic came back as an error instead of aborting. Note the + // error *code* is deliberately not returned here — it is non-zero, so returning it + // would report this success as a failure. + ddog_trace_exporter_error_free(panic_err); + return 0; +} + int main(int argc, char** argv) { // Initialize logger with optional path from command line @@ -67,8 +90,18 @@ int main(int argc, char** argv) return 1; } - int error; + // Guard to check that the library is built with catch_panic by default. + // + // Returns directly rather than `goto error`: nothing has been allocated yet, and the + // cleanup at `error:` frees `trace_exporter` and `config`, which are declared below. + // Jumping from here would skip their initializers and free indeterminate pointers + // (gcc -Wjump-misses-init). + if (test_error_on_panic() != 0) { + return 1; + } + + int error; ddog_TraceExporter* trace_exporter = NULL; ddog_CharSlice url = DDOG_CHARSLICE_C("http://localhost:8126/"); ddog_CharSlice tracer_version = DDOG_CHARSLICE_C("v0.1"); diff --git a/libdd-profiling-ffi/Cargo.toml b/libdd-profiling-ffi/Cargo.toml index d1b0458ed9..f7a08492d8 100644 --- a/libdd-profiling-ffi/Cargo.toml +++ b/libdd-profiling-ffi/Cargo.toml @@ -17,7 +17,29 @@ bench = false name = "datadog_profiling_ffi" [features] -default = ["ddcommon-ffi"] +default = ["ddcommon-ffi", "catch_panic"] +# Panic containment for every aggregated FFI sub-crate that supports it. +# +# The sub-crates enable `catch_panic` in their own `default`, but this crate pulls +# them with `default-features = false`, so containment has to be re-enabled here. +# Without it `catch_panic!` degrades to a bare call and a Rust panic unwinds into a +# non-unwinding `extern "C"` frame, aborting the host process instead of returning +# an error (APMSP-3830). +# +# Weak (`?/`) on purpose: enabling this must never activate an optional dependency +# that was not otherwise requested. +# +# Every new aggregated FFI sub-crate declaring a `catch_panic` feature belongs here. +# Whether it is actually enabled is builder's decision: builder passes an exhaustive +# --features list with --no-default-features, and mirrors this as its own `catch_panic` +# default feature. Building without it is supported and yields abort-on-panic semantics. +# examples/ffi/trace_exporter.c verifies containment is on in the artifact our own +# release process produces. +catch_panic = [ + "libdd-data-pipeline-ffi?/catch_panic", + "libdd-library-config-ffi?/catch_panic", + "libdd-shared-runtime-ffi?/catch_panic", +] cbindgen = ["build_common/cbindgen", "libdd-common-ffi/cbindgen", "libdd-shared-runtime-ffi?/cbindgen", "libdd-otel-thread-ctx-ffi?/cbindgen"] ddtelemetry-ffi = ["dep:libdd-telemetry-ffi"] datadog-log-ffi = ["dep:libdd-log-ffi"] @@ -35,7 +57,7 @@ datadog-library-config-ffi = ["dep:libdd-library-config-ffi"] ddcommon-ffi = ["dep:libdd-common-ffi"] ddsketch-ffi = ["dep:libdd-ddsketch-ffi"] libdd-ffe-ffi = ["dep:libdd-ffe-ffi"] -shared-runtime = ["dep:libdd-shared-runtime-ffi", "libdd-shared-runtime-ffi/catch_panic"] +shared-runtime = ["dep:libdd-shared-runtime-ffi"] otel-thread-ctx-ffi = ["dep:libdd-otel-thread-ctx-ffi"] regex-lite = ["libdd-common/regex-lite"]