From f7a15cbd43385b16b30deb5fa30e1f2576de05ca Mon Sep 17 00:00:00 2001 From: zarzet <42882290+zarzet@users.noreply.github.com> Date: Mon, 19 Oct 2026 00:19:06 +0700 Subject: [PATCH] refactor(rust): consolidate small duplicate helpers Keep one copy of helpers that were implemented several times, each proven equivalent to the copies it replaces by a test that runs the removed implementation (kept inline in the test module) against the survivor: - Go-style lexical path clean: new `core::fs::clean`, used by `isrc::native_files` and re-exported as `extensions::files::clean`. - Dart `String.trim` whitespace: `path_keys::dart_whitespace` and `dart_trim` now back the lyrics parser's explicit character list and the playlist importer's private `trim`; the test compares every Unicode scalar value. - `normalized()` (Dart trim then simple lowercase): the native backup importer uses the identical `native_library::normalized`. - Lowercase hex encoding: `spotiflac_core::lower_hex` replaces the per-byte `format!` collectors in data_jobs and network_tags, the signed-session `hex` and the `hex` arm of `binary::encode`; checked for all 256 bytes. - ID3 syncsafe decode: the tag writer's `size` reuses the reader's `tags::id3::syncsafe`; checked across the 0x7F/0x80 boundaries per byte. - Bounded body reads: `HttpStream::read_limited` replaces the read-to-limit loops of the lyrics HTTP client, the resolver HTTP client and the extension registry reader. Callers keep their buffer sizes (16 KiB, 16 KiB, 64 KiB), their own error mapping and their messages; a loopback test compares it with the previous loop at and around the limit. The mobile ID3 `synchsafe` is not merged: it validates the length and the high bits and returns `None`, unlike the core decoder. --- rust_backend/crates/core/src/fs.rs | 85 +++++++++++++++++++ .../crates/core/src/isrc/native_files.rs | 21 +---- rust_backend/crates/core/src/lib.rs | 3 +- rust_backend/crates/core/src/tags/id3.rs | 2 +- .../crates/core/src/tags/write/id3.rs | 40 ++++++++- rust_backend/crates/core/src/text.rs | 25 ++++++ rust_backend/crates/extensions/src/binary.rs | 9 +- rust_backend/crates/extensions/src/files.rs | 23 +---- .../crates/extensions/src/repository.rs | 22 ++--- .../extensions/src/signed_session/protocol.rs | 10 +-- rust_backend/crates/mobile/src/data_jobs.rs | 6 +- .../crates/mobile/src/native_backup.rs | 9 +- .../crates/mobile/src/native_library.rs | 2 +- .../mobile/src/native_library/path_keys.rs | 54 +++++++++++- .../crates/mobile/src/native_lyrics/text.rs | 25 +----- .../crates/mobile/src/native_playlists.rs | 5 +- .../crates/mobile/src/network_tags.rs | 13 +-- .../crates/network/src/client/stream.rs | 24 ++++++ .../crates/network/src/client/tests.rs | 75 ++++++++++++++++ .../crates/providers/src/lyrics/http.rs | 21 +---- .../crates/providers/src/resolver/http.rs | 21 +---- 21 files changed, 332 insertions(+), 163 deletions(-) create mode 100644 rust_backend/crates/core/src/fs.rs diff --git a/rust_backend/crates/core/src/fs.rs b/rust_backend/crates/core/src/fs.rs new file mode 100644 index 00000000..05d9525f --- /dev/null +++ b/rust_backend/crates/core/src/fs.rs @@ -0,0 +1,85 @@ +//! Filesystem path helpers shared by the native and extension backends. + +use std::path::{Component, Path, PathBuf}; + +/// Lexically cleans a path like Go's `filepath.Clean`: drops `.` components, +/// folds `name/..`, keeps leading `..` on relative paths, drops `..` above the +/// root, and returns `.` for an empty result. The filesystem is not consulted. +pub fn clean(path: &Path) -> PathBuf { + let mut result = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => { + if result.file_name().is_some_and(|name| name != "..") { + result.pop(); + } else if !result.has_root() { + result.push(".."); + } + } + _ => result.push(component), + } + } + if result.as_os_str().is_empty() { + ".".into() + } else { + result + } +} + +#[cfg(test)] +mod tests { + use super::*; + + // The copy `isrc::native_files` used before it shared this helper. + fn previous_isrc_clean(path: &Path) -> PathBuf { + let mut output = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir if output.file_name().is_some_and(|name| name != "..") => { + output.pop(); + } + Component::ParentDir if output.has_root() => {} + other => output.push(other.as_os_str()), + } + } + if output.as_os_str().is_empty() { + output.push("."); + } + output + } + + #[test] + fn clean_matches_previous_isrc_copy_and_go_results() { + let cases = [ + ("", "."), + (".", "."), + ("./", "."), + ("..", ".."), + ("../..", "../.."), + ("../../a", "../../a"), + ("a/..", "."), + ("a/../..", ".."), + ("a/b/../../..", ".."), + ("a/./b/", "a/b"), + ("a//b///c", "a/b/c"), + ("a/b/", "a/b"), + ("/", "/"), + ("/..", "/"), + ("/../a", "/a"), + ("/a/b/../c/./d/", "/a/c/d"), + ("/a/..", "/"), + ("//a", "/a"), + ("a/...", "a/..."), + ("a/.../..", "a"), + ("..a/../b", "b"), + ("\u{00e9}/\u{1f3b5}/../x", "\u{00e9}/x"), + ]; + for (input, expected) in cases { + let input = Path::new(input); + assert_eq!(clean(input), Path::new(expected), "{input:?}"); + assert_eq!(clean(input), previous_isrc_clean(input), "{input:?}"); + } + } +} diff --git a/rust_backend/crates/core/src/isrc/native_files.rs b/rust_backend/crates/core/src/isrc/native_files.rs index 44b25c69..0eb4896e 100644 --- a/rust_backend/crates/core/src/isrc/native_files.rs +++ b/rust_backend/crates/core/src/isrc/native_files.rs @@ -1,6 +1,7 @@ use super::{FileStamp, IndexFiles}; +use crate::fs::clean; use std::fs::{self, OpenOptions}; -use std::path::{Component, Path, PathBuf}; +use std::path::{Path, PathBuf}; use std::time::UNIX_EPOCH; /// Ambient filesystem access for the trusted native application. JavaScript @@ -90,21 +91,3 @@ fn stamp(path: &Path, metadata: &fs::Metadata) -> FileStamp { directory: metadata.is_dir(), } } - -fn clean(path: &Path) -> PathBuf { - let mut output = PathBuf::new(); - for component in path.components() { - match component { - Component::CurDir => {} - Component::ParentDir if output.file_name().is_some_and(|name| name != "..") => { - output.pop(); - } - Component::ParentDir if output.has_root() => {} - other => output.push(other.as_os_str()), - } - } - if output.as_os_str().is_empty() { - output.push("."); - } - output -} diff --git a/rust_backend/crates/core/src/lib.rs b/rust_backend/crates/core/src/lib.rs index f00f1fd0..dbc5a78b 100644 --- a/rust_backend/crates/core/src/lib.rs +++ b/rust_backend/crates/core/src/lib.rs @@ -11,6 +11,7 @@ pub mod cover; pub mod cue; pub mod downloads; pub mod filename; +pub mod fs; pub mod isrc; pub mod json; pub mod lyrics; @@ -24,4 +25,4 @@ pub mod resolver; pub mod tags; mod text; pub mod xml; -pub use text::json_surrogates as normalize_json_surrogates; +pub use text::{json_surrogates as normalize_json_surrogates, lower_hex}; diff --git a/rust_backend/crates/core/src/tags/id3.rs b/rust_backend/crates/core/src/tags/id3.rs index f0560701..a5591693 100644 --- a/rust_backend/crates/core/src/tags/id3.rs +++ b/rust_backend/crates/core/src/tags/id3.rs @@ -501,7 +501,7 @@ fn v1(reader: &mut (impl Read + Seek)) -> Result { }) } -fn syncsafe(data: &[u8]) -> u64 { +pub(crate) fn syncsafe(data: &[u8]) -> u64 { u64::from(data[0]) << 21 | u64::from(data[1]) << 14 | u64::from(data[2]) << 7 diff --git a/rust_backend/crates/core/src/tags/write/id3.rs b/rust_backend/crates/core/src/tags/write/id3.rs index 8c565671..c285aa50 100644 --- a/rust_backend/crates/core/src/tags/write/id3.rs +++ b/rust_backend/crates/core/src/tags/write/id3.rs @@ -211,10 +211,7 @@ fn syncsafe(size: usize) -> [u8; 4] { fn size(data: &[u8], syncsafe: bool) -> usize { if syncsafe { - ((data[0] as usize) << 21) - | ((data[1] as usize) << 14) - | ((data[2] as usize) << 7) - | data[3] as usize + crate::tags::id3::syncsafe(data) as usize } else { u32::from_be_bytes(data.try_into().unwrap()) as usize } @@ -379,3 +376,38 @@ fn read_frames( } Ok((frames, audio_start)) } + +#[cfg(test)] +mod tests { + use super::*; + + // The decoder `size` inlined before it shared the reader's `syncsafe`. + fn previous_syncsafe_size(data: &[u8]) -> usize { + ((data[0] as usize) << 21) + | ((data[1] as usize) << 14) + | ((data[2] as usize) << 7) + | data[3] as usize + } + + #[test] + fn syncsafe_size_matches_previous_decoder_across_bit_boundaries() { + assert_eq!(size(&[0, 0, 0, 0], true), 0); + assert_eq!(size(&[0, 0, 0, 0x7f], true), 0x7f); + assert_eq!(size(&[0, 0, 1, 0], true), 0x80); + assert_eq!(size(&[0x7f, 0x7f, 0x7f, 0x7f], true), 0x0fff_ffff); + assert_eq!(size(&[0, 0, 0, 0x80], true), 0x80); + assert_eq!(size(&[0xff, 0xff, 0xff, 0xff], true), 0x1fff_ffff); + assert_eq!(size(&[0, 0, 1, 0, 9], true), 0x80); + let values = [0x00, 0x01, 0x7f, 0x80, 0xff]; + for a in values { + for b in values { + for c in values { + for d in values { + let data = [a, b, c, d]; + assert_eq!(size(&data, true), previous_syncsafe_size(&data), "{data:?}"); + } + } + } + } + } +} diff --git a/rust_backend/crates/core/src/text.rs b/rust_backend/crates/core/src/text.rs index fc390b84..e0c939dc 100644 --- a/rust_backend/crates/core/src/text.rs +++ b/rust_backend/crates/core/src/text.rs @@ -67,3 +67,28 @@ pub fn json_surrogates(json: &str) -> std::borrow::Cow<'_, str> { std::borrow::Cow::Owned(output) } } + +/// Lowercase hexadecimal, two digits per byte (Go's `hex.EncodeToString`). +pub fn lower_hex(bytes: &[u8]) -> String { + use std::fmt::Write; + let mut output = String::with_capacity(bytes.len() * 2); + for byte in bytes { + let _ = write!(output, "{byte:02x}"); + } + output +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn lower_hex_matches_per_byte_format_for_every_byte() { + assert_eq!(lower_hex(&[]), ""); + assert_eq!(lower_hex(&[0x00, 0x0f, 0x7f, 0x80, 0xff]), "000f7f80ff"); + let every: Vec = (0..=u8::MAX).collect(); + // The `format!` per byte form used by the mobile digest helpers. + let previous: String = every.iter().map(|byte| format!("{byte:02x}")).collect(); + assert_eq!(lower_hex(&every), previous); + } +} diff --git a/rust_backend/crates/extensions/src/binary.rs b/rust_backend/crates/extensions/src/binary.rs index 4e55bb3a..ec47a1cf 100644 --- a/rust_backend/crates/extensions/src/binary.rs +++ b/rust_backend/crates/extensions/src/binary.rs @@ -139,14 +139,7 @@ pub(crate) fn decode_option_bytes( pub(crate) fn encode(bytes: &[u8], encoding: &str) -> Result { match encoding.trim().to_lowercase().as_str() { "" | "base64" => Ok(STANDARD.encode(bytes)), - "hex" => { - use std::fmt::Write; - let mut value = String::with_capacity(bytes.len() * 2); - for byte in bytes { - let _ = write!(value, "{byte:02x}"); - } - Ok(value) - } + "hex" => Ok(spotiflac_core::lower_hex(bytes)), "utf8" | "utf-8" | "text" => Ok(decode_go_utf8(bytes)), _ => Err(format!("unsupported byte encoding: {encoding}")), } diff --git a/rust_backend/crates/extensions/src/files.rs b/rust_backend/crates/extensions/src/files.rs index a1e508dd..7fbe5c9f 100644 --- a/rust_backend/crates/extensions/src/files.rs +++ b/rust_backend/crates/extensions/src/files.rs @@ -4,6 +4,7 @@ mod index; use cap_std::ambient_authority; use cap_std::fs::{Dir, File, OpenOptions}; +pub(crate) use spotiflac_core::fs::clean; use std::collections::BTreeMap; use std::io; use std::path::{Component, Path, PathBuf}; @@ -679,27 +680,5 @@ impl Drop for OutputGuard { } } -pub(crate) fn clean(path: &Path) -> PathBuf { - let mut result = PathBuf::new(); - for component in path.components() { - match component { - Component::CurDir => {} - Component::ParentDir => { - if result.file_name().is_some_and(|name| name != "..") { - result.pop(); - } else if !result.has_root() { - result.push(".."); - } - } - _ => result.push(component), - } - } - if result.as_os_str().is_empty() { - ".".into() - } else { - result - } -} - #[cfg(test)] mod tests; diff --git a/rust_backend/crates/extensions/src/repository.rs b/rust_backend/crates/extensions/src/repository.rs index b70318c5..1094b6c9 100644 --- a/rust_backend/crates/extensions/src/repository.rs +++ b/rust_backend/crates/extensions/src/repository.rs @@ -500,22 +500,14 @@ fn read_registry( let status = stream.response.status; let mut body = Vec::new(); if status == 200 { - let mut buffer = [0; 64 * 1024]; - loop { - let limit = buffer.len().min(MAX_REGISTRY_BYTES + 1 - body.len()); - let count = stream - .read(&mut buffer[..limit], check) - .map_err(|e| cause("failed to read registry", e))?; - if count == 0 { - break; - } - body.extend_from_slice(&buffer[..count]); - if body.len() > MAX_REGISTRY_BYTES { - return Err(error(format!( + body = stream + .read_limited(MAX_REGISTRY_BYTES, &mut [0; 64 * 1024], check) + .map_err(|e| cause("failed to read registry", e))? + .ok_or_else(|| { + error(format!( "registry response exceeds {MAX_REGISTRY_BYTES} bytes" - ))); - } - } + )) + })?; } Ok((status, stream.response.headers, body)) } diff --git a/rust_backend/crates/extensions/src/signed_session/protocol.rs b/rust_backend/crates/extensions/src/signed_session/protocol.rs index 7b5626c8..6672f888 100644 --- a/rust_backend/crates/extensions/src/signed_session/protocol.rs +++ b/rust_backend/crates/extensions/src/signed_session/protocol.rs @@ -4,6 +4,7 @@ use hmac::{Hmac, KeyInit, Mac}; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; use sha2::{Digest, Sha256}; +use spotiflac_core::lower_hex as hex; use spotiflac_network::{HttpResponse, query, url::UrlParts}; use std::collections::BTreeMap; use std::time::{Duration, UNIX_EPOCH}; @@ -374,15 +375,6 @@ pub fn random_hex(count: usize) -> Result { Ok(hex(&bytes)) } -fn hex(bytes: &[u8]) -> String { - use std::fmt::Write; - let mut output = String::with_capacity(bytes.len() * 2); - for byte in bytes { - let _ = write!(output, "{byte:02x}"); - } - output -} - fn hmac(key: &[u8], message: &[u8]) -> String { let mut mac = Hmac::::new_from_slice(key).expect("HMAC accepts any key length"); mac.update(message); diff --git a/rust_backend/crates/mobile/src/data_jobs.rs b/rust_backend/crates/mobile/src/data_jobs.rs index a515dff0..106e872d 100644 --- a/rust_backend/crates/mobile/src/data_jobs.rs +++ b/rust_backend/crates/mobile/src/data_jobs.rs @@ -179,11 +179,7 @@ pub(crate) fn hash_file( hash.update(&buffer[..count]); } check()?; - Ok(hash - .finalize() - .iter() - .map(|byte| format!("{byte:02x}")) - .collect()) + Ok(spotiflac_core::lower_hex(&hash.finalize())) } #[cfg(test)] diff --git a/rust_backend/crates/mobile/src/native_backup.rs b/rust_backend/crates/mobile/src/native_backup.rs index 027a89bc..a83e04d2 100644 --- a/rust_backend/crates/mobile/src/native_backup.rs +++ b/rust_backend/crates/mobile/src/native_backup.rs @@ -4,10 +4,8 @@ mod archive; mod collections; mod legacy; -use crate::native_library::dart_date; -use crate::native_library::path_keys::{ - build, dart_lower, dart_trim, dart_upper, regex_whitespace, -}; +use crate::native_library::path_keys::{build, dart_trim, dart_upper, regex_whitespace}; +use crate::native_library::{dart_date, normalized}; use rusqlite::{Connection, OpenFlags, TransactionBehavior, params, params_from_iter}; use serde_json::{Map, Value, json}; use std::{ @@ -140,9 +138,6 @@ fn number(value: &Value) -> Option { .as_i64() .or_else(|| value.as_f64().filter(|v| v.is_finite()).map(|v| v as i64)) } -fn normalized(value: &str) -> String { - dart_lower(dart_trim(value)) -} fn date(value: &str) -> Option<(String, i64)> { let parsed = dart_date::parse(value)?; Some(( diff --git a/rust_backend/crates/mobile/src/native_library.rs b/rust_backend/crates/mobile/src/native_library.rs index ec2b76c7..7c23a0fd 100644 --- a/rust_backend/crates/mobile/src/native_library.rs +++ b/rust_backend/crates/mobile/src/native_library.rs @@ -159,7 +159,7 @@ pub(crate) fn scan_incremental( import(request, Some(&delta), check) } -fn normalized(value: &str) -> String { +pub(crate) fn normalized(value: &str) -> String { path_keys::dart_lower(path_keys::dart_trim(value)) } diff --git a/rust_backend/crates/mobile/src/native_library/path_keys.rs b/rust_backend/crates/mobile/src/native_library/path_keys.rs index 67c9315c..611e178e 100644 --- a/rust_backend/crates/mobile/src/native_library/path_keys.rs +++ b/rust_backend/crates/mobile/src/native_library/path_keys.rs @@ -15,8 +15,13 @@ const EXTENSIONS: &[&str] = &[ ".flac", ".m4a", ".mp3", ".opus", ".ogg", ".wav", ".aiff", ".aif", ".aac", ]; +/// Dart's `String.trim` set: Unicode White_Space plus the U+FEFF BOM. +pub(crate) fn dart_whitespace(ch: char) -> bool { + ch.is_whitespace() || ch == '\u{feff}' +} + pub(crate) fn dart_trim(value: &str) -> &str { - value.trim_matches(|ch: char| ch.is_whitespace() || ch == '\u{feff}') + value.trim_matches(dart_whitespace) } /// Dart applies Unicode simple casing, without multi-character expansions or @@ -328,4 +333,51 @@ mod tests { .any(|key| key == "/storage/emulated/0/Music/Café.flac") ); } + + // The explicit set `native_lyrics` used before it shared `dart_trim`. + fn previous_lyrics_whitespace(c: char) -> bool { + matches!( + c, + '\t' | '\n' + | '\u{000b}' + | '\u{000c}' + | '\r' + | ' ' + | '\u{0085}' + | '\u{00a0}' + | '\u{1680}' + | '\u{2028}' + | '\u{2029}' + | '\u{202f}' + | '\u{205f}' + | '\u{3000}' + | '\u{feff}' + ) || ('\u{2000}'..='\u{200a}').contains(&c) + } + + #[test] + fn dart_whitespace_equals_previous_lyrics_set_for_every_scalar() { + for c in (0..=char::MAX as u32).filter_map(char::from_u32) { + assert_eq!(dart_whitespace(c), previous_lyrics_whitespace(c), "{c:?}"); + } + for (input, expected) in [ + ("", ""), + ("\u{feff}", ""), + ("\u{feff} a b \u{3000}", "a b"), + ("\u{0085}x\u{200a}", "x"), + ("\u{200b}x\u{200b}", "\u{200b}x\u{200b}"), + ("\u{180e}x", "\u{180e}x"), + ( + "\t\n\u{000b}\u{000c}\r Caf\u{00e9} \u{1f3b5}\u{00a0}", + "Caf\u{00e9} \u{1f3b5}", + ), + ] { + assert_eq!(dart_trim(input), expected, "{input:?}"); + assert_eq!( + dart_trim(input), + input.trim_matches(previous_lyrics_whitespace), + "{input:?}" + ); + } + } } diff --git a/rust_backend/crates/mobile/src/native_lyrics/text.rs b/rust_backend/crates/mobile/src/native_lyrics/text.rs index b4a58aa3..6aae89f0 100644 --- a/rust_backend/crates/mobile/src/native_lyrics/text.rs +++ b/rust_backend/crates/mobile/src/native_lyrics/text.rs @@ -18,28 +18,9 @@ static VOICE_PREFIX: LazyLock = LazyLock::new(|| { Regex::new(&format!(r"(?i)^({WS}*(?:<[0-9]{{1,3}}:[0-9]{{1,2}}(?:[.:][0-9]{{1,3}})?>{WS}*)*)(v[1-9][0-9]*):[ \t]*")).unwrap() }); -pub(super) fn whitespace(c: char) -> bool { - matches!( - c, - '\t' | '\n' - | '\u{000b}' - | '\u{000c}' - | '\r' - | ' ' - | '\u{0085}' - | '\u{00a0}' - | '\u{1680}' - | '\u{2028}' - | '\u{2029}' - | '\u{202f}' - | '\u{205f}' - | '\u{3000}' - | '\u{feff}' - ) || ('\u{2000}'..='\u{200a}').contains(&c) -} -pub(super) fn trim(s: &str) -> &str { - s.trim_matches(whitespace) -} +pub(super) use crate::native_library::path_keys::{ + dart_trim as trim, dart_whitespace as whitespace, +}; pub(super) fn trim_end(s: &str) -> &str { s.trim_end_matches(whitespace) } diff --git a/rust_backend/crates/mobile/src/native_playlists.rs b/rust_backend/crates/mobile/src/native_playlists.rs index b0b73173..4558d519 100644 --- a/rust_backend/crates/mobile/src/native_playlists.rs +++ b/rust_backend/crates/mobile/src/native_playlists.rs @@ -1,5 +1,6 @@ //! The existing playlist import contract, executed on the native data worker. //! CSV remains line-based for compatibility with existing exports/imports. +use crate::native_library::path_keys::dart_trim as trim; use serde_json::{Value, json}; use std::{ collections::HashMap, @@ -89,10 +90,6 @@ fn lines(content: &str) -> Vec<&str> { result } -fn trim(value: &str) -> &str { - value.trim_matches(|character: char| character.is_whitespace() || character == '\u{feff}') -} - fn track( id: String, title: &str, diff --git a/rust_backend/crates/mobile/src/network_tags.rs b/rust_backend/crates/mobile/src/network_tags.rs index 2699823c..a2697a04 100644 --- a/rust_backend/crates/mobile/src/network_tags.rs +++ b/rust_backend/crates/mobile/src/network_tags.rs @@ -3,7 +3,7 @@ use base64::Engine; use serde_json::{Value, json}; use sha2::{Digest, Sha256}; -use spotiflac_core::tags; +use spotiflac_core::{lower_hex, tags}; use spotiflac_network::{ HttpRequest, NetworkService, NetworkSession, policy::NetworkPermissions, url::UrlParts, }; @@ -125,11 +125,7 @@ pub(crate) fn matches_upload( if received != length { return Ok(json!({"matches": false})); } - let remote: String = hash - .finalize() - .iter() - .map(|byte| format!("{byte:02x}")) - .collect(); + let remote = lower_hex(&hash.finalize()); let local = crate::data_jobs::hash_file(path, check)?; check()?; Ok(json!({"matches": remote == local})) @@ -191,10 +187,7 @@ fn cache_cover( return Err("Artwork cache must be absolute".into()); } fs::create_dir_all(directory).map_err(|error| error.to_string())?; - let digest: String = Sha256::digest(bytes) - .iter() - .map(|byte| format!("{byte:02x}")) - .collect(); + let digest = lower_hex(&Sha256::digest(bytes)); let target = directory.join(format!("network_cover_{digest}.image")); let mut staged = tempfile::NamedTempFile::new_in(directory).map_err(|error| error.to_string())?; diff --git a/rust_backend/crates/network/src/client/stream.rs b/rust_backend/crates/network/src/client/stream.rs index 5995febc..8828c0cd 100644 --- a/rust_backend/crates/network/src/client/stream.rs +++ b/rust_backend/crates/network/src/client/stream.rs @@ -87,6 +87,30 @@ impl HttpStream { result } + /// Collect the rest of the body through `buffer`, returning `Ok(None)` as + /// soon as it exceeds `limit` bytes. Each read requests at most one byte + /// past the limit, so an oversized body is never buffered further. Read + /// errors are returned unchanged for the caller to map. + pub fn read_limited( + &mut self, + limit: usize, + buffer: &mut [u8], + check: impl Fn() -> Result<(), String>, + ) -> Result>, String> { + let mut body = Vec::new(); + loop { + let capacity = buffer.len().min(limit + 1 - body.len()); + let count = self.read(&mut buffer[..capacity], &check)?; + if count == 0 { + return Ok(Some(body)); + } + body.extend_from_slice(&buffer[..count]); + if body.len() > limit { + return Ok(None); + } + } + } + /// Release an unwanted body (a range probe or retryable error). A small /// HTTP/1.1 body is finished within 20 ms so its connection can return to /// the pool; HTTP/2, compressed, large or slow bodies are dropped at once. diff --git a/rust_backend/crates/network/src/client/tests.rs b/rust_backend/crates/network/src/client/tests.rs index 1d627a2d..3cc78c92 100644 --- a/rust_backend/crates/network/src/client/tests.rs +++ b/rust_backend/crates/network/src/client/tests.rs @@ -86,6 +86,81 @@ fn direct_media_does_not_send_metadata_cookies_or_follow_redirects() { server.join().unwrap(); } +#[test] +fn read_limited_matches_the_bounded_read_loops_it_replaced() { + use std::io::{Read, Write}; + const BODY: &[u8] = b"0123456789"; + // (limit, buffer length): exact fit, one byte over, single-chunk and zero. + let cases = [(10, 4), (9, 4), (10, 64 << 10), (9, 64 << 10), (0, 4)]; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let base = format!("http://{}", listener.local_addr().unwrap()); + let server = std::thread::spawn(move || { + for _ in 0..cases.len() * 2 { + let (mut stream, _) = listener.accept().unwrap(); + stream + .set_read_timeout(Some(Duration::from_secs(3))) + .unwrap(); + let mut request = Vec::new(); + while !request.ends_with(b"\r\n\r\n") { + let mut byte = [0]; + stream.read_exact(&mut byte).unwrap(); + request.push(byte[0]); + } + stream + .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 10\r\nConnection: close\r\n\r\n") + .unwrap(); + stream.write_all(BODY).unwrap(); + } + }); + let service = NetworkService::new().unwrap(); + service.set_allow_private_network(true); + let session = service.session( + NetworkPermissions { + domains: vec!["127.0.0.1".into()], + allow_http: true, + }, + Duration::from_secs(3), + ); + let open = || { + let request = + serde_json::from_value(serde_json::json!({"url":format!("{base}/body")})).unwrap(); + session + .open_stream( + request, + Duration::from_secs(3), + Duration::from_secs(3), + || Ok(()), + ) + .unwrap() + }; + // The loop shared by the lyrics, resolver and registry readers. + let previous = |stream: &mut HttpStream, limit: usize, buffer: &mut [u8]| { + let mut body = Vec::new(); + loop { + let capacity = buffer.len().min(limit + 1 - body.len()); + let count = stream.read(&mut buffer[..capacity], || Ok(()))?; + if count == 0 { + break; + } + body.extend_from_slice(&buffer[..count]); + if body.len() > limit { + return Ok::<_, String>(None); + } + } + Ok(Some(body)) + }; + for (limit, length) in cases { + let actual = open().read_limited(limit, &mut vec![0; length], || Ok(())); + let expected = previous(&mut open(), limit, &mut vec![0; length]); + assert_eq!(actual, expected, "limit {limit} buffer {length}"); + assert_eq!( + actual.unwrap(), + (limit >= BODY.len()).then(|| BODY.to_vec()) + ); + } + server.join().unwrap(); +} + #[test] fn cancellation_after_operation_is_pending_drops_it_before_return() { struct Dropped<'a>(&'a AtomicBool); diff --git a/rust_backend/crates/providers/src/lyrics/http.rs b/rust_backend/crates/providers/src/lyrics/http.rs index 0a3de5e2..246d9736 100644 --- a/rust_backend/crates/providers/src/lyrics/http.rs +++ b/rust_backend/crates/providers/src/lyrics/http.rs @@ -159,23 +159,10 @@ fn transport_error(error: String, check: &Check<'_>) -> LyricsError { } fn read(stream: &mut HttpStream, limit: usize, check: &Check<'_>) -> Result, LyricsError> { - let mut result = Vec::new(); - let mut buffer = [0; 16 << 10]; - loop { - let capacity = buffer.len().min(limit + 1 - result.len()); - let count = stream - .read(&mut buffer[..capacity], check) - .map_err(|error| transport_error(error, check))?; - if count == 0 { - break; - } - result.extend_from_slice(&buffer[..count]); - if result.len() > limit { - return Err(LyricsError::Other(format!( - "response exceeds {limit} bytes" - ))); - } - } + let result = stream + .read_limited(limit, &mut [0; 16 << 10], check) + .map_err(|error| transport_error(error, check))? + .ok_or_else(|| LyricsError::Other(format!("response exceeds {limit} bytes")))?; check().map_err(LyricsError::Cancelled)?; Ok(result) } diff --git a/rust_backend/crates/providers/src/resolver/http.rs b/rust_backend/crates/providers/src/resolver/http.rs index 964090ee..fa397555 100644 --- a/rust_backend/crates/providers/src/resolver/http.rs +++ b/rust_backend/crates/providers/src/resolver/http.rs @@ -166,23 +166,10 @@ impl ResolverHttp { } else { 2 << 20 }; - let mut body = Vec::new(); - let mut buffer = [0; 16 << 10]; - loop { - let capacity = buffer.len().min(limit + 1 - body.len()); - let read = stream - .read(&mut buffer[..capacity], check) - .map_err(failure)?; - if read == 0 { - break; - } - body.extend_from_slice(&buffer[..read]); - if body.len() > limit { - return Err(ResolverError::Failed(format!( - "response exceeds {limit} bytes" - ))); - } - } + let body = stream + .read_limited(limit, &mut [0; 16 << 10], check) + .map_err(failure)? + .ok_or_else(|| ResolverError::Failed(format!("response exceeds {limit} bytes")))?; check_active(check)?; if body.is_empty() && language.is_none() { return Err(ResolverError::Failed("response body is empty".into()));