Skip to content

Commit 7f22b1c

Browse files
committed
Auto merge of #6394 - nico-abram:unsafe_sizeof_count_copies, r=ebroto
Add lint size_of_in_element_count Fixes #6381 changelog: Add lint to check for using size_of::<T> or size_of_val::<T> in the count parameter to ptr::copy or ptr::copy_nonoverlapping, which take a count of Ts (And not a count of bytes) - \[X] Followed [lint naming conventions][lint_naming] - \[X] Added passing UI tests (including committed `.stderr` file) - \[ ] `cargo test` passes locally - \[X] Executed `cargo dev update_lints` - \[X] Added lint documentation - \[X] Run `cargo dev fmt` [lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints Running `cargo test` locally fails with this error: ``` running 1 test test fmt ... FAILED failures: ---- fmt stdout ---- status: exit code: 1 stdout: stderr: error: unable to unlink old fallback exe error: caused by: Access is denied. (os error 5) thread 'fmt' panicked at 'Formatting check failed. Run `cargo dev fmt` to update formatting.', tests\fmt.rs:32:5 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: fmt test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out ``` But I did run `cargo dev fmt`
2 parents 249b6fe + c1a5329 commit 7f22b1c

File tree

6 files changed

+418
-0
lines changed

6 files changed

+418
-0
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -2057,6 +2057,7 @@ Released 2018-09-13
20572057
[`single_element_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_element_loop
20582058
[`single_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match
20592059
[`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
2060+
[`size_of_in_element_count`]: https://rust-lang.github.io/rust-clippy/master/index.html#size_of_in_element_count
20602061
[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
20612062
[`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
20622063
[`stable_sort_primitive`]: https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive

clippy_lints/src/lib.rs

+5
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,7 @@ mod self_assignment;
306306
mod serde_api;
307307
mod shadow;
308308
mod single_component_path_imports;
309+
mod size_of_in_element_count;
309310
mod slow_vector_initialization;
310311
mod stable_sort_primitive;
311312
mod strings;
@@ -847,6 +848,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
847848
&shadow::SHADOW_SAME,
848849
&shadow::SHADOW_UNRELATED,
849850
&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS,
851+
&size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT,
850852
&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION,
851853
&stable_sort_primitive::STABLE_SORT_PRIMITIVE,
852854
&strings::STRING_ADD,
@@ -998,6 +1000,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
9981000
store.register_late_pass(move || box matches::Matches::new(msrv));
9991001
store.register_early_pass(move || box manual_non_exhaustive::ManualNonExhaustive::new(msrv));
10001002
store.register_late_pass(move || box manual_strip::ManualStrip::new(msrv));
1003+
store.register_late_pass(|| box size_of_in_element_count::SizeOfInElementCount);
10011004
store.register_late_pass(|| box map_clone::MapClone);
10021005
store.register_late_pass(|| box map_err_ignore::MapErrIgnore);
10031006
store.register_late_pass(|| box shadow::Shadow);
@@ -1559,6 +1562,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
15591562
LintId::of(&self_assignment::SELF_ASSIGNMENT),
15601563
LintId::of(&serde_api::SERDE_API_MISUSE),
15611564
LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS),
1565+
LintId::of(&size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT),
15621566
LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION),
15631567
LintId::of(&stable_sort_primitive::STABLE_SORT_PRIMITIVE),
15641568
LintId::of(&strings::STRING_FROM_UTF8_AS_BYTES),
@@ -1868,6 +1872,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
18681872
LintId::of(&regex::INVALID_REGEX),
18691873
LintId::of(&self_assignment::SELF_ASSIGNMENT),
18701874
LintId::of(&serde_api::SERDE_API_MISUSE),
1875+
LintId::of(&size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT),
18711876
LintId::of(&suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
18721877
LintId::of(&suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
18731878
LintId::of(&swap::ALMOST_SWAPPED),
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
//! Lint on use of `size_of` or `size_of_val` of T in an expression
2+
//! expecting a count of T
3+
4+
use crate::utils::{match_def_path, paths, span_lint_and_help};
5+
use if_chain::if_chain;
6+
use rustc_hir::BinOpKind;
7+
use rustc_hir::{Expr, ExprKind};
8+
use rustc_lint::{LateContext, LateLintPass};
9+
use rustc_middle::ty::{self, Ty, TyS, TypeAndMut};
10+
use rustc_session::{declare_lint_pass, declare_tool_lint};
11+
12+
declare_clippy_lint! {
13+
/// **What it does:** Detects expressions where
14+
/// size_of::<T> or size_of_val::<T> is used as a
15+
/// count of elements of type T
16+
///
17+
/// **Why is this bad?** These functions expect a count
18+
/// of T and not a number of bytes
19+
///
20+
/// **Known problems:** None.
21+
///
22+
/// **Example:**
23+
/// ```rust,no_run
24+
/// # use std::ptr::copy_nonoverlapping;
25+
/// # use std::mem::size_of;
26+
///
27+
/// const SIZE: usize = 128;
28+
/// let x = [2u8; SIZE];
29+
/// let mut y = [2u8; SIZE];
30+
/// unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>() * SIZE) };
31+
/// ```
32+
pub SIZE_OF_IN_ELEMENT_COUNT,
33+
correctness,
34+
"using size_of::<T> or size_of_val::<T> where a count of elements of T is expected"
35+
}
36+
37+
declare_lint_pass!(SizeOfInElementCount => [SIZE_OF_IN_ELEMENT_COUNT]);
38+
39+
fn get_size_of_ty(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<Ty<'tcx>> {
40+
match expr.kind {
41+
ExprKind::Call(count_func, _func_args) => {
42+
if_chain! {
43+
if let ExprKind::Path(ref count_func_qpath) = count_func.kind;
44+
if let Some(def_id) = cx.qpath_res(count_func_qpath, count_func.hir_id).opt_def_id();
45+
if match_def_path(cx, def_id, &paths::MEM_SIZE_OF)
46+
|| match_def_path(cx, def_id, &paths::MEM_SIZE_OF_VAL);
47+
then {
48+
cx.typeck_results().node_substs(count_func.hir_id).types().next()
49+
} else {
50+
None
51+
}
52+
}
53+
},
54+
ExprKind::Binary(op, left, right) if BinOpKind::Mul == op.node || BinOpKind::Div == op.node => {
55+
get_size_of_ty(cx, left).or_else(|| get_size_of_ty(cx, right))
56+
},
57+
ExprKind::Cast(expr, _) => get_size_of_ty(cx, expr),
58+
_ => None,
59+
}
60+
}
61+
62+
fn get_pointee_ty_and_count_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<(Ty<'tcx>, &'tcx Expr<'tcx>)> {
63+
const FUNCTIONS: [&[&str]; 8] = [
64+
&paths::COPY_NONOVERLAPPING,
65+
&paths::COPY,
66+
&paths::WRITE_BYTES,
67+
&paths::PTR_SWAP_NONOVERLAPPING,
68+
&paths::PTR_SLICE_FROM_RAW_PARTS,
69+
&paths::PTR_SLICE_FROM_RAW_PARTS_MUT,
70+
&paths::SLICE_FROM_RAW_PARTS,
71+
&paths::SLICE_FROM_RAW_PARTS_MUT,
72+
];
73+
const METHODS: [&str; 11] = [
74+
"write_bytes",
75+
"copy_to",
76+
"copy_from",
77+
"copy_to_nonoverlapping",
78+
"copy_from_nonoverlapping",
79+
"add",
80+
"wrapping_add",
81+
"sub",
82+
"wrapping_sub",
83+
"offset",
84+
"wrapping_offset",
85+
];
86+
87+
if_chain! {
88+
// Find calls to ptr::{copy, copy_nonoverlapping}
89+
// and ptr::{swap_nonoverlapping, write_bytes},
90+
if let ExprKind::Call(func, [.., count]) = expr.kind;
91+
if let ExprKind::Path(ref func_qpath) = func.kind;
92+
if let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id();
93+
if FUNCTIONS.iter().any(|func_path| match_def_path(cx, def_id, func_path));
94+
95+
// Get the pointee type
96+
if let Some(pointee_ty) = cx.typeck_results().node_substs(func.hir_id).types().next();
97+
then {
98+
return Some((pointee_ty, count));
99+
}
100+
};
101+
if_chain! {
102+
// Find calls to copy_{from,to}{,_nonoverlapping} and write_bytes methods
103+
if let ExprKind::MethodCall(method_path, _, [ptr_self, .., count], _) = expr.kind;
104+
let method_ident = method_path.ident.as_str();
105+
if METHODS.iter().any(|m| *m == &*method_ident);
106+
107+
// Get the pointee type
108+
if let ty::RawPtr(TypeAndMut { ty: pointee_ty, .. }) =
109+
cx.typeck_results().expr_ty(ptr_self).kind();
110+
then {
111+
return Some((pointee_ty, count));
112+
}
113+
};
114+
None
115+
}
116+
117+
impl<'tcx> LateLintPass<'tcx> for SizeOfInElementCount {
118+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
119+
const HELP_MSG: &str = "use a count of elements instead of a count of bytes\
120+
, it already gets multiplied by the size of the type";
121+
122+
const LINT_MSG: &str = "found a count of bytes \
123+
instead of a count of elements of T";
124+
125+
if_chain! {
126+
// Find calls to functions with an element count parameter and get
127+
// the pointee type and count parameter expression
128+
if let Some((pointee_ty, count_expr)) = get_pointee_ty_and_count_expr(cx, expr);
129+
130+
// Find a size_of call in the count parameter expression and
131+
// check that it's the same type
132+
if let Some(ty_used_for_size_of) = get_size_of_ty(cx, count_expr);
133+
if TyS::same_type(pointee_ty, ty_used_for_size_of);
134+
then {
135+
span_lint_and_help(
136+
cx,
137+
SIZE_OF_IN_ELEMENT_COUNT,
138+
count_expr.span,
139+
LINT_MSG,
140+
None,
141+
HELP_MSG
142+
);
143+
}
144+
};
145+
}
146+
}

clippy_lints/src/utils/paths.rs

+10
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ pub const CLONE_TRAIT: [&str; 3] = ["core", "clone", "Clone"];
2020
pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"];
2121
pub const CMP_MAX: [&str; 3] = ["core", "cmp", "max"];
2222
pub const CMP_MIN: [&str; 3] = ["core", "cmp", "min"];
23+
pub const COPY: [&str; 3] = ["core", "intrinsics", "copy_nonoverlapping"];
24+
pub const COPY_NONOVERLAPPING: [&str; 3] = ["core", "intrinsics", "copy"];
2325
pub const COW: [&str; 3] = ["alloc", "borrow", "Cow"];
2426
pub const CSTRING_AS_C_STR: [&str; 5] = ["std", "ffi", "c_str", "CString", "as_c_str"];
2527
pub const DEFAULT_TRAIT: [&str; 3] = ["core", "default", "Default"];
@@ -73,6 +75,8 @@ pub const MEM_MANUALLY_DROP: [&str; 4] = ["core", "mem", "manually_drop", "Manua
7375
pub const MEM_MAYBEUNINIT: [&str; 4] = ["core", "mem", "maybe_uninit", "MaybeUninit"];
7476
pub const MEM_MAYBEUNINIT_UNINIT: [&str; 5] = ["core", "mem", "maybe_uninit", "MaybeUninit", "uninit"];
7577
pub const MEM_REPLACE: [&str; 3] = ["core", "mem", "replace"];
78+
pub const MEM_SIZE_OF: [&str; 3] = ["core", "mem", "size_of"];
79+
pub const MEM_SIZE_OF_VAL: [&str; 3] = ["core", "mem", "size_of_val"];
7680
pub const MUTEX_GUARD: [&str; 4] = ["std", "sync", "mutex", "MutexGuard"];
7781
pub const OPEN_OPTIONS: [&str; 3] = ["std", "fs", "OpenOptions"];
7882
pub const OPS_MODULE: [&str; 2] = ["core", "ops"];
@@ -100,6 +104,9 @@ pub const POLL_READY: [&str; 5] = ["core", "task", "poll", "Poll", "Ready"];
100104
pub const PTR_EQ: [&str; 3] = ["core", "ptr", "eq"];
101105
pub const PTR_NULL: [&str; 3] = ["core", "ptr", "null"];
102106
pub const PTR_NULL_MUT: [&str; 3] = ["core", "ptr", "null_mut"];
107+
pub const PTR_SLICE_FROM_RAW_PARTS: [&str; 3] = ["core", "ptr", "slice_from_raw_parts"];
108+
pub const PTR_SLICE_FROM_RAW_PARTS_MUT: [&str; 3] = ["core", "ptr", "slice_from_raw_parts_mut"];
109+
pub const PTR_SWAP_NONOVERLAPPING: [&str; 3] = ["core", "ptr", "swap_nonoverlapping"];
103110
pub const PUSH_STR: [&str; 4] = ["alloc", "string", "String", "push_str"];
104111
pub const RANGE_ARGUMENT_TRAIT: [&str; 3] = ["core", "ops", "RangeBounds"];
105112
pub const RC: [&str; 3] = ["alloc", "rc", "Rc"];
@@ -121,6 +128,8 @@ pub const RWLOCK_READ_GUARD: [&str; 4] = ["std", "sync", "rwlock", "RwLockReadGu
121128
pub const RWLOCK_WRITE_GUARD: [&str; 4] = ["std", "sync", "rwlock", "RwLockWriteGuard"];
122129
pub const SERDE_DESERIALIZE: [&str; 3] = ["serde", "de", "Deserialize"];
123130
pub const SERDE_DE_VISITOR: [&str; 3] = ["serde", "de", "Visitor"];
131+
pub const SLICE_FROM_RAW_PARTS: [&str; 4] = ["core", "slice", "raw", "from_raw_parts"];
132+
pub const SLICE_FROM_RAW_PARTS_MUT: [&str; 4] = ["core", "slice", "raw", "from_raw_parts_mut"];
124133
pub const SLICE_INTO_VEC: [&str; 4] = ["alloc", "slice", "<impl [T]>", "into_vec"];
125134
pub const SLICE_ITER: [&str; 4] = ["core", "slice", "iter", "Iter"];
126135
pub const STDERR: [&str; 4] = ["std", "io", "stdio", "stderr"];
@@ -154,3 +163,4 @@ pub const VEC_NEW: [&str; 4] = ["alloc", "vec", "Vec", "new"];
154163
pub const VEC_RESIZE: [&str; 4] = ["alloc", "vec", "Vec", "resize"];
155164
pub const WEAK_ARC: [&str; 3] = ["alloc", "sync", "Weak"];
156165
pub const WEAK_RC: [&str; 3] = ["alloc", "rc", "Weak"];
166+
pub const WRITE_BYTES: [&str; 3] = ["core", "intrinsics", "write_bytes"];

tests/ui/size_of_in_element_count.rs

+61
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
#![warn(clippy::size_of_in_element_count)]
2+
#![allow(clippy::ptr_offset_with_cast)]
3+
4+
use std::mem::{size_of, size_of_val};
5+
use std::ptr::{
6+
copy, copy_nonoverlapping, slice_from_raw_parts, slice_from_raw_parts_mut, swap_nonoverlapping, write_bytes,
7+
};
8+
use std::slice::{from_raw_parts, from_raw_parts_mut};
9+
10+
fn main() {
11+
const SIZE: usize = 128;
12+
const HALF_SIZE: usize = SIZE / 2;
13+
const DOUBLE_SIZE: usize = SIZE * 2;
14+
let mut x = [2u8; SIZE];
15+
let mut y = [2u8; SIZE];
16+
17+
// Count is size_of (Should trigger the lint)
18+
unsafe { copy_nonoverlapping::<u8>(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>()) };
19+
unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0])) };
20+
21+
unsafe { x.as_ptr().copy_to(y.as_mut_ptr(), size_of::<u8>()) };
22+
unsafe { x.as_ptr().copy_to_nonoverlapping(y.as_mut_ptr(), size_of::<u8>()) };
23+
unsafe { y.as_mut_ptr().copy_from(x.as_ptr(), size_of::<u8>()) };
24+
unsafe { y.as_mut_ptr().copy_from_nonoverlapping(x.as_ptr(), size_of::<u8>()) };
25+
26+
unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>()) };
27+
unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0])) };
28+
29+
unsafe { y.as_mut_ptr().write_bytes(0u8, size_of::<u8>() * SIZE) };
30+
unsafe { write_bytes(y.as_mut_ptr(), 0u8, size_of::<u8>() * SIZE) };
31+
32+
unsafe { swap_nonoverlapping(y.as_mut_ptr(), x.as_mut_ptr(), size_of::<u8>() * SIZE) };
33+
34+
slice_from_raw_parts_mut(y.as_mut_ptr(), size_of::<u8>() * SIZE);
35+
slice_from_raw_parts(y.as_ptr(), size_of::<u8>() * SIZE);
36+
37+
unsafe { from_raw_parts_mut(y.as_mut_ptr(), size_of::<u8>() * SIZE) };
38+
unsafe { from_raw_parts(y.as_ptr(), size_of::<u8>() * SIZE) };
39+
40+
unsafe { y.as_mut_ptr().sub(size_of::<u8>()) };
41+
y.as_ptr().wrapping_sub(size_of::<u8>());
42+
unsafe { y.as_ptr().add(size_of::<u8>()) };
43+
y.as_mut_ptr().wrapping_add(size_of::<u8>());
44+
unsafe { y.as_ptr().offset(size_of::<u8>() as isize) };
45+
y.as_mut_ptr().wrapping_offset(size_of::<u8>() as isize);
46+
47+
// Count expression involving multiplication of size_of (Should trigger the lint)
48+
unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>() * SIZE) };
49+
50+
// Count expression involving nested multiplications of size_of (Should trigger the lint)
51+
unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), HALF_SIZE * size_of_val(&x[0]) * 2) };
52+
53+
// Count expression involving divisions of size_of (Should trigger the lint)
54+
unsafe { copy(x.as_ptr(), y.as_mut_ptr(), DOUBLE_SIZE * size_of::<u8>() / 2) };
55+
56+
// No size_of calls (Should not trigger the lint)
57+
unsafe { copy(x.as_ptr(), y.as_mut_ptr(), SIZE) };
58+
59+
// Different types for pointee and size_of (Should not trigger the lint)
60+
unsafe { y.as_mut_ptr().write_bytes(0u8, size_of::<u16>() / 2 * SIZE) };
61+
}

0 commit comments

Comments
 (0)