lint ImproperCTypes: clean up exported static variables

Author: niacdoial

compiler/rustc_lint/src/types/improper_ctypes.rs

@@ -4,6 +4,7 @@ use std::iter;
 use std::ops::ControlFlow;
 
 use rustc_abi::VariantIdx;
+use rustc_ast::Mutability;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::DiagMessage;
 use rustc_hir::def::CtorKind;
@@ -422,33 +423,43 @@ fn get_type_sizedness<'tcx, 'a>(cx: &'a LateContext<'tcx>, ty: Ty<'tcx>) -> Type
 
 #[allow(non_snake_case)]
 mod CTypesVisitorStateFlags {
-    pub(super) const NO_FLAGS: u8 = 0b00000;
+    pub(super) const NO_FLAGS: u8 = 0b000000;
     /// for use in (externally-linked) static variables
-    pub(super) const STATIC: u8 = 0b00001;
+    pub(super) const STATIC: u8 = 0b000001;
     /// for use in functions in general
-    pub(super) const FUNC: u8 = 0b00010;
+    pub(super) const FUNC: u8 = 0b000010;
     /// for variables in function returns (implicitly: not for static variables)
-    pub(super) const FN_RETURN: u8 = 0b00100;
-    /// for variables in functions which are defined in rust (implicitly: not for static variables)
-    pub(super) const FN_DEFINED: u8 = 0b01000;
-    /// for time where we are only defining the type of something
+    pub(super) const FN_RETURN: u8 = 0b000100;
+    /// for variables in functions/variables which are defined in rust
+    pub(super) const DEFINED: u8 = 0b001000;
+    /// for times where we are only defining the type of something
     /// (struct/enum/union definitions, FnPtrs)
-    pub(super) const THEORETICAL: u8 = 0b10000;
+    pub(super) const THEORETICAL: u8 = 0b010000;
+    /// if we are looking at an interface where the value can be set by the non-rust side
+    /// (important for e.g. nonzero assumptions)
+    pub(super) const FOREIGN_VALUES: u8 = 0b100000;
 }
 
 #[repr(u8)]
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 enum CTypesVisitorState {
     None = CTypesVisitorStateFlags::NO_FLAGS,
     // uses bitflags from CTypesVisitorStateFlags
-    StaticTy = CTypesVisitorStateFlags::STATIC,
-    ExportedStaticTy = CTypesVisitorStateFlags::STATIC | CTypesVisitorStateFlags::FN_DEFINED,
-    ArgumentTyInDefinition = CTypesVisitorStateFlags::FUNC | CTypesVisitorStateFlags::FN_DEFINED,
+    StaticTy = CTypesVisitorStateFlags::STATIC | CTypesVisitorStateFlags::FOREIGN_VALUES,
+    ExportedStaticTy = CTypesVisitorStateFlags::STATIC | CTypesVisitorStateFlags::DEFINED,
+    ExportedStaticMutTy = CTypesVisitorStateFlags::STATIC
+        | CTypesVisitorStateFlags::DEFINED
+        | CTypesVisitorStateFlags::FOREIGN_VALUES,
+    ArgumentTyInDefinition = CTypesVisitorStateFlags::FUNC
+        | CTypesVisitorStateFlags::DEFINED
+        | CTypesVisitorStateFlags::FOREIGN_VALUES,
     ReturnTyInDefinition = CTypesVisitorStateFlags::FUNC
         | CTypesVisitorStateFlags::FN_RETURN
-        | CTypesVisitorStateFlags::FN_DEFINED,
+        | CTypesVisitorStateFlags::DEFINED,
     ArgumentTyInDeclaration = CTypesVisitorStateFlags::FUNC,
-    ReturnTyInDeclaration = CTypesVisitorStateFlags::FUNC | CTypesVisitorStateFlags::FN_RETURN,
+    ReturnTyInDeclaration = CTypesVisitorStateFlags::FUNC
+        | CTypesVisitorStateFlags::FN_RETURN
+        | CTypesVisitorStateFlags::FOREIGN_VALUES,
     ArgumentTyInFnPtr = CTypesVisitorStateFlags::FUNC | CTypesVisitorStateFlags::THEORETICAL,
     ReturnTyInFnPtr = CTypesVisitorStateFlags::FUNC
         | CTypesVisitorStateFlags::THEORETICAL
@@ -489,58 +500,35 @@ impl CTypesVisitorState {
     /// to be treated as an opaque type on the other side of the FFI boundary
     fn is_in_defined_function(self) -> bool {
         use CTypesVisitorStateFlags::*;
-        let ret = ((self as u8) & FN_DEFINED) != 0;
-        #[cfg(debug_assertions)]
-        if ret {
-            assert!(self.is_in_function());
-        }
-        ret
-    }
-    /// whether we the type is used (directly or not) in a function pointer type
-    fn is_in_fn_ptr(self) -> bool {
-        use CTypesVisitorStateFlags::*;
-        ((self as u8) & THEORETICAL) != 0 && self.is_in_function()
+        ((self as u8) & DEFINED) != 0 && self.is_in_function()
     }
 
     /// whether we can expect type parameters and co in a given type
     fn can_expect_ty_params(self) -> bool {
         use CTypesVisitorStateFlags::*;
-        // rust-defined functions, as well as FnPtrs and ADT definitions
-        if ((self as u8) & THEORETICAL) != 0 {
-            true
-        } else {
-            ((self as u8) & FN_DEFINED) != 0 && ((self as u8) & STATIC) == 0
-        }
+        // rust-defined functions, as well as FnPtrs
+        ((self as u8) & THEORETICAL) != 0 || self.is_in_defined_function()
     }
 
     /// whether the value for that type might come from the non-rust side of a FFI boundary
     /// this is particularly useful for non-raw pointers, since rust assume they are non-null
     fn value_may_be_unchecked(self) -> bool {
-        if self.is_in_static() {
-            // FIXME: this is evidently untrue for non-mut static variables
-            // (assuming the cross-FFI code respects this)
-            true
-        } else if self.is_in_defined_function() {
-            // function definitions are assumed to be maybe-not-rust-caller, rust-callee
-            !self.is_in_function_return()
-        } else if self.is_in_fn_ptr() {
-            // 4 cases for function pointers:
-            // - rust caller, rust callee: everything comes from rust
-            // - non-rust-caller, non-rust callee: declaring invariants that are not valid
-            //   is suboptimal, but ultimately not our problem
-            // - non-rust-caller, rust callee: there will be a function declaration somewhere,
-            //   let's assume it will raise the appropriate warning in our stead
-            // - rust caller, non-rust callee: it's possible that the function is a callback,
-            //   not something from a pre-declared API.
-            // so, in theory, we need to care about the function return being possibly non-rust-controlled.
-            // sadly, we need to ignore this because making pointers out of rust-defined functions
-            // would force to systematically cast or overwrap their return types...
-            // FIXME: is there anything better we can do here?
-            false
-        } else {
-            // function declarations are assumed to be rust-caller, non-rust-callee
-            self.is_in_function_return()
-        }
+        // function definitions are assumed to be maybe-not-rust-caller, rust-callee
+        // function declarations are assumed to be rust-caller, non-rust-callee
+        // 4 cases for function pointers:
+        // - rust caller, rust callee: everything comes from rust
+        // - non-rust-caller, non-rust callee: declaring invariants that are not valid
+        //   is suboptimal, but ultimately not our problem
+        // - non-rust-caller, rust callee: there will be a function declaration somewhere,
+        //   let's assume it will raise the appropriate warning in our stead
+        // - rust caller, non-rust callee: it's possible that the function is a callback,
+        //   not something from a pre-declared API.
+        // so, in theory, we need to care about the function return being possibly non-rust-controlled.
+        // sadly, we need to ignore this because making pointers out of rust-defined functions
+        // would force to systematically cast or overwrap their return types...
+        // FIXME: is there anything better we can do here?
+        use CTypesVisitorStateFlags::*;
+        ((self as u8) & FOREIGN_VALUES) != 0
     }
 }
 
@@ -1654,10 +1642,21 @@ impl ImproperCTypesLint {
     }
 
     /// Check that a `#[no_mangle]`/`#[export_name = _]` static variable is of a ffi-safe type
-    fn check_exported_static<'tcx>(&self, cx: &LateContext<'tcx>, id: hir::OwnerId, span: Span) {
+    fn check_exported_static<'tcx>(
+        &self,
+        cx: &LateContext<'tcx>,
+        id: hir::OwnerId,
+        span: Span,
+        is_mut: bool,
+    ) {
         let ty = cx.tcx.type_of(id).instantiate_identity();
         let visitor = ImproperCTypesVisitor::new(cx);
-        let ffi_res = visitor.check_for_type(CTypesVisitorState::ExportedStaticTy, ty);
+        let state = if is_mut {
+            CTypesVisitorState::ExportedStaticMutTy
+        } else {
+            CTypesVisitorState::ExportedStaticTy
+        };
+        let ffi_res = visitor.check_for_type(state, ty);
         self.process_ffi_result(cx, span, ffi_res, CItemKind::ExportedStatic);
     }
 
@@ -1844,11 +1843,27 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesLint {
                     cx.tcx.type_of(item.owner_id).instantiate_identity(),
                 );
 
-                if matches!(item.kind, hir::ItemKind::Static(..))
-                    && (cx.tcx.has_attr(item.owner_id, sym::no_mangle)
-                        || cx.tcx.has_attr(item.owner_id, sym::export_name))
-                {
-                    self.check_exported_static(cx, item.owner_id, ty.span);
+                // FIXME: cx.tcx.has_attr no worky
+                // if let hir::ItemKind::Static(is_mut, _, _, _) = item.kind
+                //     && (cx.tcx.has_attr(item.owner_id, sym::no_mangle)
+                //         || cx.tcx.has_attr(item.owner_id, sym::export_name))
+                if let hir::ItemKind::Static(is_mut, _, _, _) = item.kind {
+                    let is_exported_static = cx.tcx.get_all_attrs(item.owner_id).iter().any(|x| {
+                        matches!(
+                            x,
+                            hir::Attribute::Parsed(
+                                hir::attrs::AttributeKind::NoMangle(_)
+                                    | hir::attrs::AttributeKind::ExportName { .. }
+                            )
+                        )
+                    });
+                    if is_exported_static {
+                        let is_mut = match is_mut {
+                            Mutability::Not => false,
+                            Mutability::Mut => true,
+                        };
+                        self.check_exported_static(cx, item.owner_id, ty.span, is_mut);
+                    }
                 }
             }
             // See `check_fn` for declarations, `check_foreign_items` for definitions in extern blocks

tests/ui/lint/improper_ctypes/ctypes.rs

@@ -5,7 +5,7 @@
 
 #![allow(private_interfaces)]
 #![deny(improper_ctypes, improper_c_callbacks)]
-#![deny(improper_c_fn_definitions)]
+#![deny(improper_c_fn_definitions, improper_c_var_definitions)]
 
 use std::cell::UnsafeCell;
 use std::marker::PhantomData;
@@ -138,6 +138,16 @@ extern "C" {
     pub fn good19(_: &String);
 }
 
+static DEFAULT_U32: u32 = 42;
+#[no_mangle]
+static EXPORTED_STATIC: &u32 = &DEFAULT_U32;
+#[no_mangle]
+static EXPORTED_STATIC_BAD: &'static str = "is this reaching you, plugin?";
+//~^ ERROR: uses type `&str`
+#[export_name="EXPORTED_STATIC_MUT_BUT_RENAMED"]
+static mut EXPORTED_STATIC_MUT: &u32 = &DEFAULT_U32;
+//~^ ERROR: uses type `&u32`
+
 #[cfg(not(target_arch = "wasm32"))]
 extern "C" {
     pub fn good1(size: *const c_int);

tests/ui/lint/improper_ctypes/ctypes.stderr

@@ -208,5 +208,29 @@ LL |     pub fn no_niche_b(b: Option<UnsafeCell<&i32>>);
    = help: consider adding a `#[repr(C)]`, `#[repr(transparent)]`, or integer `#[repr(...)]` attribute to this enum
    = note: enum has no representation hint
 
-error: aborting due to 20 previous errors
+error: foreign-code-reachable static uses type `&str`, which is not FFI-safe
+  --> $DIR/ctypes.rs:145:29
+   |
+LL | static EXPORTED_STATIC_BAD: &'static str = "is this reaching you, plugin?";
+   |                             ^^^^^^^^^^^^ not FFI-safe
+   |
+   = help: consider using `*const u8` and a length instead
+   = note: this reference to an unsized type contains metadata, which makes it incompatible with a C pointer
+note: the lint level is defined here
+  --> $DIR/ctypes.rs:8:36
+   |
+LL | #![deny(improper_c_fn_definitions, improper_c_var_definitions)]
+   |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: foreign-code-reachable static uses type `&u32`, which is not FFI-safe
+  --> $DIR/ctypes.rs:148:33
+   |
+LL | static mut EXPORTED_STATIC_MUT: &u32 = &DEFAULT_U32;
+   |                                 ^^^^ not FFI-safe
+   |
+   = help: consider using a raw pointer, or wrapping `&u32` in an `Option<_>`
+   = note: boxes, references, and function pointers are assumed to be valid (non-null, non-dangling, aligned) pointers,
+           which cannot be garanteed if their values are produced by non-rust code
+
+error: aborting due to 22 previous errors