From 2aa96f8a7610450948aecc72cbbfff02751fbe36 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 10 Nov 2026 08:30:14 +0100 Subject: [PATCH 1/6] The structural checker refuses broken archives instead of stopping on them An archive whose first bytes are zeroed passed the ZIP check, because it read the end records and never followed them to the local headers. It now walks the central directory and checks that every entry leads to a local header with the same name, with its data before the directory. Office packages go through the same walk first, so bytes after their end record are refused too. A PNG cut inside its last checksum, a PNG that is only its signature and an Office part that cannot be read stopped the check with an exception. Each is now a refusal with its reason. An exception left anywhere is reported as a fault in the checker and never counted as a refusal of the file. A verdict is raised rather than printed, so one process can judge many files: strict.py --batch reads one request per line, and oracle.StrictBatch hands it a list. Two guards hold this: real files from this tool, broken nine ways, must be refused for the right reason, and planted checks that fall over must be answered as a crash. Co-Authored-By: Claude Opus 5.5 --- internal/guard/mutation/entries.py | 67 +++++++- internal/guard/strictverdict_test.go | 233 +++++++++++++++++++++++++++ internal/oracle/strict.py | 231 ++++++++++++++++++++++---- internal/oracle/strictbatch.go | 120 ++++++++++++++ 4 files changed, 616 insertions(+), 35 deletions(-) create mode 100644 internal/guard/strictverdict_test.go create mode 100644 internal/oracle/strictbatch.go diff --git a/internal/guard/mutation/entries.py b/internal/guard/mutation/entries.py index fa8189f9..cee5ee2d 100644 --- a/internal/guard/mutation/entries.py +++ b/internal/guard/mutation/entries.py @@ -14382,9 +14382,9 @@ "TestTheEngineHandsTheStopToAContainerItIsPlanning", ), ( - "a broken picture inside a page ends the structural check with a traceback", + "a broken picture inside a page stops counting against the page", "internal/oracle/strict.py", - " except Exception as exc:\n fail(f\"picture {n} is not a good {kind} file: {type(exc).__name__}: {str(exc)[:120]}\")\n", + " if said.word != \"OK\":\n fail(f\"picture {n} is not a good {kind} file: {said.reason}\")\n", "", "TestTheStructuralCheckGivesAVerdictOnABrokenPicture", ), @@ -17015,4 +17015,67 @@ 'SKIPPED_MANIFESTS = frozenset({".github/requirements-semgrep.txt"})', "TestTheDependencyGateRefusesALicenceThisProjectCannotDistribute", ), + ( + "the structural checker stops following the central directory to each local header, so an archive with a zeroed head passes", + "internal/oracle/strict.py", + ' if local + 30 > cd_at or data[local:local + 4] != b"PK\\x03\\x04":', + " if False:", + "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", + ), + ( + "the structural checker stops comparing the name in a local header with the central directory", + "internal/oracle/strict.py", + " if local_name != name:", + " if False and local_name != name:", + "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", + ), + ( + "an Office package skips the ZIP walk, so bytes after its end record pass", + "internal/oracle/strict.py", + " zip_structure(data)\n z = opc_open(data)", + " z = opc_open(data)", + "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", + ), + ( + "a part of an Office package that cannot be read stops the check with a traceback", + "internal/oracle/strict.py", + " except (zipfile.BadZipFile, zlib.error, EOFError) as exc:", + " except KeyError as exc:", + "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", + ), + ( + "a PNG cut inside its last checksum stops the check with a traceback", + "internal/oracle/strict.py", + ' if pos + 12 + length > len(data):\n fail(f"chunk {kind!r} ends at {len(data)} B, before its 4 B checksum")\n', + "", + "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", + ), + ( + "a PNG that is only its signature stops the check with a traceback", + "internal/oracle/strict.py", + ' if not seen:\n fail("the signature is followed by no chunk at all")\n', + "", + "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", + ), + ( + "a check that stops on an exception is counted as a refusal", + "internal/oracle/strict.py", + ' return "CRASH", f"{type(exc).__name__}: {str(exc)[:200]}"', + ' return "FAIL", f"{type(exc).__name__}: {str(exc)[:200]}"', + "TestACheckThatStopsOnAnExceptionIsACrashNotARefusal", + ), + ( + "a check that ends without a verdict is counted as an acceptance", + "internal/oracle/strict.py", + ' return "CRASH", "the check returned without a verdict"', + ' return "OK", "the check returned without a verdict"', + "TestACheckThatStopsOnAnExceptionIsACrashNotARefusal", + ), + ( + "a verdict given inside a check's own error handling is swallowed", + "internal/oracle/strict.py", + "class Verdict(SystemExit):", + "class Verdict(Exception):", + "TestACheckThatStopsOnAnExceptionIsACrashNotARefusal", + ), ] diff --git a/internal/guard/strictverdict_test.go b/internal/guard/strictverdict_test.go new file mode 100644 index 00000000..46edc002 --- /dev/null +++ b/internal/guard/strictverdict_test.go @@ -0,0 +1,233 @@ +package guard + +import ( + "bytes" + "encoding/binary" + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/oracle" +) + +// strictCase is one file handed to the structural checker, with what it has +// to answer. +type strictCase struct { + name string + format string + body []byte + // verdict is the answer wanted, and reason a piece of what it has to say. + // The reason is asked for because a refusal for some other cause would + // pass a guard that read the verdict alone. + verdict string + reason string +} + +// TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping holds the three +// faults of the checker found when it became the judge of what a damage may be +// offered for (docs/DAMAGE-ETAP-2-2026-11-09.md section 3): +// +// - an archive whose first bytes are zeroed was accepted, because the check +// read the end records and never followed them to a local header +// - an Office package with bytes after its end record was accepted, because +// the ZIP reader underneath finds that record by searching for it +// - a PNG cut short and a package whose part cannot be read stopped the +// check with an exception, and an exception was counted as a refusal +// +// Each broken file is made from a healthy one this tool writes, and the +// healthy ones are asked first: a checker that refused everything would pass +// the rest of this guard. +func TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping(t *testing.T) { + png := writeEncoded(t, "png", 2048, nil) + zip := writeEncoded(t, "zip", 16384, nil) + docx := writeEncoded(t, "docx", 4096, nil) + xlsx := writeEncoded(t, "xlsx", 4096, nil) + pptx := writeEncoded(t, "pptx", 8192, nil) + + cases := []strictCase{ + {"healthy png", "png", png, oracle.StrictOK, ""}, + {"healthy zip", "zip", zip, oracle.StrictOK, ""}, + {"healthy docx", "docx", docx, oracle.StrictOK, ""}, + {"healthy xlsx", "xlsx", xlsx, oracle.StrictOK, ""}, + {"healthy pptx", "pptx", pptx, oracle.StrictOK, ""}, + + {"png with nothing after the signature", "png", png[:8], oracle.StrictFail, "no chunk at all"}, + {"png one byte short", "png", png[:len(png)-1], oracle.StrictFail, "before its 4 B checksum"}, + {"zip with a zeroed head", "zip", zeroed(zip, 4), oracle.StrictFail, "no local file header"}, + {"zip whose local header names another file", "zip", renamed(t, zip), oracle.StrictFail, "and the central directory names"}, + {"docx with a zeroed head", "docx", zeroed(docx, 4), oracle.StrictFail, "no local file header"}, + {"xlsx with a zeroed head", "xlsx", zeroed(xlsx, 4), oracle.StrictFail, "no local file header"}, + {"pptx with a zeroed head", "pptx", zeroed(pptx, 4), oracle.StrictFail, "no local file header"}, + {"docx with bytes after its end", "docx", appended(docx, 64), oracle.StrictFail, "the comment declares 0 B and 64 follow"}, + {"docx with a part that does not match its checksum", "docx", flipped(t, docx), oracle.StrictFail, "cannot be read from the archive"}, + } + + dir := t.TempDir() + reqs := make([]oracle.StrictRequest, len(cases)) + for i, c := range cases { + path := filepath.Join(dir, strings.ReplaceAll(c.name, " ", "_")+"."+c.format) + if err := os.WriteFile(path, c.body, 0o600); err != nil { + t.Fatal(err) + } + reqs[i] = oracle.StrictRequest{Format: c.format, Path: path} + } + + got, available, err := oracle.StrictBatch(reqs) + if !available { + t.Skip("the structural checker needs python, and there is none on this machine") + } + if err != nil { + t.Fatal(err) + } + for i, c := range cases { + j := got[i] + if j.Verdict != c.verdict || !strings.Contains(j.Reason, c.reason) { + t.Errorf("%s: the checker answered %s %q, and %s saying %q was wanted.\n"+ + "Reason: the checker decides which damage is offered for which format, so a\n"+ + "refusal has to come from the defect itself. A crash is a fault in the\n"+ + "checker, never a refusal of the file.", c.name, j.Verdict, j.Reason, c.verdict, c.reason) + } + } +} + +// strictPlant is run by the guard below. It loads the checker, puts checks that +// misbehave in place of real ones and asks through the same path --batch +// takes, printing one answer per line. +const strictPlant = `import importlib.util, json, sys +sys.dont_write_bytecode = True +spec = importlib.util.spec_from_file_location("tfg_strict", sys.argv[1]) +strict = importlib.util.module_from_spec(spec) +spec.loader.exec_module(strict) + +def raises(data): + raise ValueError("planted") + +def says_nothing(data): + return None + +def refuses_inside_a_handler(data): + try: + strict.fail("planted refusal") + except Exception: + pass + strict.ok("the refusal was swallowed") + +strict.CHECKS["png"] = raises +strict.CHECKS["gif"] = says_nothing +strict.CHECKS["bmp"] = refuses_inside_a_handler +for kind in ("png", "gif", "bmp"): + print(json.dumps(strict.batch_answer(json.dumps({"format": kind, "path": sys.argv[1]})))) +` + +// TestACheckThatStopsOnAnExceptionIsACrashNotARefusal holds the line the +// witness of a damage stands on. The checker answers CRASH when a check stops +// on an exception or ends without a verdict, and never FAIL. Counted as a +// refusal, a checker that falls over on every damaged file would witness every +// damage (docs/DAMAGE-ETAP-2-2026-11-09.md section 3.3). +// +// The third check refuses inside its own `except Exception` and carries on. A +// verdict has to stop the check anyway, which is why it is raised as a +// SystemExit rather than an ordinary exception. +// +// Planted checks, because the real ones are meant to have no crash left in +// them, so no real file can reach these states. +func TestACheckThatStopsOnAnExceptionIsACrashNotARefusal(t *testing.T) { + python, err := exec.LookPath("python") + if err != nil { + t.Skip("the structural checker needs python, and there is none on this machine") + } + here, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + script, ok := oracle.StrictScriptUnder(here) + if !ok { + t.Fatalf("the structural checker is not above %s", here) + } + plant := filepath.Join(t.TempDir(), "plant.py") + if err := os.WriteFile(plant, []byte(strictPlant), 0o600); err != nil { + t.Fatal(err) + } + // The interpreter is the one found on PATH, the script is the plant this + // guard just wrote, and its one argument is the checker of this repository. + // nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command + out, err := exec.Command(python, plant, script).CombinedOutput() + if err != nil { + t.Fatalf("the planted checks did not run: %v\n%s", err, out) + } + + want := []struct{ verdict, reason string }{ + {oracle.StrictCrash, "ValueError: planted"}, + {oracle.StrictCrash, "the check returned without a verdict"}, + {oracle.StrictFail, "planted refusal"}, + } + lines := strings.Split(strings.TrimSpace(string(out)), "\n") + if len(lines) != len(want) { + t.Fatalf("%d answer(s) for %d planted checks:\n%s", len(lines), len(want), out) + } + for i, w := range want { + var j oracle.StrictJudgement + if err := json.Unmarshal([]byte(strings.TrimSpace(lines[i])), &j); err != nil { + t.Fatalf("answer %d is not an answer: %v: %s", i+1, err, lines[i]) + } + if j.Verdict != w.verdict || !strings.Contains(j.Reason, w.reason) { + t.Errorf("planted check %d was answered %s %q, and %s saying %q was wanted.\n"+ + "Reason: a check that falls over is a fault in the checker. It must never\n"+ + "read as a refusal, and a refusal must never be swallowed by a check's own\n"+ + "error handling.", i+1, j.Verdict, j.Reason, w.verdict, w.reason) + } + } +} + +// zeroed is the file with its first n bytes set to zero. +func zeroed(b []byte, n int) []byte { + out := append([]byte{}, b...) + clear(out[:n]) + return out +} + +// appended is the file with n bytes of a fixed pattern after its end. +func appended(b []byte, n int) []byte { + out := append([]byte{}, b...) + for i := range n { + out = append(out, byte(i*37+11)) + } + return out +} + +// firstEntry finds the first local file header of an archive, and where its +// name and its data start. +func firstEntry(t *testing.T, b []byte) (name, data int) { + t.Helper() + at := bytes.Index(b, []byte("PK\x03\x04")) + if at < 0 || at+30 > len(b) { + t.Fatal("the archive has no local file header to change, so this case would test nothing") + } + nameLen := int(binary.LittleEndian.Uint16(b[at+26:])) + extraLen := int(binary.LittleEndian.Uint16(b[at+28:])) + if nameLen == 0 || at+30+nameLen+extraLen >= len(b) { + t.Fatal("the first entry has no name or no data, so this case would test nothing") + } + return at + 30, at + 30 + nameLen + extraLen +} + +// renamed is the archive with one letter of the name in its first local +// header changed, so the header and the central directory disagree. +func renamed(t *testing.T, b []byte) []byte { + out := append([]byte{}, b...) + name, _ := firstEntry(t, out) + out[name] ^= 0x01 + return out +} + +// flipped is the package with one byte of the first part's data changed, so +// the part no longer matches the checksum its headers carry. +func flipped(t *testing.T, b []byte) []byte { + out := append([]byte{}, b...) + _, data := firstEntry(t, out) + out[data] ^= 0xff + return out +} diff --git a/internal/oracle/strict.py b/internal/oracle/strict.py index 64d54fcd..826bcf67 100644 --- a/internal/oracle/strict.py +++ b/internal/oracle/strict.py @@ -11,8 +11,15 @@ complements the tolerant readers rather than replacing them - a file has to pass both. -Usage: python strict.py +Usage: python strict.py [key=value ...] Prints "OK " or "FAIL ". + +Or: python strict.py --batch +Reads one JSON request per line, {"format": ..., "path": ..., "settings": +{...}}, and writes one JSON answer per line, {"path": ..., "verdict": ..., +"reason": ...}, in the order asked. The verdict is OK, FAIL, CRASH (the check +stopped on an exception instead of giving a verdict) or ERROR (a request +this script cannot act on). """ import re @@ -23,14 +30,28 @@ from html.parser import HTMLParser +class Verdict(SystemExit): + """The answer a check gives, raised so that it stops the check where it is. + + It is a SystemExit, so a check's own `except Exception` cannot catch a + verdict given inside it and carry on as if nothing had been said. The word + and the reason travel with it instead of being printed, so one process can + judge many files (--batch) and a check can ask another one in process + (html_picture_verdict) without capturing what it printed. + """ + + def __init__(self, word, reason): + super().__init__(0 if word == "OK" else 1) + self.word = word + self.reason = reason + + def fail(msg): - print("FAIL " + msg) - sys.exit(1) + raise Verdict("FAIL", msg) def ok(msg): - print("OK " + msg) - sys.exit(0) + raise Verdict("OK", msg) def check_png(data): @@ -46,6 +67,8 @@ def check_png(data): body = data[pos + 8:pos + 8 + length] if len(body) != length: fail(f"chunk {kind!r} declares {length} B and the file holds {len(body)}") + if pos + 12 + length > len(data): + fail(f"chunk {kind!r} ends at {len(data)} B, before its 4 B checksum") want = struct.unpack(">I", data[pos + 8 + length:pos + 12 + length])[0] got = zlib.crc32(kind + body) & 0xFFFFFFFF if want != got: @@ -54,6 +77,8 @@ def check_png(data): chunks += 1 pos += 12 + length + if not seen: + fail("the signature is followed by no chunk at all") if seen[0] != b"IHDR": fail("the first chunk is not IHDR") if seen[-1] != b"IEND": @@ -146,17 +171,66 @@ def check_pdf(data): def check_zip(data): + entries, comment_len = zip_structure(data) + ok(f"{entries} entries, comment {comment_len} B") + + +def zip_structure(data): + """The archive as the specification lays it out, walked from its end. + + The end of central directory record says where the central directory is + and how many entries it holds. Every entry there says where its local + header is, and that has to be a local header carrying the same name, with + its data ending before the central directory starts. Reading the end alone + took an archive whose first bytes were zeroed (measured 2026-11-09): every + record read from the end was intact, and the local header a reader needs + to reach the data was gone. + + Shared with the Office packages, which are ZIP archives first. + Returns the number of entries and the length of the archive comment. + """ end = data.rfind(b"PK\x05\x06") - if end < 0: + if end < 0 or end + 22 > len(data): fail("there is no end of central directory record") comment_len = struct.unpack(" end or data[pos:pos + 4] != b"PK\x01\x02": + fail(f"central directory entry {number} of {entries} should start at {pos} and does not") + packed = struct.unpack(" end: + fail(f"central directory entry {number} of {entries} runs past the end record at {end}") + + if local + 30 > cd_at or data[local:local + 4] != b"PK\x03\x04": + fail(f"the central directory sends {name!r} to offset {local}, " + f"where there is no local file header") + local_name_len, local_extra_len = struct.unpack(" cd_at: + fail(f"the data of {name!r} runs from {local} to {data_end}, " + f"past the start of the central directory at {cd_at}") + + if pos != end: + fail(f"the {entries} central directory entries end at {pos} and the end record is at {end}") + return entries, comment_len OCTET = r"(?:0|[1-9][0-9]{0,2})" @@ -834,26 +908,20 @@ def html_pictures(text, settings, nl): def html_picture_verdict(kind, n, raw): - """One picture judged by its own format's check. Those end the process with - their verdict, so each is caught here and only a failure is passed on. - - A check can also stop on bytes it did not expect - check_png unpacks a - checksum that is not there when a chunk runs to the very end - and that is - a verdict about the picture too, so it is given as one rather than as a - traceback (review of #178).""" - import contextlib - import io - - said = io.StringIO() + """One picture judged by its own format's check, and only a failure passed on. + + A check that stops on an exception is not caught here. Until 2026-11-09 it + was, and given as a verdict about the picture (review of #178, a PNG whose + last chunk ran to the very end). That check now refuses such a PNG itself, + and an exception left anywhere is a fault in the checker. It has to show + as one (CRASH in --batch), because a crash counted as a refusal is how a + broken checker passes for a working one.""" try: - with contextlib.redirect_stdout(said): - CHECKS[kind](raw) - except SystemExit as exc: - if exc.code not in (0, None): - fail(f"picture {n} is not a good {kind} file: {said.getvalue().strip()}") + CHECKS[kind](raw) + except Verdict as said: + if said.word != "OK": + fail(f"picture {n} is not a good {kind} file: {said.reason}") return - except Exception as exc: - fail(f"picture {n} is not a good {kind} file: {type(exc).__name__}: {str(exc)[:120]}") fail(f"the {kind} check gave no verdict on picture {n}") @@ -1477,6 +1545,12 @@ def opc_check(data, roots, kind, key): """ import xml.etree.ElementTree as ET + # The ZIP layer first, the same walk a plain archive gets. Python's own + # reader finds the central directory by searching back from the end, so + # bytes after the comment and a local header that is not there both reach + # the checks below unnoticed, or stop them with an exception instead of a + # verdict (measured 2026-11-09). + zip_structure(data) z = opc_open(data) names = [i.filename for i in z.infolist()] @@ -1495,7 +1569,7 @@ def opc_check(data, roots, kind, key): fail("there is no _rels/.rels, so nothing points at the document") try: - types = ET.fromstring(z.read("[Content_Types].xml")) + types = ET.fromstring(opc_part(z, "[Content_Types].xml")) except ET.ParseError as exc: fail("[Content_Types].xml is not well formed: %s" % exc) @@ -1527,7 +1601,7 @@ def opc_check(data, roots, kind, key): if not name.endswith(".xml") and not name.endswith(".rels"): continue try: - root = ET.fromstring(z.read(name)) + root = ET.fromstring(opc_part(z, name)) except ET.ParseError as exc: fail("the part %s is not well formed XML: %s" % (name, exc)) roots_found.append((name, root.tag)) @@ -1537,7 +1611,7 @@ def opc_check(data, roots, kind, key): if not name.endswith(".rels"): continue base = name.rsplit("_rels/", 1)[0] - rels = ET.fromstring(z.read(name)) + rels = ET.fromstring(opc_part(z, name)) for rel in rels.findall(OPC_NS_REL + "Relationship"): if rel.get("TargetMode") == "External": continue @@ -1567,6 +1641,20 @@ def opc_check(data, roots, kind, key): % (kind, len(names), pad, len(z.comment), read)) +def opc_part(z, name): + """The bytes of one part, or a verdict when the archive will not give them. + + A part whose checksum does not match, or whose compressed data ends early, + is a defect in the package, and it has to read as one rather than as a + stack trace from the ZIP reader. + """ + import zipfile + try: + return z.read(name) + except (zipfile.BadZipFile, zlib.error, EOFError) as exc: + fail("the part %s cannot be read from the archive: %s" % (name, exc)) + + def opc_resolve(base, target): """Turns a relationship target into a path inside the package.""" import posixpath @@ -4073,7 +4161,74 @@ def rtf_every(text, settings): # any other one is called. TAKES_SETTINGS = {"csv", "txt", "md", "json", "xml", "html", "geojson", "eml", "rtf"} -if __name__ == "__main__": +def run_check(kind, body, settings): + """Runs one check. It ends by raising its Verdict, or by an exception.""" + if kind in TAKES_SETTINGS: + CHECKS[kind](body, settings) + else: + CHECKS[kind](body) + + +def judge(kind, body, settings): + """One file judged: ("OK" | "FAIL" | "CRASH", what was said). + + CRASH is the check stopping on an exception instead of giving a verdict. + It is kept apart from FAIL on purpose. A guard asking whether a damaged + file is refused must not count the checker falling over as a refusal. + """ + try: + run_check(kind, body, settings) + except Verdict as said: + return said.word, said.reason + except Exception as exc: # noqa: BLE001 - every exception here is a fault in a check, named below + return "CRASH", f"{type(exc).__name__}: {str(exc)[:200]}" + return "CRASH", "the check returned without a verdict" + + +def batch_answer(line): + """The answer to one --batch request line, as a dict.""" + import json + + try: + ask = json.loads(line) + kind, path = ask["format"], ask["path"] + settings = ask.get("settings") or {} + if not isinstance(kind, str) or not isinstance(path, str): + # open() takes a number as a descriptor, which is not a file asked for. + raise TypeError("format and path have to be strings") + except (ValueError, KeyError, TypeError, AttributeError) as exc: + return {"path": "", "verdict": "ERROR", "reason": f"the request {line[:80]!r} is not readable: {exc}"} + answer = {"path": path, "verdict": "ERROR"} + if kind not in CHECKS: + answer["reason"] = f"there is no check for {kind!r}" + elif not isinstance(settings, dict) or not all(isinstance(v, str) for v in settings.values()): + answer["reason"] = "settings have to be an object of strings" + elif settings and kind not in TAKES_SETTINGS: + answer["reason"] = f"the {kind} check takes no settings, and was given {sorted(settings)}" + else: + try: + with open(path, "rb") as handle: + body = handle.read() + except OSError as exc: + answer["reason"] = f"the file cannot be read: {exc}" + else: + answer["verdict"], answer["reason"] = judge(kind, body, settings) + return answer + + +def main_batch(): + """Many files in one process, so a guard judging every format against every + damage does not start Python a few hundred times.""" + import json + + for raw in sys.stdin.buffer: + line = raw.decode("utf-8", errors="replace").strip() + if line: + sys.stdout.write(json.dumps(batch_answer(line)) + "\n") + sys.stdout.flush() + + +def main_one(): if len(sys.argv) < 3 or sys.argv[1] not in CHECKS: print("FAIL usage: strict.py <" + "|".join(CHECKS) + "> [key=value ...]") sys.exit(1) @@ -4090,7 +4245,17 @@ def rtf_every(text, settings): sys.exit(1) with open(sys.argv[2], "rb") as handle: body = handle.read() - if kind in TAKES_SETTINGS: - CHECKS[kind](body, extra) + # An exception is left to end the process with its traceback, which is + # what a caller reading only OK and FAIL reports as a checker fault. + try: + run_check(kind, body, extra) + except Verdict as said: + print(said.word + " " + said.reason) + sys.exit(said.code) + + +if __name__ == "__main__": + if sys.argv[1:] == ["--batch"]: + main_batch() else: - CHECKS[kind](body) + main_one() diff --git a/internal/oracle/strictbatch.go b/internal/oracle/strictbatch.go new file mode 100644 index 00000000..69680c8f --- /dev/null +++ b/internal/oracle/strictbatch.go @@ -0,0 +1,120 @@ +package oracle + +import ( + "bufio" + "context" + "encoding/json" + "fmt" + "os/exec" + "strings" + "time" +) + +// StrictRequest is one file handed to StrictBatch. +type StrictRequest struct { + // Format is the ID of the check to run, as for Strict. + Format string `json:"format"` + // Path is the file to judge. + Path string `json:"path"` + // Settings are the key=value words Strict takes after the path. + Settings map[string]string `json:"settings,omitempty"` +} + +// StrictJudgement is the checker's answer about one file. +type StrictJudgement struct { + // Path is the file the answer is about, as it was asked. + Path string `json:"path"` + // Verdict is one of the Strict* verdict constants. + Verdict string `json:"verdict"` + // Reason is what the check said. + Reason string `json:"reason"` +} + +// The four answers the checker gives in batch mode. +const ( + // StrictOK is a file the check accepts. + StrictOK = "OK" + // StrictFail is a file the check refuses, for the reason given. + StrictFail = "FAIL" + // StrictCrash is the check stopping on an exception instead of giving + // a verdict. It is a fault in the checker, never a refusal of the file: + // counted as one, a broken checker passes for a working one + // (docs/DAMAGE-ETAP-2-2026-11-09.md section 3.3). + StrictCrash = "CRASH" + // StrictError is a request the checker could not act on. + StrictError = "ERROR" +) + +// StrictBatch judges many files in one run of the structural checker. +// +// Strict starts Python once per file. A guard that judges every format +// against every damage asks a few hundred times, so this hands the whole list +// to one process and reads one answer per file back, in the order asked. +// +// The second result is false when there is no Python or no checker to run, +// which the caller reports as a skip rather than passing quietly. An error +// means the run itself went wrong - the process failed, or the answers do not +// match the questions one to one - and no verdict in it can be trusted. +func StrictBatch(reqs []StrictRequest) ([]StrictJudgement, bool, error) { + python, ok := inPath("python")() + if !ok { + return nil, false, nil + } + script, ok := strictScriptPath() + if !ok { + return nil, false, nil + } + + var in strings.Builder + for _, r := range reqs { + line, err := json.Marshal(r) + if err != nil { + return nil, true, fmt.Errorf("writing the request for %s: %w", r.Path, err) + } + in.Write(line) + in.WriteByte('\n') + } + + // A minute for the process, and a second for every file on top. + limit := time.Minute + time.Duration(len(reqs))*time.Second + ctx, cancel := context.WithTimeout(context.Background(), limit) + defer cancel() + + //nolint:gosec // our own script, run against files this tool wrote + // nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command + cmd := exec.CommandContext(ctx, python, script, "--batch") + cmd.Stdin = strings.NewReader(in.String()) + var out, errOut strings.Builder + cmd.Stdout = &out + cmd.Stderr = &errOut + if err := cmd.Run(); err != nil { + return nil, true, fmt.Errorf("the structural checker stopped: %w: %s", err, strings.TrimSpace(errOut.String())) + } + + got := make([]StrictJudgement, 0, len(reqs)) + scan := bufio.NewScanner(strings.NewReader(out.String())) + scan.Buffer(make([]byte, 64*1024), 1024*1024) + for scan.Scan() { + line := strings.TrimSpace(scan.Text()) + if line == "" { + continue + } + var j StrictJudgement + if err := json.Unmarshal([]byte(line), &j); err != nil { + return nil, true, fmt.Errorf("the structural checker answered %q, which is not an answer: %w", line, err) + } + got = append(got, j) + } + if err := scan.Err(); err != nil { + return nil, true, fmt.Errorf("reading the structural checker's answers: %w", err) + } + if len(got) != len(reqs) { + return nil, true, fmt.Errorf("the structural checker was asked about %d file(s) and answered %d", len(reqs), len(got)) + } + for i := range got { + if got[i].Path != reqs[i].Path { + return nil, true, fmt.Errorf("answer %d is about %q and the question was about %q", i+1, got[i].Path, reqs[i].Path) + } + } + return got, true, nil +} From fe0ebfdf54b7e75ac8b89a9c1cb82d4bcbe359be Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 10 Nov 2026 08:34:59 +0100 Subject: [PATCH 2/6] Every damage is judged in every format, with the checker as the witness A damage is offered for a format only when a judge refuses the result. That was a sweep taken once, over seven formats in the test suite. A guard now writes, through the real engine, a healthy and a damaged file for every format and every registered damage, at its defaults and with each parameter at both ends, at the smallest size the pair can be given and at 20 kB, and judges them all in one run of the structural checker. A damaged file that is accepted is red, and so is a checker that falls over instead of answering. A format with no structural check is red too, because the offer would reach it by default. Co-Authored-By: Claude Opus 5.5 --- internal/damage/damage.go | 4 +- internal/guard/damagerefused_test.go | 65 +------- internal/guard/damagewitness_test.go | 223 +++++++++++++++++++++++++++ internal/guard/mutation/entries.py | 16 +- 4 files changed, 244 insertions(+), 64 deletions(-) create mode 100644 internal/guard/damagewitness_test.go diff --git a/internal/damage/damage.go b/internal/damage/damage.go index 0c6641ab..7ea1fe81 100644 --- a/internal/damage/damage.go +++ b/internal/damage/damage.go @@ -19,7 +19,9 @@ // What this package does NOT do is decide whether a damaged file is worth // writing. That is the witness rule - a pair with no judge able to refuse the // result is a pair this tool does not offer - and it belongs with the oracles, -// because it is a measurement rather than a declaration. +// because it is a measurement rather than a declaration. The guard +// TestEveryDamageIsRefusedByTheStructuralCheckInEveryFormat takes that +// measurement for every format and every damage registered here. package damage import ( diff --git a/internal/guard/damagerefused_test.go b/internal/guard/damagerefused_test.go index 5150dc97..2079a48e 100644 --- a/internal/guard/damagerefused_test.go +++ b/internal/guard/damagerefused_test.go @@ -13,53 +13,14 @@ import ( "github.com/donislawdev/TestingFilesGenerator/internal/cli" "github.com/donislawdev/TestingFilesGenerator/internal/damage" "github.com/donislawdev/TestingFilesGenerator/internal/engine" - "github.com/donislawdev/TestingFilesGenerator/internal/format" _ "github.com/donislawdev/TestingFilesGenerator/internal/format/all" "github.com/donislawdev/TestingFilesGenerator/internal/manifest" - "github.com/donislawdev/TestingFilesGenerator/internal/oracle" "github.com/donislawdev/TestingFilesGenerator/internal/recipe" ) -// A damaged file has to be REFUSED, which is the reverse of everything else in -// this package. -// -// Every other oracle guard here asks "does a judge accept what we wrote". This -// one asks the opposite, and it is the whole reason the damage axis is worth -// having: a broken file no reader complains about is a file nobody can write a -// test against, so it is a file this tool has no business producing. -// -// The formats are the text ones plus png, because those are the ones whose -// judges run without an external tool being installed. That is a limit of what -// can be asked on any machine rather than of the damage: the full sweep across -// all twenty four lives in tools/probes/corruptwitness, which is where the -// witness matrix is measured. -func TestADamagedFileIsRefusedByAJudge(t *testing.T) { - dir := t.TempDir() - - for _, id := range []string{"txt", "md", "json", "xml", "csv", "html", "png"} { - t.Run(id, func(t *testing.T) { - good, broken := writeGoodAndBroken(t, dir, id) - - // The control first. Without it this test passes for a build whose - // judge refuses everything, which would be the loudest possible - // way of proving nothing. - if res := oracle.Strict(id, good); res.Available && res.Err != nil { - t.Fatalf("the undamaged file was refused, so this row says nothing about damage: %v\n%s", - res.Err, res.Output) - } - - res := oracle.Strict(id, broken) - if !res.Available { - t.Skipf("no structural check for %s on this machine", id) - } - if res.Err == nil { - t.Errorf("the damaged file was ACCEPTED. A broken file every reader takes is one "+ - "nobody can write a test against, and the manifest calls it expected: reject.\n%s", - res.Output) - } - }) - } -} +// Whether a damaged file is refused by a judge is asked of every format and +// every damage in damagewitness_test.go, which replaced the seven formats +// asked here until 2026-11-09. // The manifest of a damaged run says what was broken and expects a rejection. // @@ -157,26 +118,6 @@ type discard struct{} func (discard) Write(p []byte) (int, error) { return len(p), nil } -// writeGoodAndBroken produces one file of this format twice, once whole and -// once damaged, and returns both paths. -func writeGoodAndBroken(t *testing.T, dir, id string) (good, broken string) { - t.Helper() - - d, err := format.Get(id) - if err != nil { - t.Fatal(err) - } - size := d.SmallestAccepted(format.Request{}) + 4096 - - goodDir := filepath.Join(dir, id+"-good") - brokenDir := filepath.Join(dir, id+"-broken") - goodMan := runOne(t, goodDir, id, size, nil) - brokenMan := runOne(t, brokenDir, id, size, damage.Chain{{ID: damage.ZeroHead}}) - - return filepath.Join(goodDir, goodMan.Files[0].Name), - filepath.Join(brokenDir, brokenMan.Files[0].Name) -} - // runDamaged is one png of a comfortable size, with the chain given. func runDamaged(t *testing.T, dir, id string, chain damage.Chain) *manifest.Manifest { t.Helper() diff --git a/internal/guard/damagewitness_test.go b/internal/guard/damagewitness_test.go new file mode 100644 index 00000000..bfe21941 --- /dev/null +++ b/internal/guard/damagewitness_test.go @@ -0,0 +1,223 @@ +package guard + +import ( + "fmt" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/damage" + "github.com/donislawdev/TestingFilesGenerator/internal/engine" + "github.com/donislawdev/TestingFilesGenerator/internal/format" + "github.com/donislawdev/TestingFilesGenerator/internal/oracle" +) + +// witnessSize is the comfortable size every pair is also judged at, beside the +// smallest one it can be given. Measured 2026-11-09: the set of witnesses was +// the same at 20 kB and at 1 MiB in every format, while the smallest size is +// where the answer moved (docs/DAMAGE-ETAP-2-2026-11-09.md section 2.1). +const witnessSize = 20 << 10 + +// damagePoint is one damage with one set of values. +type damagePoint struct { + id string + values damage.Values + label string +} + +// witnessCase is one damaged file and the healthy file of the same format +// and size beside it. +type witnessCase struct { + format string + point damagePoint + size int64 + good string + broken string +} + +// TestEveryDamageIsRefusedByTheStructuralCheckInEveryFormat is the witness +// rule as a guard rather than a measurement taken once. +// +// A damage is offered for a format only when a judge refuses the result, so a +// test written against the file can tell an upload validator that let it +// through. Until 2026-11-09 that was a sweep from September, and the offer +// covered every format, including one added the day after the sweep. Now +// every format in the registry meets every damage in the registry, with its +// default values and each parameter at both ends, at the smallest size the +// pair can be given and at 20 kB. Each file comes from the real engine, and +// the structural checker judges it beside a healthy file of the same format +// and size. +// +// Red when a damaged file is accepted, and red when the checker falls over +// instead of answering: a crash is a fault in the checker, never a witness. A +// format the checker does not know is red too. Nobody has decided whether a +// damage can be offered there, and the offer must not reach it by default. +// +// Measured at the default settings of each format. A format whose settings +// change the answer (the shape of a log line, the separator of a table) is +// judged here only as it comes by default, and that limit is written down +// in docs/DAMAGE-ETAP-2-2026-11-09.md section 2.2. +func TestEveryDamageIsRefusedByTheStructuralCheckInEveryFormat(t *testing.T) { + dir := t.TempDir() + + var points []damagePoint + for _, d := range damage.All() { + points = append(points, damagePointsOf(t, d)...) + } + if len(points) == 0 { + t.Fatal("the damage registry is empty, so this guard would pass without judging anything") + } + + var cases []witnessCase + formats := format.All() + for _, f := range formats { + if !oracle.StrictKnows(f.ID) { + t.Errorf("%s has no structural check, so no damage has a witness there.\n"+ + "Reason: the offer of a damage reaches every format by default, so a format\n"+ + "nobody can judge would be handed damaged files nothing refuses. Give it a\n"+ + "check in internal/oracle/strict.py before it is registered.", f.ID) + continue + } + cases = append(cases, witnessCasesOf(t, dir, f, points)...) + } + + reqs := make([]oracle.StrictRequest, 0, 2*len(cases)) + for _, c := range cases { + reqs = append(reqs, + oracle.StrictRequest{Format: c.format, Path: c.good}, + oracle.StrictRequest{Format: c.format, Path: c.broken}) + } + got, available, err := oracle.StrictBatch(reqs) + if !available { + t.Skip("the structural checker needs python, and there is none on this machine") + } + if err != nil { + t.Fatal(err) + } + + witnessed := 0 + for i, c := range cases { + good, broken := got[2*i], got[2*i+1] + where := fmt.Sprintf("%s at %d B with %s", c.format, c.size, c.point.label) + if good.Verdict != oracle.StrictOK { + // The control. A checker that refused everything would witness + // every damage, so a pair whose healthy file is refused says + // nothing about the damage. + t.Errorf("%s: the healthy file was answered %s %q, so this pair says nothing about the damage", + where, good.Verdict, good.Reason) + continue + } + switch broken.Verdict { + case oracle.StrictFail: + witnessed++ + case oracle.StrictOK: + t.Errorf("%s: the damaged file was ACCEPTED (%s).\n"+ + "Reason: a damaged file nothing refuses is one no test can be written against,\n"+ + "and its manifest calls it expected: reject. Either the damage is not offered\n"+ + "for this format, or the structural check is missing a rule of the format.", + where, broken.Reason) + default: + t.Errorf("%s: the checker answered %s %q about the damaged file.\n"+ + "Reason: that is a fault in the checker, not a refusal of the file. Counted as\n"+ + "a witness, a checker that falls over on every damaged file would witness\n"+ + "every damage.", where, broken.Verdict, broken.Reason) + } + } + if want := len(formats) * len(points); len(cases) < want { + t.Errorf("%d pair(s) judged, and %d formats by %d damage points need at least %d", + len(cases), len(formats), len(points), want) + } + t.Logf("%d damaged file(s) refused, across %d formats and %d damage point(s)", + witnessed, len(formats), len(points)) +} + +// damagePointsOf is one damage with its defaults, and with each parameter at +// both ends of what it accepts. +func damagePointsOf(t *testing.T, d damage.Descriptor) []damagePoint { + t.Helper() + points := []damagePoint{{id: d.ID, values: damage.Values{}, label: d.ID + " at its defaults"}} + for _, p := range d.Parameters { + var values []string + switch p.Kind { + case format.PropertyInt: + values = []string{strconv.FormatInt(p.Min, 10), strconv.FormatInt(p.Max, 10)} + case format.PropertyChoice: + values = p.Choices + default: + t.Errorf("%s has a parameter %s of kind %s, which this guard does not know how to sweep.\n"+ + "Reason: a value nobody judged is a value whose files may have no witness.", + d.ID, p.Name, p.Kind) + } + for _, v := range values { + points = append(points, damagePoint{ + id: d.ID, + values: damage.Values{p.Name: v}, + label: fmt.Sprintf("%s %s=%s", d.ID, p.Name, v), + }) + } + } + return points +} + +// witnessCasesOf writes, for one format, a healthy and a damaged file of each +// damage point at the two sizes, all in one run of the engine. +func witnessCasesOf(t *testing.T, dir string, f format.Descriptor, points []damagePoint) []witnessCase { + t.Helper() + smallest := f.SmallestAccepted(format.Request{}) + + var targets []engine.Target + var cases []witnessCase + sized := map[int64]bool{} + for n, p := range points { + chain := damage.Chain{{ID: p.id, Values: p.values}} + floor, _, err := chain.Floor() + if err != nil { + t.Fatalf("%s: %v", p.label, err) + } + for _, size := range witnessSizes(max(smallest, floor)) { + id := fmt.Sprintf("p%d-%d", n, size) + targets = append(targets, engine.Target{ID: id, Format: f.ID, Sizes: engine.Uniform(1, size), Damage: chain}) + cases = append(cases, witnessCase{format: f.ID, point: p, size: size, broken: id}) + if !sized[size] { + sized[size] = true + targets = append(targets, engine.Target{ID: fmt.Sprintf("good-%d", size), Format: f.ID, Sizes: engine.Uniform(1, size)}) + } + } + } + + out := filepath.Join(dir, f.ID) + opt := engine.Options{OutDir: out, Seed: 5, ManifestName: engine.DefaultManifestName} + planned, err := engine.Plan(targets, opt) + if err != nil { + t.Fatalf("%s: planning the witness files: %v", f.ID, err) + } + res, err := engine.Run(t.Context(), planned, opt) + if err != nil { + t.Fatalf("%s: writing the witness files: %v", f.ID, err) + } + paths := map[string]string{} + for _, m := range res.Manifest.Files { + if m.Failed { + t.Fatalf("%s: target %s failed: %s", f.ID, m.TargetID, m.Error) + } + paths[m.TargetID] = filepath.Join(out, filepath.FromSlash(m.Path)) + } + for i := range cases { + c := &cases[i] + c.good, c.broken = paths[fmt.Sprintf("good-%d", c.size)], paths[c.broken] + if c.good == "" || c.broken == "" || !strings.HasPrefix(c.broken, out) { + t.Fatalf("%s: the run wrote no file for %s at %d B", f.ID, c.point.label, c.size) + } + } + return cases +} + +// witnessSizes is the smallest size a pair can be given, and the comfortable +// one when that is larger. +func witnessSizes(smallest int64) []int64 { + if smallest >= witnessSize { + return []int64{smallest} + } + return []int64{smallest, witnessSize} +} diff --git a/internal/guard/mutation/entries.py b/internal/guard/mutation/entries.py index cee5ee2d..5aed5e90 100644 --- a/internal/guard/mutation/entries.py +++ b/internal/guard/mutation/entries.py @@ -5866,7 +5866,7 @@ "internal/engine/parallel.go", "\tif f.Damaged() {\n\t\tdamaged, opened, err := f.Target.Damage.Open(counter)", "\tif f.Damaged() && false {\n\t\tdamaged, opened, err := f.Target.Damage.Open(counter)", - "TestADamagedFileIsRefusedByAJudge", + "TestEveryDamageIsRefusedByTheStructuralCheckInEveryFormat", ), ( "the manifest stops recording what was broken about a file", @@ -17078,4 +17078,18 @@ "class Verdict(Exception):", "TestACheckThatStopsOnAnExceptionIsACrashNotARefusal", ), + ( + "the structural checker stops reading the PNG signature, so a PNG with a zeroed head passes", + "internal/oracle/strict.py", + ' if data[:8] != b"\\x89PNG\\r\\n\\x1a\\n":\n fail("the signature is not a PNG signature")', + ' if False:\n fail("the signature is not a PNG signature")', + "TestEveryDamageIsRefusedByTheStructuralCheckInEveryFormat", + ), + ( + "the structural checker stops reading the PDF header, so a PDF with a zeroed head passes", + "internal/oracle/strict.py", + ' if not data.startswith(b"%PDF-"):', + " if False:", + "TestEveryDamageIsRefusedByTheStructuralCheckInEveryFormat", + ), ] From c8e2251145583e14ea08ac2cbde75ff981212755 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 10 Nov 2026 08:38:55 +0100 Subject: [PATCH 3/6] A damaged file no longer claims its label The label is written by the generator and the damage comes after it, so it can overwrite it: zeroing the head of a text file took the label with it while the manifest still said label_embedded: true. A damaged file now says false, at the top of its entry and in its properties, with a label_not_guaranteed note saying why. A guard holds this for every format, beside a healthy file that does claim its label. The witness guard now writes its files with the label, as both surfaces do by default. Co-Authored-By: Claude Opus 5.5 --- internal/engine/engine.go | 18 +++++- internal/guard/damagelabel_test.go | 82 +++++++++++++++++++++++++++ internal/guard/damagewitness_test.go | 13 +++-- internal/guard/mutation/entries.py | 21 +++++++ internal/gui/text/locale/said/en.json | 5 ++ internal/gui/text/locale/said/pl.json | 1 + 6 files changed, 133 insertions(+), 7 deletions(-) create mode 100644 internal/guard/damagelabel_test.go diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 393cec59..6042a69c 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io/fs" + "maps" "os" "path/filepath" "strings" @@ -728,6 +729,21 @@ func (res *Result) entryFor(f PlannedFile, sha string, materialized bool, failur } label, _ := f.Plan.Properties[format.PropertyLabelEmbedded].(bool) + props := f.Plan.Properties + if label && f.Damaged() { + // The generator wrote the label and a damage came after it. Where the + // label sits differs by format - the first line of a text, pixels in + // a picture, a part inside a package - so whether a damage reached it + // cannot be said once for all of them. Measured 2026-11-09: zero-head + // on txt overwrote the label it sat beside, and the manifest still + // claimed it. So a damaged file never claims its label, and the note + // says why (docs/DAMAGE-ETAP-2-2026-11-09.md section 4.7). + label = false + props = maps.Clone(props) + props[format.PropertyLabelEmbedded] = false + notes = append(notes, res.note("label_not_guaranteed", + core.Says("engine.LabelNotGuaranteed", "The label was written before the file was damaged, so the damage may have overwritten it."))) + } e := manifest.File{ ID: f.ID, @@ -741,7 +757,7 @@ func (res *Result) entryFor(f PlannedFile, sha string, materialized bool, failur Seed: core.SeedLabel(f.Seed), Generator: manifest.GeneratorRef{Name: f.Desc.ID, Version: f.Desc.GeneratorVersion}, Determinism: string(f.Plan.Determinism), - Properties: f.Plan.Properties, + Properties: props, LabelEmbedded: label, Damage: damageFor(f), Notes: notes, diff --git a/internal/guard/damagelabel_test.go b/internal/guard/damagelabel_test.go new file mode 100644 index 00000000..53251924 --- /dev/null +++ b/internal/guard/damagelabel_test.go @@ -0,0 +1,82 @@ +package guard + +import ( + "path/filepath" + "slices" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/damage" + "github.com/donislawdev/TestingFilesGenerator/internal/engine" + "github.com/donislawdev/TestingFilesGenerator/internal/format" + "github.com/donislawdev/TestingFilesGenerator/internal/manifest" +) + +// labelNotGuaranteed is the note a damaged file carries in place of a claim +// to its label. +const labelNotGuaranteed = "label_not_guaranteed" + +// TestADamagedFileNeverClaimsItsLabel holds the manifest to what it can know +// about a label after a damage. +// +// Measured 2026-11-09: zero-head on txt overwrote the label at the start of +// the text, and the manifest still said label_embedded: true. Where a label +// sits differs by format, so the rule is the same for all of them. A damaged +// file says false, in both places the manifest says it, and a note says why. +// +// Every format, each beside a healthy file of the same format and size. The +// healthy one is the control: a format that never claims its label would pass +// this guard without the rule being there, so the guard also counts the +// formats where the claim was really taken away. +func TestADamagedFileNeverClaimsItsLabel(t *testing.T) { + dir := t.TempDir() + chain := damage.Chain{{ID: damage.ZeroHead}} + + withdrawn := 0 + for _, f := range format.All() { + out := filepath.Join(dir, f.ID) + targets := []engine.Target{ + {ID: "good", Format: f.ID, Sizes: engine.Uniform(1, witnessSize), Label: true}, + {ID: "broken", Format: f.ID, Sizes: engine.Uniform(1, witnessSize), Damage: chain, Label: true}, + } + opt := engine.Options{OutDir: out, Seed: 5, ManifestName: engine.DefaultManifestName} + planned, err := engine.Plan(targets, opt) + if err != nil { + t.Fatalf("%s: planning: %v", f.ID, err) + } + res, err := engine.Run(t.Context(), planned, opt) + if err != nil { + t.Fatalf("%s: running: %v", f.ID, err) + } + entries := map[string]manifest.File{} + for _, m := range res.Manifest.Files { + entries[m.TargetID] = m + } + good, broken := entries["good"], entries["broken"] + if good.Name == "" || broken.Name == "" { + t.Fatalf("%s: the run did not write both files", f.ID) + } + + if broken.LabelEmbedded { + t.Errorf("%s: a damaged file claims label_embedded: true.\n"+ + "Reason: the damage came after the label was written and may have overwritten it,\n"+ + "so the manifest cannot know the label is still there.", f.ID) + } + if claim, ok := broken.Properties[format.PropertyLabelEmbedded].(bool); ok && claim { + t.Errorf("%s: a damaged file claims label_embedded: true in its properties, "+ + "while the field beside them says false", f.ID) + } + noted := slices.ContainsFunc(broken.Notes, func(n manifest.Note) bool { return n.Code == labelNotGuaranteed }) + if noted != good.LabelEmbedded { + t.Errorf("%s: the healthy file claims its label: %t, and the damaged one carries the note %s: %t.\n"+ + "Reason: the note says why a label that was written is no longer claimed, so it\n"+ + "belongs exactly where a claim was taken away.", f.ID, good.LabelEmbedded, labelNotGuaranteed, noted) + } + if good.LabelEmbedded { + withdrawn++ + } + } + if withdrawn == 0 { + t.Fatal("no healthy file claimed its label, so this guard took nothing away and proved nothing") + } + t.Logf("the claim was taken away in %d format(s)", withdrawn) +} diff --git a/internal/guard/damagewitness_test.go b/internal/guard/damagewitness_test.go index bfe21941..29daa651 100644 --- a/internal/guard/damagewitness_test.go +++ b/internal/guard/damagewitness_test.go @@ -45,9 +45,9 @@ type witnessCase struct { // covered every format, including one added the day after the sweep. Now // every format in the registry meets every damage in the registry, with its // default values and each parameter at both ends, at the smallest size the -// pair can be given and at 20 kB. Each file comes from the real engine, and -// the structural checker judges it beside a healthy file of the same format -// and size. +// pair can be given and at 20 kB. Each file comes from the real engine with +// its label, as both surfaces write by default, and the structural checker +// judges it beside a healthy file of the same format and size. // // Red when a damaged file is accepted, and red when the checker falls over // instead of answering: a crash is a fault in the checker, never a witness. A @@ -164,7 +164,8 @@ func damagePointsOf(t *testing.T, d damage.Descriptor) []damagePoint { // damage point at the two sizes, all in one run of the engine. func witnessCasesOf(t *testing.T, dir string, f format.Descriptor, points []damagePoint) []witnessCase { t.Helper() - smallest := f.SmallestAccepted(format.Request{}) + // With the label, because that is how both surfaces write by default. + smallest := f.SmallestAccepted(format.Request{Label: true}) var targets []engine.Target var cases []witnessCase @@ -177,11 +178,11 @@ func witnessCasesOf(t *testing.T, dir string, f format.Descriptor, points []dama } for _, size := range witnessSizes(max(smallest, floor)) { id := fmt.Sprintf("p%d-%d", n, size) - targets = append(targets, engine.Target{ID: id, Format: f.ID, Sizes: engine.Uniform(1, size), Damage: chain}) + targets = append(targets, engine.Target{ID: id, Format: f.ID, Sizes: engine.Uniform(1, size), Damage: chain, Label: true}) cases = append(cases, witnessCase{format: f.ID, point: p, size: size, broken: id}) if !sized[size] { sized[size] = true - targets = append(targets, engine.Target{ID: fmt.Sprintf("good-%d", size), Format: f.ID, Sizes: engine.Uniform(1, size)}) + targets = append(targets, engine.Target{ID: fmt.Sprintf("good-%d", size), Format: f.ID, Sizes: engine.Uniform(1, size), Label: true}) } } } diff --git a/internal/guard/mutation/entries.py b/internal/guard/mutation/entries.py index 5aed5e90..e84d3905 100644 --- a/internal/guard/mutation/entries.py +++ b/internal/guard/mutation/entries.py @@ -17092,4 +17092,25 @@ " if False:", "TestEveryDamageIsRefusedByTheStructuralCheckInEveryFormat", ), + ( + "a damaged file claims its label again, as the generator left it", + "internal/engine/engine.go", + " if label && f.Damaged() {", + " if false && label && f.Damaged() {", + "TestADamagedFileNeverClaimsItsLabel", + ), + ( + "a damaged file stops claiming its label in one place and keeps claiming it in its properties", + "internal/engine/engine.go", + " props[format.PropertyLabelEmbedded] = false\n", + "", + "TestADamagedFileNeverClaimsItsLabel", + ), + ( + "a damaged file stops claiming its label without saying why", + "internal/engine/engine.go", + ' notes = append(notes, res.note("label_not_guaranteed",', + ' _ = append(notes, res.note("label_not_guaranteed",', + "TestADamagedFileNeverClaimsItsLabel", + ), ] diff --git a/internal/gui/text/locale/said/en.json b/internal/gui/text/locale/said/en.json index 87ee781e..f576a94c 100644 --- a/internal/gui/text/locale/said/en.json +++ b/internal/gui/text/locale/said/en.json @@ -1004,6 +1004,11 @@ "hash": "sha256-7a1261279b19", "other": "cannot write the instructions to {{.Path}}: {{.Cause}}" }, + "engine.LabelNotGuaranteed": { + "description": "Said by internal/engine - a refusal under the box it is about or at the foot of the form, or a note after a run.", + "hash": "sha256-9d0de3a39db8", + "other": "The label was written before the file was damaged, so the damage may have overwritten it." + }, "engine.MemoryOverrun": { "description": "Said by internal/engine - a refusal under the box it is about or at the foot of the form, or a note after a run. Carries {{.Name}}, {{.Holds}}, {{.Room}}, written the way the program writes them - keep each spelled exactly that way.", "hash": "sha256-168a887a89fa", diff --git a/internal/gui/text/locale/said/pl.json b/internal/gui/text/locale/said/pl.json index 94a194ec..18245add 100644 --- a/internal/gui/text/locale/said/pl.json +++ b/internal/gui/text/locale/said/pl.json @@ -199,6 +199,7 @@ "engine.IDTwice": { "hash": "sha256-724959681477", "other": "nazwa partii {{.ID}} występuje dwa razy" }, "engine.IDTwiceWhy": { "hash": "sha256-ded29dfee98c", "other": "nazwy wskazują partie, więc powtórzenie jest błędem, a nie cichym nadpisaniem" }, "engine.InstructionsNotWritten": { "hash": "sha256-7a1261279b19", "other": "nie można zapisać instrukcji do {{.Path}}: {{.Cause}}" }, + "engine.LabelNotGuaranteed": { "hash": "sha256-9d0de3a39db8", "other": "Etykieta została zapisana, zanim plik uszkodzono, więc uszkodzenie mogło ją nadpisać." }, "engine.MemoryOverrun": { "hash": "sha256-168a887a89fa", "other": "{{.Name}} zajmuje przy zapisie około {{.Holds}}, więcej niż {{.Room}}, które ustawienie pamięci zostawiło zapisywanym plikom, więc został zapisany sam, a przebieg przekroczył ustawienie, dopóki go nie skończył" }, "engine.MemoryUnknown": { "hash": "sha256-25e95b18a15e", "other": "nie udało się odczytać pamięci tej maszyny ({{.Cause}}), więc ten przebieg wziął {{.Limit}}, czyli najwięcej, ile brał każdy przebieg, zanim powstało ustawienie pamięci. Ustaw pamięć sam, żeby wybrać limit" }, "engine.NameAbsolute": { "hash": "sha256-2dc5711eaa7d", "other": "{{.Where}} daje ścieżkę bezwzględną {{.Name}}" }, From e1dca12c648e06ef4be18c54220316fda3f21a74 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 10 Nov 2026 08:43:59 +0100 Subject: [PATCH 4/6] The witness checker runs under its caller's context, and the changelog says what a user notices StrictBatch takes the caller's context, so a guard that is stopped stops the checker with it. The changelog lists the one change a user of the tool sees: a damaged file no longer claims to carry its label. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++++++ internal/guard/damagewitness_test.go | 2 +- internal/guard/strictverdict_test.go | 2 +- internal/oracle/strictbatch.go | 7 +++++-- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63ccc83e..58e84e11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,12 @@ because it turns other people's test suites red. ### Fixed +- **A damaged file no longer claims to carry its label.** Its manifest entry + said `label_embedded: true`, although the damage comes after the label is + written and can overwrite it. On a text file, `zero-head` zeroes the start of + the label itself. A damaged file now says `false`, in its `properties` too, + with a `label_not_guaranteed` note saying why. An undamaged file is + described as before, and `manifest_version` does not change. - **A recipe asking for more than a million files in all is refused before it takes the memory.** Targets were held to a million each and the run only once all their sizes were laid out, so `tfg validate` took 2.5 GB to refuse diff --git a/internal/guard/damagewitness_test.go b/internal/guard/damagewitness_test.go index 29daa651..bad5a618 100644 --- a/internal/guard/damagewitness_test.go +++ b/internal/guard/damagewitness_test.go @@ -88,7 +88,7 @@ func TestEveryDamageIsRefusedByTheStructuralCheckInEveryFormat(t *testing.T) { oracle.StrictRequest{Format: c.format, Path: c.good}, oracle.StrictRequest{Format: c.format, Path: c.broken}) } - got, available, err := oracle.StrictBatch(reqs) + got, available, err := oracle.StrictBatch(t.Context(), reqs) if !available { t.Skip("the structural checker needs python, and there is none on this machine") } diff --git a/internal/guard/strictverdict_test.go b/internal/guard/strictverdict_test.go index 46edc002..e2749360 100644 --- a/internal/guard/strictverdict_test.go +++ b/internal/guard/strictverdict_test.go @@ -75,7 +75,7 @@ func TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping(t *testing.T) { reqs[i] = oracle.StrictRequest{Format: c.format, Path: path} } - got, available, err := oracle.StrictBatch(reqs) + got, available, err := oracle.StrictBatch(t.Context(), reqs) if !available { t.Skip("the structural checker needs python, and there is none on this machine") } diff --git a/internal/oracle/strictbatch.go b/internal/oracle/strictbatch.go index 69680c8f..e222741b 100644 --- a/internal/oracle/strictbatch.go +++ b/internal/oracle/strictbatch.go @@ -55,7 +55,10 @@ const ( // which the caller reports as a skip rather than passing quietly. An error // means the run itself went wrong - the process failed, or the answers do not // match the questions one to one - and no verdict in it can be trusted. -func StrictBatch(reqs []StrictRequest) ([]StrictJudgement, bool, error) { +// +// The context is the caller's, so a guard that is stopped stops the checker +// with it. A time limit of its own sits under it. +func StrictBatch(ctx context.Context, reqs []StrictRequest) ([]StrictJudgement, bool, error) { python, ok := inPath("python")() if !ok { return nil, false, nil @@ -77,7 +80,7 @@ func StrictBatch(reqs []StrictRequest) ([]StrictJudgement, bool, error) { // A minute for the process, and a second for every file on top. limit := time.Minute + time.Duration(len(reqs))*time.Second - ctx, cancel := context.WithTimeout(context.Background(), limit) + ctx, cancel := context.WithTimeout(ctx, limit) defer cancel() //nolint:gosec // our own script, run against files this tool wrote From 7b1d93fc5148d0236bb3fff44c5858ab33b343fc Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 10 Nov 2026 08:49:53 +0100 Subject: [PATCH 5/6] Three more ways the structural checker stopped on broken bytes are refusals Random damage to healthy files of every format found three: a cross reference offset that is not a number stopped the PDF check, a label heading never closed stopped the HTML check, and an Office part packed with a compression method nobody knows stopped the package check. Each is now refused with its reason, and the guard of broken bytes holds all three. Co-Authored-By: Claude Opus 5.5 --- internal/guard/mutation/entries.py | 23 +++++++++++- internal/guard/strictverdict_test.go | 52 ++++++++++++++++++++++++++++ internal/oracle/strict.py | 16 ++++++--- 3 files changed, 85 insertions(+), 6 deletions(-) diff --git a/internal/guard/mutation/entries.py b/internal/guard/mutation/entries.py index e84d3905..d0b05e60 100644 --- a/internal/guard/mutation/entries.py +++ b/internal/guard/mutation/entries.py @@ -17039,7 +17039,7 @@ ( "a part of an Office package that cannot be read stops the check with a traceback", "internal/oracle/strict.py", - " except (zipfile.BadZipFile, zlib.error, EOFError) as exc:", + " except (zipfile.BadZipFile, zlib.error, EOFError, NotImplementedError) as exc:", " except KeyError as exc:", "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", ), @@ -17113,4 +17113,25 @@ ' _ = append(notes, res.note("label_not_guaranteed",', "TestADamagedFileNeverClaimsItsLabel", ), + ( + "a part of an Office package packed with a method no reader knows stops the check with a traceback", + "internal/oracle/strict.py", + "EOFError, NotImplementedError) as exc:", + "EOFError) as exc:", + "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", + ), + ( + "a cross reference offset that is not a number stops the PDF check with a traceback", + "internal/oracle/strict.py", + " if not parts[0].isdigit():", + " if False:", + "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", + ), + ( + "a label heading never closed stops the HTML check with a traceback", + "internal/oracle/strict.py", + " if label_end < 0:", + " if False:", + "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", + ), ] diff --git a/internal/guard/strictverdict_test.go b/internal/guard/strictverdict_test.go index e2749360..10fae38f 100644 --- a/internal/guard/strictverdict_test.go +++ b/internal/guard/strictverdict_test.go @@ -46,6 +46,8 @@ func TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping(t *testing.T) { docx := writeEncoded(t, "docx", 4096, nil) xlsx := writeEncoded(t, "xlsx", 4096, nil) pptx := writeEncoded(t, "pptx", 8192, nil) + pdf := writeEncoded(t, "pdf", 4096, nil) + html := writeEncoded(t, "html", 2048, nil) cases := []strictCase{ {"healthy png", "png", png, oracle.StrictOK, ""}, @@ -53,6 +55,8 @@ func TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping(t *testing.T) { {"healthy docx", "docx", docx, oracle.StrictOK, ""}, {"healthy xlsx", "xlsx", xlsx, oracle.StrictOK, ""}, {"healthy pptx", "pptx", pptx, oracle.StrictOK, ""}, + {"healthy pdf", "pdf", pdf, oracle.StrictOK, ""}, + {"healthy html", "html", html, oracle.StrictOK, ""}, {"png with nothing after the signature", "png", png[:8], oracle.StrictFail, "no chunk at all"}, {"png one byte short", "png", png[:len(png)-1], oracle.StrictFail, "before its 4 B checksum"}, @@ -63,6 +67,9 @@ func TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping(t *testing.T) { {"pptx with a zeroed head", "pptx", zeroed(pptx, 4), oracle.StrictFail, "no local file header"}, {"docx with bytes after its end", "docx", appended(docx, 64), oracle.StrictFail, "the comment declares 0 B and 64 follow"}, {"docx with a part that does not match its checksum", "docx", flipped(t, docx), oracle.StrictFail, "cannot be read from the archive"}, + {"docx with a compression method nobody knows", "docx", unknownMethod(t, docx), oracle.StrictFail, "compression method"}, + {"pdf whose cross reference names an offset that is not a number", "pdf", garbledOffset(t, pdf), oracle.StrictFail, "which is not a number"}, + {"html whose label heading is never closed", "html", unclosedLabel(t, html), oracle.StrictFail, "never closed on its line"}, } dir := t.TempDir() @@ -223,6 +230,51 @@ func renamed(t *testing.T, b []byte) []byte { return out } +// unknownMethod is the package with the compression method of its first +// central directory entry set to one no reader knows. +func unknownMethod(t *testing.T, b []byte) []byte { + out := append([]byte{}, b...) + at := bytes.Index(out, []byte("PK\x01\x02")) + if at < 0 || at+12 > len(out) { + t.Fatal("the package has no central directory entry to change, so this case would test nothing") + } + binary.LittleEndian.PutUint16(out[at+10:], 77) + return out +} + +// garbledOffset is the PDF with the first digit of object 1's offset in its +// cross reference table replaced by a letter. +func garbledOffset(t *testing.T, b []byte) []byte { + out := append([]byte{}, b...) + // On a line of its own, because "startxref" near the end holds the word too. + at := bytes.LastIndex(out, []byte("\nxref\n")) + if at >= 0 { + at++ + } + // Past the keyword, the count line and the free entry of object 0. + for range 3 { + next := bytes.IndexByte(out[max(at, 0):], '\n') + if at < 0 || next < 0 { + t.Fatal("the PDF has no cross reference entry for object 1, so this case would test nothing") + } + at += next + 1 + } + if at >= len(out) || out[at] < '0' || out[at] > '9' { + t.Fatal("the entry for object 1 does not start with a digit, so this case would test nothing") + } + out[at] = 'x' + return out +} + +// unclosedLabel is the page with the end tag of its label heading taken off. +func unclosedLabel(t *testing.T, b []byte) []byte { + at := bytes.Index(b, []byte("")) + if at < 0 { + t.Fatal("the page has no label heading, so this case would test nothing") + } + return append(append([]byte{}, b[:at]...), b[at+len(""):]...) +} + // flipped is the package with one byte of the first part's data changed, so // the part no longer matches the checksum its headers carry. func flipped(t *testing.T, b []byte) []byte { diff --git a/internal/oracle/strict.py b/internal/oracle/strict.py index 826bcf67..e6bcb7a0 100644 --- a/internal/oracle/strict.py +++ b/internal/oracle/strict.py @@ -161,6 +161,8 @@ def check_pdf(data): parts = entry.split() if len(parts) < 3 or parts[2] != b"n": continue + if not parts[0].isdigit(): + fail(f"the xref table gives object {i} the offset {parts[0]!r}, which is not a number") obj_at = int(parts[0]) expected = str(i).encode() + b" 0 obj" if data[obj_at:obj_at + len(expected)] != expected: @@ -796,7 +798,10 @@ def html_settings(text, shape, settings, nl): if re.match(r"]", body): # The label sits outside the levels, so a reader that drops the deep # ones still shows which file it read. - body = body[body.index("" + nl) + len("" + nl):] + label_end = body.find("" + nl) + if label_end < 0: + fail("the label heading is opened and never closed on its line") + body = body[label_end + len("" + nl):] div_open, div_close = "
" + nl, "
" + nl opened = 0 while body.startswith(div_open, opened * len(div_open)): @@ -1644,14 +1649,15 @@ def opc_check(data, roots, kind, key): def opc_part(z, name): """The bytes of one part, or a verdict when the archive will not give them. - A part whose checksum does not match, or whose compressed data ends early, - is a defect in the package, and it has to read as one rather than as a - stack trace from the ZIP reader. + A part whose checksum does not match, whose compressed data ends early, or + whose compression method no reader knows is a defect in the package, and + it has to read as one rather than as a stack trace from the ZIP reader. + Office packages are written with deflate or stored entries only. """ import zipfile try: return z.read(name) - except (zipfile.BadZipFile, zlib.error, EOFError) as exc: + except (zipfile.BadZipFile, zlib.error, EOFError, NotImplementedError) as exc: fail("the part %s cannot be read from the archive: %s" % (name, exc)) From 5f6956e52141ab383ed3a6975dc8997d02f0910c Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 10 Nov 2026 09:21:13 +0100 Subject: [PATCH 6/6] A request the checker cannot act on is one error, and a stopped caller stops the checker The batch mode stopped altogether on a line of JSON nested past what the decoder goes, and on a path holding a NUL. Each is now answered ERROR in its place, and the requests after it are still answered. Two guards hold the answers a batch gives to what it cannot act on, and that a caller whose context is stopped stops the checker. The sentence about a batch whose answers do not match its questions no longer writes file(s). Co-Authored-By: Claude Opus 5.5 --- internal/guard/mutation/entries.py | 21 ++++ internal/guard/strictverdict_test.go | 138 ++++++++++++++++++++++++--- internal/oracle/strict.py | 10 +- internal/oracle/strictbatch.go | 2 +- 4 files changed, 156 insertions(+), 15 deletions(-) diff --git a/internal/guard/mutation/entries.py b/internal/guard/mutation/entries.py index d0b05e60..e41d77be 100644 --- a/internal/guard/mutation/entries.py +++ b/internal/guard/mutation/entries.py @@ -17134,4 +17134,25 @@ " if False:", "TestTheStructuralCheckerRefusesBrokenBytesInsteadOfStopping", ), + ( + "a batch line nested past what the JSON decoder goes stops the whole batch", + "internal/oracle/strict.py", + " except (ValueError, KeyError, TypeError, AttributeError, RecursionError) as exc:", + " except (ValueError, KeyError, TypeError, AttributeError) as exc:", + "TestTheStructuralCheckerAnswersARequestItCannotActOnWithAnError", + ), + ( + "a batch request for a path holding a NUL stops the whole batch", + "internal/oracle/strict.py", + " except (OSError, ValueError) as exc:", + " except OSError as exc:", + "TestTheStructuralCheckerAnswersARequestItCannotActOnWithAnError", + ), + ( + "the structural checker runs under a context of its own, so a stopped caller leaves it running", + "internal/oracle/strictbatch.go", + " ctx, cancel := context.WithTimeout(ctx, limit)", + " ctx, cancel := context.WithTimeout(context.Background(), limit)", + "TestAStoppedCallerStopsTheStructuralChecker", + ), ] diff --git a/internal/guard/strictverdict_test.go b/internal/guard/strictverdict_test.go index 10fae38f..ab837522 100644 --- a/internal/guard/strictverdict_test.go +++ b/internal/guard/strictverdict_test.go @@ -2,11 +2,13 @@ package guard import ( "bytes" + "context" "encoding/binary" "encoding/json" "os" "os/exec" "path/filepath" + "strconv" "strings" "testing" @@ -142,18 +144,7 @@ for kind in ("png", "gif", "bmp"): // Planted checks, because the real ones are meant to have no crash left in // them, so no real file can reach these states. func TestACheckThatStopsOnAnExceptionIsACrashNotARefusal(t *testing.T) { - python, err := exec.LookPath("python") - if err != nil { - t.Skip("the structural checker needs python, and there is none on this machine") - } - here, err := os.Getwd() - if err != nil { - t.Fatal(err) - } - script, ok := oracle.StrictScriptUnder(here) - if !ok { - t.Fatalf("the structural checker is not above %s", here) - } + python, script := strictProgram(t) plant := filepath.Join(t.TempDir(), "plant.py") if err := os.WriteFile(plant, []byte(strictPlant), 0o600); err != nil { t.Fatal(err) @@ -189,6 +180,129 @@ func TestACheckThatStopsOnAnExceptionIsACrashNotARefusal(t *testing.T) { } } +// TestTheStructuralCheckerAnswersARequestItCannotActOnWithAnError holds the +// other side of the batch: a request the checker cannot act on is answered +// ERROR, in its place, and every request after it is still answered. +// +// Through StrictBatch for what a Go caller can send (a format with no check, +// settings to a check that takes none, a file that is not there, a path with +// a NUL in it), and through the program itself for lines no Go caller writes +// (a line that is not JSON, JSON nested past what the decoder goes). Before +// 2026-11-10 the last two of the first group and the nested line stopped the +// whole batch. The healthy file between them is the control: a batch that +// answered ERROR to everything would pass the rest. +func TestTheStructuralCheckerAnswersARequestItCannotActOnWithAnError(t *testing.T) { + dir := t.TempDir() + good := filepath.Join(dir, "good.zip") + if err := os.WriteFile(good, writeEncoded(t, "zip", 16384, nil), 0o600); err != nil { + t.Fatal(err) + } + asked := []struct { + req oracle.StrictRequest + answer string + reason string + }{ + {oracle.StrictRequest{Format: "nothing-of-that-name", Path: good}, oracle.StrictError, "there is no check"}, + {oracle.StrictRequest{Format: "zip", Path: good, Settings: map[string]string{"a": "b"}}, oracle.StrictError, "takes no settings"}, + {oracle.StrictRequest{Format: "zip", Path: good}, oracle.StrictOK, ""}, + {oracle.StrictRequest{Format: "zip", Path: filepath.Join(dir, "never-written.zip")}, oracle.StrictError, "cannot be read"}, + {oracle.StrictRequest{Format: "zip", Path: good + "\x00.zip"}, oracle.StrictError, "cannot be read"}, + {oracle.StrictRequest{Format: "zip", Path: good}, oracle.StrictOK, ""}, + } + reqs := make([]oracle.StrictRequest, len(asked)) + for i, a := range asked { + reqs[i] = a.req + } + got, available, err := oracle.StrictBatch(t.Context(), reqs) + if !available { + t.Skip("the structural checker needs python, and there is none on this machine") + } + if err != nil { + t.Fatalf("one request the checker cannot act on stopped the whole batch: %v", err) + } + for i, a := range asked { + if got[i].Verdict != a.answer || !strings.Contains(got[i].Reason, a.reason) { + t.Errorf("request %d (%s %q) was answered %s %q, and %s saying %q was wanted", + i+1, a.req.Format, a.req.Path, got[i].Verdict, got[i].Reason, a.answer, a.reason) + } + } + + python, script := strictProgram(t) + lines := []string{ + "this is not a request", + strings.Repeat("[", 200000) + strings.Repeat("]", 200000), + `{"format": "zip", "path": ` + strconv.Quote(good) + `}`, + } + // The interpreter is the one found on PATH, the script is the checker of + // this repository, and the input is the lines above. + // nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command + cmd := exec.CommandContext(t.Context(), python, script, "--batch") + cmd.Stdin = strings.NewReader(strings.Join(lines, "\n") + "\n") + out, err := cmd.Output() + if err != nil { + t.Fatalf("a line the checker cannot read stopped the whole batch: %v", err) + } + answers := strings.Split(strings.TrimSpace(string(out)), "\n") + if len(answers) != len(lines) { + t.Fatalf("%d line(s) asked and %d answered:\n%s", len(lines), len(answers), out) + } + for i, want := range []string{oracle.StrictError, oracle.StrictError, oracle.StrictOK} { + var j oracle.StrictJudgement + if err := json.Unmarshal([]byte(strings.TrimSpace(answers[i])), &j); err != nil { + t.Fatalf("answer %d is not an answer: %v", i+1, err) + } + if j.Verdict != want { + t.Errorf("line %d was answered %s %q, and %s was wanted", i+1, j.Verdict, j.Reason, want) + } + } +} + +// TestAStoppedCallerStopsTheStructuralChecker holds that the checker runs +// under its caller's context. A guard stopped by its time limit has to stop +// the Python process with it, rather than leave it running under a context +// of its own. +func TestAStoppedCallerStopsTheStructuralChecker(t *testing.T) { + dir := t.TempDir() + good := filepath.Join(dir, "good.zip") + if err := os.WriteFile(good, writeEncoded(t, "zip", 16384, nil), 0o600); err != nil { + t.Fatal(err) + } + reqs := []oracle.StrictRequest{{Format: "zip", Path: good}} + + // The control: the same request under a live context is answered. + if _, available, err := oracle.StrictBatch(t.Context(), reqs); !available { + t.Skip("the structural checker needs python, and there is none on this machine") + } else if err != nil { + t.Fatalf("the request was not answered under a live context: %v", err) + } + + stopped, cancel := context.WithCancel(t.Context()) + cancel() + if got, _, err := oracle.StrictBatch(stopped, reqs); err == nil { + t.Errorf("a stopped caller got %d answer(s) back. The checker ran under a context of its own, "+ + "so a guard that is stopped leaves it running", len(got)) + } +} + +// strictProgram is the interpreter on PATH and the checker of this +// repository, for a guard that runs the program itself. +func strictProgram(t *testing.T) (python, script string) { + t.Helper() + python, err := exec.LookPath("python") + if err != nil { + t.Skip("the structural checker needs python, and there is none on this machine") + } + here, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + script, ok := oracle.StrictScriptUnder(here) + if !ok { + t.Fatalf("the structural checker is not above %s", here) + } + return python, script +} + // zeroed is the file with its first n bytes set to zero. func zeroed(b []byte, n int) []byte { out := append([]byte{}, b...) diff --git a/internal/oracle/strict.py b/internal/oracle/strict.py index e6bcb7a0..21ee0eb7 100644 --- a/internal/oracle/strict.py +++ b/internal/oracle/strict.py @@ -4202,7 +4202,9 @@ def batch_answer(line): if not isinstance(kind, str) or not isinstance(path, str): # open() takes a number as a descriptor, which is not a file asked for. raise TypeError("format and path have to be strings") - except (ValueError, KeyError, TypeError, AttributeError) as exc: + except (ValueError, KeyError, TypeError, AttributeError, RecursionError) as exc: + # RecursionError is JSON nested deeper than the decoder goes. One bad + # line is one ERROR, and the requests after it are still answered. return {"path": "", "verdict": "ERROR", "reason": f"the request {line[:80]!r} is not readable: {exc}"} answer = {"path": path, "verdict": "ERROR"} if kind not in CHECKS: @@ -4212,10 +4214,14 @@ def batch_answer(line): elif settings and kind not in TAKES_SETTINGS: answer["reason"] = f"the {kind} check takes no settings, and was given {sorted(settings)}" else: + # The whole file, with no limit, on purpose: every check walks offsets + # and checksums across all of it, so a limit would turn a large healthy + # file into a refusal. The files are ones the caller has just written. + # ValueError is a path holding a NUL, which open() will not take. try: with open(path, "rb") as handle: body = handle.read() - except OSError as exc: + except (OSError, ValueError) as exc: answer["reason"] = f"the file cannot be read: {exc}" else: answer["verdict"], answer["reason"] = judge(kind, body, settings) diff --git a/internal/oracle/strictbatch.go b/internal/oracle/strictbatch.go index e222741b..9c94a477 100644 --- a/internal/oracle/strictbatch.go +++ b/internal/oracle/strictbatch.go @@ -112,7 +112,7 @@ func StrictBatch(ctx context.Context, reqs []StrictRequest) ([]StrictJudgement, return nil, true, fmt.Errorf("reading the structural checker's answers: %w", err) } if len(got) != len(reqs) { - return nil, true, fmt.Errorf("the structural checker was asked about %d file(s) and answered %d", len(reqs), len(got)) + return nil, true, fmt.Errorf("the structural checker's answers do not match its questions: asked %d, answered %d", len(reqs), len(got)) } for i := range got { if got[i].Path != reqs[i].Path {