Lint on c_str!("literal")

Author: nbdd0121

src/diagnostic_items/out_of_band.rs

@@ -1,6 +1,7 @@
 //! Out-of-band attributes attached without source code changes.
 
-use rustc_hir::def_id::{DefId, LOCAL_CRATE};
+use rustc_hir::def::{DefKind, Res};
+use rustc_hir::def_id::{CRATE_DEF_ID, DefId, LOCAL_CRATE};
 use rustc_hir::diagnostic_items::DiagnosticItems;
 use rustc_middle::middle::exported_symbols::ExportedSymbol;
 use rustc_middle::ty::TyCtxt;
@@ -11,6 +12,12 @@ pub fn infer_missing_items<'tcx>(tcx: TyCtxt<'tcx>, items: &mut DiagnosticItems)
             super::collect_item(tcx, items, crate::symbol::build_error, def_id);
         }
     }
+
+    if !items.name_to_id.contains_key(&crate::symbol::c_str) {
+        if let Some(def_id) = infer_c_str_diagnostic_item(tcx) {
+            super::collect_item(tcx, items, crate::symbol::c_str, def_id);
+        }
+    }
 }
 
 pub fn infer_build_error_diagnostic_item<'tcx>(tcx: TyCtxt<'tcx>) -> Option<DefId> {
@@ -24,3 +31,22 @@ pub fn infer_build_error_diagnostic_item<'tcx>(tcx: TyCtxt<'tcx>) -> Option<DefI
 
     None
 }
+
+pub fn infer_c_str_diagnostic_item<'tcx>(tcx: TyCtxt<'tcx>) -> Option<DefId> {
+    let name = tcx.crate_name(LOCAL_CRATE);
+
+    if name != crate::symbol::kernel {
+        return None;
+    }
+
+    let c_str = tcx
+        .module_children_local(CRATE_DEF_ID)
+        .iter()
+        .find(|c| {
+            c.ident.name == crate::symbol::c_str && matches!(c.res, Res::Def(DefKind::Macro(_), _))
+        })?
+        .res
+        .def_id();
+
+    Some(c_str)
+}

src/hir_lints/c_str_literal.rs

@@ -0,0 +1,93 @@
+use rustc_hir::Expr;
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::{self, Span, Symbol};
+
+use crate::ctxt::AnalysisCtxt;
+
+declare_tool_lint! {
+    /// The `c_str_literal` lint detects when the kernel `c_str!` macro is used on a string literal.
+    pub klint::C_STR_LITERAL,
+    Warn,
+    "`c_str!` used on a string literal"
+}
+
+pub struct CStrLiteralLint<'tcx> {
+    pub cx: &'tcx AnalysisCtxt<'tcx>,
+}
+
+impl_lint_pass!(CStrLiteralLint<'_> => [C_STR_LITERAL]);
+
+#[derive(Diagnostic)]
+#[diag("`{$macro_name}!` is used on a literal")]
+struct CStrLiteral {
+    #[primary_span]
+    #[suggestion(
+        "use C-string literals instead",
+        code = "c{arg}",
+        applicability = "machine-applicable"
+    )]
+    pub span: Span,
+    pub macro_name: Symbol,
+    pub arg: String,
+}
+
+impl<'tcx> CStrLiteralLint<'tcx> {
+    fn extract_arg(&self, span: Span) -> Option<String> {
+        let source = self.cx.sess.source_map().span_to_snippet(span).ok()?;
+
+        let arg = source.split_once('!')?.1;
+        if arg.len() <= 2 {
+            return None;
+        }
+
+        Some(arg[1..arg.len() - 1].trim().to_owned())
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for CStrLiteralLint<'tcx> {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        // If would be ideal if we can check before macro expansion. However, pre_expansion_lint is
+        // not recommended because the lint level infrastructure if not yet ready, and also it does
+        // not have information of the name resolution available so we cannot precisely determine if
+        // the macro is something that we want to lint now.
+        //
+        // So, as an alternative strategy, try to backtrace macros and find an expression that is expanded
+        // from the `c_str!` macro. Once that found, use span information to recover the argument used to
+        // call the macro.
+        //
+        // However, given that `c_str!` have used `concat!()` internally, there is no single span in the
+        // expanded HIR that maps to the input argument... As a very crude approximation, use source map
+        // to obtain the source and check based on that. This is also very hacky but at least lint level
+        // and name resolution works as intended...
+
+        // TODO: This check will replicated multiple times for each sub-expression of `c_str!`.
+        // It might be good if we stop early and stop recursing into sub-expressions, although this is not
+        // something that can be achieved with `LateLintPass`.
+        let span = expr.span;
+        let Some(expn_data) = span.macro_backtrace().next() else {
+            return;
+        };
+
+        let Some(c_str) = self.cx.get_klint_diagnostic_item(crate::symbol::c_str) else {
+            return;
+        };
+
+        if expn_data.macro_def_id != Some(c_str) {
+            return;
+        }
+
+        if let Some(arg) = self.extract_arg(expn_data.call_site) {
+            if arg.starts_with('"') && arg.ends_with('"') {
+                cx.emit_diag_lint(
+                    C_STR_LITERAL,
+                    CStrLiteral {
+                        span: expn_data.call_site,
+                        macro_name: self.cx.item_name(c_str),
+                        arg,
+                    },
+                );
+            }
+        }
+    }
+}

src/hir_lints/mod.rs

@@ -1 +1,2 @@
+pub(crate) mod c_str_literal;
 pub(crate) mod not_using_prelude;

src/main.rs

@@ -117,9 +117,16 @@ impl Callbacks for MyCallbacks {
                 infallible_allocation::INFALLIBLE_ALLOCATION,
                 atomic_context::ATOMIC_CONTEXT,
                 binary_analysis::stack_size::STACK_FRAME_TOO_LARGE,
+                hir_lints::c_str_literal::C_STR_LITERAL,
                 hir_lints::not_using_prelude::NOT_USING_PRELUDE,
             ]);
 
+            lint_store.register_late_pass(|tcx| {
+                Box::new(hir_lints::c_str_literal::CStrLiteralLint {
+                    cx: driver::cx::<MyCallbacks>(tcx),
+                })
+            });
+
             lint_store.register_late_pass(|tcx| {
                 Box::new(hir_lints::not_using_prelude::NotUsingPrelude {
                     cx: driver::cx::<MyCallbacks>(tcx),

src/symbol.rs

@@ -44,13 +44,16 @@ def! {
     sort,
     quicksort,
     partition,
+    kernel,
     diagnostic_item,
 
     any_context,
     atomic_context,
     atomic_context_only,
     process_context,
 
+    // Diagnostic items
+    c_str,
     build_error,
 
     CONFIG_FRAME_WARN,

tests/ui/c_str_literal.rs

@@ -0,0 +1,42 @@
+#![deny(klint::c_str_literal)]
+
+#[klint::diagnostic_item = "c_str"]
+macro_rules! c_str {
+    ($str:expr) => {{
+        const S: &str = concat!($str, "\0");
+        const C: &::core::ffi::CStr = match ::core::ffi::CStr::from_bytes_with_nul(S.as_bytes()) {
+            Ok(v) => v,
+            Err(_) => panic!("string contains interior NUL"),
+        };
+        C
+    }};
+}
+
+macro_rules! forward_c_str {
+    ($str:expr) => {
+        c_str!($str)
+    };
+}
+
+macro_rules! wrap_c_str {
+    () => {
+        c_str!("hello")
+    };
+}
+
+fn main() {
+    // Should warn.
+    c_str!("hello");
+
+    // Should not warn.
+    c_str!(concat!("a", "b"));
+    // Should not warn.
+    c_str!(stringify!(hello));
+
+    // Should not warn.
+    forward_c_str!("a");
+
+    // Should warn. We currently do not have ability to warn directly at `macro_rules!`; warning
+    // is only generated once the macro is used.
+    wrap_c_str!();
+}

tests/ui/c_str_literal.stderr

@@ -0,0 +1,25 @@
+error: `c_str!` is used on a literal
+  --> $DIR/c_str_literal.rs:29:5
+   |
+29 |     c_str!("hello");
+   |     ^^^^^^^^^^^^^^^ help: use C-string literals instead: `c"hello"`
+   |
+note: the lint level is defined here
+  --> $DIR/c_str_literal.rs:1:9
+   |
+ 1 | #![deny(klint::c_str_literal)]
+   |         ^^^^^^^^^^^^^^^^^^^^
+
+error: `c_str!` is used on a literal
+  --> $DIR/c_str_literal.rs:23:9
+   |
+23 |         c_str!("hello")
+   |         ^^^^^^^^^^^^^^^ help: use C-string literals instead: `c"hello"`
+...
+41 |     wrap_c_str!();
+   |     ------------- in this macro invocation
+   |
+   = note: this error originates in the macro `wrap_c_str` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 2 previous errors
+