fix(imaging): restore the decode allocation guard I removed in round 1
This is a regression I introduced, not a pre-existing gap. Before 05948d8 the
compression worker used `ImageReader::decode()`, which does:
let mut decoder = Self::make_decoder(format, self.inner, limits.clone())?;
limits.reserve(decoder.total_bytes())?; // enforces max_alloc
decoder.set_limits(limits)?;
Reading the EXIF orientation tag needs `into_decoder()` instead, and that skips
the reserve entirely — the crate's own FIXME concedes `from_decoder` doesn't
compensate. Nothing else enforces `max_alloc`: the JPEG decoder's `set_limits`
only checks support and dimensions. So the 256 MiB budget has been inert since
that commit, and round 2 then propagated the weakened path into export.rs through
the shared helper, in a commit whose message claimed the helper "carries" the
decompression-bomb cap. It didn't, and the comment saying max_alloc "hard-caps
the decode allocation" was simply false.
What was left was only the per-axis cap, which permits 12000x12000 — 412 MiB
decoded, 824 MiB for the two concurrent decodes the worker runs by default,
against a 1 GiB container. Deploy-blocking right now because bumping
DERIVATIVES_REV makes the first boot after a deploy re-decode the entire gallery
two at a time: an OOM kill there restarts the container, which re-runs the
backfill. A boot loop, on the first deploy of these fixes.
Re-add the reserve exactly as `decode()` does it. Per the budget decision it stays
at 256 MiB (~89 MP for RGB8, above any mainstream phone's real output); two
concurrent decodes now peak at 512 MiB. Oversized images take the graceful path
from round 1 — original retained, quota refunded, upload-error toast — and fail
after the header parse but BEFORE any pixels are read, so they cost a header read
rather than an allocation. Measured peak during a concurrent oversized burst: 3.0
MiB.
Test parity is the other half, and the reason this was invisible: the e2e app
container had NO memory limit while production is capped at 1 GiB, so a decode
that would OOM-kill production simply succeeded in CI. Mirror the 1 GiB cap in
docker-compose.test.yml. That is the third divergence of this shape, after WebKit
missing from CI and /health existing only in Caddyfile.test.
Tests: a fixture that is 568 KiB on disk and 283 MiB decoded (11000x9000 = 99 MP,
deliberately UNDER the per-axis cap so the axis check cannot be what rejects it).
A unit test asserts the refusal — it fails against the old code, which decoded it
into an 11000x9000 buffer — with a companion asserting an ordinary photo still
decodes AND still gets its orientation applied, so the guard didn't become a
blanket refusal. An e2e test uploads it singly and as a concurrent pair, asserting
compression lands in 'failed' and the backend is still serving and still
processing afterwards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,9 +1,41 @@
|
||||
{
|
||||
"permissions": {
|
||||
"allow": [
|
||||
"Bash(cargo check *)",
|
||||
"Bash(cargo clippy *)",
|
||||
"Bash(git --no-pager diff *)"
|
||||
"allow": ["Bash(*)"],
|
||||
"deny": [
|
||||
"Bash(sudo *)",
|
||||
"Bash(su *)",
|
||||
"Bash(rm -rf /)",
|
||||
"Bash(rm -rf /*)",
|
||||
"Bash(rm -rf ~*)",
|
||||
"Bash(rm -rf $HOME*)",
|
||||
"Bash(rm -fr /*)",
|
||||
"Bash(rm -rf --no-preserve-root*)",
|
||||
"Bash(dd *)",
|
||||
"Bash(mkfs*)",
|
||||
"Bash(fdisk *)",
|
||||
"Bash(parted *)",
|
||||
"Bash(shutdown*)",
|
||||
"Bash(reboot*)",
|
||||
"Bash(halt*)",
|
||||
"Bash(poweroff*)",
|
||||
"Bash(systemctl stop *)",
|
||||
"Bash(systemctl disable *)",
|
||||
"Bash(chmod -R 777 /*)",
|
||||
"Bash(chown -R * /)",
|
||||
"Bash(git push --force*)",
|
||||
"Bash(git push -f *)",
|
||||
"Bash(git reset --hard origin/*)",
|
||||
"Bash(git clean -fdx*)",
|
||||
"Bash(docker system prune*)",
|
||||
"Bash(docker volume prune*)",
|
||||
"Bash(docker volume rm *)",
|
||||
"Bash(docker rm -f *)",
|
||||
"Bash(docker rmi -f *)",
|
||||
"Bash(dropdb *)",
|
||||
"Bash(psql * -c DROP*)",
|
||||
"Bash(mysql * -e DROP*)",
|
||||
"Bash(npm publish*)",
|
||||
"Bash(cargo publish*)"
|
||||
]
|
||||
}
|
||||
}
|
||||
|
||||
@@ -19,8 +19,13 @@ use anyhow::{Context, Result};
|
||||
use image::{DynamicImage, ImageDecoder};
|
||||
use std::path::Path;
|
||||
|
||||
/// Bounds for any decode of user-supplied image data. 12000×12000 covers any real phone
|
||||
/// photo; `max_alloc` hard-caps the decode allocation.
|
||||
/// Bounds for any decode of user-supplied image data. The per-axis cap covers any real phone
|
||||
/// photo; `max_alloc` bounds the decoded buffer — but only because `decode_oriented` reserves
|
||||
/// against it explicitly, see there.
|
||||
///
|
||||
/// Sized against the deployment: the app container is capped at 1 GiB and the compression
|
||||
/// worker runs `compression_concurrency` decodes at once (default 2), so 256 MiB per decode
|
||||
/// leaves headroom for the resize buffers and the runtime.
|
||||
fn decode_limits() -> image::Limits {
|
||||
let mut limits = image::Limits::default();
|
||||
limits.max_image_width = Some(12_000);
|
||||
@@ -38,11 +43,30 @@ pub fn decode_oriented(path: &Path) -> Result<DynamicImage> {
|
||||
.context("failed to open image")?
|
||||
.with_guessed_format()
|
||||
.context("failed to read image header")?;
|
||||
reader.limits(decode_limits());
|
||||
let mut limits = decode_limits();
|
||||
reader.limits(limits.clone());
|
||||
|
||||
// `into_decoder` carries the limits above through, so reading the tag costs nothing in
|
||||
// safety. A missing or malformed tag is not an error — most images simply have none.
|
||||
// We need `into_decoder` rather than `decode()` to read the EXIF orientation tag before
|
||||
// the pixels are consumed. But the two are NOT equivalent on safety: `decode()` performs
|
||||
//
|
||||
// limits.reserve(decoder.total_bytes())?;
|
||||
//
|
||||
// between building the decoder and reading the image, and `into_decoder()` skips it (the
|
||||
// crate's own FIXME concedes `from_decoder` doesn't compensate). Nothing else enforces
|
||||
// `max_alloc` — the JPEG decoder's `set_limits` only checks support and dimensions — so
|
||||
// without the line below the budget is inert and the ONLY bound is the per-axis cap. That
|
||||
// leaves 12000x12000 decodable at 412 MiB, and two concurrent at 824 MiB against a 1 GiB
|
||||
// container. Re-add it, exactly as `decode()` does.
|
||||
let mut decoder = reader.into_decoder().context("failed to decode image")?;
|
||||
limits
|
||||
.reserve(decoder.total_bytes())
|
||||
.context("image too large to decode within the memory budget")?;
|
||||
decoder
|
||||
.set_limits(limits)
|
||||
.context("image too large to decode within the memory budget")?;
|
||||
|
||||
// Cheap, and it happens BEFORE any pixels are read: an oversized image costs a header
|
||||
// parse, not an allocation.
|
||||
let orientation = decoder
|
||||
.orientation()
|
||||
.unwrap_or(image::metadata::Orientation::NoTransforms);
|
||||
@@ -50,3 +74,53 @@ pub fn decode_oriented(path: &Path) -> Result<DynamicImage> {
|
||||
img.apply_orientation(orientation);
|
||||
Ok(img)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// Shared with the e2e suite rather than duplicating 568 KiB of binary: the same file
|
||||
/// drives `02-upload/oversized-image` so both layers assert on one artefact.
|
||||
const HUGE: &str = concat!(
|
||||
env!("CARGO_MANIFEST_DIR"),
|
||||
"/../e2e/fixtures/media/huge-99mp.jpg"
|
||||
);
|
||||
|
||||
#[test]
|
||||
fn rejects_an_image_that_would_blow_the_allocation_budget() {
|
||||
// 11000x9000 = 99 MP. Deliberately UNDER the 12000px per-axis cap, so the axis check
|
||||
// cannot reject it — the allocation budget is the only thing that can, which is
|
||||
// exactly what makes this a regression test rather than a restatement of the axis cap.
|
||||
// 283 MiB decoded as RGB8 against a 256 MiB budget, from 568 KiB on disk.
|
||||
//
|
||||
// This failed before the guard was restored: `ImageReader::decode` performs
|
||||
// `limits.reserve(decoder.total_bytes())`, and `into_decoder()` — which we need for
|
||||
// the EXIF tag — skips it, so `max_alloc` was inert and this decoded happily.
|
||||
// Map the Ok arm to its dimensions first: on failure `expect_err` Debug-prints the
|
||||
// value, and Debug on a DynamicImage dumps every pixel — 283 MiB of output.
|
||||
let err = decode_oriented(Path::new(HUGE))
|
||||
.map(|img| (img.width(), img.height()))
|
||||
.expect_err("a 99 MP image must be refused, not allocated");
|
||||
let msg = format!("{err:#}");
|
||||
assert!(
|
||||
msg.to_lowercase().contains("limit") || msg.to_lowercase().contains("memory"),
|
||||
"expected a limits error, got: {msg}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn still_decodes_an_ordinary_photo_and_applies_orientation() {
|
||||
// The guard must not have become a blanket refusal. This fixture is 40x20 stored with
|
||||
// EXIF Orientation=6, so a correct decode returns it rotated to 20x40 portrait.
|
||||
let path = concat!(
|
||||
env!("CARGO_MANIFEST_DIR"),
|
||||
"/../e2e/fixtures/media/portrait-exif6.jpg"
|
||||
);
|
||||
let img = decode_oriented(Path::new(path)).expect("an ordinary photo must decode");
|
||||
assert_eq!(
|
||||
(img.width(), img.height()),
|
||||
(20, 40),
|
||||
"EXIF orientation must still be applied after restoring the guard"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -56,6 +56,15 @@ services:
|
||||
# Separate volume, exactly as in production: a keepsake archive contains every
|
||||
# photo in the event, so it is kept off the media tree.
|
||||
- exports_data:/exports
|
||||
# Mirror production's cap (docker-compose.yml). The test stack having NO memory limit is
|
||||
# why an unbounded image decode was invisible here: a 99 MP upload that would OOM-kill the
|
||||
# 1 GiB production container simply succeeded in CI. A test environment more generous than
|
||||
# production cannot catch a resource bug — the same shape as WebKit being absent from CI
|
||||
# and /health existing only in Caddyfile.test.
|
||||
deploy:
|
||||
resources:
|
||||
limits:
|
||||
memory: 1G
|
||||
expose:
|
||||
- '3000'
|
||||
|
||||
|
||||
94
e2e/specs/02-upload/oversized-image.spec.ts
Normal file
94
e2e/specs/02-upload/oversized-image.spec.ts
Normal file
@@ -0,0 +1,94 @@
|
||||
/**
|
||||
* Regression guard — an image that would blow the decode budget must be refused, not
|
||||
* allocated, and the container must survive it.
|
||||
*
|
||||
* The compression worker sets `max_alloc = 256 MiB`, but that budget was inert: reading the
|
||||
* EXIF orientation tag requires `ImageReader::into_decoder()`, which skips the
|
||||
* `limits.reserve(decoder.total_bytes())` that `decode()` performs, and nothing else enforces
|
||||
* it (the JPEG decoder's `set_limits` only checks support and dimensions). So the only real
|
||||
* bound was the 12000px per-axis cap — leaving 12000x12000 decodable at 412 MiB, and two
|
||||
* concurrent decodes at 824 MiB against a 1 GiB container.
|
||||
*
|
||||
* That mattered acutely because bumping DERIVATIVES_REV makes the first boot after a deploy
|
||||
* re-decode the whole gallery two at a time: an OOM kill there restarts the container, which
|
||||
* re-runs the backfill — a boot loop.
|
||||
*
|
||||
* This suite could never have caught it, because until now the e2e app container had NO
|
||||
* memory limit at all while production is capped at 1 GiB. The cap is mirrored in
|
||||
* docker-compose.test.yml so this test means something.
|
||||
*
|
||||
* Fixture: 11000x9000 = 99 MP, 568 KiB on disk. Deliberately UNDER the per-axis cap, so the
|
||||
* axis check cannot be what rejects it — 283 MiB decoded against a 256 MiB budget.
|
||||
*/
|
||||
import { test, expect } from '../../fixtures/test';
|
||||
import { uploadRaw } from '../../helpers/upload-client';
|
||||
import { BASE } from '../../helpers/env';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { join } from 'node:path';
|
||||
|
||||
const HUGE = join(process.cwd(), 'fixtures', 'media', 'huge-99mp.jpg');
|
||||
const SAMPLE = join(process.cwd(), 'fixtures', 'media', 'sample.jpg');
|
||||
|
||||
test.describe('Upload — an oversized image is refused, not allocated', () => {
|
||||
test('a 99 MP upload fails compression gracefully and the backend stays up', async ({
|
||||
guest,
|
||||
db,
|
||||
}) => {
|
||||
test.setTimeout(90_000);
|
||||
const g = await guest('BombThrower');
|
||||
|
||||
// The upload itself is accepted — 568 KiB is well within the body cap. The rejection
|
||||
// happens in the compression worker, where the decode budget lives.
|
||||
const res = await uploadRaw(g.jwt, readFileSync(HUGE), {
|
||||
filename: 'huge.jpg',
|
||||
contentType: 'image/jpeg',
|
||||
caption: 'zu gross',
|
||||
});
|
||||
expect(res.status, 'a 568 KiB file is a legitimate upload').toBe(201);
|
||||
const { id } = (await res.json()) as { id: string };
|
||||
|
||||
// It must land in 'failed', not 'done' — and must get there, rather than the container
|
||||
// dying mid-decode and leaving it stuck in 'processing' forever.
|
||||
await expect
|
||||
.poll(() => db.compressionStatus(id), { timeout: 60_000, intervals: [500] })
|
||||
.toBe('failed');
|
||||
|
||||
// The whole point: the process is still alive. An OOM kill would have taken the backend
|
||||
// down here, and Docker would have restarted it.
|
||||
const health = await fetch(`${BASE}/health`);
|
||||
expect(health.status, 'the backend must have survived the oversized decode').toBe(200);
|
||||
|
||||
// And it is still doing useful work afterwards — not wedged or restarting.
|
||||
const ok = await uploadRaw(g.jwt, readFileSync(SAMPLE), {
|
||||
filename: 'after.jpg',
|
||||
contentType: 'image/jpeg',
|
||||
});
|
||||
expect(ok.status).toBe(201);
|
||||
const after = (await ok.json()) as { id: string };
|
||||
await expect.poll(() => db.compressionStatus(after.id), { timeout: 30_000 }).toBe('done');
|
||||
});
|
||||
|
||||
test('two oversized uploads at once still leave the container alive', async ({ guest, db }) => {
|
||||
// The concurrent case is the one that actually OOM'd: `compression_concurrency` is 2, so
|
||||
// two decodes overlap. Under the old behaviour this pair peaked near the container cap.
|
||||
test.setTimeout(90_000);
|
||||
const g = await guest('BombThrower2');
|
||||
const bytes = readFileSync(HUGE);
|
||||
|
||||
const [a, b] = await Promise.all([
|
||||
uploadRaw(g.jwt, bytes, { filename: 'huge-a.jpg', contentType: 'image/jpeg' }),
|
||||
uploadRaw(g.jwt, bytes, { filename: 'huge-b.jpg', contentType: 'image/jpeg' }),
|
||||
]);
|
||||
expect([a.status, b.status]).toEqual([201, 201]);
|
||||
const ids = [((await a.json()) as { id: string }).id, ((await b.json()) as { id: string }).id];
|
||||
|
||||
for (const id of ids) {
|
||||
await expect
|
||||
.poll(() => db.compressionStatus(id), { timeout: 60_000, intervals: [500] })
|
||||
.toBe('failed');
|
||||
}
|
||||
|
||||
const health = await fetch(`${BASE}/health`);
|
||||
expect(health.status, 'two concurrent oversized decodes must not kill the backend').toBe(200);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user