From 44a89dd6dc7864822ddb48d030af519160de90a2 Mon Sep 17 00:00:00 2001 From: Yusuke Tanaka Date: Tue, 18 Oct 2022 11:28:27 +0900 Subject: [PATCH] fix(ext/net): return an error from `startTls` and `serveHttp` if the original connection is captured elsewhere (#16242) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- cli/dts/lib.deno.ns.d.ts | 8 ++++ cli/tests/unit/http_test.ts | 80 +++++++++++++++++++++++++++++++++++++ cli/tests/unit/tls_test.ts | 27 +++++++++++++ core/resources.rs | 12 ++++++ ext/net/lib.deno_net.d.ts | 9 +++++ ext/net/ops_tls.rs | 6 ++- runtime/ops/http.rs | 16 ++++++-- 7 files changed, 154 insertions(+), 4 deletions(-) diff --git a/cli/dts/lib.deno.ns.d.ts b/cli/dts/lib.deno.ns.d.ts index 9c645882d6..c01ac80fba 100644 --- a/cli/dts/lib.deno.ns.d.ts +++ b/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; diff --git a/cli/tests/unit/http_test.ts b/cli/tests/unit/http_test.ts index 63eae3ace6..566efce6d8 100644 --- a/cli/tests/unit/http_test.ts +++ b/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); diff --git a/cli/tests/unit/tls_test.ts b/cli/tests/unit/tls_test.ts index cf335de499..860965e499 100644 --- a/cli/tests/unit/tls_test.ts +++ b/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() { diff --git a/core/resources.rs b/core/resources.rs index ee9ee689f4..164c377a21 100644 --- a/core/resources.rs +++ b/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` 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`. pub fn take(&mut self, rid: ResourceId) -> Result, Error> { let resource = self.get::(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` 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`. pub fn take_any( &mut self, rid: ResourceId, diff --git a/ext/net/lib.deno_net.d.ts b/ext/net/lib.deno_net.d.ts index d6c7d4afd1..df569f93a7 100644 --- a/ext/net/lib.deno_net.d.ts +++ b/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" }); * ``` * diff --git a/ext/net/ops_tls.rs b/ext/net/ops_tls.rs index a1b48b84e4..a59cd747ed 100644 --- a/ext/net/ops_tls.rs +++ b/ext/net/ops_tls.rs @@ -812,12 +812,16 @@ where .borrow::() .root_cert_store .clone(); + let resource_rc = state .borrow_mut() .resource_table .take::(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)?; diff --git a/runtime/ops/http.rs b/runtime/ops/http.rs index 7a58da3612..e751adae80 100644 --- a/runtime/ops/http.rs +++ b/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::(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::(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()?;