Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

Commit 0d1f1ce

Browse files
JarchoJarcho
committed
Remove surrounding unsafe block in strlen_on_c_strings when possible
1 parent c443f8f commit 0d1f1ce

File tree

4 files changed

+128
-31
lines changed

4 files changed

+128
-31
lines changed

‎clippy_lints/src/strlen_on_c_strings.rs

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2-
use clippy_utils::match_libc_symbol;
32
use clippy_utils::source::snippet_with_context;
43
use clippy_utils::ty::is_type_diagnostic_item;
4+
use clippy_utils::visitors::is_expr_unsafe;
5+
use clippy_utils::{get_parent_node, match_libc_symbol};
56
use if_chain::if_chain;
67
use rustc_errors::Applicability;
7-
use rustc_hir as hir;
8+
use rustc_hir::{Block, BlockCheckMode, Expr, ExprKind, Node, UnsafeSource};
89
use rustc_lint::{LateContext, LateLintPass};
910
use rustc_session::{declare_lint_pass, declare_tool_lint};
1011
use rustc_span::symbol::sym;
@@ -39,20 +40,31 @@ declare_clippy_lint! {
3940
declare_lint_pass!(StrlenOnCStrings => [STRLEN_ON_C_STRINGS]);
4041

4142
impl LateLintPass<'tcx> for StrlenOnCStrings {
42-
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
43+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
4344
if_chain! {
4445
if !expr.span.from_expansion();
45-
if let hir::ExprKind::Call(func, [recv]) = expr.kind;
46-
if let hir::ExprKind::Path(path) = &func.kind;
46+
if let ExprKind::Call(func, [recv]) = expr.kind;
47+
if let ExprKind::Path(path) = &func.kind;
4748
if let Some(did) = cx.qpath_res(path, func.hir_id).opt_def_id();
4849
if match_libc_symbol(cx, did, "strlen");
49-
if let hir::ExprKind::MethodCall(path, _, [self_arg], _) = recv.kind;
50+
if let ExprKind::MethodCall(path, _, [self_arg], _) = recv.kind;
5051
if !recv.span.from_expansion();
5152
if path.ident.name == sym::as_ptr;
5253
then {
54+
let ctxt = expr.span.ctxt();
55+
let span = match get_parent_node(cx.tcx, expr.hir_id) {
56+
Some(Node::Block(&Block {
57+
rules: BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided), span, ..
58+
}))
59+
if span.ctxt() == ctxt && !is_expr_unsafe(cx, self_arg) => {
60+
span
61+
}
62+
_ => expr.span,
63+
};
64+
5365
let ty = cx.typeck_results().expr_ty(self_arg).peel_refs();
54-
let mut app = Applicability::Unspecified;
55-
let val_name = snippet_with_context(cx, self_arg.span, expr.span.ctxt(), "..", &mut app).0;
66+
let mut app = Applicability::MachineApplicable;
67+
let val_name = snippet_with_context(cx, self_arg.span, ctxt, "..", &mut app).0;
5668
let method_name = if is_type_diagnostic_item(cx, ty, sym::cstring_type) {
5769
"as_bytes"
5870
} else if is_type_diagnostic_item(cx, ty, sym::CStr) {
@@ -64,11 +76,11 @@ impl LateLintPass<'tcx> for StrlenOnCStrings {
6476
span_lint_and_sugg(
6577
cx,
6678
STRLEN_ON_C_STRINGS,
67-
expr.span,
79+
span,
6880
"using `libc::strlen` on a `CString` or `CStr` value",
69-
"try this (you might also need to get rid of `unsafe` block in some cases):",
81+
"try this",
7082
format!("{}.{}().len()", val_name, method_name),
71-
Applicability::Unspecified // Sometimes unnecessary `unsafe` block
83+
app,
7284
);
7385
}
7486
}

‎clippy_utils/src/visitors.rs

Lines changed: 65 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
use crate::path_to_local_id;
22
use rustc_hir as hir;
33
use rustc_hir::def::{DefKind, Res};
4-
use rustc_hir::intravisit::{self, walk_expr, NestedVisitorMap, Visitor};
5-
use rustc_hir::{Arm, Block, Body, BodyId, Expr, ExprKind, HirId, Stmt, UnOp};
4+
use rustc_hir::intravisit::{self, walk_block, walk_expr, NestedVisitorMap, Visitor};
5+
use rustc_hir::{
6+
Arm, Block, BlockCheckMode, Body, BodyId, Expr, ExprKind, HirId, ItemId, ItemKind, Stmt, UnOp, Unsafety,
7+
};
68
use rustc_lint::LateContext;
79
use rustc_middle::hir::map::Map;
810
use rustc_middle::ty;
@@ -317,3 +319,64 @@ pub fn is_const_evaluatable(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
317319
v.visit_expr(e);
318320
v.is_const
319321
}
322+
323+
/// Checks if the given expression performs an unsafe operation outside of an unsafe block.
324+
pub fn is_expr_unsafe(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool {
325+
struct V<'a, 'tcx> {
326+
cx: &'a LateContext<'tcx>,
327+
is_unsafe: bool,
328+
}
329+
impl<'tcx> Visitor<'tcx> for V<'_, 'tcx> {
330+
type Map = Map<'tcx>;
331+
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
332+
NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
333+
}
334+
fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
335+
if self.is_unsafe {
336+
return;
337+
}
338+
match e.kind {
339+
ExprKind::Unary(UnOp::Deref, e) if self.cx.typeck_results().expr_ty(e).is_unsafe_ptr() => {
340+
self.is_unsafe = true;
341+
},
342+
ExprKind::MethodCall(..)
343+
if self
344+
.cx
345+
.typeck_results()
346+
.type_dependent_def_id(e.hir_id)
347+
.map_or(false, |id| self.cx.tcx.fn_sig(id).unsafety() == Unsafety::Unsafe) =>
348+
{
349+
self.is_unsafe = true;
350+
},
351+
ExprKind::Call(func, _) => match *self.cx.typeck_results().expr_ty(func).peel_refs().kind() {
352+
ty::FnDef(id, _) if self.cx.tcx.fn_sig(id).unsafety() == Unsafety::Unsafe => self.is_unsafe = true,
353+
ty::FnPtr(sig) if sig.unsafety() == Unsafety::Unsafe => self.is_unsafe = true,
354+
_ => walk_expr(self, e),
355+
},
356+
ExprKind::Path(ref p)
357+
if self
358+
.cx
359+
.qpath_res(p, e.hir_id)
360+
.opt_def_id()
361+
.map_or(false, |id| self.cx.tcx.is_mutable_static(id)) =>
362+
{
363+
self.is_unsafe = true;
364+
},
365+
_ => walk_expr(self, e),
366+
}
367+
}
368+
fn visit_block(&mut self, b: &'tcx Block<'_>) {
369+
if !matches!(b.rules, BlockCheckMode::UnsafeBlock(_)) {
370+
walk_block(self, b);
371+
}
372+
}
373+
fn visit_nested_item(&mut self, id: ItemId) {
374+
if let ItemKind::Impl(i) = &self.cx.tcx.hir().item(id).kind {
375+
self.is_unsafe = i.unsafety == Unsafety::Unsafe;
376+
}
377+
}
378+
}
379+
let mut v = V { cx, is_unsafe: false };
380+
v.visit_expr(e);
381+
v.is_unsafe
382+
}

‎tests/ui/strlen_on_c_strings.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,16 @@ fn main() {
1616
let len = unsafe { libc::strlen(cstr.as_ptr()) };
1717

1818
let len = unsafe { strlen(cstr.as_ptr()) };
19+
20+
let pcstr: *const &CStr = &cstr;
21+
let len = unsafe { strlen((*pcstr).as_ptr()) };
22+
23+
unsafe fn unsafe_identity<T>(x: T) -> T {
24+
x
25+
}
26+
let len = unsafe { strlen(unsafe_identity(cstr).as_ptr()) };
27+
let len = unsafe { strlen(unsafe { unsafe_identity(cstr) }.as_ptr()) };
28+
29+
let f: unsafe fn(_) -> _ = unsafe_identity;
30+
let len = unsafe { strlen(f(cstr).as_ptr()) };
1931
}

‎tests/ui/strlen_on_c_strings.stderr

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,46 @@
11
error: using `libc::strlen` on a `CString` or `CStr` value
2-
--> $DIR/strlen_on_c_strings.rs:12:24
2+
--> $DIR/strlen_on_c_strings.rs:12:15
33
|
44
LL | let len = unsafe { libc::strlen(cstring.as_ptr()) };
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `cstring.as_bytes().len()`
66
|
77
= note: `-D clippy::strlen-on-c-strings` implied by `-D warnings`
8-
help: try this (you might also need to get rid of `unsafe` block in some cases):
9-
|
10-
LL | let len = unsafe { cstring.as_bytes().len() };
11-
| ~~~~~~~~~~~~~~~~~~~~~~~~
128

139
error: using `libc::strlen` on a `CString` or `CStr` value
14-
--> $DIR/strlen_on_c_strings.rs:16:24
10+
--> $DIR/strlen_on_c_strings.rs:16:15
1511
|
1612
LL | let len = unsafe { libc::strlen(cstr.as_ptr()) };
17-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
13+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `cstr.to_bytes().len()`
14+
15+
error: using `libc::strlen` on a `CString` or `CStr` value
16+
--> $DIR/strlen_on_c_strings.rs:18:15
1817
|
19-
help: try this (you might also need to get rid of `unsafe` block in some cases):
18+
LL | let len = unsafe { strlen(cstr.as_ptr()) };
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `cstr.to_bytes().len()`
20+
21+
error: using `libc::strlen` on a `CString` or `CStr` value
22+
--> $DIR/strlen_on_c_strings.rs:21:24
2023
|
21-
LL | let len = unsafe { cstr.to_bytes().len() };
22-
| ~~~~~~~~~~~~~~~~~~~~~
24+
LL | let len = unsafe { strlen((*pcstr).as_ptr()) };
25+
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(*pcstr).to_bytes().len()`
2326

2427
error: using `libc::strlen` on a `CString` or `CStr` value
25-
--> $DIR/strlen_on_c_strings.rs:18:24
28+
--> $DIR/strlen_on_c_strings.rs:26:24
2629
|
27-
LL | let len = unsafe { strlen(cstr.as_ptr()) };
28-
| ^^^^^^^^^^^^^^^^^^^^^
30+
LL | let len = unsafe { strlen(unsafe_identity(cstr).as_ptr()) };
31+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unsafe_identity(cstr).to_bytes().len()`
32+
33+
error: using `libc::strlen` on a `CString` or `CStr` value
34+
--> $DIR/strlen_on_c_strings.rs:27:15
2935
|
30-
help: try this (you might also need to get rid of `unsafe` block in some cases):
36+
LL | let len = unsafe { strlen(unsafe { unsafe_identity(cstr) }.as_ptr()) };
37+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unsafe { unsafe_identity(cstr) }.to_bytes().len()`
38+
39+
error: using `libc::strlen` on a `CString` or `CStr` value
40+
--> $DIR/strlen_on_c_strings.rs:30:24
3141
|
32-
LL | let len = unsafe { cstr.to_bytes().len() };
33-
| ~~~~~~~~~~~~~~~~~~~~~
42+
LL | let len = unsafe { strlen(f(cstr).as_ptr()) };
43+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `f(cstr).to_bytes().len()`
3444

35-
error: aborting due to 3 previous errors
45+
error: aborting due to 7 previous errors
3646

0 commit comments

Comments
 (0)