Skip to content
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

Deprecate Read::initializer in favor of ptr::freeze #58363

Closed
wants to merge 11 commits into from
50 changes: 50 additions & 0 deletions src/libcore/ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,56 @@ pub unsafe fn write_volatile<T>(dst: *mut T, src: T) {
intrinsics::volatile_store(dst, src);
}

/// Freezes `count * size_of::<T>()` bytes of memory, converting undefined data into
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the first time we talk about this kind of data in the docs. I usually call it "uninitialized data" as I feel that is easier to understand. It also is further away from LLVM's undef, which is good -- Rust's "uninitialized data" is much more like poision than undef.

/// defined-but-arbitrary data.
///
/// Uninitialized memory has undefined contents, and interation with that data
/// can easily cause undefined behavior. This function "freezes" memory
/// contents, converting uninitialized memory to initialized memory with
/// arbitrary conents so that use of it is well defined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// arbitrary conents so that use of it is well defined.
/// arbitrary contents so that use of it is well defined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"arbitrary but fixed contents" might be a better formulation.

Also it might be worth noting that use is only well defined for integer type -- even with arbitrary but fixed contents, using this with bool or &T is UB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it might be worth noting that use is only well defined for integer type -- even with arbitrary but fixed contents, using this with bool or &T is UB.

That's a great point! Definitely worth noting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this phrased as "Every bit representation of T must be a valid value", but I don't think that's the best way of saying that. Ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this phrased as "Every bit representation of T must be a valid value",

Is there perhaps an auto-trait waiting to be invented for that? We could ostensibly make the API a bit safer by adding a constraint T: TheTrait...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's been talk of this kind of thing for quite a while (pub auto trait Pod {}), but I think that'd be relevant as part of a safe interface over this specific function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can be an auto trait -- auto traits are always implemented for fieldless enums, but this function is not sufficient to make a fieldless enum valid.

///
/// This function has no runtime effect; it is purely an instruction to the
/// compiler. In particular, it does not actually write anything to the memory.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function does have an effect in the "Rust abstract machine" though, not just in the compiler. And of course it has a run-time effect by inhibiting optimizations.

Maybe a comparison with a compiler fence helps? Those also clearly have an effect on runtime behavior even though they do not have a runtime effect themselves.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, also I think we should be clear about this counting as a write access as far as mutability and data races are concerned.

Doing this on a shared reference is UB.

///
/// # Safety
///
/// Behavior is undefined if any of the following conditions are violated:
///
/// * `dst` must be [valid] for reads.
sfackler marked this conversation as resolved.
Show resolved Hide resolved
///
/// Note that even if `T` has size `0`, the pointer must be non-NULL and properly aligned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Note that even if `T` has size `0`, the pointer must be non-NULL and properly aligned.
/// Note that even if `size_of::<T>() == 0`, the pointer must be non-NULL and properly aligned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this formulation is used consistently throughout this file, so I'd prefer that if it gets changed that happens in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah; that's a good idea; I'll see if I can remember to make such a PR... :)

///
/// [valid]: ../ptr/index.html#safety
///
/// # Examples
///
/// ```ignore (io-is-in-std)
/// use std::io::{self, Read};
/// use std::mem;
/// use std::ptr;
///
/// pub fn read_le_u32<R>(reader: &mut R) -> io::Result<u32>
/// where
/// R: Read,
/// {
/// unsafe {
/// // We're passing this buffer to an arbitrary reader and aren't
/// // guaranteed they won't read from it, so freeze to avoid UB.
/// let mut buf: [u8; 4] = mem::uninitialized();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a FIXME somewhere about porting this to MaybeUninit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is still open, from what I can see)

/// ptr::freeze(&mut buf, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use buf.as_mut_ptr() and 4?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it really matters either way - we're either freezing a single [u8; 4] value, or 4 u8 values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sure, I just figured it was a bit odd compared to how we'd expect it to idiomatically be used

/// reader.read_exact(&mut buf)?;
///
/// Ok(u32::from_le_bytes(buf))
/// }
/// }
/// ```
#[inline]
#[unstable(feature = "ptr_freeze", issue = "0")]
pub unsafe fn freeze<T>(dst: *mut T, count: usize) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right interface? It's currently a bit weird in that we don't actually use the count. It could alternatively just take the pointer, and say that it freezes all memory reachable through it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should this be unsafe in the first place? Since it's not actually modifying any of the pointed-to data, does it matter if it's valid or not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's already "basically stable", I wonder if this should take &mut [T] and move to std::mem?

I'd naively think that it could be safe and probably should be, but I'm not an expert!

We also briefly discussed maybe only taking u8 for now? I'm not sure how useful this would be beyond u8 and other POD types

Copy link
Member

@RalfJung RalfJung Feb 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should take raw pointers so people don't have to create references to uninitialized data.

count being unused just comes from the fact that LLVM does not support "real" freeze, but I think this is a much better interface than "reachable from".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change this to T: ?Sized so that you can pass a slice in? Then the count parameter would no longer be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK there's currently no way to form a *mut [T] without going through a reference first.

let _ = count;
asm!("" : "=*m"(dst) : );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constraints here are incorrect since they only tell LLVM that the inline asm is writing to one element of dst.

What you want is something like this: asm!("" :: "r" (dst) : "memory" : "volatile");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I just noticed that the current constraints indicate that the inline asm is overwriting the value in dst without reading it, which means that LLVM is free to discard any previous writes to dst which might have initialized it to a particular value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would someone be using this function if they've already initialized the memory?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you want is something like this: asm!("" :: "r" (dst) : "memory" : "volatile");

The memory clobber handles all memory, right? Why do we need to pass in dst in that case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would someone be using this function if they've already initialized the memory?

The docs for this function say that this function has no runtime effect. I would assume that this means that already-initialized data passed to this function would remain untouched.

The memory clobber handles all memory, right? Why do we need to pass in dst in that case?

The memory clobber tells LLVM to assume that any memory whose address might be visible to other threads has been modified. We pass the address to the inline asm so that LLVM must assume the address is globally visible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Clobbers all memory" does not mean literally all memory in naive source language semantics, e.g. certainly in let x = 1; asm!(... : "memory" : "volatile"); print(x); the variable x will be optimized out. It's nebulous (to me at least) what the rules are but it's definitely safer to make the pointer actually available to the asm block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would someone be using this function if they've already initialized the memory?

First of all, you called it "freeze", and that is what LLVM's proposed "freeze" does: pick non-deterministic values for uninitialized data but keep initialized data intact.

Second, we have a use-case for that in crossbeam-rs/crossbeam#315.

}

#[lang = "const_ptr"]
impl<T: ?Sized> *const T {
/// Returns `true` if the pointer is null.
Expand Down
6 changes: 5 additions & 1 deletion src/libstd/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@

use fmt;
use ffi::OsString;
use io::{self, SeekFrom, Seek, Read, Initializer, Write};
use io::{self, SeekFrom, Seek, Read, Write};
#[allow(deprecated)]
use io::Initializer;
use path::{Path, PathBuf};
use sys::fs as fs_imp;
use sys_common::{AsInnerMut, FromInner, AsInner, IntoInner};
Expand Down Expand Up @@ -600,6 +602,7 @@ impl Read for File {
}

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand All @@ -624,6 +627,7 @@ impl<'a> Read for &'a File {
}

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down
8 changes: 6 additions & 2 deletions src/libstd/io/buffered.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ use io::prelude::*;
use cmp;
use error;
use fmt;
use io::{self, Initializer, DEFAULT_BUF_SIZE, Error, ErrorKind, SeekFrom};
use io::{self, DEFAULT_BUF_SIZE, Error, ErrorKind, SeekFrom};
#[allow(deprecated)]
use io::Initializer;
use memchr;
use ptr;

/// The `BufReader` struct adds buffering to any reader.
///
Expand Down Expand Up @@ -92,7 +95,7 @@ impl<R: Read> BufReader<R> {
unsafe {
let mut buffer = Vec::with_capacity(cap);
buffer.set_len(cap);
inner.initializer().initialize(&mut buffer);
ptr::freeze(buffer.as_mut_ptr(), cap);
BufReader {
inner,
buf: buffer.into_boxed_slice(),
Expand Down Expand Up @@ -236,6 +239,7 @@ impl<R: Read> Read for BufReader<R> {
}

// we can't skip unconditionally because of the large buffer case in read.
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
self.inner.initializer()
}
Expand Down
5 changes: 4 additions & 1 deletion src/libstd/io/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ use io::prelude::*;

use core::convert::TryInto;
use cmp;
use io::{self, Initializer, SeekFrom, Error, ErrorKind};
use io::{self, SeekFrom, Error, ErrorKind};
#[allow(deprecated)]
use io::Initializer;

/// A `Cursor` wraps an in-memory buffer and provides it with a
/// [`Seek`] implementation.
Expand Down Expand Up @@ -229,6 +231,7 @@ impl<T> Read for Cursor<T> where T: AsRef<[u8]> {
}

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down
7 changes: 6 additions & 1 deletion src/libstd/io/impls.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use cmp;
use io::{self, SeekFrom, Read, Initializer, Write, Seek, BufRead, Error, ErrorKind};
use io::{self, SeekFrom, Read, Write, Seek, BufRead, Error, ErrorKind};
#[allow(deprecated)]
use io::Initializer;
use fmt;
use mem;

Expand All @@ -14,6 +16,7 @@ impl<'a, R: Read + ?Sized> Read for &'a mut R {
}

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
(**self).initializer()
}
Expand Down Expand Up @@ -83,6 +86,7 @@ impl<R: Read + ?Sized> Read for Box<R> {
}

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
(**self).initializer()
}
Expand Down Expand Up @@ -172,6 +176,7 @@ impl<'a> Read for &'a [u8] {
}

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down
21 changes: 19 additions & 2 deletions src/libstd/io/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ fn read_to_end_with_reservation<R: Read + ?Sized>(r: &mut R,
g.buf.reserve(reservation_size);
let capacity = g.buf.capacity();
g.buf.set_len(capacity);
r.initializer().initialize(&mut g.buf[g.len..]);
ptr::freeze(g.buf.as_mut_ptr().add(g.len), g.buf.len() - g.len);
}
}

Expand Down Expand Up @@ -543,6 +543,8 @@ pub trait Read {
/// [`Initializer::nop()`]: ../../std/io/struct.Initializer.html#method.nop
/// [`Initializer`]: ../../std/io/struct.Initializer.html
#[unstable(feature = "read_initializer", issue = "42788")]
#[rustc_deprecated(since = "1.33.0", reason = "use std::ptr::freeze instead")]
#[allow(deprecated)]
#[inline]
unsafe fn initializer(&self) -> Initializer {
Initializer::zeroing()
Expand Down Expand Up @@ -869,12 +871,22 @@ pub trait Read {

/// A type used to conditionally initialize buffers passed to `Read` methods.
#[unstable(feature = "read_initializer", issue = "42788")]
#[derive(Debug)]
#[rustc_deprecated(since = "1.33.0", reason = "use std::ptr::freeze instead")]
pub struct Initializer(bool);

#[allow(deprecated)]
#[unstable(feature = "read_initializer", issue = "42788")]
impl fmt::Debug for Initializer {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt.debug_tuple("Initializer").field(&self.0).finish()
}
}

#[allow(deprecated)]
impl Initializer {
/// Returns a new `Initializer` which will zero out buffers.
#[unstable(feature = "read_initializer", issue = "42788")]
#[rustc_deprecated(since = "1.33.0", reason = "use std::ptr::freeze instead")]
#[inline]
pub fn zeroing() -> Initializer {
Initializer(true)
Expand All @@ -889,20 +901,23 @@ impl Initializer {
/// the method accurately reflects the number of bytes that have been
/// written to the head of the buffer.
#[unstable(feature = "read_initializer", issue = "42788")]
#[rustc_deprecated(since = "1.33.0", reason = "use std::ptr::freeze instead")]
#[inline]
pub unsafe fn nop() -> Initializer {
Initializer(false)
}

/// Indicates if a buffer should be initialized.
#[unstable(feature = "read_initializer", issue = "42788")]
#[rustc_deprecated(since = "1.33.0", reason = "use std::ptr::freeze instead")]
#[inline]
pub fn should_initialize(&self) -> bool {
self.0
}

/// Initializes a buffer if necessary.
#[unstable(feature = "read_initializer", issue = "42788")]
#[rustc_deprecated(since = "1.33.0", reason = "use std::ptr::freeze instead")]
#[inline]
pub fn initialize(&self, buf: &mut [u8]) {
if self.should_initialize() {
Expand Down Expand Up @@ -1698,6 +1713,7 @@ impl<T: Read, U: Read> Read for Chain<T, U> {
self.second.read(buf)
}

#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
let initializer = self.first.initializer();
if initializer.should_initialize() {
Expand Down Expand Up @@ -1895,6 +1911,7 @@ impl<T: Read> Read for Take<T> {
Ok(n)
}

#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
self.inner.initializer()
}
Expand Down
7 changes: 6 additions & 1 deletion src/libstd/io/stdio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ use io::prelude::*;
use cell::RefCell;
use fmt;
use io::lazy::Lazy;
use io::{self, Initializer, BufReader, LineWriter};
use io::{self, BufReader, LineWriter};
#[allow(deprecated)]
use io::Initializer;
use sync::{Arc, Mutex, MutexGuard};
use sys::stdio;
use sys_common::remutex::{ReentrantMutex, ReentrantMutexGuard};
Expand Down Expand Up @@ -67,6 +69,7 @@ impl Read for StdinRaw {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { self.0.read(buf) }

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down Expand Up @@ -297,6 +300,7 @@ impl Read for Stdin {
self.lock().read(buf)
}
#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand All @@ -317,6 +321,7 @@ impl<'a> Read for StdinLock<'a> {
self.inner.read(buf)
}
#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down
9 changes: 7 additions & 2 deletions src/libstd/io/util.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
#![allow(missing_copy_implementations)]

use fmt;
use io::{self, Read, Initializer, Write, ErrorKind, BufRead};
use io::{self, Read, Write, ErrorKind, BufRead};
#[allow(deprecated)]
use io::Initializer;
use mem;
use ptr;

/// Copies the entire contents of a reader into a writer.
///
Expand Down Expand Up @@ -45,7 +48,7 @@ pub fn copy<R: ?Sized, W: ?Sized>(reader: &mut R, writer: &mut W) -> io::Result<
{
let mut buf = unsafe {
let mut buf: [u8; super::DEFAULT_BUF_SIZE] = mem::uninitialized();
reader.initializer().initialize(&mut buf);
ptr::freeze(&mut buf, 1);
buf
};

Expand Down Expand Up @@ -97,6 +100,7 @@ impl Read for Empty {
fn read(&mut self, _buf: &mut [u8]) -> io::Result<usize> { Ok(0) }

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down Expand Up @@ -153,6 +157,7 @@ impl Read for Repeat {
}

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down
1 change: 1 addition & 0 deletions src/libstd/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@
#![feature(panic_unwind)]
#![feature(prelude_import)]
#![feature(ptr_internals)]
#![feature(ptr_freeze)]
#![feature(raw)]
#![feature(hash_raw_entry)]
#![feature(rustc_attrs)]
Expand Down
6 changes: 5 additions & 1 deletion src/libstd/net/tcp.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use io::prelude::*;

use fmt;
use io::{self, Initializer};
use io;
#[allow(deprecated)]
use io::Initializer;
use net::{ToSocketAddrs, SocketAddr, Shutdown};
use sys_common::net as net_imp;
use sys_common::{AsInner, FromInner, IntoInner};
Expand Down Expand Up @@ -570,6 +572,7 @@ impl Read for TcpStream {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { self.0.read(buf) }

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand All @@ -584,6 +587,7 @@ impl<'a> Read for &'a TcpStream {
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { self.0.read(buf) }

#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down
6 changes: 5 additions & 1 deletion src/libstd/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ use io::prelude::*;
use ffi::OsStr;
use fmt;
use fs;
use io::{self, Initializer};
use io;
#[allow(deprecated)]
use io::Initializer;
use path::Path;
use str;
use sys::pipe::{read2, AnonPipe};
Expand Down Expand Up @@ -272,6 +274,7 @@ impl Read for ChildStdout {
self.inner.read(buf)
}
#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down Expand Up @@ -319,6 +322,7 @@ impl Read for ChildStderr {
self.inner.read(buf)
}
#[inline]
#[allow(deprecated)]
unsafe fn initializer(&self) -> Initializer {
Initializer::nop()
}
Expand Down
Loading