From 53f131c8ef6e100fcc266fafc5905384679a4560 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 07:13:59 +0200 Subject: [PATCH 1/2] fix: bound PDF color-space parsing and validate palette dimensions --- CHANGELOG.md | 3 + src/odr/internal/pdf/AGENTS.md | 3 + src/odr/internal/pdf/pdf_color.cpp | 112 ++++++++++++------ src/odr/internal/pdf/pdf_color.hpp | 11 +- src/odr/internal/pdf/pdf_document_parser.cpp | 9 +- src/odr/internal/pdf/pdf_page_extractor.cpp | 3 +- test/src/internal/pdf/pdf_color.cpp | 64 +++++++++- test/src/internal/pdf/pdf_document_parser.cpp | 22 ++++ 8 files changed, 181 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4eb7a81c9..36dd0caeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- PDF color-space parsing bounds recursive definitions and rejects invalid + component counts and palettes. Large finite palette indices clip correctly. + - Document path saves preserve the destination on write failure and retain lazy resources when saving repeatedly over the input. Existing permissions are preserved; temporary copies are private to their owner. diff --git a/src/odr/internal/pdf/AGENTS.md b/src/odr/internal/pdf/AGENTS.md index 6651ed0ff..8193cea49 100644 --- a/src/odr/internal/pdf/AGENTS.md +++ b/src/odr/internal/pdf/AGENTS.md @@ -164,6 +164,9 @@ above. So each run is raised by one font ascent: and 64 nested calls. Invalid operands or results use the existing zero-result fallback, before range clipping. ISO 32000-1 7.10.5 requires at least 100 stack entries; the larger bound accommodates ordinary tint transforms. +- Colour-space parsing carries a 64-level bound through arrays and named + resource aliases. Invalid ICC component counts, Indexed palettes and + unresolved base/alternate spaces are rejected before image allocation. - CMYK is naive (no ICC) and overprint is ignored. CIE, ICCBased, Indexed, Separation, DeviceN and Lab resolve to RGB at emission by sampling the tint `/Function` (types 0, 2, 3, 4). diff --git a/src/odr/internal/pdf/pdf_color.cpp b/src/odr/internal/pdf/pdf_color.cpp index db147c6f1..bc8a4bdd9 100644 --- a/src/odr/internal/pdf/pdf_color.cpp +++ b/src/odr/internal/pdf/pdf_color.cpp @@ -5,6 +5,7 @@ #include #include +#include #include namespace odr::internal::pdf { @@ -60,7 +61,8 @@ std::vector read_numbers(const Object &object) { /// Resolve a colour-space name to a device/pattern space, or a resource space /// via `context.named`. std::shared_ptr space_from_name(const std::string &name, - const ColorSpaceContext &ctx) { + const ColorSpaceContext &ctx, + const std::uint32_t depth) { if (name == "DeviceGray" || name == "G") { return device_space(ColorSpaceKind::device_gray, 1); } @@ -74,7 +76,7 @@ std::shared_ptr space_from_name(const std::string &name, return device_space(ColorSpaceKind::pattern, 1); } if (ctx.named) { - return ctx.named(name); + return ctx.named(name, depth + 1); } return nullptr; } @@ -118,11 +120,11 @@ ColorSpaceDef::to_rgb(const std::span c) const { return {0, 0, 0}; } const std::int32_t n = base->components; - const auto index = static_cast(std::lround(at(0))); - // `std::clamp` is undefined for hi < lo, so a negative `/HiVal` (malformed) - // must not reach it. - const auto offset = static_cast(std::clamp( - index, 0, std::max(hival, 0))) * + if (n <= 0 || hival < 0 || hival > 255 || !std::isfinite(at(0))) { + return {0, 0, 0}; + } + const double index = std::clamp(at(0), 0.0, static_cast(hival)); + const auto offset = static_cast(std::lround(index)) * static_cast(n); std::vector base_components(static_cast(n), 0.0); for (std::int32_t j = 0; j < n; ++j) { @@ -219,11 +221,15 @@ std::array pdf::cmyk_to_rgb(const double c, const double m, } std::shared_ptr -pdf::parse_color_space(const Object &object, const ColorSpaceContext &context) { +pdf::parse_color_space(const Object &object, const ColorSpaceContext &context, + const std::uint32_t depth) { + if (depth >= 64) { + return nullptr; + } const Object resolved = context.resolve(object); if (resolved.is_name()) { - return space_from_name(resolved.as_name(), context); + return space_from_name(resolved.as_name(), context, depth); } if (!resolved.is_array() || resolved.as_array().empty()) { return nullptr; @@ -243,14 +249,24 @@ pdf::parse_color_space(const Object &object, const ColorSpaceContext &context) { auto def = std::make_shared(); def->kind = ColorSpaceKind::icc_based; const Object stream_dict = context.resolve(array[1]); - if (stream_dict.is_dictionary()) { - const Dictionary &dict = stream_dict.as_dictionary(); - def->components = - dict.get("N").is_integer() - ? static_cast(dict.get("N").as_integer()) - : 3; - if (dict.has_value("Alternate")) { - def->alternate = parse_color_space(dict.get("Alternate"), context); + if (!stream_dict.is_dictionary()) { + return nullptr; + } + const Dictionary &dict = stream_dict.as_dictionary(); + const Object count = context.resolve(dict.get("N")); + if (!count.is_integer() || + (count.as_integer() != 1 && count.as_integer() != 3 && + count.as_integer() != 4)) { + return nullptr; + } + def->components = static_cast(count.as_integer()); + if (dict.has_value("Alternate")) { + def->alternate = + parse_color_space(dict.get("Alternate"), context, depth + 1); + if (def->alternate == nullptr || + def->alternate->kind == ColorSpaceKind::pattern || + def->alternate->components != def->components) { + return nullptr; } } return def; @@ -288,40 +304,58 @@ pdf::parse_color_space(const Object &object, const ColorSpaceContext &context) { auto def = std::make_shared(); def->kind = ColorSpaceKind::indexed; def->components = 1; - def->base = parse_color_space(array[1], context); - def->hival = - static_cast(context.resolve(array[2]).as_integer()); + def->base = parse_color_space(array[1], context, depth + 1); + const Object high = context.resolve(array[2]); + if (def->base == nullptr || def->base->kind == ColorSpaceKind::indexed || + def->base->kind == ColorSpaceKind::pattern || !high.is_integer() || + high.as_integer() < 0 || high.as_integer() > 255) { + return nullptr; + } + def->hival = static_cast(high.as_integer()); const Object lookup = context.resolve(array[3]); if (lookup.is_string()) { def->lookup = lookup.as_string(); } else { def->lookup = context.load_stream(array[3]); } - return def; - } - if (family == "Separation") { - if (array.size() < 4) { + const auto entries = static_cast(def->hival) + 1; + if (entries > + def->lookup.size() / static_cast(def->base->components)) { return nullptr; } - auto def = std::make_shared(); - def->kind = ColorSpaceKind::separation; - def->components = 1; - def->alternate = parse_color_space(array[2], context); - def->tint = parse_function( - array[3], FunctionContext{context.resolve, context.load_stream}); return def; } - if (family == "DeviceN") { + if (family == "Separation" || family == "DeviceN") { if (array.size() < 4) { return nullptr; } auto def = std::make_shared(); - def->kind = ColorSpaceKind::device_n; const Object names = context.resolve(array[1]); - def->components = names.is_array() - ? static_cast(names.as_array().size()) - : 1; - def->alternate = parse_color_space(array[2], context); + if (family == "Separation") { + if (!names.is_name()) { + return nullptr; + } + def->kind = ColorSpaceKind::separation; + def->components = 1; + } else { + if (!names.is_array() || names.as_array().empty() || + names.as_array().size() > + static_cast( + std::numeric_limits::max())) { + return nullptr; + } + for (const Object &name : names.as_array()) { + if (!context.resolve(name).is_name()) { + return nullptr; + } + } + def->kind = ColorSpaceKind::device_n; + def->components = static_cast(names.as_array().size()); + } + def->alternate = parse_color_space(array[2], context, depth + 1); + if (def->alternate == nullptr) { + return nullptr; + } def->tint = parse_function( array[3], FunctionContext{context.resolve, context.load_stream}); return def; @@ -329,7 +363,11 @@ pdf::parse_color_space(const Object &object, const ColorSpaceContext &context) { if (family == "Pattern") { auto def = device_space(ColorSpaceKind::pattern, 1); if (array.size() >= 2) { - def->base = parse_color_space(array[1], context); + def->base = parse_color_space(array[1], context, depth + 1); + if (def->base == nullptr || def->base->kind == ColorSpaceKind::pattern) { + return nullptr; + } + def->components = def->base->components; } return def; } diff --git a/src/odr/internal/pdf/pdf_color.hpp b/src/odr/internal/pdf/pdf_color.hpp index 1f7e00b55..53436e6dd 100644 --- a/src/odr/internal/pdf/pdf_color.hpp +++ b/src/odr/internal/pdf/pdf_color.hpp @@ -67,11 +67,13 @@ struct ColorSpaceDef { /// How `parse_color_space` reaches indirect data: `resolve` dereferences, /// `load_stream` decodes a stream's bytes (ICC profiles, an Indexed palette /// given as a stream), and `named` looks up a colour space referenced by name -/// (a base space, the `/ColorSpace` resource table). +/// (a base space, the `/ColorSpace` resource table), forwarding its depth. struct ColorSpaceContext { std::function resolve; std::function load_stream; - std::function(const std::string &)> named; + std::function(const std::string &, + std::uint32_t)> + named; }; /// DeviceCMYK -> sRGB without an ICC engine: pdf.js's polynomial fit of Adobe's @@ -80,8 +82,9 @@ std::array cmyk_to_rgb(double c, double m, double y, double k); /// Build a colour space from its PDF object — a name (`/DeviceRGB`, …) or an /// array (`[/ICCBased 5 0 R]`, `[/Separation …]`, …). Returns `nullptr` for an -/// unsupported or malformed definition. +/// unsupported, malformed or excessively nested definition. std::shared_ptr -parse_color_space(const Object &object, const ColorSpaceContext &context); +parse_color_space(const Object &object, const ColorSpaceContext &context, + std::uint32_t depth = 0); } // namespace odr::internal::pdf diff --git a/src/odr/internal/pdf/pdf_document_parser.cpp b/src/odr/internal/pdf/pdf_document_parser.cpp index 8641620e7..8427d3805 100644 --- a/src/odr/internal/pdf/pdf_document_parser.cpp +++ b/src/odr/internal/pdf/pdf_document_parser.cpp @@ -1024,7 +1024,8 @@ ColorSpaceContext make_color_space_context(DocumentParser &parser, bind_parser_io(context, parser); if (resources != nullptr) { context.named = - [resources](const std::string &name) -> std::shared_ptr { + [resources](const std::string &name, + std::uint32_t) -> std::shared_ptr { const auto it = resources->color_space.find(name); return it != resources->color_space.end() ? it->second : nullptr; }; @@ -1173,13 +1174,15 @@ Resources *parse_resources(State &state, const Object &object) { // A base/alternate space may be named (referencing another `/ColorSpace` // entry); resolve it lazily from the same table, caching the result. context.named = - [&](const std::string &name) -> std::shared_ptr { + [&](const std::string &name, + const std::uint32_t depth) -> std::shared_ptr { if (const auto it = resources->color_space.find(name); it != resources->color_space.end()) { return it->second; } if (color_space_table.has_value(name)) { - auto def = parse_color_space(color_space_table.get(name), context); + auto def = + parse_color_space(color_space_table.get(name), context, depth); resources->color_space[name] = def; return def; } diff --git a/src/odr/internal/pdf/pdf_page_extractor.cpp b/src/odr/internal/pdf/pdf_page_extractor.cpp index 79a531389..218ed441c 100644 --- a/src/odr/internal/pdf/pdf_page_extractor.cpp +++ b/src/odr/internal/pdf/pdf_page_extractor.cpp @@ -527,7 +527,8 @@ resolve_inline_color_space(const Object &color_space, context.resolve = [](const Object &o) { return o; }; context.load_stream = [](const Object &) { return std::string{}; }; context.named = - [&resources](const std::string &name) -> std::shared_ptr { + [&resources](const std::string &name, + std::uint32_t) -> std::shared_ptr { const auto it = resources.color_space.find(name); return it != resources.color_space.end() ? it->second : nullptr; }; diff --git a/test/src/internal/pdf/pdf_color.cpp b/test/src/internal/pdf/pdf_color.cpp index 4742b1332..1025f6d1a 100644 --- a/test/src/internal/pdf/pdf_color.cpp +++ b/test/src/internal/pdf/pdf_color.cpp @@ -3,6 +3,8 @@ #include #include +#include +#include #include #include @@ -28,7 +30,7 @@ Object reals(std::initializer_list values) { return Object(Array(std::move(holder))); } -ColorSpaceDef device(const ColorSpaceKind kind, const int components) { +ColorSpaceDef device(const ColorSpaceKind kind, const std::int32_t components) { ColorSpaceDef def; def.kind = kind; def.components = components; @@ -110,6 +112,12 @@ TEST(PdfColor, indexed_palette) { EXPECT_EQ(def->kind, ColorSpaceKind::indexed); EXPECT_EQ(to_rgb(*def, {0}), (std::array{1, 0, 0})); EXPECT_EQ(to_rgb(*def, {1}), (std::array{0, 1, 0})); + for (const double value : {-1e100, 0.49}) { + EXPECT_EQ(to_rgb(*def, {value}), (std::array{1, 0, 0})); + } + for (const double value : {0.5, 1e100}) { + EXPECT_EQ(to_rgb(*def, {value}), (std::array{0, 1, 0})); + } } // Separation samples its tint transform, then converts through the alternate. @@ -178,3 +186,57 @@ TEST(PdfColor, name_resolves_device_space) { EXPECT_EQ(def->kind, ColorSpaceKind::device_cmyk); EXPECT_EQ(def->components, 4); } + +TEST(PdfColor, invalid_component_counts_and_palettes_are_rejected) { + for (const Integer count : {Integer{-1}, Integer{0}, Integer{2}, Integer{5}, + std::numeric_limits::max()}) { + Dictionary profile; + profile["N"] = Object(count); + EXPECT_EQ(parse_color_space( + Object(Array({Object(Name{"ICCBased"}), Object(profile)})), + context()), + nullptr) + << count; + } + for (const Integer hival : + {Integer{-1}, Integer{256}, std::numeric_limits::max()}) { + EXPECT_EQ( + parse_color_space( + Object(Array({Object(Name{"Indexed"}), Object(Name{"DeviceGray"}), + Object(hival), + Object(StandardString(std::string(256, '\0')))})), + context()), + nullptr) + << hival; + } + EXPECT_EQ( + parse_color_space( + Object(Array({Object(Name{"Indexed"}), Object(Name{"DeviceRGB"}), + Object(Integer{1}), Object(StandardString("short"))})), + context()), + nullptr); +} + +TEST(PdfColor, device_n_requires_nonempty_colorant_names) { + for (const Object &names : {Object(Name{"Spot"}), Object(Array{}), + Object(Array({Object(Integer{1})}))}) { + EXPECT_EQ( + parse_color_space(Object(Array({Object(Name{"DeviceN"}), names, + Object(Name{"DeviceRGB"}), Object{}})), + context()), + nullptr); + } +} + +TEST(PdfColor, recursive_alternates_are_rejected) { + Dictionary profile; + profile["N"] = Object(Integer{3}); + profile["Alternate"] = Object(ObjectReference{1, 0}); + const Object cyclic(Array({Object(Name{"ICCBased"}), Object(profile)})); + ColorSpaceContext ctx = context(); + ctx.resolve = [&](const Object &object) { + return object.is_reference() ? cyclic : object; + }; + EXPECT_EQ(parse_color_space(cyclic, ctx), nullptr); + ASSERT_NE(parse_color_space(Object(Name{"DeviceRGB"}), ctx), nullptr); +} diff --git a/test/src/internal/pdf/pdf_document_parser.cpp b/test/src/internal/pdf/pdf_document_parser.cpp index 9a32ae10f..511bc4a48 100644 --- a/test/src/internal/pdf/pdf_document_parser.cpp +++ b/test/src/internal/pdf/pdf_document_parser.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -748,3 +749,24 @@ TEST(DocumentParser, shading_reads_function_stream) { EXPECT_DOUBLE_EQ(shading->stops.back().rgb[0], sampled ? 1.0 : 0.0); } } + +TEST(DocumentParser, cyclic_color_space_names_are_rejected) { + PdfFileBuilder builder; + builder.object("<< /Type /Catalog /Pages 2 0 R >>") + .object("<< /Type /Pages /Kids [3 0 R] /Count 1 >>") + .object( + "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 10 10] " + "/Resources << /ColorSpace << /A /B /B /A /Good /DeviceRGB >> >> >>") + .trailer("/Root 1 0 R"); + DocumentParser parser( + std::make_unique(builder.build_classic())); + const auto document = parser.parse_document(); + const Page *page = first_page(*document); + ASSERT_NE(page, nullptr); + ASSERT_NE(page->resources, nullptr); + const auto &spaces = page->resources->color_space; + EXPECT_EQ(spaces.at("A"), nullptr); + EXPECT_EQ(spaces.at("B"), nullptr); + ASSERT_NE(spaces.at("Good"), nullptr); + EXPECT_EQ(spaces.at("Good")->components, 3); +} From 0df3144c9baa0cc04c7b82d963e2fa10cbb8fab8 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 6 Oct 2026 19:24:18 +0200 Subject: [PATCH 2/2] fix(pdf): keep usable colour spaces with a bad alternate or short palette An ICCBased space whose Alternate has another component count, or does not parse, now drops the alternate and uses the device space N names, where it rejected the whole space. An Indexed palette shorter than HiVal asks for keeps its entries and reads the missing bytes as 0, as before. A rejected space made scn read an Indexed colour as a grey index, so the valid entries showed wrong colours too. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01MxyTMutqSUJRGfxA8CyzMc --- CHANGELOG.md | 3 ++- src/odr/internal/pdf/AGENTS.md | 6 +++-- src/odr/internal/pdf/pdf_color.cpp | 15 +++++------- test/src/internal/pdf/pdf_color.cpp | 38 +++++++++++++++++++++++------ 4 files changed, 42 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36dd0caeb..f095dc5c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,8 @@ The release run heads these entries with the version and opens a fresh ## Unreleased - PDF color-space parsing bounds recursive definitions and rejects invalid - component counts and palettes. Large finite palette indices clip correctly. + component counts and palette ranges. Large finite palette indices clip + correctly. - Document path saves preserve the destination on write failure and retain lazy resources when saving repeatedly over the input. Existing permissions diff --git a/src/odr/internal/pdf/AGENTS.md b/src/odr/internal/pdf/AGENTS.md index 8193cea49..daa8917ea 100644 --- a/src/odr/internal/pdf/AGENTS.md +++ b/src/odr/internal/pdf/AGENTS.md @@ -165,8 +165,10 @@ above. So each run is raised by one font ascent: fallback, before range clipping. ISO 32000-1 7.10.5 requires at least 100 stack entries; the larger bound accommodates ordinary tint transforms. - Colour-space parsing carries a 64-level bound through arrays and named - resource aliases. Invalid ICC component counts, Indexed palettes and - unresolved base/alternate spaces are rejected before image allocation. + resource aliases. Invalid ICC component counts, an Indexed `/HiVal` outside + 0 to 255 and unresolved base spaces are rejected before image allocation. + An ICC alternate that cannot stand in is dropped for the device space `N` + names, and a short palette reads its missing bytes as 0. - CMYK is naive (no ICC) and overprint is ignored. CIE, ICCBased, Indexed, Separation, DeviceN and Lab resolve to RGB at emission by sampling the tint `/Function` (types 0, 2, 3, 4). diff --git a/src/odr/internal/pdf/pdf_color.cpp b/src/odr/internal/pdf/pdf_color.cpp index bc8a4bdd9..84aa0cde0 100644 --- a/src/odr/internal/pdf/pdf_color.cpp +++ b/src/odr/internal/pdf/pdf_color.cpp @@ -263,10 +263,11 @@ pdf::parse_color_space(const Object &object, const ColorSpaceContext &context, if (dict.has_value("Alternate")) { def->alternate = parse_color_space(dict.get("Alternate"), context, depth + 1); - if (def->alternate == nullptr || - def->alternate->kind == ColorSpaceKind::pattern || - def->alternate->components != def->components) { - return nullptr; + // an alternate that cannot stand in leaves the device space `N` names + if (def->alternate != nullptr && + (def->alternate->kind == ColorSpaceKind::pattern || + def->alternate->components != def->components)) { + def->alternate = nullptr; } } return def; @@ -318,11 +319,7 @@ pdf::parse_color_space(const Object &object, const ColorSpaceContext &context, } else { def->lookup = context.load_stream(array[3]); } - const auto entries = static_cast(def->hival) + 1; - if (entries > - def->lookup.size() / static_cast(def->base->components)) { - return nullptr; - } + // a short palette reads its missing bytes as 0 return def; } if (family == "Separation" || family == "DeviceN") { diff --git a/test/src/internal/pdf/pdf_color.cpp b/test/src/internal/pdf/pdf_color.cpp index 1025f6d1a..8176b7a9f 100644 --- a/test/src/internal/pdf/pdf_color.cpp +++ b/test/src/internal/pdf/pdf_color.cpp @@ -209,12 +209,19 @@ TEST(PdfColor, invalid_component_counts_and_palettes_are_rejected) { nullptr) << hival; } - EXPECT_EQ( - parse_color_space( - Object(Array({Object(Name{"Indexed"}), Object(Name{"DeviceRGB"}), - Object(Integer{1}), Object(StandardString("short"))})), - context()), - nullptr); +} + +// A palette shorter than `/HiVal` asks for keeps its entries; the missing +// bytes read as 0. +TEST(PdfColor, a_short_palette_reads_missing_bytes_as_zero) { + const auto def = parse_color_space( + Object(Array({Object(Name{"Indexed"}), Object(Name{"DeviceRGB"}), + Object(Integer{1}), + Object(StandardString(std::string("\xff\0\0\0\xff", 5)))})), + context()); + ASSERT_NE(def, nullptr); + EXPECT_EQ(to_rgb(*def, {0}), (std::array{1, 0, 0})); + EXPECT_EQ(to_rgb(*def, {1}), (std::array{0, 1, 0})); } TEST(PdfColor, device_n_requires_nonempty_colorant_names) { @@ -228,7 +235,7 @@ TEST(PdfColor, device_n_requires_nonempty_colorant_names) { } } -TEST(PdfColor, recursive_alternates_are_rejected) { +TEST(PdfColor, recursive_alternates_stop_at_the_depth_limit) { Dictionary profile; profile["N"] = Object(Integer{3}); profile["Alternate"] = Object(ObjectReference{1, 0}); @@ -237,6 +244,21 @@ TEST(PdfColor, recursive_alternates_are_rejected) { ctx.resolve = [&](const Object &object) { return object.is_reference() ? cyclic : object; }; - EXPECT_EQ(parse_color_space(cyclic, ctx), nullptr); + const auto def = parse_color_space(cyclic, ctx); + ASSERT_NE(def, nullptr); + EXPECT_EQ(def->components, 3); ASSERT_NE(parse_color_space(Object(Name{"DeviceRGB"}), ctx), nullptr); } + +// An alternate with another component count cannot stand in, so the space +// falls back to the device space `N` names. +TEST(PdfColor, a_mismatched_icc_alternate_is_ignored) { + Dictionary profile; + profile["N"] = Object(Integer{3}); + profile["Alternate"] = Object(Name{"DeviceGray"}); + const auto def = parse_color_space( + Object(Array({Object(Name{"ICCBased"}), Object(profile)})), context()); + ASSERT_NE(def, nullptr); + EXPECT_EQ(def->alternate, nullptr); + EXPECT_EQ(to_rgb(*def, {1, 0, 0}), (std::array{1, 0, 0})); +}