Skip to content

Commit 57cd603

Browse files
committed
make accesses to fields of packed structs unsafe
To handle packed structs with destructors (which you'll think are a rare case, but the `#[repr(packed)] struct Packed<T>(T);` pattern is ever-popular, which requires handling packed structs with destructors to avoid monomorphization-time errors), drops of subfields of packed structs should drop a local move of the field instead of the original one. cc #27060 - this should deal with that issue after codegen of drop glue is updated. The new errors need to be changed to future-compatibility warnings, but I'll rather do a crater run first with them as errors to assess the impact.
1 parent 0defa20 commit 57cd603

File tree

6 files changed

+145
-26
lines changed

6 files changed

+145
-26
lines changed

src/librustc_mir/transform/check_unsafety.rs

+49-20
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ use rustc_data_structures::fx::FxHashSet;
1212
use rustc_data_structures::indexed_vec::IndexVec;
1313

1414
use rustc::ty::maps::Providers;
15-
use rustc::ty::{self, TyCtxt};
15+
use rustc::ty::{self, Ty, TyCtxt};
1616
use rustc::hir;
1717
use rustc::hir::def::Def;
1818
use rustc::hir::def_id::DefId;
@@ -31,6 +31,9 @@ pub struct UnsafetyChecker<'a, 'tcx: 'a> {
3131
visibility_scope_info: &'a IndexVec<VisibilityScope, VisibilityScopeInfo>,
3232
violations: Vec<UnsafetyViolation>,
3333
source_info: SourceInfo,
34+
// true if an a part of this *memory block* of this expression
35+
// is being borrowed, used for repr(packed) checking.
36+
need_check_packed: bool,
3437
tcx: TyCtxt<'a, 'tcx, 'tcx>,
3538
param_env: ty::ParamEnv<'tcx>,
3639
used_unsafe: FxHashSet<ast::NodeId>,
@@ -51,6 +54,7 @@ impl<'a, 'gcx, 'tcx> UnsafetyChecker<'a, 'tcx> {
5154
},
5255
tcx,
5356
param_env,
57+
need_check_packed: false,
5458
used_unsafe: FxHashSet(),
5559
}
5660
}
@@ -130,6 +134,14 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
130134
lvalue: &Lvalue<'tcx>,
131135
context: LvalueContext<'tcx>,
132136
location: Location) {
137+
let old_need_check_packed = self.need_check_packed;
138+
if let LvalueContext::Borrow { .. } = context {
139+
let ty = lvalue.ty(self.mir, self.tcx).to_ty(self.tcx);
140+
if !self.has_align_1(ty) {
141+
self.need_check_packed = true;
142+
}
143+
}
144+
133145
match lvalue {
134146
&Lvalue::Projection(box Projection {
135147
ref base, ref elem
@@ -143,31 +155,39 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
143155
self.source_info = self.mir.local_decls[local].source_info;
144156
}
145157
}
158+
if let &ProjectionElem::Deref = elem {
159+
self.need_check_packed = false;
160+
}
146161
let base_ty = base.ty(self.mir, self.tcx).to_ty(self.tcx);
147162
match base_ty.sty {
148163
ty::TyRawPtr(..) => {
149164
self.require_unsafe("dereference of raw pointer")
150165
}
151-
ty::TyAdt(adt, _) if adt.is_union() => {
152-
if context == LvalueContext::Store ||
153-
context == LvalueContext::Drop
154-
{
155-
let elem_ty = match elem {
156-
&ProjectionElem::Field(_, ty) => ty,
157-
_ => span_bug!(
158-
self.source_info.span,
159-
"non-field projection {:?} from union?",
160-
lvalue)
161-
};
162-
if elem_ty.moves_by_default(self.tcx, self.param_env,
163-
self.source_info.span) {
164-
self.require_unsafe(
165-
"assignment to non-`Copy` union field")
166+
ty::TyAdt(adt, _) => {
167+
if adt.is_union() {
168+
if context == LvalueContext::Store ||
169+
context == LvalueContext::Drop
170+
{
171+
let elem_ty = match elem {
172+
&ProjectionElem::Field(_, ty) => ty,
173+
_ => span_bug!(
174+
self.source_info.span,
175+
"non-field projection {:?} from union?",
176+
lvalue)
177+
};
178+
if elem_ty.moves_by_default(self.tcx, self.param_env,
179+
self.source_info.span) {
180+
self.require_unsafe(
181+
"assignment to non-`Copy` union field")
182+
} else {
183+
// write to non-move union, safe
184+
}
166185
} else {
167-
// write to non-move union, safe
186+
self.require_unsafe("access to union field")
168187
}
169-
} else {
170-
self.require_unsafe("access to union field")
188+
}
189+
if adt.repr.packed() && self.need_check_packed {
190+
self.require_unsafe("borrow of packed field")
171191
}
172192
}
173193
_ => {}
@@ -191,8 +211,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
191211
}]);
192212
}
193213
}
194-
}
214+
};
195215
self.super_lvalue(lvalue, context, location);
216+
self.need_check_packed = old_need_check_packed;
196217
}
197218
}
198219

@@ -215,6 +236,14 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
215236
}
216237
}
217238
}
239+
240+
fn has_align_1(&self, ty: Ty<'tcx>) -> bool {
241+
self.tcx.at(self.source_info.span)
242+
.layout_raw(self.param_env.and(ty))
243+
.map(|layout| layout.align(self.tcx).abi() == 1)
244+
.unwrap_or(false)
245+
}
246+
218247
fn require_unsafe(&mut self,
219248
description: &'static str)
220249
{

src/librustc_typeck/check/wfcheck.rs

+24-4
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use constrained_type_params::{identify_constrained_type_params, Parameter};
1414

1515
use hir::def_id::DefId;
1616
use rustc::traits::{self, ObligationCauseCode};
17-
use rustc::ty::{self, Ty, TyCtxt};
17+
use rustc::ty::{self, Lift, Ty, TyCtxt};
1818
use rustc::util::nodemap::{FxHashSet, FxHashMap};
1919
use rustc::middle::lang_items;
2020

@@ -223,10 +223,31 @@ impl<'a, 'gcx> CheckTypeWellFormedVisitor<'a, 'gcx> {
223223
{
224224
self.for_item(item).with_fcx(|fcx, this| {
225225
let variants = lookup_fields(fcx);
226+
let def_id = fcx.tcx.hir.local_def_id(item.id);
227+
let packed = fcx.tcx.adt_def(def_id).repr.packed();
226228

227229
for variant in &variants {
228-
// For DST, all intermediate types must be sized.
229-
let unsized_len = if all_sized || variant.fields.is_empty() { 0 } else { 1 };
230+
// For DST, or when drop needs to copy things around, all
231+
// intermediate types must be sized.
232+
let needs_drop_copy = || {
233+
packed && {
234+
let ty = variant.fields.last().unwrap().ty;
235+
let ty = fcx.tcx.erase_regions(&ty).lift_to_tcx(this.tcx)
236+
.unwrap_or_else(|| {
237+
span_bug!(item.span, "inference variables in {:?}", ty)
238+
});
239+
ty.needs_drop(this.tcx, this.tcx.param_env(def_id))
240+
}
241+
};
242+
let unsized_len = if
243+
all_sized ||
244+
variant.fields.is_empty() ||
245+
needs_drop_copy()
246+
{
247+
0
248+
} else {
249+
1
250+
};
230251
for field in &variant.fields[..variant.fields.len() - unsized_len] {
231252
fcx.register_bound(
232253
field.ty,
@@ -245,7 +266,6 @@ impl<'a, 'gcx> CheckTypeWellFormedVisitor<'a, 'gcx> {
245266
}
246267
}
247268

248-
let def_id = fcx.tcx.hir.local_def_id(item.id);
249269
let predicates = fcx.tcx.predicates_of(def_id).instantiate_identity(fcx.tcx);
250270
let predicates = fcx.normalize_associated_types_in(item.span, &predicates);
251271
this.check_where_clauses(fcx, item.span, &predicates);

src/libsyntax_pos/span_encoding.rs

+15-1
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,30 @@ use hygiene::SyntaxContext;
1919

2020
use rustc_data_structures::fx::FxHashMap;
2121
use std::cell::RefCell;
22+
use std::hash::{Hash, Hasher};
2223

2324
/// A compressed span.
2425
/// Contains either fields of `SpanData` inline if they are small, or index into span interner.
2526
/// The primary goal of `Span` is to be as small as possible and fit into other structures
2627
/// (that's why it uses `packed` as well). Decoding speed is the second priority.
2728
/// See `SpanData` for the info on span fields in decoded representation.
28-
#[derive(Clone, Copy, PartialEq, Eq, Hash)]
29+
#[derive(Clone, Copy)]
2930
#[repr(packed)]
3031
pub struct Span(u32);
3132

33+
impl PartialEq for Span {
34+
#[inline]
35+
fn eq(&self, other: &Self) -> bool { ({self.0}) == ({other.0}) }
36+
}
37+
38+
impl Eq for Span {}
39+
40+
impl Hash for Span {
41+
fn hash<H: Hasher>(&self, s: &mut H) {
42+
{self.0}.hash(s)
43+
}
44+
}
45+
3246
/// Dummy span, both position and length are zero, syntax context is zero as well.
3347
/// This span is kept inline and encoded with format 0.
3448
pub const DUMMY_SP: Span = Span(0);
+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
#[repr(packed)]
12+
pub struct Bad<T: ?Sized> {
13+
data: T, //~ ERROR `T: std::marker::Sized` is not satisfied
14+
}
15+
16+
fn main() {}

src/test/compile-fail/issue-27060.rs

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
#[repr(packed)]
12+
pub struct Good {
13+
data: &'static u32,
14+
data2: [&'static u32; 2],
15+
aligned: [u8; 32],
16+
}
17+
18+
#[repr(packed)]
19+
pub struct JustArray {
20+
array: [u32]
21+
}
22+
23+
fn main() {
24+
let good = Good {
25+
data: &0,
26+
data2: [&0, &0],
27+
aligned: [0; 32]
28+
};
29+
30+
unsafe {
31+
let _ = &good.data; // ok
32+
let _ = &good.data2[0]; // ok
33+
}
34+
35+
let _ = &good.data; //~ ERROR borrow of packed field requires unsafe
36+
let _ = &good.data2[0]; //~ ERROR borrow of packed field requires unsafe
37+
let _ = &*good.data; // ok, behind a pointer
38+
let _ = &good.aligned; // ok, has align 1
39+
let _ = &good.aligned[2]; // ok, has align 1
40+
}

src/test/run-pass/packed-struct-borrow-element.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ struct Foo {
1717

1818
pub fn main() {
1919
let foo = Foo { bar: 1, baz: 2 };
20-
let brw = &foo.baz;
20+
let brw = unsafe { &foo.baz };
2121

2222
assert_eq!(*brw, 2);
2323
}

0 commit comments

Comments
 (0)