From 35aabfc64137b7ce21e99530f52461c6c642c9f2 Mon Sep 17 00:00:00 2001 From: Vinzent Steinberg Date: Wed, 17 Aug 2022 00:41:31 +0200 Subject: [PATCH 1/3] Replace `fastrand` with `getrandom` and `base64` - Instead of setting up a predictable userspace RNG, we get unpredictable random bytes directly from the OS. This fixes #178. - To obtain a uniformly distributed alphanumeric string, we convert the the random bytes to base64 and throw away any letters we don't want (`+` and `/`). With a low probability, this may result in obtaining too few alphanumeric letters, in which case we request more randomness from the OS until we have enough. - Because we cannot control the seed anymore, a test manufacturing collisions by setting the same seed for several threads had to be removed. --- Cargo.toml | 3 ++- src/util.rs | 35 ++++++++++++++++++++++++---- tests/namedtempfile.rs | 52 ------------------------------------------ 3 files changed, 33 insertions(+), 57 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a738615a3..a7b2bffe2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,8 +17,9 @@ repository = "https://github.com/Stebalien/tempfile" description = "A library for managing temporary files and directories." [dependencies] +base64 = "0.13.0" cfg-if = "1" -fastrand = "1.6.0" +getrandom = "0.2.7" remove_dir_all = "0.5" [target.'cfg(any(unix, target_os = "wasi"))'.dependencies] diff --git a/src/util.rs b/src/util.rs index 98c3ee804..188a66936 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,16 +1,43 @@ use std::ffi::{OsStr, OsString}; use std::path::{Path, PathBuf}; -use std::{io, iter::repeat_with}; +use std::io; use crate::error::IoResultExt; +fn calculate_rand_buf_len(alphanumeric_len: usize) -> usize { + let expected_non_alphanumeric_chars = alphanumeric_len / 32; + (alphanumeric_len + expected_non_alphanumeric_chars) * 3 / 4 + 3 +} + +fn calculate_base64_len(binary_len: usize) -> usize { + binary_len * 4 / 3 + 4 +} + +fn fill_with_random_base64(rand_buf: &mut [u8], char_buf: &mut Vec) { + getrandom::getrandom(rand_buf).expect("calling getrandom failed"); + char_buf.resize(calculate_base64_len(rand_buf.len()), 0); + base64::encode_config_slice(rand_buf, base64::STANDARD_NO_PAD, char_buf); +} + fn tmpname(prefix: &OsStr, suffix: &OsStr, rand_len: usize) -> OsString { let mut buf = OsString::with_capacity(prefix.len() + suffix.len() + rand_len); buf.push(prefix); - let mut char_buf = [0u8; 4]; - for c in repeat_with(fastrand::alphanumeric).take(rand_len) { - buf.push(c.encode_utf8(&mut char_buf)); + + let mut rand_buf = vec![0; calculate_rand_buf_len(rand_len)]; + let mut char_buf = vec![0; calculate_base64_len(rand_buf.len())]; + let mut remaining_chars = rand_len; + loop { + fill_with_random_base64(&mut rand_buf, &mut char_buf); + char_buf.retain(|&c| (c != b'+') & (c != b'/') & (c != 0)); + if char_buf.len() >= remaining_chars { + buf.push(std::str::from_utf8(&char_buf[..remaining_chars]).unwrap()); + break; + } else { + buf.push(std::str::from_utf8(&char_buf).unwrap()); + remaining_chars -= char_buf.len(); + } } + buf.push(suffix); buf } diff --git a/tests/namedtempfile.rs b/tests/namedtempfile.rs index 22ec6a4f0..5f0da955b 100644 --- a/tests/namedtempfile.rs +++ b/tests/namedtempfile.rs @@ -387,55 +387,3 @@ fn test_make_uds() { assert!(temp_sock.path().exists()); } - -#[cfg(unix)] -#[test] -fn test_make_uds_conflict() { - use std::os::unix::net::UnixListener; - use std::sync::atomic::{AtomicUsize, Ordering}; - use std::sync::Arc; - - // Check that retries happen correctly by racing N different threads. - - const NTHREADS: usize = 20; - - // The number of times our callback was called. - let tries = Arc::new(AtomicUsize::new(0)); - - let mut threads = Vec::with_capacity(NTHREADS); - - for _ in 0..NTHREADS { - let tries = tries.clone(); - threads.push(std::thread::spawn(move || { - // Ensure that every thread uses the same seed so we are guaranteed - // to retry. Note that fastrand seeds are thread-local. - fastrand::seed(42); - - Builder::new() - .prefix("tmp") - .suffix(".sock") - .rand_bytes(12) - .make(|path| { - tries.fetch_add(1, Ordering::Relaxed); - UnixListener::bind(path) - }) - })); - } - - // Join all threads, but don't drop the temp file yet. Otherwise, we won't - // get a deterministic number of `tries`. - let sockets: Vec<_> = threads - .into_iter() - .map(|thread| thread.join().unwrap().unwrap()) - .collect(); - - // Number of tries is exactly equal to (n*(n+1))/2. - assert_eq!( - tries.load(Ordering::Relaxed), - (NTHREADS * (NTHREADS + 1)) / 2 - ); - - for socket in sockets { - assert!(socket.path().exists()); - } -} From b3981dd668bc5fdfac6d565c17205bc3f0ff2320 Mon Sep 17 00:00:00 2001 From: Vinzent Steinberg Date: Wed, 17 Aug 2022 19:47:35 +0200 Subject: [PATCH 2/3] Add non-deterministic retrial test --- tests/namedtempfile.rs | 49 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/namedtempfile.rs b/tests/namedtempfile.rs index 5f0da955b..6a35c2917 100644 --- a/tests/namedtempfile.rs +++ b/tests/namedtempfile.rs @@ -5,6 +5,7 @@ use std::ffi::{OsStr, OsString}; use std::fs::File; use std::io::{Read, Seek, SeekFrom, Write}; use std::path::{Path, PathBuf}; +use std::sync::Barrier; use tempfile::{tempdir, Builder, NamedTempFile, TempPath}; fn exists>(path: P) -> bool { @@ -387,3 +388,51 @@ fn test_make_uds() { assert!(temp_sock.path().exists()); } + +#[cfg(unix)] +#[test] +fn test_make_uds_conflict() { + use std::os::unix::net::UnixListener; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + // Check that retries happen correctly by racing N different threads. + + const NTHREADS: usize = 20; + + // The number of times our callback was called. + let tries = Arc::new(AtomicUsize::new(0)); + + let mut threads = Vec::with_capacity(NTHREADS); + let barrier = Arc::new(Barrier::new(NTHREADS)); + + for _ in 0..NTHREADS { + let c = Arc::clone(&barrier); + let tries = tries.clone(); + threads.push(std::thread::spawn(move || { + let builder = Builder::new() + .prefix("tmp") + .suffix(".sock") + .rand_bytes(1) + .make(|path| { + tries.fetch_add(1, Ordering::Relaxed); + UnixListener::bind(path) + }); + c.wait(); + builder + })); + } + + // Join all threads, but don't drop the temp file yet. + let sockets: Vec<_> = threads + .into_iter() + .map(|thread| thread.join().unwrap().unwrap()) + .collect(); + + assert!(tries.load(Ordering::Relaxed) > 15); + + for socket in sockets { + assert!(socket.path().exists()); + } +} + From 6356a34ff1fc6980f579e189cb0975235c79d37d Mon Sep 17 00:00:00 2001 From: Vinzent Steinberg Date: Wed, 17 Aug 2022 20:20:57 +0200 Subject: [PATCH 3/3] Propagate getrandom errors --- Cargo.toml | 2 +- src/util.rs | 16 ++++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a7b2bffe2..45405e5a0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,7 @@ description = "A library for managing temporary files and directories." [dependencies] base64 = "0.13.0" cfg-if = "1" -getrandom = "0.2.7" +getrandom = { version = "0.2.7", features = ["std"] } remove_dir_all = "0.5" [target.'cfg(any(unix, target_os = "wasi"))'.dependencies] diff --git a/src/util.rs b/src/util.rs index 188a66936..3a1fa3c14 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,6 +1,7 @@ use std::ffi::{OsStr, OsString}; use std::path::{Path, PathBuf}; use std::io; +use std::io::ErrorKind; use crate::error::IoResultExt; @@ -13,13 +14,14 @@ fn calculate_base64_len(binary_len: usize) -> usize { binary_len * 4 / 3 + 4 } -fn fill_with_random_base64(rand_buf: &mut [u8], char_buf: &mut Vec) { - getrandom::getrandom(rand_buf).expect("calling getrandom failed"); +fn fill_with_random_base64(rand_buf: &mut [u8], char_buf: &mut Vec) -> Result<(), getrandom::Error> { + getrandom::getrandom(rand_buf)?; char_buf.resize(calculate_base64_len(rand_buf.len()), 0); base64::encode_config_slice(rand_buf, base64::STANDARD_NO_PAD, char_buf); + Ok(()) } -fn tmpname(prefix: &OsStr, suffix: &OsStr, rand_len: usize) -> OsString { +fn tmpname(prefix: &OsStr, suffix: &OsStr, rand_len: usize) -> Result { let mut buf = OsString::with_capacity(prefix.len() + suffix.len() + rand_len); buf.push(prefix); @@ -27,7 +29,7 @@ fn tmpname(prefix: &OsStr, suffix: &OsStr, rand_len: usize) -> OsString { let mut char_buf = vec![0; calculate_base64_len(rand_buf.len())]; let mut remaining_chars = rand_len; loop { - fill_with_random_base64(&mut rand_buf, &mut char_buf); + fill_with_random_base64(&mut rand_buf, &mut char_buf)?; char_buf.retain(|&c| (c != b'+') & (c != b'/') & (c != 0)); if char_buf.len() >= remaining_chars { buf.push(std::str::from_utf8(&char_buf[..remaining_chars]).unwrap()); @@ -39,7 +41,7 @@ fn tmpname(prefix: &OsStr, suffix: &OsStr, rand_len: usize) -> OsString { } buf.push(suffix); - buf + Ok(buf) } pub fn create_helper( @@ -59,7 +61,9 @@ where }; for _ in 0..num_retries { - let path = base.join(tmpname(prefix, suffix, random_len)); + let name = tmpname(prefix, suffix, random_len) + .map_err(|e| io::Error::new(ErrorKind::Other, e))?; + let path = base.join(name); return match f(path) { Err(ref e) if e.kind() == io::ErrorKind::AlreadyExists => continue, // AddrInUse can happen if we're creating a UNIX domain socket and