Repository navigation
fetch: reject a close-delimited body that a connection reset cut short #46235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ use core::ptr::NonNull; | |
|
|
||
| use crate::Error; | ||
| use crate::http_thread::InitOpts as HTTPThreadInitOpts; | ||
| use crate::internal_state::ReadEnd; | ||
| use crate::ssl_config::{self, SSLConfig}; | ||
| use crate::{ | ||
| self as http, AlpnOffer, HTTPCertError, HTTPClient, InitError, ProxyTunnel, | ||
|
|
@@ -1465,12 +1466,50 @@ impl<const SSL: bool> Handler<SSL> { | |
| HTTPContext::<SSL>::terminate_socket(socket); | ||
| } | ||
|
|
||
| pub fn on_close(ptr: *mut c_void, socket: HTTPSocket<SSL>, _: c_int, _: Option<*mut c_void>) { | ||
| /// How the read side of `socket` ended, from the close that uSockets reports with | ||
| /// `code`: above `CloseCode::fast_shutdown` that is the error of the failed read (loop.c). | ||
| fn read_end(socket: HTTPSocket<SSL>, code: c_int) -> ReadEnd { | ||
| if !http::STRICT_READ_END { | ||
| return ReadEnd::Fin; | ||
| } | ||
| if SSL { | ||
| // The peer's close_notify ended its stream, whatever the connection did after | ||
| // it. BoringSSL sets the flag for a fatal read error too; uSockets closes that | ||
| // session itself, with no errno. | ||
|
Comment on lines
+1476
to
+1478
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| let peer_ended_stream = socket.ssl().is_some_and(|ssl| { | ||
| // SAFETY: uSockets frees the `SSL` after the close dispatch that this runs in. | ||
| (unsafe { bun_boringssl_sys::SSL_get_shutdown(ssl) } | ||
| & bun_boringssl_sys::SSL_RECEIVED_SHUTDOWN) | ||
| != 0 | ||
| }); | ||
| if peer_ended_stream { | ||
| return ReadEnd::Fin; | ||
| } | ||
| // A send that the kernel rejected took the connection's error. The code of the | ||
| // close that follows stands for the lost records, not for a failed read, so it | ||
| // does not say how the read side ended: the body keeps the old rule. | ||
|
Comment on lines
+1488
to
+1490
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| if socket.ssl_send_rejected() { | ||
| return ReadEnd::Fin; | ||
| } | ||
| } | ||
| if http::is_cut(code) { | ||
| ReadEnd::Error | ||
| } else { | ||
| ReadEnd::Fin | ||
| } | ||
| } | ||
|
|
||
| pub fn on_close( | ||
| ptr: *mut c_void, | ||
| socket: HTTPSocket<SSL>, | ||
| code: c_int, | ||
| _: Option<*mut c_void>, | ||
| ) { | ||
| let tagged = HTTPContext::<SSL>::get_tagged(ptr); | ||
| HTTPContext::<SSL>::mark_socket_as_dead(socket); | ||
|
|
||
| if let Some(client) = tagged.client_mut() { | ||
| return client.on_close::<SSL>(socket); | ||
| return client.on_close::<SSL>(socket, Self::read_end(socket, code)); | ||
| } | ||
| if let Some(session) = tagged.session() { | ||
| return h2::ClientSession::on_close(session, crate::Error::ConnectionClosed); | ||
|
|
@@ -1629,7 +1668,7 @@ impl<const SSL: bool> Handler<SSL> { | |
| } else { | ||
| socket.close(uws::CloseKind::Normal); | ||
| } | ||
| client.on_close::<SSL>(socket); | ||
| client.on_close::<SSL>(socket, ReadEnd::Fin); | ||
| return; | ||
| } | ||
| if let Some(session) = tagged.session() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,18 @@ pub struct InternalStateFlags { | |
| pub(crate) body_compressed: bool, | ||
| /// Held input or buffered decoder output remains for `HTTPClient::drain_response_body`. | ||
| pub(crate) decompress_output_pending: bool, | ||
| /// A request write failed with ECONNRESET. The failed `send()` took that error from the | ||
| /// kernel, so the read side then ends with `recv() == 0` like a FIN. | ||
|
Comment on lines
+101
to
+102
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) write_took_error: bool, | ||
| } | ||
|
|
||
| /// How the read side of a connection ended. | ||
| #[derive(Clone, Copy, PartialEq, Eq)] | ||
| pub(crate) enum ReadEnd { | ||
| /// At the end of the peer's stream: its FIN, or its close_notify on TLS. | ||
| Fin, | ||
| /// With an error and no end of stream, such as a reset. | ||
| Error, | ||
| } | ||
|
|
||
| impl InternalStateFlags { | ||
|
|
@@ -114,6 +126,7 @@ impl InternalStateFlags { | |
| receive_paused: false, | ||
| body_compressed: false, | ||
| decompress_output_pending: false, | ||
| write_took_error: false, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -249,16 +262,19 @@ impl<'a> InternalState<'a> { | |
|
|
||
| /// True when a socket close during `in_progress` completes the body rather | ||
| /// than failing it: chunked decoder already in the trailers state, or a | ||
| /// close-delimited response (no Content-Length, no Transfer-Encoding). | ||
| pub(crate) fn is_body_complete_on_close(&self) -> bool { | ||
| /// close-delimited response (no Content-Length, no Transfer-Encoding) whose | ||
| /// connection reached the end of the peer's stream. | ||
|
Comment on lines
+265
to
+266
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) fn is_body_complete_on_close(&self, read_end: ReadEnd) -> bool { | ||
| // Every byte arrived; only the decode is outstanding. | ||
| if self.flags.decompress_output_pending && self.is_done() { | ||
| return true; | ||
| } | ||
| if self.is_chunked_encoding() { | ||
| return bun_picohttp::phr_decode_chunked_is_in_trailers(&self.chunked_decoder) != 0; | ||
| } | ||
| self.content_length.is_none() && self.response_stage == HTTPStage::Body | ||
| self.content_length.is_none() | ||
| && self.response_stage == HTTPStage::Body | ||
| && read_end == ReadEnd::Fin | ||
| } | ||
|
|
||
| /// Mark the body complete and drive `process_body_buffer` one last time | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -973,7 +973,7 @@ use bun_uws as uws; | |
| use bun_wyhash::Wyhash; | ||
|
|
||
| use crate::http_context::HTTPSocket as HttpSocket; | ||
| use crate::internal_state::{RequestStage, ResponseStage, Stage}; | ||
| use crate::internal_state::{ReadEnd, RequestStage, ResponseStage, Stage}; | ||
|
|
||
| bun_core::declare_scope!(fetch, visible); | ||
|
|
||
|
|
@@ -1484,15 +1484,51 @@ fn print_response(response: &picohttp::Response<'_>) { | |
| Output::flush(); | ||
| } | ||
|
|
||
| /// Whether a close-delimited body completes only at the peer's FIN. Limited to the kernel whose | ||
| /// errno order was measured: on Linux a reset that follows the peer's FIN leaves EPIPE, and only | ||
| /// a reset with no FIN leaves ECONNRESET. Elsewhere every end of the connection completes such a | ||
| /// body. | ||
|
Comment on lines
+1487
to
+1490
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) const STRICT_READ_END: bool = cfg!(any(target_os = "linux", target_os = "android")); | ||
|
|
||
| /// Whether `errno`, a uSockets close code, says that the connection was cut before the peer's | ||
| /// stream ended. Up to `FastShutdown` a close code is this side's own close, and EPIPE is the | ||
| /// reset that followed the peer's FIN. | ||
|
Comment on lines
+1493
to
+1495
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) fn is_cut(errno: i32) -> bool { | ||
| errno > uws::CloseKind::FastShutdown as i32 && errno != bun_sys::E::EPIPE as i32 | ||
| } | ||
|
|
||
| /// One write to the socket that carries a request. Returns what `socket.write` returns. | ||
| /// | ||
| /// A plain-TCP `send()` that fails with ECONNRESET takes the reset out of the kernel, so the | ||
| /// read side can no longer report it: `took_error` keeps that for `HTTPClient::on_close`. | ||
| /// ECONNRESET only: a send can also fail with a timeout or an abort after the peer's FIN | ||
| /// arrived, and the body is whole then. A TLS connection that had a send rejected keeps the | ||
| /// old rule (`Handler::read_end`). | ||
|
Comment on lines
+1500
to
+1506
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| fn write_once<const IS_SSL: bool>( | ||
| socket: HttpSocket<IS_SSL>, | ||
| data: &[u8], | ||
| took_error: &mut bool, | ||
| ) -> i32 { | ||
| if IS_SSL || !STRICT_READ_END { | ||
| return socket.write(data); | ||
| } | ||
| let (written, fatal_errno) = socket.write_check_error(data); | ||
| if fatal_errno == bun_sys::E::ECONNRESET as i32 { | ||
| *took_error = true; | ||
| } | ||
| written | ||
| } | ||
|
|
||
| /// Write data to the socket (Just a error wrapper to easly handle amount written and error handling) | ||
| fn write_to_socket<const IS_SSL: bool>( | ||
| socket: HttpSocket<IS_SSL>, | ||
| data: &[u8], | ||
| took_error: &mut bool, | ||
| ) -> crate::Result<usize> { | ||
| let mut remaining = data; | ||
| let mut total_written: usize = 0; | ||
| while !remaining.is_empty() { | ||
| let amount = socket.write(remaining); | ||
| let amount = write_once::<IS_SSL>(socket, remaining, took_error); | ||
| if amount < 0 { | ||
| return Err(crate::Error::WriteFailed); | ||
| } | ||
|
|
@@ -1511,8 +1547,9 @@ fn write_to_socket_with_buffer_fallback<const IS_SSL: bool>( | |
| socket: HttpSocket<IS_SSL>, | ||
| buffer: &mut bun_io::StreamBuffer, | ||
| data: &[u8], | ||
| took_error: &mut bool, | ||
| ) -> crate::Result<usize> { | ||
| let amount = write_to_socket::<IS_SSL>(socket, data)?; | ||
| let amount = write_to_socket::<IS_SSL>(socket, data, took_error)?; | ||
| if amount < data.len() { | ||
| let _ = buffer.write(&data[amount..]); | ||
| } | ||
|
|
@@ -2246,7 +2283,28 @@ impl<'a> HTTPClient<'a> { | |
| && self.state.response_stage != ResponseStage::BodyChunk | ||
| } | ||
|
|
||
| pub(crate) fn on_close<const IS_SSL: bool>(&mut self, socket: HttpSocket<IS_SSL>) { | ||
| /// `read_end` as it counts for the body of this request. | ||
| fn body_read_end(&self, read_end: ReadEnd) -> ReadEnd { | ||
| // An upgraded connection is a byte stream with no body to cut. On a unix socket | ||
| // ECONNRESET only says that the peer closed with input unread, whole answer or not. | ||
| // Behind a CONNECT tunnel the socket ends with the proxy's teardown, not the origin's. | ||
|
Comment on lines
+2288
to
+2290
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| if self.flags.upgrade_state == HTTPUpgradeState::Upgraded | ||
| || !self.unix_socket_path.is_empty() | ||
| || self.proxy_tunnel.is_some() | ||
| { | ||
| return ReadEnd::Fin; | ||
| } | ||
| if self.state.flags.write_took_error { | ||
| return ReadEnd::Error; | ||
| } | ||
| read_end | ||
| } | ||
|
|
||
| pub(crate) fn on_close<const IS_SSL: bool>( | ||
| &mut self, | ||
| socket: HttpSocket<IS_SSL>, | ||
| read_end: ReadEnd, | ||
| ) { | ||
| bun_core::scoped_log!(fetch, "Closed {}\n", BStr::new(self.url.href)); | ||
| // the socket is closed, we need to unregister the abort tracker | ||
| self.unregister_abort_tracker(); | ||
|
|
@@ -2255,6 +2313,8 @@ impl<'a> HTTPClient<'a> { | |
| self.fail(crate::Error::Aborted); | ||
| return; | ||
| } | ||
| // Before the tunnel goes: a tunnelled body is exempt. | ||
| let read_end = self.body_read_end(read_end); | ||
| self.close_proxy_tunnel(true); | ||
| let in_progress = self.is_in_progress(); | ||
| if self.state.flags.is_redirect_pending { | ||
|
|
@@ -2264,7 +2324,10 @@ impl<'a> HTTPClient<'a> { | |
| self.do_redirect::<IS_SSL>(ctx, socket); | ||
| return; | ||
| } | ||
| if in_progress && self.state.is_body_complete_on_close() { | ||
| // A close-delimited body ends at the peer's FIN (RFC 9112 section 8): a reset leaves | ||
| // it incomplete. undici completes it (`onHttpSocketError` in client-h1.js, | ||
| // nodejs/undici#784), so Node's fetch resolves with the part that arrived. | ||
|
Comment on lines
+2327
to
+2329
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| if in_progress && self.state.is_body_complete_on_close(read_end) { | ||
| if let Err(err) = self.state.finalize_body_on_eof() { | ||
| self.fail(err); | ||
| return; | ||
|
|
@@ -3153,7 +3216,8 @@ impl<'a> HTTPClient<'a> { | |
| if socket.is_closed() || socket.is_shutdown() { | ||
| return Err(crate::Error::ConnectionClosed); | ||
| } | ||
| let amount = write_to_socket::<IS_SSL>(socket, to_send)?; | ||
| let amount = | ||
| write_to_socket::<IS_SSL>(socket, to_send, &mut self.state.flags.write_took_error)?; | ||
| if IS_FIRST_CALL { | ||
| if amount == 0 { | ||
| // don't worry about it | ||
|
|
@@ -3271,7 +3335,11 @@ impl<'a> HTTPClient<'a> { | |
|
|
||
| let to_send_len = buffer.slice().len(); | ||
| if to_send_len > 0 { | ||
| let amount = write_to_socket::<IS_SSL>(socket, buffer.slice())?; | ||
| let amount = write_to_socket::<IS_SSL>( | ||
| socket, | ||
| buffer.slice(), | ||
| &mut self.state.flags.write_took_error, | ||
| )?; | ||
| self.state.request_sent_len += amount; | ||
| buffer.cursor += amount; | ||
| if amount < to_send_len { | ||
|
|
@@ -3290,7 +3358,12 @@ impl<'a> HTTPClient<'a> { | |
| // ok we flushed all pending data so we can reset the backpressure | ||
| if !data.is_empty() { | ||
| // no backpressure everything was sended so we can just try to send | ||
| let sent = write_to_socket_with_buffer_fallback::<IS_SSL>(socket, buffer, data)?; | ||
| let sent = write_to_socket_with_buffer_fallback::<IS_SSL>( | ||
| socket, | ||
| buffer, | ||
| data, | ||
| &mut self.state.flags.write_took_error, | ||
| )?; | ||
| self.state.request_sent_len += sent; | ||
| // if we didn't send all the data we have backpressure | ||
| return Ok(sent < data.len()); | ||
|
|
@@ -3507,9 +3580,14 @@ impl<'a> HTTPClient<'a> { | |
|
|
||
| match &mut self.state.original_request_body { | ||
| HTTPRequestBody::Bytes(_) => { | ||
| let to_send = self.request_body(); | ||
| // The field, not `request_body()`: the write borrows the latch beside it. | ||
| let to_send = self.state.request_body.slice(); | ||
| if !to_send.is_empty() { | ||
| let sent = match write_to_socket::<IS_SSL>(socket, to_send) { | ||
| let sent = match write_to_socket::<IS_SSL>( | ||
| socket, | ||
| to_send, | ||
| &mut self.state.flags.write_took_error, | ||
| ) { | ||
| Ok(s) => s, | ||
| Err(err) => { | ||
| self.close_and_fail::<IS_SSL>(err, socket); | ||
|
|
@@ -4247,7 +4325,11 @@ impl<'a> HTTPClient<'a> { | |
| || self.proxy_tunnel.is_some() | ||
| || self.flags.upgrade_state == HTTPUpgradeState::Upgraded | ||
| || !self.signals.is_receive_paused() | ||
| || socket.is_closed_or_has_error() | ||
| || socket.is_closed() | ||
| || socket.is_shutdown() | ||
| // Under the rule, not SO_ERROR: reading it takes a pending reset out of the kernel, | ||
| // and the `recv()` after the pause then returns 0 like a FIN. | ||
|
Comment on lines
+4330
to
+4331
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| || (!STRICT_READ_END && socket.get_error() != 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -286,6 +286,10 @@ impl<const IS_SSL: bool> NewSocketHandler<IS_SSL> { | |
| ) | ||
| } | ||
|
|
||
| /// Reads AND clears `SO_ERROR`. Once it has taken a pending reset, the next | ||
| /// `recv()` returns 0 like a FIN, so a caller that still reads a body from | ||
| /// this socket must not call it: it is for the decision to keep or drop a | ||
| /// connection whose exchange is over. | ||
|
Comment on lines
+289
to
+292
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| #[inline] | ||
| pub fn is_closed_or_has_error(&self) -> bool { | ||
| self.is_closed() || self.is_shutdown() || self.get_error() != 0 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code