Merge #659

659: Make export::acquire() and export::release() unsafe r=Urhengulas a=jannic

Calling export::release() may enable interrupts, which is unsound
if done inside a critical section or other code which expects interrupts
to be disabled.

Calling export::acquire() is less dangerous (no obvious way to cause
unsoundness), but is still a bad idea and breaks the safety contract
of critical_section::acquire(), which must only be called paired with
critical_section::release().


Co-authored-by: Jan Niehusmann <jan@gondor.com>

Author: bors[bot]

CHANGELOG.md

@@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ## [Unreleased]
 
 - [#640]: use crate [critical-section](https://crates.io/crates/critical-section) in defmt-rtt
+- [#659]: mark extern::acquire() and extern::release() as unsafe
+
+[#640]: https://github.com/knurling-rs/defmt/pull/640
+[#659]: https://github.com/knurling-rs/defmt/pull/659
 
 ## [v0.3.1] - 2021-11-26
 

defmt/src/export/mod.rs

@@ -39,28 +39,36 @@ pub fn fetch_bytes() -> Vec<u8> {
     BYTES.with(|b| core::mem::replace(&mut *b.borrow_mut(), Vec::new()))
 }
 
+/// Only to be used by the defmt macros
+/// Safety: must be paired with a later call to release()
 #[cfg(feature = "unstable-test")]
-pub fn acquire() {}
+pub unsafe fn acquire() {}
 
+/// Only to be used by the defmt macros
+/// Safety: must be paired with a later call to release()
 #[cfg(not(feature = "unstable-test"))]
 #[inline(always)]
-pub fn acquire() {
+pub unsafe fn acquire() {
     extern "Rust" {
         fn _defmt_acquire();
     }
-    unsafe { _defmt_acquire() }
+    _defmt_acquire()
 }
 
+/// Only to be used by the defmt macros
+/// Safety: must follow an earlier call to acquire()
 #[cfg(feature = "unstable-test")]
-pub fn release() {}
+pub unsafe fn release() {}
 
+/// Only to be used by the defmt macros
+/// Safety: must follow an earlier call to acquire()
 #[cfg(not(feature = "unstable-test"))]
 #[inline(always)]
-pub fn release() {
+pub unsafe fn release() {
     extern "Rust" {
         fn _defmt_release();
     }
-    unsafe { _defmt_release() }
+    _defmt_release()
 }
 
 #[cfg(feature = "unstable-test")]

firmware/defmt-rtt/src/lib.rs

@@ -42,6 +42,7 @@ static mut ENCODER: defmt::Encoder = defmt::Encoder::new();
 
 unsafe impl defmt::Logger for Logger {
     fn acquire() {
+        // safety: Must be paired with corresponding call to release(), see below
         let token = unsafe { critical_section::acquire() };
 
         if TAKEN.load(Ordering::Relaxed) {
@@ -67,6 +68,7 @@ unsafe impl defmt::Logger for Logger {
         ENCODER.end_frame(do_write);
 
         TAKEN.store(false, Ordering::Relaxed);
+        // safety: Must be paired with corresponding call to acquire(), see above
         critical_section::release(INTERRUPTS_ACTIVE.load(Ordering::Relaxed));
     }
 

macros/src/function_like/log.rs

@@ -44,10 +44,12 @@ pub(crate) fn expand_parsed(level: Level, args: Args) -> TokenStream2 {
             match (#(&(#formatting_exprs)),*) {
                 (#(#patterns),*) => {
                     if #filter_check {
-                        defmt::export::acquire();
+                        // safety: will be released a few lines further down
+                        unsafe { defmt::export::acquire() };
                         defmt::export::header(&#header);
                         #(#exprs;)*
-                        defmt::export::release()
+                        // safety: acquire() was called a few lines above
+                        unsafe { defmt::export::release() }
                     }
                 }
             }

macros/src/function_like/println.rs

@@ -34,10 +34,12 @@ pub(crate) fn expand_parsed(args: Args) -> TokenStream2 {
     quote!({
         match (#(&(#formatting_exprs)),*) {
             (#(#patterns),*) => {
-                defmt::export::acquire();
+                // safety: will be released a few lines further down
+                unsafe { defmt::export::acquire(); }
                 defmt::export::header(&#header);
                 #(#exprs;)*
-                defmt::export::release()
+                // safety: acquire() was called a few lines above
+                unsafe { defmt::export::release() }
             }
         }
     })