fix(ext/net): don't remove sockets on unix listen (denoland#16394)

When listening on a UNIX socket path, Deno currently tries to unlink this path prior to actually listening. The implementation of this behaviour is VERY racy, involves 2 additional syscalls, and does not match the behaviour of any other runtime (Node.js, Go, Rust, etc). This commit removes this behaviour. If a user wants to listen on an existing socket, they must now unlink the file themselves prior to listening. This change in behaviour only impacts --unstable APIs, so it is not a breaking change.
marcosc90 · Oct 23, 2022 · 38213f1 · 38213f1
1 parent 0e1167d
commit 38213f1
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 30 deletions.
diff --git a/cli/tests/unit/http_test.ts b/cli/tests/unit/http_test.ts
@@ -16,6 +16,7 @@ import {
   delay,
   fail,
 } from "./test_util.ts";
+import { join } from "../../../test_util/std/path/mod.ts";
 
 async function writeRequestAndReadResponse(conn: Deno.Conn): Promise<string> {
   const encoder = new TextEncoder();
@@ -1290,14 +1291,19 @@ Deno.test(
   },
 );
 
+function tmpUnixSocketPath(): string {
+  const folder = Deno.makeTempDirSync();
+  return join(folder, "socket");
+}
+
 // https://github.com/denoland/deno/pull/13628
 Deno.test(
   {
     ignore: Deno.build.os === "windows",
     permissions: { read: true, write: true },
   },
   async function httpServerOnUnixSocket() {
-    const filePath = Deno.makeTempFileSync();
+    const filePath = tmpUnixSocketPath();
 
     let httpConn: Deno.HttpConn;
     const promise = (async () => {
@@ -2239,7 +2245,7 @@ Deno.test("upgradeHttp unix", {
   permissions: { read: true, write: true },
   ignore: Deno.build.os === "windows",
 }, async () => {
-  const filePath = Deno.makeTempFileSync();
+  const filePath = tmpUnixSocketPath();
   const promise = deferred();
 
   async function client() {
@@ -2435,7 +2441,7 @@ Deno.test(
     permissions: { read: true, write: true },
   },
   async function httpServerWithoutExclusiveAccessToUnixSocket() {
-    const filePath = await Deno.makeTempFile();
+    const filePath = tmpUnixSocketPath();
     const listener = Deno.listen({ path: filePath, transport: "unix" });
 
     const [clientConn, serverConn] = await Promise.all([

diff --git a/cli/tests/unit/net_test.ts b/cli/tests/unit/net_test.ts
@@ -9,6 +9,7 @@ import {
   delay,
   execCode,
 } from "./test_util.ts";
+import { join } from "../../../test_util/std/path/mod.ts";
 
 let isCI: boolean;
 try {
@@ -43,13 +44,18 @@ Deno.test(
   },
 );
 
+function tmpUnixSocketPath(): string {
+  const folder = Deno.makeTempDirSync();
+  return join(folder, "socket");
+}
+
 Deno.test(
   {
     ignore: Deno.build.os === "windows",
     permissions: { read: true, write: true },
   },
   function netUnixListenClose() {
-    const filePath = Deno.makeTempFileSync();
+    const filePath = tmpUnixSocketPath();
     const socket = Deno.listen({
       path: filePath,
       transport: "unix",
@@ -66,7 +72,7 @@ Deno.test(
     permissions: { read: true, write: true },
   },
   function netUnixPacketListenClose() {
-    const filePath = Deno.makeTempFileSync();
+    const filePath = tmpUnixSocketPath();
     const socket = Deno.listenDatagram({
       path: filePath,
       transport: "unixpacket",
@@ -84,7 +90,7 @@ Deno.test(
   },
   function netUnixListenWritePermission() {
     assertThrows(() => {
-      const filePath = Deno.makeTempFileSync();
+      const filePath = tmpUnixSocketPath();
       const socket = Deno.listen({
         path: filePath,
         transport: "unix",
@@ -103,7 +109,7 @@ Deno.test(
   },
   function netUnixPacketListenWritePermission() {
     assertThrows(() => {
-      const filePath = Deno.makeTempFileSync();
+      const filePath = tmpUnixSocketPath();
       const socket = Deno.listenDatagram({
         path: filePath,
         transport: "unixpacket",
@@ -139,7 +145,7 @@ Deno.test(
     permissions: { read: true, write: true },
   },
   async function netUnixCloseWhileAccept() {
-    const filePath = await Deno.makeTempFile();
+    const filePath = tmpUnixSocketPath();
     const listener = Deno.listen({
       path: filePath,
       transport: "unix",
@@ -183,7 +189,7 @@ Deno.test(
     permissions: { read: true, write: true },
   },
   async function netUnixConcurrentAccept() {
-    const filePath = await Deno.makeTempFile();
+    const filePath = tmpUnixSocketPath();
     const listener = Deno.listen({ transport: "unix", path: filePath });
     let acceptErrCount = 0;
     const checkErr = (e: Error) => {
@@ -317,7 +323,7 @@ Deno.test(
     permissions: { read: true, write: true },
   },
   async function netUnixDialListen() {
-    const filePath = await Deno.makeTempFile();
+    const filePath = tmpUnixSocketPath();
     const listener = Deno.listen({ path: filePath, transport: "unix" });
     listener.accept().then(
       async (conn) => {
@@ -463,28 +469,29 @@ Deno.test(
     permissions: { read: true, write: true },
   },
   async function netUnixPacketSendReceive() {
-    const filePath = await Deno.makeTempFile();
+    const aliceFilePath = tmpUnixSocketPath();
     const alice = Deno.listenDatagram({
-      path: filePath,
+      path: aliceFilePath,
       transport: "unixpacket",
     });
     assert(alice.addr.transport === "unixpacket");
-    assertEquals(alice.addr.path, filePath);
+    assertEquals(alice.addr.path, aliceFilePath);
 
+    const bobFilePath = tmpUnixSocketPath();
     const bob = Deno.listenDatagram({
-      path: filePath,
+      path: bobFilePath,
       transport: "unixpacket",
     });
     assert(bob.addr.transport === "unixpacket");
-    assertEquals(bob.addr.path, filePath);
+    assertEquals(bob.addr.path, bobFilePath);
 
     const sent = new Uint8Array([1, 2, 3]);
     const byteLength = await alice.send(sent, bob.addr);
     assertEquals(byteLength, 3);
 
     const [recvd, remote] = await bob.receive();
     assert(remote.transport === "unixpacket");
-    assertEquals(remote.path, filePath);
+    assertEquals(remote.path, aliceFilePath);
     assertEquals(recvd.length, 3);
     assertEquals(1, recvd[0]);
     assertEquals(2, recvd[1]);
@@ -494,11 +501,11 @@ Deno.test(
   },
 );
 
-// TODO(piscisaureus): Enable after Tokio v0.3/v1.0 upgrade.
+// TODO(lucacasonato): support concurrent reads and writes on unixpacket sockets
 Deno.test(
   { ignore: true, permissions: { read: true, write: true } },
   async function netUnixPacketConcurrentSendReceive() {
-    const filePath = await Deno.makeTempFile();
+    const filePath = tmpUnixSocketPath();
     const socket = Deno.listenDatagram({
       path: filePath,
       transport: "unixpacket",
@@ -584,7 +591,7 @@ Deno.test(
     permissions: { read: true, write: true },
   },
   async function netUnixListenCloseWhileIterating() {
-    const filePath = Deno.makeTempFileSync();
+    const filePath = tmpUnixSocketPath();
     const socket = Deno.listen({ path: filePath, transport: "unix" });
     const nextWhileClosing = socket[Symbol.asyncIterator]().next();
     socket.close();
@@ -601,7 +608,7 @@ Deno.test(
     permissions: { read: true, write: true },
   },
   async function netUnixPacketListenCloseWhileIterating() {
-    const filePath = Deno.makeTempFileSync();
+    const filePath = tmpUnixSocketPath();
     const socket = Deno.listenDatagram({
       path: filePath,
       transport: "unixpacket",
@@ -884,3 +891,18 @@ Deno.test(
     clearTimeout(timer);
   },
 );
+
+Deno.test({
+  ignore: Deno.build.os === "windows",
+  permissions: { read: true, write: true },
+}, function netUnixListenAddrAlreadyInUse() {
+  const filePath = tmpUnixSocketPath();
+  const listener = Deno.listen({ path: filePath, transport: "unix" });
+  assertThrows(
+    () => {
+      Deno.listen({ path: filePath, transport: "unix" });
+    },
+    Deno.errors.AddrInUse,
+  );
+  listener.close();
+});
diff --git a/cli/tests/unit/remove_test.ts b/cli/tests/unit/remove_test.ts
@@ -251,9 +251,7 @@ Deno.test(
       Deno.statSync(path); // check if unix socket exists
 
       await Deno[method](path);
-      assertThrows(() => {
-        Deno.statSync(path);
-      }, Deno.errors.NotFound);
+      assertThrows(() => Deno.statSync(path), Deno.errors.NotFound);
     }
   },
 );

diff --git a/ext/net/ops_unix.rs b/ext/net/ops_unix.rs
@@ -20,7 +20,6 @@ use serde::Deserialize;
 use serde::Serialize;
 use std::borrow::Cow;
 use std::cell::RefCell;
-use std::fs::remove_file;
 use std::path::Path;
 use std::rc::Rc;
 use tokio::net::UnixDatagram;
@@ -143,9 +142,6 @@ pub fn listen_unix(
   state: &mut OpState,
   addr: &Path,
 ) -> Result<(u32, tokio::net::unix::SocketAddr), AnyError> {
-  if addr.exists() {
-    remove_file(&addr)?;
-  }
   let listener = UnixListener::bind(&addr)?;
   let local_addr = listener.local_addr()?;
   let listener_resource = UnixListenerResource {
@@ -161,9 +157,6 @@ pub fn listen_unix_packet(
   state: &mut OpState,
   addr: &Path,
 ) -> Result<(u32, tokio::net::unix::SocketAddr), AnyError> {
-  if addr.exists() {
-    remove_file(&addr)?;
-  }
   let socket = UnixDatagram::bind(&addr)?;
   let local_addr = socket.local_addr()?;
   let datagram_resource = UnixDatagramResource {