fix(conformance): bound the message a preflight finding retains - #404
Merged
Conversation
PdfPreflight.Validate keeps every PreflightAssertion for the result's lifetime, and several rules interpolated a producer-controlled name or string into the finding message whole. A 900,000-byte /Filter name shared by 400 streams retained 705.7 MiB from a 990 KB file (#403); ISO 32000-2 Annex C.1 sets no bound on a name's length (Table C.1's 127 bytes is informative only). Two layers: Layer A (the sink). PreflightContext.Report now cuts any message over MaxMessageChars (1024) to "<first 1024 chars>... (<length> chars)", with a surrogate-pair check so the cut never leaves a lone high surrogate in the retained string (messages built from UTF-16BE text can put a pair astride the boundary). This alone bounds every message, including the "Rule evaluation failed: {ex.Message}" wrapper in PdfPreflight.cs, which can quote a whole oversized token thrown by the reader. Layer B (the name sites). The nine PdfName.Value interpolations that name a producer-controlled value in a report message (StreamRule's forbidden-filter message, both ActionRule messages, AnnotationRule's /AP-extra-key message, FontStructureRule's Type0 CMap message, LogicalStructureRule's /RoleMap message, PermissionsRule's /Perms-key message, BlendModeRule's /BM message, UaCMapRule's UA-1 CMap message) now quote through DiagnosticExcerpt.Quote, keeping the sentence shape ("...the /AAAA...AAA... (1048576 bytes) filter...") instead of a mid-sentence cut from the sink. AnnotationRule additionally quotes the /Subtype text once where it builds `label`, since that string is reused by the nine messages that follow it; a site whose value arrives already typed `string` (op, resourceName, and the other string-typed sites) is left to the sink, since it is written once per occurrence, not reused. Bound, before -> after: message length at a Layer B site for an N-byte name, about N+60 -> at most 54 (the 32-character excerpt plus the "... (N bytes)" suffix) + the fixed sentence for any int N; message length at every other site, unbounded -> at most 1046 for any int length; the 400-page issue shape, 705.7 MiB retained -> longest message under 1046 chars, sum of all message lengths under 128 KiB. Verdicts, rule ids, clauses, and assertion counts are unchanged; a message for a name of 32 characters or fewer is byte-identical to before. New tests/VellumPdf.Conformance.Tests/PreflightMessageBoundTests.cs, kept independent of PdfPreflightTests.cs (12,000+ lines already) with its own copy of AssemblePdf. Tests 1-3 and 5 (all but the boundary Theory, which cannot compile before MaxMessageChars exists) were run against 52c403e before this fix and observed failing: all 12 relevant cases failed, 11 with "Strings differ" (the unbounded message doesn't match the fixed-excerpt expectation) and FourHundredPagesSharingOneOversizedFilterName_retainOnlyBoundedMessages with "404 out of 407 items in the collection did not pass" its length bound. After the fix, all 15 cases in the new class pass. Self-review: git diff --stat touches only the ten rule/sink files, the CHANGELOG, and the new test file (no PublicAPI.*.txt, nothing under VellumPdf.Reader, VellumPdf.Cli, or TestSupport). grep for '.Value}' outside DiagnosticExcerpt.Quote in src/VellumPdf.Conformance finds only numeric or box.Value interpolations, already bounded by type. grep for "new PreflightAssertion(" finds exactly the one call site in PreflightContext.Report. dotnet build: 0 warnings, 0 errors. dotnet format --verify-no-changes: clean. eng/clean-room-check.ps1: passed. Full test runs with QPDF_HOME, POPPLER_HOME, VERAPDF_HOME, REQUIRE_ORACLES=1, REQUIRE_VERAPDF=1 set: Conformance 1296 total (1281 baseline + 15 new), 0 failed, 0 skipped; Reader 1349 total, 0 failed, 12 skipped; Cli 908 total, 0 failed, 0 skipped, matching the pre-change baselines plus the new class. No added em dash or double hyphen, no added hollow intensifier, no review-history narration left in shipped comments.
Round 1 of #404 found the prose presenting the ten excerpt sites as the complete set of producer-controlled interpolations. They are not: about thirty more sites rely on the 1024-character sink cut alone, which cuts mid-value and loses the sentence's tail for an oversized value. Sweeping them is a separate change (#405); this commit makes the split explicit. - MaxMessageChars doc: names the ten sites and what they quote, says the sink cut is the only bound every other interpolation has, and records why the two bounds differ (a PdfName value is Latin-1, so Quote counts bytes; the sink sees UTF-16BE-decoded text and steps around a surrogate pair). - Report: the param doc states the step-back and the code-unit count; the guard is a property pattern so a null message still passes through to the constructor as before. - CHANGELOG: a Changed bullet for the message-text change, since Conformance is Shipped and the CLI's text, JSON and SARIF output carries the new text; the Fixed bullet says ten sites, 400 pages, and that the numbers were measured in #403. The transient allocation from the Reader's own unknown-filter throw is unchanged and tracked in #406. - Tests: the class doc counts ten sites and points at #405; the section headers drop their numbering; the surrogate assertion is Assert.DoesNotContain(retained, char.IsSurrogate), which fails for scenario 2 with the step-back removed (verified); the 400-page test names its 22-character suffix allowance and reports the rule id on a failure; BuildUa1TaggedPdf's doc gives the reason that holds (WriterPdfTagged is private to OracleCorpus); the redundant System.Linq using goes (ImplicitUsings is on). The commit message of 74b5ee4 described the Layer B criterion as "arrives already typed string, written once"; the criterion that holds is "interpolates a PdfName.Value whole". Pushed history, recorded here and in the PR body.
Round-2 review of #404. No behaviour changes. - PreflightAssertion.Message now says what the text is and is not: no compatibility contract (switch on RuleId), cut at 1024 characters with the length appended, producer names excerpted at 32 characters. - The boundary Theory's comment named DoesNotContain as the assertion that catches a removed surrogate step-back; Assert.Equal fails first (re-verified with the step-back disabled). The comment says so, and the closing check is a lone-surrogate walk, since IsSurrogate would also reject a prefix that ends in a complete pair. - The 400-page test comment quoted #403's 705.7 MiB as this fixture's pre-fix total; the fixture's own figure is 693.6 MiB. - BuildUa1TaggedPdf's doc gives the reason Ua1BadCMapName cannot be reused: it hardcodes a ten-character name. - MaxMessageChars doc and CHANGELOG: 705.7 MiB is a GC delta; the Latin-1 claim is scoped to a name parsed from a document; the byte-identity claim is scoped to the ten excerpt sites. - The cut builds its string with string.Concat over a span instead of an intermediate substring.
The PreflightAssertion.Message doc said every producer name a rule quotes is kept to 32 characters. Only ten sites excerpt; the rest are bounded by the 1024-character sink cut alone, and the doc now says so. The MaxMessageChars summary said a message never carries the whole of a producer value; a value short enough to fit is carried whole, so the sentence is now about oversized values only. The 400-page test comment labels #403's 705.7 MiB as a GC delta, the way the CHANGELOG and the MaxMessageChars doc already do. No code change.
Round 4 of #404 read the PreflightAssertion.Message doc as saying that any quoted producer name is excerpted, and that the unit is the rule. DiagnosticExcerpt.Quote returns a name of 32 bytes or fewer whole, and FontStructureRule excerpts at one site while interpolating a base font name whole at four others, so the doc now says "ten sites" and "an oversized one", matching the MaxMessageChars doc, the class doc and the CHANGELOG bullet. The MaxMessageChars summary is reflowed; the round-3 edit had left a ten-character line inside the paragraph. No code change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #403.
What
PdfPreflight.Validatekeeps everyPreflightAssertionfor the result's lifetime, and several rules interpolated a producer-controlled name whole into the finding message. The #402 round-9 review measured the shape: 400 pages sharing one 900,000-byte/Filtername, a 990 KB file, 705.7 MiB (GC delta) retained. ISO 32000-2 Annex C.1 sets no bound on a name's length (Table C.1's 127 bytes is informative), so the bound has to be ours.Two layers, both in
src/VellumPdf.Conformance:PreflightContext.Reportcuts any message overMaxMessageChars(1024) to the first 1024 characters plus... (N chars), stepping back one character when the cut would split a surrogate pair (messages built from UTF-16BE/Langor XMP text can put one astride the boundary). This bounds every message, including theRule evaluation failed: {ex.Message}wrapper inPdfPreflight.cs, whose exception text can quote a whole oversized token thrown by the reader.PdfName.Valueinterpolations that quoted a producer name whole (StreamRulethe/Filter;ActionRulethe action type and the named action;AnnotationRulethe extra/APkey;FontStructureRuleandUaCMapRulea composite font's/EncodingCMap name;LogicalStructureRulea/RoleMapkey;PermissionsRulea/Permskey;BlendModeRulethe blend mode) quote through the Reader'sDiagnosticExcerpt.Quote(first 32 characters plus... (N bytes)), reachable through the existingInternalsVisibleTogrant, so the sentence keeps its shape instead of ending mid-word. The tenth site isAnnotationRule's/Subtype, quoted once where it buildslabel, which the nine messages after it reuse. These ten are not the complete set of producer-controlled interpolations in the rules: about forty more (font names, colorant names,/Langstrings, structure types, XMP namespaces and the like) rely on the sink cut alone, which bounds retention but cuts mid-value and loses the sentence's tail for an oversized value. Sweeping them is Conformance: the remaining producer-controlled interpolations rely on the sink cut alone #405; theMaxMessageCharsdoc records the split.Bound, before and after: a Layer B message for an N-byte name goes from about N + 60 characters to at most 54 plus the fixed sentence; every other message from unbounded to at most 1046 characters; the issue's 400-page shape from 705.7 MiB (GC delta) to a longest message under 1046 characters and a message total under 128 Ki characters.
Verdicts, rule ids, clauses and assertion counts are unchanged. At the ten sites, a message whose named value is 32 characters or shorter is byte-identical to before. No
PublicAPI.*.txtline changes;PreflightContextis internal. Nothing undersrc/VellumPdf.Reader,src/VellumPdf.Cliortests/VellumPdf.TestSupportchanges. Conformance is Shipped, so the message-text change carries a### ChangedCHANGELOG bullet beside the### Fixedone, the #397 shape;vellum-preflight's text, JSON and SARIF output carries the same text.What this PR bounds is the retained result. The Reader still builds its own
InvalidDataExceptionmessage with the whole/Filtername each timeFilters.csrejects an unknown filter (2404 throws for the 400-page shape, about 4.1 GiB of transient allocation at a 900,000-byte name, 96 KiB retained afterwards); that is a Reader change with a Conformance test to update and is #406.Tests
New
tests/VellumPdf.Conformance.Tests/PreflightMessageBoundTests.cs(15 cases): one exact-message test per Layer B site at a 1 MiB name, the sink cut asserted on everyRule evaluation failedmessage for an unknown 1 MiB filter, a boundary Theory at 1024 and 1025 characters plus a 1056-character case whose surrogate pair straddles the cut, and the 400-page issue shape asserting every message length and the total. Run against52c403ebefore the fix, with the class's threeMaxMessageCharsreferences replaced by the literal 1024, the ten site tests and the sink test failed on the message text and the 400-page case had 404 of 407 messages over the bound (12 cases in all). The boundary Theory was not run there: its scenarios are defined by the constant itself, so substituting it would have tested a different thing.Not in
PdfPreflightTests.cs(12,317 lines already); the class carries its ownAssemblePdf.Departures from the brief
BuildUa1TaggedPdf()helper rather thanOracleCorpus.WriterPdfTagged, which is private toOracleCorpus.Rule evaluation failedassertion, not the first one, since several rules each try to decode the same stream and each wraps the same thrown message.Round 1 (at
74b5ee4)Three reviewers (Shipped-surface and byte-identity; bounds and adversarial measurement; prose and claim verification). No HIGH and no behavioural defect. The bounds lens measured the branch: longest retained message 1042 characters, 400-page total 49,167 characters (96.0 KiB), 14 of 15 cases discriminate (reverting the sink cut fails 4, reverting
Quotefails 11), class runtime 2.8 s with no wall-clock assertion,Quoteapplied before interpolation at all ten sites, the Annex C.1 and Table C.1 quotations verbatim, the 522-character longest-sentence figure exact. Findings and howd28654btakes them:MaxMessageCharsdoc presented the ten excerpt sites as the complete set of producer-controlled interpolations; the reviewers listed about thirty more that rely on the sink cut alone. Not swept here (a thirty-site change to a Shipped assembly gets its own review); filed as Conformance: the remaining producer-controlled interpolations rely on the sink cut alone #405, and every one of those four texts now states the split and the reason the two bounds differ (aPdfNamevalue is Latin-1, soQuotecounts bytes exactly; the sink sees UTF-16BE-decoded text and counts UTF-16 code units).### Changedbullet for a Shipped assembly's output change. Added, in the Kernel: always set /P bit 10 in the encryption dictionary (ISO 32000-2 Table 22) #397 pair shape.Assert.False(retained.Length > 0 && char.IsHighSurrogate(retained[^1]))could not fail, since a cut string never ends in the surrogate. NowAssert.DoesNotContain(retained, char.IsSurrogate); with the step-back replaced by a no-op, scenario 2 fails and scenarios 0 and 1 pass (verified locally).Reportparam doc omitted the step-back and the unit. Both stated.BuildUa1TaggedPdf's doc gave a reason ("must not change what any oracle test validates") that does not follow from building a second copy. It now gives the reason that holds:WriterPdfTaggedis private toOracleCorpus.+ 22and131_072were unexplained. The 22 is now a named constant ("... ("plus up to ten digits plus" chars)"), theAssert.Allreports the rule id and length on failure, and the total is labelled as a character count.message is { Length: > MaxMessageChars }so a null message reaches the constructor as before; the redundantusing System.Linq;is gone.Correction to pushed history:
74b5ee4's body justifies leaving a site to the sink because its value "arrives already typed string" and is "written once per occurrence, not reused". That is not the criterion that was applied: nine of the ten quoted sites are the ones whose interpolation hole read<expr>.Valuedirectly, and the tenth is the annotation labelAnnotationRulebuilds from one and nine of its messages reuse. The other sites that bind aPdfName.Valueto a local before interpolating (font names, colourant names, structure types) were not swept and are in #405. The sentence stays in the history, corrected here and ind28654b's body.Round 2 (at
d28654b)Same three lenses. No HIGH, no behavioural defect; the round-1 fixes verified (the surface lens proved the byte-identity claim mechanically over all 32 messages in the eight rule files and serialised a 1043-character message through the CLI's text, JSON and SARIF writers; the bounds lens reproduced the 407 / 1042 / 49,167 figures and the three mutations, with
cut += 0failing exactly scenario 2). Findings and how7b86a40takes them:DoesNotContainline when the step-back is removed, but theAssert.Equaltwo lines above fails first (verified again: with the step-back disabled, scenario 2 fails onAssert.Equalwith the lone surrogate visible in the actual string). The comment now namesAssert.Equalas the discriminating check, and the closing assertion is a lone-surrogate walk (a high surrogate must be followed by a low one, a low one must be preceded by a high one) rather thanDoesNotContain(char.IsSurrogate), which would also fail on a kept prefix containing any complete pair.PreflightAssertion.Message's doc did not say the text is bounded or that it is not a contract. It now carries thePdfReaderDiagnostic.Messagedisclaimer (switch onRuleId, not the text) and states the 1024-character cut and the 32-character excerpt.52c403e; the Theory referencesMaxMessageCharsand cannot compile there. Corrected above, with the count of cases that were run (12).74b5ee4correction paragraph misquoted the commit and stated the criterion loosely. Rewritten above.BuildUa1OversizedCMapNamePdf's doc saidOracleCorpus.Ua1BadCMapName"is a registered oracle fixture and is not reused directly"; the reason it cannot be reused is that it hardcodesnew PdfName("FooBarCMap"), a ten-character name. Now says so.message[..cut]allocated an intermediate substring; nowstring.Concatover a span.MaxMessageCharsdoc's "aPdfNamevalue is Latin-1" is scoped to a name parsed from a document (a caller can construct one from any string).FontStructureRulequotes/Subtype; it quotes the composite font's/EncodingCMap name. Corrected on the issue.Round 3 (at
7b86a40)Single prose reviewer over
d28654b..7b86a40, this body and the squash body. No HIGH and no behavioural finding; the round-2 discrimination claim reproduced by mutation (with the surrogate step-back neutered, only scenario 2 of the boundary Theory fails, on theAssert.Equalthe comment names). Fix-upbe8bb67(docs and one test comment, no code change):PreflightAssertion.Message's new doc said a producer name a rule quotes is kept to 32 characters, as if every quoting site excerpted. Only the ten sites do; elsewhere an oversized value is bounded by the 1024-character cut alone. The doc now says so.BuildUa1TaggedPdf; it isBuildUa1OversizedCMapNamePdf's. Corrected above. The same slip is in7b86a40's commit body, which is squashed away on merge.74b5ee4correction paragraph said the ten sites were "exactly those that interpolated aPdfName.Valuewhole"; six sites outside the ten do that through a local (FontEmbeddingRule.cs:102,ToUnicodeRule.cs:52,UaFontEmbeddingRule.cs:184,UaCidToGidMapRule.cs:72,UaSymbolicFontRule.cs:73,SeparationConsistencyRule.cs:434). The paragraph now states the criterion that was applied: an interpolation hole reading<expr>.Valuedirectly.MaxMessageCharswhile the 400-page case and the class doc reference it too, so the twelve cases ran at52c403ewith the constant substituted. Rewritten above.MaxMessageCharsdoc carry. Labelled.MaxMessageCharssummary said a message "never" carries the whole of a producer value; a value that fits is carried whole. Now "an oversized producer value".d28654bgates paragraph replaced by "the review checkout's own path".DoesNotContain(char.IsSurrogate)rejects (any complete pair in the retained text). Corrected.Round 4 (at
be8bb67)Single prose reviewer over the round-3 delta, this body and the squash body. No HIGH, no behavioural finding. CI green first time at
be8bb67(run 33943281539).<expr>.Valuein the interpolation hole, and said sites that bind the value to a local were not swept.AnnotationRulebindssubtypeto a local and was swept. Paragraph corrected to nine plus the annotation label.PreflightAssertion.Messagedoc read as unconditional ("keeping its first 32 characters") and counted rules where every other text counts sites. Fixed in8763761: "Ten sites additionally excerpt the producer name they quote, keeping an oversized one to its first 32 characters".Rule evaluation failedmessage was "once per stream"; the fixture has one stream, and the messages come from several rules each wrapping the same thrown message. Corrected.MaxMessageCharssummary reflowed (8763761);FontEmbeddingRule.cs:102andToUnicodeRule.cs:52are the interpolation lines, not:101/:51;OracleCorpus.WriterPdfTaggedis private toOracleCorpus, not to a test class;be8bb67's commit body says the doc "now says so" about the ten-site count when it stated the split without the count (history, recorded here).be8bb67's body against their deltas; the tenQuotesites; the 15/12/three-reference counts;FooBarCMapat ten characters; the 693.6 / 705.7 MiB attributions consistent across the four texts, 693.6 corroborated as 404 oversized of 407; with the surrogate step-back deleted and the project rebuilt, exactly scenario 2 fails on theAssert.Equalthe comment names.Gates at
8763761dotnet build0 warnings 0 errors after a rebuild;dotnet format --verify-no-changesclean;git grep -ifor the banned fragment clean; no added em dash or--againstorigin/main; no added comment line over 100 characters; noPublicAPI.*.txtchange;PreflightMessageBoundTests15/0. Doc and comment text only (two files, 8 lines), so the oracle runs at7b86a40stand.Gates at
be8bb67dotnet build0 warnings 0 errors after a rebuild;dotnet format --verify-no-changesclean; no added em dash or--againstorigin/main; no added comment line over 100 characters; noPublicAPI.*.txtchange;PreflightMessageBoundTests15/0. The delta is doc and comment text only, so the Conformance and Cli oracle runs at7b86a40stand.Gates at
7b86a40dotnet build0 warnings 0 errors after a rebuild;dotnet format --verify-no-changesclean; clean-room check passed (script plus thegit grep -ifile scan, as in round 1); no added em dash or--againstorigin/main; noPublicAPI.*.txtchange; comment lines within 100 characters. WithREQUIRE_ORACLES=1andREQUIRE_VERAPDF=1:PreflightMessageBoundTests15/0, Conformance 1296/0/0 (591 s), Cli 908/0/0. Reader is untouched since74b5ee4(1349/0/12).Gates at
d28654bdotnet build0 warnings 0 errors after a rebuild;dotnet format --verify-no-changesclean; clean-room check passed (the script's directory filter skips every file under the review checkout's own path, so the file half was repeated asgit grep -iover tracked files: no hit; the commit-message half ran as normal); no added em dash or--againstorigin/main; noPublicAPI.*.txtchange. WithREQUIRE_ORACLES=1andREQUIRE_VERAPDF=1:PreflightMessageBoundTests15/0, Conformance 1296/0/0 (1281 baseline + 15), Cli 908/0/0. Reader is untouched byd28654b(1349/0/12 at74b5ee4).