From 781ac3e777a5f47bdfaba05ee17f8b79845670b1 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Sun, 4 May 2014 23:17:37 +1000 Subject: [PATCH 1/2] std: deprecate cast::transmute_mut. Turning a `&T` into an `&mut T` carries a large risk of undefined behaviour, and needs to be done very very carefully. Providing a convenience function for exactly this task is a bad idea, just tempting people into doing the wrong thing. The right thing is to use types like `Cell`, `RefCell` or `Unsafe`. For memory safety, Rust has that guarantee that `&mut` pointers do not alias with any other pointer, that is, if you have a `&mut T` then that is the only usable pointer to that `T`. This allows Rust to assume that writes through a `&mut T` do not affect the values of any other `&` or `&mut` references. `&` pointers have no guarantees about aliasing or not, so it's entirely possible for the same pointer to be passed into both arguments of a function like fn foo(x: &int, y: &int) { ... } Converting either of `x` or `y` to a `&mut` pointer and modifying it would affect the other value: invalid behaviour. (Similarly, it's undefined behaviour to modify the value of an immutable local, like `let x = 1;`.) At a low-level, the *only* safe way to obtain an `&mut` out of a `&` is using the `Unsafe` type (there are higher level wrappers around it, like `Cell`, `RefCell`, `Mutex` etc.). The `Unsafe` type is registered with the compiler so that it can reason a little about these `&` to `&mut` casts, but it is still up to the user to ensure that the `&mut`s obtained out of an `Unsafe` never alias. (Note that *any* conversion from `&` to `&mut` can be invalid, including a plain `transmute`, or casting `&T` -> `*T` -> `*mut T` -> `&mut T`.) [breaking-change] --- src/libarena/lib.rs | 9 +++++---- src/libnative/io/file_win32.rs | 3 ++- src/librustuv/file.rs | 3 ++- src/libstd/cast.rs | 1 + src/libstd/comm/mod.rs | 19 ++++++++++++------- src/libstd/local_data.rs | 4 ++-- src/libstd/slice.rs | 20 ++++++++++++-------- src/libsync/arc.rs | 2 +- 8 files changed, 37 insertions(+), 24 deletions(-) diff --git a/src/libarena/lib.rs b/src/libarena/lib.rs index 95062d84e09..d80385587da 100644 --- a/src/libarena/lib.rs +++ b/src/libarena/lib.rs @@ -26,7 +26,7 @@ extern crate collections; -use std::cast::{transmute, transmute_mut, transmute_mut_lifetime}; +use std::cast::{transmute, transmute_mut_lifetime}; use std::cast; use std::cell::{Cell, RefCell}; use std::mem; @@ -281,8 +281,8 @@ impl Arena { #[inline] pub fn alloc<'a, T>(&'a self, op: || -> T) -> &'a T { unsafe { - // FIXME: Borrow check - let this = transmute_mut(self); + // FIXME #13933: Remove/justify all `&T` to `&mut T` transmutes + let this: &mut Arena = transmute::<&_, &mut _>(self); if intrinsics::needs_drop::() { this.alloc_noncopy(op) } else { @@ -438,7 +438,8 @@ impl TypedArena { #[inline] pub fn alloc<'a>(&'a self, object: T) -> &'a T { unsafe { - let this = cast::transmute_mut(self); + // FIXME #13933: Remove/justify all `&T` to `&mut T` transmutes + let this: &mut TypedArena = cast::transmute::<&_, &mut _>(self); if this.ptr == this.end { this.grow() } diff --git a/src/libnative/io/file_win32.rs b/src/libnative/io/file_win32.rs index 88ba6dcbc7e..aae15a86614 100644 --- a/src/libnative/io/file_win32.rs +++ b/src/libnative/io/file_win32.rs @@ -174,7 +174,8 @@ impl rtio::RtioFileStream for FileDesc { fn tell(&self) -> Result { // This transmute is fine because our seek implementation doesn't // actually use the mutable self at all. - unsafe { cast::transmute_mut(self).seek(0, io::SeekCur) } + // FIXME #13933: Remove/justify all `&T` to `&mut T` transmutes + unsafe { cast::transmute::<&_, &mut FileDesc>(self).seek(0, io::SeekCur) } } fn fsync(&mut self) -> Result<(), IoError> { diff --git a/src/librustuv/file.rs b/src/librustuv/file.rs index 665d418ab2a..9037363f9a0 100644 --- a/src/librustuv/file.rs +++ b/src/librustuv/file.rs @@ -445,7 +445,8 @@ impl rtio::RtioFileStream for FileWatcher { fn tell(&self) -> Result { use libc::SEEK_CUR; // this is temporary - let self_ = unsafe { cast::transmute_mut(self) }; + // FIXME #13933: Remove/justify all `&T` to `&mut T` transmutes + let self_ = unsafe { cast::transmute::<&_, &mut FileWatcher>(self) }; self_.seek_common(0, SEEK_CUR) } fn fsync(&mut self) -> Result<(), IoError> { diff --git a/src/libstd/cast.rs b/src/libstd/cast.rs index bcbf804d7b3..7a8f517bbf9 100644 --- a/src/libstd/cast.rs +++ b/src/libstd/cast.rs @@ -60,6 +60,7 @@ pub unsafe fn transmute(thing: L) -> G { /// Coerce an immutable reference to be mutable. #[inline] +#[deprecated="casting &T to &mut T is undefined behaviour: use Cell, RefCell or Unsafe"] pub unsafe fn transmute_mut<'a,T>(ptr: &'a T) -> &'a mut T { transmute(ptr) } /// Coerce a reference to have an arbitrary associated lifetime. diff --git a/src/libstd/comm/mod.rs b/src/libstd/comm/mod.rs index 92e3e82c1c5..bbe34d20f6a 100644 --- a/src/libstd/comm/mod.rs +++ b/src/libstd/comm/mod.rs @@ -318,6 +318,11 @@ mod stream; mod shared; mod sync; +// FIXME #13933: Remove/justify all `&T` to `&mut T` transmutes +unsafe fn transmute_mut<'a,T>(x: &'a T) -> &'a mut T { + cast::transmute::<&_, &mut _>(x) +} + // Use a power of 2 to allow LLVM to optimize to something that's not a // division, this is hit pretty regularly. static RESCHED_FREQ: int = 256; @@ -565,7 +570,7 @@ impl Sender { unsafe { let mut tmp = Sender::new(Stream(new_inner)); - mem::swap(&mut cast::transmute_mut(self).inner, &mut tmp.inner); + mem::swap(&mut transmute_mut(self).inner, &mut tmp.inner); } return ret; } @@ -599,7 +604,7 @@ impl Clone for Sender { (*packet.get()).inherit_blocker(sleeper); let mut tmp = Sender::new(Shared(packet.clone())); - mem::swap(&mut cast::transmute_mut(self).inner, &mut tmp.inner); + mem::swap(&mut transmute_mut(self).inner, &mut tmp.inner); } Sender::new(Shared(packet)) } @@ -790,7 +795,7 @@ impl Receiver { } }; unsafe { - mem::swap(&mut cast::transmute_mut(self).inner, + mem::swap(&mut transmute_mut(self).inner, &mut new_port.inner); } } @@ -837,7 +842,7 @@ impl Receiver { Sync(ref p) => return unsafe { (*p.get()).recv() } }; unsafe { - mem::swap(&mut cast::transmute_mut(self).inner, + mem::swap(&mut transmute_mut(self).inner, &mut new_port.inner); } } @@ -874,7 +879,7 @@ impl select::Packet for Receiver { } }; unsafe { - mem::swap(&mut cast::transmute_mut(self).inner, + mem::swap(&mut transmute_mut(self).inner, &mut new_port.inner); } } @@ -906,7 +911,7 @@ impl select::Packet for Receiver { }; task = t; unsafe { - mem::swap(&mut cast::transmute_mut(self).inner, + mem::swap(&mut transmute_mut(self).inner, &mut new_port.inner); } } @@ -930,7 +935,7 @@ impl select::Packet for Receiver { let mut new_port = match result { Ok(b) => return b, Err(p) => p }; was_upgrade = true; unsafe { - mem::swap(&mut cast::transmute_mut(self).inner, + mem::swap(&mut transmute_mut(self).inner, &mut new_port.inner); } } diff --git a/src/libstd/local_data.rs b/src/libstd/local_data.rs index bc6e324a5f7..e5c7ba4aa54 100644 --- a/src/libstd/local_data.rs +++ b/src/libstd/local_data.rs @@ -196,11 +196,11 @@ pub fn get_mut(key: Key, f: |Option<&mut T>| -> U) -> U { match x { None => f(None), // We're violating a lot of compiler guarantees with this - // invocation of `transmute_mut`, but we're doing runtime checks to + // invocation of `transmute`, but we're doing runtime checks to // ensure that it's always valid (only one at a time). // // there is no need to be upset! - Some(x) => { f(Some(unsafe { cast::transmute_mut(x) })) } + Some(x) => { f(Some(unsafe { cast::transmute::<&_, &mut _>(x) })) } } }) } diff --git a/src/libstd/slice.rs b/src/libstd/slice.rs index cb8550d248e..d8c866ef44a 100644 --- a/src/libstd/slice.rs +++ b/src/libstd/slice.rs @@ -1739,7 +1739,9 @@ impl<'a,T> MutableVector<'a, T> for &'a mut [T] { if self.len() == 0 { return None; } unsafe { let s: &mut Slice = transmute(self); - Some(cast::transmute_mut(&*raw::shift_ptr(s))) + // FIXME #13933: this `&` -> `&mut` cast is a little + // dubious + Some(&mut *(raw::shift_ptr(s) as *mut _)) } } @@ -1747,7 +1749,9 @@ impl<'a,T> MutableVector<'a, T> for &'a mut [T] { if self.len() == 0 { return None; } unsafe { let s: &mut Slice = transmute(self); - Some(cast::transmute_mut(&*raw::pop_ptr(s))) + // FIXME #13933: this `&` -> `&mut` cast is a little + // dubious + Some(&mut *(raw::pop_ptr(s) as *mut _)) } } @@ -3108,23 +3112,23 @@ mod tests { #[should_fail] fn test_from_elem_fail() { use cast; + use cell::Cell; use rc::Rc; struct S { - f: int, + f: Cell, boxes: (~int, Rc) } impl Clone for S { fn clone(&self) -> S { - let s = unsafe { cast::transmute_mut(self) }; - s.f += 1; - if s.f == 10 { fail!() } - S { f: s.f, boxes: s.boxes.clone() } + self.f.set(self.f.get() + 1); + if self.f.get() == 10 { fail!() } + S { f: self.f, boxes: self.boxes.clone() } } } - let s = S { f: 0, boxes: (box 0, Rc::new(0)) }; + let s = S { f: Cell::new(0), boxes: (box 0, Rc::new(0)) }; let _ = Vec::from_elem(100, s); } diff --git a/src/libsync/arc.rs b/src/libsync/arc.rs index b5c66075952..f5369ec862f 100644 --- a/src/libsync/arc.rs +++ b/src/libsync/arc.rs @@ -148,7 +148,7 @@ impl Arc { // reference count is guaranteed to be 1 at this point, and we required // the Arc itself to be `mut`, so we're returning the only possible // reference to the inner data. - unsafe { cast::transmute_mut(self.deref()) } + unsafe { cast::transmute::<&_, &mut _>(self.deref()) } } } From edd9bad4eeb1367464e190d63113600fca0ef25a Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Sun, 4 May 2014 23:35:48 +1000 Subject: [PATCH 2/2] std::comm: use Unsafe to avoid U.B. & -> &mut transmutes. --- src/libstd/comm/mod.rs | 83 +++++++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 34 deletions(-) diff --git a/src/libstd/comm/mod.rs b/src/libstd/comm/mod.rs index bbe34d20f6a..c7849892465 100644 --- a/src/libstd/comm/mod.rs +++ b/src/libstd/comm/mod.rs @@ -271,7 +271,6 @@ // And now that you've seen all the races that I found and attempted to fix, // here's the code for you to find some more! -use cast; use cell::Cell; use clone::Clone; use iter::Iterator; @@ -284,6 +283,7 @@ use result::{Ok, Err, Result}; use rt::local::Local; use rt::task::{Task, BlockedTask}; use sync::arc::UnsafeArc; +use ty::Unsafe; pub use comm::select::{Select, Handle}; @@ -318,11 +318,6 @@ mod stream; mod shared; mod sync; -// FIXME #13933: Remove/justify all `&T` to `&mut T` transmutes -unsafe fn transmute_mut<'a,T>(x: &'a T) -> &'a mut T { - cast::transmute::<&_, &mut _>(x) -} - // Use a power of 2 to allow LLVM to optimize to something that's not a // division, this is hit pretty regularly. static RESCHED_FREQ: int = 256; @@ -330,7 +325,7 @@ static RESCHED_FREQ: int = 256; /// The receiving-half of Rust's channel type. This half can only be owned by /// one task pub struct Receiver { - inner: Flavor, + inner: Unsafe>, receives: Cell, // can't share in an arc marker: marker::NoShare, @@ -346,7 +341,7 @@ pub struct Messages<'a, T> { /// The sending-half of Rust's asynchronous channel type. This half can only be /// owned by one task, but it can be cloned to send to other tasks. pub struct Sender { - inner: Flavor, + inner: Unsafe>, sends: Cell, // can't share in an arc marker: marker::NoShare, @@ -395,6 +390,27 @@ enum Flavor { Sync(UnsafeArc>), } +#[doc(hidden)] +trait UnsafeFlavor { + fn inner_unsafe<'a>(&'a self) -> &'a Unsafe>; + unsafe fn mut_inner<'a>(&'a self) -> &'a mut Flavor { + &mut *self.inner_unsafe().get() + } + unsafe fn inner<'a>(&'a self) -> &'a Flavor { + &*self.inner_unsafe().get() + } +} +impl UnsafeFlavor for Sender { + fn inner_unsafe<'a>(&'a self) -> &'a Unsafe> { + &self.inner + } +} +impl UnsafeFlavor for Receiver { + fn inner_unsafe<'a>(&'a self) -> &'a Unsafe> { + &self.inner + } +} + /// Creates a new asynchronous channel, returning the sender/receiver halves. /// /// All data sent on the sender will become available on the receiver, and no @@ -463,7 +479,7 @@ pub fn sync_channel(bound: uint) -> (SyncSender, Receiver) { impl Sender { fn new(inner: Flavor) -> Sender { - Sender { inner: inner, sends: Cell::new(0), marker: marker::NoShare } + Sender { inner: Unsafe::new(inner), sends: Cell::new(0), marker: marker::NoShare } } /// Sends a value along this channel to be received by the corresponding @@ -537,7 +553,7 @@ impl Sender { task.map(|t| t.maybe_yield()); } - let (new_inner, ret) = match self.inner { + let (new_inner, ret) = match *unsafe { self.inner() } { Oneshot(ref p) => { let p = p.get(); unsafe { @@ -569,8 +585,8 @@ impl Sender { }; unsafe { - let mut tmp = Sender::new(Stream(new_inner)); - mem::swap(&mut transmute_mut(self).inner, &mut tmp.inner); + let tmp = Sender::new(Stream(new_inner)); + mem::swap(self.mut_inner(), tmp.mut_inner()); } return ret; } @@ -578,7 +594,7 @@ impl Sender { impl Clone for Sender { fn clone(&self) -> Sender { - let (packet, sleeper) = match self.inner { + let (packet, sleeper) = match *unsafe { self.inner() } { Oneshot(ref p) => { let (a, b) = UnsafeArc::new2(shared::Packet::new()); match unsafe { (*p.get()).upgrade(Receiver::new(Shared(a))) } { @@ -603,8 +619,8 @@ impl Clone for Sender { unsafe { (*packet.get()).inherit_blocker(sleeper); - let mut tmp = Sender::new(Shared(packet.clone())); - mem::swap(&mut transmute_mut(self).inner, &mut tmp.inner); + let tmp = Sender::new(Shared(packet.clone())); + mem::swap(self.mut_inner(), tmp.mut_inner()); } Sender::new(Shared(packet)) } @@ -613,7 +629,7 @@ impl Clone for Sender { #[unsafe_destructor] impl Drop for Sender { fn drop(&mut self) { - match self.inner { + match *unsafe { self.mut_inner() } { Oneshot(ref mut p) => unsafe { (*p.get()).drop_chan(); }, Stream(ref mut p) => unsafe { (*p.get()).drop_chan(); }, Shared(ref mut p) => unsafe { (*p.get()).drop_chan(); }, @@ -710,7 +726,7 @@ impl Drop for SyncSender { impl Receiver { fn new(inner: Flavor) -> Receiver { - Receiver { inner: inner, receives: Cell::new(0), marker: marker::NoShare } + Receiver { inner: Unsafe::new(inner), receives: Cell::new(0), marker: marker::NoShare } } /// Blocks waiting for a value on this receiver @@ -762,7 +778,7 @@ impl Receiver { } loop { - let mut new_port = match self.inner { + let new_port = match *unsafe { self.inner() } { Oneshot(ref p) => { match unsafe { (*p.get()).try_recv() } { Ok(t) => return Ok(t), @@ -795,8 +811,8 @@ impl Receiver { } }; unsafe { - mem::swap(&mut transmute_mut(self).inner, - &mut new_port.inner); + mem::swap(self.mut_inner(), + new_port.mut_inner()); } } } @@ -815,7 +831,7 @@ impl Receiver { /// the value found on the receiver is returned. pub fn recv_opt(&self) -> Result { loop { - let mut new_port = match self.inner { + let new_port = match *unsafe { self.inner() } { Oneshot(ref p) => { match unsafe { (*p.get()).recv() } { Ok(t) => return Ok(t), @@ -842,8 +858,7 @@ impl Receiver { Sync(ref p) => return unsafe { (*p.get()).recv() } }; unsafe { - mem::swap(&mut transmute_mut(self).inner, - &mut new_port.inner); + mem::swap(self.mut_inner(), new_port.mut_inner()); } } } @@ -858,7 +873,7 @@ impl Receiver { impl select::Packet for Receiver { fn can_recv(&self) -> bool { loop { - let mut new_port = match self.inner { + let new_port = match *unsafe { self.inner() } { Oneshot(ref p) => { match unsafe { (*p.get()).can_recv() } { Ok(ret) => return ret, @@ -879,15 +894,15 @@ impl select::Packet for Receiver { } }; unsafe { - mem::swap(&mut transmute_mut(self).inner, - &mut new_port.inner); + mem::swap(self.mut_inner(), + new_port.mut_inner()); } } } fn start_selection(&self, mut task: BlockedTask) -> Result<(), BlockedTask>{ loop { - let (t, mut new_port) = match self.inner { + let (t, new_port) = match *unsafe { self.inner() } { Oneshot(ref p) => { match unsafe { (*p.get()).start_selection(task) } { oneshot::SelSuccess => return Ok(()), @@ -911,8 +926,8 @@ impl select::Packet for Receiver { }; task = t; unsafe { - mem::swap(&mut transmute_mut(self).inner, - &mut new_port.inner); + mem::swap(self.mut_inner(), + new_port.mut_inner()); } } } @@ -920,7 +935,7 @@ impl select::Packet for Receiver { fn abort_selection(&self) -> bool { let mut was_upgrade = false; loop { - let result = match self.inner { + let result = match *unsafe { self.inner() } { Oneshot(ref p) => unsafe { (*p.get()).abort_selection() }, Stream(ref p) => unsafe { (*p.get()).abort_selection(was_upgrade) @@ -932,11 +947,11 @@ impl select::Packet for Receiver { (*p.get()).abort_selection() }, }; - let mut new_port = match result { Ok(b) => return b, Err(p) => p }; + let new_port = match result { Ok(b) => return b, Err(p) => p }; was_upgrade = true; unsafe { - mem::swap(&mut transmute_mut(self).inner, - &mut new_port.inner); + mem::swap(self.mut_inner(), + new_port.mut_inner()); } } } @@ -949,7 +964,7 @@ impl<'a, T: Send> Iterator for Messages<'a, T> { #[unsafe_destructor] impl Drop for Receiver { fn drop(&mut self) { - match self.inner { + match *unsafe { self.mut_inner() } { Oneshot(ref mut p) => unsafe { (*p.get()).drop_port(); }, Stream(ref mut p) => unsafe { (*p.get()).drop_port(); }, Shared(ref mut p) => unsafe { (*p.get()).drop_port(); },