summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMatt Mastracci <matthew@mastracci.com>2024-04-15 14:10:09 -0600
committerGitHub <noreply@github.com>2024-04-15 14:10:09 -0600
commit7e4ee02e2e37db8adfaf4a05aba3819838904650 (patch)
tree560f7d4abcee351af4db58c5d943fd8aaac99318
parenta080acc1b46ce9915760ce5c818763c64be8dca1 (diff)
fix(ext/io): Fix NUL termination error in windows named pipes (#23379)
Due to a terminating NUL that was placed in a `r#` string, we were not actually NUL-terminating pipe names on Windows. While this has no security implications due to the random nature of the prefix, it would occasionally cause random failures when the trailing garbage would make the pipe name invalid.
-rw-r--r--cli/tools/test/mod.rs12
-rw-r--r--ext/io/winpipe.rs81
2 files changed, 28 insertions, 65 deletions
diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs
index 2a406e560..f48827117 100644
--- a/cli/tools/test/mod.rs
+++ b/cli/tools/test/mod.rs
@@ -1346,18 +1346,8 @@ async fn test_specifiers(
})
});
- // TODO(mmastrac): Temporarily limit concurrency in windows testing to avoid named pipe issue:
- // *** Unexpected server pipe failure '"\\\\.\\pipe\\deno_pipe_e30f45c9df61b1e4.1198.222\\0"': 3
- // This is likely because we're hitting some sort of invisible resource limit
- // This limit is both in cli/lsp/testing/execution.rs and cli/tools/test/mod.rs
- let concurrent = if cfg!(windows) {
- std::cmp::min(concurrent_jobs.get(), 4)
- } else {
- concurrent_jobs.get()
- };
-
let join_stream = stream::iter(join_handles)
- .buffer_unordered(concurrent)
+ .buffer_unordered(concurrent_jobs.get())
.collect::<Vec<Result<Result<(), AnyError>, tokio::task::JoinError>>>();
let handler = spawn(async move { report_tests(receiver, reporter).await.0 });
diff --git a/ext/io/winpipe.rs b/ext/io/winpipe.rs
index f6e66eeb3..f66dec6b6 100644
--- a/ext/io/winpipe.rs
+++ b/ext/io/winpipe.rs
@@ -5,7 +5,6 @@ use std::io;
use std::os::windows::io::RawHandle;
use std::sync::atomic::AtomicU32;
use std::sync::atomic::Ordering;
-use std::time::Duration;
use winapi::shared::minwindef::DWORD;
use winapi::um::errhandlingapi::GetLastError;
use winapi::um::fileapi::CreateFileA;
@@ -31,12 +30,6 @@ use winapi::um::winnt::GENERIC_WRITE;
/// well as offering a complex NTAPI solution if we decide to try to make these pipes truely
/// anonymous: https://stackoverflow.com/questions/60645/overlapped-i-o-on-anonymous-pipe
pub fn create_named_pipe() -> io::Result<(RawHandle, RawHandle)> {
- // Silently retry up to 10 times.
- for _ in 0..10 {
- if let Ok(res) = create_named_pipe_inner() {
- return Ok(res);
- }
- }
create_named_pipe_inner()
}
@@ -44,7 +37,7 @@ fn create_named_pipe_inner() -> io::Result<(RawHandle, RawHandle)> {
static NEXT_ID: AtomicU32 = AtomicU32::new(0);
// Create an extremely-likely-unique pipe name from randomness, identity and a serial counter.
let pipe_name = format!(
- r#"\\.\pipe\deno_pipe_{:x}.{:x}.{:x}\0"#,
+ concat!(r#"\\.\pipe\deno_pipe_{:x}.{:x}.{:x}"#, "\0"),
thread_rng().next_u64(),
std::process::id(),
NEXT_ID.fetch_add(1, Ordering::SeqCst),
@@ -89,56 +82,36 @@ fn create_named_pipe_inner() -> io::Result<(RawHandle, RawHandle)> {
return Err(io::Error::last_os_error());
}
- // The pipe might not be ready yet in rare cases, so we loop for a bit
- for i in 0..10 {
- // SAFETY: Create the pipe client with non-inheritable handle
- let client_handle = unsafe {
- CreateFileA(
- pipe_name.as_ptr() as *const i8,
- GENERIC_READ | GENERIC_WRITE,
- 0,
- &mut security_attributes,
- OPEN_EXISTING,
- FILE_FLAG_OVERLAPPED,
- std::ptr::null_mut(),
- )
- };
-
- // There is a very rare case where the pipe is not ready to open. If we get `ERROR_PATH_NOT_FOUND`,
- // we spin and try again in 1-10ms.
- if client_handle == INVALID_HANDLE_VALUE {
- // SAFETY: Getting last error for diagnostics
- let error = unsafe { GetLastError() };
- if error == winapi::shared::winerror::ERROR_FILE_NOT_FOUND
- || error == winapi::shared::winerror::ERROR_PATH_NOT_FOUND
- {
- // Exponential backoff, but don't sleep longer than 10ms
- eprintln!(
- "*** Unexpected client pipe not found failure '{pipe_name:?}': {:x}",
- error
- );
- std::thread::sleep(Duration::from_millis(10.min(2_u64.pow(i) + 1)));
- continue;
- }
+ // SAFETY: Create the pipe client with non-inheritable handle
+ let client_handle = unsafe {
+ CreateFileA(
+ pipe_name.as_ptr() as *const i8,
+ GENERIC_READ | GENERIC_WRITE,
+ 0,
+ &mut security_attributes,
+ OPEN_EXISTING,
+ FILE_FLAG_OVERLAPPED,
+ std::ptr::null_mut(),
+ )
+ };
- // This should not happen, so we would like to get some better diagnostics here.
- eprintln!(
- "*** Unexpected client pipe failure '{pipe_name:?}': {:x}",
- error
- );
- let err = io::Error::last_os_error();
- // SAFETY: Close the handles if we failed
- unsafe {
- CloseHandle(server_handle);
- }
- return Err(err);
+ if client_handle == INVALID_HANDLE_VALUE {
+ // SAFETY: Getting last error for diagnostics
+ let error = unsafe { GetLastError() };
+ // This should not happen, so we would like to get some better diagnostics here.
+ eprintln!(
+ "*** Unexpected client pipe failure '{pipe_name:?}': {:x}",
+ error
+ );
+ let err = io::Error::last_os_error();
+ // SAFETY: Close the handles if we failed
+ unsafe {
+ CloseHandle(server_handle);
}
-
- return Ok((server_handle, client_handle));
+ return Err(err);
}
- // We failed to open the pipe despite sleeping
- Err(std::io::ErrorKind::NotFound.into())
+ Ok((server_handle, client_handle))
}
#[cfg(test)]