Skip to content

Commit 8733ac2

Browse files
authored
fix(sidecar-ffi): revert ddog_free_charslice to Box::from_raw (UB) (#2031)
# What does this PR do? Fix potential UB. Reverts `ddog_free_charslice` from `CString::from_raw` (introduced in #1698) back to `Box::from_raw`. Addresses [APMSP-3062](https://datadoghq.atlassian.net/browse/APMSP-3062). # Motivation #1698 changed the free function to use `CString::from_raw`, because the previous code was technically Undefined Behavior: we freed a single pointer instead of freeing a proper slice (wrong layout). In addition, while it turns out `CString` use a `Box<[u8]>` as a backing, it's not guaranteed nor exposed, so we should not rely on this. #1698 wrongly assumed that slices to be freed by `ddog_free_charslice` were coming from `ddog_span_debug_log` exclusively. In this case, using `CString::from_raw` to free is indeed correct. Unfortunately, `dd-trace-php` also frees slice obtained from `ddog_serialize_trace_into_charslice`. On binary MessagePack data this can theoretically cause a buffer over-read and heap corruption. The original bug hasn't been included in a release yet: #1698 landed after `v34.0.0`. # Additional Notes As mentioned, the previous behavior was technically UB. However, this was the behavior before #1698 and hasn't caused observed issues. It turns out the default allocator ignores the layout, as it probably keeps its own separate header data (https://doc.rust-lang.org/src/std/sys/alloc/unix.rs.html#47-49). As mentioned, `CString` does use `Box<[u8]>` under the hood. So while bad on paper, this was the status quo before and no actual corruption should be triggered as long as we keep using the main allocator. In the meantime, a proper fix (e.g. separate free functions or unified allocation strategy) should absolutely be made. # How to test the change? Run `cargo test -p datadog-sidecar-ffi`. [APMSP-3062]: https://datadoghq.atlassian.net/browse/APMSP-3062?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: yann.hamdaoui <yann.hamdaoui@datadoghq.com>
1 parent 3bf778c commit 8733ac2

1 file changed

Lines changed: 2 additions & 3 deletions

File tree

datadog-sidecar-ffi/src/span.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,10 +216,9 @@ pub unsafe extern "C" fn ddog_free_charslice(slice: CharSlice<'static>) {
216216
return;
217217
}
218218

219-
// Safety: the owned char slices returned by functions comes from `CString::into_raw` in
220-
// `ddog_span_debug_log`.
221219
unsafe {
222-
let _ = CString::from_raw(ptr as *mut c_char);
220+
let owned_ptr = ptr as *mut c_char;
221+
let _ = Box::from_raw(owned_ptr);
223222
}
224223
}
225224

0 commit comments

Comments
 (0)