From 606bb53c8bd98fa507b6ccc661dfb3b71f9959a2 Mon Sep 17 00:00:00 2001 From: Gianni Date: Sat, 10 Oct 2026 16:26:02 -0700 Subject: [PATCH 1/6] fix: initialize fuzz options and bound decoded image sizes Initialize the current C options layout, share a one-megapixel budget between the raw and end-to-end fuzzers, and pass it through the Rust and C decoder options. Check raw dimensions without allocating pixels while keeping malformed headers in the fuzzing domain. Correct the Wuffs Meson dependency name so the pinned fallback can be resolved. Keep the build fixes scoped as requested by the original review; do not add CI configuration or unrelated documentation changes. Validation: isolated fuzz budget tests and clippy; all four nightly fuzz targets build and pass seeded ASan smoke runs; default Meson setup resolves Wuffs successfully. References: https://github.com/halidecx/wpd/pull/36 https://github.com/halidecx/wpd/pull/36#pullrequestreview-5407902070 --- fuzz/budget.rs | 13 ++++++++++++ fuzz/fuzz_targets/container.rs | 1 - fuzz/fuzz_targets/e2e.rs | 9 +++++++++ fuzz/fuzz_targets/vp8.rs | 7 ++++++- fuzz/fuzz_targets/vp8l.rs | 7 ++++++- fuzz/tests/budget.rs | 36 ++++++++++++++++++++++++++++++++++ meson.build | 2 +- 7 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 fuzz/budget.rs create mode 100644 fuzz/tests/budget.rs diff --git a/fuzz/budget.rs b/fuzz/budget.rs new file mode 100644 index 0000000..5d85a34 --- /dev/null +++ b/fuzz/budget.rs @@ -0,0 +1,13 @@ +pub const MAX_PIXELS: u32 = 1 << 20; + +pub fn fits(data: &[u8]) -> bool { + // Raw codec fuzzers have no frame-size option. Read dimensions without + // allocating pixels, and still exercise headers that fail to parse. + wpd::api::info(data).map_or(true, |info| { + wpd::api::Options { + frame_size_limit: MAX_PIXELS, + ..Default::default() + } + .fits(info.width, info.height) + }) +} diff --git a/fuzz/fuzz_targets/container.rs b/fuzz/fuzz_targets/container.rs index ff1825b..85373cf 100644 --- a/fuzz/fuzz_targets/container.rs +++ b/fuzz/fuzz_targets/container.rs @@ -1,4 +1,3 @@ - #![no_main] use libfuzzer_sys::fuzz_target; diff --git a/fuzz/fuzz_targets/e2e.rs b/fuzz/fuzz_targets/e2e.rs index 9832126..70c0a75 100644 --- a/fuzz/fuzz_targets/e2e.rs +++ b/fuzz/fuzz_targets/e2e.rs @@ -11,6 +11,9 @@ use wpd_capi::decoder::{wpd_decode_into, WPDOutputBuffer}; use wpd_capi::frame::{WPDFrame, WPDOutputPlane}; use wpd_capi::options::WPDDecoderOptions; +#[path = "../budget.rs"] +mod budget; + const FORMATS: [Format; 16] = [ Format::Yuv420p, Format::Yuva420p, @@ -40,6 +43,7 @@ fn decode_options(data: &[u8]) -> (Options, bool) { let flags = byte(data, 1); let subframe = flags & 4 != 0; let mut options = Options { + frame_size_limit: budget::MAX_PIXELS, n_threads: [1, 2, 3, 8][usize::from(flags >> 6)], bypass_filtering: flags & 8 != 0, no_fancy_upsampling: flags & 16 != 0, @@ -123,6 +127,8 @@ fn decode_external(data: &[u8], options: Options) { flip: i32::from(options.flip), reserved: 0, n_threads: options.n_threads, + reserved2: 0, + frame_size_limit: options.frame_size_limit, }; let mut frame = WPDFrame { struct_size: mem::size_of::(), @@ -154,6 +160,9 @@ fn decode_external(data: &[u8], options: Options) { } fuzz_target!(|data: &[u8]| { + if !budget::fits(data) { + return; + } let Some(&first) = data.first() else { return; }; diff --git a/fuzz/fuzz_targets/vp8.rs b/fuzz/fuzz_targets/vp8.rs index dbe3ec7..f2794f6 100644 --- a/fuzz/fuzz_targets/vp8.rs +++ b/fuzz/fuzz_targets/vp8.rs @@ -1,10 +1,15 @@ - #![no_main] use libfuzzer_sys::fuzz_target; use wpd::vp8::Decoder; +#[path = "../budget.rs"] +mod budget; + fuzz_target!(|data: &[u8]| { + if !budget::fits(data) { + return; + } let mut decoder = Decoder::new(); let _ = decoder.decode_frame(data); diff --git a/fuzz/fuzz_targets/vp8l.rs b/fuzz/fuzz_targets/vp8l.rs index 48367bd..fc91499 100644 --- a/fuzz/fuzz_targets/vp8l.rs +++ b/fuzz/fuzz_targets/vp8l.rs @@ -1,14 +1,19 @@ - #![no_main] use libfuzzer_sys::fuzz_target; use wpd::vp8l::{AlphaDst, Decoder, Target}; +#[path = "../budget.rs"] +mod budget; + fuzz_target!(|data: &[u8]| { if data.len() < 2 { return; } let (head, payload) = data.split_at(2); + if !budget::fits(payload) { + return; + } let mut decoder = Decoder::new(); let alpha_chunk = head[0] & 1 != 0; diff --git a/fuzz/tests/budget.rs b/fuzz/tests/budget.rs new file mode 100644 index 0000000..75c2987 --- /dev/null +++ b/fuzz/tests/budget.rs @@ -0,0 +1,36 @@ +#[path = "../budget.rs"] +mod budget; + +#[test] +fn raw_dimensions_are_bounded_without_decoding_pixels() { + for (width, height, fits) in [ + (1024, 1024, true), + (1025, 1024, false), + (16384, 14336, false), + ] { + let bits = (width - 1) | (height - 1) << 14; + let mut lossless = vec![0x2f]; + + lossless.extend_from_slice(&u32::to_le_bytes(bits)); + assert_eq!(budget::fits(&lossless), fits); + if width < 16384 { + let mut lossy = vec![0x10, 0, 0, 0x9d, 1, 0x2a]; + + lossy.extend_from_slice(&(width as u16).to_le_bytes()); + lossy.extend_from_slice(&(height as u16).to_le_bytes()); + assert_eq!(budget::fits(&lossy), fits); + } + } +} + +#[test] +fn malformed_headers_remain_in_the_fuzzing_domain() { + for data in [ + &b""[..], + &b"RIFF"[..], + &[0x2f, 0, 0][..], + &b"not a header"[..], + ] { + assert!(budget::fits(data)); + } +} diff --git a/meson.build b/meson.build index f3eb390..66af0e2 100644 --- a/meson.build +++ b/meson.build @@ -517,7 +517,7 @@ custom_target( message('imagewebpdec: enabled (build explicitly with target imagewebpdec)') wuffs_dep = dependency( - '', + 'wuffs', fallback: ['wuffs', 'wuffs_dep'], required: get_option('wuffs'), ) From 2743d74c3f6b97807e27871f69039e58def46006 Mon Sep 17 00:00:00 2001 From: Gianni Date: Sat, 10 Oct 2026 16:26:28 -0700 Subject: [PATCH 2/6] fix: validate the first RIFF chunk and compare damaged decodes Reject a RIFF WebP whose first chunk is not VP8, VP8L or VP8X, including incremental input. Add RGBA PAM output to the pinned libwebp harness and a standalone Python comparison tool that reports acceptance, geometry, visible pixels and RGB beneath zero alpha separately. Record input and output hashes, tool versions, abnormal exits and timeout failures. Preserve declared payload bounds and existing decoder arithmetic rather than introducing the compatibility changes criticized in PR #37. The damaged-input corpus and provenance are published separately in wpd-test-data commit 54b2d0e2876c4ca223e29a47c1871bad1ac6ad56. Validation: first-chunk unit and streamed CLI regressions; exact RGBA and geometry agreement on 49 valid files; 19 damaged files compared with no abnormal decoder exits. References: https://github.com/halidecx/wpd/pull/37 https://github.com/halidecx/wpd/pull/37#pullrequestreview-5407953971 --- scripts/webpcompare.py | 168 +++++++++++++++++++++++++++++++++++++++++ src/container.rs | 23 ++++++ tools/libwebpdec.c | 66 +++++++++++----- 3 files changed, 238 insertions(+), 19 deletions(-) create mode 100644 scripts/webpcompare.py diff --git a/scripts/webpcompare.py b/scripts/webpcompare.py new file mode 100644 index 0000000..7c62f52 --- /dev/null +++ b/scripts/webpcompare.py @@ -0,0 +1,168 @@ +#!/usr/bin/env python3 +"""Compare WebP acceptance, frame geometry and RGBA pixels with libwebpdec. + +Build the pinned reference with `meson compile -C build libwebpdec`. +Directories contribute their .webp files recursively; explicit files may be +extensionless fuzz artifacts. Outputs stay under wpd-test-data. Exit 1 means an +acceptance, geometry, visible-pixel or abnormal-exit difference; RGB beneath +fully transparent pixels is reported separately and does not fail the run. +""" + +import argparse +from collections import Counter +import hashlib +import json +import math +from pathlib import Path +import subprocess +import tempfile + + +def digest(path): + result = hashlib.sha256() + with path.open("rb") as source: + for data in iter(lambda: source.read(65536), b""): + result.update(data) + return result.hexdigest() + + +def run(command, log, timeout): + with log.open("wb") as stderr: + try: + status = subprocess.run(command, stdout=subprocess.DEVNULL, + stderr=stderr, timeout=timeout).returncode + except subprocess.TimeoutExpired: + return "timeout", "wall-clock limit exceeded" + with log.open("rb") as stderr: + stderr.seek(max(0, log.stat().st_size - 4096)) + return status, stderr.read().decode("utf-8", "replace").strip() + + +def header(source): + magic = source.readline(4) + if not magic: + return None + if magic != b"P7\n": + raise ValueError("invalid PAM magic") + fields = {} + for _ in range(16): + line = source.readline(256) + if line == b"ENDHDR\n": + break + key, value = line.rstrip(b"\n").split(b" ", 1) + if key in fields: + raise ValueError("duplicate PAM field") + fields[key] = value + else: + raise ValueError("invalid PAM header") + if (fields[b"DEPTH"] != b"4" or fields[b"MAXVAL"] != b"255" or + fields[b"TUPLTYPE"] != b"RGB_ALPHA"): + raise ValueError("expected straight-RGBA PAM") + width, height = int(fields[b"WIDTH"]), int(fields[b"HEIGHT"]) + if not 0 < width <= 16384 or not 0 < height <= 16384: + raise ValueError("invalid PAM dimensions") + return width, height + + +def compare(a_path, b_path): + frames = visible = transparent = 0 + with a_path.open("rb") as a, b_path.open("rb") as b: + while True: + a_size, b_size = header(a), header(b) + if a_size != b_size: + return "geometry", frames, visible, transparent + if a_size is None: + if not frames: + raise ValueError("success without frames") + break + frames += 1 + remaining = a_size[0] * a_size[1] * 4 + while remaining: + size = min(remaining, 65536) + aa, bb = a.read(size), b.read(size) + if len(aa) != size or len(bb) != size: + raise ValueError("truncated PAM pixels") + remaining -= size + if aa == bb: + continue + for i in range(0, size, 4): + if aa[i:i + 4] == bb[i:i + 4]: + continue + if aa[i + 3] == bb[i + 3] == 0: + transparent += 1 + else: + visible += 1 + outcome = "visible" if visible else "transparent" if transparent else "identical" + return outcome, frames, visible, transparent + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("inputs", nargs="+", type=Path) + parser.add_argument("--wpd", type=Path, default=Path("build/wpd")) + parser.add_argument("--libwebp", type=Path, default=Path("build/libwebpdec")) + parser.add_argument("--timeout", type=float, default=60) + parser.add_argument("--work-dir", type=Path, default=Path("wpd-test-data")) + args = parser.parse_args() + if not math.isfinite(args.timeout) or args.timeout <= 0: + parser.error("timeout must be positive") + tools = [args.wpd.resolve(), args.libwebp.resolve()] + if any(not tool.is_file() for tool in tools): + parser.error("build both wpd and libwebpdec first") + files = set() + for path in args.inputs: + if path.is_file(): + files.add(path.resolve()) + elif path.is_dir(): + files.update(p.resolve() for p in path.rglob("*.webp") if p.is_file()) + else: + parser.error(f"input does not exist: {path}") + if not files: + parser.error("no input files") + args.work_dir.mkdir(parents=True, exist_ok=True) + counts = Counter() + versions = None + for path in sorted(files): + record = {"path": str(path), "input_sha256": digest(path)} + with tempfile.TemporaryDirectory(prefix="webpcompare-", dir=args.work_dir) as td: + work = Path(td) + outputs = [work / "wpd.pam", work / "libwebp.pam"] + results = [run([str(tool), "--fmt", "rgba", "--muxer", "pam", + str(path), str(output)], work / f"{i}.log", args.timeout) + for i, (tool, output) in enumerate(zip(tools, outputs))] + if versions is None: + versions = [error.splitlines()[0] if error else "" for _, error in results] + for name, (status, error) in zip(["wpd", "libwebp"], results): + record[f"{name}_status"] = status + if status: + record[f"{name}_error"] = error + statuses = [status for status, _ in results] + if any(status not in (0, 1) for status in statuses): + outcome = "abnormal" + elif statuses == [0, 0]: + try: + outcome, frames, visible, transparent = compare(*outputs) + record.update(frames=frames, visible_pixels=visible, + transparent_rgb_pixels=transparent, + wpd_output_sha256=digest(outputs[0]), + libwebp_output_sha256=digest(outputs[1])) + except (KeyError, ValueError) as error: + outcome = "abnormal" + record["output_error"] = str(error) + else: + outcome = ("wpd_only" if statuses[0] == 0 else + "libwebp_only" if statuses[1] == 0 else "neither") + record["outcome"] = outcome + counts[outcome] += 1 + print(json.dumps(record), flush=True) + print(json.dumps({"summary": dict(counts), "files": len(files), + "tools": [str(tool) for tool in tools], "versions": versions})) + return int(any(counts[key] for key in ("visible", "geometry", "wpd_only", + "libwebp_only", "abnormal"))) + + +if __name__ == "__main__": + try: + raise SystemExit(main()) + except (OSError, subprocess.SubprocessError) as error: + raise SystemExit(str(error)) diff --git a/src/container.rs b/src/container.rs index eb31075..3ec0d3a 100644 --- a/src/container.rs +++ b/src/container.rs @@ -544,6 +544,10 @@ impl Scan { let tag = rl32(buf, at); let size = rl32(buf, at + 4); + if self.pos == 12 && !matches!(tag, TAG_VP8X | TAG_VP8 | TAG_VP8L) { + log::error("RIFF must start with VP8, VP8L or VP8X"); + return Err(Error::InvalidData); + } if size == u32::MAX { self.info.truncated = true; break; @@ -751,6 +755,25 @@ mod tests { assert_eq!(get_info(b"not a webp file at all"), Err(Error::NotWebp)); } + #[test] + fn an_unknown_first_chunk_cannot_hide_the_extended_header() { + let mut payload = chunk(b"VP9X", &[0x10, 0, 0, 0, 0, 0, 0, 0, 0, 0]); + + payload.extend(chunk(b"VP8L", &vp8l_header(1, 1, true))); + let file = riff(&payload); + + assert_eq!(get_info(&file), Err(Error::InvalidData)); + for split in 12..20 { + let mut scan = Scan::new(); + + assert_eq!( + scan.headers(&file[..split], 0, true, true), + Err(Error::Truncated) + ); + assert_eq!(scan.headers(&file, 0, true, true), Err(Error::InvalidData)); + } + } + #[test] fn oversized_vp8x_dimensions_are_refused_before_allocating() { let payload = chunk(b"VP8X", &[2, 0, 0, 0, 0, 64, 0, 0, 0, 0]); diff --git a/tools/libwebpdec.c b/tools/libwebpdec.c index 5c5965a..d4f6437 100644 --- a/tools/libwebpdec.c +++ b/tools/libwebpdec.c @@ -16,6 +16,7 @@ static const struct option long_options[] = { {"repeat", required_argument, NULL, 'r'}, {"fmt", required_argument, NULL, 'f'}, {"threads", required_argument, NULL, 't'}, + {"muxer", required_argument, NULL, 'm'}, {NULL, 0, NULL, 0}, }; @@ -81,20 +82,23 @@ static void print_banner(void) { static void usage(const char *app, const char *reason) { if (reason) fprintf(stderr, "\n%s\n", reason); - fprintf(stderr, - "\nusage: %s [options] input output\n" - "\noptions:\n" - " -h, --help\n" - " view help menu\n" - " -r, --repeat u32\n" - " repeat decode for benchmarking (1..INT_MAX); default 1\n" - " -f, --fmt str\n" - " output pixel format; default auto. one of\n" - " auto, yuv420p, yuva420p,\n" - " argb, rgba, bgra, rgb, bgr, Argb, rgbA, bgrA\n" - " -t, --threads 1|2\n" - " 1 disables threading; 2 enables libwebp's worker; default 1\n", - app); + fprintf( + stderr, + "\nusage: %s [options] input output\n" + "\noptions:\n" + " -h, --help\n" + " view help menu\n" + " -r, --repeat u32\n" + " repeat decode for benchmarking (1..INT_MAX); default 1\n" + " -f, --fmt str\n" + " output pixel format; default auto. one of\n" + " auto, yuv420p, yuva420p,\n" + " argb, rgba, bgra, rgb, bgr, Argb, rgbA, bgrA\n" + " -t, --threads 1|2\n" + " 1 disables threading; 2 enables libwebp's worker; default 1\n" + " --muxer raw|pam\n" + " default raw; pam requires rgba and preserves frame dimensions\n", + app); } static int parse_repeat(const char *value, int *repeat) { @@ -170,9 +174,16 @@ static const char *format_name(const Frame *frame) { } static int write_frame(FILE *output, const Frame *frame, - const char *pixel_format) { + const char *pixel_format, int pam) { int planes; + if (pam && + fprintf(output, + "P7\nWIDTH %d\nHEIGHT %d\nDEPTH 4\nMAXVAL 255\n" + "TUPLTYPE RGB_ALPHA\nENDHDR\n", + frame->width, + frame->height) < 0) + return -1; if (!pixel_format) pixel_format = format_name(frame); @@ -238,7 +249,7 @@ static const char *status_name(VP8StatusCode status) { static int decode_still(const char *input_name, const uint8_t *data, size_t size, const WebPBitstreamFeatures *features, FILE *sink, const char *pixel_format, int threads, - int *frames) { + int pam, int *frames) { WebPDecoderConfig config; VP8StatusCode status; Frame frame = {0}; @@ -288,7 +299,7 @@ static int decode_still(const char *input_name, const uint8_t *data, frame.stride[3] = config.output.u.YUVA.a_stride; } - if (sink && write_frame(sink, &frame, pixel_format) < 0) { + if (sink && write_frame(sink, &frame, pixel_format, pam) < 0) { WebPFreeDecBuffer(&config.output); return -1; } @@ -299,7 +310,7 @@ static int decode_still(const char *input_name, const uint8_t *data, static int decode_animation(const char *input_name, const uint8_t *data, size_t size, FILE *sink, const char *pixel_format, - int threads, int *frames) { + int threads, int pam, int *frames) { WebPData webp_data = {data, size}; WebPAnimDecoderOptions options; WebPAnimDecoder *decoder = NULL; @@ -350,7 +361,7 @@ static int decode_animation(const char *input_name, const uint8_t *data, goto done; } frame.data[0] = rgba; - if (sink && write_frame(sink, &frame, pixel_format) < 0) + if (sink && write_frame(sink, &frame, pixel_format, pam) < 0) goto done; (*frames)++; } @@ -399,6 +410,7 @@ int main(int argc, char **argv) { const char *input_name, *output_name; int discard_output, frames = 0, repeat = 1, status = 1; int threads = 1; + int pam = 0; print_banner(); opterr = 0; @@ -426,6 +438,13 @@ int main(int argc, char **argv) { return 2; } break; + case 'm': + if (strcmp(optarg, "raw") && strcmp(optarg, "pam")) { + usage(argv[0], "invalid output muxer; expected raw or pam"); + return 2; + } + pam = !strcmp(optarg, "pam"); + break; default: usage(argv[0], "unknown option or missing option value"); return 2; @@ -437,6 +456,13 @@ int main(int argc, char **argv) { : "unexpected argument"); return 2; } + if (pam) { + if (pixel_format && strcmp(pixel_format, "rgba")) { + usage(argv[0], "pam requires rgba output"); + return 2; + } + pixel_format = "rgba"; + } input_name = argv[optind]; output_name = argv[optind + 1]; @@ -471,6 +497,7 @@ int main(int argc, char **argv) { sink, pixel_format, threads, + pam, &frames) < 0) goto done; } else if (decode_still(input_name, @@ -480,6 +507,7 @@ int main(int argc, char **argv) { sink, pixel_format, threads, + pam, &frames) < 0) { goto done; } From 1ba7a2489aa686e65bfd073171251f6dd1778742 Mon Sep 17 00:00:00 2001 From: Gianni Date: Sat, 10 Oct 2026 16:26:31 -0700 Subject: [PATCH 3/6] feat: report JSON info and safely extract binary metadata Add --info=json with canvas, coding, frame count, loop count, metadata presence and original frame durations, plus --icc-out, --exif-out and --xmp-out for exact metadata bytes. Emit reports once after decoding, pixel output closure and optional MD5 verification succeed. Missing metadata produces an empty file. Validate input/output identities before opening pixel output, including symlinks, hardlinks and redirected Unix streams. Stage metadata in same-directory temporary files before atomic publication, preserve valid destination symlinks, and recheck newly-created filesystem aliases to prevent silent output collisions. Keep reporting out of the ordinary decode loop through compile-time specialization. Add Meson CLI regressions using fixtures published in wpd-test-data commit 54b2d0e2876c4ca223e29a47c1871bad1ac6ad56. Tests cover whole/stream input, binary metadata, repeats, loops, scaling, output failures and filesystem aliases. Validation: 16 CLI cases in assembly and scalar builds (the Linux-only filename-byte case is skipped on APFS); 1,271 existing CLI comparisons agree; text info is unchanged on 49 files in whole and stream modes; paired 30,000-repeat benchmarks show no slowdown in four configurations. References: https://github.com/halidecx/wpd/pull/38 https://github.com/halidecx/wpd/pull/38#pullrequestreview-5407958017 --- tests/cli.py | 267 +++++++++++++++++++++++++++++++++++++++++ tests/meson.build | 4 + tools/report.rs | 299 ++++++++++++++++++++++++++++++++++++++++++++++ tools/wpd.rs | 144 ++++++++++++++++++---- 4 files changed, 690 insertions(+), 24 deletions(-) create mode 100644 tests/cli.py create mode 100644 tools/report.rs diff --git a/tests/cli.py b/tests/cli.py new file mode 100644 index 0000000..44f2bbf --- /dev/null +++ b/tests/cli.py @@ -0,0 +1,267 @@ +#!/usr/bin/env python3 +"""CLI reporting regressions; all writable artifacts stay in the test-data dir.""" + +import argparse +import importlib.util +import json +import os +from pathlib import Path +import subprocess +import sys +import tempfile +import unittest + + +class Cli(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory(prefix="cli-", dir=CORPUS) + self.addCleanup(self.temp.cleanup) + self.work = Path(self.temp.name) + self.input = self.work / "input.bin" + self.input.write_bytes((CORPUS / "metadata_binary.webp").read_bytes()) + + def command(self, *args, input=None, stdout=subprocess.PIPE): + return subprocess.run([str(WPD), *map(os.fspath, args)], input=input, + stdout=stdout, stderr=subprocess.PIPE, timeout=30) + + def success(self, result): + self.assertEqual(result.returncode, 0, result.stderr.decode("utf-8", "replace")) + + def test_json_uses_original_info_and_emits_once(self): + source = CORPUS / "durations.webp" + expected = dict(width=100, height=80, coding="unknown", is_animation=True, + frame_count=4, loop_count=7, has_alpha=True, has_icc=False, + has_exif=False, has_xmp=False, + durations_ms=[1, 0, 65535, 16777215]) + for stream in [[], ["--stream", "1"], ["--stream", "997"]]: + with self.subTest(stream=stream): + result = self.command("--info=json", "--repeat", "2", "--loops", "2", + "--scale", "50x40", *stream, source) + self.success(result) + self.assertEqual(json.loads(result.stdout), expected) + self.assertEqual(result.stdout.count(b"\n"), 1) + result = self.command("--info=json", "--stream", "1", "-", input=source.read_bytes()) + self.success(result) + self.assertEqual(json.loads(result.stdout), expected) + + def test_binary_metadata_is_exact_for_whole_stream_and_stdin(self): + paths = [self.work / name for name in ["profile.icc", "image.exif", "image.xmp"]] + expected = [b"ICC\0\xffprofile", b"Exif\0\0\x01\xff\x80", b"\0\xfe"] + flags = [value for flag, path in zip(["--icc-out", "--exif-out", "--xmp-out"], paths) + for value in (flag, path)] + for stream in [[], ["--stream", "1"], ["--stream", "997"]]: + result = self.command("--info=json", "--repeat", "2", "--loops", "2", + *flags, *stream, self.input) + self.success(result) + info = json.loads(result.stdout) + self.assertEqual((info["width"], info["height"], info["durations_ms"]), (1, 1, [0])) + self.assertTrue(all(info[key] for key in ["has_icc", "has_exif", "has_xmp"])) + self.assertEqual([p.read_bytes() for p in paths], expected) + result = self.command(*flags, "--stream", "1", "-", input=self.input.read_bytes()) + self.success(result) + self.assertEqual(result.stdout, b"") + self.assertEqual([p.read_bytes() for p in paths], expected) + self.assertEqual(list(self.work.glob("*.part")), []) + + def test_missing_metadata_creates_empty_files(self): + for flag in ["--icc-out", "--exif-out", "--xmp-out"]: + path = self.work / flag + path.write_bytes(b"old metadata") + result = self.command(flag, path, CORPUS / "odd_lossy.webp") + self.success(result) + self.assertEqual(path.read_bytes(), b"") + + def test_raw_lossless_json(self): + source = self.work / "raw.bin" + source.write_bytes(bytes.fromhex("2f00000000071011118888fe0700")) + result = self.command("--info=json", source) + self.success(result) + info = json.loads(result.stdout) + self.assertEqual((info["width"], info["height"], info["frame_count"]), (1, 1, 1)) + self.assertFalse(info["is_animation"]) + + def test_failed_decoding_and_pixel_output_leave_metadata_untouched(self): + metadata = self.work / "profile.icc" + for source, flags in [ + (CORPUS / "damaged" / "truncated-metadata.webp", []), + (CORPUS / "damaged" / "truncated-lossless.webp", []), + (self.input, ["--max-output", "1", "--muxer", "pam"]), + ]: + with self.subTest(source=source, flags=flags): + metadata.write_bytes(b"preserve me") + result = self.command("--info=json", "--icc-out", metadata, + *flags, source, self.work / "pixels.pam") + self.assertEqual(result.returncode, 1, result.stderr) + self.assertEqual(result.stdout, b"") + self.assertEqual(metadata.read_bytes(), b"preserve me") + self.assertEqual(list(self.work.glob("*.part")), []) + + def test_verification_must_succeed_before_reporting(self): + metadata = self.work / "profile.icc" + metadata.write_bytes(b"preserve me") + result = self.command("--info=json", "--icc-out", metadata, + "--verify", "0" * 32, self.input) + self.assertEqual(result.returncode, 1, result.stderr) + self.assertEqual(result.stdout, b"") + self.assertEqual(metadata.read_bytes(), b"preserve me") + + def test_collisions_are_rejected_before_truncating(self): + pixel = self.work / "pixels.raw" + alias_dir = self.work / "alias" + alias_dir.mkdir() + for metadata in [pixel, self.work / "." / "pixels.raw", alias_dir / ".." / "pixels.raw"]: + pixel.write_bytes(b"preserve pixels") + result = self.command("--icc-out", metadata, self.input, pixel) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(pixel.read_bytes(), b"preserve pixels") + pixel.unlink() + result = self.command("--icc-out", pixel, self.input, pixel) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertFalse(pixel.exists()) + original = self.input.read_bytes() + for args in [("--icc-out", self.input, self.input), + ("--info=json", self.input, self.input)]: + result = self.command(*args) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(self.input.read_bytes(), original) + pixel.write_bytes(b"preserve metadata") + result = self.command("--icc-out", pixel, "--exif-out", pixel, self.input) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(pixel.read_bytes(), b"preserve metadata") + + @unittest.skipUnless(os.name == "posix", "Unix link identities") + def test_symlinks_and_hardlinks_cannot_hide_collisions(self): + pixel = self.work / "pixels.raw" + pixel.write_bytes(b"preserve pixels") + for link in ["symlink", "hardlink"]: + alias = self.work / link + if link == "symlink": + alias.symlink_to(pixel) + else: + os.link(pixel, alias) + result = self.command("--icc-out", alias, self.input, pixel) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(pixel.read_bytes(), b"preserve pixels") + result = self.command("--icc-out", pixel, "--exif-out", alias, self.input) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(pixel.read_bytes(), b"preserve pixels") + target = self.work / "metadata.icc" + target.write_bytes(b"old") + alias = self.work / "metadata-link" + alias.symlink_to(target) + result = self.command("--icc-out", alias, self.input) + self.success(result) + self.assertTrue(alias.is_symlink()) + self.assertEqual(target.read_bytes(), b"ICC\0\xffprofile") + + def test_stdout_cannot_mix_json_and_pixels(self): + for alias in ["-", "/dev/stdout", "/dev/fd/1", "/proc/self/fd/1"]: + result = self.command("--info=json", self.input, alias) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(result.stdout, b"") + result = self.command("--icc-out", alias, self.input) + self.assertEqual(result.returncode, 2, result.stderr) + if os.name == "posix": + alias = self.work / "stdout-link" + alias.symlink_to("/dev/stdout") + result = self.command("--info=json", self.input, alias) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(result.stdout, b"") + target = self.work / "stdout-file" + target.write_bytes(b"preserve stdout") + with target.open("ab") as stdout: + result = self.command("--info=json", self.input, target, stdout=stdout) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(target.read_bytes(), b"preserve stdout") + + def test_invalid_destinations_and_arguments(self): + for args in [("--info=xml", self.input), ("--icc-out=", self.input), + ("--icc-out", self.work, self.input)]: + result = self.command(*args) + self.assertEqual(result.returncode, 2, result.stderr) + metadata = self.work / "profile.icc" + metadata.write_bytes(b"preserve me") + result = self.command("--icc-out", metadata, "--exif-out", self.work / "missing" / "file", self.input) + self.assertEqual(result.returncode, 1, result.stderr) + self.assertEqual(metadata.read_bytes(), b"preserve me") + + def test_new_case_aliases_do_not_overwrite_completed_outputs(self): + probe = self.work / "case-probe" + probe.write_bytes(b"probe") + if not (self.work / "CASE-PROBE").exists(): + self.skipTest("case-sensitive filesystem") + pixel = self.work / "pixels.raw" + alias = self.work / "PIXELS.RAW" + expected = self.command("--fmt", "rgba", self.input, "-") + self.success(expected) + result = self.command("--fmt", "rgba", "--icc-out", alias, self.input, pixel) + self.assertNotEqual(result.returncode, 0, result.stderr) + self.assertEqual(pixel.read_bytes(), expected.stdout) + a, b = self.work / "metadata", self.work / "METADATA" + result = self.command("--icc-out", a, "--exif-out", b, self.input) + self.assertNotEqual(result.returncode, 0, result.stderr) + self.assertEqual(a.read_bytes(), b"ICC\0\xffprofile") + + @unittest.skipUnless(os.name == "posix", "Unix file descriptors") + def test_redirected_stdin_is_not_a_metadata_or_pixel_destination(self): + original = self.input.read_bytes() + for flags in [["--icc-out", str(self.input), "-"], + ["--info=json", "-", str(self.input)]]: + with self.input.open("rb") as stdin: + result = subprocess.run([str(WPD), *flags], stdin=stdin, + capture_output=True, timeout=30) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(self.input.read_bytes(), original) + + def test_unicode_metadata_filename(self): + dest = self.work / "profile-Ω.icc" + result = self.command("--icc-out", dest, self.input) + self.success(result) + self.assertEqual(dest.read_bytes(), b"ICC\0\xffprofile") + + @unittest.skipUnless(sys.platform.startswith("linux"), "Linux filename bytes; APFS requires valid Unicode") + def test_non_utf8_metadata_path_is_preserved(self): + name = os.fsencode(self.work) + b"/profile-\xff.icc" + result = self.command("--icc-out", name, self.input) + self.success(result) + with open(name, "rb") as source: + self.assertEqual(source.read(), b"ICC\0\xffprofile") + + def test_first_riff_chunk_is_validated_in_whole_and_split_input(self): + for name in ["invalid-first-vp8x.webp", "invalid-first-junk.webp", "short-first-partition.webp"]: + source = CORPUS / "damaged" / name + for stream in [[], ["--stream", "1"], ["--stream", "19"]]: + result = self.command("--info=json", *stream, source) + self.assertEqual(result.returncode, 1, result.stderr) + self.assertEqual(result.stdout, b"") + + def test_comparison_distinguishes_visible_transparent_and_geometry(self): + sys.dont_write_bytecode = True + path = Path(__file__).resolve().parents[1] / "scripts" / "webpcompare.py" + spec = importlib.util.spec_from_file_location("webpcompare", path) + compare = importlib.util.module_from_spec(spec) + spec.loader.exec_module(compare) + a, b = self.work / "a.pam", self.work / "b.pam" + + def pam(width, pixels): + return (f"P7\nWIDTH {width}\nHEIGHT 1\nDEPTH 4\nMAXVAL 255\nTUPLTYPE RGB_ALPHA\nENDHDR\n".encode() + pixels) + + a.write_bytes(pam(1, b"\x01\x02\x03\0")) + b.write_bytes(pam(1, b"\x03\x02\x01\0")) + self.assertEqual(compare.compare(a, b), ("transparent", 1, 0, 1)) + b.write_bytes(pam(1, b"\x03\x02\x01\xff")) + self.assertEqual(compare.compare(a, b), ("visible", 1, 1, 0)) + b.write_bytes(pam(2, bytes(8))) + self.assertEqual(compare.compare(a, b)[0], "geometry") + b.write_bytes(pam(1, b"\x01")) + with self.assertRaises(ValueError): + compare.compare(a, b) + + +if __name__ == "__main__": + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--wpd", type=Path, required=True) + parser.add_argument("--corpus", type=Path, required=True) + args, remaining = parser.parse_known_args() + WPD, CORPUS = args.wpd.resolve(), args.corpus.resolve() + unittest.main(argv=[__file__, *remaining]) diff --git a/tests/meson.build b/tests/meson.build index 7536ff0..bf30b11 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -130,6 +130,10 @@ testdata = [ ] test('api-testdata', api_test, suite: ['testdata', 'api'], args: [testdata_dir]) +test('cli-reporting', find_program('python3'), + suite: ['testdata', 'cli'], + args: [files('cli.py'), '--wpd', wpd, '--corpus', testdata_dir], + depends: wpd, timeout: 120) test('parity', parity_test, suite: ['testdata', 'parity'], args: [testdata_dir], timeout: 300) test('stream-reopen', wpd, suite: ['testdata', 'testdata-stream'], diff --git a/tools/report.rs b/tools/report.rs new file mode 100644 index 0000000..a60b1e0 --- /dev/null +++ b/tools/report.rs @@ -0,0 +1,299 @@ +use std::ffi::OsStr; +use std::fs::{self, File, OpenOptions}; +use std::io::{self, Write}; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +use wpd::api::{Decoder, Metadata}; + +pub fn is_stdout(path: &OsStr) -> bool { + ["-", "/dev/stdout", "/dev/fd/1", "/proc/self/fd/1"] + .iter() + .any(|alias| path == OsStr::new(alias)) +} + +fn same_file(a: &Path, b: &Path) -> bool { + if a == b { + return true; + } + let resolve = |p: &Path| { + p.canonicalize().or_else(|e| { + if e.kind() != io::ErrorKind::NotFound { + return Err(e); + } + let parent = p + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or(Path::new(".")); + let name = p.file_name().ok_or(e)?; + + Ok(parent.canonicalize()?.join(name)) + }) + }; + if let (Ok(a), Ok(b)) = (resolve(a), resolve(b)) { + if a == b { + return true; + } + } + #[cfg(unix)] + if let (Ok(a), Ok(b)) = (fs::metadata(a), fs::metadata(b)) { + use std::os::unix::fs::MetadataExt; + + return a.dev() == b.dev() && a.ino() == b.ino(); + } + false +} + +#[cfg(unix)] +fn same_handle(path: &Path, handle: &impl std::os::fd::AsFd) -> bool { + use std::os::unix::fs::MetadataExt; + + let Ok(fd) = handle.as_fd().try_clone_to_owned() else { + return false; + }; + let file = File::from(fd); + + if let (Ok(a), Ok(b)) = (fs::metadata(path), file.metadata()) { + a.dev() == b.dev() && a.ino() == b.ino() + } else { + false + } +} + +fn stdout_alias(path: &Path) -> bool { + if same_file(path, Path::new("/dev/stdout")) { + return true; + } + #[cfg(unix)] + if same_handle(path, &io::stdout()) { + return true; + } + false +} + +fn input_alias(path: &Path, input: &Path) -> bool { + if same_file(path, input) { + return true; + } + #[cfg(unix)] + if input == Path::new("/dev/stdin") && same_handle(path, &io::stdin()) { + return true; + } + false +} + +fn destination(path: &Path) -> io::Result { + match fs::metadata(path) { + Ok(meta) if meta.is_file() => path.canonicalize(), + Ok(_) => Err(io::Error::new( + io::ErrorKind::InvalidInput, + "metadata output must name a regular file", + )), + Err(e) if e.kind() == io::ErrorKind::NotFound => { + // Refuse dangling links rather than replace the link itself. + if fs::symlink_metadata(path).is_ok() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "metadata output is a dangling link", + )); + } + let parent = path + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or(Path::new(".")); + let name = path.file_name().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + "metadata output requires a file name", + ) + })?; + + Ok(parent.canonicalize()?.join(name)) + } + Err(e) => Err(e), + } +} + +pub fn validate_paths( + input: &OsStr, + output: Option<&OsStr>, + json: bool, + metadata: &mut [Option; 3], +) -> io::Result<()> { + let stdout = Path::new("/dev/stdout"); + let input = if input == OsStr::new("-") { + Path::new("/dev/stdin") + } else { + Path::new(input) + }; + let output = output.map(|p| if is_stdout(p) { stdout } else { Path::new(p) }); + + if json && output.is_some_and(stdout_alias) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "JSON info and decoded output cannot both use stdout", + )); + } + if output.is_some_and(|p| input_alias(p, input)) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "decoded output collides with input", + )); + } + let mut destinations: [Option; 3] = Default::default(); + + for (i, path) in metadata.iter().enumerate() { + let Some(path) = path else { continue }; + let dest = destination(path)?; + + if input_alias(&dest, input) + || output.is_some_and(|p| same_file(&dest, p)) + || stdout_alias(&dest) + || same_file(&dest, Path::new("/dev/stderr")) + || destinations.iter().flatten().any(|p| same_file(&dest, p)) + { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "metadata output collides with input or another output", + )); + } + #[cfg(unix)] + if same_handle(&dest, &io::stderr()) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "metadata output collides with an input or output stream", + )); + } + destinations[i] = Some(dest); + } + *metadata = destinations; + Ok(()) +} + +fn json_info(decoder: &mut Decoder<'_>) -> io::Result> { + let image = decoder.info().map_err(io::Error::other)?; + let mut json = Vec::new(); + + write!(json, "{{\"width\":{},\"height\":{},\"coding\":\"{}\",\"is_animation\":{},\"frame_count\":{},\"loop_count\":{},\"has_alpha\":{},\"has_icc\":{},\"has_exif\":{},\"has_xmp\":{},\"durations_ms\":[", + image.width, image.height, image.coding.name(), image.is_animation, + image.frame_count, image.loop_count, image.has_alpha, + decoder.metadata(Metadata::Iccp).is_some(), + decoder.metadata(Metadata::Exif).is_some(), + decoder.metadata(Metadata::Xmp).is_some())?; + for i in 0..image.frame_count { + let duration = if image.is_animation { + decoder.frame_info(i).map_err(io::Error::other)?.duration + } else { + 0 + }; + + write!(json, "{}{duration}", if i == 0 { "" } else { "," })?; + } + json.extend_from_slice(b"]}\n"); + Ok(json) +} + +struct Pending { + path: PathBuf, + dest: PathBuf, +} + +impl Pending { + fn new(dest: &Path, data: &[u8]) -> io::Result { + static NEXT: AtomicU64 = AtomicU64::new(0); + + for _ in 0..64 { + let mut name = dest.file_name().unwrap().to_os_string(); + + name.push(format!( + ".{}.{}.part", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + )); + let path = dest.with_file_name(name); + let file = OpenOptions::new().write(true).create_new(true).open(&path); + + let mut file: File = match file { + Ok(f) => f, + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e), + }; + let pending = Self { + path, + dest: dest.to_owned(), + }; + + file.write_all(data)?; + file.flush()?; + if let Ok(meta) = fs::metadata(dest) { + file.set_permissions(meta.permissions())?; + } + return Ok(pending); + } + Err(io::Error::new( + io::ErrorKind::AlreadyExists, + "cannot create metadata temporary file", + )) + } + + fn publish(&self) -> io::Result<()> { + fs::rename(&self.path, &self.dest) + } +} + +impl Drop for Pending { + fn drop(&mut self) { + let _ = fs::remove_file(&self.path); + } +} + +pub fn write( + decoder: &mut Decoder<'_>, + json: bool, + metadata: &[Option; 3], + input: &OsStr, + output: Option<&OsStr>, +) -> io::Result<()> { + // A newly created pixel file can reveal aliases that did not exist at + // argument validation, including case-insensitive filesystem names. + let mut metadata = metadata.clone(); + + validate_paths(input, output, json, &mut metadata)?; + let json = if json { + Some(json_info(decoder)?) + } else { + None + }; + let mut pending = Vec::new(); + + for (kind, path) in [Metadata::Iccp, Metadata::Exif, Metadata::Xmp] + .into_iter() + .zip(&metadata) + { + if let Some(path) = path { + pending.push(Pending::new( + path, + decoder.metadata(kind).unwrap_or_default(), + )?); + } + } + for (i, file) in pending.iter().enumerate() { + if pending[..i] + .iter() + .any(|previous| same_file(&file.dest, &previous.dest)) + { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "metadata outputs resolve to the same file", + )); + } + file.publish()?; + } + if let Some(json) = json { + let mut stdout = io::stdout().lock(); + + stdout.write_all(&json)?; + stdout.flush()?; + } + Ok(()) +} diff --git a/tools/wpd.rs b/tools/wpd.rs index d54e2c5..6a234ec 100644 --- a/tools/wpd.rs +++ b/tools/wpd.rs @@ -2,9 +2,11 @@ mod md5; mod output; +mod report; use std::ffi::{OsStr, OsString}; use std::io::{Read, Write}; +use std::path::PathBuf; use std::process::ExitCode; use wpd::api::{self, Animation, Decoder, Metadata}; @@ -99,6 +101,12 @@ const USAGE_TAIL: &str = concat!( " --info\n", " print canvas, animation, the frame table and per-frame\n", " timing to stdout\n", + " --info=json\n", + " print one JSON object after successful decoding and output;\n", + " includes canvas, coding, frames, loops, metadata and durations\n", + " --icc-out path, --exif-out path, --xmp-out path\n", + " extract original metadata bytes to distinct files after success;\n", + " absent metadata writes an empty file. paths cannot be stdout\n", " --stream u32\n", " decode incrementally, appending this many bytes at a time,\n", " instead of opening the file whole\n", @@ -280,6 +288,8 @@ struct Options { scale: Option<(i32, i32)>, frame_size_limit: u32, info: bool, + info_json: bool, + metadata_out: [Option; 3], subframe: bool, muxer: Option, verify: Option, @@ -303,6 +313,9 @@ const OPTIONS: &[(&str, Option, bool)] = &[ ("muxer", None, true), ("verify", None, true), ("info", None, false), + ("icc-out", None, true), + ("exif-out", None, true), + ("xmp-out", None, true), ("loops", None, true), ("cpumask", None, true), ("subframe", None, false), @@ -335,17 +348,29 @@ fn by_letter(letter: char) -> Option<(&'static str, bool)> { .map(|&(name, _, takes_value)| (name, takes_value)) } -fn set(o: &mut Options, name: &str, value: String) -> Result<(), &'static str> { +fn set(o: &mut Options, name: &str, value: OsString) -> Result<(), &'static str> { + if let Some(i) = ["icc-out", "exif-out", "xmp-out"] + .iter() + .position(|&n| n == name) + { + if value.is_empty() || report::is_stdout(&value) { + return Err(BAD_METADATA_OUT); + } + o.metadata_out[i] = Some(PathBuf::from(value)); + return Ok(()); + } + let value = value.to_str().ok_or(MISSING)?; + match name { - "repeat" => o.repeat = parse_repeat(&value).ok_or(BAD_REPEAT)?, - "loops" => o.loops = parse_repeat(&value).ok_or(BAD_LOOPS)?, - "stream" => o.stream = parse_repeat(&value).ok_or(BAD_STREAM)? as usize, - "scale" => o.scale = Some(parse_scale(&value).ok_or(BAD_SCALE)?), + "repeat" => o.repeat = parse_repeat(value).ok_or(BAD_REPEAT)?, + "loops" => o.loops = parse_repeat(value).ok_or(BAD_LOOPS)?, + "stream" => o.stream = parse_repeat(value).ok_or(BAD_STREAM)? as usize, + "scale" => o.scale = Some(parse_scale(value).ok_or(BAD_SCALE)?), "frame-size-limit" => { - o.frame_size_limit = parse_pixels(&value).ok_or(BAD_PIXELS)?; + o.frame_size_limit = parse_pixels(value).ok_or(BAD_PIXELS)?; } - "max-input" => o.max_input = parse_size(&value).ok_or(BAD_SIZE)?, - "max-output" => o.max_output = parse_size(&value).ok_or(BAD_SIZE)?, + "max-input" => o.max_input = parse_size(value).ok_or(BAD_SIZE)?, + "max-output" => o.max_output = parse_size(value).ok_or(BAD_SIZE)?, "threads" => { o.n_threads = value .parse::() @@ -354,25 +379,35 @@ fn set(o: &mut Options, name: &str, value: String) -> Result<(), &'static str> { .ok_or(BAD_THREADS)? } "fmt" => { - let (pixel_format, out_format) = parse_format(&value).ok_or(BAD_FORMAT)?; + let (pixel_format, out_format) = parse_format(value).ok_or(BAD_FORMAT)?; o.pixel_format = pixel_format; o.out_format = out_format; } "muxer" => { - if !matches!(value.as_str(), "raw" | "md5" | "ppm" | "pam" | "y4m") { + if !matches!(value, "raw" | "md5" | "ppm" | "pam" | "y4m") { return Err(BAD_MUXER); } - o.muxer = Some(value); + o.muxer = Some(value.to_owned()); } - "verify" => o.verify = Some(value), + "verify" => o.verify = Some(value.to_owned()), "cpumask" => { - let mask = parse_cpumask(&value).ok_or(BAD_CPUMASK)?; + let mask = parse_cpumask(value).ok_or(BAD_CPUMASK)?; warn_baseline_cpumask(mask); api::set_cpu_flags_mask(mask); } - "info" => o.info = true, + "info" => match value { + "" => { + o.info = true; + o.info_json = false; + } + "json" => { + o.info = false; + o.info_json = true; + } + _ => return Err(BAD_INFO), + }, "subframe" => o.subframe = true, _ => return Err(MISSING), } @@ -408,7 +443,7 @@ fn parse_args(argv: &[OsString]) -> Parsed { /* A long option carries its value after '=', a short one in the rest * of its cluster; either may instead take the next argument. */ let next = |i: &mut usize| { - let v = argv.get(*i).map(|v| v.to_string_lossy().into_owned()); + let v = argv.get(*i).cloned(); if v.is_some() { *i += 1; @@ -417,15 +452,18 @@ fn parse_args(argv: &[OsString]) -> Parsed { }; if let Some(long) = arg.strip_prefix("--") { + if arg_os.to_str().is_none() { + return Parsed::Bad(MISSING); + } let (prefix, attached) = match long.split_once('=') { - Some((n, v)) => (n, Some(v.to_owned())), + Some((n, v)) => (n, Some(OsString::from(v))), None => (long, None), }; let Some((name, takes_value)) = by_prefix(prefix) else { return Parsed::Bad(MISSING); }; - if !takes_value && attached.is_some() { + if !takes_value && attached.is_some() && name != "info" { return Parsed::Bad(MISSING); } if name == "help" { @@ -438,7 +476,7 @@ fn parse_args(argv: &[OsString]) -> Parsed { None => return Parsed::Bad(MISSING), } } else { - String::new() + attached.unwrap_or_default() }; if let Err(e) = set(&mut o, name, value) { @@ -467,7 +505,7 @@ fn parse_args(argv: &[OsString]) -> Parsed { let v: String = cluster[c..].iter().collect(); c = cluster.len(); - v + OsString::from(v) }); let value = if takes_value { match rest.or_else(|| next(&mut i)) { @@ -475,7 +513,7 @@ fn parse_args(argv: &[OsString]) -> Parsed { None => return Parsed::Bad(MISSING), } } else { - String::new() + OsString::new() }; if let Err(e) = set(&mut o, name, value) { @@ -497,6 +535,8 @@ const BAD_PIXELS: &str = "invalid frame size limit; expected a pixel count or Wx const BAD_FORMAT: &str = "invalid output pixel format"; const BAD_MUXER: &str = "invalid output muxer; expected raw, md5, ppm, pam or y4m"; const BAD_SIZE: &str = "invalid byte count; expected digits with an optional K, M or G"; +const BAD_INFO: &str = "invalid info format; expected --info or --info=json"; +const BAD_METADATA_OUT: &str = "metadata output requires a file path, not stdout"; fn errmsg(e: &std::io::Error) -> String { let text = e.to_string(); @@ -746,7 +786,7 @@ fn main() -> ExitCode { print_banner(); - let opts = match parse_args(&argv) { + let mut opts = match parse_args(&argv) { Parsed::Ok(o) => o, Parsed::Help => { usage(&app, None); @@ -786,7 +826,12 @@ fn main() -> ExitCode { let operands = opts.positional.len(); let max = if verifying { 1 } else { 2 }; - if operands < 1 || operands > max || (!verifying && !opts.info && operands != 2) { + let reporting = opts.info_json || opts.metadata_out.iter().any(Option::is_some); + + if operands < 1 + || operands > max + || (!verifying && !opts.info && !reporting && operands != 2) + { let reason = if verifying { if operands < 1 { "input is required" @@ -810,10 +855,27 @@ fn main() -> ExitCode { Some(opts.positional[1].as_os_str()) }; - run(&opts, input_name, output_name, expected_md5) + if reporting { + if let Err(e) = report::validate_paths( + input_name, + output_name, + opts.info_json, + &mut opts.metadata_out, + ) { + eprintln!("{}", errmsg(&e)); + return if e.kind() == std::io::ErrorKind::InvalidInput { + ExitCode::from(2) + } else { + ExitCode::FAILURE + }; + } + run::(&opts, input_name, output_name, expected_md5) + } else { + run::(&opts, input_name, output_name, expected_md5) + } } -fn run( +fn run( opts: &Options, input_name: &OsStr, output_name: Option<&OsStr>, @@ -954,12 +1016,46 @@ fn run( if ret < 0 { return ExitCode::FAILURE; } + // The ordinary decode loop specializes this away. Reporting keeps + // only the final successful decoder alive until the pixel sink closes. + if REPORT && iter + 1 == opts.repeat { + if frames == 0 { + eprintln!("{}: no image data found", input_name.to_string_lossy()); + return ExitCode::FAILURE; + } + let status = finish_output(output, opened, expected_md5); + + if status != ExitCode::SUCCESS { + return status; + } + return match report::write( + &mut decoder, + opts.info_json, + &opts.metadata_out, + input_name, + output_name, + ) { + Ok(()) => ExitCode::SUCCESS, + Err(e) => { + eprintln!("report: {}", errmsg(&e)); + ExitCode::FAILURE + } + }; + } } if frames == 0 { eprintln!("{}: no image data found", input_name.to_string_lossy()); return ExitCode::FAILURE; } + finish_output(output, opened, expected_md5) +} + +fn finish_output( + output: Output, + opened: bool, + expected_md5: Option<[u8; 16]>, +) -> ExitCode { if let Some(expected) = expected_md5 { return if output.verify(&expected) { ExitCode::SUCCESS From f97580ea0fb7309ec9368cfa10fc565b64680085 Mon Sep 17 00:00:00 2001 From: Gianni Date: Sat, 10 Oct 2026 17:00:28 -0700 Subject: [PATCH 4/6] fix: preserve CLI inputs and accept valid metadata filenames Recognize stdin descriptor aliases and symlinks when comparing metadata and pixel destinations against redirected stdin. Reject JSON stdout that aliases the source, including stdin/stdout handles referring to the same file on macOS, before decoding or publishing any metadata. Use bounded staging basenames independent of the destination name, avoid requested destination aliases, and relinquish temporary-file ownership after publication. Preserve arbitrary Unix filename bytes in attached metadata arguments while continuing to validate option names and scalar values as UTF-8. Add regressions for descriptor and JSON aliases, existing and absent filenames at the filesystem component limit, and all three metadata types with attached/separate non-UTF-8 paths. The filesystem bugs are reproduced against the previous CLI using disposable input copies. Validation: 237 assembly and 235 scalar Meson tests and stylecheck pass. All 18 CLI cases pass where applicable on macOS and native Linux; Linux runs the non-UTF-8 extraction case and APFS runs the case-alias case. CLI parser unit tests pass on both platforms. References: https://github.com/halidecx/wpd/pull/39 --- tests/cli.py | 81 +++++++++++++++++++++++++++++++++++++------- tools/report.rs | 90 +++++++++++++++++++++++++++++++++++++++---------- tools/wpd.rs | 74 ++++++++++++++++++++++++++++++++++++---- 3 files changed, 209 insertions(+), 36 deletions(-) diff --git a/tests/cli.py b/tests/cli.py index 44f2bbf..9b46952 100644 --- a/tests/cli.py +++ b/tests/cli.py @@ -205,13 +205,64 @@ def test_new_case_aliases_do_not_overwrite_completed_outputs(self): @unittest.skipUnless(os.name == "posix", "Unix file descriptors") def test_redirected_stdin_is_not_a_metadata_or_pixel_destination(self): original = self.input.read_bytes() - for flags in [["--icc-out", str(self.input), "-"], - ["--info=json", "-", str(self.input)]]: - with self.input.open("rb") as stdin: - result = subprocess.run([str(WPD), *flags], stdin=stdin, - capture_output=True, timeout=30) - self.assertEqual(result.returncode, 2, result.stderr) - self.assertEqual(self.input.read_bytes(), original) + link = self.work / "stdin-link" + link.symlink_to("/dev/fd/0") + aliases = ["-", "/dev/stdin", "/dev/fd/0", str(link)] + if Path("/proc/self/fd").is_dir(): + aliases.append("/proc/self/fd/0") + for alias in aliases: + for flags in [["--icc-out", str(self.input), alias], + ["--info=json", alias, str(self.input)]]: + with self.subTest(flags=flags): + self.input.write_bytes(original) + with self.input.open("rb") as stdin: + result = subprocess.run([str(WPD), *flags], stdin=stdin, + capture_output=True, timeout=30) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(self.input.read_bytes(), original) + + @unittest.skipUnless(os.name == "posix", "Unix file descriptors") + def test_json_stdout_cannot_alias_the_input(self): + original = self.input.read_bytes() + metadata = self.work / "profile.icc" + symlink = self.work / "input-link" + symlink.symlink_to(self.input) + hardlink = self.work / "input-hardlink" + os.link(self.input, hardlink) + stdin_link = self.work / "stdin-link" + stdin_link.symlink_to("/dev/fd/0") + inputs = [self.input, "-", "/dev/stdin", "/dev/fd/0", stdin_link] + if Path("/proc/self/fd").is_dir(): + inputs.append("/proc/self/fd/0") + for source in inputs: + for target in [self.input, symlink, hardlink]: + with self.subTest(source=source, target=target): + self.input.write_bytes(original) + metadata.write_bytes(b"preserve metadata") + with self.input.open("rb") as stdin, target.open("ab") as stdout: + result = subprocess.run([str(WPD), "--info=json", "--icc-out", + str(metadata), str(source)], stdin=stdin, + stdout=stdout, stderr=subprocess.PIPE, timeout=30) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(self.input.read_bytes(), original) + self.assertEqual(metadata.read_bytes(), b"preserve metadata") + + def test_metadata_destination_names_near_the_component_limit(self): + limit = os.pathconf(self.work, "PC_NAME_MAX") if os.name == "posix" else 255 + for length in [limit - 11, limit]: + for flag, expected in [("--icc-out", b"ICC\0\xffprofile"), + ("--exif-out", b"Exif\0\0\x01\xff\x80"), + ("--xmp-out", b"\0\xfe")]: + dest = self.work / (flag[2] * length) + for existing in [False, True]: + with self.subTest(length=length, flag=flag, existing=existing): + if existing: + dest.write_bytes(b"old metadata") + result = self.command(flag, dest, self.input) + self.success(result) + self.assertEqual(dest.read_bytes(), expected) + dest.unlink() + self.assertEqual(list(self.work.glob("*.part")), []) def test_unicode_metadata_filename(self): dest = self.work / "profile-Ω.icc" @@ -221,11 +272,17 @@ def test_unicode_metadata_filename(self): @unittest.skipUnless(sys.platform.startswith("linux"), "Linux filename bytes; APFS requires valid Unicode") def test_non_utf8_metadata_path_is_preserved(self): - name = os.fsencode(self.work) + b"/profile-\xff.icc" - result = self.command("--icc-out", name, self.input) - self.success(result) - with open(name, "rb") as source: - self.assertEqual(source.read(), b"ICC\0\xffprofile") + for flag, expected in [(b"--icc-out", b"ICC\0\xffprofile"), + (b"--exif-out", b"Exif\0\0\x01\xff\x80"), + (b"--xmp-out", b"\0\xfe")]: + name = os.fsencode(self.work) + b"/profile-\xff=" + flag[2:] + for args in [(flag, name), (flag + b"=" + name,)]: + with self.subTest(args=args): + result = self.command(*args, self.input) + self.success(result) + with open(name, "rb") as source: + self.assertEqual(source.read(), expected) + os.unlink(name) def test_first_riff_chunk_is_validated_in_whole_and_split_input(self): for name in ["invalid-first-vp8x.webp", "invalid-first-junk.webp", "short-first-partition.webp"]: diff --git a/tools/report.rs b/tools/report.rs index a60b1e0..8f58615 100644 --- a/tools/report.rs +++ b/tools/report.rs @@ -44,16 +44,16 @@ fn same_file(a: &Path, b: &Path) -> bool { false } +#[cfg(unix)] +fn handle_metadata(handle: &impl std::os::fd::AsFd) -> io::Result { + File::from(handle.as_fd().try_clone_to_owned()?).metadata() +} + #[cfg(unix)] fn same_handle(path: &Path, handle: &impl std::os::fd::AsFd) -> bool { use std::os::unix::fs::MetadataExt; - let Ok(fd) = handle.as_fd().try_clone_to_owned() else { - return false; - }; - let file = File::from(fd); - - if let (Ok(a), Ok(b)) = (fs::metadata(path), file.metadata()) { + if let (Ok(a), Ok(b)) = (fs::metadata(path), handle_metadata(handle)) { a.dev() == b.dev() && a.ino() == b.ino() } else { false @@ -71,17 +71,45 @@ fn stdout_alias(path: &Path) -> bool { false } +#[cfg(unix)] +fn stdin_alias(path: &Path) -> bool { + ["-", "/dev/stdin", "/dev/fd/0", "/proc/self/fd/0"] + .iter() + .any(|alias| path == Path::new(alias)) + || same_file(path, Path::new("/dev/stdin")) +} + fn input_alias(path: &Path, input: &Path) -> bool { if same_file(path, input) { return true; } #[cfg(unix)] - if input == Path::new("/dev/stdin") && same_handle(path, &io::stdin()) { + if stdin_alias(input) && same_handle(path, &io::stdin()) { return true; } false } +fn input_is_stdout(input: &Path) -> bool { + if stdout_alias(input) { + return true; + } + #[cfg(unix)] + if stdin_alias(input) { + use std::os::unix::fs::MetadataExt; + + // Descriptor paths on macOS identify fdesc nodes. Compare the actual + // handles when both input and JSON use redirected standard streams. + if let (Ok(a), Ok(b)) = ( + handle_metadata(&io::stdin()), + handle_metadata(&io::stdout()), + ) { + return a.dev() == b.dev() && a.ino() == b.ino(); + } + } + false +} + fn destination(path: &Path) -> io::Result { match fs::metadata(path) { Ok(meta) if meta.is_file() => path.canonicalize(), @@ -128,6 +156,12 @@ pub fn validate_paths( }; let output = output.map(|p| if is_stdout(p) { stdout } else { Path::new(p) }); + if json && input_is_stdout(input) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "JSON info output collides with input", + )); + } if json && output.is_some_and(stdout_alias) { return Err(io::Error::new( io::ErrorKind::InvalidInput, @@ -196,21 +230,23 @@ fn json_info(decoder: &mut Decoder<'_>) -> io::Result> { struct Pending { path: PathBuf, dest: PathBuf, + published: bool, } impl Pending { - fn new(dest: &Path, data: &[u8]) -> io::Result { + fn new( + dest: &Path, + data: &[u8], + destinations: &[Option; 3], + ) -> io::Result { static NEXT: AtomicU64 = AtomicU64::new(0); for _ in 0..64 { - let mut name = dest.file_name().unwrap().to_os_string(); - - name.push(format!( - ".{}.{}.part", + let path = dest.with_file_name(format!( + ".wpd-meta.{}.{}.part", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed) )); - let path = dest.with_file_name(name); let file = OpenOptions::new().write(true).create_new(true).open(&path); let mut file: File = match file { @@ -221,8 +257,18 @@ impl Pending { let pending = Self { path, dest: dest.to_owned(), + published: false, }; + // The bounded staging name must not occupy another requested + // destination, including an absent case-insensitive alias. + if destinations + .iter() + .flatten() + .any(|p| same_file(&pending.path, p)) + { + continue; + } file.write_all(data)?; file.flush()?; if let Ok(meta) = fs::metadata(dest) { @@ -236,14 +282,18 @@ impl Pending { )) } - fn publish(&self) -> io::Result<()> { - fs::rename(&self.path, &self.dest) + fn publish(&mut self) -> io::Result<()> { + fs::rename(&self.path, &self.dest)?; + self.published = true; + Ok(()) } } impl Drop for Pending { fn drop(&mut self) { - let _ = fs::remove_file(&self.path); + if !self.published { + let _ = fs::remove_file(&self.path); + } } } @@ -274,11 +324,15 @@ pub fn write( pending.push(Pending::new( path, decoder.metadata(kind).unwrap_or_default(), + &metadata, )?); } } - for (i, file) in pending.iter().enumerate() { - if pending[..i] + for i in 0..pending.len() { + let (previous, remaining) = pending.split_at_mut(i); + let file = &mut remaining[0]; + + if previous .iter() .any(|previous| same_file(&file.dest, &previous.dest)) { diff --git a/tools/wpd.rs b/tools/wpd.rs index 6a234ec..5f01df6 100644 --- a/tools/wpd.rs +++ b/tools/wpd.rs @@ -414,6 +414,33 @@ fn set(o: &mut Options, name: &str, value: OsString) -> Result<(), &'static str> Ok(()) } +fn split_long(arg: &OsStr) -> Option<(&str, Option)> { + #[cfg(unix)] + { + use std::os::unix::ffi::OsStrExt; + + let bytes = arg.as_bytes().strip_prefix(b"--")?; + let (name, value) = match bytes.iter().position(|&b| b == b'=') { + Some(i) => ( + &bytes[..i], + Some(OsStr::from_bytes(&bytes[i + 1..]).to_owned()), + ), + None => (bytes, None), + }; + + Some((std::str::from_utf8(name).ok()?, value)) + } + #[cfg(not(unix))] + { + let long = arg.to_str()?.strip_prefix("--")?; + + Some(match long.split_once('=') { + Some((name, value)) => (name, Some(OsString::from(value))), + None => (long, None), + }) + } +} + #[inline(never)] fn parse_args(argv: &[OsString]) -> Parsed { let mut o = Options { @@ -451,13 +478,9 @@ fn parse_args(argv: &[OsString]) -> Parsed { v }; - if let Some(long) = arg.strip_prefix("--") { - if arg_os.to_str().is_none() { + if arg.starts_with("--") { + let Some((prefix, attached)) = split_long(arg_os) else { return Parsed::Bad(MISSING); - } - let (prefix, attached) = match long.split_once('=') { - Some((n, v)) => (n, Some(OsString::from(v))), - None => (long, None), }; let Some((name, takes_value)) = by_prefix(prefix) else { return Parsed::Bad(MISSING); @@ -1079,6 +1102,45 @@ fn finish_output( mod tests { use super::*; + #[cfg(unix)] + #[test] + fn attached_metadata_arguments_preserve_filename_bytes() { + use std::os::unix::ffi::{OsStrExt, OsStringExt}; + + let path = OsStr::from_bytes(b"profile-\xff=meta"); + + for (i, name) in ["icc-out", "exif-out", "xmp-out"].iter().enumerate() { + let mut attached = format!("--{name}=").into_bytes(); + + attached.extend_from_slice(path.as_bytes()); + let Parsed::Ok(o) = parse_args(&[ + OsString::from("wpd"), + OsString::from_vec(attached), + OsString::from("input.webp"), + ]) else { + panic!("attached metadata path must parse"); + }; + + assert_eq!( + o.metadata_out[i].as_deref(), + Some(std::path::Path::new(path)) + ); + } + for invalid in [ + b"--icc-\xff=path".as_slice(), + b"--info=\xff", + b"--repeat=\xff", + ] { + assert!(matches!( + parse_args(&[ + OsString::from("wpd"), + OsString::from_vec(invalid.to_vec()) + ]), + Parsed::Bad(_) + )); + } + } + #[test] fn a_size_takes_a_binary_suffix_and_rejects_overflow() { assert_eq!(parse_size("0"), Some(0)); From 09275968aa1ba5f42941c8f868a1e3ac7a4cd2de Mon Sep 17 00:00:00 2001 From: Gianni Date: Sat, 10 Oct 2026 17:53:30 -0700 Subject: [PATCH 5/6] fix: validate opened input identity before publishing outputs Compare metadata, pixel and JSON destinations against the actual reader handle, including non-stdin descriptor inputs. Stat macOS descriptor output handles without truncation to avoid fdesc device-number mismatches. Honor the explicit /dev/null discard sink in stdout collision checks. Add descriptor and symlink regressions that preserve source and existing metadata bytes, plus JSON/discard-sink coverage. Assembly and scalar Meson suites and native macOS/Linux CLI suites pass. --- tests/cli.py | 92 +++++++++++++++++++++++++++++++++++ tools/report.rs | 124 ++++++++++++++++++++++++++++++++---------------- tools/wpd.rs | 59 ++++++++++++++++------- 3 files changed, 215 insertions(+), 60 deletions(-) diff --git a/tests/cli.py b/tests/cli.py index 9b46952..5b0e95c 100644 --- a/tests/cli.py +++ b/tests/cli.py @@ -221,6 +221,98 @@ def test_redirected_stdin_is_not_a_metadata_or_pixel_destination(self): self.assertEqual(result.returncode, 2, result.stderr) self.assertEqual(self.input.read_bytes(), original) + @unittest.skipUnless(os.name == "posix", "Unix file descriptors") + def test_descriptor_input_is_not_a_metadata_or_pixel_destination(self): + original = self.input.read_bytes() + link = self.work / "descriptor-link" + metadata = self.work / "profile.icc" + for linked in [False, True]: + for flag in ["--icc-out", "--exif-out", "--xmp-out", None]: + with self.subTest(linked=linked, flag=flag): + self.input.write_bytes(original) + with self.input.open("rb") as source: + descriptor = f"/dev/fd/{source.fileno()}" + link.unlink(missing_ok=True) + link.symlink_to(descriptor) + name = str(link) if linked else descriptor + args = [flag, str(self.input), name] if flag else ["--info=json", name, str(self.input)] + result = subprocess.run([str(WPD), *args], stdin=subprocess.DEVNULL, + pass_fds=(source.fileno(),), capture_output=True, timeout=30) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(self.input.read_bytes(), original) + self.assertEqual(list(self.work.glob("*.part")), []) + with self.input.open("rb") as source: + descriptor = f"/dev/fd/{source.fileno()}" + link.unlink(missing_ok=True) + link.symlink_to(descriptor) + name = str(link) if linked else descriptor + result = subprocess.run([str(WPD), "--icc-out", str(metadata), "--stream", "1", name], + stdin=subprocess.DEVNULL, pass_fds=(source.fileno(),), + capture_output=True, timeout=30) + self.success(result) + self.assertEqual(metadata.read_bytes(), b"ICC\0\xffprofile") + self.assertEqual(self.input.read_bytes(), original) + + @unittest.skipUnless(os.name == "posix", "Unix file descriptors") + def test_descriptor_input_cannot_alias_json_stdout(self): + original = self.input.read_bytes() + metadata = self.work / "profile.icc" + link = self.work / "descriptor-link" + for linked in [False, True]: + with self.subTest(linked=linked): + self.input.write_bytes(original) + metadata.write_bytes(b"preserve metadata") + with self.input.open("rb") as source, self.input.open("ab") as stdout: + descriptor = f"/dev/fd/{source.fileno()}" + link.unlink(missing_ok=True) + link.symlink_to(descriptor) + name = str(link) if linked else descriptor + result = subprocess.run([str(WPD), "--info=json", "--icc-out", str(metadata), name], + stdin=subprocess.DEVNULL, stdout=stdout, stderr=subprocess.PIPE, + pass_fds=(source.fileno(),), timeout=30) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(self.input.read_bytes(), original) + self.assertEqual(metadata.read_bytes(), b"preserve metadata") + + @unittest.skipUnless(os.name == "posix", "Unix file descriptors") + def test_descriptor_pixel_output_cannot_modify_the_input(self): + original = self.input.read_bytes() + metadata = self.work / "profile.icc" + link = self.work / "output-descriptor-link" + for input_kind in ["path", "descriptor", "stdin"]: + for mode in [os.O_RDWR, os.O_WRONLY]: + for linked in [False, True]: + with self.subTest(input_kind=input_kind, mode=mode, linked=linked): + self.input.write_bytes(original) + metadata.write_bytes(b"preserve metadata") + with self.input.open("rb") as source, os.fdopen(os.open(self.input, mode), "wb") as target: + name = (str(self.input) if input_kind == "path" else "-" if input_kind == "stdin" + else f"/dev/fd/{source.fileno()}") + descriptor = f"/dev/fd/{target.fileno()}" + link.unlink(missing_ok=True) + link.symlink_to(descriptor) + output = str(link) if linked else descriptor + result = subprocess.run([str(WPD), "--info=json", "--icc-out", str(metadata), name, output], + stdin=source if input_kind == "stdin" else subprocess.DEVNULL, + pass_fds=(source.fileno(), target.fileno()), + capture_output=True, timeout=30) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(self.input.read_bytes(), original) + self.assertEqual(metadata.read_bytes(), b"preserve metadata") + + @unittest.skipUnless(os.name == "posix", "Unix discard sink") + def test_json_and_discard_output_can_share_dev_null(self): + result = self.command("--info=json", self.input, "/dev/null") + self.success(result) + self.assertEqual(json.loads(result.stdout)["width"], 1) + metadata = self.work / "profile.icc" + with open(os.devnull, "wb") as stdout: + result = self.command("--info=json", "--icc-out", metadata, self.input, "/dev/null", stdout=stdout) + self.success(result) + result = self.command("--info=json", self.input, "-", stdout=stdout) + self.assertEqual(result.returncode, 2, result.stderr) + self.assertEqual(metadata.read_bytes(), b"ICC\0\xffprofile") + @unittest.skipUnless(os.name == "posix", "Unix file descriptors") def test_json_stdout_cannot_alias_the_input(self): original = self.input.read_bytes() diff --git a/tools/report.rs b/tools/report.rs index 8f58615..5572471 100644 --- a/tools/report.rs +++ b/tools/report.rs @@ -12,6 +12,23 @@ pub fn is_stdout(path: &OsStr) -> bool { .any(|alias| path == OsStr::new(alias)) } +fn path_metadata(path: &Path) -> io::Result { + #[cfg(target_os = "macos")] + if let Ok(resolved) = path.canonicalize() { + if resolved.parent() == Some(Path::new("/dev/fd")) { + // fdesc path metadata has a synthetic device number. Open the + // descriptor without truncation and stat the handle it duplicates. + // realpath can replace the descriptor number with the underlying + // basename, so use the original path to duplicate the descriptor. + let file = File::open(path) + .or_else(|_| OpenOptions::new().write(true).open(path))?; + + return file.metadata(); + } + } + fs::metadata(path) +} + fn same_file(a: &Path, b: &Path) -> bool { if a == b { return true; @@ -36,14 +53,19 @@ fn same_file(a: &Path, b: &Path) -> bool { } } #[cfg(unix)] - if let (Ok(a), Ok(b)) = (fs::metadata(a), fs::metadata(b)) { - use std::os::unix::fs::MetadataExt; - - return a.dev() == b.dev() && a.ino() == b.ino(); + if let (Ok(a), Ok(b)) = (path_metadata(a), path_metadata(b)) { + return same_metadata(&a, &b); } false } +#[cfg(unix)] +fn same_metadata(a: &fs::Metadata, b: &fs::Metadata) -> bool { + use std::os::unix::fs::MetadataExt; + + a.dev() == b.dev() && a.ino() == b.ino() +} + #[cfg(unix)] fn handle_metadata(handle: &impl std::os::fd::AsFd) -> io::Result { File::from(handle.as_fd().try_clone_to_owned()?).metadata() @@ -51,10 +73,8 @@ fn handle_metadata(handle: &impl std::os::fd::AsFd) -> io::Result #[cfg(unix)] fn same_handle(path: &Path, handle: &impl std::os::fd::AsFd) -> bool { - use std::os::unix::fs::MetadataExt; - - if let (Ok(a), Ok(b)) = (fs::metadata(path), handle_metadata(handle)) { - a.dev() == b.dev() && a.ino() == b.ino() + if let (Ok(a), Ok(b)) = (path_metadata(path), handle_metadata(handle)) { + same_metadata(&a, &b) } else { false } @@ -71,43 +91,52 @@ fn stdout_alias(path: &Path) -> bool { false } -#[cfg(unix)] -fn stdin_alias(path: &Path) -> bool { - ["-", "/dev/stdin", "/dev/fd/0", "/proc/self/fd/0"] - .iter() - .any(|alias| path == Path::new(alias)) - || same_file(path, Path::new("/dev/stdin")) +pub fn input_metadata(file: Option<&File>) -> io::Result> { + if let Some(file) = file { + return file.metadata().map(Some); + } + #[cfg(unix)] + { + handle_metadata(&io::stdin()).map(Some) + } + #[cfg(not(unix))] + { + Ok(None) + } } -fn input_alias(path: &Path, input: &Path) -> bool { +fn input_alias( + path: &Path, + input: &Path, + identity: Option<&fs::Metadata>, +) -> io::Result { if same_file(path, input) { - return true; + return Ok(true); } #[cfg(unix)] - if stdin_alias(input) && same_handle(path, &io::stdin()) { - return true; + if let Some(input) = identity { + return match path_metadata(path) { + Ok(dest) => Ok(same_metadata(input, &dest)), + Err(e) if e.kind() == io::ErrorKind::NotFound => Ok(false), + Err(e) => Err(e), + }; } - false + #[cfg(not(unix))] + let _ = identity; + Ok(false) } -fn input_is_stdout(input: &Path) -> bool { +fn input_is_stdout(input: &Path, identity: Option<&fs::Metadata>) -> io::Result { if stdout_alias(input) { - return true; + return Ok(true); } #[cfg(unix)] - if stdin_alias(input) { - use std::os::unix::fs::MetadataExt; - - // Descriptor paths on macOS identify fdesc nodes. Compare the actual - // handles when both input and JSON use redirected standard streams. - if let (Ok(a), Ok(b)) = ( - handle_metadata(&io::stdin()), - handle_metadata(&io::stdout()), - ) { - return a.dev() == b.dev() && a.ino() == b.ino(); - } + if let Some(input) = identity { + return Ok(same_metadata(input, &handle_metadata(&io::stdout())?)); } - false + #[cfg(not(unix))] + let _ = identity; + Ok(false) } fn destination(path: &Path) -> io::Result { @@ -144,6 +173,7 @@ fn destination(path: &Path) -> io::Result { pub fn validate_paths( input: &OsStr, + identity: Option<&fs::Metadata>, output: Option<&OsStr>, json: bool, metadata: &mut [Option; 3], @@ -154,9 +184,16 @@ pub fn validate_paths( } else { Path::new(input) }; - let output = output.map(|p| if is_stdout(p) { stdout } else { Path::new(p) }); + // Output::open treats this exact path as a discard sink. + let output = output.filter(|p| *p != OsStr::new("/dev/null")).map(|p| { + if is_stdout(p) { + stdout + } else { + Path::new(p) + } + }); - if json && input_is_stdout(input) { + if json && input_is_stdout(input, identity)? { return Err(io::Error::new( io::ErrorKind::InvalidInput, "JSON info output collides with input", @@ -168,11 +205,13 @@ pub fn validate_paths( "JSON info and decoded output cannot both use stdout", )); } - if output.is_some_and(|p| input_alias(p, input)) { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - "decoded output collides with input", - )); + if let Some(output) = output { + if input_alias(output, input, identity)? { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "decoded output collides with input", + )); + } } let mut destinations: [Option; 3] = Default::default(); @@ -180,7 +219,7 @@ pub fn validate_paths( let Some(path) = path else { continue }; let dest = destination(path)?; - if input_alias(&dest, input) + if input_alias(&dest, input, identity)? || output.is_some_and(|p| same_file(&dest, p)) || stdout_alias(&dest) || same_file(&dest, Path::new("/dev/stderr")) @@ -302,13 +341,14 @@ pub fn write( json: bool, metadata: &[Option; 3], input: &OsStr, + identity: Option<&fs::Metadata>, output: Option<&OsStr>, ) -> io::Result<()> { // A newly created pixel file can reveal aliases that did not exist at // argument validation, including case-insensitive filesystem names. let mut metadata = metadata.clone(); - validate_paths(input, output, json, &mut metadata)?; + validate_paths(input, identity, output, json, &mut metadata)?; let json = if json { Some(json_info(decoder)?) } else { diff --git a/tools/wpd.rs b/tools/wpd.rs index 5f01df6..82d44c6 100644 --- a/tools/wpd.rs +++ b/tools/wpd.rs @@ -772,12 +772,19 @@ fn read_limited(reader: R, limit: u64, hint: u64) -> std::io::Result std::io::Result> { +fn read_file( + name: &OsStr, + limit: u64, + validate: impl FnOnce(Option<&std::fs::File>) -> std::io::Result<()>, +) -> std::io::Result> { if name == OsStr::new("-") { + validate(None)?; return read_limited(std::io::stdin().lock(), limit, 0); } let file = std::fs::File::open(name)?; + + validate(Some(&file))?; let hint = file.metadata().map_or(0, |m| m.len()); read_limited(file, limit, hint) @@ -809,7 +816,7 @@ fn main() -> ExitCode { print_banner(); - let mut opts = match parse_args(&argv) { + let opts = match parse_args(&argv) { Parsed::Ok(o) => o, Parsed::Help => { usage(&app, None); @@ -879,19 +886,6 @@ fn main() -> ExitCode { }; if reporting { - if let Err(e) = report::validate_paths( - input_name, - output_name, - opts.info_json, - &mut opts.metadata_out, - ) { - eprintln!("{}", errmsg(&e)); - return if e.kind() == std::io::ErrorKind::InvalidInput { - ExitCode::from(2) - } else { - ExitCode::FAILURE - }; - } run::(&opts, input_name, output_name, expected_md5) } else { run::(&opts, input_name, output_name, expected_md5) @@ -904,11 +898,39 @@ fn run( output_name: Option<&OsStr>, expected_md5: Option<[u8; 16]>, ) -> ExitCode { - let data = match read_file(input_name, opts.max_input) { + let mut metadata_out = if REPORT { + opts.metadata_out.clone() + } else { + Default::default() + }; + let mut input_identity = None; + let mut invalid_paths = false; + let data = match read_file(input_name, opts.max_input, |file| { + if REPORT { + // Capture the identity of the actual reader before any output + // opens. Descriptor paths can describe fdesc nodes on macOS. + input_identity = report::input_metadata(file)?; + if let Err(e) = report::validate_paths( + input_name, + input_identity.as_ref(), + output_name, + opts.info_json, + &mut metadata_out, + ) { + invalid_paths = e.kind() == std::io::ErrorKind::InvalidInput; + return Err(e); + } + } + Ok(()) + }) { Ok(d) => d, Err(e) => { eprintln!("{}: {}", input_name.to_string_lossy(), errmsg(&e)); - return ExitCode::FAILURE; + return if invalid_paths { + ExitCode::from(2) + } else { + ExitCode::FAILURE + }; } }; @@ -1054,8 +1076,9 @@ fn run( return match report::write( &mut decoder, opts.info_json, - &opts.metadata_out, + &metadata_out, input_name, + input_identity.as_ref(), output_name, ) { Ok(()) => ExitCode::SUCCESS, From 0a932c5498569e65472850065221da70ee546fd4 Mon Sep 17 00:00:00 2001 From: Gianni Date: Sat, 10 Oct 2026 17:53:33 -0700 Subject: [PATCH 6/6] style: add Ruff script and format Python tooling --- scripts/ruff.sh | 5 +++ scripts/webpcompare.py | 85 +++++++++++++++++++++++++++++++----------- 2 files changed, 68 insertions(+), 22 deletions(-) create mode 100755 scripts/ruff.sh diff --git a/scripts/ruff.sh b/scripts/ruff.sh new file mode 100755 index 0000000..0d3dc13 --- /dev/null +++ b/scripts/ruff.sh @@ -0,0 +1,5 @@ +#!/bin/bash -eu + +find . -type f -name '*.py' -exec ruff format {} + +find . -type f -name '*.py' -exec ruff check --select I --fix {} + +find . -type f -name '*.py' -exec ruff check --fix {} + diff --git a/scripts/webpcompare.py b/scripts/webpcompare.py index 7c62f52..065d96d 100644 --- a/scripts/webpcompare.py +++ b/scripts/webpcompare.py @@ -9,13 +9,13 @@ """ import argparse -from collections import Counter import hashlib import json import math -from pathlib import Path import subprocess import tempfile +from collections import Counter +from pathlib import Path def digest(path): @@ -29,8 +29,9 @@ def digest(path): def run(command, log, timeout): with log.open("wb") as stderr: try: - status = subprocess.run(command, stdout=subprocess.DEVNULL, - stderr=stderr, timeout=timeout).returncode + status = subprocess.run( + command, stdout=subprocess.DEVNULL, stderr=stderr, timeout=timeout + ).returncode except subprocess.TimeoutExpired: return "timeout", "wall-clock limit exceeded" with log.open("rb") as stderr: @@ -55,8 +56,11 @@ def header(source): fields[key] = value else: raise ValueError("invalid PAM header") - if (fields[b"DEPTH"] != b"4" or fields[b"MAXVAL"] != b"255" or - fields[b"TUPLTYPE"] != b"RGB_ALPHA"): + if ( + fields[b"DEPTH"] != b"4" + or fields[b"MAXVAL"] != b"255" + or fields[b"TUPLTYPE"] != b"RGB_ALPHA" + ): raise ValueError("expected straight-RGBA PAM") width, height = int(fields[b"WIDTH"]), int(fields[b"HEIGHT"]) if not 0 < width <= 16384 or not 0 < height <= 16384: @@ -86,7 +90,7 @@ def compare(a_path, b_path): if aa == bb: continue for i in range(0, size, 4): - if aa[i:i + 4] == bb[i:i + 4]: + if aa[i : i + 4] == bb[i : i + 4]: continue if aa[i + 3] == bb[i + 3] == 0: transparent += 1 @@ -124,14 +128,31 @@ def main(): versions = None for path in sorted(files): record = {"path": str(path), "input_sha256": digest(path)} - with tempfile.TemporaryDirectory(prefix="webpcompare-", dir=args.work_dir) as td: + with tempfile.TemporaryDirectory( + prefix="webpcompare-", dir=args.work_dir + ) as td: work = Path(td) outputs = [work / "wpd.pam", work / "libwebp.pam"] - results = [run([str(tool), "--fmt", "rgba", "--muxer", "pam", - str(path), str(output)], work / f"{i}.log", args.timeout) - for i, (tool, output) in enumerate(zip(tools, outputs))] + results = [ + run( + [ + str(tool), + "--fmt", + "rgba", + "--muxer", + "pam", + str(path), + str(output), + ], + work / f"{i}.log", + args.timeout, + ) + for i, (tool, output) in enumerate(zip(tools, outputs)) + ] if versions is None: - versions = [error.splitlines()[0] if error else "" for _, error in results] + versions = [ + error.splitlines()[0] if error else "" for _, error in results + ] for name, (status, error) in zip(["wpd", "libwebp"], results): record[f"{name}_status"] = status if status: @@ -142,23 +163,43 @@ def main(): elif statuses == [0, 0]: try: outcome, frames, visible, transparent = compare(*outputs) - record.update(frames=frames, visible_pixels=visible, - transparent_rgb_pixels=transparent, - wpd_output_sha256=digest(outputs[0]), - libwebp_output_sha256=digest(outputs[1])) + record.update( + frames=frames, + visible_pixels=visible, + transparent_rgb_pixels=transparent, + wpd_output_sha256=digest(outputs[0]), + libwebp_output_sha256=digest(outputs[1]), + ) except (KeyError, ValueError) as error: outcome = "abnormal" record["output_error"] = str(error) else: - outcome = ("wpd_only" if statuses[0] == 0 else - "libwebp_only" if statuses[1] == 0 else "neither") + outcome = ( + "wpd_only" + if statuses[0] == 0 + else "libwebp_only" + if statuses[1] == 0 + else "neither" + ) record["outcome"] = outcome counts[outcome] += 1 print(json.dumps(record), flush=True) - print(json.dumps({"summary": dict(counts), "files": len(files), - "tools": [str(tool) for tool in tools], "versions": versions})) - return int(any(counts[key] for key in ("visible", "geometry", "wpd_only", - "libwebp_only", "abnormal"))) + print( + json.dumps( + { + "summary": dict(counts), + "files": len(files), + "tools": [str(tool) for tool in tools], + "versions": versions, + } + ) + ) + return int( + any( + counts[key] + for key in ("visible", "geometry", "wpd_only", "libwebp_only", "abnormal") + ) + ) if __name__ == "__main__":