fix(ext/net): return an error from startTls and serveHttp if the original connection is captured elsewhere (#16242)

This commit removes the calls to `expect()` on `std::rc::Rc`, which caused
Deno to panic under certain situations. We now return an error if `Rc`
is referenced by other variables.

Fixes #9360
Fixes #13345
Fixes #13926
Fixes #16241

Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>

cli/dts/lib.deno.ns.d.ts

@@ -4017,6 +4017,14 @@ declare namespace Deno {
    * }
    * ```
    *
+   * Note that this function *consumes* the given connection passed to it, thus the
+   * original connection will be unusable after calling this. Additionally, you
+   * need to ensure that the connection is not being used elsewhere when calling
+   * this function in order for the connection to be consumed properly.
+   * For instance, if there is a `Promise` that is waiting for read operation on
+   * the connection to complete, it is considered that the connection is being
+   * used elsewhere. In such a case, this function will fail.
+   *
    * @category HTTP Server
    */
   export function serveHttp(conn: Conn): HttpConn;

cli/tests/unit/http_test.ts

@@ -2373,6 +2373,86 @@ Deno.test(
   },
 );
 
+Deno.test(
+  {
+    permissions: { net: true },
+  },
+  async function httpServerWithoutExclusiveAccessToTcp() {
+    const port = 4506;
+    const listener = Deno.listen({ port });
+
+    const [clientConn, serverConn] = await Promise.all([
+      Deno.connect({ port }),
+      listener.accept(),
+    ]);
+
+    const buf = new Uint8Array(128);
+    const readPromise = serverConn.read(buf);
+    assertThrows(() => Deno.serveHttp(serverConn), Deno.errors.BadResource);
+
+    clientConn.close();
+    listener.close();
+    await readPromise;
+  },
+);
+
+Deno.test(
+  {
+    permissions: { net: true, read: true },
+  },
+  async function httpServerWithoutExclusiveAccessToTls() {
+    const hostname = "localhost";
+    const port = 4507;
+    const listener = Deno.listenTls({
+      hostname,
+      port,
+      certFile: "cli/tests/testdata/tls/localhost.crt",
+      keyFile: "cli/tests/testdata/tls/localhost.key",
+    });
+
+    const caCerts = [
+      await Deno.readTextFile("cli/tests/testdata/tls/RootCA.pem"),
+    ];
+    const [clientConn, serverConn] = await Promise.all([
+      Deno.connectTls({ hostname, port, caCerts }),
+      listener.accept(),
+    ]);
+    await Promise.all([clientConn.handshake(), serverConn.handshake()]);
+
+    const buf = new Uint8Array(128);
+    const readPromise = serverConn.read(buf);
+    assertThrows(() => Deno.serveHttp(serverConn), Deno.errors.BadResource);
+
+    clientConn.close();
+    listener.close();
+    await readPromise;
+  },
+);
+
+Deno.test(
+  {
+    ignore: Deno.build.os === "windows",
+    permissions: { read: true, write: true },
+  },
+  async function httpServerWithoutExclusiveAccessToUnixSocket() {
+    const filePath = await Deno.makeTempFile();
+    const listener = Deno.listen({ path: filePath, transport: "unix" });
+
+    const [clientConn, serverConn] = await Promise.all([
+      Deno.connect({ path: filePath, transport: "unix" }),
+      listener.accept(),
+    ]);
+
+    const buf = new Uint8Array(128);
+    const readPromise = serverConn.read(buf);
+    assertThrows(() => Deno.serveHttp(serverConn), Deno.errors.BadResource);
+
+    clientConn.close();
+    listener.close();
+    await readPromise;
+  },
+);
+
 function chunkedBodyReader(h: Headers, r: BufReader): Deno.Reader {
   // Based on https://tools.ietf.org/html/rfc2616#section-19.4.6
   const tp = new TextProtoReader(r);

cli/tests/unit/tls_test.ts

@@ -147,6 +147,33 @@ Deno.test(
   },
 );
 
+Deno.test(
+  { permissions: { net: true } },
+  async function startTlsWithoutExclusiveAccessToTcpConn() {
+    const hostname = "localhost";
+    const port = getPort();
+
+    const tcpListener = Deno.listen({ hostname, port });
+    const [serverConn, clientConn] = await Promise.all([
+      tcpListener.accept(),
+      Deno.connect({ hostname, port }),
+    ]);
+
+    const buf = new Uint8Array(128);
+    const readPromise = clientConn.read(buf);
+    // `clientConn` is being used by a pending promise (`readPromise`) so
+    // `Deno.startTls` cannot consume the connection.
+    await assertRejects(
+      () => Deno.startTls(clientConn, { hostname }),
+      Deno.errors.BadResource,
+    );
+
+    serverConn.close();
+    tcpListener.close();
+    await readPromise;
+  },
+);
+
 Deno.test(
   { permissions: { read: true, net: true } },
   async function dialAndListenTLS() {

core/resources.rs

@@ -291,6 +291,12 @@ impl ResourceTable {
   /// If a resource with the given `rid` exists but its type does not match `T`,
   /// it is not removed from the resource table. Note that the resource's
   /// `close()` method is *not* called.
+  ///
+  /// Also note that there might be a case where
+  /// the returned `Rc<T>` is referenced by other variables. That is, we cannot
+  /// assume that `Rc::strong_count(&returned_rc)` is always equal to 1 on success.
+  /// In particular, be really careful when you want to extract the inner value of
+  /// type `T` from `Rc<T>`.
   pub fn take<T: Resource>(&mut self, rid: ResourceId) -> Result<Rc<T>, Error> {
     let resource = self.get::<T>(rid)?;
     self.index.remove(&rid);
@@ -299,6 +305,12 @@ impl ResourceTable {
 
   /// Removes a resource from the resource table and returns it. Note that the
   /// resource's `close()` method is *not* called.
+  ///
+  /// Also note that there might be a
+  /// case where the returned `Rc<T>` is referenced by other variables. That is,
+  /// we cannot assume that `Rc::strong_count(&returned_rc)` is always equal to 1
+  /// on success. In particular, be really careful when you want to extract the
+  /// inner value of type `T` from `Rc<T>`.
   pub fn take_any(
     &mut self,
     rid: ResourceId,

ext/net/lib.deno_net.d.ts

@@ -253,9 +253,18 @@ declare namespace Deno {
    * this function requires that the other end of the connection is prepared for
    * a TLS handshake.
    *
+   * Note that this function *consumes* the TCP connection passed to it, thus the
+   * original TCP connection will be unusable after calling this. Additionally,
+   * you need to ensure that the TCP connection is not being used elsewhere when
+   * calling this function in order for the TCP connection to be consumed properly.
+   * For instance, if there is a `Promise` that is waiting for read operation on
+   * the TCP connection to complete, it is considered that the TCP connection is
+   * being used elsewhere. In such a case, this function will fail.
+   *
    * ```ts
    * const conn = await Deno.connect({ port: 80, hostname: "127.0.0.1" });
    * const caCert = await Deno.readTextFile("./certs/my_custom_root_CA.pem");
+   * // `conn` becomes unusable after calling `Deno.startTls`
    * const tlsConn = await Deno.startTls(conn, { caCerts: [caCert], hostname: "localhost" });
    * ```
    *

ext/net/ops_tls.rs

@@ -812,12 +812,16 @@ where
     .borrow::<DefaultTlsOptions>()
     .root_cert_store
     .clone();
+
   let resource_rc = state
     .borrow_mut()
     .resource_table
     .take::<TcpStreamResource>(rid)?;
+  // This TCP connection might be used somewhere else. If it's the case, we cannot proceed with the
+  // process of starting a TLS connection on top of this TCP connection, so we just return a bad
+  // resource error. See also: https://github.com/denoland/deno/pull/16242
   let resource = Rc::try_unwrap(resource_rc)
-    .expect("Only a single use of this resource should happen");
+    .map_err(|_| bad_resource("TCP stream is currently in use"))?;
   let (read_half, write_half) = resource.into_inner();
   let tcp_stream = read_half.reunite(write_half)?;
 

runtime/ops/http.rs

@@ -1,6 +1,7 @@
 use std::cell::RefCell;
 use std::rc::Rc;
 
+use deno_core::error::bad_resource;
 use deno_core::error::bad_resource_id;
 use deno_core::error::custom_error;
 use deno_core::error::AnyError;
@@ -44,8 +45,11 @@ fn op_http_start(
     .resource_table
     .take::<TcpStreamResource>(tcp_stream_rid)
   {
+    // This TCP connection might be used somewhere else. If it's the case, we cannot proceed with the
+    // process of starting a HTTP server on top of this TCP connection, so we just return a bad
+    // resource error. See also: https://github.com/denoland/deno/pull/16242
     let resource = Rc::try_unwrap(resource_rc)
-      .expect("Only a single use of this resource should happen");
+      .map_err(|_| bad_resource("TCP stream is currently in use"))?;
     let (read_half, write_half) = resource.into_inner();
     let tcp_stream = read_half.reunite(write_half)?;
     let addr = tcp_stream.local_addr()?;
@@ -56,8 +60,11 @@ fn op_http_start(
     .resource_table
     .take::<TlsStreamResource>(tcp_stream_rid)
   {
+    // This TLS connection might be used somewhere else. If it's the case, we cannot proceed with the
+    // process of starting a HTTP server on top of this TLS connection, so we just return a bad
+    // resource error. See also: https://github.com/denoland/deno/pull/16242
     let resource = Rc::try_unwrap(resource_rc)
-      .expect("Only a single use of this resource should happen");
+      .map_err(|_| bad_resource("TLS stream is currently in use"))?;
     let (read_half, write_half) = resource.into_inner();
     let tls_stream = read_half.reunite(write_half);
     let addr = tls_stream.get_ref().0.local_addr()?;
@@ -71,8 +78,11 @@ fn op_http_start(
   {
     super::check_unstable(state, "Deno.serveHttp");
 
+    // This UNIX socket might be used somewhere else. If it's the case, we cannot proceed with the
+    // process of starting a HTTP server on top of this UNIX socket, so we just return a bad
+    // resource error. See also: https://github.com/denoland/deno/pull/16242
     let resource = Rc::try_unwrap(resource_rc)
-      .expect("Only a single use of this resource should happen");
+      .map_err(|_| bad_resource("UNIX stream is currently in use"))?;
     let (read_half, write_half) = resource.into_inner();
     let unix_stream = read_half.reunite(write_half)?;
     let addr = unix_stream.local_addr()?;