From 310facdcef45f176fc53d1673ca1a57cc6a2e199 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sat, 8 Aug 2026 12:56:53 +0000 Subject: [PATCH 01/11] s3: read S3's XML responses with the XML parser instead of substring scraping ListObjectsV2 results, bodies (simple requests and download streams) and the multipart UploadId were extracted with index_of("") scans, which returned XML-escaped text verbatim (a key "a & b" came back with the entity in it), could be confused by markup-looking text, and half-parsed ill-formed bodies. They now go through bun_parsers::xml (node shape, so text is exact) via a small s3/xml_response helper. A 200 ListObjectsV2 body that is not a well-formed now rejects with code InvalidResponse instead of resolving with a partial or empty listing. Also: the per-thread recycled parse arena is factored out of the Bun.{XML,JSONC,TOML,YAML,JSON5}.parse scaffold (RecycledArena) and shared with the S3 path, and the xml_parse feature counter moves from the parser to its API entry points so internal use is not counted. --- src/bundler/ParseTask.rs | 1 + src/bundler/transpiler.rs | 1 + src/parsers/xml.rs | 1 - src/runtime/api.rs | 51 ++- src/runtime/api/XMLObject.rs | 1 + src/runtime/webcore.rs | 4 + src/runtime/webcore/s3/download_stream.rs | 108 +++--- src/runtime/webcore/s3/list_objects.rs | 399 +++------------------- src/runtime/webcore/s3/multipart.rs | 20 +- src/runtime/webcore/s3/simple_request.rs | 121 ++++--- src/runtime/webcore/s3/xml_response.rs | 130 +++++++ test/js/bun/s3/s3-list-objects.test.ts | 115 +++++-- 12 files changed, 427 insertions(+), 525 deletions(-) create mode 100644 src/runtime/webcore/s3/xml_response.rs diff --git a/src/bundler/ParseTask.rs b/src/bundler/ParseTask.rs index 841a828cce53..10a36a7050b1 100644 --- a/src/bundler/ParseTask.rs +++ b/src/bundler/ParseTask.rs @@ -847,6 +847,7 @@ pub mod parse_worker { let _trace = perf::trace("Bundler.ParseXML"); let mut temp_log = Log::init(); let result = (|| -> core::result::Result, AnyError> { + bun_core::analytics::Features::xml_parse_inc(); let rows: Expr = bun_parsers::xml::XML::parse( source, &mut temp_log, diff --git a/src/bundler/transpiler.rs b/src/bundler/transpiler.rs index ffba2316eae1..c6848d9bfce9 100644 --- a/src/bundler/transpiler.rs +++ b/src/bundler/transpiler.rs @@ -1875,6 +1875,7 @@ fn parse_data_loader<'a>( compact: true, encoding: bun_parsers::xml::InputEncoding::File, }; + bun_core::analytics::Features::xml_parse_inc(); match bun_parsers::xml::XML::parse(source, log, arena, options) { Ok(e) => e, Err(_) => return None, diff --git a/src/parsers/xml.rs b/src/parsers/xml.rs index 999fd9163266..300a70188cc6 100644 --- a/src/parsers/xml.rs +++ b/src/parsers/xml.rs @@ -112,7 +112,6 @@ impl XML { bump: &'a Bump, options: Options, ) -> crate::Result { - bun_core::analytics::Features::xml_parse_inc(); let mut tape = Tape::new_in(bump, core::mem::size_of_val(contents)); // SAFETY: see `Tape::object_from`. unsafe { tape.tape.as_mut() }.encoding = if U::WIDE { diff --git a/src/runtime/api.rs b/src/runtime/api.rs index 76bf14fbf80a..26f0f00c467a 100644 --- a/src/runtime/api.rs +++ b/src/runtime/api.rs @@ -248,6 +248,37 @@ enum SourceEncoding { Utf16Text, } +/// The calling thread's scratch arena for parsing one document, handed back +/// (reset, keeping up to 2 MiB) on drop. A private mi_heap costs microseconds +/// to create — more than parsing a small document — so one is kept per thread. +/// `#[thread_local]` rather than `thread_local!` so there is no destructor +/// racing mimalloc's own thread teardown (as in `ast_memory_allocator.rs`); a +/// parked heap is reclaimed with the thread. Re-entrant use just gets a fresh +/// arena. +pub(crate) struct RecycledArena(Option); + +#[thread_local] +static PARKED_ARENA: core::cell::Cell> = core::cell::Cell::new(None); + +impl RecycledArena { + pub(crate) fn take() -> Self { + Self(Some(PARKED_ARENA.take().unwrap_or_default())) + } + + pub(crate) fn arena(&self) -> &bun_alloc::Arena { + self.0.as_ref().expect("live until drop") + } +} + +impl Drop for RecycledArena { + fn drop(&mut self) { + if let Some(mut arena) = self.0.take() { + arena.reset_retain_with_limit(2 * 1024 * 1024); + PARKED_ARENA.set(Some(arena)); + } + } +} + fn with_text_format_source_encoded( global: &bun_jsc::JSGlobalObject, frame: &bun_jsc::CallFrame, @@ -264,24 +295,8 @@ fn with_text_format_source_encoded( ) -> bun_jsc::JsResult { use crate::node::{BlobOrStringOrBuffer, StringOrBuffer}; - // A private mi_heap costs microseconds to create, more than parsing a - // small document: keep one per thread and recycle it between calls. - // `#[thread_local]` rather than `thread_local!` so there is no - // destructor racing mimalloc's own thread teardown (as in - // `ast_memory_allocator.rs`); a parked heap is reclaimed with the thread. - #[thread_local] - static ARENA: core::cell::Cell> = core::cell::Cell::new(None); - struct Recycle(Option); - impl Drop for Recycle { - fn drop(&mut self) { - if let Some(mut arena) = self.0.take() { - arena.reset_retain_with_limit(2 * 1024 * 1024); - ARENA.set(Some(arena)); - } - } - } - let recycle = Recycle(Some(ARENA.take().unwrap_or_default())); - let arena = recycle.0.as_ref().expect("set above"); + let recycle = RecycledArena::take(); + let arena = recycle.arena(); let mut ast_memory_allocator = bun_ast::ASTMemoryAllocator::borrowing(arena); let _ast_scope = ast_memory_allocator.enter(); diff --git a/src/runtime/api/XMLObject.rs b/src/runtime/api/XMLObject.rs index 51d1fc73b7e5..8afe303573fe 100644 --- a/src/runtime/api/XMLObject.rs +++ b/src/runtime/api/XMLObject.rs @@ -59,6 +59,7 @@ pub(crate) fn parse(global: &JSGlobalObject, frame: &CallFrame) -> JsResult xml::InputEncoding::Latin1, super::SourceEncoding::Utf16Text => xml::InputEncoding::Text, }; + bun_core::analytics::Features::xml_parse_inc(); let mut result = if source_encoding == super::SourceEncoding::Utf16Text { // The scaffold hands the string's code units over as bytes. let units: &[u16] = bytemuck::cast_slice(&source.contents); diff --git a/src/runtime/webcore.rs b/src/runtime/webcore.rs index cdf852169cf4..271109c2f172 100644 --- a/src/runtime/webcore.rs +++ b/src/runtime/webcore.rs @@ -327,6 +327,9 @@ pub mod __s3_multipart; #[doc(hidden)] #[path = "webcore/s3/simple_request.rs"] pub mod __s3_simple_request; +#[doc(hidden)] +#[path = "webcore/s3/xml_response.rs"] +pub mod __s3_xml_response; pub mod s3 { pub use super::multipart_options_impl as multipart_options; pub use super::multipart_options_impl::MultiPartUploadOptions; @@ -339,6 +342,7 @@ pub mod s3 { pub use super::__s3_list_objects as list_objects; pub use super::__s3_multipart as multipart; pub use super::__s3_simple_request as simple_request; + pub(crate) use super::__s3_xml_response as xml_response; pub use multipart::MultiPartUpload; } diff --git a/src/runtime/webcore/s3/download_stream.rs b/src/runtime/webcore/s3/download_stream.rs index 24de3c8a204b..1f744679a94b 100644 --- a/src/runtime/webcore/s3/download_stream.rs +++ b/src/runtime/webcore/s3/download_stream.rs @@ -2,13 +2,15 @@ use core::ffi::c_void; use core::ptr::NonNull; use core::sync::atomic::{AtomicBool, AtomicU64, Ordering}; -use bun_core::{MutableString, strings}; +use bun_core::MutableString; use bun_event_loop::ConcurrentTask::{AutoDeinit, ConcurrentTask}; use bun_event_loop::{TaskTag, Taskable, task_tag}; use bun_http::{AsyncHTTP, HTTPClientResult, Headers, Signals}; use bun_io::KeepAlive; use bun_s3_signing::credentials::SignResult; use bun_s3_signing::error::S3Error; + +use crate::webcore::s3::xml_response; use bun_threading::Mutex; bun_core::declare_scope!(S3, hidden); @@ -67,86 +69,56 @@ impl S3HttpDownloadStreamingTask { fn report_progress(&mut self, state: State) { let has_more = state.has_more(); - let mut err: Option = None; let failed = match state.status_code() { 200 | 204 | 206 => state.request_error() != 0, _ => true, }; - - // reshaped for borrowck — `code`/`message` borrow from - // `self.reported_response_buffer`, so we compute the chunk after the - // borrow scope ends rather than inside the labeled block. - let chunk: MutableString = 'brk: { - if failed { - if !has_more { - let mut _has_body_code = false; - let mut _has_body_message = false; - - let mut code: &[u8] = b"UnknownError"; - let mut message: &[u8] = b"an unexpected error has occurred"; - if let Some(req_err) = self.request_error { - code = req_err.name().as_bytes(); - _has_body_code = true; - } else { - let bytes = self.reported_response_buffer.list.as_slice(); - if !bytes.is_empty() { - message = bytes; - - if let Some(start) = strings::index_of(bytes, b"") { - let value_start = start + b"".len(); - if let Some(end) = - strings::index_of(&bytes[value_start..], b"") - { - code = &bytes[value_start..value_start + end]; - _has_body_code = true; - } - } - if let Some(start) = strings::index_of(bytes, b"") { - let value_start = start + b"".len(); - if let Some(end) = - strings::index_of(&bytes[value_start..], b"") - { - message = &bytes[value_start..value_start + end]; - _has_body_message = true; - } - } - } - } - - // `code`/`message` borrow `self.reported_response_buffer`; - // the callback consumes them before any reset/deinit. - err = Some(S3Error { code, message }); - } - break 'brk MutableString::default(); - } else { - // `core::mem::take` transfers ownership of the buffer, leaving an - // empty MutableString behind. - let buffer = core::mem::take(&mut self.reported_response_buffer); - break 'brk buffer; - } - }; bun_core::scoped_log!( S3, "reportProgres failed: {} has_more: {} len: {}", failed, has_more, - chunk.len() + self.reported_response_buffer.list.len() ); + if failed { - if !has_more { - (self.callback)(&chunk, false, err, self.callback_context.as_ptr().cast()); + if has_more { + return; } - } else { - // dont report empty chunks if we have more data to read - if !has_more || chunk.len() > 0 { - (self.callback)( - &chunk, - has_more, - None, - self.callback_context.as_ptr().cast(), - ); - self.reported_response_buffer.reset(); + let callback = self.callback; + let context = self.callback_context.as_ptr().cast(); + let empty = MutableString::default(); + let message: &[u8] = b"an unexpected error has occurred"; + if let Some(req_err) = self.request_error { + let code = req_err.name().as_bytes(); + callback(&empty, false, Some(S3Error { code, message }), context); + return; } + // `code` / `message` borrow the parsed body; the callback consumes + // them before it returns. + let bytes = self.reported_response_buffer.list.as_slice(); + xml_response::with_error(bytes, |error| { + let (code, body_message) = error.unwrap_or((None, None)); + let code = code.unwrap_or(b"UnknownError"); + let message = + body_message.unwrap_or(if bytes.is_empty() { message } else { bytes }); + callback(&empty, false, Some(S3Error { code, message }), context); + }); + return; + } + + // dont report empty chunks if we have more data to read + if !has_more || self.reported_response_buffer.list.len() > 0 { + // `core::mem::take` transfers ownership of the buffer, leaving an + // empty MutableString behind. + let chunk = core::mem::take(&mut self.reported_response_buffer); + (self.callback)( + &chunk, + has_more, + None, + self.callback_context.as_ptr().cast(), + ); + self.reported_response_buffer.reset(); } } diff --git a/src/runtime/webcore/s3/list_objects.rs b/src/runtime/webcore/s3/list_objects.rs index 05962cdb1d22..0dae1192007d 100644 --- a/src/runtime/webcore/s3/list_objects.rs +++ b/src/runtime/webcore/s3/list_objects.rs @@ -1,10 +1,9 @@ -use std::borrow::Cow; - use bun_jsc::bun_string_jsc::create_utf8_for_js; use bun_jsc::{JSGlobalObject, JSValue, JsResult}; // Shared S3 option-string ladder (get_truthy → is_string → from_js → to_utf8). use super::__s3_credentials_jsc::get_truthy_string_utf8; -use bun_core::{ZigStringSlice as Utf8Slice, strings}; +use super::s3::xml_response; +use bun_core::ZigStringSlice as Utf8Slice; pub struct S3ListObjectsOptions { // Each `Utf8Slice` owns (or ref-holds) its backing storage; readers go @@ -21,12 +20,9 @@ pub struct S3ListObjectsOptions { // Each Utf8Slice field cleans up via Drop, so no explicit `impl Drop` is // needed here. -// result structs borrow slices out of the input `xml: &[u8]` -// passed to `parse_s3_list_objects_result` (they alias the request body -// buffer). Represented with an explicit `'a` — -// the borrow is unambiguous and any other encoding (Box / raw ptr) would -// misrepresent ownership. The caller keeps `xml` alive for the result's -// lifetime (result is consumed by toJS before the response body is freed). +// The result structs borrow from the parsed response document handed to +// `parse_s3_list_objects_result`; the caller consumes them (toJS) inside +// that document's scope. struct ObjectOwner<'a> { id: Option<&'a [u8]>, @@ -35,8 +31,7 @@ struct ObjectOwner<'a> { pub struct S3ListObjectsContents<'a> { key: &'a [u8], - // a maybe-owned slice. - etag: Option>, + etag: Option<&'a [u8]>, checksum_type: Option<&'a [u8]>, checksum_algorithm: Option<&'a [u8]>, last_modified: Option<&'a [u8]>, @@ -60,8 +55,6 @@ pub struct S3ListObjectsV2Result<'a> { pub(crate) contents: Option>>, } -// `contents` items (etag) + the two Vecs are all handled by Drop on -// Vec / Cow; no explicit Drop impl needed. impl<'a> S3ListObjectsV2Result<'a> { pub(crate) fn to_js(&self, global_object: &JSGlobalObject) -> JsResult { @@ -97,7 +90,7 @@ impl<'a> S3ListObjectsV2Result<'a> { create_utf8_for_js(global_object, item.key)?, ); - object_info.put_optional_utf8(global_object, b"eTag", item.etag.as_deref())?; + object_info.put_optional_utf8(global_object, b"eTag", item.etag)?; if let Some(algorithm) = item.checksum_algorithm { let js_algorithm = create_utf8_for_js(global_object, algorithm)?; object_info.put(global_object, b"checksumAlgorithm", js_algorithm); @@ -175,9 +168,11 @@ impl<'a> S3ListObjectsV2Result<'a> { } } -// Infallible: the only fallible operations are allocations -// (Vec::push / alloc), which abort on OOM. -pub(crate) fn parse_s3_list_objects_result(xml: &[u8]) -> S3ListObjectsV2Result<'_> { +/// Reads a `ListObjectsV2` response's ``; the result +/// borrows from it. +pub(crate) fn parse_s3_list_objects_result<'a>( + root: xml_response::Node<'a>, +) -> S3ListObjectsV2Result<'a> { let mut result = S3ListObjectsV2Result { contents: None, common_prefixes: None, @@ -192,336 +187,50 @@ pub(crate) fn parse_s3_list_objects_result(xml: &[u8]) -> S3ListObjectsV2Result< prefix: None, start_after: None, }; - - let mut contents: Vec> = Vec::new(); - let mut common_prefixes: Vec<&[u8]> = Vec::new(); - - // we dont use trailing ">" as it may finish with xmlns=... - if let Some(delete_result_pos) = strings::index_of(xml, b"") { - i += 1; - let tag_name_end_pos = i + end; // +1 for < - - let tag_name = &xml[i..tag_name_end_pos]; - i = tag_name_end_pos + 1; // +1 for > - - if tag_name == b"Contents" { - let mut looking_for_end_tag = true; - - let mut object_key: Option<&[u8]> = None; - let mut last_modified: Option<&[u8]> = None; - let mut object_size: Option = None; - let mut storage_class: Option<&[u8]> = None; - let mut etag: Option<&[u8]> = None; - let mut etag_owned: Option> = None; - let mut checksum_type: Option<&[u8]> = None; - let mut checksum_algorithm: Option<&[u8]> = None; - let mut owner_id: Option<&[u8]> = None; - let mut owner_display_name: Option<&[u8]> = None; - - while looking_for_end_tag { - if i >= xml.len() { - break; - } - - if xml[i] == b'<' { - if let Some(__end) = strings::index_of(&xml[i + 1..], b">") { - let inner_tag_name_or_tag_end = &xml[i + 1..i + 1 + __end]; - - i = i + 2 + __end; - - if inner_tag_name_or_tag_end == b"/Contents" { - looking_for_end_tag = false; - } else if inner_tag_name_or_tag_end == b"Key" { - if let Some(__tag_end) = strings::index_of(&xml[i..], b"") - { - object_key = Some(&xml[i..i + __tag_end]); - i = i + __tag_end + 6; - } else { - i = xml.len(); - } - } else if inner_tag_name_or_tag_end == b"LastModified" { - if let Some(__tag_end) = - strings::index_of(&xml[i..], b"") - { - last_modified = Some(&xml[i..i + __tag_end]); - i = i + __tag_end + 15; - } else { - i = xml.len(); - } - } else if inner_tag_name_or_tag_end == b"Size" { - if let Some(__tag_end) = - strings::index_of(&xml[i..], b"") - { - let size = &xml[i..i + __tag_end]; - - object_size = bun_core::fmt::parse_decimal::(size); - i = i + __tag_end + 7; - } else { - i = xml.len(); - } - } else if inner_tag_name_or_tag_end == b"StorageClass" { - if let Some(__tag_end) = - strings::index_of(&xml[i..], b"") - { - storage_class = Some(&xml[i..i + __tag_end]); - i = i + __tag_end + 15; - } else { - i = xml.len(); - } - } else if inner_tag_name_or_tag_end == b"ChecksumType" { - if let Some(__tag_end) = - strings::index_of(&xml[i..], b"") - { - checksum_type = Some(&xml[i..i + __tag_end]); - i = i + __tag_end + 15; - } else { - i = xml.len(); - } - } else if inner_tag_name_or_tag_end == b"ChecksumAlgorithm" { - if let Some(__tag_end) = - strings::index_of(&xml[i..], b"") - { - checksum_algorithm = Some(&xml[i..i + __tag_end]); - i = i + __tag_end + 20; - } else { - i = xml.len(); - } - } else if inner_tag_name_or_tag_end == b"ETag" { - if let Some(__tag_end) = - strings::index_of(&xml[i..], b"") - { - let input = &xml[i..i + __tag_end]; - - // unescape """ → "\"" - let output = - strings::replace_owned(input, b""", b"\""); - if output.len() != input.len() { - etag_owned = Some(output); - etag = None; // sentinel: owned path uses etag_owned - } else { - etag = Some(input); - } - - i = i + __tag_end + 7; - } else { - i = xml.len(); - } - } else if inner_tag_name_or_tag_end == b"Owner" { - if let Some(__tag_end) = - strings::index_of(&xml[i..], b"") - { - let owner = &xml[i..i + __tag_end]; - i = i + __tag_end + 8; - - if let Some(id_start) = strings::index_of(owner, b"") { - let id_start_pos = id_start + 4; - if let Some(id_end) = strings::index_of(owner, b"") - { - let is_not_empty = id_start_pos < id_end; - if is_not_empty { - owner_id = Some(&owner[id_start_pos..id_end]); - } - } - } - - if let Some(id_start) = - strings::index_of(owner, b"") - { - let id_start_pos = id_start + 13; - if let Some(id_end) = - strings::index_of(owner, b"") - { - let is_not_empty = id_start_pos < id_end; - if is_not_empty { - owner_display_name = - Some(&owner[id_start_pos..id_end]); - } - } - } - } else { - i = xml.len(); - } - } else if inner_tag_name_or_tag_end == b"RestoreStatus" { - if let Some(__tag_end) = - strings::index_of(&xml[i..], b"") - { - i = i + __tag_end + 16; - } else { - i = xml.len(); - } - } - } else { - i = xml.len(); - } - } else { - // char is not < - i += 1; - } - } - - if let Some(object_key_val) = object_key { - let mut owner: Option> = None; - - if owner_id.is_some() || owner_display_name.is_some() { - owner = Some(ObjectOwner { - id: owner_id, - display_name: owner_display_name, - }); - } - - contents.push(S3ListObjectsContents { - key: object_key_val, - etag: match (etag_owned, etag) { - (Some(owned), _) => Some(Cow::Owned(owned)), - (None, Some(borrowed)) => Some(Cow::Borrowed(borrowed)), - (None, None) => None, - }, - checksum_type, - checksum_algorithm, - last_modified, - object_size, - storage_class, - owner, - }); - } - } else if tag_name == b"Name" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - result.name = Some(&xml[i..i + _end]); - i += _end; - } else { - break; - } - } else if tag_name == b"Delimiter" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - result.delimiter = Some(&xml[i..i + _end]); - i += _end; - } else { - break; - } - } else if tag_name == b"NextContinuationToken" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - result.next_continuation_token = Some(&xml[i..i + _end]); - i += _end; - } else { - break; - } - } else if tag_name == b"ContinuationToken" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - result.continuation_token = Some(&xml[i..i + _end]); - i += _end; - } else { - break; - } - } else if tag_name == b"StartAfter" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - result.start_after = Some(&xml[i..i + _end]); - i += _end; - } else { - break; - } - } else if tag_name == b"EncodingType" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - result.encoding_type = Some(&xml[i..i + _end]); - i += _end; - } else { - break; - } - } else if tag_name == b"KeyCount" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - let key_count = &xml[i..i + _end]; - result.key_count = bun_core::fmt::parse_decimal::(key_count); - - i += _end; - } else { - break; - } - } else if tag_name == b"MaxKeys" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - let max_keys = &xml[i..i + _end]; - result.max_keys = bun_core::fmt::parse_decimal::(max_keys); - - i += _end; - } else { - break; - } - } else if tag_name == b"Prefix" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - let prefix = &xml[i..i + _end]; - - if !prefix.is_empty() { - result.prefix = Some(prefix); - } - - i += _end; - } else { - break; - } - } else if tag_name == b"IsTruncated" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - let is_truncated = &xml[i..i + _end]; - - if is_truncated == b"true" { - result.is_truncated = Some(true); - } else if is_truncated == b"false" { - result.is_truncated = Some(false); - } - - i += _end; - } else { - break; - } - } else if tag_name == b"CommonPrefixes" { - if let Some(_end) = strings::index_of(&xml[i..], b"") { - let common_prefixes_string = &xml[i..i + _end]; - i += _end; - - let mut j: usize = 0; - while j < common_prefixes_string.len() { - if let Some(start) = - strings::index_of(&common_prefixes_string[j..], b"") - { - j = j + start + 8; - - if let Some(__end) = - strings::index_of(&common_prefixes_string[j..], b"") - { - common_prefixes.push(&common_prefixes_string[j..j + __end]); - j += __end; - } else { - break; - } - } else { - break; - } - } - } else { - break; - } - } - } else { - break; - } - } - - if !contents.is_empty() { - result.contents = Some(contents); - } - // else branch: Vec drops itself - - if !common_prefixes.is_empty() { - result.common_prefixes = Some(common_prefixes); - } - // else branch: Vec drops itself + result.name = root.child_text(b"Name"); + result.prefix = root.child_text(b"Prefix").filter(|p| !p.is_empty()); + result.delimiter = root.child_text(b"Delimiter"); + result.start_after = root.child_text(b"StartAfter"); + result.encoding_type = root.child_text(b"EncodingType"); + result.continuation_token = root.child_text(b"ContinuationToken"); + result.next_continuation_token = root.child_text(b"NextContinuationToken"); + result.is_truncated = root.child_bool(b"IsTruncated"); + result.key_count = root.child_i64(b"KeyCount"); + result.max_keys = root.child_i64(b"MaxKeys"); + + let contents: Vec> = root + .children(b"Contents") + .filter_map(|object| { + Some(S3ListObjectsContents { + key: object.child_text(b"Key")?, + etag: object.child_text(b"ETag"), + checksum_type: object.child_text(b"ChecksumType"), + checksum_algorithm: object.child_text(b"ChecksumAlgorithm"), + last_modified: object.child_text(b"LastModified"), + object_size: object.child_i64(b"Size"), + storage_class: object.child_text(b"StorageClass"), + owner: object.child(b"Owner").and_then(|owner| { + let id = owner.child_text(b"ID").filter(|s| !s.is_empty()); + let display_name = owner.child_text(b"DisplayName").filter(|s| !s.is_empty()); + (id.is_some() || display_name.is_some()) + .then_some(ObjectOwner { id, display_name }) + }), + }) + }) + .collect(); + if !contents.is_empty() { + result.contents = Some(contents); } + let common_prefixes: Vec<&'a [u8]> = root + .children(b"CommonPrefixes") + .flat_map(|entry| entry.children(b"Prefix")) + .map(xml_response::Node::text) + .filter(|prefix| !prefix.is_empty()) + .collect(); + if !common_prefixes.is_empty() { + result.common_prefixes = Some(common_prefixes); + } result } diff --git a/src/runtime/webcore/s3/multipart.rs b/src/runtime/webcore/s3/multipart.rs index 429fac114065..21ad49f33d0d 100644 --- a/src/runtime/webcore/s3/multipart.rs +++ b/src/runtime/webcore/s3/multipart.rs @@ -97,7 +97,6 @@ use bstr::BStr; use bun_alloc::AllocError; use bun_collections::IntegerBitSet; -use bun_core::strings; use bun_core::{declare_scope, scoped_log}; use bun_io::KeepAlive; use bun_io::StreamBuffer; @@ -112,6 +111,7 @@ use bun_s3_signing::storage_class::StorageClass; // here is `crate::webcore`, not the `s3` directory. Route through the `s3` // re-export hub instead. use crate::webcore::s3::multipart_options::MultiPartUploadOptions; +use crate::webcore::s3::xml_response; use crate::webcore::s3::simple_request::{ self as s3_simple_request, S3CommitResult, S3DownloadResult, S3PartResult, S3UploadResult, execute_simple_s3_request, @@ -658,17 +658,15 @@ impl MultiPartUpload { S3DownloadResult::Success(response) => { // response.body is bun.MutableString — `list` is a Vec let slice = response.body.list.as_slice(); - // PERF: upload_id is duped out of the body instead of slicing into it - if let Some(start) = strings::index_of(slice, b"") { - let value_start = start + b"".len(); - if let Some(end) = strings::index_of(slice, b"") { - if end >= value_start { - self_ - .upload_id - .set(Box::<[u8]>::from(&slice[value_start..end])); - } + // + xml_response::with_document(slice, |document| { + if let Some(upload_id) = document + .filter(|root| root.name == b"InitiateMultipartUploadResult") + .and_then(|root| root.child_text(b"UploadId")) + { + self_.upload_id.set(Box::<[u8]>::from(upload_id)); } - } + }); let upload_id = self_.upload_id.get(); if upload_id.is_empty() || upload_id.len() > Self::MAX_UPLOAD_ID_LEN diff --git a/src/runtime/webcore/s3/simple_request.rs b/src/runtime/webcore/s3/simple_request.rs index 977c341ca2ff..24bc0af4b16d 100644 --- a/src/runtime/webcore/s3/simple_request.rs +++ b/src/runtime/webcore/s3/simple_request.rs @@ -2,7 +2,6 @@ use core::ffi::c_void; use core::sync::atomic::Ordering; use bun_core::MutableString; -use bun_core::strings; use bun_event_loop::ConcurrentTask::{AutoDeinit, ConcurrentTask}; use bun_event_loop::{TaskTag, Taskable, task_tag}; use bun_http::async_http::Options as HttpOptions; @@ -20,7 +19,7 @@ use bun_s3_signing::storage_class::StorageClass; use bun_threading::thread_pool; use bun_url::URL; -use crate::webcore::s3::list_objects; +use crate::webcore::s3::{list_objects, xml_response}; // The result/options structs below carry borrowed slices that are valid only for the // duration of the callback invocation (not owned; they must be copied if used @@ -215,7 +214,7 @@ impl S3HttpSimpleTask { fn error_with_body(&self, error_type: ErrorType) -> JsTerminatedResult<()> { let mut code: &[u8] = b"UnknownError"; - let mut message: &[u8] = b"an unexpected error has occurred"; + let message: &[u8] = b"an unexpected error has occurred"; let mut has_error_code = false; if let Some(err) = self.result.fail { code = err.name().as_bytes(); @@ -223,27 +222,27 @@ impl S3HttpSimpleTask { } else { let bytes = self.response_buffer.list.as_slice(); if !bytes.is_empty() { - message = bytes; - if let Some(start) = strings::index_of(bytes, b"") { - let value_start = start + b"".len(); - if let Some(end) = strings::index_of(bytes, b"") { - if end >= value_start { - code = &bytes[value_start..end]; - has_error_code = true; - } - } - } - if let Some(start) = strings::index_of(bytes, b"") { - let value_start = start + b"".len(); - if let Some(end) = strings::index_of(bytes, b"") { - if end >= value_start { - message = &bytes[value_start..end]; - } - } - } + return xml_response::with_error(bytes, |error| match error { + Some((body_code, body_message)) => self.finish_error( + error_type, + body_code.unwrap_or(code), + body_message.unwrap_or(bytes), + body_code.is_some(), + ), + None => self.finish_error(error_type, code, bytes, false), + }); } } + self.finish_error(error_type, code, message, has_error_code) + } + fn finish_error( + &self, + error_type: ErrorType, + mut code: &[u8], + mut message: &[u8], + has_error_code: bool, + ) -> JsTerminatedResult<()> { if error_type == ErrorType::NotFound { if !has_error_code { code = b"NoSuchKey"; @@ -257,43 +256,31 @@ impl S3HttpSimpleTask { Ok(()) } + /// A commit can answer 200 and still carry an `` document. fn fail_if_contains_error(&mut self, status: u32) -> JsTerminatedResult { - let mut code: &[u8] = b"UnknownError"; - let mut message: &[u8] = b"an unexpected error has occurred"; + let code: &[u8] = b"UnknownError"; + let message: &[u8] = b"an unexpected error has occurred"; if let Some(err) = self.result.fail { - code = err.name().as_bytes(); - } else { - let bytes = self.response_buffer.list.as_slice(); - let mut has_error = false; - if !bytes.is_empty() { - message = bytes; - if strings::index_of(bytes, b"").is_some() { - has_error = true; - if let Some(start) = strings::index_of(bytes, b"") { - let value_start = start + b"".len(); - if let Some(end) = strings::index_of(bytes, b"") { - if end >= value_start { - code = &bytes[value_start..end]; - } - } - } - if let Some(start) = strings::index_of(bytes, b"") { - let value_start = start + b"".len(); - if let Some(end) = strings::index_of(bytes, b"") { - if end >= value_start { - message = &bytes[value_start..end]; - } - } - } - } - } - if (!has_error && status == 200) || status == 206 { - return Ok(false); - } + self.callback + .fail(err.name().as_bytes(), message, self.callback_context)?; + return Ok(true); } - self.callback.fail(code, message, self.callback_context)?; - Ok(true) + let bytes = self.response_buffer.list.as_slice(); + xml_response::with_error(bytes, |error| { + let fallback_message = if bytes.is_empty() { message } else { bytes }; + let (code, message) = match error { + _ if status == 206 => return Ok(false), + None if status == 200 => return Ok(false), + None => (code, fallback_message), + Some((body_code, body_message)) => ( + body_code.unwrap_or(code), + body_message.unwrap_or(fallback_message), + ), + }; + self.callback.fail(code, message, self.callback_context)?; + Ok(true) + }) } /// this is the task callback from the last task result and is always in the main thread @@ -347,14 +334,26 @@ impl S3HttpSimpleTask { }, Callback::ListObjects(callback) => match response.status_code { 200 => { - // parse_s3_list_objects_result is infallible (alloc-only - // failure modes abort). - let success = list_objects::parse_s3_list_objects_result( + let context = this.callback_context; + xml_response::with_document( this.response_buffer.list.as_slice(), - ); - callback( - S3ListObjectsResult::Success(Box::new(success)), - this.callback_context, + |document| match document { + Some(root) if root.name == b"ListBucketResult" => { + let success = list_objects::parse_s3_list_objects_result(root); + callback(S3ListObjectsResult::Success(Box::new(success)), context) + } + // Half a listing is worse than none: S3 emits keys + // with control characters as (ill-formed) XML + // unless asked to URL-encode them. + _ => callback( + S3ListObjectsResult::Failure(S3Error { + code: b"InvalidResponse", + message: + b"ListObjectsV2 response is not a well-formed document (if keys can contain control characters, pass encodingType: \"url\")", + }), + context, + ), + }, )?; } 404 => this.error_with_body(ErrorType::NotFound)?, diff --git a/src/runtime/webcore/s3/xml_response.rs b/src/runtime/webcore/s3/xml_response.rs new file mode 100644 index 000000000000..f83ab6d6a6c0 --- /dev/null +++ b/src/runtime/webcore/s3/xml_response.rs @@ -0,0 +1,130 @@ +//! S3 answers in XML; its responses are read through the XML parser (the +//! `{ name, attributes, children }` node shape, whose text is exact). + +use bun_ast::E; +use bun_ast::expr::Data; +use bun_parsers::xml::{self, XML}; + +use crate::api::RecycledArena; + +/// One element of a parsed response. +#[derive(Clone, Copy)] +pub(crate) struct Node<'a> { + pub(crate) name: &'a [u8], + children: &'a [E::JsonValue], + arena: &'a bun_alloc::Arena, +} + +impl<'a> Node<'a> { + fn of(value: &'a E::JsonValue, arena: &'a bun_alloc::Arena) -> Option> { + let element = value.as_object()?; + Some(Node { + name: element.get(b"name")?.as_str()?, + children: element + .get(b"children") + .and_then(E::JsonValue::as_array) + .map_or(&[], E::ArrayJSON::items), + arena, + }) + } + + /// The child element `name` (the first, if repeated). + pub(crate) fn child(self, name: &[u8]) -> Option> { + self.children(name).next() + } + + /// Every child element called `name`, in document order. + pub(crate) fn children<'n>( + self, + name: &'n [u8], + ) -> impl Iterator> + use<'a, 'n> { + self.children + .iter() + .filter_map(move |child| Node::of(child, self.arena)) + .filter(move |child| child.name == name) + } + + /// The element's character data, exactly (entities and CDATA decoded, + /// whitespace kept); empty for an element with element children. + pub(crate) fn text(self) -> &'a [u8] { + match self.children { + [] => b"", + [only] => only.as_str().unwrap_or(b""), + // Character data interrupted by comments / PIs. + runs => { + use bun_alloc::ArenaVecExt as _; + let mut joined = bun_alloc::ArenaVec::new_in(self.arena); + for run in runs { + let Some(run) = run.as_str() else { + return b""; + }; + joined.extend_from_slice(run); + } + joined.into_bump_slice() + } + } + } + + pub(crate) fn child_text(self, name: &[u8]) -> Option<&'a [u8]> { + self.child(name).map(Node::text) + } + + pub(crate) fn child_i64(self, name: &[u8]) -> Option { + // All-ASCII-digit text is UTF-8. + core::str::from_utf8(self.child_text(name)?.trim_ascii()) + .ok()? + .parse() + .ok() + } + + pub(crate) fn child_bool(self, name: &[u8]) -> Option { + match self.child_text(name)?.trim_ascii() { + b"true" => Some(true), + b"false" => Some(false), + _ => None, + } + } +} + +/// Parses `body` and hands its root element to `f` (or `None` if it is not +/// a well-formed XML document). Everything a `Node` lends lives until `f` +/// returns. +pub(crate) fn with_document(body: &[u8], f: impl FnOnce(Option>) -> R) -> R { + if body.is_empty() { + return f(None); + } + let recycle = RecycledArena::take(); + let arena = recycle.arena(); + let mut ast_memory_allocator = bun_ast::ASTMemoryAllocator::borrowing(arena); + let _ast_scope = ast_memory_allocator.enter(); + let mut log = bun_ast::Log::init(); + let source = bun_ast::Source::init_path_string(b"response.xml", body); + let options = xml::Options { + compact: false, + encoding: xml::InputEncoding::Bytes, + }; + let root = match XML::parse(&source, &mut log, arena, options) { + Ok(bun_ast::Expr { + data: Data::EObjectJSON(root), + .. + }) => root, + _ => return f(None), + }; + let root = E::JsonValue::Object(root); + f(Node::of(&root, arena)) +} + +/// The `` and `` of an S3 `` document; `None` if the +/// body is not one. +#[allow(clippy::type_complexity)] +pub(crate) fn with_error( + body: &[u8], + f: impl FnOnce(Option<(Option<&[u8]>, Option<&[u8]>)>) -> R, +) -> R { + with_document(body, |root| match root { + Some(error) if error.name == b"Error" => { + f(Some((error.child_text(b"Code"), error.child_text(b"Message")))) + } + _ => f(None), + }) +} diff --git a/test/js/bun/s3/s3-list-objects.test.ts b/test/js/bun/s3/s3-list-objects.test.ts index 3bcd517ad474..61f7be97e40c 100644 --- a/test/js/bun/s3/s3-list-objects.test.ts +++ b/test/js/bun/s3/s3-list-objects.test.ts @@ -25,7 +25,7 @@ describe.concurrent("S3 - List Objects", () => { let reqUrl: string; using server = createBunServer(async req => { reqUrl = req.url; - return new Response(`<>`, { + return new Response(``, { headers: { "Content-Type": "application/xml", }, @@ -49,7 +49,7 @@ describe.concurrent("S3 - List Objects", () => { let reqUrl: string; using server = createBunServer(async req => { reqUrl = req.url; - return new Response(`<>`, { + return new Response(``, { headers: { "Content-Type": "application/xml", }, @@ -73,7 +73,7 @@ describe.concurrent("S3 - List Objects", () => { let reqUrl: string; using server = createBunServer(async req => { reqUrl = req.url; - return new Response(`<>`, { + return new Response(``, { headers: { "Content-Type": "application/xml", }, @@ -97,7 +97,7 @@ describe.concurrent("S3 - List Objects", () => { let reqUrl: string; using server = createBunServer(async req => { reqUrl = req.url; - return new Response(`<>`, { + return new Response(``, { headers: { "Content-Type": "application/xml", }, @@ -121,7 +121,7 @@ describe.concurrent("S3 - List Objects", () => { let reqUrl: string; using server = createBunServer(async req => { reqUrl = req.url; - return new Response(`<>`, { + return new Response(``, { headers: { "Content-Type": "application/xml", }, @@ -145,7 +145,7 @@ describe.concurrent("S3 - List Objects", () => { let reqUrl: string; using server = createBunServer(async req => { reqUrl = req.url; - return new Response(`<>`, { + return new Response(``, { headers: { "Content-Type": "application/xml", }, @@ -169,7 +169,7 @@ describe.concurrent("S3 - List Objects", () => { let reqUrl: string; using server = createBunServer(async req => { reqUrl = req.url; - return new Response(`<>`, { + return new Response(``, { headers: { "Content-Type": "application/xml", }, @@ -193,7 +193,7 @@ describe.concurrent("S3 - List Objects", () => { let reqUrl: string; using server = createBunServer(async req => { reqUrl = req.url; - return new Response(`<>`, { + return new Response(``, { headers: { "Content-Type": "application/xml", }, @@ -217,7 +217,7 @@ describe.concurrent("S3 - List Objects", () => { let reqUrl: string; using server = createBunServer(async req => { reqUrl = req.url; - return new Response(`<>`, { + return new Response(``, { headers: { "Content-Type": "application/xml", }, @@ -442,7 +442,7 @@ describe.concurrent("S3 - List Objects", () => { using server = createBunServer(async => { return new Response( ` - good@&/de$ + good@&/de$</limiter `, { headers: { @@ -775,7 +775,7 @@ describe.concurrent("S3 - List Objects", () => { / 0 10000 - awsome.dummy thing + awsome.<files>dummy thing</files> current pagination token url some next token @@ -928,7 +928,7 @@ describe.concurrent("S3 - List Objects", () => { }); }); - it("Should not crash with bad xml", async () => { + it("Should reject bad xml", async () => { using server = createBunServer(async => { return new Response( ` @@ -947,8 +947,81 @@ describe.concurrent("S3 - List Objects", () => { endpoint: server.url.href, }); - const res = await client.list(); - expect(res).toEqual({}); + // Not well-formed: rejected rather than half-listed. + const error = await client.list().then( + () => undefined, + e => e, + ); + expect(error?.code).toBe("InvalidResponse"); + }); + + it("Should decode XML-escaped text the way S3 sends it (keys, prefixes, error messages)", async () => { + // S3 escapes markup characters in every text node; a `&` in a key is a `&`. + let status = 200; + using server = createBunServer(async () => { + if (status !== 200) { + return new Response( + `NoSuch&KeyThe key "a&b<c>.txt" does not exista&b<c>.txt`, + { headers: { "Content-Type": "application/xml" }, status }, + ); + } + return new Response( + ` + + my_bucket + Tom & Jerry/ + 3 + false + + Tom & Jerry/<pilot> 🐱.mp4 + "etag-1" + 10 + true + R&Did-1 + + + .txt]]> + "etag-2" + 0 + + leading and trailing spaces are part of the key 7 + Tom & Jerry/S01/ + Tom & Jerry/S02/ + `, + { headers: { "Content-Type": "application/xml" }, status: 200 }, + ); + }); + + const client = new S3Client({ + ...options, + endpoint: server.url.href, + }); + + expect(await client.list()).toEqual({ + name: "my_bucket", + prefix: "Tom & Jerry/", + keyCount: 3, + isTruncated: false, + contents: [ + { + key: "Tom & Jerry/ \u{1F431}.mp4", + eTag: '"etag-1"', + size: 10, + owner: { displayName: "R&D", id: "id-1" }, + }, + { key: "Tom & Jerry/raw .txt", eTag: '"etag-2"', size: 0 }, + { key: " leading and trailing spaces are part of the key ", size: 7 }, + ], + commonPrefixes: [{ prefix: "Tom & Jerry/S01/" }, { prefix: "Tom & Jerry/S02/" }], + }); + + status = 404; + const error = await client.list().then( + () => undefined, + e => e, + ); + expect(error?.code).toBe("NoSuch&Key"); + expect(error?.message).toBe('The key "a&b.txt" does not exist'); }); it("Should throw Error if request failed", async () => { @@ -1174,7 +1247,7 @@ describe.skipIf(!optionsFromEnv.accessKeyId)("S3 - CI - List Objects", () => { }); }); -it("parses a large list response containing repeated unclosed Key tags quickly", async () => { +it("rejects a large malformed list response (repeated unclosed Key tags) quickly", async () => { // ListObjectsV2 body with a valid followed by ~5MB of opening tags // that never have a matching closing tag. const malformed = `my_bucket${Buffer.alloc(5_000_000, "").toString()}`; @@ -1194,14 +1267,14 @@ it("parses a large list response containing repeated unclosed Key tags quickly", }); const start = performance.now(); - const res = await client.list(); + const error = await client.list().then( + () => undefined, + e => e, + ); const elapsed = performance.now() - start; - // Fields parsed before the malformed section are still returned; the unterminated - // entries are ignored instead of producing bogus contents. - expect(res).toEqual({ - name: "my_bucket", - }); + // Not a well-formed document: rejected rather than half-listed. + expect(error?.code).toBe("InvalidResponse"); // Parsing must scale linearly with the response size. Even on slow debug/ASAN builds // a single 5MB response should be handled in well under 10 seconds. From d1e3efba2a86feaae34c2a5c926a48b907c78af8 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 8 Aug 2026 13:00:34 +0000 Subject: [PATCH 02/11] [autofix.ci] apply automated fixes --- src/runtime/webcore/s3/list_objects.rs | 1 - src/runtime/webcore/s3/multipart.rs | 2 +- src/runtime/webcore/s3/simple_request.rs | 4 +++- src/runtime/webcore/s3/xml_response.rs | 7 ++++--- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/runtime/webcore/s3/list_objects.rs b/src/runtime/webcore/s3/list_objects.rs index 0dae1192007d..df4c6ffda04f 100644 --- a/src/runtime/webcore/s3/list_objects.rs +++ b/src/runtime/webcore/s3/list_objects.rs @@ -55,7 +55,6 @@ pub struct S3ListObjectsV2Result<'a> { pub(crate) contents: Option>>, } - impl<'a> S3ListObjectsV2Result<'a> { pub(crate) fn to_js(&self, global_object: &JSGlobalObject) -> JsResult { let js_result = JSValue::create_empty_object(global_object, 0); diff --git a/src/runtime/webcore/s3/multipart.rs b/src/runtime/webcore/s3/multipart.rs index 21ad49f33d0d..e1c83100204d 100644 --- a/src/runtime/webcore/s3/multipart.rs +++ b/src/runtime/webcore/s3/multipart.rs @@ -111,11 +111,11 @@ use bun_s3_signing::storage_class::StorageClass; // here is `crate::webcore`, not the `s3` directory. Route through the `s3` // re-export hub instead. use crate::webcore::s3::multipart_options::MultiPartUploadOptions; -use crate::webcore::s3::xml_response; use crate::webcore::s3::simple_request::{ self as s3_simple_request, S3CommitResult, S3DownloadResult, S3PartResult, S3UploadResult, execute_simple_s3_request, }; +use crate::webcore::s3::xml_response; type JsTerminatedResult = Result; diff --git a/src/runtime/webcore/s3/simple_request.rs b/src/runtime/webcore/s3/simple_request.rs index 24bc0af4b16d..79cb520bff13 100644 --- a/src/runtime/webcore/s3/simple_request.rs +++ b/src/runtime/webcore/s3/simple_request.rs @@ -337,7 +337,8 @@ impl S3HttpSimpleTask { let context = this.callback_context; xml_response::with_document( this.response_buffer.list.as_slice(), - |document| match document { + |document| { + match document { Some(root) if root.name == b"ListBucketResult" => { let success = list_objects::parse_s3_list_objects_result(root); callback(S3ListObjectsResult::Success(Box::new(success)), context) @@ -353,6 +354,7 @@ impl S3HttpSimpleTask { }), context, ), + } }, )?; } diff --git a/src/runtime/webcore/s3/xml_response.rs b/src/runtime/webcore/s3/xml_response.rs index f83ab6d6a6c0..3e1be91c73b7 100644 --- a/src/runtime/webcore/s3/xml_response.rs +++ b/src/runtime/webcore/s3/xml_response.rs @@ -122,9 +122,10 @@ pub(crate) fn with_error( f: impl FnOnce(Option<(Option<&[u8]>, Option<&[u8]>)>) -> R, ) -> R { with_document(body, |root| match root { - Some(error) if error.name == b"Error" => { - f(Some((error.child_text(b"Code"), error.child_text(b"Message")))) - } + Some(error) if error.name == b"Error" => f(Some(( + error.child_text(b"Code"), + error.child_text(b"Message"), + ))), _ => f(None), }) } From 3c07de62ffb7aecc253616fd41795e282cf9eb75 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sat, 8 Aug 2026 13:18:20 +0000 Subject: [PATCH 03/11] s3/xml_response: skip bodies past the parser's 32-bit positions; treat empty / as absent so the NoSuchKey / body fallbacks still apply; test --- src/runtime/webcore/s3/xml_response.rs | 11 ++++++----- test/js/bun/s3/s3-list-objects.test.ts | 12 ++++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/runtime/webcore/s3/xml_response.rs b/src/runtime/webcore/s3/xml_response.rs index 3e1be91c73b7..a0f5b4e2ef60 100644 --- a/src/runtime/webcore/s3/xml_response.rs +++ b/src/runtime/webcore/s3/xml_response.rs @@ -90,7 +90,8 @@ impl<'a> Node<'a> { /// a well-formed XML document). Everything a `Node` lends lives until `f` /// returns. pub(crate) fn with_document(body: &[u8], f: impl FnOnce(Option>) -> R) -> R { - if body.is_empty() { + // The parser's positions are 32-bit. + if body.is_empty() || body.len() > i32::MAX as usize { return f(None); } let recycle = RecycledArena::take(); @@ -114,8 +115,8 @@ pub(crate) fn with_document(body: &[u8], f: impl FnOnce(Option>) -> f(Node::of(&root, arena)) } -/// The `` and `` of an S3 `` document; `None` if the -/// body is not one. +/// The (non-empty) `` and `` of an S3 `` document; +/// `None` if the body is not one. #[allow(clippy::type_complexity)] pub(crate) fn with_error( body: &[u8], @@ -123,8 +124,8 @@ pub(crate) fn with_error( ) -> R { with_document(body, |root| match root { Some(error) if error.name == b"Error" => f(Some(( - error.child_text(b"Code"), - error.child_text(b"Message"), + error.child_text(b"Code").filter(|code| !code.is_empty()), + error.child_text(b"Message").filter(|message| !message.is_empty()), ))), _ => f(None), }) diff --git a/test/js/bun/s3/s3-list-objects.test.ts b/test/js/bun/s3/s3-list-objects.test.ts index 61f7be97e40c..c5ea5650fdb6 100644 --- a/test/js/bun/s3/s3-list-objects.test.ts +++ b/test/js/bun/s3/s3-list-objects.test.ts @@ -1024,6 +1024,18 @@ describe.concurrent("S3 - List Objects", () => { expect(error?.message).toBe('The key "a&b.txt" does not exist'); }); + it("Should fall back to NoSuchKey for a 404 whose has no usable ", async () => { + for (const body of [``, `gone`, `not xml`]) { + using server = createBunServer(async () => new Response(body, { status: 404 })); + const client = new S3Client({ ...options, endpoint: server.url.href }); + const error = await client.list().then( + () => undefined, + e => e, + ); + expect(error?.code).toBe("NoSuchKey"); + } + }); + it("Should throw Error if request failed", async () => { using server = createBunServer(async => { return new Response( From 1d08cd3ec2ddbbd112fbba4ce5981df14347a970 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 8 Aug 2026 13:39:59 +0000 Subject: [PATCH 04/11] [autofix.ci] apply automated fixes --- src/runtime/webcore/s3/xml_response.rs | 4 +++- test/js/bun/s3/s3-list-objects.test.ts | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/runtime/webcore/s3/xml_response.rs b/src/runtime/webcore/s3/xml_response.rs index a0f5b4e2ef60..05a780b3c22d 100644 --- a/src/runtime/webcore/s3/xml_response.rs +++ b/src/runtime/webcore/s3/xml_response.rs @@ -125,7 +125,9 @@ pub(crate) fn with_error( with_document(body, |root| match root { Some(error) if error.name == b"Error" => f(Some(( error.child_text(b"Code").filter(|code| !code.is_empty()), - error.child_text(b"Message").filter(|message| !message.is_empty()), + error + .child_text(b"Message") + .filter(|message| !message.is_empty()), ))), _ => f(None), }) diff --git a/test/js/bun/s3/s3-list-objects.test.ts b/test/js/bun/s3/s3-list-objects.test.ts index c5ea5650fdb6..3060611f180b 100644 --- a/test/js/bun/s3/s3-list-objects.test.ts +++ b/test/js/bun/s3/s3-list-objects.test.ts @@ -1025,7 +1025,11 @@ describe.concurrent("S3 - List Objects", () => { }); it("Should fall back to NoSuchKey for a 404 whose has no usable ", async () => { - for (const body of [``, `gone`, `not xml`]) { + for (const body of [ + ``, + `gone`, + `not xml`, + ]) { using server = createBunServer(async () => new Response(body, { status: 404 })); const client = new S3Client({ ...options, endpoint: server.url.href }); const error = await client.list().then( From 824052b4ff8ece1ffb0a42db482c6def54b170a0 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sat, 8 Aug 2026 14:46:39 +0000 Subject: [PATCH 05/11] s3: tolerate the keep-alive whitespace CompleteMultipartUpload streams ahead of its (possibly ) document; validate the UploadId before storing it; tests --- src/runtime/webcore/s3/multipart.rs | 26 +++++++++++++++----------- src/runtime/webcore/s3/xml_response.rs | 4 ++++ test/js/bun/s3/s3-list-objects.test.ts | 16 ++++++++++++++++ 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/runtime/webcore/s3/multipart.rs b/src/runtime/webcore/s3/multipart.rs index e1c83100204d..7c35a399f255 100644 --- a/src/runtime/webcore/s3/multipart.rs +++ b/src/runtime/webcore/s3/multipart.rs @@ -659,21 +659,25 @@ impl MultiPartUpload { // response.body is bun.MutableString — `list` is a Vec let slice = response.body.list.as_slice(); // - xml_response::with_document(slice, |document| { - if let Some(upload_id) = document + let valid = xml_response::with_document(slice, |document| { + match document .filter(|root| root.name == b"InitiateMultipartUploadResult") .and_then(|root| root.child_text(b"UploadId")) { - self_.upload_id.set(Box::<[u8]>::from(upload_id)); + Some(upload_id) + if !upload_id.is_empty() + && upload_id.len() <= Self::MAX_UPLOAD_ID_LEN + && upload_id + .iter() + .all(|b| b.is_ascii() && !b.is_ascii_control()) => + { + self_.upload_id.set(Box::<[u8]>::from(upload_id)); + true + } + _ => false, } }); - let upload_id = self_.upload_id.get(); - if upload_id.is_empty() - || upload_id.len() > Self::MAX_UPLOAD_ID_LEN - || upload_id - .iter() - .any(|b| !b.is_ascii() || b.is_ascii_control()) - { + if !valid { // Unknown type of response error from AWS scoped_log!( S3MultiPartUpload, @@ -690,7 +694,7 @@ impl MultiPartUpload { S3MultiPartUpload, "startMultiPartRequestResult {} success id: {}", BStr::new(&self_.path), - BStr::new(upload_id) + BStr::new(self_.upload_id.get()) ); self_.state.set(State::MultipartCompleted); // start draining the parts diff --git a/src/runtime/webcore/s3/xml_response.rs b/src/runtime/webcore/s3/xml_response.rs index 05a780b3c22d..9cb6cf57e78d 100644 --- a/src/runtime/webcore/s3/xml_response.rs +++ b/src/runtime/webcore/s3/xml_response.rs @@ -90,6 +90,10 @@ impl<'a> Node<'a> { /// a well-formed XML document). Everything a `Node` lends lives until `f` /// returns. pub(crate) fn with_document(body: &[u8], f: impl FnOnce(Option>) -> R) -> R { + // `CompleteMultipartUpload` streams keep-alive whitespace ahead of the + // document (even ahead of an `` on a 200), which XML proper does + // not allow before the declaration. + let body = body.trim_ascii_start(); // The parser's positions are 32-bit. if body.is_empty() || body.len() > i32::MAX as usize { return f(None); diff --git a/test/js/bun/s3/s3-list-objects.test.ts b/test/js/bun/s3/s3-list-objects.test.ts index 3060611f180b..600a179aff53 100644 --- a/test/js/bun/s3/s3-list-objects.test.ts +++ b/test/js/bun/s3/s3-list-objects.test.ts @@ -1024,6 +1024,22 @@ describe.concurrent("S3 - List Objects", () => { expect(error?.message).toBe('The key "a&b.txt" does not exist'); }); + it("Should read an that follows keep-alive whitespace (CompleteMultipartUpload style)", async () => { + using server = createBunServer( + async () => + new Response(` \n\n\nInternalErrortry again`, { + status: 500, + }), + ); + const client = new S3Client({ ...options, endpoint: server.url.href }); + const error = await client.list().then( + () => undefined, + e => e, + ); + expect(error?.code).toBe("InternalError"); + expect(error?.message).toBe("try again"); + }); + it("Should fall back to NoSuchKey for a 404 whose has no usable ", async () => { for (const body of [ ``, From d914d49941e8bfb9209ac290654ade4b8a62c9d3 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 8 Aug 2026 15:24:17 +0000 Subject: [PATCH 06/11] [autofix.ci] apply automated fixes --- test/js/bun/s3/s3-list-objects.test.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/js/bun/s3/s3-list-objects.test.ts b/test/js/bun/s3/s3-list-objects.test.ts index 600a179aff53..3c9b6a278e01 100644 --- a/test/js/bun/s3/s3-list-objects.test.ts +++ b/test/js/bun/s3/s3-list-objects.test.ts @@ -1027,9 +1027,12 @@ describe.concurrent("S3 - List Objects", () => { it("Should read an that follows keep-alive whitespace (CompleteMultipartUpload style)", async () => { using server = createBunServer( async () => - new Response(` \n\n\nInternalErrortry again`, { - status: 500, - }), + new Response( + ` \n\n\nInternalErrortry again`, + { + status: 500, + }, + ), ); const client = new S3Client({ ...options, endpoint: server.url.href }); const error = await client.list().then( From 93ec7a94f60df97780e25e45e59a24ae57b5b76e Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sat, 8 Aug 2026 21:56:43 +0000 Subject: [PATCH 07/11] s3: parse into a plain per-call arena and copy the few strings out into owned results (no thread-local arena, no borrowed lifetimes through the callbacks); api.rs back to unchanged --- src/runtime/api.rs | 51 +++--- src/runtime/webcore/s3/download_stream.rs | 36 +++-- src/runtime/webcore/s3/list_objects.rs | 188 ++++++++++------------ src/runtime/webcore/s3/multipart.rs | 31 ++-- src/runtime/webcore/s3/simple_request.rs | 109 ++++++------- src/runtime/webcore/s3/xml_response.rs | 97 ++++++----- 6 files changed, 229 insertions(+), 283 deletions(-) diff --git a/src/runtime/api.rs b/src/runtime/api.rs index 26f0f00c467a..76bf14fbf80a 100644 --- a/src/runtime/api.rs +++ b/src/runtime/api.rs @@ -248,37 +248,6 @@ enum SourceEncoding { Utf16Text, } -/// The calling thread's scratch arena for parsing one document, handed back -/// (reset, keeping up to 2 MiB) on drop. A private mi_heap costs microseconds -/// to create — more than parsing a small document — so one is kept per thread. -/// `#[thread_local]` rather than `thread_local!` so there is no destructor -/// racing mimalloc's own thread teardown (as in `ast_memory_allocator.rs`); a -/// parked heap is reclaimed with the thread. Re-entrant use just gets a fresh -/// arena. -pub(crate) struct RecycledArena(Option); - -#[thread_local] -static PARKED_ARENA: core::cell::Cell> = core::cell::Cell::new(None); - -impl RecycledArena { - pub(crate) fn take() -> Self { - Self(Some(PARKED_ARENA.take().unwrap_or_default())) - } - - pub(crate) fn arena(&self) -> &bun_alloc::Arena { - self.0.as_ref().expect("live until drop") - } -} - -impl Drop for RecycledArena { - fn drop(&mut self) { - if let Some(mut arena) = self.0.take() { - arena.reset_retain_with_limit(2 * 1024 * 1024); - PARKED_ARENA.set(Some(arena)); - } - } -} - fn with_text_format_source_encoded( global: &bun_jsc::JSGlobalObject, frame: &bun_jsc::CallFrame, @@ -295,8 +264,24 @@ fn with_text_format_source_encoded( ) -> bun_jsc::JsResult { use crate::node::{BlobOrStringOrBuffer, StringOrBuffer}; - let recycle = RecycledArena::take(); - let arena = recycle.arena(); + // A private mi_heap costs microseconds to create, more than parsing a + // small document: keep one per thread and recycle it between calls. + // `#[thread_local]` rather than `thread_local!` so there is no + // destructor racing mimalloc's own thread teardown (as in + // `ast_memory_allocator.rs`); a parked heap is reclaimed with the thread. + #[thread_local] + static ARENA: core::cell::Cell> = core::cell::Cell::new(None); + struct Recycle(Option); + impl Drop for Recycle { + fn drop(&mut self) { + if let Some(mut arena) = self.0.take() { + arena.reset_retain_with_limit(2 * 1024 * 1024); + ARENA.set(Some(arena)); + } + } + } + let recycle = Recycle(Some(ARENA.take().unwrap_or_default())); + let arena = recycle.0.as_ref().expect("set above"); let mut ast_memory_allocator = bun_ast::ASTMemoryAllocator::borrowing(arena); let _ast_scope = ast_memory_allocator.enter(); diff --git a/src/runtime/webcore/s3/download_stream.rs b/src/runtime/webcore/s3/download_stream.rs index 1f744679a94b..a283c6341922 100644 --- a/src/runtime/webcore/s3/download_stream.rs +++ b/src/runtime/webcore/s3/download_stream.rs @@ -85,25 +85,29 @@ impl S3HttpDownloadStreamingTask { if has_more { return; } - let callback = self.callback; - let context = self.callback_context.as_ptr().cast(); let empty = MutableString::default(); - let message: &[u8] = b"an unexpected error has occurred"; + let mut code: &[u8] = b"UnknownError"; + let mut message: &[u8] = b"an unexpected error has occurred"; + let parsed; if let Some(req_err) = self.request_error { - let code = req_err.name().as_bytes(); - callback(&empty, false, Some(S3Error { code, message }), context); - return; + code = req_err.name().as_bytes(); + } else { + let bytes = self.reported_response_buffer.list.as_slice(); + if !bytes.is_empty() { + message = bytes; + } + parsed = xml_response::parse_error(bytes); + if let Some(error) = &parsed { + code = error.code.as_deref().unwrap_or(code); + message = error.message.as_deref().unwrap_or(message); + } } - // `code` / `message` borrow the parsed body; the callback consumes - // them before it returns. - let bytes = self.reported_response_buffer.list.as_slice(); - xml_response::with_error(bytes, |error| { - let (code, body_message) = error.unwrap_or((None, None)); - let code = code.unwrap_or(b"UnknownError"); - let message = - body_message.unwrap_or(if bytes.is_empty() { message } else { bytes }); - callback(&empty, false, Some(S3Error { code, message }), context); - }); + (self.callback)( + &empty, + false, + Some(S3Error { code, message }), + self.callback_context.as_ptr().cast(), + ); return; } diff --git a/src/runtime/webcore/s3/list_objects.rs b/src/runtime/webcore/s3/list_objects.rs index df4c6ffda04f..197280356b9f 100644 --- a/src/runtime/webcore/s3/list_objects.rs +++ b/src/runtime/webcore/s3/list_objects.rs @@ -20,59 +20,56 @@ pub struct S3ListObjectsOptions { // Each Utf8Slice field cleans up via Drop, so no explicit `impl Drop` is // needed here. -// The result structs borrow from the parsed response document handed to -// `parse_s3_list_objects_result`; the caller consumes them (toJS) inside -// that document's scope. - -struct ObjectOwner<'a> { - id: Option<&'a [u8]>, - display_name: Option<&'a [u8]>, +struct ObjectOwner { + id: Option>, + display_name: Option>, } -pub struct S3ListObjectsContents<'a> { - key: &'a [u8], - etag: Option<&'a [u8]>, - checksum_type: Option<&'a [u8]>, - checksum_algorithm: Option<&'a [u8]>, - last_modified: Option<&'a [u8]>, +pub struct S3ListObjectsContents { + key: Box<[u8]>, + etag: Option>, + checksum_type: Option>, + checksum_algorithm: Option>, + last_modified: Option>, object_size: Option, - storage_class: Option<&'a [u8]>, - owner: Option>, + storage_class: Option>, + owner: Option, } -pub struct S3ListObjectsV2Result<'a> { - pub name: Option<&'a [u8]>, - pub(crate) prefix: Option<&'a [u8]>, +#[derive(Default)] +pub struct S3ListObjectsV2Result { + pub name: Option>, + pub(crate) prefix: Option>, pub(crate) key_count: Option, pub(crate) max_keys: Option, - pub(crate) delimiter: Option<&'a [u8]>, - pub(crate) encoding_type: Option<&'a [u8]>, + pub(crate) delimiter: Option>, + pub(crate) encoding_type: Option>, pub(crate) is_truncated: Option, - pub(crate) continuation_token: Option<&'a [u8]>, - pub(crate) next_continuation_token: Option<&'a [u8]>, - pub(crate) start_after: Option<&'a [u8]>, - pub(crate) common_prefixes: Option>, - pub(crate) contents: Option>>, + pub(crate) continuation_token: Option>, + pub(crate) next_continuation_token: Option>, + pub(crate) start_after: Option>, + pub(crate) common_prefixes: Option>>, + pub(crate) contents: Option>, } -impl<'a> S3ListObjectsV2Result<'a> { +impl S3ListObjectsV2Result { pub(crate) fn to_js(&self, global_object: &JSGlobalObject) -> JsResult { let js_result = JSValue::create_empty_object(global_object, 0); - js_result.put_optional_utf8(global_object, b"name", self.name)?; - js_result.put_optional_utf8(global_object, b"prefix", self.prefix)?; - js_result.put_optional_utf8(global_object, b"delimiter", self.delimiter)?; - js_result.put_optional_utf8(global_object, b"startAfter", self.start_after)?; - js_result.put_optional_utf8(global_object, b"encodingType", self.encoding_type)?; + js_result.put_optional_utf8(global_object, b"name", self.name.as_deref())?; + js_result.put_optional_utf8(global_object, b"prefix", self.prefix.as_deref())?; + js_result.put_optional_utf8(global_object, b"delimiter", self.delimiter.as_deref())?; + js_result.put_optional_utf8(global_object, b"startAfter", self.start_after.as_deref())?; + js_result.put_optional_utf8(global_object, b"encodingType", self.encoding_type.as_deref())?; js_result.put_optional_utf8( global_object, b"continuationToken", - self.continuation_token, + self.continuation_token.as_deref(), )?; js_result.put_optional_utf8( global_object, b"nextContinuationToken", - self.next_continuation_token, + self.next_continuation_token.as_deref(), )?; js_result.put_optional(global_object, b"isTruncated", self.is_truncated); js_result.put_optional(global_object, b"keyCount", self.key_count.map(|n| n as f64)); @@ -86,11 +83,11 @@ impl<'a> S3ListObjectsV2Result<'a> { object_info.put( global_object, b"key", - create_utf8_for_js(global_object, item.key)?, + create_utf8_for_js(global_object, &item.key)?, ); - object_info.put_optional_utf8(global_object, b"eTag", item.etag)?; - if let Some(algorithm) = item.checksum_algorithm { + object_info.put_optional_utf8(global_object, b"eTag", item.etag.as_deref())?; + if let Some(algorithm) = item.checksum_algorithm.as_deref() { let js_algorithm = create_utf8_for_js(global_object, algorithm)?; object_info.put(global_object, b"checksumAlgorithm", js_algorithm); // Back-compat alias for the original misspelling (#19142). @@ -103,12 +100,12 @@ impl<'a> S3ListObjectsV2Result<'a> { object_info.put_optional_utf8( global_object, b"checksumType", - item.checksum_type, + item.checksum_type.as_deref(), )?; object_info.put_optional_utf8( global_object, b"lastModified", - item.last_modified, + item.last_modified.as_deref(), )?; object_info.put_optional( global_object, @@ -118,16 +115,16 @@ impl<'a> S3ListObjectsV2Result<'a> { object_info.put_optional_utf8( global_object, b"storageClass", - item.storage_class, + item.storage_class.as_deref(), )?; if let Some(owner) = &item.owner { let js_owner = JSValue::create_empty_object(global_object, 0); - js_owner.put_optional_utf8(global_object, b"id", owner.id)?; + js_owner.put_optional_utf8(global_object, b"id", owner.id.as_deref())?; js_owner.put_optional_utf8( global_object, b"displayName", - owner.display_name, + owner.display_name.as_deref(), )?; object_info.put(global_object, b"owner", js_owner); } @@ -167,70 +164,55 @@ impl<'a> S3ListObjectsV2Result<'a> { } } -/// Reads a `ListObjectsV2` response's ``; the result -/// borrows from it. -pub(crate) fn parse_s3_list_objects_result<'a>( - root: xml_response::Node<'a>, -) -> S3ListObjectsV2Result<'a> { - let mut result = S3ListObjectsV2Result { - contents: None, - common_prefixes: None, - continuation_token: None, - delimiter: None, - encoding_type: None, - is_truncated: None, - key_count: None, - max_keys: None, - name: None, - next_continuation_token: None, - prefix: None, - start_after: None, - }; - result.name = root.child_text(b"Name"); - result.prefix = root.child_text(b"Prefix").filter(|p| !p.is_empty()); - result.delimiter = root.child_text(b"Delimiter"); - result.start_after = root.child_text(b"StartAfter"); - result.encoding_type = root.child_text(b"EncodingType"); - result.continuation_token = root.child_text(b"ContinuationToken"); - result.next_continuation_token = root.child_text(b"NextContinuationToken"); - result.is_truncated = root.child_bool(b"IsTruncated"); - result.key_count = root.child_i64(b"KeyCount"); - result.max_keys = root.child_i64(b"MaxKeys"); - - let contents: Vec> = root - .children(b"Contents") - .filter_map(|object| { - Some(S3ListObjectsContents { - key: object.child_text(b"Key")?, - etag: object.child_text(b"ETag"), - checksum_type: object.child_text(b"ChecksumType"), - checksum_algorithm: object.child_text(b"ChecksumAlgorithm"), - last_modified: object.child_text(b"LastModified"), - object_size: object.child_i64(b"Size"), - storage_class: object.child_text(b"StorageClass"), - owner: object.child(b"Owner").and_then(|owner| { - let id = owner.child_text(b"ID").filter(|s| !s.is_empty()); - let display_name = owner.child_text(b"DisplayName").filter(|s| !s.is_empty()); - (id.is_some() || display_name.is_some()) - .then_some(ObjectOwner { id, display_name }) - }), +/// Reads a `ListObjectsV2` response body; `None` unless it is a well-formed +/// `` document. +pub(crate) fn parse_s3_list_objects_result(body: &[u8]) -> Option { + xml_response::parse(body, |root| { + if root.name != b"ListBucketResult" { + return None; + } + let contents: Vec = root + .children(b"Contents") + .filter_map(|object| { + Some(S3ListObjectsContents { + key: object.child_text(b"Key")?, + etag: object.child_text(b"ETag"), + checksum_type: object.child_text(b"ChecksumType"), + checksum_algorithm: object.child_text(b"ChecksumAlgorithm"), + last_modified: object.child_text(b"LastModified"), + object_size: object.child_i64(b"Size"), + storage_class: object.child_text(b"StorageClass"), + owner: object.child(b"Owner").and_then(|owner| { + let id = owner.child_nonempty_text(b"ID"); + let display_name = owner.child_nonempty_text(b"DisplayName"); + (id.is_some() || display_name.is_some()) + .then_some(ObjectOwner { id, display_name }) + }), + }) }) + .collect(); + let common_prefixes: Vec> = root + .children(b"CommonPrefixes") + .flat_map(|entry| entry.children(b"Prefix")) + .filter_map(xml_response::Node::text) + .filter(|prefix| !prefix.is_empty()) + .collect(); + Some(S3ListObjectsV2Result { + name: root.child_text(b"Name"), + prefix: root.child_nonempty_text(b"Prefix"), + key_count: root.child_i64(b"KeyCount"), + max_keys: root.child_i64(b"MaxKeys"), + delimiter: root.child_text(b"Delimiter"), + encoding_type: root.child_text(b"EncodingType"), + is_truncated: root.child_bool(b"IsTruncated"), + continuation_token: root.child_text(b"ContinuationToken"), + next_continuation_token: root.child_text(b"NextContinuationToken"), + start_after: root.child_text(b"StartAfter"), + common_prefixes: (!common_prefixes.is_empty()).then_some(common_prefixes), + contents: (!contents.is_empty()).then_some(contents), }) - .collect(); - if !contents.is_empty() { - result.contents = Some(contents); - } - - let common_prefixes: Vec<&'a [u8]> = root - .children(b"CommonPrefixes") - .flat_map(|entry| entry.children(b"Prefix")) - .map(xml_response::Node::text) - .filter(|prefix| !prefix.is_empty()) - .collect(); - if !common_prefixes.is_empty() { - result.common_prefixes = Some(common_prefixes); - } - result + }) + .flatten() } pub(crate) fn get_list_objects_options_from_js( diff --git a/src/runtime/webcore/s3/multipart.rs b/src/runtime/webcore/s3/multipart.rs index 7c35a399f255..8bb8a8b73c43 100644 --- a/src/runtime/webcore/s3/multipart.rs +++ b/src/runtime/webcore/s3/multipart.rs @@ -659,24 +659,21 @@ impl MultiPartUpload { // response.body is bun.MutableString — `list` is a Vec let slice = response.body.list.as_slice(); // - let valid = xml_response::with_document(slice, |document| { - match document - .filter(|root| root.name == b"InitiateMultipartUploadResult") - .and_then(|root| root.child_text(b"UploadId")) - { - Some(upload_id) - if !upload_id.is_empty() - && upload_id.len() <= Self::MAX_UPLOAD_ID_LEN - && upload_id - .iter() - .all(|b| b.is_ascii() && !b.is_ascii_control()) => - { - self_.upload_id.set(Box::<[u8]>::from(upload_id)); - true - } - _ => false, - } + let upload_id = xml_response::parse(slice, |root| { + (root.name == b"InitiateMultipartUploadResult") + .then(|| root.child_text(b"UploadId")) + .flatten() + }) + .flatten() + .filter(|id| { + !id.is_empty() + && id.len() <= Self::MAX_UPLOAD_ID_LEN + && id.iter().all(|b| b.is_ascii() && !b.is_ascii_control()) }); + let valid = upload_id.is_some(); + if let Some(upload_id) = upload_id { + self_.upload_id.set(upload_id); + } if !valid { // Unknown type of response error from AWS scoped_log!( diff --git a/src/runtime/webcore/s3/simple_request.rs b/src/runtime/webcore/s3/simple_request.rs index 79cb520bff13..11144952b0e6 100644 --- a/src/runtime/webcore/s3/simple_request.rs +++ b/src/runtime/webcore/s3/simple_request.rs @@ -85,7 +85,7 @@ pub enum S3DeleteResult<'a> { } pub enum S3ListObjectsResult<'a> { - Success(Box>), + Success(Box), NotFound(S3Error<'a>), /// failure error is not owned and need to be copied if used after this callback Failure(S3Error<'a>), @@ -214,35 +214,29 @@ impl S3HttpSimpleTask { fn error_with_body(&self, error_type: ErrorType) -> JsTerminatedResult<()> { let mut code: &[u8] = b"UnknownError"; - let message: &[u8] = b"an unexpected error has occurred"; + let mut message: &[u8] = b"an unexpected error has occurred"; let mut has_error_code = false; + let parsed; if let Some(err) = self.result.fail { code = err.name().as_bytes(); has_error_code = true; } else { let bytes = self.response_buffer.list.as_slice(); if !bytes.is_empty() { - return xml_response::with_error(bytes, |error| match error { - Some((body_code, body_message)) => self.finish_error( - error_type, - body_code.unwrap_or(code), - body_message.unwrap_or(bytes), - body_code.is_some(), - ), - None => self.finish_error(error_type, code, bytes, false), - }); + message = bytes; + parsed = xml_response::parse_error(bytes); + if let Some(error) = &parsed { + if let Some(body_code) = error.code.as_deref() { + code = body_code; + has_error_code = true; + } + if let Some(body_message) = error.message.as_deref() { + message = body_message; + } + } } } - self.finish_error(error_type, code, message, has_error_code) - } - fn finish_error( - &self, - error_type: ErrorType, - mut code: &[u8], - mut message: &[u8], - has_error_code: bool, - ) -> JsTerminatedResult<()> { if error_type == ErrorType::NotFound { if !has_error_code { code = b"NoSuchKey"; @@ -258,29 +252,27 @@ impl S3HttpSimpleTask { /// A commit can answer 200 and still carry an `` document. fn fail_if_contains_error(&mut self, status: u32) -> JsTerminatedResult { - let code: &[u8] = b"UnknownError"; - let message: &[u8] = b"an unexpected error has occurred"; - + let mut code: &[u8] = b"UnknownError"; + let mut message: &[u8] = b"an unexpected error has occurred"; + let parsed; if let Some(err) = self.result.fail { - self.callback - .fail(err.name().as_bytes(), message, self.callback_context)?; - return Ok(true); + code = err.name().as_bytes(); + } else { + let bytes = self.response_buffer.list.as_slice(); + if !bytes.is_empty() { + message = bytes; + } + parsed = xml_response::parse_error(bytes); + if let Some(error) = &parsed { + code = error.code.as_deref().unwrap_or(code); + message = error.message.as_deref().unwrap_or(message); + } + if (parsed.is_none() && status == 200) || status == 206 { + return Ok(false); + } } - let bytes = self.response_buffer.list.as_slice(); - xml_response::with_error(bytes, |error| { - let fallback_message = if bytes.is_empty() { message } else { bytes }; - let (code, message) = match error { - _ if status == 206 => return Ok(false), - None if status == 200 => return Ok(false), - None => (code, fallback_message), - Some((body_code, body_message)) => ( - body_code.unwrap_or(code), - body_message.unwrap_or(fallback_message), - ), - }; - self.callback.fail(code, message, self.callback_context)?; - Ok(true) - }) + self.callback.fail(code, message, self.callback_context)?; + Ok(true) } /// this is the task callback from the last task result and is always in the main thread @@ -334,29 +326,18 @@ impl S3HttpSimpleTask { }, Callback::ListObjects(callback) => match response.status_code { 200 => { - let context = this.callback_context; - xml_response::with_document( - this.response_buffer.list.as_slice(), - |document| { - match document { - Some(root) if root.name == b"ListBucketResult" => { - let success = list_objects::parse_s3_list_objects_result(root); - callback(S3ListObjectsResult::Success(Box::new(success)), context) - } - // Half a listing is worse than none: S3 emits keys - // with control characters as (ill-formed) XML - // unless asked to URL-encode them. - _ => callback( - S3ListObjectsResult::Failure(S3Error { - code: b"InvalidResponse", - message: - b"ListObjectsV2 response is not a well-formed document (if keys can contain control characters, pass encodingType: \"url\")", - }), - context, - ), - } - }, - )?; + let body = this.response_buffer.list.as_slice(); + let result = match list_objects::parse_s3_list_objects_result(body) { + Some(listing) => S3ListObjectsResult::Success(Box::new(listing)), + // Half a listing is worse than none: S3 emits keys + // with control characters as (ill-formed) XML + // unless asked to URL-encode them. + None => S3ListObjectsResult::Failure(S3Error { + code: b"InvalidResponse", + message: b"ListObjectsV2 response is not a well-formed document (if keys can contain control characters, pass encodingType: \"url\")", + }), + }; + callback(result, this.callback_context)?; } 404 => this.error_with_body(ErrorType::NotFound)?, _ => this.error_with_body(ErrorType::Failure)?, diff --git a/src/runtime/webcore/s3/xml_response.rs b/src/runtime/webcore/s3/xml_response.rs index 9cb6cf57e78d..4fbb2e38f0eb 100644 --- a/src/runtime/webcore/s3/xml_response.rs +++ b/src/runtime/webcore/s3/xml_response.rs @@ -1,22 +1,20 @@ //! S3 answers in XML; its responses are read through the XML parser (the -//! `{ name, attributes, children }` node shape, whose text is exact). +//! `{ name, attributes, children }` node shape, whose text is exact) and the +//! few strings wanted are copied out. use bun_ast::E; use bun_ast::expr::Data; use bun_parsers::xml::{self, XML}; -use crate::api::RecycledArena; - /// One element of a parsed response. #[derive(Clone, Copy)] pub(crate) struct Node<'a> { pub(crate) name: &'a [u8], children: &'a [E::JsonValue], - arena: &'a bun_alloc::Arena, } impl<'a> Node<'a> { - fn of(value: &'a E::JsonValue, arena: &'a bun_alloc::Arena) -> Option> { + fn of(value: &'a E::JsonValue) -> Option> { let element = value.as_object()?; Some(Node { name: element.get(b"name")?.as_str()?, @@ -24,7 +22,6 @@ impl<'a> Node<'a> { .get(b"children") .and_then(E::JsonValue::as_array) .map_or(&[], E::ArrayJSON::items), - arena, }) } @@ -40,33 +37,35 @@ impl<'a> Node<'a> { ) -> impl Iterator> + use<'a, 'n> { self.children .iter() - .filter_map(move |child| Node::of(child, self.arena)) + .filter_map(Node::of) .filter(move |child| child.name == name) } /// The element's character data, exactly (entities and CDATA decoded, - /// whitespace kept); empty for an element with element children. - pub(crate) fn text(self) -> &'a [u8] { + /// whitespace kept), copied out; `None` for an element with element + /// children. + pub(crate) fn text(self) -> Option> { match self.children { - [] => b"", - [only] => only.as_str().unwrap_or(b""), + [] => Some(Box::default()), + [only] => only.as_str().map(Box::from), // Character data interrupted by comments / PIs. runs => { - use bun_alloc::ArenaVecExt as _; - let mut joined = bun_alloc::ArenaVec::new_in(self.arena); + let mut joined = Vec::new(); for run in runs { - let Some(run) = run.as_str() else { - return b""; - }; - joined.extend_from_slice(run); + joined.extend_from_slice(run.as_str()?); } - joined.into_bump_slice() + Some(joined.into_boxed_slice()) } } } - pub(crate) fn child_text(self, name: &[u8]) -> Option<&'a [u8]> { - self.child(name).map(Node::text) + pub(crate) fn child_text(self, name: &[u8]) -> Option> { + self.child(name)?.text() + } + + /// `child_text`, but an empty element counts as absent. + pub(crate) fn child_nonempty_text(self, name: &[u8]) -> Option> { + self.child_text(name).filter(|text| !text.is_empty()) } pub(crate) fn child_i64(self, name: &[u8]) -> Option { @@ -86,21 +85,20 @@ impl<'a> Node<'a> { } } -/// Parses `body` and hands its root element to `f` (or `None` if it is not -/// a well-formed XML document). Everything a `Node` lends lives until `f` -/// returns. -pub(crate) fn with_document(body: &[u8], f: impl FnOnce(Option>) -> R) -> R { +/// Parses `body` and maps its root element through `read`; `None` if it is +/// not a well-formed XML document. The parse lives in a throwaway arena, so +/// `read` copies out what it keeps. +pub(crate) fn parse(body: &[u8], read: impl FnOnce(Node<'_>) -> R) -> Option { // `CompleteMultipartUpload` streams keep-alive whitespace ahead of the // document (even ahead of an `` on a 200), which XML proper does // not allow before the declaration. let body = body.trim_ascii_start(); // The parser's positions are 32-bit. if body.is_empty() || body.len() > i32::MAX as usize { - return f(None); + return None; } - let recycle = RecycledArena::take(); - let arena = recycle.arena(); - let mut ast_memory_allocator = bun_ast::ASTMemoryAllocator::borrowing(arena); + let arena = bun_alloc::Arena::default(); + let mut ast_memory_allocator = bun_ast::ASTMemoryAllocator::borrowing(&arena); let _ast_scope = ast_memory_allocator.enter(); let mut log = bun_ast::Log::init(); let source = bun_ast::Source::init_path_string(b"response.xml", body); @@ -108,31 +106,30 @@ pub(crate) fn with_document(body: &[u8], f: impl FnOnce(Option>) -> compact: false, encoding: xml::InputEncoding::Bytes, }; - let root = match XML::parse(&source, &mut log, arena, options) { - Ok(bun_ast::Expr { - data: Data::EObjectJSON(root), - .. - }) => root, - _ => return f(None), + let Ok(bun_ast::Expr { + data: Data::EObjectJSON(root), + .. + }) = XML::parse(&source, &mut log, &arena, options) + else { + return None; }; let root = E::JsonValue::Object(root); - f(Node::of(&root, arena)) + Node::of(&root).map(read) } -/// The (non-empty) `` and `` of an S3 `` document; -/// `None` if the body is not one. -#[allow(clippy::type_complexity)] -pub(crate) fn with_error( - body: &[u8], - f: impl FnOnce(Option<(Option<&[u8]>, Option<&[u8]>)>) -> R, -) -> R { - with_document(body, |root| match root { - Some(error) if error.name == b"Error" => f(Some(( - error.child_text(b"Code").filter(|code| !code.is_empty()), - error - .child_text(b"Message") - .filter(|message| !message.is_empty()), - ))), - _ => f(None), +/// The `` and `` (each if present and non-empty) of an S3 +/// `` document; `None` if the body is not one. +pub(crate) struct ErrorBody { + pub code: Option>, + pub message: Option>, +} + +pub(crate) fn parse_error(body: &[u8]) -> Option { + parse(body, |root| { + (root.name == b"Error").then(|| ErrorBody { + code: root.child_nonempty_text(b"Code"), + message: root.child_nonempty_text(b"Message"), + }) }) + .flatten() } From 9972319573f5e68d647d7e64cae79e42b68dd1ab Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 8 Aug 2026 22:00:09 +0000 Subject: [PATCH 08/11] [autofix.ci] apply automated fixes --- src/runtime/webcore/s3/list_objects.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/runtime/webcore/s3/list_objects.rs b/src/runtime/webcore/s3/list_objects.rs index 197280356b9f..d6035b030dfb 100644 --- a/src/runtime/webcore/s3/list_objects.rs +++ b/src/runtime/webcore/s3/list_objects.rs @@ -60,7 +60,11 @@ impl S3ListObjectsV2Result { js_result.put_optional_utf8(global_object, b"prefix", self.prefix.as_deref())?; js_result.put_optional_utf8(global_object, b"delimiter", self.delimiter.as_deref())?; js_result.put_optional_utf8(global_object, b"startAfter", self.start_after.as_deref())?; - js_result.put_optional_utf8(global_object, b"encodingType", self.encoding_type.as_deref())?; + js_result.put_optional_utf8( + global_object, + b"encodingType", + self.encoding_type.as_deref(), + )?; js_result.put_optional_utf8( global_object, b"continuationToken", From 4b462bb3fe3a040d8cb6680b8791c9a706b15df4 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sat, 8 Aug 2026 22:14:03 +0000 Subject: [PATCH 09/11] s3: a without a makes the whole listing invalid (all-or-nothing); UploadId must be printable ASCII with nothing that ends or splits a query value; test --- src/runtime/webcore/s3/list_objects.rs | 5 +++-- src/runtime/webcore/s3/multipart.rs | 6 +++++- test/js/bun/s3/s3-list-objects.test.ts | 13 +++++++++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/runtime/webcore/s3/list_objects.rs b/src/runtime/webcore/s3/list_objects.rs index d6035b030dfb..0c3fe86db7ac 100644 --- a/src/runtime/webcore/s3/list_objects.rs +++ b/src/runtime/webcore/s3/list_objects.rs @@ -175,9 +175,10 @@ pub(crate) fn parse_s3_list_objects_result(body: &[u8]) -> Option` names a key, or the listing is not one. let contents: Vec = root .children(b"Contents") - .filter_map(|object| { + .map(|object| { Some(S3ListObjectsContents { key: object.child_text(b"Key")?, etag: object.child_text(b"ETag"), @@ -194,7 +195,7 @@ pub(crate) fn parse_s3_list_objects_result(body: &[u8]) -> Option>()?; let common_prefixes: Vec> = root .children(b"CommonPrefixes") .flat_map(|entry| entry.children(b"Prefix")) diff --git a/src/runtime/webcore/s3/multipart.rs b/src/runtime/webcore/s3/multipart.rs index 8bb8a8b73c43..bf1d1cecc253 100644 --- a/src/runtime/webcore/s3/multipart.rs +++ b/src/runtime/webcore/s3/multipart.rs @@ -665,10 +665,14 @@ impl MultiPartUpload { .flatten() }) .flatten() + // It goes into query strings as is: printable, and nothing + // that would end or split a query value. .filter(|id| { !id.is_empty() && id.len() <= Self::MAX_UPLOAD_ID_LEN - && id.iter().all(|b| b.is_ascii() && !b.is_ascii_control()) + && id + .iter() + .all(|&b| b.is_ascii_graphic() && !matches!(b, b'&' | b'#' | b'?')) }); let valid = upload_id.is_some(); if let Some(upload_id) = upload_id { diff --git a/test/js/bun/s3/s3-list-objects.test.ts b/test/js/bun/s3/s3-list-objects.test.ts index 3c9b6a278e01..beeb5e4e0fb7 100644 --- a/test/js/bun/s3/s3-list-objects.test.ts +++ b/test/js/bun/s3/s3-list-objects.test.ts @@ -1043,6 +1043,19 @@ describe.concurrent("S3 - List Objects", () => { expect(error?.message).toBe("try again"); }); + it("Should reject a listing whose has no ", async () => { + using server = createBunServer( + async () => + new Response(`ba1`), + ); + const client = new S3Client({ ...options, endpoint: server.url.href }); + const error = await client.list().then( + () => undefined, + e => e, + ); + expect(error?.code).toBe("InvalidResponse"); + }); + it("Should fall back to NoSuchKey for a 404 whose has no usable ", async () => { for (const body of [ ``, From 4059a6a685b66c0bfe4151dce7079876afc9f0fe Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Sat, 8 Aug 2026 22:17:15 +0000 Subject: [PATCH 10/11] [autofix.ci] apply automated fixes --- test/js/bun/s3/s3-list-objects.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/js/bun/s3/s3-list-objects.test.ts b/test/js/bun/s3/s3-list-objects.test.ts index beeb5e4e0fb7..c7e45939816d 100644 --- a/test/js/bun/s3/s3-list-objects.test.ts +++ b/test/js/bun/s3/s3-list-objects.test.ts @@ -1046,7 +1046,9 @@ describe.concurrent("S3 - List Objects", () => { it("Should reject a listing whose has no ", async () => { using server = createBunServer( async () => - new Response(`ba1`), + new Response( + `ba1`, + ), ); const client = new S3Client({ ...options, endpoint: server.url.href }); const error = await client.list().then( From f769a49f01711269b8e6bcfd79579492a3b655f5 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sat, 8 Aug 2026 22:51:35 +0000 Subject: [PATCH 11/11] doc: xml_parse_inc is bumped by the API entry points No-Verification-Needed: comment only --- src/bun_core/Global.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bun_core/Global.rs b/src/bun_core/Global.rs index 56cff6a2b82d..56a912aec7f0 100644 --- a/src/bun_core/Global.rs +++ b/src/bun_core/Global.rs @@ -386,7 +386,8 @@ pub mod features { pub fn yaml_parse_inc() { YAML_PARSE.fetch_add(1, core::sync::atomic::Ordering::Relaxed); } - /// parsers crate calls `bun_core::analytics::Features::xml_parse_inc()`. + /// Bumped by the `Bun.XML` API and `.xml` imports (not by internal users + /// of the parser, such as the S3 client). #[inline] pub fn xml_parse_inc() { XML_PARSE.fetch_add(1, core::sync::atomic::Ordering::Relaxed);