From 56087074c655bc17af048ad6483ecf7c32ce1762 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Thu, 9 Jun 2022 12:34:23 +0200 Subject: [PATCH 1/5] Stabilize `Path::try_exists()` and improve doc This stabilizes the `Path::try_exists()` method which returns `Result` instead of `bool` allowing handling of errors unrelated to the file not existing. (e.g permission errors) Along with the stabilization it also: * Warns that the `exists()` method is error-prone and suggests to use the newly stabilized one. * Suggests it instead of `metadata()` to handle errors. * Mentions TOCTOU bugs to avoid false assumption that `try_exists()` is completely safe fixed version of `exists()`. * Renames the feature of still-unstable `std::fs::try_exists()` to `fs_try_exists` to avoid name conflict. The tracking issue #83186 remains open to track `fs_try_exists`. --- compiler/rustc_error_messages/src/lib.rs | 1 - library/std/src/fs.rs | 8 ++++++-- library/std/src/path.rs | 17 +++++++++++------ 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_error_messages/src/lib.rs b/compiler/rustc_error_messages/src/lib.rs index fd4b2daae9c13..b21ff0e90836d 100644 --- a/compiler/rustc_error_messages/src/lib.rs +++ b/compiler/rustc_error_messages/src/lib.rs @@ -1,6 +1,5 @@ #![feature(let_chains)] #![feature(once_cell)] -#![feature(path_try_exists)] #![feature(rustc_attrs)] #![feature(type_alias_impl_trait)] diff --git a/library/std/src/fs.rs b/library/std/src/fs.rs index 55bd2c59406df..f46997b807ab2 100644 --- a/library/std/src/fs.rs +++ b/library/std/src/fs.rs @@ -2317,10 +2317,14 @@ impl AsInnerMut for DirBuilder { /// unrelated to the path not existing. (E.g. it will return `Err(_)` in case of permission /// denied on some of the parent directories.) /// +/// Note that while this avoids some pitfalls of the `exists()` method, it still can not +/// prevent time-of-check to time-of-use (TOCTOU) bugs. You should only use it in scenarios +/// where those bugs are not an issue. +/// /// # Examples /// /// ```no_run -/// #![feature(path_try_exists)] +/// #![feature(fs_try_exists)] /// use std::fs; /// /// assert!(!fs::try_exists("does_not_exist.txt").expect("Can't check existence of file does_not_exist.txt")); @@ -2330,7 +2334,7 @@ impl AsInnerMut for DirBuilder { /// [`Path::exists`]: crate::path::Path::exists // FIXME: stabilization should modify documentation of `exists()` to recommend this method // instead. -#[unstable(feature = "path_try_exists", issue = "83186")] +#[unstable(feature = "fs_try_exists", issue = "83186")] #[inline] pub fn try_exists>(path: P) -> io::Result { fs_imp::try_exists(path.as_ref()) diff --git a/library/std/src/path.rs b/library/std/src/path.rs index 36d6469c02d31..c56d5aa9b2e8d 100644 --- a/library/std/src/path.rs +++ b/library/std/src/path.rs @@ -2705,6 +2705,9 @@ impl Path { /// Returns `true` if the path points at an existing entity. /// + /// Warning: this method may be error-prone, consider using [`try_exists()`] instead! + /// It also has a risk of introducing time-of-check to time-of-use (TOCTOU) bugs. + /// /// This function will traverse symbolic links to query information about the /// destination file. /// @@ -2721,7 +2724,9 @@ impl Path { /// # See Also /// /// This is a convenience function that coerces errors to false. If you want to - /// check errors, call [`fs::metadata`]. + /// check errors, call [`Path::try_exists`]. + /// + /// [`try_exists()`]: Self::try_exists #[stable(feature = "path_ext", since = "1.5.0")] #[must_use] #[inline] @@ -2738,20 +2743,20 @@ impl Path { /// unrelated to the path not existing. (E.g. it will return `Err(_)` in case of permission /// denied on some of the parent directories.) /// + /// Note that while this avoids some pitfalls of the `exists()` method, it still can not + /// prevent time-of-check to time-of-use (TOCTOU) bugs. You should only use it in scenarios + /// where those bugs are not an issue. + /// /// # Examples /// /// ```no_run - /// #![feature(path_try_exists)] - /// /// use std::path::Path; /// assert!(!Path::new("does_not_exist.txt").try_exists().expect("Can't check existence of file does_not_exist.txt")); /// assert!(Path::new("/root/secret_file.txt").try_exists().is_err()); /// ``` /// /// [`exists()`]: Self::exists - // FIXME: stabilization should modify documentation of `exists()` to recommend this method - // instead. - #[unstable(feature = "path_try_exists", issue = "83186")] + #[stable(feature = "path_try_exists", since = "1.63.0")] #[inline] pub fn try_exists(&self) -> io::Result { fs::try_exists(self) From 072b7db56161ee4c7a4411d8398c90512c153e16 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 18 Jun 2022 10:19:24 +0000 Subject: [PATCH 2/5] Make debug_triple depend on target json file content rather than file path This ensures that changes to target json files will force a recompilation. And more importantly that moving the files doesn't force a recompilation. --- compiler/rustc_target/src/spec/mod.rs | 50 +++++++++++++++------------ 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/compiler/rustc_target/src/spec/mod.rs b/compiler/rustc_target/src/spec/mod.rs index 422af66787579..6670f3845c8bd 100644 --- a/compiler/rustc_target/src/spec/mod.rs +++ b/compiler/rustc_target/src/spec/mod.rs @@ -2183,7 +2183,7 @@ impl Target { TargetTriple::TargetTriple(ref target_triple) => { load_builtin(target_triple).expect("built-in target") } - TargetTriple::TargetPath(..) => { + TargetTriple::TargetJson { .. } => { panic!("built-in targets doens't support target-paths") } } @@ -2248,11 +2248,9 @@ impl Target { Err(format!("Could not find specification for target {:?}", target_triple)) } - TargetTriple::TargetPath(ref target_path) => { - if target_path.is_file() { - return load_file(&target_path); - } - Err(format!("Target path {:?} is not a valid file", target_path)) + TargetTriple::TargetJson { triple: _, ref contents } => { + let obj = serde_json::from_str(contents).map_err(|e| e.to_string())?; + Target::from_json(obj) } } } @@ -2424,7 +2422,7 @@ impl ToJson for Target { #[derive(PartialEq, Clone, Debug, Hash, Encodable, Decodable)] pub enum TargetTriple { TargetTriple(String), - TargetPath(PathBuf), + TargetJson { triple: String, contents: String }, } impl TargetTriple { @@ -2436,7 +2434,19 @@ impl TargetTriple { /// Creates a target triple from the passed target path. pub fn from_path(path: &Path) -> Result { let canonicalized_path = path.canonicalize()?; - Ok(TargetTriple::TargetPath(canonicalized_path)) + let contents = std::fs::read_to_string(&canonicalized_path).map_err(|err| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("Target path {:?} is not a valid file: {}", canonicalized_path, err), + ) + })?; + let triple = canonicalized_path + .file_stem() + .expect("target path must not be empty") + .to_str() + .expect("target path must be valid unicode") + .to_owned(); + Ok(TargetTriple::TargetJson { triple, contents }) } /// Returns a string triple for this target. @@ -2444,12 +2454,8 @@ impl TargetTriple { /// If this target is a path, the file name (without extension) is returned. pub fn triple(&self) -> &str { match *self { - TargetTriple::TargetTriple(ref triple) => triple, - TargetTriple::TargetPath(ref path) => path - .file_stem() - .expect("target path must not be empty") - .to_str() - .expect("target path must be valid unicode"), + TargetTriple::TargetTriple(ref triple) + | TargetTriple::TargetJson { ref triple, contents: _ } => triple, } } @@ -2461,14 +2467,14 @@ impl TargetTriple { use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; - let triple = self.triple(); - if let TargetTriple::TargetPath(ref path) = *self { - let mut hasher = DefaultHasher::new(); - path.hash(&mut hasher); - let hash = hasher.finish(); - format!("{}-{}", triple, hash) - } else { - triple.into() + match self { + TargetTriple::TargetTriple(triple) => triple.to_owned(), + TargetTriple::TargetJson { triple, contents: content } => { + let mut hasher = DefaultHasher::new(); + content.hash(&mut hasher); + let hash = hasher.finish(); + format!("{}-{}", triple, hash) + } } } } From f0144aea7402cd483157b1d4c4474b7c66f3a559 Mon Sep 17 00:00:00 2001 From: KaDiWa4 Date: Sun, 19 Jun 2022 17:07:38 +0200 Subject: [PATCH 3/5] typos in `IntoFuture` docs --- library/core/src/future/into_future.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/future/into_future.rs b/library/core/src/future/into_future.rs index d22094130ad9a..ad9e80e117f1e 100644 --- a/library/core/src/future/into_future.rs +++ b/library/core/src/future/into_future.rs @@ -2,7 +2,7 @@ use crate::future::Future; /// Conversion into a `Future`. /// -/// By implementing `Intofuture` for a type, you define how it will be +/// By implementing `IntoFuture` for a type, you define how it will be /// converted to a future. /// /// # `.await` desugaring @@ -29,7 +29,7 @@ use crate::future::Future; /// When implementing futures manually there will often be a choice between /// implementing `Future` or `IntoFuture` for a type. Implementing `Future` is a /// good choice in most cases. But implementing `IntoFuture` is most useful when -/// implementing "async builder" types, which allows the type to be modified +/// implementing "async builder" types, which allow their values to be modified /// multiple times before being `.await`ed. /// /// ```rust From b4b536d34d8d2371a4be95a2b294bbf0088807b7 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sat, 18 Jun 2022 10:48:46 +0000 Subject: [PATCH 4/5] Preserve the path of the target spec json file for usage by rustdoc --- compiler/rustc_target/src/spec/mod.rs | 84 ++++++++++++++++++++++++--- src/librustdoc/doctest.rs | 4 +- 2 files changed, 78 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_target/src/spec/mod.rs b/compiler/rustc_target/src/spec/mod.rs index 6670f3845c8bd..fd0c3f36e7299 100644 --- a/compiler/rustc_target/src/spec/mod.rs +++ b/compiler/rustc_target/src/spec/mod.rs @@ -39,11 +39,13 @@ use crate::json::{Json, ToJson}; use crate::spec::abi::{lookup as lookup_abi, Abi}; use crate::spec::crt_objects::{CrtObjects, CrtObjectsFallback}; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; +use rustc_serialize::{Decodable, Decoder, Encodable, Encoder}; use rustc_span::symbol::{sym, Symbol}; use serde_json::Value; use std::borrow::Cow; use std::collections::BTreeMap; use std::convert::TryFrom; +use std::hash::{Hash, Hasher}; use std::iter::FromIterator; use std::ops::{Deref, DerefMut}; use std::path::{Path, PathBuf}; @@ -2248,7 +2250,7 @@ impl Target { Err(format!("Could not find specification for target {:?}", target_triple)) } - TargetTriple::TargetJson { triple: _, ref contents } => { + TargetTriple::TargetJson { ref contents, .. } => { let obj = serde_json::from_str(contents).map_err(|e| e.to_string())?; Target::from_json(obj) } @@ -2419,10 +2421,77 @@ impl ToJson for Target { } /// Either a target triple string or a path to a JSON file. -#[derive(PartialEq, Clone, Debug, Hash, Encodable, Decodable)] +#[derive(Clone, Debug)] pub enum TargetTriple { TargetTriple(String), - TargetJson { triple: String, contents: String }, + TargetJson { + /// Warning: This field may only be used by rustdoc. Using it anywhere else will lead to + /// inconsistencies as it is discarded during serialization. + path_for_rustdoc: PathBuf, + triple: String, + contents: String, + }, +} + +// Use a manual implementation to ignore the path field +impl PartialEq for TargetTriple { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (Self::TargetTriple(l0), Self::TargetTriple(r0)) => l0 == r0, + ( + Self::TargetJson { path_for_rustdoc: _, triple: l_triple, contents: l_contents }, + Self::TargetJson { path_for_rustdoc: _, triple: r_triple, contents: r_contents }, + ) => l_triple == r_triple && l_contents == r_contents, + _ => false, + } + } +} + +// Use a manual implementation to ignore the path field +impl Hash for TargetTriple { + fn hash(&self, state: &mut H) -> () { + match self { + TargetTriple::TargetTriple(triple) => { + 0u8.hash(state); + triple.hash(state) + } + TargetTriple::TargetJson { path_for_rustdoc: _, triple, contents } => { + 1u8.hash(state); + triple.hash(state); + contents.hash(state) + } + } + } +} + +// Use a manual implementation to prevent encoding the target json file path in the crate metadata +impl Encodable for TargetTriple { + fn encode(&self, s: &mut S) { + match self { + TargetTriple::TargetTriple(triple) => s.emit_enum_variant(0, |s| s.emit_str(triple)), + TargetTriple::TargetJson { path_for_rustdoc: _, triple, contents } => s + .emit_enum_variant(1, |s| { + s.emit_str(triple); + s.emit_str(contents) + }), + } + } +} + +impl Decodable for TargetTriple { + fn decode(d: &mut D) -> Self { + match d.read_usize() { + 0 => TargetTriple::TargetTriple(d.read_str().to_owned()), + 1 => TargetTriple::TargetJson { + path_for_rustdoc: PathBuf::new(), + triple: d.read_str().to_owned(), + contents: d.read_str().to_owned(), + }, + _ => { + panic!("invalid enum variant tag while decoding `TargetTriple`, expected 0..2"); + } + } + } } impl TargetTriple { @@ -2437,7 +2506,7 @@ impl TargetTriple { let contents = std::fs::read_to_string(&canonicalized_path).map_err(|err| { io::Error::new( io::ErrorKind::InvalidInput, - format!("Target path {:?} is not a valid file: {}", canonicalized_path, err), + format!("target path {:?} is not a valid file: {}", canonicalized_path, err), ) })?; let triple = canonicalized_path @@ -2446,7 +2515,7 @@ impl TargetTriple { .to_str() .expect("target path must be valid unicode") .to_owned(); - Ok(TargetTriple::TargetJson { triple, contents }) + Ok(TargetTriple::TargetJson { path_for_rustdoc: canonicalized_path, triple, contents }) } /// Returns a string triple for this target. @@ -2455,7 +2524,7 @@ impl TargetTriple { pub fn triple(&self) -> &str { match *self { TargetTriple::TargetTriple(ref triple) - | TargetTriple::TargetJson { ref triple, contents: _ } => triple, + | TargetTriple::TargetJson { ref triple, .. } => triple, } } @@ -2465,11 +2534,10 @@ impl TargetTriple { /// by `triple()`. pub fn debug_triple(&self) -> String { use std::collections::hash_map::DefaultHasher; - use std::hash::{Hash, Hasher}; match self { TargetTriple::TargetTriple(triple) => triple.to_owned(), - TargetTriple::TargetJson { triple, contents: content } => { + TargetTriple::TargetJson { path_for_rustdoc: _, triple, contents: content } => { let mut hasher = DefaultHasher::new(); content.hash(&mut hasher); let hash = hasher.finish(); diff --git a/src/librustdoc/doctest.rs b/src/librustdoc/doctest.rs index 86c58cd79dce0..ab72f4a3f502c 100644 --- a/src/librustdoc/doctest.rs +++ b/src/librustdoc/doctest.rs @@ -365,8 +365,8 @@ fn run_test( } compiler.arg("--target").arg(match target { TargetTriple::TargetTriple(s) => s, - TargetTriple::TargetPath(path) => { - path.to_str().expect("target path must be valid unicode").to_string() + TargetTriple::TargetJson { path_for_rustdoc, .. } => { + path_for_rustdoc.to_str().expect("target path must be valid unicode").to_string() } }); if let ErrorOutputType::HumanReadable(kind) = rustdoc_options.error_format { From 9ac6277bad856e0a1373e1fd26ee3e70b10074a8 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Thu, 31 Mar 2022 13:01:26 -0500 Subject: [PATCH 5/5] Add `core::mem::copy` to complement `core::mem::drop`. This is useful for combinators. I didn't add `clone` since you can already use `Clone::clone` in its place; copy has no such corresponding function. --- library/core/src/mem/mod.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/library/core/src/mem/mod.rs b/library/core/src/mem/mod.rs index 8a99bed6a96ab..03e94c8cd6ed1 100644 --- a/library/core/src/mem/mod.rs +++ b/library/core/src/mem/mod.rs @@ -964,6 +964,28 @@ pub const fn replace(dest: &mut T, src: T) -> T { #[cfg_attr(not(test), rustc_diagnostic_item = "mem_drop")] pub fn drop(_x: T) {} +/// Bitwise-copies a value. +/// +/// This function is not magic; it is literally defined as +/// ``` +/// pub fn copy(x: &T) -> T { *x } +/// ``` +/// +/// It is useful when you want to pass a function pointer to a combinator, rather than defining a new closure. +/// +/// Example: +/// ``` +/// #![feature(mem_copy_fn)] +/// use core::mem::copy; +/// let result_from_ffi_function: Result<(), &i32> = Err(&1); +/// let result_copied: Result<(), i32> = result_from_ffi_function.map_err(copy); +/// ``` +#[inline] +#[unstable(feature = "mem_copy_fn", issue = "98262")] +pub fn copy(x: &T) -> T { + *x +} + /// Interprets `src` as having type `&U`, and then reads `src` without moving /// the contained value. ///