From 7e4ee02e2e37db8adfaf4a05aba3819838904650 Mon Sep 17 00:00:00 2001 From: Matt Mastracci Date: Mon, 15 Apr 2024 14:10:09 -0600 Subject: [PATCH] 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. --- cli/tools/test/mod.rs | 12 +------ ext/io/winpipe.rs | 81 +++++++++++++++---------------------------- 2 files changed, 28 insertions(+), 65 deletions(-) diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 2a406e560b..f488271179 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::, 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 f6e66eeb30..f66dec6b6b 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(), - ) - }; + // 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; - } - - // 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)]