Skip to content

fix: whole-codebase correctness and conciseness review - #658

Merged
andiwand merged 13 commits into
mainfrom
review/correctness-and-conciseness
Aug 7, 2026
Merged

fix: whole-codebase correctness and conciseness review#658
andiwand merged 13 commits into
mainfrom
review/correctness-and-conciseness

Conversation

@andiwand

@andiwand andiwand commented Aug 7, 2026

Copy link
Copy Markdown
Member

🤖 Generated with Claude Code

A whole-codebase review for correctness and conciseness, run as twelve parallel
per-area reviews (public API, common/util/detection, odf, ooxml, oldms, pdf,
html renderer, font/zip/cfb/crypto, python+cli, jni+android, apple, tests) with
every finding verified against a build and the full suite.

172 files, +2651 / −3442. Net −799 comment lines.

Verification

Baseline at main 759 tests, 750 passed, 0 failed
Build (lib + tests + CLI + JNI) clean, 0 warnings
Full suite 767 tests, 758 passed, 9 skipped, 0 failed
JNI JUnit (ctest) passed
Python bindings build clean, 49/49 pytest
Apple bindings (ODR_APPLE=ON) build clean, 0 compiler warnings
clang-format clean

The bugs that matter most

CID-keyed CFF fonts had wrong glyph metrics in every PDF. The parser never
read /FDArray or /FDSelect, so nominalWidthX stayed 0 and every
charstring width resolved against it. Real subset fonts came out with negative
advances that wrapped into an advanceWidthMax of 65431 — 65 em. All 152
glyphs of two corpus PDFs now agree with fontTools' own CFF interpreter, with
zero mismatches.

TablePosition::to_column_string produced wrong column names. The borrow
was column /= 27 where bijective base 26 needs column / 26 - 1; they agree
only while the quotient is ≤ 27. 792 of the first 20 000 columns were wrong,
the first at 727 (ZZ instead of AAZ). Reaches spreadsheet headers and the
Python and JNI bindings.

Document content was written into HTML attributes unescaped — link href,
bookmark id, image src, font name, font shadow, the four cell border
strings, archive entry paths. A quote in document content broke out of the
attribute.

Two unbounded recursions and an iterator invalidation in ODF and OOXML
style resolution: a cyclic w:basedOn / style:parent-style-name recursed
until the stack ran out, and resolving an unknown parent id through
operator[] rehashed the map that generate_styles_ was iterating.

JNI wrappers could be collected mid-call. Html.translate and
HtmlService.bringOffline passed a bare handle into a static native, so
nothing in the Java frame referred to the wrapper. Plus local-reference leaks
that overflow ART's table on a chatty translate.

Python reported failed typed casts as valid. Only Element::operator bool
was bound, so bool(paragraph.as_slide()) was True for a cast that failed
and callers silently read defaults. Iterator-yielded elements also did not keep
their document alive.

Beyond those: bounds and overflow hardening across the hand-rolled binary
parsers (PDF, CFB, ZIP, fonts, oldms) — unchecked substr, wrapping lengths,
allocation bombs from file-supplied counts, empty-stack pops, unbounded
nesting — and Path::descendant_of was simply backwards, with its only caller
written against the inverted answer so the two bugs cancelled.

⚠️ Reference output changes — 72 of 1141 files

Every diff is traced and each is a correction. The output repos need
regenerating before this merges.

  • 65 files — repeated ODS cells lose contenteditable.
    element_is_editable recursed to the parent before testing sheet_cell, and
    a sheet cell always has a sheet parent, so the branch was dead and everything
    reported editable. Confirmed against the source XML that these are
    table:number-columns-repeated cells sharing one node, where editing one
    would silently rewrite all of them.
  • 2 files& now escaped in href. The current reference output is
    invalid HTML.
  • 5 files — the PDF font-metrics fix above.

Conciseness

Comments were cut to the AGENTS.md bar — a couple of terse lines stating the
key point, citing the spec rather than paraphrasing it. Worst offenders:
html/pdf_file.cpp 534 → 344 comment lines, pdf_page_element.hpp 125 → 51,
pdf_document_element.hpp 197 → 111. Several were also factually wrong
(sfnt_transform.hpp still documented a BMP-only limitation the code had
outgrown). Structural dedup where it was clearly safe: three near-identical
HTML fragment classes behind one template, 23 pybind11 class bindings behind a
helper, the override_if_set collapse in style.cpp, link_child in the ODF
registry.

A stacked follow-up carries the findings this PR deliberately left alone.

andiwand and others added 12 commits August 7, 2026 23:29
A CID-keyed CFF keeps its Private DICTs per FD, so nominalWidthX stayed 0 and
every charstring width resolved against it. Real subset fonts came out with
negative advances that wrapped into an advanceWidthMax of 65431 - 65 em.

Also hardens the binary readers these share: Type1 charstring and /Subrs
bounds, CFF INDEX offset monotonicity, CFB name length and directory cycles,
zip short-read reporting, AES-GCM length validation and the SVM header skip.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
Link href, bookmark id, image src, font name, font shadow, the four cell
border strings and archive entry paths were written into attributes raw, so a
quote in document content broke out of the attribute.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
element_is_editable recursed to the parent before testing sheet_cell, and a
sheet cell always has a sheet parent, so the branch was dead and every element
reported editable. Repeated cells share one node, so editing one silently
rewrote all of them.

Also breaks a cyclic style:parent-style-name, stops generate_style_ growing
m_index while it is iterated, and keeps text_set_content("") from leaving the
registry pointing at freed pugixml nodes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
…erating

Resolving w:basedOn through operator[] inserted a default entry for an unknown
parent id, rehashing the map generate_styles_ was iterating. The in-progress
guard inserted a null entry that the next visit did not recognise, so an
A -> B -> A chain recursed until the stack ran out.

Also tolerates a missing <dimension>, which is minOccurs=0 in ECMA-376
18.3.1.35 and previously aborted the whole workbook.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
Unbalanced Q popped an empty graphics-state stack; array and dictionary
nesting recursed without limit; a /Kids cycle recursed until the stack ran
out; the type 0/2/3/4 functions indexed /C1, /Encode, /Domain and the
PostScript operand stack without checking their length; a missing /Length
threw out of read_object_stream instead of taking the documented
scan-to-endstream path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
PlcMap::n() computed (cbPlc - 4) / stride on a size taken from the file, so a
truncated table stream wrapped it to a huge count and read far past the
buffer. [MS-DOC] 2.2.2 requires the division to be whole.

Also rejects a non-ascending PlcPcd.aCp ([MS-DOC] 2.8.35), an odd
TextCharsAtom recLen ([MS-PPT] 2.9.42) and a missing .doc table stream, and
routes the uncompressed string readers through the throwing byte_stream.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
descendant_of tested the relation backwards, and its only caller was written
against the inverted answer, so two bugs cancelled. parent() dropped the ..
prefix of an escaping relative path. The prefix checks were not component
aligned, so /ab/c counted as a descendant of /a.

SystemFilesystem::copy translated the target path twice, writing to
<root>/<root>/<path>. VirtualFileWalker::clone left the iterator pointing into
the source map. byte_stream::try_read wrote over the optional, not its value.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
…as_*

to_column_string borrowed with column /= 27 where bijective base 26 needs
column / 26 - 1. The two agree only while the quotient is at most 27, so 792
of the first 20000 columns were wrong, the first at 727 - ZZ instead of AAZ.

All 23 Element::as_*() dereferenced the adapter without the exists_() guard
every other accessor uses, so a default-constructed Element was a null
dereference. Color::argb() shifted a promoted int into the sign bit for any
alpha >= 128, and alpha defaults to 255.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
Html.translate and HtmlService.bringOffline passed a bare handle into a static
native, so nothing in the Java frame referred to the wrapper and the reaper
could destroy it mid-call.

JavaLogger::log, make_html and make_content created jstrings per iteration and
never released them; on an already-attached thread those accumulate in the
outermost frame until ART's local reference table overflows. The destroy entry
points were also the only natives running outside odr_jni::guarded.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
TablePosition::to_column_num throws on any character outside A-Z and
std::stoul throws on non-numeric input, so TableAddress.columnNumber(from:)
unwound a C++ exception into Swift and called std::terminate.
SinkLogger::will_log was the one sink callback without an @try/@catch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
Only Element::operator bool was bound, so Python inherited it through the MRO
and bool(paragraph.as_slide()) was true for a cast that failed - callers then
read defaults from every accessor. ElementIterator::operator* returns by
value, so pybind11 drops make_iterator's implicit keep-alive and yielded
elements held a raw adapter pointer with nothing owning the document.

The four CLI tools indexed argv without checking argc, and pyodr --serve used
an API that no longer exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
Adds Path parent/descendant_of/component-boundary cases, map_util and
xml_util bounds, and promotes assertions whose failure would crash rather than
report. Fixes a test that re-asserted the previous case's size and one with no
assertions at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8GkXFfKmN7KaCoeYcAayS
@andiwand
andiwand merged commit 85f6adb into main Aug 7, 2026
34 checks passed
@andiwand
andiwand deleted the review/correctness-and-conciseness branch August 7, 2026 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant