[legion-implementer]

For the reviewer

Outcome: When tools/check_links.py reads a link record's map that has non-blank lines and keeps no input section of it, the build now gets the reader's own problem, with counts, as its only detail, and no longer a wrong cause in the link.
Why: #161 made the reader keep nothing from every map, and each build then said "the link took no file of the toolchain image" and named findings about the project's code that the reader never examined. The reader had no guard of its own (OSSV-445).
Change:

• A map with non-blank lines of which the reader keeps no input section gives the build no uninstrumented list and the result error with the detail the reader kept no input section of the link's map: <N> non-blank lines, <M> read as input sections, 0 kept; the first line: <line> (after "the plain build instruments nothing, by design; " in plain).
• That problem is the build's whole detail: no note about the toolchain image and no finding about the project's code.
• A ubsan or asan-ubsan build beside a plain build with the problem says that it has no plain build to compare with, as it does with no plain build.
• An empty or blank map, an incomplete record, a link with no map file, a Go program and a record from which the reader keeps sections give the same results as before.
• Check 4 of docs/build-contract.md and the docstrings of tools/check_links.py say so; check 4 also says that a plain build with the problem gives the UBSan comparison no baseline.
  Look at first: tools/check_links.py parse_map (the shape test now runs before the loaded test, and taken stays the same as at the base), read_record (the problem text of R1), and the early branch in analyse that keeps a build with the problem out of scanned (the UBSan comparison).
  Proven by: python3 -P -m unittest discover -s tools/tests -p test_binary_checks.py at the head (121 tests, OK) with the real record of tools/tests/fixtures/link-record/curl-fuzz_bufq.plain/, and the parent's script dispatch://OSSV-398/artifact/root-check-py version 3 in both modes (all PASS).
  Not proven / risk:
• The first line of the map goes into the detail at full length. The Markdown report cuts a detail to 600 characters (tools/check_binaries.py:547), but the manifest's facts do not. The spec fixes the exact text and plan decision 7 chose no limit; the build's result is error in that case.
• A map with carriage returns: N counts lines split on line feeds only (plan decision 6). lld writes line feeds only, and no test covers a carriage return.
  Size: 3 files changed, +250/−42

Dispatch: OSSV-445

Verification

Threads: 3 resolved, 0 unresolved. Each answered and resolved individually by the implementer, the pull request's author, never in bulk (the reviewer names the bot threads it accepted; it resolves none itself):

• Thread PRRT_kwDOVTV78s62k5Uo: fixed in 5c3ab16 — RealRecord.fixture, unloaded, split and written are shared, and ReaderKeptNothing copies none of RealRecord's code; resolved at 5c3ab16.
• Thread PRRT_kwDOVTV78s62k5Uu: fixed in 5c3ab16 — test_the_first_line_is_the_first_non_blank_line: a map that starts with blank lines, its first non-blank line with trailing blanks; resolved at 5c3ab16.
• Thread PRRT_kwDOVTV78s62k5U3: fixed in 5c3ab16 — PLAIN_DETAIL, one module constant, in analyse's three places; resolved at 5c3ab16.

Thermo: simplify-settled-diff once at 04ae0b6: applied → new head 71473f1 (the non-blank lines are counted only when nothing is kept; the new sentences follow the sentence on the listed files and say "non-blank lines" and "the number of"; one constant for the unloaded part's first line in the tests); defects reported: none; thermonuclear pair at 71473f1: deep review APPROVE (two Minor items), code quality CHANGES (one Medium: the new test class copies RealRecord's fixture handling). Round 2, thermonuclear pair at 5778637: deep review APPROVE (three Minor items: two are fixed by the reviewer's prose commit 399eeeb, one is not a defect), code quality CHANGES (three test-structure items, judged not defects in the review); 399eeeb changes one comment and one sentence of check 4 alone.

Checks:

• Check c-none-kept (before_merge): pass at 5c3ab16, recorded by implementer. Three tests, each in the builds plain , ubsan , asan and asan-ubsan . The first
• Check c-none-kept (before_merge): pass at 5c3ab16, recorded by tester. Three tests, each in the builds plain , ubsan , asan and asan-ubsan . The first
• Check c-parse-map-pair (before_merge): pass at 5c3ab16, recorded by implementer. MapParsing and RealRecord take the pair: on the real map parse_map returns 224 s
• Check c-parse-map-pair (before_merge): pass at 5c3ab16, recorded by tester. MapParsing and RealRecord take the pair: on the real map parse_map returns 224 s
• Check c-empty-apart (before_merge): pass at 5c3ab16, recorded by implementer. Tests read a record whose map is empty, and one whose map has only blank lines,
• Check c-empty-apart (before_merge): pass at 5c3ab16, recorded by tester. Tests read a record whose map is empty, and one whose map has only blank lines,
• Check c-real-record (before_merge): pass at 5c3ab16, recorded by implementer. Each test of RealRecord passes. A new test reads the real record with its files
• Check c-real-record (before_merge): pass at 5c3ab16, recorded by tester. Each test of RealRecord passes. A new test reads the real record with its files
• Check c-parent-script (before_merge): pass at 5c3ab16, recorded by implementer. The parent's script, version 3 of fetched with dispatch doc-read --issue OSSV-39
• Check c-parent-script (before_merge): pass at 5c3ab16, recorded by tester. The parent's script, version 3 of fetched with dispatch doc-read --issue OSSV-39
• Check c-doc (before_merge): pass at 5c3ab16, recorded by implementer. The fourth check of docs/build-contract.md and the docstrings of the module and
• Check c-doc (before_merge): pass at 5c3ab16, recorded by tester. The fourth check of docs/build-contract.md and the docstrings of the module and
• Check c-suite (before_merge): pass at 5c3ab16, recorded by implementer. The required check tests of the pull request's head passes. It runs each test in
• Check c-suite (before_merge): pass at 5c3ab16, recorded by tester. The required check tests of the pull request's head passes. It runs each test in
• Check c-none-kept (before_merge): pass at 399eeeb, recorded by reviewer. Three tests, each in the builds plain , ubsan , asan and asan-ubsan . The first
• Check c-parse-map-pair (before_merge): pass at 399eeeb, recorded by reviewer. MapParsing and RealRecord take the pair: on the real map parse_map returns 224 s
• Check c-empty-apart (before_merge): pass at 399eeeb, recorded by reviewer. Tests read a record whose map is empty, and one whose map has only blank lines,
• Check c-real-record (before_merge): pass at 399eeeb, recorded by reviewer. Each test of RealRecord passes. A new test reads the real record with its files
• Check c-parent-script (before_merge): pass at 399eeeb, recorded by reviewer. The parent's script, version 3 of fetched with dispatch doc-read --issue OSSV-39
• Check c-doc (before_merge): pass at 399eeeb, recorded by reviewer. The fourth check of docs/build-contract.md and the docstrings of the module and
• Check c-suite (before_merge): pass at 399eeeb, recorded by reviewer. The required check tests of the pull request's head passes. It runs each test in

Fast-follow: none

Chain: not stacked.

[legion-reviewer]

Review of 71473f1: changes requested, for two test findings and one small code finding. The behaviour is correct.

What I verified at this head:

• CI: the four required checks of main (enrollment, project-builds, terms-accepted, tests) are completed and success, and suite and CodeQL pass too.
• dispatch results --issue OSSV-445: each of the seven before_merge checks (c-none-kept, c-parse-map-pair, c-empty-apart, c-real-record, c-parent-script, c-doc, c-suite) has a passing result by the tester that is current at this head.
• My own run, from a git archive copy of the head: python3 -P -m unittest discover -s tools/tests -p test_binary_checks.py gives 120 tests, OK. The parent's script (dispatch://OSSV-398/artifact/root-check-py, version 3) exits 0 in reader and in unchanged mode. Negative control: with the base db31635's tools/check_links.py in that copy, the tests give failures=13, errors=9, and reader gives 13 FAIL lines.
• The plan's reviewer items: the diff against the base db31635 holds only the three files. parse_map's taken is the same as at the base: the old if not loaded: continue is now part of if not loaded or not shaped, and the shape test has no side effect. The deep review also compared the base and head parse_map on the real map and on 30,000 random maps, with 0 differences. The problem text is the text of R1, character for character. The early branch of analyse comes before project_code, so the build is not put in scanned or problems_of. The documents follow the text around them. The departures D1 and D2 of plan v3 stand.
• Threads: none on this pull request.

Findings (inline):

1. The test class ReaderKeptNothing copies the fixture code of RealRecord.
2. No test shows that the reported first line is the first non-blank line.
3. One sentence of analyse is now written in three places.

Not a defect: the deep review found that the map's first line goes into the detail at full length, and the manifest does not cut it. R1 fixes the exact text and plan decision 7 chose no limit. The Markdown report cuts a detail to 600 characters (tools/check_binaries.py:547), and the build's result is already error. I wrote this in the body's Not proven / risk.

Contract change census: not needed. parse_map is a function inside tools/check_links.py. Its callers are read_record and the tests. Its one outside reader, OSSV-398's root-check.py v3, expects the pair. The signatures of read_record and analyse do not change.

Thermo: thermonuclear-deep-review at 71473f1: APPROVE, two Minor items (finding 2 here, and the length item above). thermonuclear-code-quality at 71473f1: CHANGES, one Medium (finding 1) and one Low (finding 3).

Security: untrusted-input — map text that a project's build can write goes into the build's detail without change (tools/check_links.py:654-658, 1212-1215). The Markdown report removes control characters and cuts it to 600 characters (tools/check_binaries.py:547, tools/check_checks.py:461-464). The manifest writes it with yaml.safe_dump (tools/manifest.py:373), which escapes it but sets no length limit, up to MAX_RECORD_BYTES (tools/check_links.py:200). No later step makes the escaped text raw again (ASVS v5.0.0-1.2.x). This is hardening only: there is no exploit path, because the build's result is already error. No other row applies (authz, secret, prompt, supply-chain, sandbox, agent-def, transport).

Fast-follow: none

[legion-reviewer]

Approved at 399eeeb.

Round 2. The fixes for my three round-1 findings are in 5c3ab16, and I checked each one in the diff:

• RealRecord.fixture, unloaded, split and written hold the fixture code once.
• test_the_first_line_is_the_first_non_blank_line pins the first non-blank line, without its leading and trailing blanks.
• PLAIN_DETAIL is used in the three places in analyse.
  My replies on the threads:
• PRRT_kwDOVTV78s62k5Uo: "Accepted: fixed in 5c3ab16… RealRecord.fixture, unloaded, split and written hold the map load, the cut of the unloaded part, the split of inputs.tsv and the record write once."
• PRRT_kwDOVTV78s62k5Uu: "Accepted: fixed in 5c3ab16… It expects 7632 non-blank lines, 7631 read and the stripped TAIL_FIRST, so a raw first line or a partial strip fails it."
• PRRT_kwDOVTV78s62k5U3: "Accepted: fixed in 5c3ab16… PLAIN_DETAIL is used at the three places, and the exact-text tests pass unchanged."
  The implementer resolved all three threads, and I accept those resolutions. The pull request has no other threads.

My prose commits:

• 5778637 and 399eeeb: the comment of PLAIN_DETAIL (tools/check_links.py:130) now says where the words stand. They are at the start of the detail, or at the end of a Go program's detail, and a plain build with no link record does not have them.
• 399eeeb: check 4 of docs/build-contract.md now says that the UBSan comparison is also an error when the check kept no input section of the plain build's map. This is what analyse does (tools/check_links.py:1352, R2).
  Neither commit changes code. At 399eeeb I ran test_binary_checks.py: 121 tests, OK.

Gate facts at 399eeeb:

• The four required checks of main are completed and success: enrollment, project-builds, terms-accepted and tests.
• The tester's results at 5c3ab16 became older when I pushed my prose commits. So I ran each of the seven before_merge checks again at 399eeeb, from a git archive copy, and recorded each one as reviewer. dispatch results --issue OSSV-445 now shows each one as a current pass:
  • c-none-kept, c-parse-map-pair, c-empty-apart, c-real-record: 121 tests, OK.
  • c-parent-script: reader exits 0 with 15 PASS, and unchanged exits 0 with 7 PASS.
  • c-doc: grep prints 1, and the one-liner prints True.
  • c-suite: tests passes in run 43206204223.
• Each negative control failed at that head, as it must:
  • the base reader: failures=13, errors=10, and the script gives 13 FAIL lines;
  • #161's reader: failures=20, errors=12;
  • parse_map that counts only the lines it keeps: failures=15;
  • the problem given on empty maps: failures=2;
  • the documents at the base: grep 0, and False.

Thermo, round 2:

• thermonuclear-deep-review at 5778637: APPROVE, three Minor items. Two are the prose fixes above. The third, that test_a_link_with_no_map_file writes its own record, is not a defect: the record has a digest of its own, and its inputs.tsv and driver show the case R3 names, a record that has every file but the map.
• thermonuclear-code-quality at 5778637: CHANGES, three test-structure items. I judge each of them not a defect:
  • The inputs.tsv and driver of the no-map test are the scenario, for the reason above.
  • The four-build loop is written in three tests: each test is the plan's T5 test, with its own map and its own counts.
  • The helpers are methods of RealRecord: that is one of the two places my round-1 comment offered.
• The only commit after 5778637 is 399eeeb, which changes one comment and one sentence of the document.

Contract change census: not needed. parse_map is a function inside tools/check_links.py. Its outside reader, OSSV-398's root-check.py v3, expects the pair. The signatures of read_record and analyse do not change.

Security: untrusted-input: map text that a project's build can write goes into the build's detail without change (tools/check_links.py:656-660, 1213-1219). The Markdown report removes control characters, cuts the detail to 600 characters and puts it in a code span (tools/check_binaries.py:547, tools/check_checks.py:461-464). The facts get the detail as an escaped string with no length limit. Plan decision 7 accepts this, and the PR body names it under Not proven / risk. No shell or interpreter receives the text. No other row applies (authz, secret, prompt, supply-chain, sandbox, agent-def, transport).

Fast-follow: none