Skip to content

Commit e53d1cf

Browse files
lloekiekumphoolioh
authored
fix(data-pipeline): contain panics in combined FFI (#2302)
AI-generated code disclosure: this PR was implemented with substantial AI assistance and manually reviewed and validated. # What does this PR do? Ensures the combined libdatadog profiling artifact enables `catch_panic` for data-pipeline FFI entry points. It also adds a packaged C regression that triggers capacity overflow and verifies that the FFI returns an error instead of aborting the process. # Motivation The standalone data-pipeline FFI crate enables panic containment by default, but the combined profiling artifact disables dependency defaults and did not re-enable it. This closes [APMSP-3830](https://datadoghq.atlassian.net/browse/APMSP-3830) before the native trace exporter payload APIs ship together. # Additional Notes **Stack position: L0, current PR: [DataDog/libdatadog PR 2302](#2302 These open L0-L5 changes are not included in [libdatadog v38.0.0](https://github.com/DataDog/libdatadog/releases/tag/v38.0.0). They are planned to ship together in a single libdatadog v39 release, with the common dd-trace-rb uptake tracked by [R3 DataDog/dd-trace-rb PR 6143](DataDog/dd-trace-rb#6143). Complete expected libdatadog review and merge order for that single v39 release: | Level | libdatadog PR | Matching dd-trace-rb consumer | | --- | --- | --- | | **L0 (current)** | **[DataDog/libdatadog PR 2302](#2302 | Release prerequisite; no payload API consumer | | L1 | [DataDog/libdatadog PR 2303](#2303) | [R4 DataDog/dd-trace-rb PR 6130](DataDog/dd-trace-rb#6130) | | L2 | [DataDog/libdatadog PR 2305](#2305) | [R5 DataDog/dd-trace-rb PR 6129](DataDog/dd-trace-rb#6129) | | L3 | [DataDog/libdatadog PR 2301](#2301) | [R6 DataDog/dd-trace-rb PR 6134](DataDog/dd-trace-rb#6134) | | L4 | [DataDog/libdatadog PR 2304](#2304) | [R7 DataDog/dd-trace-rb PR 6132](DataDog/dd-trace-rb#6132) | | L5 | [DataDog/libdatadog PR 2300](#2300) | [R8 DataDog/dd-trace-rb PR 6133](DataDog/dd-trace-rb#6133) | L0-L3 are independent and can be reviewed in parallel. Merging them in the listed order is still recommended because it reduces integration and release-branch conflicts, and lands panic containment before the new payload surfaces. L4 is stacked on L1, and L5 is stacked on L4. The matching payload consumers are [R4 DataDog/dd-trace-rb PR 6130](DataDog/dd-trace-rb#6130), [R5 DataDog/dd-trace-rb PR 6129](DataDog/dd-trace-rb#6129), [R6 DataDog/dd-trace-rb PR 6134](DataDog/dd-trace-rb#6134), [R7 DataDog/dd-trace-rb PR 6132](DataDog/dd-trace-rb#6132), and [R8 DataDog/dd-trace-rb PR 6133](DataDog/dd-trace-rb#6133). All depend through the common v39 uptake in [R3 DataDog/dd-trace-rb PR 6143](DataDog/dd-trace-rb#6143). The downstream review and merge order after R3 is R4, R5, R6, R7, then R8; R7 also depends on R4, and R8 depends on R7. [DataDog/libdatadog-rb v37 PR 65](DataDog/libdatadog-rb#65) and [DataDog/libdatadog-rb v38 PR 66](DataDog/libdatadog-rb#66) are separate ordered prerequisite releases only for [R2 DataDog/dd-trace-rb PR 6131](DataDog/dd-trace-rb#6131) and its `ForkSafeRuntime` uptake. They do not contain the open L0-L5 changes or payload APIs. R3 is stacked on R2, but R3, rather than either prerequisite package, is the common v39 payload-API uptake. L0 tracks [APMSP-3830](https://datadoghq.atlassian.net/browse/APMSP-3830); L1-L5 implement the payload work tracked by [APMSP-3219](https://datadoghq.atlassian.net/browse/APMSP-3219). # How to test the change? Prior local validation completed successfully: - `nix develop -c cargo test -p libdd-profiling-ffi --features data-pipeline-ffi` passed the combined profiling/data-pipeline FFI suite: 57 tests. - `nix develop -c cargo test -p libdd-data-pipeline-ffi --features catch_panic trace_chunks_new_with_overflow_capacity_returns_panic_error` passed. - `nix develop -c cargo test -p libdd-data-pipeline-ffi --features catch_panic begin_chunk_with_overflow_capacity_returns_panic_error` passed. - `nix develop -c cargo ffi-test --filter trace_exporter_panic_containment` built and passed the packaged C panic-containment regression. - `nix develop .#nightly --command cargo fmt --all -- --check` passed. - `nix develop -c cargo clippy -p libdd-profiling-ffi --all-targets --features data-pipeline-ffi -- -D warnings` passed. [APMSP-3830]: https://datadoghq.atlassian.net/browse/APMSP-3830?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [APMSP-3830]: https://datadoghq.atlassian.net/browse/APMSP-3830?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: ekump <edmund.kump@datadoghq.com> Co-authored-by: hoolioh <julio.gonzalez@datadoghq.com>
1 parent 31b871c commit e53d1cf

9 files changed

Lines changed: 412 additions & 40 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ libdatadog is integrated into many runtimes and languages via FFI, and runs in D
7373
- Bubble errors up to the library caller with detail — prefer structured error enums (e.g. `thiserror`) over opaque strings.
7474
- Stay free of global effects unless a feature requires them: no spawning threads, no globals, no reading environment variables behind the caller's back.
7575
- Care about performance, especially memory allocations on hot paths.
76-
- 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.
76+
- 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.
7777
- 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.
7878

7979
### Cryptography

builder/Cargo.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ default = [
2020
"ddsketch",
2121
"ffe",
2222
"shared-runtime",
23+
"catch_panic",
2324
]
2425
crashtracker = []
2526
profiling = []
@@ -35,11 +36,11 @@ ddsketch = []
3536
ffe = []
3637
shared-runtime = []
3738
otel-thread-ctx = []
39+
catch_panic = []
3840
regex-lite = ["libdd-common/regex-lite"]
3941

4042
[lib]
4143
bench = false
42-
test = false
4344
doctest = false
4445

4546
[dependencies]

builder/src/bin/release.rs

Lines changed: 2 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use builder::builder::Builder;
99
use builder::common::Common;
1010
#[cfg(feature = "crashtracker")]
1111
use builder::crashtracker::CrashTracker;
12+
use builder::features::{profiling_features, Selection};
1213
#[cfg(feature = "profiling")]
1314
use builder::profiling::Profiling;
1415
use builder::utils::project_root;
@@ -54,34 +55,7 @@ pub fn main() {
5455
host.clone()
5556
};
5657

57-
#[allow(clippy::vec_init_then_push)]
58-
let features = {
59-
#[allow(unused_mut)]
60-
let mut f: Vec<String> = vec![];
61-
#[cfg(feature = "telemetry")]
62-
f.push("ddtelemetry-ffi".to_string());
63-
#[cfg(feature = "data-pipeline")]
64-
f.push("data-pipeline-ffi".to_string());
65-
#[cfg(feature = "data-pipeline-compression")]
66-
f.push("data-pipeline-compression".to_string());
67-
#[cfg(feature = "crashtracker")]
68-
f.push("crashtracker-ffi".to_string());
69-
#[cfg(feature = "symbolizer")]
70-
f.push("symbolizer".to_string());
71-
#[cfg(feature = "library-config")]
72-
f.push("datadog-library-config-ffi".to_string());
73-
#[cfg(feature = "log")]
74-
f.push("datadog-log-ffi".to_string());
75-
#[cfg(feature = "ddsketch")]
76-
f.push("ddsketch-ffi".to_string());
77-
#[cfg(feature = "ffe")]
78-
f.push("libdd-ffe-ffi".to_string());
79-
#[cfg(feature = "shared-runtime")]
80-
f.push("shared-runtime".to_string());
81-
#[cfg(feature = "otel-thread-ctx")]
82-
f.push("otel-thread-ctx-ffi".to_string());
83-
f
84-
};
58+
let features = profiling_features(&Selection::from_cargo_features());
8559

8660
let mut builder = Builder::new(
8761
source_path.to_str().unwrap(),

builder/src/builder.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ use crate::utils::{file_replace, project_root};
4040
/// Ok(())
4141
/// }
4242
/// }
43-
/// let mut builder = Builder::new("source", "target", "arch", "features", "profile", "version");
43+
/// let mut builder = Builder::new("source", "target", "arch", "profile", "features", "version");
4444
/// let core = Box::new(Core {
4545
/// version: builder.version.clone(),
4646
/// });

0 commit comments

Comments
 (0)