From 82ccdab96cd8c146d9958c1050a2d2669dd367e8 Mon Sep 17 00:00:00 2001
From: Alexis Bourget <alexis.bourget@gmail.com>
Date: Wed, 29 Jul 2020 21:58:09 +0200
Subject: [PATCH] Disallow missing unsafe blocks in unsafe fn in panicking.rs

This adds SAFETY comments where necessary, explaining the preconditions
and how they are respected.
---
 library/std/src/panicking.rs    | 50 +++++++++++++++++++++++++++++----
 library/std/src/thread/local.rs |  6 +++-
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/library/std/src/panicking.rs b/library/std/src/panicking.rs
index 9542e7209b4..f92c1dd76a1 100644
--- a/library/std/src/panicking.rs
+++ b/library/std/src/panicking.rs
@@ -7,6 +7,8 @@
 //! * Executing a panic up to doing the actual implementation
 //! * Shims around "try"
 
+#![deny(unsafe_op_in_unsafe_fn)]
+
 use core::panic::{BoxMeUp, Location, PanicInfo};
 
 use crate::any::Any;
@@ -322,11 +324,22 @@ pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>>
     let mut data = Data { f: ManuallyDrop::new(f) };
 
     let data_ptr = &mut data as *mut _ as *mut u8;
-    return if intrinsics::r#try(do_call::<F, R>, data_ptr, do_catch::<F, R>) == 0 {
-        Ok(ManuallyDrop::into_inner(data.r))
-    } else {
-        Err(ManuallyDrop::into_inner(data.p))
-    };
+    // SAFETY:
+    //
+    // Access to the union's fields: this is `std` and we know that the `r#try`
+    // intrinsic fills in the `r` or `p` union field based on its return value.
+    //
+    // The call to `intrinsics::r#try` is made safe by:
+    // - `do_call`, the first argument, can be called with the initial `data_ptr`.
+    // - `do_catch`, the second argument, can be called with the `data_ptr` as well.
+    // See their safety preconditions for more informations
+    unsafe {
+        return if intrinsics::r#try(do_call::<F, R>, data_ptr, do_catch::<F, R>) == 0 {
+            Ok(ManuallyDrop::into_inner(data.r))
+        } else {
+            Err(ManuallyDrop::into_inner(data.p))
+        };
+    }
 
     // We consider unwinding to be rare, so mark this function as cold. However,
     // do not mark it no-inline -- that decision is best to leave to the
@@ -334,13 +347,25 @@ pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>>
     // non-cold function, though, as of the writing of this comment).
     #[cold]
     unsafe fn cleanup(payload: *mut u8) -> Box<dyn Any + Send + 'static> {
-        let obj = Box::from_raw(__rust_panic_cleanup(payload));
+        // SAFETY: The whole unsafe block hinges on a correct implementation of
+        // the panic handler `__rust_panic_cleanup`. As such we can only
+        // assume it returns the correct thing for `Box::from_raw` to work
+        // without undefined behavior.
+        let obj = unsafe { Box::from_raw(__rust_panic_cleanup(payload)) };
         panic_count::decrease();
         obj
     }
 
+    // SAFETY:
+    // data must be non-NUL, correctly aligned, and a pointer to a `Data<F, R>`
+    // Its must contains a valid `f` (type: F) value that can be use to fill
+    // `data.r`.
+    //
+    // This function cannot be marked as `unsafe` because `intrinsics::r#try`
+    // expects normal function pointers.
     #[inline]
     fn do_call<F: FnOnce() -> R, R>(data: *mut u8) {
+        // SAFETY: this is the responsibilty of the caller, see above.
         unsafe {
             let data = data as *mut Data<F, R>;
             let data = &mut (*data);
@@ -352,8 +377,21 @@ pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>>
     // We *do* want this part of the catch to be inlined: this allows the
     // compiler to properly track accesses to the Data union and optimize it
     // away most of the time.
+    //
+    // SAFETY:
+    // data must be non-NUL, correctly aligned, and a pointer to a `Data<F, R>`
+    // Since this uses `cleanup` it also hinges on a correct implementation of
+    // `__rustc_panic_cleanup`.
+    //
+    // This function cannot be marked as `unsafe` because `intrinsics::r#try`
+    // expects normal function pointers.
     #[inline]
     fn do_catch<F: FnOnce() -> R, R>(data: *mut u8, payload: *mut u8) {
+        // SAFETY: this is the responsibilty of the caller, see above.
+        //
+        // When `__rustc_panic_cleaner` is correctly implemented we can rely
+        // on `obj` being the correct thing to pass to `data.p` (after wrapping
+        // in `ManuallyDrop`).
         unsafe {
             let data = data as *mut Data<F, R>;
             let data = &mut (*data);
diff --git a/library/std/src/thread/local.rs b/library/std/src/thread/local.rs
index ecd6fbc6b93..66508f06b28 100644
--- a/library/std/src/thread/local.rs
+++ b/library/std/src/thread/local.rs
@@ -172,7 +172,11 @@ macro_rules! __thread_local_inner {
                 static __KEY: $crate::thread::__OsLocalKeyInner<$t> =
                     $crate::thread::__OsLocalKeyInner::new();
 
-                __KEY.get(__init)
+                // FIXME: remove the #[allow(...)] marker when macros don't
+                // raise warning for missing/extraneous unsafe blocks anymore.
+                // See https://github.com/rust-lang/rust/issues/74838.
+                #[allow(unused_unsafe)]
+                unsafe { __KEY.get(__init) }
             }
 
             unsafe {