diff --git a/CHANGELOG.md b/CHANGELOG.md index 856c229f4679..96a4d14524f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7455,6 +7455,7 @@ Released 2018-09-13 [`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive [`std_instead_of_alloc`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_alloc [`std_instead_of_core`]: https://rust-lang.github.io/rust-clippy/master/index.html#std_instead_of_core +[`str_ptr_in_c_abi`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_ptr_in_c_abi [`str_split_at_newline`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_split_at_newline [`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string [`string_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index e77a0ff4a8ab..e5f46330cb5c 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -714,6 +714,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::std_instead_of_core::ALLOC_INSTEAD_OF_CORE_INFO, crate::std_instead_of_core::STD_INSTEAD_OF_ALLOC_INFO, crate::std_instead_of_core::STD_INSTEAD_OF_CORE_INFO, + crate::str_ptr_in_c_abi::STR_PTR_IN_C_ABI_INFO, crate::string_patterns::MANUAL_PATTERN_CHAR_COMPARISON_INFO, crate::string_patterns::SINGLE_CHAR_PATTERN_INFO, crate::strings::STR_TO_STRING_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 74e03c9b3859..80a43560f961 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -348,6 +348,7 @@ mod size_of_in_element_count; mod size_of_ref; mod slow_vector_initialization; mod std_instead_of_core; +mod str_ptr_in_c_abi; mod string_patterns; mod strings; mod strlen_on_c_strings; @@ -866,6 +867,7 @@ rustc_lint::late_lint_methods!( RefPatterns: ref_patterns::RefPatterns = ref_patterns::RefPatterns, RedundantElse: redundant_else::RedundantElse = redundant_else::RedundantElse, RestWhenDestructuringStruct: rest_when_destructuring_struct::RestWhenDestructuringStruct = rest_when_destructuring_struct::RestWhenDestructuringStruct, + StrPtrInCAbi: str_ptr_in_c_abi::StrPtrInCAbi = str_ptr_in_c_abi::StrPtrInCAbi, // add late passes here, used by `cargo dev new_lint` ]] ); diff --git a/clippy_lints/src/str_ptr_in_c_abi.rs b/clippy_lints/src/str_ptr_in_c_abi.rs new file mode 100644 index 000000000000..5c5389111bf9 --- /dev/null +++ b/clippy_lints/src/str_ptr_in_c_abi.rs @@ -0,0 +1,91 @@ +use rustc_hir::{Expr, ExprKind, TyKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; + +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::sym; + +declare_clippy_lint! { + /// ### What it does + /// + /// This lint triggers if a pointer to a Rust `str` is passed into an `extern "C"` interface + /// where you should instead be providing a pointer to a `CString`. + /// + /// ### Why is this bad? + /// + /// Foreign functions under the C ABI expect that a string ends with a null byte (`'\0'`). + /// Rust's `str` doesn't provide a null byte. Instead it contains a length for the string. + /// This leads to two problems. + /// + /// 1. The length parameter of the Rust string will be misinterpreted as a character, which is logically invalid. + /// 2. Without a null byte, foreign functions will read beyond the memory allocated to the string searching for the null terminator, causing undefined behavior (UB). + /// + /// ### Example + /// ```no_run + /// # unsafe extern "C" fn strlen(s: *const i8) -> usize { unimplemented!() } + /// unsafe { strlen("Hello".as_ptr() as *const _) }; + /// ``` + /// Use instead: + /// ```no_run + /// # unsafe extern "C" fn strlen(s: *const i8) -> usize { unimplemented!() } + /// let cstring = std::ffi::CString::new("Hello".as_bytes()).unwrap(); + /// unsafe { strlen(cstring.as_ptr()) }; + /// ``` + #[clippy::version = "1.99.0"] + pub STR_PTR_IN_C_ABI, + nursery, + "discourage str pointers in C ABI fns" +} + +declare_lint_pass!(StrPtrInCAbi => [STR_PTR_IN_C_ABI]); + +impl<'tcx> LateLintPass<'tcx> for StrPtrInCAbi { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + // find call expressions + if let ExprKind::Call(callee, args) = expr.kind + // where the signature of the callee shows it is + // extern "C" fn + && is_extern_c_fn(cx, callee) + // and takes a raw pointer as an argument + // and to that argument gives a pointer to a Rust `str` + && let span = args + .iter() + .filter(|arg| is_cast_str_ptr_to_raw_ptr(cx, arg)) + .map(|arg| arg.span) + .collect::>() + && !span.is_empty() + { + span_lint_and_help( + cx, + STR_PTR_IN_C_ABI, + span, + "giving a pointer to a Rust `str` to an `extern \"C\" fn` can cause undefined behavior", + /* help_span */ None, + "first convert the `str` to a `std::ffi::CString` and then get a pointer from there", + ); + } + } +} + +/// Does the expression represent an `extern "C" fn` of some type? +fn is_extern_c_fn<'tcx>(cx: &LateContext<'tcx>, callee: &'tcx Expr<'tcx>) -> bool { + let callee_ty = cx.typeck_results().expr_ty(callee); + if !(callee_ty.is_fn() || callee_ty.is_fn_ptr()) { + return false; + } + matches!(callee_ty.fn_sig(cx.tcx).abi(), rustc_abi::ExternAbi::C { .. }) +} + +/// If `arg` is `derefs_to_str.as(_mut)_ptr() as *const(mut) _`, then true. +fn is_cast_str_ptr_to_raw_ptr<'tcx>(cx: &LateContext<'tcx>, arg: &Expr<'tcx>) -> bool { + let typeck = cx.typeck_results(); + if let ExprKind::Cast(expr, ty) = arg.kind + && matches!(ty.kind, TyKind::Ptr(_)) + && let ExprKind::MethodCall(method, this, _args, _span) = expr.kind + && matches!(method.ident.name, sym::as_ptr | sym::as_mut_ptr) + && typeck.expr_ty_adjusted(this).peel_refs().is_str() + { + return true; + } + false +} diff --git a/clippy_utils/src/sym.rs b/clippy_utils/src/sym.rs index 5da94bfda5e6..10b9d2229e8f 100644 --- a/clippy_utils/src/sym.rs +++ b/clippy_utils/src/sym.rs @@ -138,6 +138,7 @@ generate! { as_deref, as_deref_mut, as_mut, + as_mut_ptr, as_path, as_ptr, as_str, diff --git a/tests/ui/str_ptr_in_c_abi.rs b/tests/ui/str_ptr_in_c_abi.rs new file mode 100644 index 000000000000..eb3b54cc8c74 --- /dev/null +++ b/tests/ui/str_ptr_in_c_abi.rs @@ -0,0 +1,47 @@ +#![warn(clippy::str_ptr_in_c_abi)] + +use std::ffi::CString; + +fn main() { + // This should use a pointer to a CString + unsafe { printf("Hello".as_ptr() as *const _) }; + //~^ str_ptr_in_c_abi + + // Like this + let cstring = CString::new("Hello".as_bytes()).unwrap(); + unsafe { printf(cstring.as_ptr()) }; + + // One should also use mut pointers to CStrings + let mut buffer = String::new(); + let mut buffer = buffer.as_mut_str(); // this lint can only detect `str`s for now + unsafe { strcpy(buffer.as_mut_ptr() as *mut _, cstring.as_ptr()) }; + //~^ str_ptr_in_c_abi + + let mut cstring_mut = CString::new([]).unwrap(); + unsafe { strcpy(cstring_mut.into_raw(), cstring.as_ptr()) }; + + // Two rust strings at once! + unsafe { strcpy(buffer.as_mut_ptr() as *mut _, "Hello".as_ptr() as *const _) }; + //~^ str_ptr_in_c_abi + + // It can detect smart pointers to str + let hello_string: String = "Hello".into(); + let hello_box: Box = "Hello".into(); + let hello_rc: std::rc::Rc = "Hello".into(); + unsafe { printf(hello_string.as_ptr() as *const _) }; + //~^ str_ptr_in_c_abi + unsafe { printf(hello_box.as_ptr() as *const _) }; + //~^ str_ptr_in_c_abi + unsafe { printf(hello_rc.as_ptr() as *const _) }; + //~^ str_ptr_in_c_abi + + // It detects str to ptr casts in variadics + let fmt = CString::new("%s\n".as_bytes()).unwrap(); + unsafe { printf(fmt.as_ptr(), "I'm (incorrectly) printf-ing a str!".as_ptr() as *const _) }; + //~^ str_ptr_in_c_abi +} + +unsafe extern "C" { + fn strcpy(dst: *mut i8, src: *const i8) -> *mut i8; + fn printf(format: *const i8, ...) -> i32; +} diff --git a/tests/ui/str_ptr_in_c_abi.stderr b/tests/ui/str_ptr_in_c_abi.stderr new file mode 100644 index 000000000000..baf61a5c373d --- /dev/null +++ b/tests/ui/str_ptr_in_c_abi.stderr @@ -0,0 +1,60 @@ +error: giving a pointer to a Rust `str` to an `extern "C" fn` can cause undefined behavior + --> tests/ui/str_ptr_in_c_abi.rs:7:21 + | +LL | unsafe { printf("Hello".as_ptr() as *const _) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: first convert the `str` to a `std::ffi::CString` and then get a pointer from there + = note: `-D clippy::str-ptr-in-c-abi` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::str_ptr_in_c_abi)]` + +error: giving a pointer to a Rust `str` to an `extern "C" fn` can cause undefined behavior + --> tests/ui/str_ptr_in_c_abi.rs:17:21 + | +LL | unsafe { strcpy(buffer.as_mut_ptr() as *mut _, cstring.as_ptr()) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: first convert the `str` to a `std::ffi::CString` and then get a pointer from there + +error: giving a pointer to a Rust `str` to an `extern "C" fn` can cause undefined behavior + --> tests/ui/str_ptr_in_c_abi.rs:24:21 + | +LL | unsafe { strcpy(buffer.as_mut_ptr() as *mut _, "Hello".as_ptr() as *const _) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: first convert the `str` to a `std::ffi::CString` and then get a pointer from there + +error: giving a pointer to a Rust `str` to an `extern "C" fn` can cause undefined behavior + --> tests/ui/str_ptr_in_c_abi.rs:31:21 + | +LL | unsafe { printf(hello_string.as_ptr() as *const _) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: first convert the `str` to a `std::ffi::CString` and then get a pointer from there + +error: giving a pointer to a Rust `str` to an `extern "C" fn` can cause undefined behavior + --> tests/ui/str_ptr_in_c_abi.rs:33:21 + | +LL | unsafe { printf(hello_box.as_ptr() as *const _) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: first convert the `str` to a `std::ffi::CString` and then get a pointer from there + +error: giving a pointer to a Rust `str` to an `extern "C" fn` can cause undefined behavior + --> tests/ui/str_ptr_in_c_abi.rs:35:21 + | +LL | unsafe { printf(hello_rc.as_ptr() as *const _) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: first convert the `str` to a `std::ffi::CString` and then get a pointer from there + +error: giving a pointer to a Rust `str` to an `extern "C" fn` can cause undefined behavior + --> tests/ui/str_ptr_in_c_abi.rs:40:35 + | +LL | unsafe { printf(fmt.as_ptr(), "I'm (incorrectly) printf-ing a str!".as_ptr() as *const _) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: first convert the `str` to a `std::ffi::CString` and then get a pointer from there + +error: aborting due to 7 previous errors +