Make accesses to fields of packed structs unsafe #44884

arielb1 · 2017-09-27T11:35:20Z

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.

That's it, I think I'll use a strategy suggested by @Zoxc, where this mir

drop(packed_struct.field)

is replaced by

tmp0 = packed_struct.field;
drop tmp0

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.

cc @eddyb

Things which still need to be done for this:

- handle repr(packed) structs in derive the same way I did in Span, and use derive there again
- implement the "fix packed drops" pass and call it in both the MIR shim and validated MIR pipelines
- do a crater run
- convert the errors to compatibility warnings

rust-highfive · 2017-09-27T11:35:31Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

arielb1 · 2017-09-27T11:35:44Z

r? @eddyb

eddyb · 2017-09-27T11:58:13Z

LGTM. cc @rust-lang/compiler

kennytm · 2017-09-27T12:55:46Z

CI failure is due to tendril.

The relevant #[repr(packed)] has been removed in servo/tendril@f9c672f. The latest version of tendril is 0.4.0, but we are currently using 0.3.1. #44785 does not include this update. We need to get kuchiki to update its dependencies.

Dependency tree:

tendril 0.3.1 (current: 0.4.0)
- markup5ever 0.3.2 (current: 0.5.0)
  - html5ever 0.18.0 (current: 0.20.0)
    - kuchiki 0.5.1 (~~already latest~~ current: 0.6.0)
      - html-diff 0.0.4 (~~already latest~~ current: 0.0.5)
        
        rustdoc 0.0.0

[00:34:19] error[E0133]: borrow of packed field requires unsafe function or block
[00:34:19]    --> /cargo/registry/src/github.com-1ecc6299db9ec823/tendril-0.3.1/src/tendril.rs:486:26
[00:34:19]     |
[00:34:19] 486 |         let kind = match self.ptr.get().get() {
[00:34:19]     |                          ^^^^^^^^ borrow of packed field
[00:34:19] 
[00:34:19] error[E0133]: borrow of packed field requires unsafe function or block
[00:34:19]    --> /cargo/registry/src/github.com-1ecc6299db9ec823/tendril-0.3.1/src/tendril.rs:562:15
[00:34:19]     |
[00:34:19] 562 |         match self.ptr.get().get() {
[00:34:19]     |               ^^^^^^^^ borrow of packed field
[00:34:19] 
[00:34:19] error[E0133]: borrow of packed field requires unsafe function or block
[00:34:19]    --> /cargo/registry/src/github.com-1ecc6299db9ec823/tendril-0.3.1/src/tendril.rs:572:17
[00:34:19]     |
[00:34:19] 572 |         let n = self.ptr.get().get();
[00:34:19]     |                 ^^^^^^^^ borrow of packed field
[00:34:19] 
[00:34:19] error[E0133]: borrow of packed field requires unsafe function or block
[00:34:19]    --> /cargo/registry/src/github.com-1ecc6299db9ec823/tendril-0.3.1/src/tendril.rs:580:17
[00:34:19]     |
[00:34:19] 580 |         let n = self.ptr.get().get();
[00:34:19]     |                 ^^^^^^^^ borrow of packed field
[00:34:19] 
[00:34:19] error[E0133]: borrow of packed field requires unsafe function or block
[00:34:19]    --> /cargo/registry/src/github.com-1ecc6299db9ec823/tendril-0.3.1/src/tendril.rs:582:39
[00:34:19]     |
[00:34:19] 582 |         (n > MAX_INLINE_TAG) && (n == other.ptr.get().get())
[00:34:19]     |                                       ^^^^^^^^^ borrow of packed field
[00:34:19] 
[00:34:19] error[E0133]: borrow of packed field requires unsafe function or block
[00:34:19]    --> /cargo/registry/src/github.com-1ecc6299db9ec823/tendril-0.3.1/src/tendril.rs:588:12
[00:34:19]     |
[00:34:19] 588 |         if self.ptr.get().get() <= MAX_INLINE_TAG {
[00:34:19]     |            ^^^^^^^^ borrow of packed field
[00:34:19] 
[00:34:19] error[E0133]: borrow of packed field requires unsafe function or block
[00:34:19]    --> /cargo/registry/src/github.com-1ecc6299db9ec823/tendril-0.3.1/src/tendril.rs:589:13
[00:34:19]     |
[00:34:19] 589 |             self.ptr.set(unsafe { NonZeroUsize::new(EMPTY_TAG) });
[00:34:19]     |             ^^^^^^^^ borrow of packed field
[00:34:19] 
[00:34:19] error: aborting due to 7 previous errors

arielb1 · 2017-09-27T13:06:17Z

The error is correct - calling Cell::get on an unaligned Cell is absolutely UB.

pnkfelix · 2017-10-02T12:06:12Z

@arielb1 do you know if the now merged update to kuchiki mean we should re-do a travis run on this?

arielb1 · 2017-10-02T17:21:31Z

Code is green. Can we have a crater run to see whether we break the world before I go make this a future-compat warning?

arielb1 · 2017-10-02T22:02:47Z

@bors try

bors · 2017-10-02T22:02:58Z

⌛ Trying commit 4ec62e4 with merge 618297e...

@Zoxc

[WIP] 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. That's it, I think I'll use a strategy suggested by @Zoxc, where this mir ``` drop(packed_struct.field) ``` is replaced by ``` tmp0 = packed_struct.field; drop tmp0 ``` 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. cc @eddyb Things which still need to be done for this: - [ ] - handle `repr(packed)` structs in `derive` the same way I did in `Span`, and use derive there again - [ ] - implement the "fix packed drops" pass and call it in both the MIR shim and validated MIR pipelines - [ ] - do a crater run - [ ] - convert the errors to compatibility warnings

bors · 2017-10-02T23:35:11Z

☀️ Test successful - status-travis
State: approved= try=True

Mark-Simulacrum · 2017-10-03T17:30:43Z

Cargobomb run started.

carols10cents · 2017-10-09T14:52:17Z

@Mark-Simulacrum has the cargobomb run completed on this?

Mark-Simulacrum · 2017-10-10T00:38:55Z

Cargobomb complete: http://cargobomb-reports.s3.amazonaws.com/pr-44884/index.html

arielb1 · 2017-10-12T17:07:20Z

This appears to be quite a problem, we have 31 root regressions.

Summary:
basic_dsp_vector 0.5.2 - derive
cgmath 0.15.0 - derive
simd 0.2.0 - derive
tendril 0.2.4 - borrow, fixed
btrfs 1.2.2 - borrow, not on github
gjio 0.1.3 - borrow, deprecated
carp 0.1.0 - derive, no dependent crates, dead?
rust-extra 0.0.17 - borrow, badly unsound
elfloader 0.0.4 - borrow, needs {}
vek 0.1.0 - derive, needs copy
libpijul 0.8.2 - derive, need copy?
lm4f120 0.2.0 - borrow, needs to not be packed?
multiboot 0.3.0 - derive, needs more Copy?
multiboot2 0.3.2 - derive, no repo, needs more Copy?
orbclient 0.3.12 - borrow, needs {}
p0f_api 0.1.1 - borrow, needs {}
pe 0.1.1 - borrow, needs {} I think
pelite 0.4.0 - borrow, needs {}
persistent_hashmap 0.3.0 - derive, not sure how to fix
btrfs2 1.2.2 - borrow, not on github
rpcap 0.3.0 - borrow, needs to switch to Packed fields?
rux 0.1.0 - borrow, needs {}
sgx-isa 0.1.2 - borrow, needs to use by-value
simd-alt 0.2.0 - derive, like simd
tantivy 0.4.3 - borrow, needs extra packed annotations on datastructs
td_revent - borrow, needs {}
vec-2-10-10-10 - borrow, unsound public API
x86_64 0.1.2 - EntryOptions also needs to be packed because the compiler is stupid
zfs 0.1.0 - borrow, needs {}
lout (https://github.com/rrichardson/lout) - borrow, needs {}

arielb1 · 2017-10-12T17:09:48Z

There are a few common types of regressions:

One easily-fixed case is println!("{}", packed.field);, which needs to be changed to println!("{}", {packed.field}); to avoid creating a direct reference to a packed field.

The other annoying one is derive on a non-Copy packed struct. The worst one is this from https://github.com/JoNil/persistent_hashmap/blob/master/src/lib.rs:

#[derive(Clone, Copy, Default)]
#[repr(C, packed)]
struct HashmapEntry<V> {
    // 1 bit occupied and 63 bit hash
    state: u64,
    value: V,
}

The Debug & Clone impls are currently UB, and I don't see a good way of fixing them except for either removing the packed or adding a V: Copy bound.

arielb1 · 2017-10-12T17:10:48Z

I think the derive wrapper needs to be changed to add an attribute that warns if there's a non-Copy field, to avoid breaking half the world.

bors · 2017-10-13T04:06:17Z

☔ The latest upstream changes (presumably #45013) made this pull request unmergeable. Please resolve the merge conflicts.

arielb1 · 2017-11-26T17:02:37Z

r? @eddyb - the last commit is new and non-trivial

nrc · 2017-11-26T23:19:56Z

Rustfmt is fixed by #46144, I was waiting for this to land, but it looks like it is taking longer than expected. Since 46144 is ready to go and fixes the RLS which is currently broken, I want to land it sooner rather than later. Assuming it lands first (which it should) could you remove the 'mark rustfmt as broken' commit here since it shouldn't be needed.

eddyb · 2017-11-27T11:52:14Z

@bors r=nikomatsakis,eddyb

bors · 2017-11-27T11:52:15Z

📌 Commit f3b2d7f has been approved by nikomatsakis,eddyb

@Zoxc

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. That's it, I think I'll use a strategy suggested by @Zoxc, where this mir ``` drop(packed_struct.field) ``` is replaced by ``` tmp0 = packed_struct.field; drop tmp0 ``` 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. cc @eddyb Things which still need to be done for this: - [ ] - handle `repr(packed)` structs in `derive` the same way I did in `Span`, and use derive there again - [ ] - implement the "fix packed drops" pass and call it in both the MIR shim and validated MIR pipelines - [ ] - do a crater run - [ ] - convert the errors to compatibility warnings

bors · 2017-11-27T14:23:11Z

⌛ Testing commit f3b2d7f with merge 58e1234...

bors · 2017-11-27T17:13:15Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis,eddyb
Pushing 58e1234 to master...

That commit had accidentally snuck in into rust-lang#44884. Revert it. This reverts commit c48650e.

@bors

Revert "fix treatment of local types in "remote coherence" mode" That commit had accidentally snuck in into #44884 and it causes regressions. Revert it. This reverts commit c48650e. @bors p=10 - fixes breakage in nightly r? @eddyb

rust-highfive assigned pnkfelix Sep 27, 2017

rust-highfive assigned eddyb and unassigned pnkfelix Sep 27, 2017

This was referenced Sep 27, 2017

Update html5ever to 0.20.0 kuchiki-rs/kuchiki#38

Merged

Update kuchiki to 0.6 GuillaumeGomez/html-diff-rs#10

Merged

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 28, 2017

arielb1 force-pushed the pack-safe branch 2 times, most recently from 51e8c00 to 4ec62e4 Compare October 2, 2017 14:00

arielb1 added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 2, 2017

arielb1 force-pushed the pack-safe branch from 86e2262 to 25f57b5 Compare October 5, 2017 13:49

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 13, 2017

arielb1 force-pushed the pack-safe branch from 5f593a0 to 47d53e8 Compare November 26, 2017 14:12

improve error messages

f3b2d7f

rust-highfive assigned eddyb and unassigned nikomatsakis Nov 26, 2017

nrc mentioned this pull request Nov 26, 2017

Update RLS and Rustfmt #46144

Merged

bors merged commit f3b2d7f into rust-lang:master Nov 27, 2017

This was referenced Nov 27, 2017

incr.comp.: Some preparatory work for caching more query results. #46299

Merged

Bump to 1.24.0 #46288

Merged

Move rustc_back modules where they belong. #46305

Merged

Make GlobalCtxt implement Sync #45912

Closed

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 27, 2017

SimonSapin mentioned this pull request Nov 28, 2017

Regression: conflicting implementations of trait std::convert::Into<std::boxed::Box<_>> for type Bow<'_, _> #46328

Closed

arielb1 added a commit to arielb1/rust that referenced this pull request Nov 28, 2017

Revert "fix treatment of local types in "remote coherence" mode"

bef4fe9

That commit had accidentally snuck in into rust-lang#44884. Revert it. This reverts commit c48650e.

arielb1 mentioned this pull request Nov 28, 2017

Revert "fix treatment of local types in "remote coherence" mode" #46329

Merged

phil-opp mentioned this pull request Dec 6, 2017

Various cleanups. rust-osdev/multiboot2#36

Merged

This was referenced Dec 29, 2017

repr(packed) allows invalid unaligned loads #27060

Closed

repr(packed) calls drop with unaligned pointer #43306

Closed

Packed unsized struct with drop: Confusing error message #47068

Closed

ehuss mentioned this pull request Mar 8, 2019

Undocumented Attributes rust-lang/reference#532

Closed

6 tasks

pnkfelix mentioned this pull request Jun 17, 2019

error: internal compiler error: "inference variables in ..." on packed type #61402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make accesses to fields of packed structs unsafe #44884

Make accesses to fields of packed structs unsafe #44884

arielb1 commented Sep 27, 2017 •

edited

Loading

rust-highfive commented Sep 27, 2017

arielb1 commented Sep 27, 2017

eddyb commented Sep 27, 2017

kennytm commented Sep 27, 2017 •

edited

Loading

arielb1 commented Sep 27, 2017

pnkfelix commented Oct 2, 2017

arielb1 commented Oct 2, 2017

arielb1 commented Oct 2, 2017

bors commented Oct 2, 2017

bors commented Oct 2, 2017

Mark-Simulacrum commented Oct 3, 2017

carols10cents commented Oct 9, 2017

Mark-Simulacrum commented Oct 10, 2017

arielb1 commented Oct 12, 2017

arielb1 commented Oct 12, 2017 •

edited

Loading

arielb1 commented Oct 12, 2017

bors commented Oct 13, 2017

arielb1 commented Nov 26, 2017

nrc commented Nov 26, 2017

eddyb commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

Make accesses to fields of packed structs unsafe #44884

Make accesses to fields of packed structs unsafe #44884

Conversation

arielb1 commented Sep 27, 2017 • edited Loading

rust-highfive commented Sep 27, 2017

arielb1 commented Sep 27, 2017

eddyb commented Sep 27, 2017

kennytm commented Sep 27, 2017 • edited Loading

arielb1 commented Sep 27, 2017

pnkfelix commented Oct 2, 2017

arielb1 commented Oct 2, 2017

arielb1 commented Oct 2, 2017

bors commented Oct 2, 2017

bors commented Oct 2, 2017

Mark-Simulacrum commented Oct 3, 2017

carols10cents commented Oct 9, 2017

Mark-Simulacrum commented Oct 10, 2017

arielb1 commented Oct 12, 2017

arielb1 commented Oct 12, 2017 • edited Loading

arielb1 commented Oct 12, 2017

bors commented Oct 13, 2017

arielb1 commented Nov 26, 2017

nrc commented Nov 26, 2017

eddyb commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

bors commented Nov 27, 2017

arielb1 commented Sep 27, 2017 •

edited

Loading

kennytm commented Sep 27, 2017 •

edited

Loading

arielb1 commented Oct 12, 2017 •

edited

Loading