Repository: denoland/deno
Pull request: fix(ext/io): Fix NUL termination error in windows named pipes

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.

Changed files:
filename: cli/tools/test/mod.rs
patch: @@ -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 });

filename: ext/io/winpipe.rs
patch: @@ -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,20 +30,14 @@ 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()
 }
 
 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)]