diff --git a/ddcommon-ffi/src/error.rs b/ddcommon-ffi/src/error.rs new file mode 100644 index 000000000..bcc8e783c --- /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 a `ddog_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 a `ddog_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..07e73821c 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,12 +17,9 @@ pub extern "C" fn ddog_Vec_Tag_drop(_: crate::Vec) {} #[repr(C)] pub enum PushTagResult { Ok, - Err(crate::Vec), + 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. @@ -44,14 +42,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 +62,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 +124,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..d11019842 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[]) { @@ -69,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; } @@ -90,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"), @@ -107,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; } @@ -129,10 +128,11 @@ 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); - ddog_prof_Exporter_Request_BuildResult_drop(build_result); + ddog_Error_drop(&build_result.err); return 1; } @@ -164,12 +164,14 @@ 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); + // 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; } diff --git a/examples/ffi/profiles.c b/examples/ffi/profiles.c index a5c36649c..a8476896f 100644 --- a/examples/ffi/profiles.c +++ b/examples/ffi/profiles.c @@ -48,8 +48,9 @@ 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_prof_Profile_AddResult_drop(add_result); + ddog_CharSlice message = ddog_Error_message(&add_result.err); + fprintf(stderr, "%*s", (int)message.len, message.ptr); + ddog_Error_drop(&add_result.err); } 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. 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..ef038bcb1 100644 --- a/profiling-ffi/src/exporter.rs +++ b/profiling-ffi/src/exporter.rs @@ -10,31 +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), } -#[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 SendResult { + HttpResponse(HttpStatus), + Err(Error), } #[repr(C)] @@ -163,44 +160,27 @@ 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)), } } } -/// 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. +/// 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_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( @@ -234,12 +214,23 @@ 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` nor -/// `ddog_prof_Exporter_SendResult_drop` on `request` after calling +/// * `request` - Takes ownership of the request. Do not call +/// `ddog_prof_Exporter_Request_drop` on `request` after calling /// `ddog_prof_Exporter_send` on it. /// * `cancel` - Borrows the cancel, if any. /// @@ -254,16 +245,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, }; @@ -357,11 +346,6 @@ pub extern "C" fn ddog_CancellationToken_drop(_cancel: Option { @@ -325,28 +338,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 @@ -428,6 +429,18 @@ 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 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 { fn from(value: datadog_profiling::profile::EncodedProfile) -> Self { let start = value.start.into(); @@ -444,14 +457,10 @@ 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. +/// Serialize the aggregated profile. +/// +/// 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. @@ -486,9 +495,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 {