Skip to content

Commit 3d9c4a6

Browse files
committed
Auto merge of #7682 - Qwaz:uninit_vec, r=xFrednet
Implement uninit_vec lint changelog: add the new lint [`uninit_vec`] Fix #7681
2 parents d3905af + 4ed3a4f commit 3d9c4a6

12 files changed

+499
-60
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -3039,6 +3039,7 @@ Released 2018-09-13
30393039
[`unicode_not_nfc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unicode_not_nfc
30403040
[`unimplemented`]: https://rust-lang.github.io/rust-clippy/master/index.html#unimplemented
30413041
[`uninit_assumed_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_assumed_init
3042+
[`uninit_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_vec
30423043
[`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
30433044
[`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp
30443045
[`unit_return_expecting_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_return_expecting_ord

clippy_lints/src/lib.register_all.rs

+1
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
279279
LintId::of(types::VEC_BOX),
280280
LintId::of(undropped_manually_drops::UNDROPPED_MANUALLY_DROPS),
281281
LintId::of(unicode::INVISIBLE_CHARACTERS),
282+
LintId::of(uninit_vec::UNINIT_VEC),
282283
LintId::of(unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD),
283284
LintId::of(unit_types::UNIT_ARG),
284285
LintId::of(unit_types::UNIT_CMP),

clippy_lints/src/lib.register_correctness.rs

+1
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve
6464
LintId::of(transmuting_null::TRANSMUTING_NULL),
6565
LintId::of(undropped_manually_drops::UNDROPPED_MANUALLY_DROPS),
6666
LintId::of(unicode::INVISIBLE_CHARACTERS),
67+
LintId::of(uninit_vec::UNINIT_VEC),
6768
LintId::of(unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD),
6869
LintId::of(unit_types::UNIT_CMP),
6970
LintId::of(unnamed_address::FN_ADDRESS_COMPARISONS),

clippy_lints/src/lib.register_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -471,6 +471,7 @@ store.register_lints(&[
471471
unicode::INVISIBLE_CHARACTERS,
472472
unicode::NON_ASCII_LITERAL,
473473
unicode::UNICODE_NOT_NFC,
474+
uninit_vec::UNINIT_VEC,
474475
unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD,
475476
unit_types::LET_UNIT_VALUE,
476477
unit_types::UNIT_ARG,

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,7 @@ mod types;
362362
mod undocumented_unsafe_blocks;
363363
mod undropped_manually_drops;
364364
mod unicode;
365+
mod uninit_vec;
365366
mod unit_return_expecting_ord;
366367
mod unit_types;
367368
mod unnamed_address;
@@ -519,6 +520,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
519520
store.register_late_pass(|| Box::new(blocks_in_if_conditions::BlocksInIfConditions));
520521
store.register_late_pass(|| Box::new(collapsible_match::CollapsibleMatch));
521522
store.register_late_pass(|| Box::new(unicode::Unicode));
523+
store.register_late_pass(|| Box::new(uninit_vec::UninitVec));
522524
store.register_late_pass(|| Box::new(unit_return_expecting_ord::UnitReturnExpectingOrd));
523525
store.register_late_pass(|| Box::new(strings::StringAdd));
524526
store.register_late_pass(|| Box::new(implicit_return::ImplicitReturn));
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, paths, ty::is_uninit_value_valid_for_ty};
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_value_valid_for_ty(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

+223
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
2+
use clippy_utils::higher::{get_vec_init_kind, VecInitKind};
3+
use clippy_utils::ty::{is_type_diagnostic_item, is_uninit_value_valid_for_ty};
4+
use clippy_utils::{is_lint_allowed, path_to_local_id, peel_hir_expr_while, SpanlessEq};
5+
use rustc_hir::{Block, Expr, ExprKind, HirId, PatKind, PathSegment, Stmt, StmtKind};
6+
use rustc_lint::{LateContext, LateLintPass};
7+
use rustc_middle::lint::in_external_macro;
8+
use rustc_middle::ty;
9+
use rustc_session::{declare_lint_pass, declare_tool_lint};
10+
use rustc_span::{sym, Span};
11+
12+
// TODO: add `ReadBuf` (RFC 2930) in "How to fix" once it is available in std
13+
declare_clippy_lint! {
14+
/// ### What it does
15+
/// Checks for `set_len()` call that creates `Vec` with uninitialized elements.
16+
/// This is commonly caused by calling `set_len()` right after allocating or
17+
/// reserving a buffer with `new()`, `default()`, `with_capacity()`, or `reserve()`.
18+
///
19+
/// ### Why is this bad?
20+
/// It creates a `Vec` with uninitialized data, which leads to
21+
/// undefined behavior with most safe operations. Notably, uninitialized
22+
/// `Vec<u8>` must not be used with generic `Read`.
23+
///
24+
/// Moreover, calling `set_len()` on a `Vec` created with `new()` or `default()`
25+
/// creates out-of-bound values that lead to heap memory corruption when used.
26+
///
27+
/// ### Known Problems
28+
/// This lint only checks directly adjacent statements.
29+
///
30+
/// ### Example
31+
/// ```rust,ignore
32+
/// let mut vec: Vec<u8> = Vec::with_capacity(1000);
33+
/// unsafe { vec.set_len(1000); }
34+
/// reader.read(&mut vec); // undefined behavior!
35+
/// ```
36+
///
37+
/// ### How to fix?
38+
/// 1. Use an initialized buffer:
39+
/// ```rust,ignore
40+
/// let mut vec: Vec<u8> = vec![0; 1000];
41+
/// reader.read(&mut vec);
42+
/// ```
43+
/// 2. Wrap the content in `MaybeUninit`:
44+
/// ```rust,ignore
45+
/// let mut vec: Vec<MaybeUninit<T>> = Vec::with_capacity(1000);
46+
/// vec.set_len(1000); // `MaybeUninit` can be uninitialized
47+
/// ```
48+
/// 3. If you are on nightly, `Vec::spare_capacity_mut()` is available:
49+
/// ```rust,ignore
50+
/// let mut vec: Vec<u8> = Vec::with_capacity(1000);
51+
/// let remaining = vec.spare_capacity_mut(); // `&mut [MaybeUninit<u8>]`
52+
/// // perform initialization with `remaining`
53+
/// vec.set_len(...); // Safe to call `set_len()` on initialized part
54+
/// ```
55+
pub UNINIT_VEC,
56+
correctness,
57+
"Vec with uninitialized data"
58+
}
59+
60+
declare_lint_pass!(UninitVec => [UNINIT_VEC]);
61+
62+
// FIXME: update to a visitor-based implementation.
63+
// Threads: https://github.com/rust-lang/rust-clippy/pull/7682#discussion_r710998368
64+
impl<'tcx> LateLintPass<'tcx> for UninitVec {
65+
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) {
66+
if !in_external_macro(cx.tcx.sess, block.span) {
67+
for w in block.stmts.windows(2) {
68+
if let StmtKind::Expr(expr) | StmtKind::Semi(expr) = w[1].kind {
69+
handle_uninit_vec_pair(cx, &w[0], expr);
70+
}
71+
}
72+
73+
if let (Some(stmt), Some(expr)) = (block.stmts.last(), block.expr) {
74+
handle_uninit_vec_pair(cx, stmt, expr);
75+
}
76+
}
77+
}
78+
}
79+
80+
fn handle_uninit_vec_pair(
81+
cx: &LateContext<'tcx>,
82+
maybe_init_or_reserve: &'tcx Stmt<'tcx>,
83+
maybe_set_len: &'tcx Expr<'tcx>,
84+
) {
85+
if_chain! {
86+
if let Some(vec) = extract_init_or_reserve_target(cx, maybe_init_or_reserve);
87+
if let Some((set_len_self, call_span)) = extract_set_len_self(cx, maybe_set_len);
88+
if vec.location.eq_expr(cx, set_len_self);
89+
if let ty::Ref(_, vec_ty, _) = cx.typeck_results().expr_ty_adjusted(set_len_self).kind();
90+
if let ty::Adt(_, substs) = vec_ty.kind();
91+
// `#[allow(...)]` attribute can be set on enclosing unsafe block of `set_len()`
92+
if !is_lint_allowed(cx, UNINIT_VEC, maybe_set_len.hir_id);
93+
then {
94+
if vec.has_capacity() {
95+
// with_capacity / reserve -> set_len
96+
97+
// Check T of Vec<T>
98+
if !is_uninit_value_valid_for_ty(cx, substs.type_at(0)) {
99+
// FIXME: #7698, false positive of the internal lints
100+
#[allow(clippy::collapsible_span_lint_calls)]
101+
span_lint_and_then(
102+
cx,
103+
UNINIT_VEC,
104+
vec![call_span, maybe_init_or_reserve.span],
105+
"calling `set_len()` immediately after reserving a buffer creates uninitialized values",
106+
|diag| {
107+
diag.help("initialize the buffer or wrap the content in `MaybeUninit`");
108+
},
109+
);
110+
}
111+
} else {
112+
// new / default -> set_len
113+
span_lint(
114+
cx,
115+
UNINIT_VEC,
116+
vec![call_span, maybe_init_or_reserve.span],
117+
"calling `set_len()` on empty `Vec` creates out-of-bound values",
118+
);
119+
}
120+
}
121+
}
122+
}
123+
124+
/// The target `Vec` that is initialized or reserved
125+
#[derive(Clone, Copy)]
126+
struct TargetVec<'tcx> {
127+
location: VecLocation<'tcx>,
128+
/// `None` if `reserve()`
129+
init_kind: Option<VecInitKind>,
130+
}
131+
132+
impl TargetVec<'_> {
133+
pub fn has_capacity(self) -> bool {
134+
!matches!(self.init_kind, Some(VecInitKind::New | VecInitKind::Default))
135+
}
136+
}
137+
138+
#[derive(Clone, Copy)]
139+
enum VecLocation<'tcx> {
140+
Local(HirId),
141+
Expr(&'tcx Expr<'tcx>),
142+
}
143+
144+
impl<'tcx> VecLocation<'tcx> {
145+
pub fn eq_expr(self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
146+
match self {
147+
VecLocation::Local(hir_id) => path_to_local_id(expr, hir_id),
148+
VecLocation::Expr(self_expr) => SpanlessEq::new(cx).eq_expr(self_expr, expr),
149+
}
150+
}
151+
}
152+
153+
/// Finds the target location where the result of `Vec` initialization is stored
154+
/// or `self` expression for `Vec::reserve()`.
155+
fn extract_init_or_reserve_target<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) -> Option<TargetVec<'tcx>> {
156+
match stmt.kind {
157+
StmtKind::Local(local) => {
158+
if_chain! {
159+
if let Some(init_expr) = local.init;
160+
if let PatKind::Binding(_, hir_id, _, None) = local.pat.kind;
161+
if let Some(init_kind) = get_vec_init_kind(cx, init_expr);
162+
then {
163+
return Some(TargetVec {
164+
location: VecLocation::Local(hir_id),
165+
init_kind: Some(init_kind),
166+
})
167+
}
168+
}
169+
},
170+
StmtKind::Expr(expr) | StmtKind::Semi(expr) => match expr.kind {
171+
ExprKind::Assign(lhs, rhs, _span) => {
172+
if let Some(init_kind) = get_vec_init_kind(cx, rhs) {
173+
return Some(TargetVec {
174+
location: VecLocation::Expr(lhs),
175+
init_kind: Some(init_kind),
176+
});
177+
}
178+
},
179+
ExprKind::MethodCall(path, _, [self_expr, _], _) if is_reserve(cx, path, self_expr) => {
180+
return Some(TargetVec {
181+
location: VecLocation::Expr(self_expr),
182+
init_kind: None,
183+
});
184+
},
185+
_ => (),
186+
},
187+
StmtKind::Item(_) => (),
188+
}
189+
None
190+
}
191+
192+
fn is_reserve(cx: &LateContext<'_>, path: &PathSegment<'_>, self_expr: &Expr<'_>) -> bool {
193+
is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_expr).peel_refs(), sym::Vec)
194+
&& path.ident.name.as_str() == "reserve"
195+
}
196+
197+
/// Returns self if the expression is `Vec::set_len()`
198+
fn extract_set_len_self(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<(&'tcx Expr<'tcx>, Span)> {
199+
// peel unsafe blocks in `unsafe { vec.set_len() }`
200+
let expr = peel_hir_expr_while(expr, |e| {
201+
if let ExprKind::Block(block, _) = e.kind {
202+
// Extract the first statement/expression
203+
match (block.stmts.get(0).map(|stmt| &stmt.kind), block.expr) {
204+
(None, Some(expr)) => Some(expr),
205+
(Some(StmtKind::Expr(expr) | StmtKind::Semi(expr)), _) => Some(expr),
206+
_ => None,
207+
}
208+
} else {
209+
None
210+
}
211+
});
212+
match expr.kind {
213+
ExprKind::MethodCall(path, _, [self_expr, _], _) => {
214+
let self_type = cx.typeck_results().expr_ty(self_expr).peel_refs();
215+
if is_type_diagnostic_item(cx, self_type, sym::Vec) && path.ident.name.as_str() == "set_len" {
216+
Some((self_expr, expr.span))
217+
} else {
218+
None
219+
}
220+
},
221+
_ => None,
222+
}
223+
}

clippy_lints/src/vec_init_then_push.rs

+6-46
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,14 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use clippy_utils::higher::{get_vec_init_kind, VecInitKind};
23
use clippy_utils::source::snippet;
3-
use clippy_utils::ty::is_type_diagnostic_item;
4-
use clippy_utils::{match_def_path, path_to_local, path_to_local_id, paths};
4+
use clippy_utils::{path_to_local, path_to_local_id};
55
use if_chain::if_chain;
6-
use rustc_ast::ast::LitKind;
76
use rustc_errors::Applicability;
8-
use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Local, PatKind, QPath, Stmt, StmtKind};
7+
use rustc_hir::{BindingAnnotation, Block, Expr, ExprKind, HirId, Local, PatKind, Stmt, StmtKind};
98
use rustc_lint::{LateContext, LateLintPass, LintContext};
109
use rustc_middle::lint::in_external_macro;
1110
use rustc_session::{declare_tool_lint, impl_lint_pass};
12-
use rustc_span::{symbol::sym, Span};
13-
use std::convert::TryInto;
11+
use rustc_span::Span;
1412

1513
declare_clippy_lint! {
1614
/// ### What it does
@@ -41,11 +39,6 @@ pub struct VecInitThenPush {
4139
searcher: Option<VecPushSearcher>,
4240
}
4341

44-
#[derive(Clone, Copy)]
45-
enum VecInitKind {
46-
New,
47-
WithCapacity(u64),
48-
}
4942
struct VecPushSearcher {
5043
local_id: HirId,
5144
init: VecInitKind,
@@ -58,7 +51,8 @@ impl VecPushSearcher {
5851
fn display_err(&self, cx: &LateContext<'_>) {
5952
match self.init {
6053
_ if self.found == 0 => return,
61-
VecInitKind::WithCapacity(x) if x > self.found => return,
54+
VecInitKind::WithLiteralCapacity(x) if x > self.found => return,
55+
VecInitKind::WithExprCapacity(_) => return,
6256
_ => (),
6357
};
6458

@@ -152,37 +146,3 @@ impl LateLintPass<'_> for VecInitThenPush {
152146
}
153147
}
154148
}
155-
156-
fn get_vec_init_kind<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<VecInitKind> {
157-
if let ExprKind::Call(func, args) = expr.kind {
158-
match func.kind {
159-
ExprKind::Path(QPath::TypeRelative(ty, name))
160-
if is_type_diagnostic_item(cx, cx.typeck_results().node_type(ty.hir_id), sym::Vec) =>
161-
{
162-
if name.ident.name == sym::new {
163-
return Some(VecInitKind::New);
164-
} else if name.ident.name.as_str() == "with_capacity" {
165-
return args.get(0).and_then(|arg| {
166-
if_chain! {
167-
if let ExprKind::Lit(lit) = &arg.kind;
168-
if let LitKind::Int(num, _) = lit.node;
169-
then {
170-
Some(VecInitKind::WithCapacity(num.try_into().ok()?))
171-
} else {
172-
None
173-
}
174-
}
175-
});
176-
}
177-
}
178-
ExprKind::Path(QPath::Resolved(_, path))
179-
if match_def_path(cx, path.res.opt_def_id()?, &paths::DEFAULT_TRAIT_METHOD)
180-
&& is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Vec) =>
181-
{
182-
return Some(VecInitKind::New);
183-
}
184-
_ => (),
185-
}
186-
}
187-
None
188-
}

0 commit comments

Comments
 (0)