fix(sidecar-ffi): revert ddog_free_charslice to Box::from_raw (UB)#2031
Conversation
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
yannham
changed the title
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: f19f327 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2031 +/- ##
==========================================
- Coverage 72.76% 72.74% -0.02%
==========================================
Files 458 458
Lines 75789 75790 +1
==========================================
- Hits 55147 55135 -12
- Misses 20642 20655 +13
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c944b2c37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unsafe { | ||
| let _ = CString::from_raw(ptr as *mut c_char); | ||
| let owned_ptr = ptr as *mut c_char; | ||
| let _ = Box::from_raw(owned_ptr); |
There was a problem hiding this comment.
When callers free a slice returned by ddog_span_debug_log, the pointer was produced by CString::into_raw in this same file, but this line reconstructs a Box<c_char> instead. Box::from_raw must be given a pointer that came from Box::into_raw for the same type/layout; using it for the CString allocation deallocates with a one-byte layout and can corrupt or abort on allocators that validate layout. This also means the shared free function still cannot safely handle both current owned producers without reconstructing the correct owner for each allocation kind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is true, but done knowingly. cf the PR description.
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Tests failed on this commit 4d693d7: What to do next?
|
CString::from_raw scans for NUL in MessagePack binary data from ddog_serialize_trace_into_charslice, causing buffer over-reads and heap corruption. Revert to the prior Box::from_raw deallocation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yannham
force-pushed
the
yannham/revert-free-charslice
branch
from
6c944b2 to
f19f327
Compare
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
yannham/revert-free-charslice
branch
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
4 participants
What does this PR do?
Fix potential UB.
Reverts
ddog_free_charslicefromCString::from_raw(introduced in #1698) back toBox::from_raw.Addresses 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 outCStringuse aBox<[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 byddog_free_charslicewere coming fromddog_span_debug_logexclusively. In this case, usingCString::from_rawto free is indeed correct.Unfortunately,
dd-trace-phpalso frees slice obtained fromddog_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,
CStringdoes useBox<[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.