diff --git a/crates/formats/apt/src/control.rs b/crates/formats/apt/src/control.rs index fa6d758e..9d3221b3 100644 --- a/crates/formats/apt/src/control.rs +++ b/crates/formats/apt/src/control.rs @@ -10,8 +10,12 @@ use std::collections::HashSet; use std::fmt; -/// The longest control file read from a package, in bytes. -pub const MAX_CONTROL_BYTES: usize = 1024 * 1024; +/// The longest control file read from a package, in bytes. Debian's +/// largest control files are a few KiB (a long `Description`, the +/// `Provides` of a metapackage); 64 KiB leaves room for any real one. An +/// index lists each package's control data once per architecture, so this +/// bounds what a package costs every index it is in (#797). +pub const MAX_CONTROL_BYTES: usize = 64 * 1024; /// The most fields a paragraph may hold. Real paragraphs hold a few dozen: /// a binary package's control file has the 20-odd fields of Debian Policy @@ -777,12 +781,14 @@ mod tests { #[test] fn a_control_file_of_many_fields_is_refused_quickly() { - // A whole control file (1 MiB) of distinct short fields: about - // 100,000 of them, which took ~20 s when each field was compared - // with every earlier one. + // 1 MiB of distinct short fields (what a control file could hold + // before #797; an index's text can hold more): about 100,000 of + // them, which took ~20 s when each field was compared with every + // earlier one. + const BYTES: usize = 1024 * 1024; let mut text = String::new(); let mut i = 0; - while text.len() < MAX_CONTROL_BYTES - 16 { + while text.len() < BYTES - 16 { text.push_str(&format!("a{i}:\n")); i += 1; } @@ -797,7 +803,7 @@ mod tests { // Distinct fields spread over paragraphs under the bound are read // in linear time too. let mut text = String::new(); - while text.len() < MAX_CONTROL_BYTES - 8192 { + while text.len() < BYTES - 8192 { text.push_str(&fields(MAX_FIELDS)); text.push('\n'); } diff --git a/crates/formats/apt/src/deb.rs b/crates/formats/apt/src/deb.rs index ba81825a..ab6a8e2b 100644 --- a/crates/formats/apt/src/deb.rs +++ b/crates/formats/apt/src/deb.rs @@ -337,7 +337,10 @@ fn control_in_tar(tar: impl Read) -> Result, DebError> { return invalid("the package's control is not a file"); } if entry.size() > MAX_CONTROL_BYTES as u64 { - return invalid("the package's control file is too large"); + return invalid(format!( + "the package's control file is larger than {} KiB", + MAX_CONTROL_BYTES >> 10 + )); } let mut control = Vec::new(); entry @@ -519,6 +522,20 @@ pub(crate) mod tests { assert!(err.to_string().contains("no control file"), "{err}"); } + #[test] + fn a_control_file_over_the_cap_is_refused() { + // A control file of exactly the cap is read; one byte more is + // refused (#797: it was 1 MiB). + let head = "Package: big\nVersion: 1\nArchitecture: all\nDescription: x\n "; + let at_cap = format!("{head}{}\n", "y".repeat(MAX_CONTROL_BYTES - head.len() - 1)); + assert_eq!(at_cap.len(), MAX_CONTROL_BYTES); + let read = control_file(&mut Cursor::new(deb_of(&[("./control", &at_cap)]))).unwrap(); + assert_eq!(read.len(), MAX_CONTROL_BYTES); + let over = format!("{at_cap}z"); + let err = control_file(&mut Cursor::new(deb_of(&[("./control", &over)]))).unwrap_err(); + assert!(err.to_string().contains("larger than 64 KiB"), "{err}"); + } + /// A `.deb` whose control member holds a regular file of `data` at each /// of `files`' paths, as written: a leading `/` is kept. fn deb_of(files: &[(&str, &str)]) -> Vec { diff --git a/crates/formats/apt/src/index.rs b/crates/formats/apt/src/index.rs index 7a1f0632..004c67ae 100644 --- a/crates/formats/apt/src/index.rs +++ b/crates/formats/apt/src/index.rs @@ -14,8 +14,13 @@ //! //! Everything but `Release`'s `Date` is a function of the packages: the //! same packages give the same `Packages`, byte for byte. +//! +//! The lists are made one at a time and handed to the host as they are +//! made, after their size was checked (#797). A package whose stored +//! control data is longer than any upload may give +//! ([`MAX_LISTED_CONTROL_BYTES`]) is not listed. -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeSet; use std::io::Write; use forge_plugin_api::{ @@ -23,7 +28,7 @@ use forge_plugin_api::{ }; use sha2::{Digest, Sha256}; -use crate::control::Paragraph; +use crate::control::{Paragraph, MAX_CONTROL_BYTES}; use crate::layout::{self, KEY_ASC, KEY_GPG}; use crate::manifest::Summary; @@ -40,6 +45,16 @@ pub struct ReleaseInfo<'a> { pub label: &'a str, } +/// The longest stored control data a package may have to be listed. Every +/// package uploaded or imported has a control file of at most +/// [`MAX_CONTROL_BYTES`], which its stored control data spells canonically +/// (a space after each `:`, ` .` for a blank line), a few bytes more at most; +/// twice the cap leaves room for that. A package stored while the cap was +/// 1 MiB (before #797) may have more: it is left out of the indexes, so that +/// it neither costs every index generation its size per list nor fails +/// them. It stays in the pool until it is deleted. +pub const MAX_LISTED_CONTROL_BYTES: usize = 2 * MAX_CONTROL_BYTES; + /// One package's entry, ready to be listed. struct Entry { component: String, @@ -47,7 +62,9 @@ struct Entry { stanza: String, } -fn entry(package: &IndexedPackage) -> Result { +/// The entry of `package`; `None` when its control data is too long to be +/// listed ([`MAX_LISTED_CONTROL_BYTES`]). +fn entry(package: &IndexedPackage) -> Result, FormatError> { let summary = package .summary .as_deref() @@ -58,6 +75,9 @@ fn entry(package: &IndexedPackage) -> Result { package.id, package.version )) })?; + if summary.control.len() > MAX_LISTED_CONTROL_BYTES { + return Ok(None); + } let mut paragraph = Paragraph::parse_one(&summary.control) .map_err(|e| FormatError::Internal(format!("{} {}: {e}", package.id, package.version)))?; for field in INDEX_FIELDS { @@ -72,20 +92,35 @@ fn entry(package: &IndexedPackage) -> Result { paragraph.set("Filename", &filename); paragraph.set("Size", &package.size.to_string()); paragraph.set("SHA256", &package.sha256); - Ok(Entry { + Ok(Some(Entry { component: summary.component, architecture: summary.architecture, stanza: paragraph.to_text(), - }) + })) } -/// The index files of `input` (see the module's documentation). -pub fn render(info: &ReleaseInfo<'_>, input: &IndexInput) -> Result, FormatError> { - let entries = input - .packages - .iter() - .map(entry) - .collect::, _>>()?; +/// Whether `entry` is listed in the `Packages` of `component` and `arch`: +/// its own, and every architecture's for an `all` package. +fn listed_in(entry: &Entry, component: &str, arch: &str) -> bool { + entry.component == component && (entry.architecture == arch || entry.architecture == "all") +} + +/// The index files of `input` (see the module's documentation), each handed +/// to `sink` as it is made. The `Packages` lists are made one at a time: +/// what is held at once is the packages' stanzas, once each, and one list +/// with its compressed copies (#797). Before any is made, their size is +/// checked against [`IndexInput::max_bytes`]: each list is stored twice +/// uncompressed (by name and by hash), and an index whose lists alone hold +/// more is refused. +pub fn render( + info: &ReleaseInfo<'_>, + input: &IndexInput, + sink: &mut dyn FnMut(IndexFile) -> Result<(), FormatError>, +) -> Result<(), FormatError> { + let mut entries = Vec::with_capacity(input.packages.len()); + for package in &input.packages { + entries.extend(entry(package)?); + } // Components and architectures: those of the packages; `main` and // `all` when there are none. @@ -104,61 +139,66 @@ pub fn render(info: &ReleaseInfo<'_>, input: &IndexInput) -> Result = BTreeMap::new(); + // What the lists will hold, before any is made: a stanza's bytes for + // each list it is in, and a blank line between two stanzas. + let mut plain_bytes: u64 = 0; for component in &components { for arch in &architectures { - lists.insert((component.clone(), arch.clone()), String::new()); + let (count, bytes) = entries + .iter() + .filter(|e| listed_in(e, component, arch)) + .fold((0u64, 0u64), |(n, b), e| (n + 1, b + e.stanza.len() as u64)); + plain_bytes += bytes + count.saturating_sub(1); } } - for e in &entries { - let targets: Vec<&String> = if e.architecture == "all" { - architectures.iter().collect() - } else { - vec![&e.architecture] - }; - for arch in targets { - let list = lists - .get_mut(&(e.component.clone(), arch.clone())) - .expect("every component and architecture has a list"); - if !list.is_empty() { - list.push('\n'); - } - list.push_str(&e.stanza); - } + let stored = plain_bytes.saturating_mul(2); + if stored > input.max_bytes { + return Err(FormatError::Internal(format!( + "the index's Packages lists would hold {stored} bytes, more than the {} an index may hold", + input.max_bytes + ))); } let base = format!("dists/{}", info.distribution); - let mut files = Vec::new(); // (path below dists//, size, sha256), for Release. let mut listed: Vec<(String, usize, String)> = Vec::new(); - for ((component, arch), packages) in &lists { - let dir = format!("{component}/binary-{arch}"); - let plain = packages.as_bytes().to_vec(); - let gz = gzip(&plain)?; - let xz = xz(&plain)?; - for (name, content, content_type) in [ - ("Packages", plain, "text/plain; charset=utf-8"), - ("Packages.gz", gz, "application/gzip"), - ("Packages.xz", xz, "application/x-xz"), - ] { - let sha256 = hex::encode(Sha256::digest(&content)); - listed.push((format!("{dir}/{name}"), content.len(), sha256.clone())); - files.push(IndexFile { - path: format!("{base}/{dir}/by-hash/SHA256/{sha256}"), - content: content.clone(), - content_type: content_type.into(), - immutable: true, - signed: Vec::new(), - }); - files.push(IndexFile { - path: format!("{base}/{dir}/{name}"), - content, - content_type: content_type.into(), - immutable: false, - signed: Vec::new(), - }); + // Packages of each component and architecture, in input order (by id, + // then version), made, handed over and dropped one at a time. + for component in &components { + for arch in &architectures { + let mut packages = String::new(); + for e in entries.iter().filter(|e| listed_in(e, component, arch)) { + if !packages.is_empty() { + packages.push('\n'); + } + packages.push_str(&e.stanza); + } + let dir = format!("{component}/binary-{arch}"); + let plain = packages.into_bytes(); + let gz = gzip(&plain)?; + let xz = xz(&plain)?; + for (name, content, content_type) in [ + ("Packages", plain, "text/plain; charset=utf-8"), + ("Packages.gz", gz, "application/gzip"), + ("Packages.xz", xz, "application/x-xz"), + ] { + let sha256 = hex::encode(Sha256::digest(&content)); + listed.push((format!("{dir}/{name}"), content.len(), sha256.clone())); + sink(IndexFile { + path: format!("{base}/{dir}/by-hash/SHA256/{sha256}"), + content: content.clone(), + content_type: content_type.into(), + immutable: true, + signed: Vec::new(), + })?; + sink(IndexFile { + path: format!("{base}/{dir}/{name}"), + content, + content_type: content_type.into(), + immutable: false, + signed: Vec::new(), + })?; + } } } @@ -171,7 +211,7 @@ pub fn render(info: &ReleaseInfo<'_>, input: &IndexInput) -> Result, input: &IndexInput) -> Result, input: &IndexInput) -> Result, FormatError> { + let mut files = Vec::new(); + render(info, input, &mut |file| { + files.push(file); + Ok(()) + })?; + Ok(files) + } + fn file<'a>(files: &'a [IndexFile], path: &str) -> &'a IndexFile { files .iter() @@ -318,7 +368,7 @@ mod tests { #[test] fn packages_list_every_version_under_its_component_and_architecture() { - let files = render( + let files = collect( &INFO, &input(vec![ package( @@ -402,7 +452,7 @@ mod tests { #[test] fn compressed_copies_hold_the_same_list() { - let files = render( + let files = collect( &INFO, &input(vec![package( "Package: hello\nVersion: 1.0\nArchitecture: amd64\nDescription: hi\n", @@ -430,7 +480,7 @@ mod tests { #[test] fn an_empty_repository_has_an_empty_index() { - let files = render(&INFO, &input(Vec::new())).unwrap(); + let files = collect(&INFO, &input(Vec::new())).unwrap(); assert!(file(&files, "dists/bookworm/main/binary-all/Packages") .content .is_empty()); @@ -444,7 +494,7 @@ mod tests { #[test] fn the_same_packages_give_the_same_files() { - let one = render( + let one = collect( &INFO, &input(vec![package( "Package: a1\nVersion: 1\nArchitecture: amd64\n", @@ -457,7 +507,7 @@ mod tests { 1, )]); later.generated = "2027-01-01T00:00:00Z".into(); - let two = render(&INFO, &later).unwrap(); + let two = collect(&INFO, &later).unwrap(); for (a, b) in one.iter().zip(&two) { assert_eq!(a.path, b.path); if !a.path.ends_with("/Release") { @@ -465,4 +515,99 @@ mod tests { } } } + + /// A package of `arch` whose `Description` makes its control data + /// `bytes` long, about. + fn large(name: &str, arch: &str, bytes: usize) -> IndexedPackage { + package( + &format!( + "Package: {name}\nVersion: 1\nArchitecture: {arch}\nDescription: big\n {}\n", + "x".repeat(bytes) + ), + 1, + ) + } + + #[test] + fn an_index_over_the_byte_cap_fails_before_anything_is_compressed() { + // 20 `all` packages of 60 KiB and one amd64 package: the `all` + // packages are listed under binary-all and binary-amd64, so the + // lists hold 2 × 20 × 60 KiB, stored twice: some 4.7 MiB. + let mut packages: Vec<_> = (0..20) + .map(|i| large(&format!("p{i:02}"), "all", 60 * 1024)) + .collect(); + packages.push(package("Package: zz\nVersion: 1\nArchitecture: amd64\n", 1)); + let mut capped = input(packages); + capped.max_bytes = 4 * 1024 * 1024; + let mut handed = 0; + let err = render(&INFO, &capped, &mut |_| { + handed += 1; + Ok(()) + }) + .unwrap_err(); + assert!(err.to_string().contains("more than the 4194304"), "{err}"); + // Refused before the first list was made, compressed and handed + // over. + assert_eq!(handed, 0); + // Within the cap it is made. + capped.max_bytes = 5 * 1024 * 1024; + assert!(collect(&INFO, &capped).is_ok()); + } + + #[test] + fn lists_are_handed_over_one_at_a_time() { + // The sink sees each list's files before the next list is made, + // and its error stops the rendering. + let packages = vec![ + package("Package: aa\nVersion: 1\nArchitecture: amd64\n", 1), + package("Package: bb\nVersion: 1\nArchitecture: arm64\n", 1), + ]; + let mut paths = Vec::new(); + let err = render(&INFO, &input(packages), &mut |file| { + paths.push(file.path); + if paths.len() == 6 { + return Err(FormatError::Internal("full".into())); + } + Ok(()) + }) + .unwrap_err(); + assert_eq!( + err.to_string(), + FormatError::Internal("full".into()).to_string() + ); + assert_eq!(paths.len(), 6); + assert!( + paths.iter().all(|p| p.contains("/main/binary-amd64/")), + "{paths:?}" + ); + } + + #[test] + fn a_package_stored_over_the_cap_is_left_out() { + // Stored when a control file could be 1 MiB (before #797): not + // listed, and the index is made of the others. + let files = collect( + &INFO, + &input(vec![ + large("huge", "all", MAX_LISTED_CONTROL_BYTES), + package("Package: small\nVersion: 1\nArchitecture: amd64\n", 1), + ]), + ) + .unwrap(); + let amd64 = &file(&files, "dists/bookworm/main/binary-amd64/Packages").content; + let amd64 = String::from_utf8(amd64.clone()).unwrap(); + assert!(amd64.starts_with("Package: small\n"), "{amd64}"); + assert!(!amd64.contains("huge")); + let release = + String::from_utf8(file(&files, "dists/bookworm/Release").content.clone()).unwrap(); + assert!(release.contains("Architectures: amd64\n"), "{release}"); + // One at the upload cap is listed. + let files = collect( + &INFO, + &input(vec![large("fits", "all", MAX_CONTROL_BYTES - 100)]), + ) + .unwrap(); + let all = &file(&files, "dists/bookworm/main/binary-all/Packages").content; + assert!(all.starts_with(b"Package: fits\n")); + } } diff --git a/crates/formats/apt/src/lib.rs b/crates/formats/apt/src/lib.rs index a82d4182..b05edaf7 100644 --- a/crates/formats/apt/src/lib.rs +++ b/crates/formats/apt/src/lib.rs @@ -162,7 +162,12 @@ impl FormatPlugin for Apt { } } - fn index(&self, repo: &Repository, input: &IndexInput) -> Result, FormatError> { + fn index( + &self, + repo: &Repository, + input: &IndexInput, + sink: &mut dyn FnMut(IndexFile) -> Result<(), FormatError>, + ) -> Result<(), FormatError> { let distribution = repo .option(DISTRIBUTION) .ok_or_else(|| FormatError::Internal("the repository has no distribution".into()))?; @@ -172,6 +177,7 @@ impl FormatPlugin for Apt { label: &repo.name, }, input, + sink, ) } diff --git a/crates/plugin-api/src/lib.rs b/crates/plugin-api/src/lib.rs index b25f3550..f33bd4bd 100644 --- a/crates/plugin-api/src/lib.rs +++ b/crates/plugin-api/src/lib.rs @@ -325,8 +325,20 @@ pub trait FormatPlugin: Send + Sync + 'static { /// and replaces the previous index with all the files at once. Must be /// deterministic but for [`IndexInput::generated`]. The default makes /// none. - fn index(&self, repo: &Repository, input: &IndexInput) -> Result, FormatError> { - let _ = (repo, input); + /// + /// Each file goes to `sink` as soon as it is made, which stores it, so + /// that a format holds one file (an apt list and its copies) at a time, + /// not the whole index (#797). An error from `sink` (the index grew + /// past what the host stores) is returned as it is. A format checks + /// what its files will hold against [`IndexInput::max_bytes`] before it + /// makes any. + fn index( + &self, + repo: &Repository, + input: &IndexInput, + sink: &mut dyn FnMut(IndexFile) -> Result<(), FormatError>, + ) -> Result<(), FormatError> { + let _ = (repo, input, sink); Err(FormatError::Internal("this format keeps no indexes".into())) } diff --git a/crates/plugin-api/src/types.rs b/crates/plugin-api/src/types.rs index 195ef39d..c434aa90 100644 --- a/crates/plugin-api/src/types.rs +++ b/crates/plugin-api/src/types.rs @@ -1506,6 +1506,9 @@ pub struct IndexInput { pub keyring: Vec, /// The same keys, ASCII-armored in one block. pub keyring_armored: String, + /// The most bytes the index's files may hold together. The host stops + /// storing an index once its files hold more. + pub max_bytes: u64, } /// One version of an [`IndexInput`]. diff --git a/crates/server/src/indexes/mod.rs b/crates/server/src/indexes/mod.rs index 8d83bd06..e611a08e 100644 --- a/crates/server/src/indexes/mod.rs +++ b/crates/server/src/indexes/mod.rs @@ -275,6 +275,33 @@ struct Stored { immutable: bool, } +/// A file in the object store, to be recorded with the generation. +struct Ingested { + path: String, + content_type: String, + immutable: bool, + stored: forge_storage::StoredBlob, + sha1: String, + md5: String, +} + +/// Store `file`'s content (outside the transaction) and drop it. +async fn ingest(cas: &forge_storage::Cas, file: Stored) -> Result { + let staged = cas + .stage(&file.content[..], file.content.len() as u64) + .await + .map_err(cas_error)?; + let stored = cas.commit(staged).await.map_err(cas_error)?; + Ok(Ingested { + sha1: hex::encode(Sha1::digest(&file.content)), + md5: hex::encode(md5::Md5::digest(&file.content)), + path: file.path, + content_type: file.content_type, + immutable: file.immutable, + stored, + }) +} + /// Make and store a generation. `false` when the repository changed while /// it was made (nothing was stored). async fn generate( @@ -326,6 +353,7 @@ async fn generate( generated: chrono::Utc::now().to_rfc3339_opts(chrono::SecondsFormat::Secs, true), keyring, keyring_armored, + max_bytes: MAX_INDEX_BYTES as u64, }; let target = protocol::Repository { name: repo.name.clone(), @@ -335,29 +363,39 @@ async fn generate( options: repo.format_options(), }; let implementation = plugin.clone(); - let files = tokio::task::spawn_blocking(move || -> Result, IndexError> { - let made = implementation - .index(&target, &input) - .map_err(|e| IndexError::Internal(format!("the format made no index: {e}")))?; - sign_and_check(made, &keys) + let cas = state.cas.clone(); + let runtime = tokio::runtime::Handle::current(); + // Each file is signed, checked and stored as the format hands it over, + // then dropped: what is held at once is one file, not the index (#797). + let files = tokio::task::spawn_blocking(move || -> Result, IndexError> { + let mut bounds = Bounds::default(); + let mut ingested = Vec::new(); + // What failed in the host, kept to be returned as it is (an + // unavailable object store stays one). + let mut failed: Option = None; + let made = implementation.index(&target, &input, &mut |file| { + let store = || -> Result<(), IndexError> { + for file in sign(file, &keys)? { + bounds.add(&file)?; + ingested.push(runtime.block_on(ingest(&cas, file))?); + } + Ok(()) + }; + store().map_err(|e| { + let message = e.to_string(); + failed = Some(e); + protocol::FormatError::Internal(message) + }) + }); + if let Some(e) = failed { + return Err(e); + } + made.map_err(|e| IndexError::Internal(format!("the format made no index: {e}")))?; + Ok(ingested) }) .await .map_err(|e| IndexError::Internal(format!("index: {e}")))??; - // The content first, outside the transaction. - let mut ingested = Vec::with_capacity(files.len()); - for file in &files { - let staged = state - .cas - .stage(&file.content[..], file.content.len() as u64) - .await - .map_err(cas_error)?; - let stored = state.cas.commit(staged).await.map_err(cas_error)?; - let sha1 = hex::encode(Sha1::digest(&file.content)); - let md5 = hex::encode(md5::Md5::digest(&file.content)); - ingested.push((stored, sha1, md5)); - } - let grace = state.config.outbox.grace(); let mut tx = forge_db::begin(&state.pool).await?; let current = index_dao::lock(&mut tx, repo.id).await?; @@ -377,7 +415,8 @@ async fn generate( if versions_now != versions_digest || keys_now != keys_then { return Ok(false); } - for (file, (stored, sha1, md5)) in files.iter().zip(&ingested) { + for file in &files { + let stored = &file.stored; raw_dao::store( &mut tx, repo.id, @@ -387,8 +426,8 @@ async fn generate( size: i64::try_from(stored.size).unwrap_or(i64::MAX), content_type: &file.content_type, sha512_base64: &stored.sha512_base64, - sha1, - md5, + sha1: &file.sha1, + md5: &file.md5, upstream_etag: None, upstream_last_modified: None, checked: true, @@ -422,49 +461,54 @@ fn cas_error(e: forge_storage::CasError) -> IndexError { } } -/// Check what the plugin made (valid, distinct paths; bounded) and add the -/// signed copies. -fn sign_and_check(made: Vec, keys: &[Key]) -> Result, IndexError> { - let bad = |what: String| IndexError::Internal(format!("the format's index: {what}")); - let mut out = Vec::with_capacity(made.len() + 2); - for file in made { - for copy in &file.signed { - let content = match copy.signature { - SignatureKind::Cleartext => { - let text = std::str::from_utf8(&file.content).map_err(|_| { - bad(format!("{} is signed as text but is not UTF-8", file.path)) - })?; - signing::sign_cleartext(keys, text)? - } - SignatureKind::Detached => signing::sign_detached(keys, &file.content)?, - }; - out.push(Stored { - path: copy.path.clone(), - content: content.into_bytes(), - content_type: match copy.signature { - SignatureKind::Cleartext => "text/plain; charset=utf-8".into(), - SignatureKind::Detached => "application/pgp-signature".into(), - }, - immutable: false, - }); - } +fn bad(what: String) -> IndexError { + IndexError::Internal(format!("the format's index: {what}")) +} + +/// A file the plugin made and the signed copies it names, the copies first. +fn sign(file: protocol::IndexFile, keys: &[Key]) -> Result, IndexError> { + let mut out = Vec::with_capacity(file.signed.len() + 1); + for copy in &file.signed { + let content = match copy.signature { + SignatureKind::Cleartext => { + let text = std::str::from_utf8(&file.content).map_err(|_| { + bad(format!("{} is signed as text but is not UTF-8", file.path)) + })?; + signing::sign_cleartext(keys, text)? + } + SignatureKind::Detached => signing::sign_detached(keys, &file.content)?, + }; out.push(Stored { - path: file.path, - content: file.content, - content_type: file.content_type, - immutable: file.immutable, + path: copy.path.clone(), + content: content.into_bytes(), + content_type: match copy.signature { + SignatureKind::Cleartext => "text/plain; charset=utf-8".into(), + SignatureKind::Detached => "application/pgp-signature".into(), + }, + immutable: false, }); } - if out.len() > MAX_INDEX_FILES { - return Err(bad(format!("more than {MAX_INDEX_FILES} files"))); - } - let mut seen = HashSet::new(); - let mut total = 0usize; - for file in &out { + out.push(Stored { + path: file.path, + content: file.content, + content_type: file.content_type, + immutable: file.immutable, + }); + Ok(out) +} + +/// What an index's files held so far: each is checked (a valid path, not +/// seen before; a valid content type) as it comes, and the index is +/// refused once its files are too many or too large together. +#[derive(Default)] +struct Bounds { + paths: HashSet, + bytes: usize, +} + +impl Bounds { + fn add(&mut self, file: &Stored) -> Result<(), IndexError> { files::validate_path(&file.path).map_err(|e| bad(format!("{:?}: {e}", file.path)))?; - if !seen.insert(file.path.as_str()) { - return Err(bad(format!("{:?} twice", file.path))); - } let essence = file.content_type.split(';').next().unwrap_or("").trim(); let content_type_ok = file.content_type.len() <= 127 && file @@ -480,10 +524,16 @@ fn sign_and_check(made: Vec, keys: &[Key]) -> Result MAX_INDEX_BYTES { - return Err(bad(format!("larger than {MAX_INDEX_BYTES} bytes"))); + if self.paths.len() >= MAX_INDEX_FILES { + return Err(bad(format!("more than {MAX_INDEX_FILES} files"))); + } + if !self.paths.insert(file.path.clone()) { + return Err(bad(format!("{:?} twice", file.path))); + } + self.bytes = self.bytes.saturating_add(file.content.len()); + if self.bytes > MAX_INDEX_BYTES { + return Err(bad(format!("larger than {MAX_INDEX_BYTES} bytes"))); + } + Ok(()) } - Ok(out) } diff --git a/crates/server/tests/apt.rs b/crates/server/tests/apt.rs index ca346013..68947d50 100644 --- a/crates/server/tests/apt.rs +++ b/crates/server/tests/apt.rs @@ -370,6 +370,16 @@ async fn hosted_repositories_publish_signed_indexes() { assert_eq!(status, StatusCode::BAD_REQUEST, "{field}: {value}: {text}"); assert!(text.contains(field), "{field}: {value}: {text}"); } + // And a control file over 64 KiB (#797): its stanza would be listed + // once per architecture of every index generation. + let (status, text) = apt + .post(&deb_of(&format!( + "Package: wordy\nVersion: 1\nArchitecture: all\nDescription: w\n {}\n", + "w".repeat(64 * 1024) + ))) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{text}"); + assert!(text.contains("larger than 64 KiB"), "{text}"); // InRelease is Release, signed with the repository's key. let keys = keys(&srv, &name).await; diff --git a/docs/apt.md b/docs/apt.md index c80868aa..6d46ce07 100644 --- a/docs/apt.md +++ b/docs/apt.md @@ -145,7 +145,12 @@ exhaust the server's memory. A package is refused with `400` when its control member inflates to more than 8 MiB before the `control` file ends, or when its compression declares more than dpkg-deb's levels up to `-z19` do: an xz dictionary over 64 MiB, a zstd window over 8 MiB -([zstd levels 20 to 22](#limitations)). Imports read packages the same way. +([zstd levels 20 to 22](#limitations)), and when its `control` file is +larger than 64 KiB (Debian's largest are a few KiB): an index lists each +package's control data once per architecture. Imports read packages the same +way. A package stored with a larger `control` file before this limit (a +build of 2.2.0's development line) is left out of the indexes; it stays in +the pool until it is deleted. `DELETE` of a package's pool file deletes the version (delete permission): diff --git a/docs/changes/2.2.0/797.fixed.md b/docs/changes/2.2.0/797.fixed.md new file mode 100644 index 00000000..d4cbb858 --- /dev/null +++ b/docs/changes/2.2.0/797.fixed.md @@ -0,0 +1,9 @@ +An apt package's `control` file could be 1 MiB, and an index generation +held every list's text and copies at once, a few times each package's +control file per architecture: a writer's few hundred tiny packages with +large descriptions made every `apt-get update` OOM-kill the instance. A +`control` file over 64 KiB is refused with `400`, on upload and on import; +an index's size is checked before its lists are made, and they are made, +stored and dropped one at a time. A package stored before with a larger +`control` file is left out of the indexes (it stays in the pool) instead of +failing them ([apt repositories](docs/apt.md#uploading-packages)). diff --git a/docs/format-plugins.md b/docs/format-plugins.md index ed013c77..0237c3f2 100644 --- a/docs/format-plugins.md +++ b/docs/format-plugins.md @@ -379,8 +379,11 @@ descriptor. The host then keeps the indexes of its hosted repositories: - **Made of the packages.** `index` gets every listed version of the repository (its id, version, stored summary, size and digests, by id, - then version) and returns the index files: paths, content, content types. - The plugin makes them as a pure function of its input (apt's + then version) and hands the index files to the host one at a time (paths, + content, content types), which signs and stores each as it comes: a + plugin holds one file, not the whole index. It checks what its files will + hold against the most an index may hold (`max_bytes`) before it makes + any. The plugin makes them as a pure function of its input (apt's [`index.rs`](../crates/formats/apt/src/index.rs)), so the summary a package format stores at publish should hold what its index lists of a version: making an index reads no package and no manifest.