New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for `safe_packed_borrows` compatibility lint #46043

Open
arielb1 opened this Issue Nov 16, 2017 · 23 comments

Comments

Projects
None yet
@arielb1
Contributor

arielb1 commented Nov 16, 2017

What is this warning about

Fields of structs with the #[repr(packed)] attribute might be unaligned. Even if a field has a valid offset (e.g. 0) within the struct, the struct itself might be unaligned.

On the other hand, safe references (&T and &mut T) must always have valid alignment (even when they are just created) - not only will dereferencing these references cause segfaults on architectures such as Sparc, but the compiler is allowed to optimize based on that assumption (e.g. in a future version of rustc, it might use the "free" alignment bits to represent enum discriminants), so even just creating an unaligned reference is Undefined Behavior and might cause the program to behave in unexpected ways.

Therefore, borrowing a field in the interior of a packed structure with alignment other than 1 is unsafe - if the reference is not known to be aligned, the borrow must be done to an unsafe pointer.

Note that borrowing a field with alignment 1 is always safe.

For example, consider the common struct Unaligned:

#[repr(packed)]
pub struct Unaligned<T>(pub T);

That struct can be placed inside a bigger struct at an unaligned offset:

pub struct Foo {
    start: u8,
    data: Unaligned<u32>,
}

In that case, a borrow of a field of the struct would be Undefined Behavior (UB), and therefore is made unsafe:

let x = Foo { start: 0, data: Unaligned(1) };
let y = &x.data.0; // UB, also future-compatibility warning
println!("{}", x.data.0); // this borrows `x.data.0`, so UB + future-compat warning
use(y);

This used to be left unchecked by the compiler - issue #27060.

How to fix this warning/error

The most common ways to fix this warnings are:

  1. remove the #[repr(packed)] attribute if it is not actually needed. A few crates had unnecessary #[repr(packed)] annotations - for example, tendril.
  2. copy the fields to a local first. When accessing a field of a packed struct directly without using a borrow, the compiler will make sure the access is done correctly even when the field is unaligned. The then-aligned local can then be freely used. For example:
    let x = Foo { start: 0, data: Unaligned(1) };
    let temp = x.data.0;
    println!("{}", temp); // works
    // or, to the same effect, using an `{x}` block:
    println!("{}", {x.data.0}); // works
    use(y);

In some cases, it might be necessary to borrow the fields as raw pointers and use read_unaligned/write_unaligned to access them, but I haven't encountered such a case.

Derives

One annoying case where this problem can appear is if a packed struct has builtin derives, e.g.

#[derive(PartialEq, ...)]
#[repr(packed)]
pub struct Unaligned<T>(pub T);

Which essentially expands to this:

impl<T: PartialEq> PartialEq for Unaligned<T> {
    fn eq(&self, other: &Self) -> bool {
        PartialEq::eq(&self.0, &other.0) // this is UB and unsafe
    }
}

If your struct already derives Copy and has no generics, the compiler will generate code that copies the fields to locals and will therefore work. Otherwise, you'll have to write the derive implementations manually.

For example, you might want to add a T: Copy bound and copy things out:

#[repr(packed)]
pub struct Unaligned<T: Copy>(pub T);

impl<T: Copy + PartialEq> PartialEq for Unaligned<T> {
    fn eq(&self, other: &Self) -> bool {
        ({self.0}) == ({other.0}) // this copies fields to temps, and works
    }
}

@arielb1 arielb1 changed the title from Tracking issue for `safe_packed_borrow` compatibility lint to Tracking issue for `safe_packed_borrows` compatibility lint Nov 16, 2017

CasualX added a commit to CasualX/pelite that referenced this issue Jan 5, 2018

Fix for new rustc warnings
* Remove all `#[repr(packed)]` on the IMAGE structs due to a misunderstanding of the Windows.h struct definitions:
Only the old 16-bit headers want 2 byte packing, all the others want 4 byte packing. However all of these structs end up already aligned anyway.
Safeguard their implementation by asserting their sizes.

  Fixes rust-lang/rust#46043

* Explicitly specify the raw pointer type when casting from reference and calling a method on it.

  Fixes rust-lang/rust#46906

CasualX added a commit to CasualX/pelite that referenced this issue Jan 5, 2018

Fix for new rustc warnings
* Remove all `#[repr(packed)]` on the IMAGE structs due to a misunderstanding of the Windows.h struct definitions:
Only the old 16-bit headers want 2 byte packing, all the others want 4 byte packing. However all of these structs end up already aligned anyway.
Safeguard their implementation by asserting their sizes.

  Fixes rust-lang/rust#46043

* Explicitly specify the raw pointer type when casting from reference and calling a method on it.

  Fixes rust-lang/rust#46906

ppannuto added a commit to tock/tock that referenced this issue Jan 7, 2018

refactor: remove `packed` from MMIO structs
rust-lang/rust#46043 makes accessing members
of packed structs unsafe. For MMIO structs, `repr(C)` alone should be
sufficient as the layout by definition has no holes.

jlwatson pushed a commit to jlwatson/tock that referenced this issue Jan 22, 2018

Jean-Luc Watson
Bug fix for excessive ENOACK preambles
kernel: give tick parameters meaningful names

Closes tock#604.

Use valid BLE advertisement address for NRF5X

BLE advertisement address hardcoded in user-space

Hardcoded advertisement address in user-space instead in the low
level radio configuration

debug: document `debug_verbose!` in moduledoc

debug: implement `debug_gpio!`

debug: remove unneeded indirection for gpio

debug: update documentation for `debug_gpio!`

debug: import `debug_gpio!` by default

Following the same rationale as importing `debug!` for folks

sam4l: fix RXRDY bit position

Noticed this working on other things - nothing's using RX so it shouldn't affect anything, but it was wrong.
(24.7.3 / p628)

debug: allow use of debug_gpio! in unsafe context

Fix wrong comment

Remove superfluous and misleading size parameters

According to the definiton of static_init! the size parameters are not
evaluated. Parameters without any effect should be removed as they lead
to unnecessary distraction. In addition, the size parameters sometimes
were wrong, e.g. 4*11 instead of 4*15.

Use more idiomatic pointer conversions instead of mem::transmute

mem::transmute is very unsafe as it does not really type check the
input value. The more idiomatic and more typesafe way is to use a safe
pointer conversion from usize to *const T followed by an unsafe
pointer-to-reference conversion via &*T. The advantage of using the
conversion over mem::transmute is that the compiler would complain as
soon as you stop using usize as an input value type.

doc: Start on new porting guide

Porting guide WIP notice

doc: update porting

doc: add details on porting boards, some chip stuff

doc: add info on arch crate to porting

nrf5x readme use offset 0x2000 when using tockloader

Entire advertisement buffer constructed in capsule

The ble_advertising example app works and I can view it in my android
phone.

More things need to be moved into the grant for "complete"
virtualization

More virtualization

Now, moved all the data members to the Grant except the alarm

Added support for more "concurrency"

* Added a state variable inside the grant instead of "is_advertising" var
* Only place where the busy flag (mutex) is used is in fired callback
* Can find the advertisments in my iPhone now....
* May need to implement a separate trait in RNG to fetch random bytes

re-wrote the BLE capules in more a 'rustocean way'

Introduced more types with enums etc but conversion from an usize to
enum is painful!!

However this will probably be a bit slower and the binary is slight
bigger

New approach (fd93af18) :
   text	   data	    bss	    dec	    hex
  62348	   2044	  13464	  77856	  13020

Old approach (9c4ea6d):
   text    data     bss     dec     hex
  61756	   2044	  13464	  77264	  12dd0

refactor, need fix ble scanning with size etc

Kernel support number of bytes read BLE rx

I have added functionlity so that the kernel adds the number of
received bytes during Bluetooth Low Energy passive scanning.

There exist no primitive to do this in hardware what I have found at
least. Instead I read the BLE header that indicates how many bytes "it"
should be in the packet.

Modified the capsule slightly with some different state validations

BLE Advertisement address compliant with BLE Spec

Use AppId as "randomness/uniqueness"

ble_reset_advertisment() bug fixed

Forgot to change the offset when the advertisement buffer was reset

Migrated nRF51 Radio the new BLE Virtualization

* Moved all common nRF5X Constants to a seperate file
* Changed all radio memory mapped I/O to lowercase
* nRF51 works with the BLE Virtualization

Enables several processes for nRF52DK and Ticks

* Works to run several processes on nRF52DK in Tock
* Followed @alevy suggestion on advertisement interval and it's
stored as milliseconds in the Grant and is then converted just
before an virtual timer is configured
* Seem to have race-conditions
* use this command for tockloader:
tockloader list --jtag --board nrf52dk --arch cortex-m4 --app-address
0x20000 --jtag-device nrf52

Progress debugging prints indicates advertisements

I still can only find one device in my phone but the
debugging prints say that advertisements are performed by two different
devices.

However, it seem buggy so far since it looks like the TockOS App
advertises more often than CoolOS. I think it has something to do with
AppIds in "for each in Grant". To be cont'd

Ugly hardcoded implementation for only two apps

At least it works now I can see CoolOS and TockOS in my phone

TODO Implement IRC Discussion:
<alevy> basically, you could take the alarm driver (which provides a
single alarm to each userland app), taking _only_ the parts that
implement the periodic timer interface, and extending that with ble
advertising data in each app's grant
<alevy> so each app's grant has meta-data about a periodic timer
(interval + t0)
<alevy> to set the next alarm, at any given point, you iterate through
the apps and find the app with the soonest timer expiration and use that
as the next alarm
<alevy> when `fired` is invoked, you iterate through the apps to find
all apps that have timers that (conceptually) expired (this is something
like current_time > t0 + interval, but accounting for wrap-around)
<alevy> here's where the alarm driver and your driver would differ
<alevy> the alarm driver can just schedule callbacks for _all_ apps
whose timer expired while we were asleep, if there are multiple (usually
there aren't)
<alevy> your driver is gonna have to pick a winner because you can't
schedule two advertisements at once
<niklasad1> Ok, the simplest implementation of this I can think of is to
decide which is the next_app after an advertisement/scann has been fired
(callback from chip driver).
<niklasad1> then we find the shortest time => when the alarms fires =>
advertise/scan => repeat

Implemented Alarm to detect the "current app"

More or less copy-pasted the AlarmData struct from the alarm capsule
and it is a part of each process's grant

Every time an alarms fires the grants are searched to find the
process that expired or have the smallest value

However, in according to NRF Connect the advertisement interval don't match that
configured interval according to my phone but I will test is with my
logic analyzer

Added appid to the chip(radio.rs) which is used for example to determine which
app the callback belong to

Adapted nrf51/radio.rs to the new capsule but it don't work at moment.
Haven't debugged it but seem to hang somewhere suspect the new interrupt scheme.

some progress but I need queue for missed advertisements

Moved logic of adv/scanning to the capsules

* added a simple circular buffer for queue in safe Rust (based on Cell)
* the queue is not used atm, little worried that it "destroys" determinism
* scanning is WIP
* advertisement works fine but need to tweak the get_current_process
because it trade-off fairness over everything else
* alot of debug stuff left in the code

Can run 3 apps concurrently: 2 adv and 1 scanning

* Passive Scanning (Stable)
* Enabled the queue
* Advertisement intevals looks good in my phone but are "non-deterministic" and differ a little bit
* Good enough for me but the passive scanning app need to be fixed ofcourse

Fix nrf51-DK to the new interface

Docs and some changes

* Added limited in inline comments
* Added a new TxClient trait
* Refactor radio code for NRF52
* Removed LED debugging
* Very few debug prints left

Re-organize BLE Driver

* Moved most functions that operate on the Grant to methods
* Renamed BLE HIL functions
* Reomved the duplicate advertising app

Remove AppId from BLE advertising HIL

The low-level (radio) driver shouldn't track things like which process
has outstanding requests. Instead, that functionality can all be done
more effectively in userland drivers by simply adding fields to keep
track of which app we last made a request on behalf of.

Remove unnecessary use of `Cell`

Data that will be accessed from Grants isn't shared, so it doesn't need
to use Cells for leaf data.

docs

Simplify BLE advertising timer implementation

This commig significantly simplifies how timers for BLE advertising and
scanning are handled. In the process it fixes some concurrency bugs
wherein the underlying alarm may be overriden incorrectly and adds the
pseudo random `advDelay` to the advertising interval required by the BLE
specification.

First, it separates the logic for determining the next alarm for each
app from determining which alarm to set with the underlying hardware.
One is app-specific (and therefore a method of the `App` struct), while
the latter is a global decision (and therefore a method of the `BLE`
struct).

Second, it gets rid of the circular buffer for keeping priorities of
apps. For now, it's just simpler to assume collisions can be dropped on
the floor. In any case, priorities are likely better implemented using a
per-app field that stores that last successful event, rather than an
explicit list.

These two let us get rid of a bunch of unnecessary code and meta-data.
It also better mirrors how the alarm capsule is implemented, which might
eventually lead to extracting some common code in the future.

BLE HIL changed

* the buffer is passed with the transmit_advertisement() instead of a
separate function call
* renamed HIL functions to a more "Tockisch" fashion i.e. full names etc
* changed radio.rs for NRF51 & NRF52
* replaced `fn replaced_buffer` with send_advertisement in BLE Driver
* Un-necessary passing of buffer via transmit_advertisement for the
channels 38 and 39 when advertising

ble_advertising_app and update BLE driver idx

* Faulty inline comment removed from ble_advertising_app
* Forgot to changed idx in the advertisement buffer onces reseting the
advertisement buffer

documentation nits

AlarmData doesn't need to be public

Typo fixed and some clarifications

bump nightly to 2018-01-05

No code changes.

refactor: remove `packed` from MMIO structs

rust-lang/rust#46043 makes accessing members
of packed structs unsafe. For MMIO structs, `repr(C)` alone should be
sufficient as the layout by definition has no holes.

travis: don't be verbose unpacking tar archive

Try to work around travis-ci/travis-ci#4704

update rustfmt (0.7.1 -> 0.3.4) *no code changes*

Yes, 0.7.1 -> 0.3.4 is an update, upstream reset versioning.

This commit only updates the tooling, the subsequent commit updates the
actual code, and minimal style changes such that the new formatter does
not exit with a hard error.

Branches should be able to cherry-pick just this commit and update
formatting locally to minimize merge churn.

style: update tree with new rustfmt

This commit contains only auto-generated changes from the updated rustfmt.

Prevent auto-format when using VSCode

Enable formatOnSave in VSCode and use a specific RLS version

tools: semantic instead of lexical version compare

This is a bit obnoxious to do in a cross-platform way, but this pure-bash
solution should work according to the wonderful wonderful Internet.

Enable remaining GPIO pins on NRF52DK

* Test and enable P0.02, P0.11, P0.12, P0.22 to P0.27
* Test and document internal connection of P0.00, P0.01, P0.05 to P0.10
* Test and disable P0.21 (RESET)

Do not expose internally connected GPIO pins

Internally connected GPIO pins are supposed to be accessed via a
suitable driver instead

Remove commented-out lines

* Remove commented-out lines
* Apadt documentation for P0.00 and P0.01

Removes remnant of old instructions.

NRF5X Radio Configuration

* Support Packets with payload up 255 bytes (but advertising driver is
only using 39)
* Added some inline comments about the packet structure
* Added four simple tests for BLE
* Made the interface to control tx_power level and advertisement interval accessable for user-space outside
the ble_initialize function
* Configure tx power before advertising/scanning (ble_advertising
driver)
* libtock simple_ble modified don't need to configure advertisment
buffer in scanning

Adressed @brghena's comments and renamed ble_init

* Use explicit pattern matching for the enum TxPower
* Renamed nrf52:📻:ble_init() to ble_initialize
* Adressed all other feedback

rustfmt

TryFrom Trait

* Introduced TryFrom in the nrf5x, nrf51 & nrf52 crates
* I have implemented it "manually" and in the long-from it would be
good to derrive the implementation if it gets used from widely

doc: add networking stack design document

This document will eventually serve as a complete interface guide for
developers to use when writing code for the networking stack on Tock. It will
describe the interfaces between layers, the nature of layering and
encapsulation, and more. For now, the document is in its very early stages
and likely contains some erroneous information in addition to lots of missing
information.

Add
- a better stack architecture diagram
- an additional section that addresses file names
- revisions to the traits and implementation section
- Describe several message transmission paths and inter-layer dependencies

convert to markdown

Small fixes

Mostly grammatical fixes and a couple changes for clarity.

doc: clarify that main's return value may change

In reviewing tock#714, I got to thinking about process behavior, in particular `main`'s return code. Perhaps we should attribute it some significance (e.g. kill processes where `main` failed somehow, maybe respawn if error > 0, kill < 0). Not necessarily a debate to have today, just a thought to let rattle around. In the meantime, be explicit that main should return 0 for forward compatibility.

userland: fix libc++ scripts to build on OS X too

userland: update libc++ to match newlib version

WIP - blink working

- Removed all memory setup from kernel.

- Implemeted GOT relocation and BSS zeroing in crt0.c

This is enough for blink, but blink doesn't have a data section (that
should be an easy fix), and doesn't have static relocations (the hard
part).

Copy data section in crt0

Remove unused PIC header from TBF

Deal with .rel.data section in crt0

Make stack size tunable in crt0

comment userland_layout.ld

Don't needlessly align to 8 bytes

Cleanup elf2tbf

Removed unnecessary align4 in process.rs

Add heap/stack tracking to crt0

Move C++ exceptions table to end of ELF

We currently don't actually serialize it to the TBF (we probably should
be if we care about exceptions), so it was screwing up the computation
for where the rel.data section would be placed. Placing it at the end,
after the dummy `endsec` section, avoids that, and otherwise doesn't
make a difference.

Use explicitly sized types for process header

comment in crt0 for header structure

Add explicit, optional calibration to CM4 SysTick

In some cases, the SysTick calibration is not set by the core. So, in
order to get 10s of ms units, the board/chip must tell us explicitly
what is the clock speed of the SysTick.

This change, first, creates a real struct for SysTick, similar to the
MPU struct, which contains a tenms field. If the core's tenms register
is 0, we use the struct's field instead. It also adds a constructor that
takes a clock speed and converts it to a tenms.

This change requires changing how SysTick is instantiated in chip crates
since constructors now return a struct instead of a reference, but it
should only be some small type changes, and it's more similar to the MPU
this way anyway.

Add MPU + SysTick to NRF52

Adds support for the MPU and SysTick to the NRF52 chip.

We use a calibration value of 64Mhz for the SysTick since the NRF52 does
not contain a callibration value in the core itself (and SysTick is
clocked by the CPU clock, which is always 64Mhz).

The MPU required fixing a "bug" in the NRF52DK's chip_layout, which was
setting the MIN_MPU_ALIGNMENT to 8 _bytes_ rather than 8K. Other boards
using the NRF52 will also have to ensure this value is set properly.

Finally, updates the SAM4L to use the very slighly modified SysTick
constructor interface.

Avoid overflow in systick

Avoids possible multiplication overflows in the `set_timer` and `value`
methods of the cortex-m4 SysTick by replacing with multiplications with
division. This sacrifices a bit of percision if the clock speed isn't
round in base 10, but oh well... To avoid division by 0, also cap
`set_timer` argument to 1 second.

Document and cleanup systick

systick: comments only, herz -> hertz

doc: herz -> Hertz

userland: add gitignore for libg++ build artifacts

Cleanup userland_generic.ld

Cleaned up a few of the comments, removed unused symbols and...
improtantly... made sure the text segment ends on a 4-byte aligned
boundary. RODATA can be non-word aligned (e.g. if there is an odd-sized
string), and if there's nothing to align it afterwards, it can end up
placing the RAM sections in non-word-aligned flash. On Cortex-M0 this
causes a fault when doing relocation since Cortex-M0 can only do
word-aligned memory loads.

doc: grammar nit

style: update tree with new rustfmt

This commit contains only auto-generated changes from the updated rustfmt.

capsules: add X-MAC mac layer

Bug fix for excessive ENOACK preambles

capsules: add X-MAC mac layer

Bug fix for excessive ENOACK preambles

another bug

layer rework WIP before rustfmt bump

bump nightly to 2018-01-05

No code changes.

update rustfmt (0.7.1 -> 0.3.4) *no code changes*

Yes, 0.7.1 -> 0.3.4 is an update, upstream reset versioning.

This commit only updates the tooling, the subsequent commit updates the
actual code, and minimal style changes such that the new formatter does
not exit with a hard error.

Branches should be able to cherry-pick just this commit and update
formatting locally to minimize merge churn.

fixing random files changed by format bump

rust format fixes

fixing elf2tbf issue from rustfmt update

updated dependencies
@luser

This comment has been minimized.

Contributor

luser commented Feb 24, 2018

My rust-minidump crate has a bunch of packed structs derived from a C header that I ran bindgen on. Now that I've updated to a newer rustc I'm getting a slew of E0133 warnings. The fact that you get a warning using a field from a packed struct in println! is pretty annoying, especially when the field itself is Copy! Is there any way we can make this case more ergonomic?

yodaldevoid added a commit to yodaldevoid/mkl26 that referenced this issue Mar 7, 2018

Put `unsafe` around reads of packed fields
See rust-lang/rust#46043

Signed-off-by: Gabriel Smith <ga29smith@gmail.com>
@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Apr 16, 2018

What's the time-line for turning this warning a hard-error on nightly, and moving towards making it a hard error on stable ? It has been a warning on nightly for a couple of releases already.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Apr 16, 2018

@luser
The println! likely actually takes a reference and uses it, so adding an unsafe block doesn't even fix anything -- it just makes your code compile with UB. Try wrapping the arguments in curly braces:

println!("{}", {self.packed_field});

This should work if they are Copy, should not require an unsafe block because no reference is taken, and properly avoids the UB.


However, I am worried many people do not realize what this unsafety really is about, and just add an unsafe block to silence the warning. I just read over the commit by @yodaldevoid that is linked above because they mentioned this issue (yodaldevoid/mkl26@c1c1d5c), and it contains code like (comment is mine)

impl<'a> Adc<'a> {
    pub fn is_conv_done(&mut self) -> bool {
        // self.reg has type AdcRegs which is packed; then we call RW::read(&self, u32)
        unsafe { self.reg.sc1a.read().get_bit(7) }
  }
}

So, this code is UB if these fields are actually unaligned. @yodaldevoid, it would be interesting to learn why you thought that passing an unaligned reference to RW::read is okay, or did you ensure that the reference is actually aligned despite packing? (Looking at AdcRegs, it consists entirely of u32 and i32, so repr(packed) is likely not even needed?)

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Apr 16, 2018

@RalfJung the current warning message is pretty bad:

See it live (playground):

#[repr(packed)]
struct A {
    x: u8,
    y: u64
}

fn main() {
    let a = A { x: 1, y: 2 };
    let _ = &a.y;
}

outputs:

warning: borrow of packed field requires unsafe function or block (error E0133)
 --> src/main.rs:9:13
  |
9 |     let _ = &a.y;
  |             ^^^^
  |
  = note: #[warn(safe_packed_borrows)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

It says that taking a reference requires an unsafe block, which encourages users to do just that. Open an unsafe block and be done with it. It should at least mention why this is unsafe, and encourage users to fix these issues with care:

warning: borrow of packed field is unsafe (error E0133)
 --> src/main.rs:9:13
  |
9 |     let _ = &a.y;
  |             ^^^^
  |
  = note: #[warn(safe_packed_borrows)] on by default
  = warning: fields of packed structs might be misaligned: dereferencing a misaligned reference is undefined behavior. 
  = note: this is warning will turn into a hard error in the next Rust 1.2x.0 release. Taking references to packed struct fields allows undefined behavior to happen in safe Rust. Fix this with great care - for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

It is also "only" a warning, which encourage people that numbly pursue clean builds to just want to "silence it" without digging deeper. Also some users might conclude that, if this was important, it would be a hard error, saying that it will become an error at some undetermined point in the future doesn't really convey much seriousness.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Apr 16, 2018

@gnzlbg

the current warning message is pretty bad:

Yeah that probably doesn't help. However, I also feel this violates the usual pattern in Rust that you should not make unsafe operations accidentally. The only other non-function-call operations we have that are unsafe involve dereferencing a raw pointer, and everyone kind-of knows why that is unsafe (though they may forget about alignment). That the mere act of taking a reference can be UB will probably be surprising to many. If they are doing this e.g. in an unsafe fn, they will never even see the warning/error, no matter how much we improve it.

I think long-term we want to have an operation (that @arielb1 has proposed several times I think) that directly creates a raw pointer to a field, and we want to make taking a reference to a packed field outright illegal. People should use the raw pointer operations instead. However, I don't even know how the syntax for that should look like.

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Apr 16, 2018

I think long-term we want to have an operation (that @arielb1 has proposed several times I think) that directly creates a raw pointer to a field, and we want to make taking a reference to a packed field outright illegal.

Honestly, I would be fine with making taking references to fields of packed structs illegal at first. For FFI this is not something that's really needed. I don't know how that would interact with derives but at worst they just won't compile and users that want to can manually implement these.

However, I also feel this violates the usual pattern in Rust that you should not make unsafe operations accidentally.

Sure and I agree. As you mention, this is surprising and people can slip which is why we need to be more proactive here.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Apr 16, 2018

Honestly, I would be fine with making taking references to fields of packed structs illegal at first.

Given that this has been possible on stable for several years, I don't think that's realistic.

Though maybe we can make it illegal unless it is immediately followed by as *const _ or as *mut _?

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Apr 16, 2018

Though maybe we can make it illegal unless it is immediately followed by as *const _ or as *mut _?

That would be an improvement.

Given that this has been possible on stable for several years, I don't think that's realistic.

Turning this warning into an error is a breaking change, and breaking changes that fix soundness bugs are "allowed". What complicates this case is that the code might actually be working fine in the platforms that the users are actually using, and that without an escape hatch users don't have a "trivally-easy" way to work around this.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Apr 16, 2018

What complicates this case is that the code might actually be working fine in the platforms that the users are actually using,

We provide alignment annotations to LLVM irrespective of the platform. So, even on platforms where all loads are allowed to be unaligned, this kind of code only "happens to work" in the sense that the compiler was not "smart enough" to exploit the UB.

But yes, what I meant is that at the least, we need an escape hatch. We can't just make it entirely impossible to write such code.

But before we talk about a migration strategy, I think we should have some idea of what the goal is. Do we consider &x as *const _ to be a primitive operation (with its own MIR representation) that immediately creates a raw pointer? In that case, doing this to unaligned fields could even be a safe operation. That's probably pretty confusing though; let-expanding the code would then result in compiler errors or (worse) introduce UB. Or do we actually declare it legal for unsafe code to create an unaligned reference as long as they don't do much with it? But then, what are they allowed to do?

There is another case where we'd like people to use such a raw-ptr operation: Imagine we have a x: *const T which may not be aligned (because it could originate from a packed struct), and now we want to get a pointer to a field. &(*x).field would create an unaligned reference, which is a bad value and actually causes insta-UB under some proposed unsafe code guidelines. We'd rather have people write &(*x).field as *const _ (or whatever the primitive create-raw-ptr looks like) so that the bad reference is never even created.
However, I have no idea how we can enforce this. I imagine plenty of code around packed structs would be UB under this model.

(One reason we want it to be insta-UB is to allow more layout optimizations: Something like Option<Option<&i32>> could be ptr-sized if we could use the bit patterns for 0, 1, 2, 3 as discriminants. All of these are illegal for a 4-aligned pointer like &i32. But if we want to do this, we better make it illegal for there to ever be a reference that is not aligned.)

@RalfJung

This comment has been minimized.

Member

RalfJung commented Apr 19, 2018

In the MaybeUninit discussion, another use case for immediately creating a raw pointer came up: Creating a pointer to an uninitialized field of a union. Creating a reference is invalid because it is not yet initialized, but creating a raw pointer is fine.

@retep998

This comment has been minimized.

Member

retep998 commented May 16, 2018

I just received this warning in winapi in case where it should not occur.

> cargo build --all-features
   Compiling winapi v0.3.4 (file:///C:/Users/Peter/Code/winapi-rs)
warning: #[derive] can't be used on a non-Copy #[repr(packed)] struct (error E0133)
   --> src\macros.rs:354:54
    |
354 |           STRUCT!{#[cfg_attr(feature = "debug", derive(Debug))] $($rest)*}
    |                                                        ^^^^^
    |
   ::: src\um\wingdi.rs:599:1
    |
599 | / STRUCT!{ #[debug] #[repr(packed)] struct BITMAPFILEHEADER {
600 | |     bfType: WORD,
601 | |     bfSize: DWORD,
602 | |     bfReserved1: WORD,
603 | |     bfReserved2: WORD,
604 | |     bfOffBits: DWORD,
605 | | }}
    | |__- in this macro invocation
    |
    = note: #[warn(safe_packed_borrows)] on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

    Finished dev [unoptimized + debuginfo] target(s) in 33.10s

All structs defined by STRUCT! have manual Copy and Clone impls and should never trigger this warning, and yet the fact that is triggering indicates something is wrong with how the warning is implemented. Please fix this and don't you dare turn this into a hard error like this.

@RalfJung

This comment has been minimized.

Member

RalfJung commented May 17, 2018

I reported a minimal example as #50826

don't you dare turn this into a hard error like this.

Calm down, it's just a bug. That's why we have these warning cycles.

@ColinFinck

This comment has been minimized.

Contributor

ColinFinck commented May 30, 2018

For the record, declaring the borrowing of packed structure fields an unsafe operation makes Rust warn about getting the address of a packed structure field (like &fadt_table.x_dsdt as *const _ as usize), too.
However, getting a memory address is a totally safe operation.

Some may already conclude this from the previous comments, but this is the problem I'm currently facing.
Rust definitely needs a warning-free way for getting the address or a raw pointer to a packed structure field before this warning is turned into a hard error.

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented May 30, 2018

Can't we just add an intrinsic that gives you a pointer to a value without going through a reference? Once we have that, if we really need sugar for this, we can add this later.

@rkruppe

This comment has been minimized.

Contributor

rkruppe commented May 30, 2018

Intrinsics are still functions so they can't move a value from the caller and then magically restore it to its original address and return that address. If we want this operation it must be a language concept.

However, since for legacy reasons &<something> as *const _ needs to be work anyway (at least when written exactly like that), the lint should really be updated to not warn on that pattern.

@RalfJung

This comment has been minimized.

Member

RalfJung commented May 30, 2018

Rust definitely needs a warning-free way for getting the address or a raw pointer to a packed structure field before this warning is turned into a hard error.

It has such a way: Add an unsafe block.

We may want to have a safe way as well but IMHO that's much less critical.

RalfJung added a commit to RalfJung/rust that referenced this issue Jul 10, 2018

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 11, 2018

Rollup merge of rust-lang#52207 - RalfJung:unsafety-errors, r=estebank
improve error message shown for unsafe operations

Add a short explanation saying why undefined behavior could arise. In particular, the error many people got for "creating a pointer to a packed field requires unsafe block" was not worded great -- it lead to people just adding the unsafe block without considering if what they are doing follows the rules.

I am not sure if a "note" is the right thing, but that was the easiest thing to add...

Inspired by @gnzlbg at rust-lang#46043 (comment)
@exscape

This comment has been minimized.

Contributor

exscape commented Sep 29, 2018

I'm getting this warning despite deriving Copy and Clone.

extern crate serde;
extern crate serde_json;
#[macro_use] extern crate serde_derive;

#[repr(C, packed)]
#[derive(Copy, Clone, Serialize, Deserialize)]
struct Header {
    len: u16,
    kind: u8
}

fn main() {
}

Error message:

warning: #[derive] can't be used on a #[repr(packed)] struct that does not derive Copy (error E0133)
 --> src/main.rs:6:23
  |
6 | #[derive(Copy, Clone, Serialize, Deserialize)]
  |                       ^^^^^^^^^
  |
  = note: #[warn(safe_packed_borrows)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

This is on rustc 1.30.0-beta.8 (269c1bd 2018-09-27) with edition 2018. The error message above is from the Rust playground as of a few minutes prior to submitting this comment; I just hit "run", so the default toolchain (with the 2015 edition).

@retep998

This comment has been minimized.

Member

retep998 commented Sep 29, 2018

@exscape That's a bug with serde_derive. It isn't noticing the #[repr(packed)] and failing to generate a Serialize implementation that can work with packed fields.

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Oct 1, 2018

Is there a bug open for this in serde upstream?

yodaldevoid added a commit to yodaldevoid/mkl26 that referenced this issue Oct 13, 2018

Put `unsafe` around reads of packed fields
See rust-lang/rust#46043

Signed-off-by: Gabriel Smith <ga29smith@gmail.com>
@emilio

This comment has been minimized.

Contributor

emilio commented Oct 14, 2018

This warns as well on bindgen for packed structs, but we cannot always derive copy and clone (since we don't want to allow copying, structs that contain incomplete arrays for example)... What's the way to go about that? Should the #[derive(Debug)] code learn about these?

See rust-lang/rust-bindgen#1333

LukasKalbertodt added a commit to OsnaCS/plantex that referenced this issue Nov 12, 2018

Fix some warnings
Notably: AxialPoint and AxialVector are not `#[repr(C, packed)]`
anymore. I don't know why they were before, but it seems to work
without. This caused warnings because taking references to fields
of packed structs is bad. See:
rust-lang/rust#46043
@cramertj

This comment has been minimized.

Member

cramertj commented Nov 28, 2018

@emilio It's not possible to implement Debug for non-Copy fields of a packed struct, since calling Debug::fmt on them requires creating an aligned reference to the field, which isn't possible when the field itself isn't Copy (and so can't be moved to a location w/ the proper alignment).

@retep998

This comment has been minimized.

Member

retep998 commented Nov 28, 2018

Of note is that I have since switched winapi to use derive(Copy) instead of impl Copy and that resolved the issue I was having earlier. Still unfortunate that it can't detect impl Copy but it's no longer a blocker on my end.

@cramertj

This comment has been minimized.

Member

cramertj commented Nov 28, 2018

@retep998 ah good to know! Thanks. Yeah, it is unfortunate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment