From c7706450d6459571adea3addc96696dfde62d513 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 4 Oct 2026 23:53:09 +0200 Subject: [PATCH 1/3] fix: enforce PowerPoint record and formatting bounds --- CHANGELOG.md | 3 + src/odr/internal/oldms/presentation/AGENTS.md | 5 +- .../oldms/presentation/ppt_document.cpp | 10 +- .../oldms/presentation/ppt_parser.cpp | 89 +++++++----- .../internal/oldms/presentation/ppt_style.cpp | 32 +++-- .../internal/oldms/presentation/ppt_style.hpp | 5 +- test/src/internal/oldms/ppt_test.cpp | 129 ++++++++++++++++++ 7 files changed, 219 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4eabd4a2e..f3c05471b 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 +- Legacy PowerPoint validates nested picture bounds, record lengths, and formatting + counts, limits text-container nesting, and avoids overflow in frame dimensions. + - Markdown replaces NUL bytes in inline code and rejects oversized input. RTF rejects unreadable streams; legacy PowerPoint rejects truncated text records. Both binary readers safely propagate I/O exceptions. diff --git a/src/odr/internal/oldms/presentation/AGENTS.md b/src/odr/internal/oldms/presentation/AGENTS.md index fa83b5708..d34fb39de 100644 --- a/src/odr/internal/oldms/presentation/AGENTS.md +++ b/src/odr/internal/oldms/presentation/AGENTS.md @@ -62,7 +62,10 @@ record type (`read_header`); a record that overruns its container; a missing mandatory child (`DrawingContainer`, `OfficeArtDgContainer`, `OfficeArtSpgrContainer`, via `require_child`); an `OfficeArtClientAnchor` whose `recLen` is neither 8 nor 16; a looping or empty `UserEditAtom` chain; -an unresolved `docPersistIdRef`; a slide `persistIdRef` not in the directory. +an unresolved `docPersistIdRef`; a slide `persistIdRef` not in the directory; +truncated skipped bytes, properties or persist-directory entries; an embedded +BLIP outside its store entry; formatting counts that do not cover the text. +Text-container recursion stops at 1024 levels. Pass through: an absent slide list, a shape with no anchor (unpositioned frame), nested groups and non-`Sp` records in a group, any unrecognised child. diff --git a/src/odr/internal/oldms/presentation/ppt_document.cpp b/src/odr/internal/oldms/presentation/ppt_document.cpp index bab6045ce..4a003dcc6 100644 --- a/src/odr/internal/oldms/presentation/ppt_document.cpp +++ b/src/odr/internal/oldms/presentation/ppt_document.cpp @@ -107,13 +107,15 @@ class ElementAdapter final : public AdapterBase { } [[nodiscard]] std::optional frame_width(const ElementIdentifier element_id) const override { - return anchor_measure(element_id, - [](const Anchor &a) { return a.right - a.left; }); + return anchor_measure(element_id, [](const Anchor &a) { + return std::int64_t{a.right} - a.left; + }); } [[nodiscard]] std::optional frame_height(const ElementIdentifier element_id) const override { - return anchor_measure(element_id, - [](const Anchor &a) { return a.bottom - a.top; }); + return anchor_measure(element_id, [](const Anchor &a) { + return std::int64_t{a.bottom} - a.top; + }); } [[nodiscard]] std::optional frame_z_index(const ElementIdentifier /*element_id*/) const override { diff --git a/src/odr/internal/oldms/presentation/ppt_parser.cpp b/src/odr/internal/oldms/presentation/ppt_parser.cpp index f6c6f1e3e..a9a81849e 100644 --- a/src/odr/internal/oldms/presentation/ppt_parser.cpp +++ b/src/odr/internal/oldms/presentation/ppt_parser.cpp @@ -8,7 +8,6 @@ #include #include #include -#include #include #include @@ -33,6 +32,16 @@ constexpr char line_break_mark = '\x0B'; /// The control characters `build_paragraphs` splits on. constexpr std::array control_marks = {paragraph_mark, line_break_mark}; +void skip_bytes(std::istream &in, const std::uint32_t count) { + if (!std::in_range(count)) { + throw std::runtime_error("ppt: record exceeds stream size limit"); + } + in.ignore(static_cast(count)); + if (!in || in.gcount() != count) { + throw std::runtime_error("ppt: truncated record"); + } +} + /// Sequentially walks a container's child records; the stream must be at the /// container body. next() leaves the stream at the child's body — read up to /// recLen bytes and report them via consume(), the rest is skipped. @@ -69,7 +78,7 @@ class ChildCursor { void skip_body() { if (m_body > 0) { - m_in->ignore(m_body); + skip_bytes(*m_in, m_body); m_remaining -= static_cast(m_body); m_body = 0; } @@ -205,7 +214,7 @@ StyledText style_pending(const PendingText &pending, if (at >= count) { break; // the runs also cover the implicit final paragraph mark } - const std::size_t end = std::min(count, at + run.count); + const std::size_t end = at + std::min(count - at, run.count); if (std::string text = pending.decode(at, end); !text.empty()) { result.push_back({std::move(text), resolve_style(run, context)}); } @@ -322,7 +331,7 @@ void read_pending_text(std::istream &in, const RecordHeader &header, std::vector read_style_atom(std::istream &in, const RecordHeader &header, const PendingText &pending) { - const std::string body = util::stream::read(in, header.recLen); + const std::string body = util::byte_stream::read_u8s(in, header.recLen); return parse_style_text_prop_atom(body, pending.char_count() + 1); } @@ -334,7 +343,10 @@ std::vector read_style_atom(std::istream &in, void gather_text(std::istream &in, const RecordHeader &container, StyledText &slide_text, const std::vector &outline_texts, - StyleContext &context) { + StyleContext &context, const std::size_t depth = 0) { + if (depth >= 1024) { + throw std::runtime_error("ppt: text container nesting limit exceeded"); + } PendingText pending; const auto flush = [&](const std::vector &runs) { if (pending.has_value()) { @@ -367,7 +379,7 @@ void gather_text(std::istream &in, const RecordHeader &container, } } else if (child->is_container()) { flush({}); - gather_text(in, *child, slide_text, outline_texts, context); + gather_text(in, *child, slide_text, outline_texts, context, depth + 1); children.consume(child->recLen); } // Other atoms: left unconsumed; the cursor skips their bodies. @@ -400,9 +412,10 @@ Shape read_shape(std::istream &in, const RecordHeader &header, // rh.recInstance is the property count; complex data (skipped by the // cursor) follows the array ([MS-ODRAW] 2.2.9). const std::uint16_t count = child->recInstance; - for (std::uint16_t i = 0; - i < count && (i + 1) * sizeof(OfficeArtFopte) <= child->recLen; - ++i) { + if (count > child->recLen / sizeof(OfficeArtFopte)) { + throw std::runtime_error("ppt: truncated property array"); + } + for (std::size_t i = 0; i < count; ++i) { const auto property = util::byte_stream::read(in); children.consume(sizeof(OfficeArtFopte)); if (property.fComplex != 0) { @@ -479,12 +492,18 @@ void read_persist_directory(std::istream &in, const RecordHeader &header, constexpr std::uint32_t field_size = sizeof(std::uint32_t); std::uint32_t remaining = header.recLen; - while (remaining >= field_size) { + while (remaining > 0) { + if (remaining < field_size) { + throw std::runtime_error("ppt: truncated persist directory entry"); + } const std::uint32_t entry = read_u32(in); // persistId:20 | cPersist:12 remaining -= field_size; const std::uint32_t persist_id = entry & 0x000FFFFF; const std::uint32_t count = entry >> 20; - for (std::uint32_t i = 0; i < count && remaining >= field_size; ++i) { + if (count == 0 || count > remaining / field_size || persist_id > 0xFFFFE) { + throw std::runtime_error("ppt: invalid persist directory entry"); + } + for (std::uint32_t i = 0; i < count; ++i) { const std::uint32_t persist_offset = read_u32(in); remaining -= field_size; directory.emplace(persist_id + i, persist_offset); @@ -569,23 +588,19 @@ std::uint32_t blip_prefix_size(const RecordHeader &header) { return uid_size + (two_uids ? uid_size : 0) + 1; } -/// Reads an OfficeArt BLIP record ([MS-ODRAW] 2.2.23) at the stream position -/// and returns its image file bytes; empty for BLIP types not modelled -/// (WMF/EMF/PICT/DIB/TIFF). -std::string read_blip_record(std::istream &in) { - const RecordHeader header = read_record_header(in); - +/// Reads a BLIP body ([MS-ODRAW] 2.2.23); unsupported image types return empty. +std::string read_blip_body(std::istream &in, const RecordHeader &header) { if (header.recType != RT_OfficeArtBlipJPEG && header.recType != RT_OfficeArtBlipPNG) { - in.ignore(header.recLen); + skip_bytes(in, header.recLen); return {}; } const std::uint32_t prefix = blip_prefix_size(header); if (header.recLen < prefix) { throw std::runtime_error("ppt: truncated BLIP record"); } - in.ignore(prefix); - return util::stream::read(in, header.recLen - prefix); + skip_bytes(in, prefix); + return util::byte_stream::read_u8s(in, header.recLen - prefix); } /// One slot of the BLIP store: the offset of the BLIP in the "Pictures" @@ -622,32 +637,31 @@ std::vector read_blip_store(std::istream &in, while (const std::optional child = entries.next()) { BlipSlot &slot = slots.emplace_back(); if (child->recType == RT_OfficeArtFBSE) { - const auto fbse = util::byte_stream::read(in); entries.consume(sizeof(OfficeArtFbseFixed)); - in.ignore(fbse.cbName); + const auto fbse = util::byte_stream::read(in); entries.consume(fbse.cbName); + skip_bytes(in, fbse.cbName); // An embedded BLIP follows the name ([MS-ODRAW] 2.2.32); otherwise the // BLIP lives in the delay ("Pictures") stream at foDelay. if (child->recLen > sizeof(OfficeArtFbseFixed) + std::uint32_t{fbse.cbName}) { const std::uint32_t remaining = child->recLen - sizeof(OfficeArtFbseFixed) - fbse.cbName; - slot.data = read_blip_record(in); + if (remaining < sizeof(RecordHeader)) { + throw std::runtime_error("ppt: truncated embedded BLIP header"); + } + const RecordHeader blip = read_record_header(in); + if (blip.recLen != remaining - sizeof(RecordHeader)) { + throw std::runtime_error("ppt: embedded BLIP exceeds store entry"); + } + slot.data = read_blip_body(in, blip); entries.consume(remaining); } else { slot.fo_delay = fbse.foDelay; } } else if (child->recType == RT_OfficeArtBlipJPEG || child->recType == RT_OfficeArtBlipPNG) { - // A BLIP directly in the store occupies a slot of its own; its header is - // already consumed, so the body is read here rather than via - // read_blip_record. - const std::uint32_t prefix = blip_prefix_size(*child); - if (child->recLen < prefix) { - throw std::runtime_error("ppt: truncated BLIP record"); - } - in.ignore(prefix); - slot.data = util::stream::read(in, child->recLen - prefix); + slot.data = read_blip_body(in, *child); entries.consume(child->recLen); } // Other record types leave an empty slot; the cursor skips them. @@ -706,7 +720,8 @@ collect_slides(std::istream ¤t_user, std::istream &document, ElementRegistry ®istry, StyleContext &context) { // Newest user edit offset, from the Current User stream. const CurrentUserAtomHead head = read_current_user_atom_head(current_user); - if (head.rh.recType != RT_CurrentUserAtom) { + if (head.rh.recType != RT_CurrentUserAtom || + head.rh.recLen < sizeof(CurrentUserAtomHead) - sizeof(RecordHeader)) { throw std::runtime_error( "ppt: invalid CurrentUserAtom in \"Current User\" stream"); } @@ -720,7 +735,10 @@ collect_slides(std::istream ¤t_user, std::istream &document, for (bool first = true; edit_offset != 0; first = false) { document.clear(); document.seekg(edit_offset); - read_header(document, RT_UserEditAtom); + const RecordHeader edit_header = read_header(document, RT_UserEditAtom); + if (edit_header.recLen < sizeof(UserEditAtomBody)) { + throw std::runtime_error("ppt: truncated UserEditAtom"); + } const UserEditAtomBody edit = read_user_edit_atom_body(document); if (first) { doc_persist_id = edit.docPersistIdRef; @@ -836,7 +854,8 @@ collect_slides(std::istream ¤t_user, std::istream &document, pictures_stream != nullptr) { pictures_stream->clear(); pictures_stream->seekg(slot.fo_delay); - slot.data = read_blip_record(*pictures_stream); + slot.data = read_blip_body(*pictures_stream, + read_record_header(*pictures_stream)); slot.fo_delay = 0xFFFFFFFF; // resolved (possibly to unsupported/empty) } shape.image = slot.data; diff --git a/src/odr/internal/oldms/presentation/ppt_style.cpp b/src/odr/internal/oldms/presentation/ppt_style.cpp index 2bdf95266..616f5996a 100644 --- a/src/odr/internal/oldms/presentation/ppt_style.cpp +++ b/src/odr/internal/oldms/presentation/ppt_style.cpp @@ -197,22 +197,28 @@ presentation::parse_style_text_prop_atom(const std::string_view body, const std::size_t char_count) { BodyCursor cursor(body); - // rgTextPFRun: skipped; the counts cover the whole corresponding text. - for (std::size_t covered = 0; - covered < char_count && cursor.remaining() > 0;) { - covered += cursor.read(); // count - cursor.skip(2); // indentLevel + const auto read_count = [&](std::size_t &remaining) { + const auto count = cursor.read(); + if (count > remaining) { + throw std::runtime_error("ppt: formatting run exceeds text length"); + } + remaining -= count; + return count; + }; + + // [MS-PPT] 2.9.44: both run arrays must cover the corresponding text. + for (std::size_t remaining = char_count; remaining > 0;) { + read_count(remaining); + cursor.skip(2); // indentLevel skip_text_pf_exception(cursor); } // rgTextCFRun. std::vector runs; - for (std::size_t covered = 0; - covered < char_count && cursor.remaining() > 0;) { + for (std::size_t remaining = char_count; remaining > 0;) { TextCFRun run; - run.count = cursor.read(); + run.count = read_count(remaining); read_text_cf_exception(cursor, run); - covered += run.count; runs.push_back(run); } return runs; @@ -250,8 +256,12 @@ std::uint32_t presentation::resolve_style(const TextCFRun &run, } style.font_name = context.fonts[*run.font_ref]; } - context.styles.push_back(style); - return static_cast(context.styles.size() - 1); + if (!std::in_range(context.styles.size())) { + throw std::length_error("ppt: too many character styles"); + } + const auto index = static_cast(context.styles.size()); + context.styles.push_back(std::move(style)); + return index; } } // namespace odr::internal::oldms diff --git a/src/odr/internal/oldms/presentation/ppt_style.hpp b/src/odr/internal/oldms/presentation/ppt_style.hpp index 0d76c34bb..9c87e03b0 100644 --- a/src/odr/internal/oldms/presentation/ppt_style.hpp +++ b/src/odr/internal/oldms/presentation/ppt_style.hpp @@ -23,9 +23,8 @@ struct TextCFRun final { }; /// Parses a StyleTextPropAtom body ([MS-PPT] 2.9.44): the paragraph-level runs -/// are skipped, the character-level runs returned. `char_count` is the -/// corresponding text's length in characters plus one (the runs also cover an -/// implicit final paragraph mark); fewer covered characters end the parse. +/// are skipped, the character runs returned. `char_count` includes the final +/// paragraph mark. std::vector parse_style_text_prop_atom(std::string_view body, std::size_t char_count); diff --git a/test/src/internal/oldms/ppt_test.cpp b/test/src/internal/oldms/ppt_test.cpp index 9a03f5895..7dd3f792e 100644 --- a/test/src/internal/oldms/ppt_test.cpp +++ b/test/src/internal/oldms/ppt_test.cpp @@ -9,6 +9,10 @@ #include #include +#include +#include +#include +#include #include #include #include @@ -26,6 +30,55 @@ using odr::test::oldms::append_u16; using odr::test::oldms::append_u32; using odr::test::oldms::collect_text; +namespace { +namespace ppt = internal::oldms::presentation; + +std::string ppt_record(const std::uint16_t type, const std::string &body, + const std::uint16_t flags = 0) { + std::string record; + append_u16(record, flags); + append_u16(record, type); + append_u32(record, static_cast(body.size())); + return record + body; +} + +std::shared_ptr +ppt_files(const std::string &document_body, const std::string &slide_body = {}, + const std::string &directory_tail = {}) { + std::string document = + ppt_record(ppt::RT_DocumentContainer, document_body, 15); + const auto slide_offset = static_cast(document.size()); + document += ppt_record(ppt::RT_SlideContainer, slide_body, 15); + const auto directory_offset = static_cast(document.size()); + std::string directory; + append_u32(directory, (2u << 20) | 1u); + append_u32(directory, 0); + append_u32(directory, slide_offset); + document += + ppt_record(ppt::RT_PersistDirectoryAtom, directory + directory_tail); + const auto edit_offset = static_cast(document.size()); + std::string edit(12, '\0'); + append_u32(edit, directory_offset); + append_u32(edit, 1); + append_u32(edit, 3); + edit.resize(28, '\0'); + document += ppt_record(ppt::RT_UserEditAtom, edit); + std::string user; + append_u32(user, 20); + append_u32(user, ppt::current_user_token_plain); + append_u32(user, edit_offset); + user.resize(20, '\0'); + auto files = std::make_shared(); + files->copy(std::make_shared(document), + internal::AbsPath("/PowerPoint Document")); + files->copy(std::make_shared( + ppt_record(ppt::RT_CurrentUserAtom, user)), + internal::AbsPath("/Current User")); + return files; +} + +} // namespace + // A StyleTextPropAtom body ([MS-PPT] 2.9.44): the paragraph-level runs are // skipped (including their mask-dependent fields), the character-level runs // map masks/CFStyle/fontRef/size/color onto TextCFRun. @@ -209,3 +262,79 @@ TEST(OldMs, ppt_text_bytes_require_complete_record) { throwing.exceptions(std::ios::failbit | std::ios::badbit); EXPECT_THROW(read_raw_text_bytes(throwing, 3), std::ios_base::failure); } + +TEST(OldMs, ppt_rejects_invalid_record_boundaries) { + EXPECT_NO_THROW(ppt::Document{ppt_files({})}); + EXPECT_THROW(ppt::Document{ppt_files({}, {}, "x")}, std::runtime_error); + std::string missing_offsets; + append_u32(missing_offsets, (2u << 20) | 3u); + EXPECT_THROW(ppt::Document{ppt_files({}, {}, missing_offsets)}, + std::runtime_error); + + std::string blip = + ppt_record(ppt::RT_OfficeArtBlipPNG, std::string(17, '\0')); + blip.pop_back(); + const std::string fbse = + ppt_record(ppt::RT_OfficeArtFBSE, std::string(36, '\0') + blip); + const std::string store = + ppt_record(ppt::RT_OfficeArtBStoreContainer, fbse, 15); + const std::string drawing = + ppt_record(ppt::RT_DrawingGroup, + ppt_record(ppt::RT_OfficeArtDggContainer, store, 15), 15); + EXPECT_THROW(ppt::Document{ppt_files(drawing)}, std::runtime_error); +} + +TEST(OldMs, ppt_text_nesting_is_bounded) { + const auto files = [](const std::size_t depth) { + std::string text = ppt_record(ppt::RT_TextBytesAtom, "visible"); + for (std::size_t i = 0; i < depth; ++i) { + text = ppt_record(0x7777, text, 15); + } + const std::string shape = + ppt_record(ppt::RT_OfficeArtSpContainer, + ppt_record(ppt::RT_OfficeArtClientTextbox, text), 15); + const std::string slide = ppt_record( + ppt::RT_Drawing, + ppt_record(ppt::RT_OfficeArtDgContainer, + ppt_record(ppt::RT_OfficeArtSpgrContainer, shape, 15), 15), + 15); + std::string persist; + append_u32(persist, 2); + return ppt_files(ppt_record(ppt::RT_SlideListWithText, + ppt_record(ppt::RT_SlidePersistAtom, persist), + 15), + slide); + }; + const Document shallow(std::make_shared(files(4))); + EXPECT_EQ(collect_text(shallow.root_element()), "visible"); + EXPECT_THROW(ppt::Document{files(1024)}, std::runtime_error); +} + +TEST(OldMs, ppt_style_runs_require_complete_coverage) { + std::string body; + append_u32(body, 3); + append_u16(body, 0); + append_u32(body, 0); + append_u32(body, 3); + append_u32(body, 0); + EXPECT_EQ(ppt::parse_style_text_prop_atom(body, 3).size(), 1); + EXPECT_THROW(ppt::parse_style_text_prop_atom(body, 2), std::runtime_error); + EXPECT_THROW(ppt::parse_style_text_prop_atom(body, 4), std::runtime_error); + body.pop_back(); + EXPECT_THROW(ppt::parse_style_text_prop_atom(body, 3), std::runtime_error); +} + +TEST(OldMs, ppt_anchor_differences_do_not_overflow) { + ppt::Document document(ppt_files({})); + auto [id, element, frame] = + document.element_registry().create_frame_element(); + frame.anchor = ppt::Anchor{std::numeric_limits::min(), + std::numeric_limits::min(), + std::numeric_limits::max(), + std::numeric_limits::max()}; + const Frame handle = Element(document.element_adapter(), id).as_frame(); + const Measure extent(4294967295.0 / ppt::master_units_per_inch, + DynamicUnit("in")); + EXPECT_EQ(handle.width(), extent); + EXPECT_EQ(handle.height(), extent); +} From 856d022420f4be52c5e0a26060e2d3cdf25310a9 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 10:45:04 +0200 Subject: [PATCH 2/3] fix(ppt): cut an overshooting style run and allow BLIP padding A formatting run past the end of its text now threw, and the exception refused the whole presentation. Writers do not always get the counts exactly right, so the run is now cut to the remaining text, as LibreOffice does. style_pending already gives characters after the last run the default style. A body that ends inside a run still throws. The embedded-BLIP check required the BLIP to fill its store entry exactly, although it reports a BLIP that exceeds it. It now refuses only a larger BLIP, and the cursor consumes the bytes that were read, so padding is skipped. ppt_style.cpp also includes and , which it uses. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Gb1fLafqqzehpqPuU6uBfn --- .../oldms/presentation/ppt_parser.cpp | 4 ++-- .../internal/oldms/presentation/ppt_style.cpp | 20 +++++++++++-------- test/src/internal/oldms/ppt_test.cpp | 8 ++++++-- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/odr/internal/oldms/presentation/ppt_parser.cpp b/src/odr/internal/oldms/presentation/ppt_parser.cpp index a9a81849e..5fc89ecd9 100644 --- a/src/odr/internal/oldms/presentation/ppt_parser.cpp +++ b/src/odr/internal/oldms/presentation/ppt_parser.cpp @@ -651,11 +651,11 @@ std::vector read_blip_store(std::istream &in, throw std::runtime_error("ppt: truncated embedded BLIP header"); } const RecordHeader blip = read_record_header(in); - if (blip.recLen != remaining - sizeof(RecordHeader)) { + if (blip.recLen > remaining - sizeof(RecordHeader)) { throw std::runtime_error("ppt: embedded BLIP exceeds store entry"); } slot.data = read_blip_body(in, blip); - entries.consume(remaining); + entries.consume(sizeof(RecordHeader) + blip.recLen); } else { slot.fo_delay = fbse.foDelay; } diff --git a/src/odr/internal/oldms/presentation/ppt_style.cpp b/src/odr/internal/oldms/presentation/ppt_style.cpp index 616f5996a..c1e68a647 100644 --- a/src/odr/internal/oldms/presentation/ppt_style.cpp +++ b/src/odr/internal/oldms/presentation/ppt_style.cpp @@ -2,6 +2,8 @@ #include +#include +#include #include namespace odr::internal::oldms::presentation { @@ -197,17 +199,18 @@ presentation::parse_style_text_prop_atom(const std::string_view body, const std::size_t char_count) { BodyCursor cursor(body); + // [MS-PPT] 2.9.44: both run arrays cover the text. A run past its end is + // cut, as LibreOffice does, so a slightly wrong count does not refuse the + // file; `style_pending` gives characters after the last run the default. const auto read_count = [&](std::size_t &remaining) { - const auto count = cursor.read(); - if (count > remaining) { - throw std::runtime_error("ppt: formatting run exceeds text length"); - } + const auto count = + std::min(cursor.read(), remaining); remaining -= count; - return count; + return static_cast(count); }; - // [MS-PPT] 2.9.44: both run arrays must cover the corresponding text. - for (std::size_t remaining = char_count; remaining > 0;) { + for (std::size_t remaining = char_count; + remaining > 0 && cursor.remaining() > 0;) { read_count(remaining); cursor.skip(2); // indentLevel skip_text_pf_exception(cursor); @@ -215,7 +218,8 @@ presentation::parse_style_text_prop_atom(const std::string_view body, // rgTextCFRun. std::vector runs; - for (std::size_t remaining = char_count; remaining > 0;) { + for (std::size_t remaining = char_count; + remaining > 0 && cursor.remaining() > 0;) { TextCFRun run; run.count = read_count(remaining); read_text_cf_exception(cursor, run); diff --git a/test/src/internal/oldms/ppt_test.cpp b/test/src/internal/oldms/ppt_test.cpp index 7dd3f792e..e0c7f5cb1 100644 --- a/test/src/internal/oldms/ppt_test.cpp +++ b/test/src/internal/oldms/ppt_test.cpp @@ -310,7 +310,7 @@ TEST(OldMs, ppt_text_nesting_is_bounded) { EXPECT_THROW(ppt::Document{files(1024)}, std::runtime_error); } -TEST(OldMs, ppt_style_runs_require_complete_coverage) { +TEST(OldMs, ppt_style_runs_are_bounded_by_the_text) { std::string body; append_u32(body, 3); append_u16(body, 0); @@ -318,7 +318,11 @@ TEST(OldMs, ppt_style_runs_require_complete_coverage) { append_u32(body, 3); append_u32(body, 0); EXPECT_EQ(ppt::parse_style_text_prop_atom(body, 3).size(), 1); - EXPECT_THROW(ppt::parse_style_text_prop_atom(body, 2), std::runtime_error); + // a run past the end of the text is cut, not refused + const auto cut = ppt::parse_style_text_prop_atom(body, 2); + ASSERT_EQ(cut.size(), 1); + EXPECT_EQ(cut[0].count, 2); + // too short a run array reads into the next one and runs out of bytes EXPECT_THROW(ppt::parse_style_text_prop_atom(body, 4), std::runtime_error); body.pop_back(); EXPECT_THROW(ppt::parse_style_text_prop_atom(body, 3), std::runtime_error); From 851c366cc98510970f6061707235ff451b399bbc Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 10:51:51 +0200 Subject: [PATCH 3/3] refactor: move the checked byte skip into byte_stream The ppt reader had its own skip_bytes. A skip that stops short is the same failure as a short read, so byte_stream::skip now sits next to read and throws the same error. It skips a count above streamsize in steps, so the caller needs no range check. The ppt reader uses it in all four places. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Gb1fLafqqzehpqPuU6uBfn --- .../internal/oldms/presentation/ppt_parser.cpp | 18 ++++-------------- src/odr/internal/util/byte_stream_util.cpp | 13 +++++++++++++ src/odr/internal/util/byte_stream_util.hpp | 2 ++ test/src/internal/util/stream_util_test.cpp | 8 ++++++++ 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/odr/internal/oldms/presentation/ppt_parser.cpp b/src/odr/internal/oldms/presentation/ppt_parser.cpp index 5fc89ecd9..e3728b2ed 100644 --- a/src/odr/internal/oldms/presentation/ppt_parser.cpp +++ b/src/odr/internal/oldms/presentation/ppt_parser.cpp @@ -32,16 +32,6 @@ constexpr char line_break_mark = '\x0B'; /// The control characters `build_paragraphs` splits on. constexpr std::array control_marks = {paragraph_mark, line_break_mark}; -void skip_bytes(std::istream &in, const std::uint32_t count) { - if (!std::in_range(count)) { - throw std::runtime_error("ppt: record exceeds stream size limit"); - } - in.ignore(static_cast(count)); - if (!in || in.gcount() != count) { - throw std::runtime_error("ppt: truncated record"); - } -} - /// Sequentially walks a container's child records; the stream must be at the /// container body. next() leaves the stream at the child's body — read up to /// recLen bytes and report them via consume(), the rest is skipped. @@ -78,7 +68,7 @@ class ChildCursor { void skip_body() { if (m_body > 0) { - skip_bytes(*m_in, m_body); + util::byte_stream::skip(*m_in, m_body); m_remaining -= static_cast(m_body); m_body = 0; } @@ -592,14 +582,14 @@ std::uint32_t blip_prefix_size(const RecordHeader &header) { std::string read_blip_body(std::istream &in, const RecordHeader &header) { if (header.recType != RT_OfficeArtBlipJPEG && header.recType != RT_OfficeArtBlipPNG) { - skip_bytes(in, header.recLen); + util::byte_stream::skip(in, header.recLen); return {}; } const std::uint32_t prefix = blip_prefix_size(header); if (header.recLen < prefix) { throw std::runtime_error("ppt: truncated BLIP record"); } - skip_bytes(in, prefix); + util::byte_stream::skip(in, prefix); return util::byte_stream::read_u8s(in, header.recLen - prefix); } @@ -640,7 +630,7 @@ std::vector read_blip_store(std::istream &in, entries.consume(sizeof(OfficeArtFbseFixed)); const auto fbse = util::byte_stream::read(in); entries.consume(fbse.cbName); - skip_bytes(in, fbse.cbName); + util::byte_stream::skip(in, fbse.cbName); // An embedded BLIP follows the name ([MS-ODRAW] 2.2.32); otherwise the // BLIP lives in the delay ("Pictures") stream at foDelay. if (child->recLen > diff --git a/src/odr/internal/util/byte_stream_util.cpp b/src/odr/internal/util/byte_stream_util.cpp index 504064494..f60ea711b 100644 --- a/src/odr/internal/util/byte_stream_util.cpp +++ b/src/odr/internal/util/byte_stream_util.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include namespace odr::internal::util { @@ -37,6 +38,18 @@ void byte_stream::read(std::istream &in, char *out, std::size_t count) { } } +void byte_stream::skip(std::istream &in, std::uint64_t count) { + while (count > 0) { + const auto step = static_cast(std::min( + count, std::numeric_limits::max())); + in.ignore(step); + if (in.gcount() != step) { + throw_exhausted(); + } + count -= static_cast(step); + } +} + std::uint8_t byte_stream::read_u8(std::istream &in) { const auto c = in.rdbuf()->sbumpc(); if (c == eof) { diff --git a/src/odr/internal/util/byte_stream_util.hpp b/src/odr/internal/util/byte_stream_util.hpp index a49db9305..4b5840585 100644 --- a/src/odr/internal/util/byte_stream_util.hpp +++ b/src/odr/internal/util/byte_stream_util.hpp @@ -31,6 +31,8 @@ template std::optional try_read(std::istream &in) { } void read(std::istream &in, char *out, std::size_t count); +/// Skips exactly @p count bytes; throws like `read` where the stream ends. +void skip(std::istream &in, std::uint64_t count); template void read(std::istream &in, T &out) { read(in, reinterpret_cast(&out), sizeof(T)); diff --git a/test/src/internal/util/stream_util_test.cpp b/test/src/internal/util/stream_util_test.cpp index 699fbeab7..1f9920ec6 100644 --- a/test/src/internal/util/stream_util_test.cpp +++ b/test/src/internal/util/stream_util_test.cpp @@ -108,6 +108,14 @@ TEST(Stream, copying_reports_input_and_output_failures) { EXPECT_THROW(stream::read(broken), std::ios_base::failure); } +TEST(ByteStream, skip_moves_exactly_and_refuses_a_short_stream) { + std::istringstream in("abcdef"); + byte_stream::skip(in, 0); + byte_stream::skip(in, 2); + EXPECT_EQ(byte_stream::read_u8(in), 'c'); + EXPECT_THROW(byte_stream::skip(in, 4), std::runtime_error); +} + TEST(ByteStream, length_prefixed_reads_handle_chunks_and_stream_exceptions) { const std::string data(4097, 'x'); std::istringstream in(data);