fix(tls): flush send buffer in the background after closing TLS stream #10146

piscisaureus · 2021-04-12T14:59:18Z

In #9118, TLS streams were split into a "read half" and a "write half"
using tokio::io::split() to allow concurrent Conn#read() and
Conn#write() calls without one blocking the other. However, this
introduced a bug: outgoing data gets discarded when the TLS stream is
gracefully closed, because the read half is closed too early, before all
TLS control data has been received.

Fixes: #9692
Fixes: #10049
Fixes: #10296
Fixes: denoland/std#750

runtime/ops/tls.rs

bnoordhuis · 2021-04-13T17:15:23Z

runtime/ops/io.rs


-impl Resource for TlsClientStreamResource {
+impl Resource for TlsStreamResource {
  fn name(&self) -> Cow<str> {


A bit of a tangent but for my edification: why Cow<str> instead of &'static str? Literally all implementers call into() on a &'static str.

Good point actually. But I think changing that belongs in a separate commit.

runtime/ops/tls.rs

ry

Can tokio-rustls be removed from cli/Cargo.toml and test_util/Cargo.toml now?

quininer · 2021-04-15T08:14:15Z

tokio-rustls streams misbehave when split with tokio::io::split() into a "read half" and a "write half".

Hi, can you describe the problem? I am very interested in it.

runtime/ops/tls.rs

quininer · 2021-04-15T09:26:15Z

runtime/ops/tls.rs

+    ready!(self.poll_io(cx, Flow::Write))?;
+
+    match self.tls.write(buf) {
+      Ok(0) => unreachable!(),


It may return 0 when the internal buffer of rustls is full.

That shouldn't happen, because poll_io() returns Poll::Pending when the buffer is full.
However it will return 0 when buf is empty, so I removed the assertion anyway.

No, it will not return to pending. see https://github.com/ctz/rustls/blob/47d309233f2d21ccf058d35c58d684e5e4c9c643/rustls/src/server/mod.rs#L738

No, it will not return to pending. see https://github.com/ctz/rustls/blob/47d309233f2d21ccf058d35c58d684e5e4c9c643/rustls/src/server/mod.rs#L738

poll_io() returns Poll::Pending when the buffer is full:

deno/runtime/ops/tls.rs

Line 325 in 800ae5e

true => Poll::Pending,

Sorry, I didn't see to poll_io, you are right.

quininer · 2021-04-15T09:30:30Z

runtime/ops/tls.rs

+      unsafe { &mut *(buf.unfilled_mut() as *mut [MaybeUninit<u8>] as *mut _) };
+
+    match self.tls.read(buf_slice) {
+      Ok(0) => unreachable!(),


I think it will return 0 at eof.

quininer · 2021-04-15T09:32:32Z

runtime/ops/tls.rs

+  ) -> Poll<io::Result<()>> {
+    ready!(self.poll_io(cx, Flow::Read))?;
+
+    if matches!(self.rd_shut, Shut::TlsShut | Shut::TcpShut) {


I think it obscures the shutdown status of tcp stream and tls stream. When eof occurs in the tcp stream, there may still be readable data in the buffer of the tls stream.

runtime/ops/tls.rs

runtime/ops/http.rs

ry · 2021-04-16T10:37:09Z

op_crates/websocket/lib.rs

@@ -161,7 +161,7 @@ where

      let tls_connector = TlsConnector::from(Arc::new(config));
      let dnsname =
-        DNSNameRef::try_from_ascii_str(&domain).expect("Invalid DNS lookup");
+        DNSNameRef::try_from_ascii_str(&domain).expect("invalid hostname");


is this related to tokio-rustls?

ry · 2021-04-16T10:37:17Z

cli/tests/unit/tls_test.ts

@@ -32,7 +32,7 @@ unitTest(

    await assertThrowsAsync(async () => {
      await Deno.connectTls({ hostname: "127.0.0.1", port: 3567 });
-    }, Error);
+    }, TypeError);


ry · 2021-04-16T10:38:00Z

cli/tests/listen_tls_alpn.ts

@@ -8,5 +10,8 @@ const listener = Deno.listenTls({
 console.log("READY");

 for await (const conn of listener) {
+  if (await conn.read(new Uint8Array(1)) !== null) {
+    throw new Error("should not receive data on TLS connection");
+  }


Is this demonstrating the bug this PR is fixing?

cli/tests/integration_tests.rs

piscisaureus · 2021-04-16T18:25:49Z

Can tokio-rustls be removed from cli/Cargo.toml and test_util/Cargo.toml now?

From CLI, yes.

But test_util and op_crates/deno_websocket still use tokio-rustls. It would be possible to switch them, but in order to do that we would have to move the TlsStream implementation to a separate crate, since neither test_util nor deno_websocket depend on deno_runtime, and it seems best to keep it that way.

Note that deno_websocket inherits the dependency from tokio-tungstenite, which uses a very similar mitigation for what appears to be the same problem.

ry · 2021-05-04T10:59:56Z

@piscisaureus What's the status of this PR?

denoland#10146) In denoland#9118, TLS streams were split into a "read half" and a "write half" using tokio::io::split() to allow concurrent Conn#read() and Conn#write() calls without one blocking the other. However, this introduced a bug: outgoing data gets discarded when the TLS stream is gracefully closed, because the read half is closed too early, before all TLS control data has been received. Fixes: denoland#9692 Fixes: denoland#10049 Fixes: denoland#10296 Fixes: denoland/std#750

…LS stream (denoland#10146)" This reverts commit 640d431.

piscisaureus force-pushed the tls branch from 649e4f2 to e388fa9 Compare April 13, 2021 00:55

piscisaureus requested review from bnoordhuis, bartlomieju and lucacasonato and removed request for bartlomieju April 13, 2021 11:50

This comment has been minimized.

Sign in to view

piscisaureus commented Apr 13, 2021

View reviewed changes

runtime/ops/tls.rs Outdated Show resolved Hide resolved

bnoordhuis approved these changes Apr 13, 2021

View reviewed changes

bnoordhuis approved these changes Apr 14, 2021

View reviewed changes

runtime/ops/tls.rs Show resolved Hide resolved

ry reviewed Apr 14, 2021

View reviewed changes

quininer reviewed Apr 15, 2021

View reviewed changes

runtime/ops/tls.rs Show resolved Hide resolved

quininer reviewed Apr 15, 2021

View reviewed changes

runtime/ops/tls.rs Outdated Show resolved Hide resolved

ry reviewed Apr 16, 2021

View reviewed changes

runtime/ops/http.rs Outdated Show resolved Hide resolved

ry reviewed Apr 16, 2021

View reviewed changes

cli/tests/integration_tests.rs Outdated Show resolved Hide resolved

quininer mentioned this pull request Apr 16, 2021

WakerProxy may be unnecessary snapview/tokio-tungstenite#163

Open

AaronO mentioned this pull request Apr 22, 2021

Deno hangs after a while, serving a static file over HTTPS #10296

Closed

lucacasonato mentioned this pull request Apr 30, 2021

Native HTTP listener hangs with TLS #10419

Closed

piscisaureus changed the title ~~WIP: integrate rustls without using tokio-rustls~~ fix(tls): flush send buffer in the background after closing TLS stream May 10, 2021

piscisaureus force-pushed the tls branch from ccf5c80 to 8e24951 Compare May 11, 2021 00:50

piscisaureus force-pushed the tls branch from 8e24951 to 640d431 Compare May 11, 2021 01:11

piscisaureus merged commit 640d431 into denoland:main May 11, 2021

piscisaureus deleted the tls branch May 11, 2021 01:40

kitsonk mentioned this pull request May 11, 2021

Unable to send large files oakserver/oak#317

Closed

bartlomieju added a commit to bartlomieju/deno that referenced this pull request May 11, 2021

Revert "fix(tls): flush send buffer in the background after closing T…

21d4571

…LS stream (denoland#10146)" This reverts commit 640d431.

bartlomieju mentioned this pull request May 11, 2021

Revert "fix(tls): flush send buffer in the background after closing T… #10595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tls): flush send buffer in the background after closing TLS stream #10146

fix(tls): flush send buffer in the background after closing TLS stream #10146

piscisaureus commented Apr 12, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

bnoordhuis Apr 13, 2021

piscisaureus Apr 13, 2021

ry left a comment

quininer commented Apr 15, 2021

quininer Apr 15, 2021

piscisaureus Apr 16, 2021 •

edited

Loading

quininer Apr 16, 2021

piscisaureus Apr 16, 2021 •

edited

Loading

quininer Apr 16, 2021

quininer Apr 15, 2021

quininer Apr 15, 2021

ry Apr 16, 2021

ry Apr 16, 2021

ry Apr 16, 2021

piscisaureus commented Apr 16, 2021

ry commented May 4, 2021

fix(tls): flush send buffer in the background after closing TLS stream #10146

fix(tls): flush send buffer in the background after closing TLS stream #10146

Conversation

piscisaureus commented Apr 12, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment

Choose a reason for hiding this comment

quininer commented Apr 15, 2021

Choose a reason for hiding this comment

piscisaureus Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus commented Apr 16, 2021

ry commented May 4, 2021

piscisaureus commented Apr 12, 2021 •

edited

Loading

piscisaureus Apr 16, 2021 •

edited

Loading

piscisaureus Apr 16, 2021 •

edited

Loading