Skip to content

Commit 9cc3494

Browse files
QwazQwaz
committed
Implement uninit_vec lint
1 parent fe999e8 commit 9cc3494

File tree

12 files changed

+278
-12
lines changed

12 files changed

+278
-12
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3034,6 +3034,7 @@ Released 2018-09-13
30343034
[`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
30353035
[`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
30363036
[`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
3037+
[`uninit_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_vec
30373038
[`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
30383039
[`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp
30393040
[`unit_return_expecting_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_return_expecting_ord

‎clippy_lints/src/lib.mods.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,7 @@ mod try_err;
204204
mod types;
205205
mod undropped_manually_drops;
206206
mod unicode;
207+
mod uninit_vec;
207208
mod unit_return_expecting_ord;
208209
mod unit_types;
209210
mod unnamed_address;

‎clippy_lints/src/lib.register_all.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
278278
LintId::of(types::VEC_BOX),
279279
LintId::of(undropped_manually_drops::UNDROPPED_MANUALLY_DROPS),
280280
LintId::of(unicode::INVISIBLE_CHARACTERS),
281+
LintId::of(uninit_vec::UNINIT_VEC),
281282
LintId::of(unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD),
282283
LintId::of(unit_types::UNIT_ARG),
283284
LintId::of(unit_types::UNIT_CMP),

‎clippy_lints/src/lib.register_correctness.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve
6363
LintId::of(transmuting_null::TRANSMUTING_NULL),
6464
LintId::of(undropped_manually_drops::UNDROPPED_MANUALLY_DROPS),
6565
LintId::of(unicode::INVISIBLE_CHARACTERS),
66+
LintId::of(uninit_vec::UNINIT_VEC),
6667
LintId::of(unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD),
6768
LintId::of(unit_types::UNIT_CMP),
6869
LintId::of(unnamed_address::FN_ADDRESS_COMPARISONS),

‎clippy_lints/src/lib.register_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -466,6 +466,7 @@ store.register_lints(&[
466466
unicode::INVISIBLE_CHARACTERS,
467467
unicode::NON_ASCII_LITERAL,
468468
unicode::UNICODE_NOT_NFC,
469+
uninit_vec::UNINIT_VEC,
469470
unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD,
470471
unit_types::LET_UNIT_VALUE,
471472
unit_types::UNIT_ARG,

‎clippy_lints/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
286286
store.register_late_pass(|| Box::new(blocks_in_if_conditions::BlocksInIfConditions));
287287
store.register_late_pass(|| Box::new(collapsible_match::CollapsibleMatch));
288288
store.register_late_pass(|| Box::new(unicode::Unicode));
289+
store.register_late_pass(|| Box::new(uninit_vec::UninitVec));
289290
store.register_late_pass(|| Box::new(unit_return_expecting_ord::UnitReturnExpectingOrd));
290291
store.register_late_pass(|| Box::new(strings::StringAdd));
291292
store.register_late_pass(|| Box::new(implicit_return::ImplicitReturn));

‎clippy_lints/src/methods/uninit_assumed_init.rs

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
use clippy_utils::diagnostics::span_lint;
2-
use clippy_utils::{is_expr_path_def_path, match_def_path, paths};
2+
use clippy_utils::{is_expr_path_def_path, is_uninit_ty_valid, paths};
33
use if_chain::if_chain;
44
use rustc_hir as hir;
55
use rustc_lint::LateContext;
6-
use rustc_middle::ty::{self, Ty};
76

87
use super::UNINIT_ASSUMED_INIT;
98

@@ -13,7 +12,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr
1312
if let hir::ExprKind::Call(callee, args) = recv.kind;
1413
if args.is_empty();
1514
if is_expr_path_def_path(cx, callee, &paths::MEM_MAYBEUNINIT_UNINIT);
16-
if !is_maybe_uninit_ty_valid(cx, cx.typeck_results().expr_ty_adjusted(expr));
15+
if !is_uninit_ty_valid(cx, cx.typeck_results().expr_ty_adjusted(expr));
1716
then {
1817
span_lint(
1918
cx,
@@ -24,12 +23,3 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, recv: &hir::Expr
2423
}
2524
}
2625
}
27-
28-
fn is_maybe_uninit_ty_valid(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
29-
match ty.kind() {
30-
ty::Array(component, _) => is_maybe_uninit_ty_valid(cx, component),
31-
ty::Tuple(types) => types.types().all(|ty| is_maybe_uninit_ty_valid(cx, ty)),
32-
ty::Adt(adt, _) => match_def_path(cx, adt.did, &paths::MEM_MAYBEUNINIT),
33-
_ => false,
34-
}
35-
}

‎clippy_lints/src/uninit_vec.rs

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
use clippy_utils::diagnostics::span_lint_and_note;
2+
use clippy_utils::{is_uninit_ty_valid, match_def_path, path_to_local_id, paths, peel_hir_expr_while, SpanlessEq};
3+
use rustc_hir::def::Res;
4+
use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, Stmt, StmtKind};
5+
use rustc_lint::{LateContext, LateLintPass};
6+
use rustc_middle::ty;
7+
use rustc_session::{declare_lint_pass, declare_tool_lint};
8+
use rustc_span::Span;
9+
10+
declare_clippy_lint! {
11+
/// ### What it does
12+
/// Checks for the creation of uninitialized `Vec<T>` by calling `set_len()`
13+
/// immediately after `with_capacity()` or `reserve()`.
14+
///
15+
/// ### Why is this bad?
16+
/// It creates `Vec<T>` that contains uninitialized data, which leads to an
17+
/// undefined behavior with most safe operations.
18+
/// Notably, using uninitialized `Vec<u8>` with generic `Read` is unsound.
19+
///
20+
/// ### Example
21+
/// ```rust,ignore
22+
/// let mut vec: Vec<u8> = Vec::with_capacity(1000);
23+
/// unsafe { vec.set_len(1000); }
24+
/// reader.read(&mut vec); // undefined behavior!
25+
/// ```
26+
/// Use an initialized buffer:
27+
/// ```rust,ignore
28+
/// let mut vec: Vec<u8> = vec![0; 1000];
29+
/// reader.read(&mut vec);
30+
/// ```
31+
/// Or, wrap the content in `MaybeUninit`:
32+
/// ```rust,ignore
33+
/// let mut vec: Vec<MaybeUninit<T>> = Vec::with_capacity(1000);
34+
/// unsafe { vec.set_len(1000); }
35+
/// ```
36+
pub UNINIT_VEC,
37+
correctness,
38+
"Vec with uninitialized data"
39+
}
40+
41+
declare_lint_pass!(UninitVec => [UNINIT_VEC]);
42+
43+
impl<'tcx> LateLintPass<'tcx> for UninitVec {
44+
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) {
45+
for w in block.stmts.windows(2) {
46+
if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = w[1].kind {
47+
handle_pair(cx, &w[0], expr);
48+
}
49+
}
50+
51+
if let (Some(stmt), Some(expr)) = (block.stmts.last(), block.expr) {
52+
handle_pair(cx, stmt, expr);
53+
}
54+
}
55+
}
56+
57+
fn handle_pair(cx: &LateContext<'tcx>, first: &'tcx Stmt<'tcx>, second: &'tcx Expr<'tcx>) {
58+
if_chain! {
59+
if let Some(vec) = extract_with_capacity_or_reserve_target(cx, first);
60+
if let Some((set_len_self, call_span)) = extract_set_len_self(cx, second);
61+
if vec.eq_expr(cx, set_len_self);
62+
if let ty::Ref(_, vec_ty, _) = cx.typeck_results().expr_ty_adjusted(set_len_self).kind();
63+
if let ty::Adt(_, substs) = vec_ty.kind();
64+
// Check T of Vec<T>
65+
if !is_uninit_ty_valid(cx, substs.type_at(0));
66+
then {
67+
span_lint_and_note(
68+
cx,
69+
UNINIT_VEC,
70+
call_span,
71+
"calling `set_len()` immediately after reserving a buffer creates uninitialized values",
72+
Some(first.span),
73+
"the buffer is reserved here"
74+
);
75+
}
76+
}
77+
}
78+
79+
#[derive(Clone, Copy, Debug)]
80+
enum LocalOrExpr<'tcx> {
81+
Local(HirId),
82+
Expr(&'tcx Expr<'tcx>),
83+
}
84+
85+
impl<'tcx> LocalOrExpr<'tcx> {
86+
fn eq_expr(self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
87+
match self {
88+
LocalOrExpr::Local(hir_id) => path_to_local_id(expr, hir_id),
89+
LocalOrExpr::Expr(self_expr) => SpanlessEq::new(cx).eq_expr(self_expr, expr),
90+
}
91+
}
92+
}
93+
94+
/// Returns the target vec of Vec::with_capacity() or Vec::reserve()
95+
fn extract_with_capacity_or_reserve_target(cx: &LateContext<'_>, stmt: &'tcx Stmt<'_>) -> Option<LocalOrExpr<'tcx>> {
96+
match stmt.kind {
97+
StmtKind::Local(local) => {
98+
// let mut x = Vec::with_capacity()
99+
if_chain! {
100+
if let Some(init_expr) = local.init;
101+
if let PatKind::Binding(_, hir_id, _, None) = local.pat.kind;
102+
if is_with_capacity(cx, init_expr);
103+
then {
104+
Some(LocalOrExpr::Local(hir_id))
105+
} else {
106+
None
107+
}
108+
}
109+
},
110+
StmtKind::Expr(expr) | StmtKind::Semi(expr) => {
111+
match expr.kind {
112+
ExprKind::Assign(lhs, rhs, _span) if is_with_capacity(cx, rhs) => {
113+
// self.vec = Vec::with_capacity()
114+
Some(LocalOrExpr::Expr(lhs))
115+
},
116+
ExprKind::MethodCall(_, _, [vec_expr, _], _) => {
117+
// self.vec.reserve()
118+
if_chain! {
119+
if let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id);
120+
if match_def_path(cx, id, &paths::VEC_RESERVE);
121+
then {
122+
Some(LocalOrExpr::Expr(vec_expr))
123+
} else {
124+
None
125+
}
126+
}
127+
},
128+
_ => None,
129+
}
130+
},
131+
_ => None,
132+
}
133+
}
134+
135+
fn is_with_capacity(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> bool {
136+
if_chain! {
137+
if let ExprKind::Call(path_expr, _) = &expr.kind;
138+
if let ExprKind::Path(qpath) = &path_expr.kind;
139+
if let Res::Def(_, def_id) = cx.qpath_res(qpath, path_expr.hir_id);
140+
then {
141+
match_def_path(cx, def_id, &paths::VEC_WITH_CAPACITY)
142+
} else {
143+
false
144+
}
145+
}
146+
}
147+
148+
/// Returns self if the expression is Vec::set_len()
149+
fn extract_set_len_self(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<(&'tcx Expr<'tcx>, Span)> {
150+
// peel unsafe blocks in `unsafe { vec.set_len() }`
151+
let expr = peel_hir_expr_while(expr, |e| {
152+
if let ExprKind::Block(block, _) = e.kind {
153+
match (block.stmts.get(0).map(|stmt| &stmt.kind), block.expr) {
154+
(None, Some(expr)) => Some(expr),
155+
(Some(StmtKind::Expr(expr) | StmtKind::Semi(expr)), None) => Some(expr),
156+
_ => None,
157+
}
158+
} else {
159+
None
160+
}
161+
});
162+
match expr.kind {
163+
ExprKind::MethodCall(_, _, [vec_expr, _], _) => {
164+
cx.typeck_results().type_dependent_def_id(expr.hir_id).and_then(|id| {
165+
if match_def_path(cx, id, &paths::VEC_SET_LEN) {
166+
Some((vec_expr, expr.span))
167+
} else {
168+
None
169+
}
170+
})
171+
},
172+
_ => None,
173+
}
174+
}

‎clippy_utils/src/lib.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1478,6 +1478,16 @@ pub fn is_self_ty(slf: &hir::Ty<'_>) -> bool {
14781478
false
14791479
}
14801480

1481+
/// Checks if a given type looks safe to be uninitialized.
1482+
pub fn is_uninit_ty_valid(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
1483+
match ty.kind() {
1484+
rustc_ty::Array(component, _) => is_uninit_ty_valid(cx, component),
1485+
rustc_ty::Tuple(types) => types.types().all(|ty| is_uninit_ty_valid(cx, ty)),
1486+
rustc_ty::Adt(adt, _) => match_def_path(cx, adt.did, &paths::MEM_MAYBEUNINIT),
1487+
_ => false,
1488+
}
1489+
}
1490+
14811491
pub fn iter_input_pats<'tcx>(decl: &FnDecl<'_>, body: &'tcx Body<'_>) -> impl Iterator<Item = &'tcx Param<'tcx>> {
14821492
(0..decl.inputs.len()).map(move |i| &body.params[i])
14831493
}

‎clippy_utils/src/paths.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,5 +177,8 @@ pub const VEC_AS_SLICE: [&str; 4] = ["alloc", "vec", "Vec", "as_slice"];
177177
pub const VEC_FROM_ELEM: [&str; 3] = ["alloc", "vec", "from_elem"];
178178
pub const VEC_NEW: [&str; 4] = ["alloc", "vec", "Vec", "new"];
179179
pub const VEC_RESIZE: [&str; 4] = ["alloc", "vec", "Vec", "resize"];
180+
pub const VEC_RESERVE: [&str; 4] = ["alloc", "vec", "Vec", "reserve"];
181+
pub const VEC_WITH_CAPACITY: [&str; 4] = ["alloc", "vec", "Vec", "with_capacity"];
182+
pub const VEC_SET_LEN: [&str; 4] = ["alloc", "vec", "Vec", "set_len"];
180183
pub const WEAK_ARC: [&str; 3] = ["alloc", "sync", "Weak"];
181184
pub const WEAK_RC: [&str; 3] = ["alloc", "rc", "Weak"];

‎tests/ui/uninit_vec.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
#![warn(clippy::uninit_vec)]
2+
3+
use std::mem::MaybeUninit;
4+
5+
fn main() {
6+
// with_capacity() -> set_len() should be detected
7+
let mut vec: Vec<u8> = Vec::with_capacity(1000);
8+
unsafe {
9+
vec.set_len(200);
10+
}
11+
12+
// reserve() -> set_len() should be detected
13+
vec.reserve(1000);
14+
unsafe {
15+
vec.set_len(200);
16+
}
17+
18+
// test when both calls are enclosed in the same unsafe block
19+
unsafe {
20+
let mut vec: Vec<u8> = Vec::with_capacity(1000);
21+
vec.set_len(200);
22+
23+
vec.reserve(1000);
24+
vec.set_len(200);
25+
}
26+
27+
// MaybeUninit-wrapped types should not be detected
28+
let mut vec: Vec<MaybeUninit<u8>> = Vec::with_capacity(1000);
29+
unsafe {
30+
vec.set_len(200);
31+
}
32+
}

‎tests/ui/uninit_vec.stderr

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
error: calling `set_len()` immediately after reserving a buffer creates uninitialized values
2+
--> $DIR/uninit_vec.rs:9:9
3+
|
4+
LL | vec.set_len(200);
5+
| ^^^^^^^^^^^^^^^^
6+
|
7+
= note: `-D clippy::uninit-vec` implied by `-D warnings`
8+
note: the buffer is reserved here
9+
--> $DIR/uninit_vec.rs:7:5
10+
|
11+
LL | let mut vec: Vec<u8> = Vec::with_capacity(1000);
12+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13+
14+
error: calling `set_len()` immediately after reserving a buffer creates uninitialized values
15+
--> $DIR/uninit_vec.rs:15:9
16+
|
17+
LL | vec.set_len(200);
18+
| ^^^^^^^^^^^^^^^^
19+
|
20+
note: the buffer is reserved here
21+
--> $DIR/uninit_vec.rs:13:5
22+
|
23+
LL | vec.reserve(1000);
24+
| ^^^^^^^^^^^^^^^^^^
25+
26+
error: calling `set_len()` immediately after reserving a buffer creates uninitialized values
27+
--> $DIR/uninit_vec.rs:21:9
28+
|
29+
LL | vec.set_len(200);
30+
| ^^^^^^^^^^^^^^^^
31+
|
32+
note: the buffer is reserved here
33+
--> $DIR/uninit_vec.rs:20:9
34+
|
35+
LL | let mut vec: Vec<u8> = Vec::with_capacity(1000);
36+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
37+
38+
error: calling `set_len()` immediately after reserving a buffer creates uninitialized values
39+
--> $DIR/uninit_vec.rs:24:9
40+
|
41+
LL | vec.set_len(200);
42+
| ^^^^^^^^^^^^^^^^
43+
|
44+
note: the buffer is reserved here
45+
--> $DIR/uninit_vec.rs:23:9
46+
|
47+
LL | vec.reserve(1000);
48+
| ^^^^^^^^^^^^^^^^^^
49+
50+
error: aborting due to 4 previous errors
51+

0 commit comments

Comments
 (0)