From 9b51061907827e1f37a5ca48e145b87587119a78 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 25 Jan 2023 16:31:19 -0700 Subject: [PATCH 1/9] refactor(profiling)!: create FFI Error type This extracts an FFI Error type `ddog_Error`. It contains an FFI Vec internally: ```rust /// Please treat this as opaque; do not reach into it, and especially /// don't write into it! pub struct Error { /// This is a String stuffed into the vec. message: Vec, } ``` FFI result types have been updated e.g.: ```rust pub enum SendResult { HttpResponse(HttpStatus), Err(Error), } ``` Instead of reaching into the buffer, use these APIs: ```c /** * # Safety * Only pass null or a valid reference to an Error. */ void ddog_Error_drop(struct ddog_Error *error); /** * Returns a CharSlice of the error's message that is valid until the error * is dropped. * # Safety * Only pass null or a valid reference to an Error. */ ddog_CharSlice ddog_Error_message(const struct ddog_Error *error); ``` --- ddcommon-ffi/src/error.rs | 86 +++++++++++++++++++++++++++++++++++ ddcommon-ffi/src/lib.rs | 4 ++ ddcommon-ffi/src/tags.rs | 18 ++++---- examples/ffi/exporter.cpp | 5 +- examples/ffi/profiles.c | 3 +- profiling-ffi/cbindgen.toml | 1 + profiling-ffi/src/exporter.rs | 38 +++++++--------- profiling-ffi/src/profiles.rs | 42 ++++++++--------- 8 files changed, 143 insertions(+), 54 deletions(-) create mode 100644 ddcommon-ffi/src/error.rs diff --git a/ddcommon-ffi/src/error.rs b/ddcommon-ffi/src/error.rs new file mode 100644 index 000000000..90c2976a6 --- /dev/null +++ b/ddcommon-ffi/src/error.rs @@ -0,0 +1,86 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc. + +use crate::slice::CharSlice; +use crate::vec::Vec; +use std::fmt::{Display, Formatter}; + +/// Please treat this as opaque; do not reach into it, and especially don't +/// write into it! +#[derive(Debug)] +#[repr(C)] +pub struct Error { + /// This is a String stuffed into the vec. + message: Vec, +} + +impl AsRef for Error { + fn as_ref(&self) -> &str { + // Safety: .message is a String (just FFI safe). + unsafe { std::str::from_utf8_unchecked(self.message.as_slice().as_slice()) } + } +} + +impl Display for Error { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_ref()) + } +} + +impl std::error::Error for Error {} + +impl From for Error { + fn from(value: String) -> Self { + let message = Vec::from(value.into_bytes()); + Self { message } + } +} + +impl From for String { + fn from(value: Error) -> String { + // Safety: .message is a String (just FFI safe). + unsafe { String::from_utf8_unchecked(value.message.into()) } + } +} + +impl From<&str> for Error { + fn from(value: &str) -> Self { + Self::from(value.to_string()) + } +} + +impl From for Error { + fn from(value: anyhow::Error) -> Self { + Self::from(value.to_string()) + } +} + +impl From> for Error { + fn from(value: Box<&dyn std::error::Error>) -> Self { + Self::from(value.to_string()) + } +} + +/// # Safety +/// Only pass null or a valid reference to an Error. +#[no_mangle] +pub unsafe extern "C" fn ddog_Error_drop(error: Option<&mut Error>) { + if let Some(err) = error { + // Leave an empty String in place to help with double-free issues. + let mut tmp = Error::from(String::new()); + std::mem::swap(&mut tmp, err); + drop(tmp) + } +} + +/// Returns a CharSlice of the error's message that is valid until the error +/// is dropped. +/// # Safety +/// Only pass null or a valid reference to an Error. +#[no_mangle] +pub unsafe extern "C" fn ddog_Error_message(error: Option<&Error>) -> CharSlice { + match error { + None => CharSlice::default(), + Some(err) => CharSlice::from(err.as_ref()), + } +} diff --git a/ddcommon-ffi/src/lib.rs b/ddcommon-ffi/src/lib.rs index 3a62b83ad..0fb2a552f 100644 --- a/ddcommon-ffi/src/lib.rs +++ b/ddcommon-ffi/src/lib.rs @@ -1,11 +1,15 @@ // Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0. // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021-Present Datadog, Inc. +mod error; + pub mod option; pub mod slice; pub mod tags; pub mod vec; +pub use error::*; + pub use option::Option; pub use slice::{CharSlice, Slice}; pub use vec::Vec; diff --git a/ddcommon-ffi/src/tags.rs b/ddcommon-ffi/src/tags.rs index 96fb379b2..709c32ccf 100644 --- a/ddcommon-ffi/src/tags.rs +++ b/ddcommon-ffi/src/tags.rs @@ -1,7 +1,8 @@ // Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0. // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022-Present Datadog, Inc. -use crate::{slice::AsBytes, slice::CharSlice}; +use crate::slice::{AsBytes, CharSlice}; +use crate::Error; use ddcommon::tag::{parse_tags, Tag}; #[must_use] @@ -16,7 +17,7 @@ pub extern "C" fn ddog_Vec_Tag_drop(_: crate::Vec) {} #[repr(C)] pub enum PushTagResult { Ok, - Err(crate::Vec), + Err(Error), } #[no_mangle] @@ -44,14 +45,14 @@ pub unsafe extern "C" fn ddog_Vec_Tag_push( vec.push(tag); PushTagResult::Ok } - Err(err) => PushTagResult::Err(err.as_bytes().to_vec().into()), + Err(err) => PushTagResult::Err(Error::from(err.as_ref())), } } #[repr(C)] pub struct ParseTagsResult { tags: crate::Vec, - error_message: Option>>, + error_message: Option>, } /// # Safety @@ -64,7 +65,7 @@ pub unsafe extern "C" fn ddog_Vec_Tag_parse(string: CharSlice) -> ParseTagsResul let (tags, error) = parse_tags(string.as_ref()); ParseTagsResult { tags: tags.into(), - error_message: error.map(|message| Box::new(crate::Vec::from(message.into_bytes()))), + error_message: error.map(Error::from).map(Box::new), } } @@ -126,10 +127,11 @@ mod tests { // 'tags:' cannot end in a semi-colon, so expect an error. assert!(result.error_message.is_some()); - let error_message: Vec = (*result.error_message.unwrap()).into(); + let error = *result.error_message.unwrap(); + let error_message = error.as_ref(); assert!(!error_message.is_empty()); - let expected_error_message = b"Errors while parsing tags: tag 'tags:' ends with a colon"; - assert_eq!(expected_error_message, error_message.as_slice()) + let expected_error_message = "Errors while parsing tags: tag 'tags:' ends with a colon"; + assert_eq!(expected_error_message, error_message) } } diff --git a/examples/ffi/exporter.cpp b/examples/ffi/exporter.cpp index d156cd141..49bfbf04a 100644 --- a/examples/ffi/exporter.cpp +++ b/examples/ffi/exporter.cpp @@ -15,8 +15,9 @@ struct Deleter { void operator()(ddog_prof_Profile *object) { ddog_prof_Profile_drop(object); } }; -template void print_error(const char *s, const T &err) { - printf("%s (%.*s)\n", s, static_cast(err.len), err.ptr); +void print_error(const char *s, const ddog_Error &err) { + auto charslice = ddog_Error_message(&err); + printf("%s (%.*s)\n", s, static_cast(charslice.len), charslice.ptr); } int main(int argc, char *argv[]) { diff --git a/examples/ffi/profiles.c b/examples/ffi/profiles.c index a5c36649c..99fd941c1 100644 --- a/examples/ffi/profiles.c +++ b/examples/ffi/profiles.c @@ -48,7 +48,8 @@ int main(void) { ddog_prof_Profile_AddResult add_result = ddog_prof_Profile_add(profile, sample); if (add_result.tag != DDOG_PROF_PROFILE_ADD_RESULT_OK) { - fprintf(stderr, "%*s", (int)add_result.err.len, add_result.err.ptr); + ddog_CharSlice message = ddog_Error_message(&add_result.err); + fprintf(stderr, "%*s", (int)message.len, message.ptr); ddog_prof_Profile_AddResult_drop(add_result); } diff --git a/profiling-ffi/cbindgen.toml b/profiling-ffi/cbindgen.toml index 584f1fbc0..5f8144367 100644 --- a/profiling-ffi/cbindgen.toml +++ b/profiling-ffi/cbindgen.toml @@ -22,6 +22,7 @@ renaming_overrides_prefixing = true "CancellationToken" = "ddog_CancellationToken" "CharSlice" = "ddog_CharSlice" "Endpoint" = "ddog_Endpoint" +"Error" = "ddog_Error" "HttpStatus" = "ddog_HttpStatus" "Slice_CChar" = "ddog_Slice_CChar" "Slice_I64" = "ddog_Slice_I64" diff --git a/profiling-ffi/src/exporter.rs b/profiling-ffi/src/exporter.rs index f13ea3f5c..d7988fd0c 100644 --- a/profiling-ffi/src/exporter.rs +++ b/profiling-ffi/src/exporter.rs @@ -10,20 +10,28 @@ use datadog_profiling::exporter::{ProfileExporter, Request}; use datadog_profiling::profile::profiled_endpoints; use ddcommon::tag::Tag; use ddcommon_ffi::slice::{AsBytes, ByteSlice, CharSlice, Slice}; +use ddcommon_ffi::Error; use std::borrow::Cow; use std::ptr::NonNull; use std::str::FromStr; #[repr(C)] -pub enum SendResult { - HttpResponse(HttpStatus), - Err(ddcommon_ffi::Vec), +pub enum ExporterNewResult { + Ok(*mut ProfileExporter), + Err(Error), } #[repr(C)] -pub enum ExporterNewResult { - Ok(*mut ProfileExporter), - Err(ddcommon_ffi::Vec), +pub enum RequestBuildResult { + // This is a Box::into_raw pointer. + Ok(*mut Request), + Err(Error), +} + +#[repr(C)] +pub enum SendResult { + HttpResponse(HttpStatus), + Err(Error), } #[no_mangle] @@ -163,23 +171,13 @@ unsafe fn into_vec_files<'a>(slice: Slice<'a, File>) -> Vec> .collect() } -#[repr(C)] -pub enum RequestBuildResult { - // This is a Box::into_raw pointer. - Ok(*mut Request), - Err(ddcommon_ffi::Vec), -} - #[cfg(test)] impl From for Result, String> { fn from(result: RequestBuildResult) -> Self { match result { // Safety: Request is opaque, can only be built from Rust. RequestBuildResult::Ok(ok) => Ok(unsafe { Box::from_raw(ok) }), - RequestBuildResult::Err(err) => { - // Safety: not generally safe but this is for test code. - Err(unsafe { String::from_utf8_unchecked(err.into()) }) - } + RequestBuildResult::Err(err) => Err(String::from(err)), } } } @@ -254,16 +252,14 @@ pub unsafe extern "C" fn ddog_prof_Exporter_send( ) -> SendResult { // Re-box the request first, to avoid leaks on other errors. let request = if request.is_null() { - let buf: &[u8] = b"Failed to export: request was null"; - return SendResult::Err(ddcommon_ffi::Vec::from(Vec::from(buf))); + return SendResult::Err(Error::from("failed to export: request was null")); } else { Box::from_raw(request) }; let exp_ptr = match exporter { None => { - let buf: &[u8] = b"Failed to export: exporter was null"; - return SendResult::Err(ddcommon_ffi::Vec::from(Vec::from(buf))); + return SendResult::Err(Error::from("failed to export: exporter was null")); } Some(e) => e, }; diff --git a/profiling-ffi/src/profiles.rs b/profiling-ffi/src/profiles.rs index 8be6b735a..5fd70cc94 100644 --- a/profiling-ffi/src/profiles.rs +++ b/profiling-ffi/src/profiles.rs @@ -4,11 +4,30 @@ use crate::Timespec; use datadog_profiling::profile as profiles; use ddcommon_ffi::slice::{AsBytes, CharSlice, Slice}; +use ddcommon_ffi::Error; use profiles::profiled_endpoints; use std::convert::{TryFrom, TryInto}; use std::str::Utf8Error; use std::time::{Duration, SystemTime}; +#[repr(C)] +pub enum ProfileAddResult { + Ok(u64), + Err(Error), +} + +#[repr(C)] +pub enum SerializeResult { + Ok(EncodedProfile), + Err(Error), +} + +#[no_mangle] +pub extern "C" fn ddog_prof_Profile_AddResult_drop(_result: ProfileAddResult) {} + +#[no_mangle] +pub unsafe extern "C" fn ddog_prof_Profile_SerializeResult_drop(_result: SerializeResult) {} + #[repr(C)] #[derive(Copy, Clone)] pub struct ValueType<'a> { @@ -325,28 +344,16 @@ pub unsafe extern "C" fn ddog_prof_Profile_drop( ) { } -#[repr(C)] -pub enum ProfileAddResult { - Ok(u64), - Err(ddcommon_ffi::Vec), -} - #[cfg(test)] impl From for Result { fn from(result: ProfileAddResult) -> Self { match result { ProfileAddResult::Ok(ok) => Ok(ok), - ProfileAddResult::Err(err) => { - // Safety: not generally safe but this is for test code. - Err(unsafe { String::from_utf8_unchecked(err.into()) }) - } + ProfileAddResult::Err(err) => Err(err.into()), } } } -#[no_mangle] -pub extern "C" fn ddog_prof_Profile_AddResult_drop(_result: ProfileAddResult) {} - /// # Safety /// The `profile` ptr must point to a valid Profile object created by this /// module. All pointers inside the `sample` need to be valid for the duration @@ -444,12 +451,6 @@ impl From for EncodedProfile { } } -#[repr(C)] -pub enum SerializeResult { - Ok(EncodedProfile), - Err(ddcommon_ffi::Vec), -} - /// Serialize the aggregated profile. Don't forget to clean up the result by /// calling ddog_prof_Profile_SerializeResult_drop. /// @@ -486,9 +487,6 @@ pub unsafe extern "C" fn ddog_prof_Profile_serialize( } } -#[no_mangle] -pub unsafe extern "C" fn ddog_prof_Profile_SerializeResult_drop(_result: SerializeResult) {} - #[must_use] #[no_mangle] pub unsafe extern "C" fn ddog_Vec_U8_as_slice(vec: &ddcommon_ffi::Vec) -> Slice { From 9c579831e4c8c032b60886ae3f3c674655cfe31f Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 25 Jan 2023 16:37:43 -0700 Subject: [PATCH 2/9] Use more precise language --- ddcommon-ffi/src/error.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ddcommon-ffi/src/error.rs b/ddcommon-ffi/src/error.rs index 90c2976a6..bcc8e783c 100644 --- a/ddcommon-ffi/src/error.rs +++ b/ddcommon-ffi/src/error.rs @@ -62,7 +62,7 @@ impl From> for Error { } /// # Safety -/// Only pass null or a valid reference to an Error. +/// Only pass null or a valid reference to a `ddog_Error`. #[no_mangle] pub unsafe extern "C" fn ddog_Error_drop(error: Option<&mut Error>) { if let Some(err) = error { @@ -76,7 +76,7 @@ pub unsafe extern "C" fn ddog_Error_drop(error: Option<&mut Error>) { /// Returns a CharSlice of the error's message that is valid until the error /// is dropped. /// # Safety -/// Only pass null or a valid reference to an Error. +/// Only pass null or a valid reference to a `ddog_Error`. #[no_mangle] pub unsafe extern "C" fn ddog_Error_message(error: Option<&Error>) -> CharSlice { match error { From 0b7899bea57c2799ed3e4b4ae2c6d02f70c95e26 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 25 Jan 2023 17:12:08 -0700 Subject: [PATCH 3/9] Drop *Result_drop functions I think API usage is more likely to be correct when each case is dropped as needed. This shows the diff for our examples. --- ddcommon-ffi/src/tags.rs | 3 --- examples/ffi/exporter.cpp | 16 +++++++--------- examples/ffi/profiles.c | 2 +- profiling-ffi/src/exporter.rs | 30 +++--------------------------- profiling-ffi/src/profiles.rs | 6 ------ 5 files changed, 11 insertions(+), 46 deletions(-) diff --git a/ddcommon-ffi/src/tags.rs b/ddcommon-ffi/src/tags.rs index 709c32ccf..07e73821c 100644 --- a/ddcommon-ffi/src/tags.rs +++ b/ddcommon-ffi/src/tags.rs @@ -20,9 +20,6 @@ pub enum PushTagResult { Err(Error), } -#[no_mangle] -pub extern "C" fn ddog_Vec_Tag_PushResult_drop(_: PushTagResult) {} - /// Creates a new Tag from the provided `key` and `value` by doing a utf8 /// lossy conversion, and pushes into the `vec`. The strings `key` and `value` /// are cloned to avoid FFI lifetime issues. diff --git a/examples/ffi/exporter.cpp b/examples/ffi/exporter.cpp index 49bfbf04a..1febefb22 100644 --- a/examples/ffi/exporter.cpp +++ b/examples/ffi/exporter.cpp @@ -70,14 +70,14 @@ int main(int argc, char *argv[]) { auto add_result = ddog_prof_Profile_add(profile.get(), sample); if (add_result.tag != DDOG_PROF_PROFILE_ADD_RESULT_OK) { print_error("Failed to add sample to profile: ", add_result.err); - ddog_prof_Profile_AddResult_drop(add_result); + ddog_Error_drop(&add_result.err); return 1; } ddog_prof_Profile_SerializeResult serialize_result = ddog_prof_Profile_serialize(profile.get(), nullptr, nullptr); if (serialize_result.tag == DDOG_PROF_PROFILE_SERIALIZE_RESULT_ERR) { print_error("Failed to serialize profile: ", serialize_result.err); - ddog_prof_Profile_SerializeResult_drop(serialize_result); + ddog_Error_drop(&serialize_result.err); return 1; } @@ -91,12 +91,10 @@ int main(int argc, char *argv[]) { ddog_Vec_Tag_push(&tags, DDOG_CHARSLICE_C("service"), to_slice_c_char(service)); if (tag_result.tag == DDOG_VEC_TAG_PUSH_RESULT_ERR) { print_error("Failed to push tag: ", tag_result.err); - ddog_Vec_Tag_PushResult_drop(tag_result); + ddog_Error_drop(&tag_result.err); return 1; } - ddog_Vec_Tag_PushResult_drop(tag_result); - ddog_prof_Exporter_NewResult exporter_new_result = ddog_prof_Exporter_new( DDOG_CHARSLICE_C("exporter-example"), DDOG_CHARSLICE_C("1.2.3"), @@ -108,7 +106,7 @@ int main(int argc, char *argv[]) { if (exporter_new_result.tag == DDOG_PROF_EXPORTER_NEW_RESULT_ERR) { print_error("Failed to create exporter: ", exporter_new_result.err); - ddog_prof_Exporter_NewResult_drop(exporter_new_result); + ddog_Error_drop(&exporter_new_result.err); return 1; } @@ -133,7 +131,7 @@ int main(int argc, char *argv[]) { if (build_result.tag == DDOG_PROF_EXPORTER_REQUEST_BUILD_RESULT_ERR) { print_error("Failed to build request: ", build_result.err); - ddog_prof_Exporter_Request_BuildResult_drop(build_result); + ddog_Error_drop(&build_result.err); return 1; } @@ -165,12 +163,12 @@ int main(int argc, char *argv[]) { if (send_result.tag == DDOG_PROF_EXPORTER_SEND_RESULT_ERR) { print_error("Failed to send profile: ", send_result.err); exit_code = 1; + ddog_Error_drop(&send_result.err); } else { printf("Response code: %d\n", send_result.http_response.code); } - ddog_prof_Exporter_NewResult_drop(exporter_new_result); - ddog_prof_Exporter_SendResult_drop(send_result); + ddog_prof_Exporter_drop(exporter); ddog_CancellationToken_drop(cancel); return exit_code; } diff --git a/examples/ffi/profiles.c b/examples/ffi/profiles.c index 99fd941c1..a8476896f 100644 --- a/examples/ffi/profiles.c +++ b/examples/ffi/profiles.c @@ -50,7 +50,7 @@ int main(void) { if (add_result.tag != DDOG_PROF_PROFILE_ADD_RESULT_OK) { ddog_CharSlice message = ddog_Error_message(&add_result.err); fprintf(stderr, "%*s", (int)message.len, message.ptr); - ddog_prof_Profile_AddResult_drop(add_result); + ddog_Error_drop(&add_result.err); } diff --git a/profiling-ffi/src/exporter.rs b/profiling-ffi/src/exporter.rs index d7988fd0c..90aa82708 100644 --- a/profiling-ffi/src/exporter.rs +++ b/profiling-ffi/src/exporter.rs @@ -34,17 +34,6 @@ pub enum SendResult { Err(Error), } -#[no_mangle] -pub unsafe extern "C" fn ddog_prof_Exporter_NewResult_drop(result: ExporterNewResult) { - match result { - ExporterNewResult::Ok(ptr) => { - let exporter = Box::from_raw(ptr); - drop(exporter) - } - ExporterNewResult::Err(message) => drop(message), - } -} - #[repr(C)] pub enum Endpoint<'a> { Agent(CharSlice<'a>), @@ -182,19 +171,12 @@ impl From for Result, String> { } } -/// Drops the result. Since `ddog_prof_Exporter_send` will take ownership of -/// the Request object, do not call this method if you call -/// `ddog_prof_Exporter_send` on the `ok` value! -#[no_mangle] -pub extern "C" fn ddog_prof_Exporter_Request_BuildResult_drop(_result: RequestBuildResult) {} - /// If successful, builds a `ddog_prof_Exporter_Request` object based on the profile data supplied. /// If unsuccessful, it returns an error message. /// /// The main use of the `ddog_prof_Exporter_Request` object is to be used in /// `ddog_prof_Exporter_send`, which will take ownership of the object. Do not pass it to -/// `ddog_prof_Exporter_Request_drop` in such cases, nor call -/// `ddog_prof_Exporter_Request_BuildResult_drop` on the result! +/// `ddog_prof_Exporter_Request_drop` after that! /// /// # Safety /// The `exporter` and the files inside of the `files` slice need to have been @@ -236,9 +218,8 @@ pub unsafe extern "C" fn ddog_prof_Exporter_Request_build( /// /// # Arguments /// * `exporter` - Borrows the exporter for sending the request. -/// * `request` - Takes ownership of the request. Do not call `ddog_prof_Request_drop` nor -/// `ddog_prof_Exporter_SendResult_drop` on `request` after calling -/// `ddog_prof_Exporter_send` on it. +/// * `request` - Takes ownership of the request. Do not call `ddog_prof_Request_drop` on +/// `request` after calling `ddog_prof_Exporter_send` on it. /// * `cancel` - Borrows the cancel, if any. /// /// # Safety @@ -353,11 +334,6 @@ pub extern "C" fn ddog_CancellationToken_drop(_cancel: Option { From f36696f5ae01eb325d88889b0f4489aad98438ce Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 26 Jan 2023 10:08:19 -0700 Subject: [PATCH 4/9] Make a note about #[must_use] --- profiling-ffi/README.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 profiling-ffi/README.md diff --git a/profiling-ffi/README.md b/profiling-ffi/README.md new file mode 100644 index 000000000..af0a22f04 --- /dev/null +++ b/profiling-ffi/README.md @@ -0,0 +1,28 @@ +# Datadog Profiling FFI Notes + +## \#[must_use] on functions + +Many FFI functions should use `#[must_use]`. As an example, there are many +Result types which need to be used for correctness reasons: + +```rust +#[repr(C)] +pub enum ProfileAddResult { + Ok(u64), + Err(Error), +} +``` + +Then on `ddog_prof_Profile_add` which returns a `ProfileAddResult`, there is a +`#[must_use]`. If the C user of this API doesn't touch the return value, then +they'll get a warning, something like: + +> warning: ignoring return value of function declared with +> 'warn_unused_result' attribute [-Wunused-result] + +Additionally, many types (including Error types) have memory that needs to +be dropped. If the user doesn't use the result, then they definitely leak. + +It would be nice if we could put `#[must_use]` directly on the type, rather +than on the functions which return them. At the moment, cbindgen doesn't +handle this case, so we have to put `#[must_use]` on functions. From e130a2c7a78131666cc23937782b2278bfce3d91 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 26 Jan 2023 10:28:56 -0700 Subject: [PATCH 5/9] Add ddog_prof_EncodedProfile_drop --- examples/ffi/exporter.cpp | 1 + profiling-ffi/src/profiles.rs | 10 ++++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/examples/ffi/exporter.cpp b/examples/ffi/exporter.cpp index 1febefb22..a8a175e8b 100644 --- a/examples/ffi/exporter.cpp +++ b/examples/ffi/exporter.cpp @@ -128,6 +128,7 @@ int main(int argc, char *argv[]) { nullptr, 30000 ); + ddog_prof_EncodedProfile_drop(encoded_profile); if (build_result.tag == DDOG_PROF_EXPORTER_REQUEST_BUILD_RESULT_ERR) { print_error("Failed to build request: ", build_result.err); diff --git a/profiling-ffi/src/profiles.rs b/profiling-ffi/src/profiles.rs index 0a48ce20a..46fcdb5c5 100644 --- a/profiling-ffi/src/profiles.rs +++ b/profiling-ffi/src/profiles.rs @@ -429,6 +429,11 @@ pub struct EncodedProfile { endpoints_stats: Box, } +#[no_mangle] +pub extern "C" fn ddog_prof_EncodedProfile_drop(profile: EncodedProfile) { + drop(profile) +} + impl From for EncodedProfile { fn from(value: datadog_profiling::profile::EncodedProfile) -> Self { let start = value.start.into(); @@ -445,8 +450,9 @@ impl From for EncodedProfile { } } -/// Serialize the aggregated profile. Don't forget to clean up the result by -/// calling ddog_prof_Profile_SerializeResult_drop. +/// Serialize the aggregated profile. +/// +/// Don't forget to clean up the ok or error variant once you are done with it! /// /// # Arguments /// * `profile` - a reference to the profile being serialized. From 15626b914c3da334182e61d51f43ec0c25178d5f Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 26 Jan 2023 10:46:20 -0700 Subject: [PATCH 6/9] take reference for ddog_prof_EncodedProfile_drop --- profiling-ffi/src/profiles.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/profiling-ffi/src/profiles.rs b/profiling-ffi/src/profiles.rs index 46fcdb5c5..fd99f45e0 100644 --- a/profiling-ffi/src/profiles.rs +++ b/profiling-ffi/src/profiles.rs @@ -429,9 +429,16 @@ pub struct EncodedProfile { endpoints_stats: Box, } +/// Only pass a reference to a valid `ddog_prof_EncodedProfile`, or null. A +/// valid reference also means that it hasn't already been dropped (do not +/// call this twice on the same object); #[no_mangle] -pub extern "C" fn ddog_prof_EncodedProfile_drop(profile: EncodedProfile) { - drop(profile) +pub unsafe extern "C" fn ddog_prof_EncodedProfile_drop(profile: Option<&mut EncodedProfile>) { + if let Some(profile) = profile { + let ptr = profile as *mut _; + // Programmer's responsibility to protect this from being double-free. + std::ptr::drop_in_place(ptr) + } } impl From for EncodedProfile { @@ -452,7 +459,8 @@ impl From for EncodedProfile { /// Serialize the aggregated profile. /// -/// Don't forget to clean up the ok or error variant once you are done with it! +/// Don't forget to clean up the ok with `ddog_prof_EncodedProfile_drop` or +/// the error variant with `ddog_Error_drop` when you are done with them. /// /// # Arguments /// * `profile` - a reference to the profile being serialized. From 05555e4d37297206513940cb0811c527bc151d6e Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 27 Jan 2023 09:42:51 -0700 Subject: [PATCH 7/9] add missing ddog_prof_Exporter_Request_drop, fix docs --- profiling-ffi/src/exporter.rs | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/profiling-ffi/src/exporter.rs b/profiling-ffi/src/exporter.rs index 90aa82708..9356869b7 100644 --- a/profiling-ffi/src/exporter.rs +++ b/profiling-ffi/src/exporter.rs @@ -171,16 +171,16 @@ impl From for Result, String> { } } -/// If successful, builds a `ddog_prof_Exporter_Request` object based on the profile data supplied. -/// If unsuccessful, it returns an error message. +/// If successful, builds a `ddog_prof_Exporter_Request` object based on the +/// profile data supplied. If unsuccessful, it returns an error message. /// /// The main use of the `ddog_prof_Exporter_Request` object is to be used in -/// `ddog_prof_Exporter_send`, which will take ownership of the object. Do not pass it to -/// `ddog_prof_Exporter_Request_drop` after that! +/// `ddog_prof_Exporter_send`, which will take ownership of the object. Do not +/// pass it to `ddog_prof_Exporter_Request_drop` after that! /// /// # Safety -/// The `exporter` and the files inside of the `files` slice need to have been -/// created by this module. +/// The `exporter`, `additional_stats`, and `endpoint_stats` args should be +/// valid objects created by this module, except NULL is allowed. #[no_mangle] #[must_use] pub unsafe extern "C" fn ddog_prof_Exporter_Request_build( @@ -214,12 +214,24 @@ pub unsafe extern "C" fn ddog_prof_Exporter_Request_build( } } +/// # Safety +/// The `request` must be valid, or null. Notably, it is invalid if it's been +/// sent to `ddog_prof_Exporter_send` or `ddog_prof_Exporter_Request_drop` +/// already. +#[no_mangle] +pub unsafe extern "C" fn ddog_prof_Exporter_Request_drop(request: *mut Request) { + if !request.is_null() { + drop(Box::from_raw(request)); + } +} + /// Sends the request, returning the HttpStatus. /// /// # Arguments /// * `exporter` - Borrows the exporter for sending the request. -/// * `request` - Takes ownership of the request. Do not call `ddog_prof_Request_drop` on -/// `request` after calling `ddog_prof_Exporter_send` on it. +/// * `request` - Takes ownership of the request. Do not call +/// `ddog_prof_Exporter_Request` on `request` after calling +/// `ddog_prof_Exporter_send` on it. /// * `cancel` - Borrows the cancel, if any. /// /// # Safety From ccaceb2e5c7dc0fb72b0b5ffe5a028c42424d295 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 27 Jan 2023 09:56:59 -0700 Subject: [PATCH 8/9] [ci skip] adjust name doc comment --- profiling-ffi/src/exporter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiling-ffi/src/exporter.rs b/profiling-ffi/src/exporter.rs index 9356869b7..ef038bcb1 100644 --- a/profiling-ffi/src/exporter.rs +++ b/profiling-ffi/src/exporter.rs @@ -230,7 +230,7 @@ pub unsafe extern "C" fn ddog_prof_Exporter_Request_drop(request: *mut Request) /// # Arguments /// * `exporter` - Borrows the exporter for sending the request. /// * `request` - Takes ownership of the request. Do not call -/// `ddog_prof_Exporter_Request` on `request` after calling +/// `ddog_prof_Exporter_Request_drop` on `request` after calling /// `ddog_prof_Exporter_send` on it. /// * `cancel` - Borrows the cancel, if any. /// From f47f2a062d88855957c3fcb1515ea2ad7888c36f Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Fri, 27 Jan 2023 10:22:56 -0700 Subject: [PATCH 9/9] write note about avoiding ddog_prof_Exporter_Request_drop --- examples/ffi/exporter.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/examples/ffi/exporter.cpp b/examples/ffi/exporter.cpp index a8a175e8b..d11019842 100644 --- a/examples/ffi/exporter.cpp +++ b/examples/ffi/exporter.cpp @@ -169,6 +169,8 @@ int main(int argc, char *argv[]) { printf("Response code: %d\n", send_result.http_response.code); } + // no ddog_prof_Exporter_Request_drop(request) since it was sent to Exporter_send + ddog_prof_Exporter_drop(exporter); ddog_CancellationToken_drop(cancel); return exit_code;