From 52fc8d03476b946ad754d893c56366ebe075ad39 Mon Sep 17 00:00:00 2001 From: Venus Xeon-Blonde Date: Mon, 22 Jul 2024 15:26:24 -0400 Subject: [PATCH 1/2] (Breaking Changes) Re-implement tracing wrapper in a safer way, add smoke test, update tracing callback to use `&[u8]` instead of `&str`. --- src/tracing.rs | 96 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 75 insertions(+), 21 deletions(-) diff --git a/src/tracing.rs b/src/tracing.rs index 5acae8a850..6ff671d951 100644 --- a/src/tracing.rs +++ b/src/tracing.rs @@ -1,8 +1,8 @@ -use std::sync::atomic::{AtomicUsize, Ordering}; +use std::{ffi::CStr, sync::atomic::{AtomicPtr, Ordering}}; -use libc::c_char; +use libc::{c_char, c_int}; -use crate::{panic, raw, util::Binding}; +use crate::{panic, raw, util::Binding, Error}; /// Available tracing levels. When tracing is set to a particular level, /// callers will be provided tracing at the given level and all lower levels. @@ -57,29 +57,83 @@ impl Binding for TraceLevel { } } -//TODO: pass raw &[u8] and leave conversion to consumer (breaking API) /// Callback type used to pass tracing events to the subscriber. /// see `trace_set` to register a subscriber. -pub type TracingCb = fn(TraceLevel, &str); - -static CALLBACK: AtomicUsize = AtomicUsize::new(0); - -/// -pub fn trace_set(level: TraceLevel, cb: TracingCb) -> bool { - CALLBACK.store(cb as usize, Ordering::SeqCst); - - unsafe { - raw::git_trace_set(level.raw(), Some(tracing_cb_c)); +pub type TracingCb = fn(TraceLevel, &[u8]); + +/// Use an atomic pointer to store the global tracing subscriber function. +static CALLBACK: AtomicPtr<()> = AtomicPtr::new(std::ptr::null_mut()); + +/// Set the global subscriber called when libgit2 produces a tracing message. +pub fn trace_set(level: TraceLevel, cb: TracingCb) -> Result<(), Error> { + // Store the callback in the global atomic. + CALLBACK.store(cb as *mut (), Ordering::SeqCst); + + // git_trace_set returns 0 if there was no error. + let return_code: c_int = unsafe { + raw::git_trace_set(level.raw(), Some(tracing_cb_c)) + }; + + if return_code != 0 { + // Unwrap here is fine since `Error::last_error` always returns `Some`. + Err(Error::last_error(return_code).unwrap()) + } else { + Ok(()) } - - return true; } +/// The tracing callback we pass to libgit2 (C ABI compatible). extern "C" fn tracing_cb_c(level: raw::git_trace_level_t, msg: *const c_char) { - let cb = CALLBACK.load(Ordering::SeqCst); - panic::wrap(|| unsafe { - let cb: TracingCb = std::mem::transmute(cb); - let msg = std::ffi::CStr::from_ptr(msg).to_string_lossy(); - cb(Binding::from_raw(level), msg.as_ref()); + // Load the callback function pointer from the global atomic. + let cb: *mut () = CALLBACK.load(Ordering::SeqCst); + + // Transmute the callback pointer into the function pointer we know it to be. + // + // SAFETY: We only ever set the callback pointer with something cast from a TracingCb + // so transmuting back to a TracingCb is safe. This is notably not an integer-to-pointer + // transmute as described in the mem::transmute documentation and is in-line with the + // example in that documentation for casing between *const () to fn pointers. + let cb: TracingCb = unsafe { std::mem::transmute(cb) }; + + // If libgit2 passes us a message that is null, drop it and do not pass it to the callback. + // This is to avoid ever exposing rust code to a null ref, which would be Undefined Behavior. + if msg.is_null() { + return; + } + + // Convert the message from a *const c_char to a &[u8] and pass it to the callback. + // + // SAFETY: We've just checked that the pointer is not null. The other safety requirements are left to + // libgit2 to enforce -- namely that it gives us a valid, nul-terminated, C string, that that string exists + // entirely in one allocation, that the string will not be mutated once passed to us, and that the nul-terminator is + // within isize::MAX bytes from the given pointers data address. + let msg: &CStr = unsafe { CStr::from_ptr(msg) }; + + // Convert from a CStr to &[u8] to pass to the rust code callback. + let msg: &[u8] = CStr::to_bytes(msg); + + // Do the remaining part of this function in a panic wrapper, to catch any panics it produces. + panic::wrap(|| { + // Convert the raw trace level into a type we can pass to the rust callback fn. + // + // SAFETY: Currently the implementation of this function (above) may panic, but is only marked as unsafe to match + // the trait definition, thus we can consider this call safe. + let level: TraceLevel = unsafe { Binding::from_raw(level) }; + + // Call the user-supplied callback (which may panic). + (cb)(level, msg); }); } + +#[cfg(test)] +mod tests { + use super::TraceLevel; + + // Test that using the above function to set a tracing callback doesn't panic. + #[test] + fn smoke() { + super::trace_set(TraceLevel::Trace, |level, msg| { + dbg!(level, msg); + }).expect("libgit2 can set global trace callback"); + } +} From 7be752595440748152ced84363773c9c5c207960 Mon Sep 17 00:00:00 2001 From: Venus Xeon-Blonde Date: Mon, 22 Jul 2024 15:34:31 -0400 Subject: [PATCH 2/2] Cargo fmt --- src/tracing.rs | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/tracing.rs b/src/tracing.rs index 6ff671d951..f06460d240 100644 --- a/src/tracing.rs +++ b/src/tracing.rs @@ -1,4 +1,7 @@ -use std::{ffi::CStr, sync::atomic::{AtomicPtr, Ordering}}; +use std::{ + ffi::CStr, + sync::atomic::{AtomicPtr, Ordering}, +}; use libc::{c_char, c_int}; @@ -61,7 +64,7 @@ impl Binding for TraceLevel { /// see `trace_set` to register a subscriber. pub type TracingCb = fn(TraceLevel, &[u8]); -/// Use an atomic pointer to store the global tracing subscriber function. +/// Use an atomic pointer to store the global tracing subscriber function. static CALLBACK: AtomicPtr<()> = AtomicPtr::new(std::ptr::null_mut()); /// Set the global subscriber called when libgit2 produces a tracing message. @@ -69,10 +72,8 @@ pub fn trace_set(level: TraceLevel, cb: TracingCb) -> Result<(), Error> { // Store the callback in the global atomic. CALLBACK.store(cb as *mut (), Ordering::SeqCst); - // git_trace_set returns 0 if there was no error. - let return_code: c_int = unsafe { - raw::git_trace_set(level.raw(), Some(tracing_cb_c)) - }; + // git_trace_set returns 0 if there was no error. + let return_code: c_int = unsafe { raw::git_trace_set(level.raw(), Some(tracing_cb_c)) }; if return_code != 0 { // Unwrap here is fine since `Error::last_error` always returns `Some`. @@ -88,10 +89,10 @@ extern "C" fn tracing_cb_c(level: raw::git_trace_level_t, msg: *const c_char) { let cb: *mut () = CALLBACK.load(Ordering::SeqCst); // Transmute the callback pointer into the function pointer we know it to be. - // - // SAFETY: We only ever set the callback pointer with something cast from a TracingCb - // so transmuting back to a TracingCb is safe. This is notably not an integer-to-pointer - // transmute as described in the mem::transmute documentation and is in-line with the + // + // SAFETY: We only ever set the callback pointer with something cast from a TracingCb + // so transmuting back to a TracingCb is safe. This is notably not an integer-to-pointer + // transmute as described in the mem::transmute documentation and is in-line with the // example in that documentation for casing between *const () to fn pointers. let cb: TracingCb = unsafe { std::mem::transmute(cb) }; @@ -102,10 +103,10 @@ extern "C" fn tracing_cb_c(level: raw::git_trace_level_t, msg: *const c_char) { } // Convert the message from a *const c_char to a &[u8] and pass it to the callback. - // - // SAFETY: We've just checked that the pointer is not null. The other safety requirements are left to + // + // SAFETY: We've just checked that the pointer is not null. The other safety requirements are left to // libgit2 to enforce -- namely that it gives us a valid, nul-terminated, C string, that that string exists - // entirely in one allocation, that the string will not be mutated once passed to us, and that the nul-terminator is + // entirely in one allocation, that the string will not be mutated once passed to us, and that the nul-terminator is // within isize::MAX bytes from the given pointers data address. let msg: &CStr = unsafe { CStr::from_ptr(msg) }; @@ -116,7 +117,7 @@ extern "C" fn tracing_cb_c(level: raw::git_trace_level_t, msg: *const c_char) { panic::wrap(|| { // Convert the raw trace level into a type we can pass to the rust callback fn. // - // SAFETY: Currently the implementation of this function (above) may panic, but is only marked as unsafe to match + // SAFETY: Currently the implementation of this function (above) may panic, but is only marked as unsafe to match // the trait definition, thus we can consider this call safe. let level: TraceLevel = unsafe { Binding::from_raw(level) }; @@ -134,6 +135,7 @@ mod tests { fn smoke() { super::trace_set(TraceLevel::Trace, |level, msg| { dbg!(level, msg); - }).expect("libgit2 can set global trace callback"); + }) + .expect("libgit2 can set global trace callback"); } }