diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1dc392505..2694bbacd 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -316,10 +316,23 @@ jobs: # install.sh and install.ps1 are the highest-consequence non-executable # bytes we publish: users pipe them straight into a shell. # - # The selected executables are re-submitted too. That is deliberate and - # nearly free: VirusTotal is content-addressed, so identical bytes return - # the analysis it already holds instead of re-running 70+ engines — the - # same property that made the analysis-id equality check untenable. + # The selected executables are NOT re-submitted. They were scanned as + # candidates, and verify-release-selection.py has just proven every + # published container carries exactly those bytes — identity is settled by + # hash, so a second scan adds no assurance. + # + # It also is not free, contrary to what this comment used to claim. + # Measured on v0.10.5: all eight re-submissions produced a NEW analysis + # (same file-id, timestamp 47 minutes later), and Microsoft's ML engine + # answered differently for two of them within that hour — in opposite + # directions. The release notes then cited one scan while the links showed + # the other. See scripts/ci/exclude-rescanned-selected-objects.sh. + - name: Withhold already-scanned selected executables + run: | + bash scripts/ci/exclude-rescanned-selected-objects.sh \ + binaries/objects "$RUNNER_TEMP/release-selection.tsv" \ + binaries/virustotal-withheld.tsv + - name: VirusTotal scan of every extracted release object uses: crazy-max/ghaction-virustotal@936d8c5c00afe97d3d9a1af26d017cfdf26800a2 # v5.0.0 id: virustotal diff --git a/internal/cbm/cbm.c b/internal/cbm/cbm.c index ea149dd9c..d7468af5a 100644 --- a/internal/cbm/cbm.c +++ b/internal/cbm/cbm.c @@ -787,7 +787,40 @@ static void cbm_error_regions_push(cbm_error_regions_t *acc, TSNode n) { acc->count++; } -static void cbm_collect_error_regions(TSNode n, cbm_error_regions_t *acc) { +/* #1610: a file that does not end with a newline leaves the grammar's + * mandatory line terminator MISSING. That node is ZERO-WIDTH and sits at EOF. + * + * It is not a miss. The parser consumed no source for it — start_byte == + * end_byte — so by construction nothing was dropped: no construct can live in + * a zero-byte span, and every real instruction above it parsed normally. This + * is a property of the grammar's terminator rule, not of the file. + * + * Flagging it made the verdict arbitrary. Grammars whose terminator token is + * VISIBLE (dockerfile, tcl, fish, gomod, hyprlang) reported parse_partial for + * a missing final newline; grammars whose terminator is HIDDEN (ini, fsharp, + * beancount, requirements, gitignore, sshconfig, kconfig) reported nothing for + * exactly the same omission, because a hidden node is invisible to + * ts_node_child(). Whether a user was told their file was partially parsed + * depended on a grammar-authoring accident. + * + * The cost was not cosmetic: a phantom parse_partial writes a + * "::missed" shadow row, and until #1609 that row made the project + * fail cross-repo validation as both source and target. + * + * Deliberately narrow — ZERO-WIDTH AT EOF ONLY. A MISSING or ERROR node with + * WIDTH still counts even at EOF (a Makefile whose last recipe line is + * unterminated really does lose the recipe), and anything before EOF is + * untouched. */ +static bool cbm_is_eof_terminator_miss(TSNode n, int source_len) { + if (!ts_node_is_missing(n) || source_len < 0) { + return false; + } + uint32_t start = ts_node_start_byte(n); + uint32_t end = ts_node_end_byte(n); + return start == end && end == (uint32_t)source_len; +} + +static void cbm_collect_error_regions(TSNode n, cbm_error_regions_t *acc, int source_len) { if (acc->count >= CBM_MAX_ERROR_REGIONS) { return; } @@ -795,9 +828,12 @@ static void cbm_collect_error_regions(TSNode n, cbm_error_regions_t *acc) { for (uint32_t i = 0; i < k && acc->count < CBM_MAX_ERROR_REGIONS; i++) { TSNode c = ts_node_child(n, i); if (ts_node_is_missing(c) || strcmp(ts_node_type(c), "ERROR") == 0) { + if (cbm_is_eof_terminator_miss(c, source_len)) { + continue; /* absent final newline only — nothing was dropped */ + } cbm_error_regions_push(acc, c); /* top-most region; do not descend */ } else if (ts_node_has_error(c)) { - cbm_collect_error_regions(c, acc); + cbm_collect_error_regions(c, acc, source_len); } } } @@ -1413,7 +1449,7 @@ CBMFileResult *cbm_extract_file_ex(const char *source, int source_len, CBMLangua * already extract. */ if (ts_node_has_error(root)) { cbm_error_regions_t raw_regs = {{0}, {0}, 0}; - cbm_collect_error_regions(root, &raw_regs); + cbm_collect_error_regions(root, &raw_regs, source_len); if (raw_regs.count > 0) { int defs_before = result->defs.count; cbm_extract_definitions(&pp_ctx); @@ -1571,7 +1607,7 @@ CBMFileResult *cbm_extract_file_ex(const char *source, int source_len, CBMLangua if (strcmp(ts_node_type(root), "ERROR") == 0) { cbm_error_regions_push(®s, root); /* whole file unparseable */ } else { - cbm_collect_error_regions(root, ®s); + cbm_collect_error_regions(root, ®s, source_len); } cbm_subtract_recovered_regions(®s, &result->defs); /* #1071: don't flag a benign function-like-macro call (defined in-file) diff --git a/scripts/ci/exclude-rescanned-selected-objects.sh b/scripts/ci/exclude-rescanned-selected-objects.sh new file mode 100755 index 000000000..22aabd2cf --- /dev/null +++ b/scripts/ci/exclude-rescanned-selected-objects.sh @@ -0,0 +1,101 @@ +#!/usr/bin/env bash +# Withhold the selected executables from the full-surface VirusTotal pass. +# +# WHY (measured, not assumed). The verify pass used to submit every extracted +# object, selected executables included, on the stated grounds that "VirusTotal +# is content-addressed, so identical bytes return the analysis it already holds +# instead of re-running 70+ engines". That is not what happens. On the v0.10.5 +# release all EIGHT re-submissions produced a NEW analysis: same VirusTotal +# file-id, a timestamp 47 minutes later. +# +# candidate: file-id=2c00f485... ts=1786795957 (12:12:37Z) +# verify : file-id=2c00f485... ts=1786798758 (12:59:18Z) +# +# Re-analysing identical bytes re-rolls a probabilistic classifier, and it +# answered differently in the same hour, in both directions: +# +# 82750cd1 (linux-amd64) microsoft-ml -> clean +# 6d3c5be6 (darwin-arm64) clean -> microsoft-ml +# +# The published release notes then cited the candidate verdict while the linked +# page showed the verify verdict, so the table said "clean" for a binary +# VirusTotal was flagging and vice versa. Nothing about the binaries differed: +# same sha256 in both scans. +# +# The second scan also proved nothing the first did not. Identity is already +# established by hash: verify-release-selection.py reconciles every published +# container to the selected bytes before this step runs, and checksums.txt binds +# the same digests publicly. A re-scan adds no assurance and one more roll. +# +# What still gets scanned is everything the candidate pass never saw: install.sh, +# install.ps1, LICENSE, THIRD_PARTY_NOTICES.md, the MCPB manifest.json and the +# unpacked UI assets. install.sh and install.ps1 are the highest-consequence +# non-executable bytes we publish - users pipe them straight into a shell - and +# that coverage is untouched. +# +# Fails closed: if nothing matches, the selection binding is broken; if nothing +# is left, the surface scan would silently become a no-op. +# +# Usage: exclude-rescanned-selected-objects.sh [manifest-out] +set -euo pipefail + +OBJECTS_DIR="${1:?usage: exclude-rescanned-selected-objects.sh [manifest-out]}" +SELECTION="${2:?usage: exclude-rescanned-selected-objects.sh [manifest-out]}" +MANIFEST="${3:-}" + +test -d "$OBJECTS_DIR" || { echo "error: no objects directory: $OBJECTS_DIR" >&2; exit 1; } +test -s "$SELECTION" || { echo "error: no selection evidence: $SELECTION" >&2; exit 1; } + +head -n 1 "$SELECTION" | grep -qx '# cbm-release-selection-v1' || { + echo "error: wrong evidence marker in $SELECTION" >&2; exit 1; } + +# selected_sha256 is the 4th column; skip the '#' metadata block and the header. +selected="$(mktemp)" +withheld="$(mktemp)" +trap 'rm -f "$selected" "$withheld"' EXIT +# `|| true` on the grep: with `set -o pipefail` a zero-match grep would abort the +# script here, before the explicit check below could say why. A guard that exits +# silently is not a guard. +awk -F '\t' '/^#/ {next} $1=="target" {next} {print $4}' "$SELECTION" \ + | { grep -E '^[0-9a-f]{64}$' || true; } | sort -u > "$selected" +if [ ! -s "$selected" ]; then + echo "error: no selected sha256 values in $SELECTION — the selection" >&2 + echo " evidence names no shipped bytes, so nothing can be matched." >&2 + exit 1 +fi + +kept=0 +for f in "$OBJECTS_DIR"/*; do + [ -f "$f" ] || continue + sha="$(sha256sum "$f" | awk '{print $1}')" + if grep -qx "$sha" "$selected"; then + printf '%s\t%s\n' "$sha" "$(basename "$f")" >> "$withheld" + rm -f -- "$f" + else + kept=$((kept + 1)) + fi +done + +count="$(wc -l < "$withheld" | tr -d ' ')" +if [ "$count" -eq 0 ]; then + echo "error: no extracted object matched a selected sha256 — the release" >&2 + echo " containers do not carry the bytes the selection recorded." >&2 + exit 1 +fi +if [ "$kept" -eq 0 ]; then + echo "error: every extracted object was withheld; the full-surface scan" >&2 + echo " would cover nothing." >&2 + exit 1 +fi + +if [ -n "$MANIFEST" ]; then + { + echo "# cbm-virustotal-withheld-v1" + echo "# reason=already-scanned-as-candidate" + echo "# evidence=virustotal-candidate-results.tsv" + printf 'sha256\tobject\n' + cat "$withheld" + } > "$MANIFEST" +fi + +echo "withheld $count already-scanned selected executable(s); $kept object(s) remain for the surface scan" diff --git a/tests/test_parse_coverage.c b/tests/test_parse_coverage.c index 7d3645ffb..1c0720c94 100644 --- a/tests/test_parse_coverage.c +++ b/tests/test_parse_coverage.c @@ -247,6 +247,128 @@ TEST(c_trailing_recovered_defs_keep_flag) { /* ── Suite ────────────────────────────────────────────────────────────────── */ +/* ── #1610: a missing FINAL NEWLINE is not a parse failure ──────────────────── + * + * A file that does not end with "\n" leaves the grammar's mandatory line + * terminator MISSING. That node is ZERO-WIDTH and sits at EOF: the parser + * consumed no source for it, so by construction nothing was dropped — no + * construct can live in a zero-byte span. Every instruction still parses. + * + * Reported on #1610 for Dockerfile, where a reporter proved with a byte-exact + * matrix that the trigger is independent of BOM, CRLF/LF, exec-form vs + * shell-form and file length — it is purely the absent final newline. + * + * It was never Dockerfile-specific: tcl, fish, gomod and hyprlang flag the same + * way, while ini, fsharp, beancount and others do NOT — only because those + * grammars declare the terminator token hidden rather than visible. Whether a + * user saw a phantom parse_partial came down to a grammar-authoring accident. + * + * The cost was not cosmetic: a phantom flag writes a "::missed" shadow + * row, and until #1609 that row removed the whole project from cross-repo + * linking, as source AND as target. */ +TEST(dockerfile_missing_final_newline_not_flagged_issue1610) { + const char *src = "FROM mcr.microsoft.com/dotnet/aspnet:8.0\n" + "ENTRYPOINT [\"dotnet\", \"App.dll\"]"; /* deliberately no \n */ + CBMFileResult *r = do_extract(src, CBM_LANG_DOCKERFILE, "Dockerfile"); + ASSERT_NOT_NULL(r); + bool flagged = r->parse_incomplete; + cbm_free_result(r); + if (flagged) { + FAIL("a Dockerfile lacking only its final newline must not be parse_partial"); + } + PASS(); +} + +/* The same bytes WITH the newline must stay clean — pins the equivalence the + * reporter's matrix proved, so a future change cannot "fix" one by breaking the + * other. */ +TEST(dockerfile_with_final_newline_still_clean_issue1610) { + const char *src = "FROM mcr.microsoft.com/dotnet/aspnet:8.0\n" + "ENTRYPOINT [\"dotnet\", \"App.dll\"]\n"; + CBMFileResult *r = do_extract(src, CBM_LANG_DOCKERFILE, "Dockerfile"); + ASSERT_NOT_NULL(r); + bool flagged = r->parse_incomplete; + cbm_free_result(r); + if (flagged) { + FAIL("a terminated Dockerfile must not be parse_partial"); + } + PASS(); +} + +/* Language-general, not a Dockerfile patch: these four were each proven to flag + * on a stripped trailing newline. */ +TEST(missing_final_newline_not_flagged_across_grammars_issue1610) { + struct { + const char *src; + CBMLanguage lang; + const char *path; + } cases[] = { + {"proc foo {} {}\nproc bar {} {}", CBM_LANG_TCL, "a.tcl"}, + {"function foo\n echo hi\nend", CBM_LANG_FISH, "a.fish"}, + {"module example.com/m\n\ngo 1.21", CBM_LANG_GOMOD, "go.mod"}, + {"general {\n gaps_in = 5\n}", CBM_LANG_HYPRLANG, "hypr.conf"}, + }; + for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) { + CBMFileResult *r = do_extract(cases[i].src, cases[i].lang, cases[i].path); + ASSERT_NOT_NULL(r); + bool flagged = r->parse_incomplete; + if (flagged) { + fprintf(stderr, " %s flagged: ranges=%s\n", cases[i].path, + r->error_ranges ? r->error_ranges : "(none)"); + } + cbm_free_result(r); + if (flagged) { + FAIL("an unterminated final line must not be parse_partial in any grammar"); + } + } + PASS(); +} + +/* GUARD (the reason this suppression is safe rather than convenient): the rule + * is ZERO-WIDTH AT EOF only. A real failure earlier in the file must still be + * reported, and its range must name the broken line — not be swallowed along + * with the terminator. */ +TEST(real_error_before_eof_still_flagged_without_final_newline_issue1610) { + /* Built from C_IFDEF_SPLIT, the fixture this suite already proves is + * flagged, with its trailing newline removed. Two conditions now hold at + * once: a genuine width-bearing ERROR mid-file, AND an unterminated last + * line. Suppressing the EOF terminator must not swallow the real one. */ + size_t n = strlen(C_IFDEF_SPLIT); + char *unterminated = (char *)malloc(n + 1); + ASSERT_NOT_NULL(unterminated); + memcpy(unterminated, C_IFDEF_SPLIT, n); + unterminated[n - 1] = '\0'; /* drop the final newline */ + + CBMFileResult *r = do_extract(unterminated, CBM_LANG_C, "split.c"); + free(unterminated); + ASSERT_NOT_NULL(r); + bool flagged = r->parse_incomplete; + bool has_ranges = r->error_ranges != NULL; + cbm_free_result(r); + if (!flagged) { + FAIL("a real mid-file parse failure must still be reported when the file also lacks its final newline"); + } + if (!has_ranges) { + FAIL("a reported failure must still name its line range"); + } + PASS(); +} + +/* GUARD: a MISSING/ERROR node WITH WIDTH at EOF is a genuine loss and must + * still be flagged. A Makefile whose final recipe line lacks its newline really + * does drop the recipe from the tree — cbm's flag is honest there. */ +TEST(width_bearing_error_at_eof_still_flagged_issue1610) { + const char *src = "all:\n\techo hi"; /* no trailing newline; recipe is lost */ + CBMFileResult *r = do_extract(src, CBM_LANG_MAKEFILE, "Makefile"); + ASSERT_NOT_NULL(r); + bool flagged = r->parse_incomplete; + cbm_free_result(r); + if (!flagged) { + FAIL("a width-bearing parse failure at EOF must still be reported"); + } + PASS(); +} + SUITE(parse_coverage) { RUN_TEST(c_ifdef_split_brace_sets_parse_incomplete); RUN_TEST(c_ifdef_split_brace_neighbors_still_extracted); @@ -257,4 +379,9 @@ SUITE(parse_coverage) { RUN_TEST(py_clean_file_not_flagged); RUN_TEST(error_region_cap_is_honored); RUN_TEST(c_trailing_recovered_defs_keep_flag); + RUN_TEST(dockerfile_missing_final_newline_not_flagged_issue1610); + RUN_TEST(dockerfile_with_final_newline_still_clean_issue1610); + RUN_TEST(missing_final_newline_not_flagged_across_grammars_issue1610); + RUN_TEST(real_error_before_eof_still_flagged_without_final_newline_issue1610); + RUN_TEST(width_bearing_error_at_eof_still_flagged_issue1610); }