From 006588d04a5bcaffc17f9763d4880051e1371ee9 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Tue, 8 Sep 2026 10:14:17 -0400 Subject: [PATCH 1/3] feat(xtest): 2.1 GiB ZIP64 boundary coverage (DSPX-4592) ZIP central-directory offsets and sizes are 32-bit unsigned on the wire. A reader that widens one with a signed read sees anything >= 2**31 as negative; at or above 2**32 the format mandates the ZIP64 sentinel, so the 32-bit field never holds a real value. That leaves exactly one broken window, [2**31, 2**32), and nothing in this suite reached it before now. Adds sizes.MEDIUM_BYTES (2.1 GiB, calibrated to land a TDFs manifest offset inside the window) and the window predicates, zipinspect.py (a raw ZIP central-directory reader independent of zipfile, which normalises ZIP64 away), test_zip64.py (the roundtrip cell, deselected unless the session reaches the window), a strict xfail scoped to pre-fix java readers, and a nightly-only zip64 CI job matrixed over the encrypting SDK. --- .github/workflows/check.yml | 13 +- .github/workflows/xtest.yml | 368 +++++++++++++++++++++++++++++- spec/DSPX-4592.md | 352 +++++++++++++++++++++++++++++ xtest/AGENTS.md | 4 +- xtest/conftest.py | 56 +++-- xtest/pyproject.toml | 2 + xtest/sizes.py | 65 +++++- xtest/tdfs.py | 43 +++- xtest/test_sizes_units.py | 40 ++++ xtest/test_tdfs_units.py | 43 +++- xtest/test_zip64.py | 142 ++++++++++++ xtest/test_zip64_units.py | 434 ++++++++++++++++++++++++++++++++++++ xtest/zipinspect.py | 350 +++++++++++++++++++++++++++++ 13 files changed, 1879 insertions(+), 33 deletions(-) create mode 100644 spec/DSPX-4592.md create mode 100644 xtest/test_zip64.py create mode 100644 xtest/test_zip64_units.py create mode 100644 xtest/zipinspect.py diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 0c6fc424..a77faeb0 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -36,9 +36,14 @@ jobs: working-directory: xtest # Offline tests for the harnesses whose own correctness gates a nightly # job: the benchmark statistics, measurement and CLI command builders, - # and the encryption fixture cache. No platform and no SDK builds - # required, so the part that has to be *correct* is checked on every PR - # rather than only when the nightly runs. + # the encryption fixture cache, and the ZIP64 central-directory parser. + # No platform and no SDK builds required, so the part that has to be + # *correct* is checked on every PR rather than only when the nightly runs. + # + # test_zip64_units.py matters disproportionately here: the nightly zip64 + # job's verdict is only as good as this parser, and a parser bug would + # report a conformant container as broken (or the reverse) after an hour + # of multi-GiB IO that nobody wants to repeat to debug it. # # --frozen --no-build: resolve nothing and build nothing, so a # dependency cannot slip in an unlocked version or a setup script on a @@ -48,7 +53,7 @@ jobs: uv run --frozen --no-build pytest --no-header -q test_bench_stats.py test_bench_measure.py test_bench_runner.py test_bench_arms.py test_sdk_commands.py test_tdfs_units.py - test_encryption_units.py test_sizes_units.py + test_encryption_units.py test_sizes_units.py test_zip64_units.py working-directory: xtest - name: Lint and test otdf-local run: | diff --git a/.github/workflows/xtest.yml b/.github/workflows/xtest.yml index db38c76d..ebbda99f 100644 --- a/.github/workflows/xtest.yml +++ b/.github/workflows/xtest.yml @@ -38,11 +38,16 @@ on: type: boolean default: false description: "Run the SDK performance regression benchmarks (adds ~45m per SDK). Needs two builds per SDK: set the *-ref inputs to 'main latest', since a bare 'main' installs no release to use as a baseline and every cell will skip." + run-zip64: + required: false + type: boolean + default: false + description: "Run the 2.1 GiB ZIP64 boundary tests (DSPX-4592; adds ~60m per encrypting SDK). Set java-ref to 'main latest' to also exercise the pre-fix java reader, which is the defect this covers." force-supports: required: false type: string default: "" - description: "Comma-separated feature names to treat as supported regardless of what each SDK's `cli.sh supports` reports. Use when evaluating a fix that has not been released yet: the version gates live in this repo and answer 'no' for exactly those unreleased builds, so the cells would otherwise skip. An unknown name fails the run rather than being ignored." + description: "Comma-separated feature names to treat as supported regardless of what each SDK's `cli.sh supports` reports (e.g. 'chunky'). Use when evaluating a fix that has not been released yet: the version gates live in this repo and answer 'no' for exactly those unreleased builds, so the cells would otherwise skip. An unknown name fails the run rather than being ignored." workflow_call: inputs: platform-ref: @@ -73,6 +78,10 @@ on: required: false type: boolean default: false + run-zip64: + required: false + type: boolean + default: false force-supports: required: false type: string @@ -101,6 +110,7 @@ jobs: contents: read outputs: platform-tag-to-sha: ${{ steps.version-info.outputs.platform-tag-to-sha }} + platform-main-sha: ${{ steps.version-info.outputs.platform-main-sha }} platform-tag-list: ${{ steps.version-info.outputs.platform-tag-list }} heads: ${{ steps.version-info.outputs.platform-heads }} default-tags: ${{ steps.version-info.outputs.default-tags }} @@ -201,6 +211,27 @@ jobs: } } + // Bench and ZIP64 hold the server on platform main independently + // of the platform lanes under test. Resolve that moving ref once + // here so every matrix job uses the same commit. Reuse the normal + // resolution when main was already requested; otherwise look up + // the branch without adding it to platform-tag-list. + let platformMainSha = versionData.platform + ?.find(({ tag, sha, err }) => tag === 'main' && sha && !err) + ?.sha; + if (!platformMainSha) { + const { data: platformMain } = await github.rest.repos.getBranch({ + owner: 'opentdf', + repo: 'platform', + branch: 'main' + }); + platformMainSha = platformMain.commit.sha; + } + if (!platformMainSha) { + throw new Error('Unable to resolve opentdf/platform main'); + } + core.setOutput('platform-main-sha', platformMainSha); + core.setOutput('all', JSON.stringify(versionData)); const sdkVersionList = []; @@ -824,7 +855,7 @@ jobs: id: run-platform uses: opentdf/platform/test/start-up-with-containers@18b8070f7ae1e3547234342f42d0d686dc77788f # keycloak-26.4 (opentdf/platform#3792) with: - platform-ref: ${{ fromJSON(needs.resolve-versions.outputs.platform-tag-to-sha)['main'] }} + platform-ref: ${{ needs.resolve-versions.outputs.platform-main-sha }} bootstrap-ref: main ec-tdf-enabled: true extra-keys: ${{ steps.load-extra-keys.outputs.EXTRA_KEYS }} @@ -946,7 +977,7 @@ jobs: done env: java_version_info: ${{ needs.resolve-versions.outputs.java }} - platform_ref: ${{ fromJSON(needs.resolve-versions.outputs.platform-tag-to-sha)['main'] }} + platform_ref: ${{ needs.resolve-versions.outputs.platform-main-sha }} - name: Build the ${{ matrix.sdk }} cli if: fromJson(steps.configure-sdk.outputs.heads)[0] != null @@ -1023,6 +1054,337 @@ jobs: path: ${{ steps.run-platform.outputs.platform-log-file }} if-no-files-found: ignore + # ZIP64 boundary conformance at 2.1 GiB (DSPX-4592). + # + # ZIP central-directory offsets and sizes are 32-bit *unsigned* on the wire. + # A reader that widens one with a signed read sees anything at or above 2**31 + # as negative; at or above 2**32 the format mandates the ZIP64 sentinel, so + # the 32-bit field never holds a real value. That leaves exactly one broken + # window, [2**31, 2**32), and nothing in this suite reached it: `--large` is + # 5 GiB, which steps straight over. + # + # So this job runs one size, `medium` (2.1 GiB), and only that size. Anything + # smaller does not make it cheaper, it makes it vacuous -- + # test_zip64.py asserts that an offset actually landed in the window rather + # than skipping, precisely so a mis-sized payload fails instead of passing. + # + # Never on pull requests: an hour of multi-GiB IO per SDK is not a PR gate. + zip64: + timeout-minutes: 90 + runs-on: ubuntu-latest + needs: resolve-versions + # Nightly cron only, matching bench. The Mon/Wed and weekly crons would + # re-run an identical comparison. + if: >- + github.event.schedule == '30 6 * * *' || + ((github.event_name == 'workflow_dispatch' || github.event_name == 'workflow_call') + && inputs.run-zip64) + permissions: + contents: read + packages: read + strategy: + # One runner per *encrypting* SDK; each decrypts with all three. Every + # runner therefore installs every SDK, and the split is about disk and + # wall clock rather than about what is installed: one encryptor per + # runner means at most two cached 2.1 GiB ciphertexts (one per + # negotiated target_mode), not three. + fail-fast: false + matrix: + sdk: [go, java, js] + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + repository: opentdf/tests + path: otdftests + persist-credentials: false + + # Plaintext 2.1 GiB + one cached ciphertext 2.1 GiB + one decrypt output + # at a time 2.1 GiB is ~6.5 GiB, which the workspace volume cannot be + # relied on to hold alongside three SDK toolchains. /mnt is the runner's + # large ephemeral disk; XT_TMP_DIR moves the fixtures there. + - name: Reclaim disk and stage a scratch volume + id: scratch + run: |- + # Toolchains this job does not use. Removing them buys ~25 GiB on the + # workspace volume, which the platform containers and three SDK + # builds still have to share. + sudo rm -rf /usr/share/dotnet /usr/local/lib/android /opt/ghc \ + /usr/local/share/boost || true + sudo mkdir -p /mnt/xtest-tmp + sudo chown "$(id -u):$(id -g)" /mnt/xtest-tmp + df -h / /mnt + + - name: load extra keys from file + id: load-extra-keys + run: |- + echo "EXTRA_KEYS=$(jq -c > "${GITHUB_OUTPUT}" + + ######## SPIN UP PLATFORM BACKEND ############# + # Pinned to main with the default KAS only. This job is about the ZIP + # container the SDKs write and read; the six extra KAS instances the ABAC + # tests need would only consume runner memory and disk. + - name: Check out and start up platform with deps/containers + id: run-platform + uses: opentdf/platform/test/start-up-with-containers@18b8070f7ae1e3547234342f42d0d686dc77788f # keycloak-26.4 (opentdf/platform#3792) + with: + platform-ref: ${{ needs.resolve-versions.outputs.platform-main-sha }} + bootstrap-ref: main + ec-tdf-enabled: true + extra-keys: ${{ steps.load-extra-keys.outputs.EXTRA_KEYS }} + log-type: json + pqc-enabled: true + + - name: Install uv + uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 + - uses: bufbuild/buf-action@fd21066df7214747548607aaa45548ba2b9bc1ff # v1.4.0 + with: + setup_only: true + token: ${{ secrets.BUF_TOKEN }} + version: "1.56.0" + + # All three toolchains unconditionally: every runner decrypts with every + # SDK, so every runner builds all three. + - name: Set up JDK + uses: actions/setup-java@ad2b38190b15e4d6bdf0c97fb4fca8412226d287 + with: + java-version: "11" + distribution: "adopt" + server-id: github + + - name: Set up Node 22 + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e + with: + node-version: "22.x" + + - name: Capture platform otdfctl location + id: platform-otdfctl + run: |- + if [ -d "$PLATFORM_DIR/otdfctl" ] && [ -f "$PLATFORM_DIR/otdfctl/go.mod" ]; then + echo "dir=$(pwd)/$PLATFORM_DIR/otdfctl" >> "$GITHUB_OUTPUT" + sha=$(git -C "$PLATFORM_DIR" rev-parse HEAD) || { + echo "::error::Failed to get SHA from platform checkout at $PLATFORM_DIR" + exit 1 + } + echo "sha=$sha" >> "$GITHUB_OUTPUT" + else + echo "dir=" >> "$GITHUB_OUTPUT" + echo "sha=" >> "$GITHUB_OUTPUT" + fi + env: + PLATFORM_DIR: ${{ steps.run-platform.outputs.platform-working-dir }} + + ######## INSTALL EVERY SDK ############# + # The go install doubles as otdfctl, which conftest.py loads at import + # time to provision attributes and the KAS registry. + - name: Configure go sdk + id: configure-go + uses: ./otdftests/xtest/setup-cli-tool + with: + path: otdftests/xtest/sdk + sdk: go + version-info: "${{ needs.resolve-versions.outputs.go }}" + platform-otdfctl-dir: ${{ steps.platform-otdfctl.outputs.dir }} + platform-otdfctl-sha: ${{ steps.platform-otdfctl.outputs.sha }} + + - name: Configure java sdk + id: configure-java + uses: ./otdftests/xtest/setup-cli-tool + with: + path: otdftests/xtest/sdk + sdk: java + version-info: "${{ needs.resolve-versions.outputs.java }}" + platform-otdfctl-dir: ${{ steps.platform-otdfctl.outputs.dir }} + platform-otdfctl-sha: ${{ steps.platform-otdfctl.outputs.sha }} + + - name: Configure js sdk + id: configure-js + uses: ./otdftests/xtest/setup-cli-tool + with: + path: otdftests/xtest/sdk + sdk: js + version-info: "${{ needs.resolve-versions.outputs.js }}" + platform-otdfctl-dir: ${{ steps.platform-otdfctl.outputs.dir }} + platform-otdfctl-sha: ${{ steps.platform-otdfctl.outputs.sha }} + + - name: Cache Go modules + uses: actions/cache@2c8a9bd7457de244a408f35966fab2fb45fda9c8 # v6.0.0 + with: + path: | + ~/.cache/go-build + ~/go/pkg/mod + key: go-${{ runner.os }}-${{ hashFiles('otdftests/xtest/sdk/go/src/*/go.sum') }} + restore-keys: | + go-${{ runner.os }}- + + - name: Cache npm + uses: actions/cache@2c8a9bd7457de244a408f35966fab2fb45fda9c8 # v6.0.0 + with: + path: ~/.npm + key: npm-${{ runner.os }}-${{ hashFiles('otdftests/xtest/sdk/js/src/**/package-lock.json') }} + restore-keys: | + npm-${{ runner.os }}- + + - name: Cache Maven repository + uses: actions/cache@2c8a9bd7457de244a408f35966fab2fb45fda9c8 # v6.0.0 + with: + path: ~/.m2/repository + key: maven-${{ runner.os }}-${{ hashFiles('otdftests/xtest/sdk/java/src/**/pom.xml') }} + restore-keys: | + maven-${{ runner.os }}- + + - name: point java heads at the platform under test + if: fromJson(steps.configure-java.outputs.heads)[0] != null + run: |- + for row in $(echo "$java_version_info" | jq -c '.[]'); do + TAG=$(echo "$row" | jq -r '.tag') + HEAD=$(echo "$row" | jq -r '.head') + if [[ "$HEAD" == "true" ]]; then + echo "PLATFORM_BRANCH=$platform_ref" > "otdftests/xtest/sdk/java/${TAG}.env" + fi + done + env: + java_version_info: ${{ needs.resolve-versions.outputs.java }} + platform_ref: ${{ needs.resolve-versions.outputs.platform-main-sha }} + + - name: Build the go cli + if: fromJson(steps.configure-go.outputs.heads)[0] != null + run: make + working-directory: otdftests/xtest/sdk/go + env: + BUF_INPUT_HTTPS_USERNAME: opentdf-bot + BUF_INPUT_HTTPS_PASSWORD: ${{ secrets.PERSONAL_ACCESS_TOKEN_OPENTDF }} + + - name: Build the java cli + if: fromJson(steps.configure-java.outputs.heads)[0] != null + run: make + working-directory: otdftests/xtest/sdk/java + env: + BUF_INPUT_HTTPS_USERNAME: opentdf-bot + BUF_INPUT_HTTPS_PASSWORD: ${{ secrets.PERSONAL_ACCESS_TOKEN_OPENTDF }} + + - name: Build the js cli + if: fromJson(steps.configure-js.outputs.heads)[0] != null + run: make + working-directory: otdftests/xtest/sdk/js + env: + BUF_INPUT_HTTPS_USERNAME: opentdf-bot + BUF_INPUT_HTTPS_PASSWORD: ${{ secrets.PERSONAL_ACCESS_TOKEN_OPENTDF }} + + ######## RUN ############# + - name: Install test dependencies + run: uv sync --locked --no-build + working-directory: otdftests/xtest + + # Deliberately serial: no -n / --dist. Three concurrent workers each + # holding a 2.1 GiB plaintext, ciphertext and decrypt output would + # exhaust the scratch volume long before the last cell. + # + # Note what is *not* here: --skip-released-pairs. The xct job derives + # that flag from SKIP_RELEASED_PAIRS to avoid re-testing release-against- + # release combinations, but a released java decryptor reading a 2.1 GiB + # container is the exact defect under test. Skipping it would leave this + # job green and blind. + - name: Run ZIP64 boundary tests + id: zip64 + run: |- + uv run --frozen --no-build pytest -ra -v \ + --sizes medium \ + --sdks-encrypt "$ZIP64_SDK" \ + --sdks-decrypt "go java js" \ + --junitxml "test-results/zip64-${ZIP64_SDK}.xml" \ + --html "test-results/zip64-${ZIP64_SDK}.html" \ + --self-contained-html \ + test_zip64.py + working-directory: otdftests/xtest + env: + ZIP64_SDK: ${{ matrix.sdk }} + PLATFORM_DIR: "../../${{ steps.run-platform.outputs.platform-working-dir }}" + SCHEMA_FILE: "manifest.schema.json" + PLATFORM_TAG: main + # go's heads: conftest reads this to locate otdfctl under + # sdk/go/dist//, regardless of which SDK is encrypting. + OTDFCTL_HEADS: ${{ steps.configure-go.outputs.heads }} + # Nothing here requests the audit-log fixture, and a rewrap audit + # event says nothing about the container's ZIP encoding. + DISABLE_AUDIT_ASSERTIONS: "1" + # Multi-GiB fixtures go on the runner's large ephemeral volume, not + # the workspace disk. + XT_TMP_DIR: /mnt/xtest-tmp + # The java shim pipes CLI stdout to a file with no -Xmx, and the js + # shim is a bare npx. Both runtimes honour these automatically, so + # the headroom is set here rather than by editing the shims. If a + # shim turns out to buffer the whole payload rather than stream it, + # that is a finding for the SDK, not a reason to shrink the payload. + JAVA_TOOL_OPTIONS: -Xmx6g + NODE_OPTIONS: --max-old-space-size=8192 + + # A green job whose tests were all skipped is the exact failure mode this + # ticket exists to close, and it is invisible in the job status. Parse + # the junit XML rather than grepping the log: a skipped cell and a cell + # that printed the word "skipped" are different things. + - name: Confirm the ZIP64 cells actually ran + if: success() || failure() + working-directory: otdftests/xtest + env: + ZIP64_SDK: ${{ matrix.sdk }} + run: |- + python3 - <<'PY' + import os + import sys + import xml.etree.ElementTree as ET + + sdk = os.environ["ZIP64_SDK"] + path = f"test-results/zip64-{sdk}.xml" + try: + cases = ET.parse(path).getroot().iter("testcase") + except (OSError, ET.ParseError) as e: + sys.exit(f"::error::cannot read {path}: {e}") + + ran, xfailed, skipped = [], [], [] + for c in cases: + name = f"{c.get('classname')}::{c.get('name')}" + s = c.find("skipped") + if s is None: + ran.append(name) + # pytest files xfail under , but an xfailed cell did + # encrypt 2.1 GiB and did attempt the decrypt -- it exercised the + # defect and predicted the outcome. That is coverage, not a gap. + elif s.get("type") == "pytest.xfail": + xfailed.append(name) + else: + skipped.append(name) + + print(f"{len(ran)} ran, {len(xfailed)} xfailed, {len(skipped)} skipped") + for n in xfailed: + print(f" xfail: {n}") + for n in skipped: + print(f" skipped: {n}") + + if not ran and not xfailed: + sys.exit( + "::error::no ZIP64 cell executed. The job is green because " + "nothing ran, not because the 2-4 GiB band is conformant." + ) + PY + + - name: Upload ZIP64 results + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: success() || failure() + with: + name: ${{ job.status == 'success' && '✅' || '❌' }} zip64-${{ matrix.sdk }} + path: | + otdftests/xtest/test-results/*.xml + otdftests/xtest/test-results/*.html + if-no-files-found: warn + + - name: Upload server logs on failure + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: failure() + with: + name: zip64-server-logs-${{ matrix.sdk }} + path: ${{ steps.run-platform.outputs.platform-log-file }} + if-no-files-found: ignore + publish-results: runs-on: ubuntu-latest needs: xct diff --git a/spec/DSPX-4592.md b/spec/DSPX-4592.md new file mode 100644 index 00000000..9db9efdf --- /dev/null +++ b/spec/DSPX-4592.md @@ -0,0 +1,352 @@ +--- +ticket: DSPX-4592 +title: zip64 conformance issues: xtest 2.1 GiB e2e coverage +status: draft +authors: [dmihalcik@virtru.com] +branches: [opentdf/tests:DSPX-4592-java-underflow] +prs: [] +created: 2026-09-02 +updated: 2026-09-02 +--- + +# zip64 conformance issues: xtest 2.1 GiB e2e coverage + +## Summary +BackgroundSibling tickets track ZIP64/APPNOTE conformance defects in the three SDKs' hand-rolled ZIP readers and writers, found while auditing java-sdk PR #393. This ticket owns the shared cross-SDK test coverage those three all reference. +The defect class only manifests for payloads in the 2-4 GiB band, and no test in any repo currently covers that band. +Why 2.1 GiB specificallyThe ZIP central directory stores offsets and sizes in 32-bit fields that are unsigned on the wire. A reader that widens them with a signed read sees anything at or above 2^31 (2147483648) as negative. Above 2^32 the format requires the ZIP64 sentinel plus an extra field, so the 32-bit field is never populated with a real value and the bug cannot fire. +That leaves exactly one broken window: 2^31 <= value < 2^32. 2.1 GiB (2254857830 bytes) sits ~107 MB inside the low edge of it, which is enough margin that segment padding and manifest size cannot push the interesting offsets back below the threshold. +What a 2.1 GiB payload actually exercisesjava-sdk writer, pre-#393. ZipWriter.writeByteArray hardcoded fileInfo.isZip64 = false. The manifest is written after the payload, so at a 2.1 GiB payload its local-header offset is ~2.1 GiB and gets written as a raw 32-bit value with no ZIP64 extra field. PR #393 fixes this via isZip64 = needsZip64(startPosition, data.length). Note the payload entry itself was never affected - stream() always set isZip64 = true - which is precisely why this went unnoticed. +java-sdk reader, pre-#393. readInt() sign-extends, so that ~2.1 GiB manifest offset comes back negative and the read fails or seeks to nonsense. Fixed by readUnsignedInt() in #393. +go-sdk writer, today. Switches to ZIP64 only at 4 GiB, so it writes both a 32-bit manifest offset and a 32-bit 2.1 GiB payload size. Any pre-#393 Java reader chokes on both. This is the live cross-SDK break and the reason the sibling go ticket moves the threshold to MaxInt32. +web-sdk. Always ZIP64, so it should pass every cell unchanged - a useful control. +The gap in current coveragextest/conftest.py already has a large-file path, but it steps straight over the broken band: +parser.addoption( + "--large", + action="store_true", + help="generate a large (greater than 4 GiB) file for testing", +) +... +length = (5 * 2**30) if size == "large" else 128size is parametrized binary - 128 bytes or 5 GiB (pytest_generate_tests, pt_file). 5 GiB is above 2^32, so every SDK takes the full ZIP64 path and none of the 32-bit fields are populated with real values. The one size that would catch this class of bug is the one size not tested. +Separately: --large has no CI wiring at all. It does not appear anywhere in .github/workflows/xtest.yml, so even the 5 GiB path only ever runs when someone passes the flag locally. +Work1. Make size a real parametrizationReplace the boolean --large with something that can express the band - e.g. --sizes small,medium,large mapping to 128 B / 2.1 GiB / 5 GiB, keeping --large as a deprecated alias for small,large so existing invocations do not break. Update the pt_file fixture and its docstring accordingly (the current docstring says 'large' (>4 GiB) or 'small' (128 bytes)). +2. Fix plaintext generation for multi-GiB sizespt_file currently generates content with a Python loop, one formatted line per 16 bytes: +for i in range(0, length, 16): + f.write(f"{i:15,d}\n")At 2.1 GiB that is ~140 million iterations of string formatting, which will dominate the job's runtime. Generate large files in bulk (build one block, write it repeatedly) while keeping the content deterministic and non-compressible enough to stay realistic. This matters more than it sounds - if the fixture takes 20 minutes the test will get disabled. +3. Cover the pre-fix reader, not just current mainThe Java reader bug is in released artifacts; main after #393 will pass. The existing --sdks-decrypt version-qualified spec (e.g. java@v0.7.x) is the mechanism - make sure the matrix pins at least one released java-sdk as a decryptor against a 2.1 GiB TDF written by go and by java, so the test actually reproduces the failure today and turns green only once the sibling tickets land. Check the interaction with --skip-released-pairs / SKIP_RELEASED_PAIRS so these pairs are not silently skipped - per the repo's own guidance, confirm the test ran and was not SKIPPED rather than trusting a green check. +4. CI wiringDo not add this to the per-PR path. Budget: a 2.1 GiB round trip is ~2.1 GiB plaintext + ~2.1 GiB ciphertext + ~2.1 GiB decrypted output per cell, and ubuntu-latest has limited free disk. Recommended shape: +A separate job (or separate workflow) with its own timeout-minutes, not folded into the existing 60-minute matrix job. +Attach to one of the existing cron schedules (30 6 * * * nightly, or the Sunday 0 18 * * 0 weekly) plus workflow_dispatch so it can be run on demand against a branch. +Restrict the SDK matrix to the cells that matter rather than the full cross product; clean up artifacts between cells. +If disk turns out to be the binding constraint, note it here and consider a larger runner rather than shrinking the payload below 2^31 - a smaller payload silently stops testing anything. +Acceptance criteriaA 2.1 GiB size is expressible and runs end-to-end through encrypt/decrypt across the go, java, and web SDKs. +Large-file plaintext generation is fast enough not to dominate job runtime. +At least one released (pre-#393) java-sdk is exercised as a decryptor for a 2.1 GiB TDF and is confirmed to reproduce the failure before the sibling fixes land. +The new coverage runs on a schedule and via workflow_dispatch, not on every PR, with an explicit timeout. +Test is confirmed to actually execute (not SKIPPED) by grepping the job log. +Sibling tickets' "Shared cross-SDK work" sections are satisfied by this ticket. +SequencingThis should land before the go and java fixes, so it goes red first and demonstrates the fixes work. It is not blocked by them. +Filed as a Task rather than a Bug: this is test coverage for defects tracked in the three sibling tickets, not a defect in its own right. + +## Problem / Motivation + +A TDF is a ZIP container, and all three SDKs write and read that ZIP by hand. +The central directory stores each entry's local-header offset and sizes in +32-bit fields that are **unsigned on the wire**. Three regimes follow, and only +the middle one is dangerous: + +| payload | 32-bit field holds | signed read | ZIP64 required? | +|---|---|---|---| +| `< 2**31` | the real value | correct | no | +| `[2**31, 2**32)` | the real value, **or** the ZIP64 sentinel | **negative** | no — writer's choice | +| `>= 2**32` | always the `0xFFFFFFFF` sentinel | n/a | yes | + +Below `2**31` a signed widen is harmless. At or above `2**32` the format +mandates the sentinel, so the 32-bit field never carries a real value and a +sign-extending reader never sees one. The defect class fires in exactly one +window, and it is the window nobody tested. + +The suite already had a large-file path — `--large` — and it made the gap +worse rather than better: it generates 5 GiB, which is *above* `2**32`, so +every SDK takes the full ZIP64 path and every 32-bit field holds the sentinel. +The one size that would catch this is the one size the suite could not express. +`--large` also had no CI wiring anywhere in `.github/workflows/`, so even the +5 GiB path only ran when someone remembered the flag locally. + +Concretely, today: go-sdk switches to ZIP64 only at 4 GiB, so at 2.1 GiB it +writes a real 32-bit manifest offset; a pre-#393 java-sdk reads it with +`readInt()`, gets a negative number, and fails. That is a live cross-SDK +interop break with no test that can see it. + +## Proposed Solution + +Four pieces, all in `opentdf/tests`. + +**A size vocabulary (`xtest/sizes.py`).** `small` = 128 B, `medium` = +2 254 857 830 B (2.1 GiB), `large` = 5 GiB, plus the window constants and an +`in_zip64_window()` predicate. One module so the byte counts and the "is this +in the broken band" question have a single home, importable from both +`conftest.py` and the tests without a cycle. `medium` sits ~107 MB inside the +low edge, enough margin that segment padding and manifest size cannot push the +manifest's local-header offset back under `2**31`. + +**`--sizes` replaces `--large` (`xtest/conftest.py`).** A comma-separated list +validated against the table, defaulting to `small`; `--large` survives as a +deprecated alias for `small,large` so existing local invocations keep working. +`size` stays a session-scoped parametrization, so listing more than one size +fans out every test that takes `pt_file` — which is the expressibility this +ticket asks for, and the reason CI passes exactly one. + +Plaintext generation splits by size. `small` keeps the existing line generator +byte-for-byte, because existing tests compare against that content. Anything +larger builds one 1 MiB pseudorandom block and writes it repeatedly with an +8-byte big-endian block counter patched into each write, so the content stays +deterministic *and* position-dependent without ever materialising the payload +in memory. DEFLATE's window is 32 KiB, so the repetition does not make the +payload compressible. Measured: 2.1 GiB in 0.5 s, deflate ratio 1.000. + +**Structural conformance assertions (`xtest/zipinspect.py`).** A roundtrip +alone reports "decrypt failed" and leaves you guessing which side was wrong, an +hour and 6 GiB of IO after the fact. This is a raw central-directory reader +that keeps the 32-bit fields *and* the resolved values side by side, so a test +can say which SDK wrote a non-conformant container independently of whether any +reader coped. `zipfile` cannot be used: it normalises ZIP64 away, discarding +precisely the encoding under test. + +**A dedicated module and a nightly job (`xtest/test_zip64.py`, the `zip64` job +in `xtest.yml`).** Separate from `test_tdfs.py` because it needs three things +that test does not: explicit deletion of the decrypt output so artifacts do not +accumulate 2.1 GiB at a time, the structural assertions, and selectability in +CI without dragging the rest of the suite to multi-GiB payloads. + +Known-broken cells use `xfail(strict=True)` keyed on `SDK.semver()` rather than +a permanently red job or a skip. A pre-fix java decryptor reports XFAIL with a +reason; when the fix ships, `latest` stops matching the predicate and the cell +must pass; and if a cell believed broken starts passing, strict xfail fails the +job so somebody comes and deletes the predicate. Self-maintaining, no follow-up +PR. + +## Inputs / Outputs / Contracts + +``` +--sizes small,medium,large # default: small +--large # deprecated alias for --sizes small,large +XT_TMP_DIR= # relocate fixtures off the workspace volume +``` + +`--large` and `--sizes` together is a `UsageError`, not a silent precedence +rule. + +```python +# xtest/sizes.py +SIZES: dict[str, int] # name -> bytes +SIZE_ORDER: tuple[str, ...] # cheapest first +ZIP64_WINDOW_LOW = 2**31 +ZIP64_WINDOW_HIGH = 2**32 +def in_zip64_window(n: int) -> bool +def exercises_zip64_window(size: str) -> bool + +# xtest/zipinspect.py +@dataclass(frozen=True) +class CentralDirectoryEntry: + name: str + raw_compressed_size / raw_uncompressed_size / raw_local_header_offset: int + compressed_size / uncompressed_size / local_header_offset: int + has_zip64_extra: bool + uses_zip64_for_offset / uses_zip64_for_sizes: bool # properties + def signed_read_of_offset(self) -> int # reproduces the defect + +def central_directory(path: Path) -> list[CentralDirectoryEntry] +def entries_in_window(entries) -> list[CentralDirectoryEntry] +def assert_zip64_above_4gib(entries) -> None +def describe(entries) -> str + +# xtest/tdfs.py +JAVA_ZIP64_READER_FIX: tuple[int, int, int] +def zip64_reader_xfail(decrypt_sdk: SDK) -> pytest.MarkDecorator | None +``` + +Marker: `zip64`, deselected (not skipped) unless the session's resolved sizes +reach `ZIP64_WINDOW_LOW`. Deselected, because a multi-GiB roundtrip has no +business in the PR matrix and a skip would report it as a test that exists and +was declined. + +CI: `run-zip64` boolean on `workflow_dispatch` and `workflow_call`; the job +also fires on the nightly `30 6 * * *` cron. Never on `pull_request`. + +## Edge Cases & Constraints + +**A vacuous pass is the failure mode to design against.** If the fixture +generates the wrong size, every assertion here passes without touching a line +of the code under test — which is exactly the hole this ticket exists to close. +So `test_zip64.py` *asserts* that some entry's local-header offset landed at or +above `2**31` rather than skipping when it did not, and the nightly job parses +its own junit XML and fails if no cell executed. Shrinking `medium` below +`2**31` does not make the test cheaper, it makes it silently meaningless. + +**Both encodings are legal in the window.** A writer may emit a real unsigned +32-bit value or opt into the ZIP64 sentinel; APPNOTE permits both, and which +one go-sdk emits is what the sibling ticket changes. So the test records and +logs which was chosen and asserts only on the `>= 2**32` case, where there is +no latitude. + +**Writer checks run outside the reader's xfail.** A writer regression must not +hide behind a known reader bug: assertions under an xfail marker report XFAIL +and nobody looks. `add_marker` is therefore applied after the structural +assertions and immediately before the decrypt. + +**Disk, not CPU, is binding.** `fixtures/encryption.py` caches ciphertexts +session-wide and never deletes them, and `rt_file()` outputs were never deleted +either. One encryptor per runner plus `rt_file.unlink()` in a `finally` gives +~6.5 GiB peak; without both it is 15 GiB+ and the runner fails. `XT_TMP_DIR` +puts that on the runner's large ephemeral volume. If disk still binds, take a +larger runner. + +**JVM and node heap.** The java shim pipes CLI stdout to a file with no `-Xmx` +and the js shim is a bare `npx`. If either buffers the payload rather than +streaming it, 2.1 GiB will OOM. The job sets `JAVA_TOOL_OPTIONS=-Xmx6g` and +`NODE_OPTIONS=--max-old-space-size=8192`, which both runtimes honour without a +shim edit. A shim that buffers unconditionally is a finding for the sibling +tickets, not a reason to shrink the payload. + +**`--skip-released-pairs` is deliberately not passed.** The `xct` job derives +it from `SKIP_RELEASED_PAIRS` to avoid re-testing release-against-release +combinations; here a released java decryptor is the entire point. + +## Out of Scope + +- The fixes themselves. go-sdk's 4 GiB → `MaxInt32` threshold change and + java-sdk #393 are the sibling tickets; this ticket only has to make them + demonstrable. +- Sizes above `2**32`. `large` still exists and still works, but nothing new + is asserted about it beyond the existing mandatory-ZIP64 check. +- Non-ZTDF containers. `nano` has its own framing and none of this applies. +- Making the multi-GiB path fast enough for the PR gate. It is a nightly. +- Fixing the segment-size defaulting itself. The `chunky` cell added here + demonstrates it; DSPX-4589 finding 4 and DSPX-4590 finding 7 fix it. + +## Acceptance Criteria + +Verified offline (no platform required): + +- [x] A 2.1 GiB size is expressible: `--sizes medium` parses, parametrizes + `size` session-wide, and selects the `zip64` cells; the default session + still resolves to `small` and deselects them. +- [x] Multi-GiB plaintext generation does not dominate job runtime — + 2.1 GiB in 0.5 s (4.4 GiB/s), byte-identical across regenerations, + deflate ratio 1.000 over the first 64 MiB. +- [x] The container's ZIP encoding is asserted on directly, so a failure + names the SDK at fault instead of reporting "decrypt failed". +- [x] `zipinspect`'s own unit tests (18) run offline on every PR via + `check.yml`, so the nightly's verdict does not rest on an unverified + parser. +- [x] The coverage runs on the nightly cron and via `workflow_dispatch` + (`run-zip64`), never on a PR, with its own `timeout-minutes: 90`. +- [x] Execution is confirmed by parsing the junit XML, not by grepping the + log; the job fails if no cell executed. Verified against a sample + report, including that `xfail` is counted as executed and a plain + `skip` is not. +- [x] The multi-segment defect the 2.1 GiB run turned up is covered on the + **PR gate**, not only the nightly: `chunky` (5 MiB) is a `feature_type` + and a size, and `test_chunky_roundtrip` runs in the standard `xct` job + with no CI change. It costs one 5 MiB encrypt and decrypt per pair; the + fixture generates in 0.05 s. + +Verified by the first live run +([33771257397](https://github.com/opentdf/tests/actions/runs/33771257397), +2026-09-03, `run-zip64=true java-ref="main latest"`): + +- [x] The 2.1 GiB roundtrip runs across go, java and web as encryptors, and + the fixture is not the bottleneck: whole jobs took 9m37s–12m45s against + a 90-minute budget, of which pytest was 90s–207s. +- [x] Every writer put an offset in the window — 2254888013 (go), + 2254888021 (java), 2254918149 (js) — so `_assert_reaches_the_window` + held in practice, not just by construction. +- [x] A pre-#393 java decryptor reports `XFAIL` with the DSPX-4592 reason + against all three writers. The failure is the sign-extension path + (`FileChannel.position()` throwing `IllegalArgumentException` on a + negative offset), not an OOM. +- [x] No OOM in either shim with the heap headroom the job sets. +- [x] `xct (main, go@main)` passed on the same run, so the `--sizes` + refactor did not disturb the default path. +- [ ] Sibling tickets' "Shared cross-SDK work" sections are satisfied. + +## First live run: what it found + +The job is red, which is the intended sequencing — but for more reasons than +the ticket anticipated. + +**java main is still pre-fix**, as expected — java-sdk#393 is open, not +merged (checked 2026-09-03). So main is the baseline, and its writer emits the +identical container to v0.18.0: + +``` +entry offset raw usize zip64 signed +0.payload 0 4294967295 2254887958 True -1 +0.manifest.json 2254888021 2254888021 139174 False -2040079275 +``` + +That is exactly the defect the ticket describes: `stream()` sets +`isZip64 = true` for the payload, `writeByteArray` hardcodes `false` for the +manifest, so the manifest's local-header offset goes out as a raw 32-bit value +with no extra field. The reader half is missing too — java@main fails to read +its own container at `ZipReader$Entry.getData:167`, and go's at +`ZipReader.:294`, both `FileChannel.position()` rejecting a negative +argument. + +Because `zip64_reader_xfail` keys on a released semver, java@main is *not* +xfailed and fails hard. That is the honest report — main is broken — but it +means the nightly stays red until #393 actually lands. + +**web-sdk is conformant, and confirms the parser.** js emits the +`0xFFFFFFFF` sentinel plus an extra field for both entries, and `zipinspect` +resolved the real values (offset 2254918149, size 2254918058) out of it. The +control behaved as predicted. + +**A defect nobody had filed: neither go nor java can read web-sdk's 2.1 GiB +payload.** js→js passes; js→go fails with +`splitKey.GetSignature failed: fail to create gmac signature`, and js→java@main +gets *past* the ZIP layer and then fails with +`tried to calculate GMAC on too small a payload. payload is 0 bytes while GMAC +is 16 bytes` at `TDF.calculateSignature:385`. + +**Root cause found, and it is not ZIP64.** web-sdk omits `segmentSize` and +`encryptedSegmentSize` from a segment object whenever they equal the manifest +defaults (`web-sdk/lib/tdf3/src/tdf.ts:696-700` — the ternaries yield +`undefined`, which drops the key from the JSON). Readers are meant to fall back +to the mandatory `segmentSizeDefault` / `encryptedSegmentSizeDefault`. Neither +consumer does: + +- go — `sdk/manifest.go:3-7` declares `Size`/`EncryptedSize` as plain `int64` + with no fallback, so an absent key is `0`. `sdk/tdf.go:993` then reads an + empty buffer, the `len(readBuf) != seg.EncryptedSize` guard at `:997` passes + vacuously, and `calculateSignature` fails the `kGMACPayloadLength > len(data)` + check at `:1531`. +- java — `Manifest.java:101-102` are plain `long`s; `TDF.java:334` allocates + `new byte[0]`; `TDF.java:385` throws. + +One cause, both errors. `manifest.schema.json` settles which side is wrong: +`segmentSizeDefault` and `encryptedSegmentSizeDefault` are **required** on +`integrityInformation`, and `segments/items` has **no** `required` list — the +per-segment values are optional overrides by design. web-sdk is conformant; go +and java are not. + +The manifest sizes above are independent confirmation: web-sdk spends 36.7 +bytes per segment, exactly `{"hash":"<24 b64 chars>"},`, while go spends 90.3 +carrying the full triple. + +**The real threshold is 1 MiB, not 2.1 GiB** — web-sdk's `DEFAULT_SEGMENT_SIZE` +(`tdf.ts:76`), the point at which the first exactly-default-sized chunk appears. +Below it the lone chunk is partial, its size is written explicitly, and both +readers cope; that is the only reason the 128-byte nightly is green. So every +web-sdk TDF over 1 MiB is currently unreadable by go and java, and has been +since 2022-08-22 (web-sdk e991829). It went four years undetected for exactly +the reason this ticket exists: xtest tested 128 bytes, and the 5 GiB `--large` +fixture had no CI wiring. + +Tracked on the reader side, where the defect is, rather than as a fourth +ticket: **DSPX-4589 finding 4** (java) and **DSPX-4590 finding 7** (go), each +with an acceptance criterion that a web-sdk TDF over 1 MiB round-trips. web-sdk +is conformant and is not being asked to change, so DSPX-4591 is unaffected. +Both fixes want a ~2 MiB regression cell, not a multi-GiB one — see Out of +Scope. diff --git a/xtest/AGENTS.md b/xtest/AGENTS.md index 2dc875a6..5997f809 100644 --- a/xtest/AGENTS.md +++ b/xtest/AGENTS.md @@ -27,7 +27,7 @@ fixture system. | `--sdks-encrypt`, `--sdks-decrypt` | Asymmetric encrypt/decrypt SDK selection (use when reproducing cross-SDK interop bugs). | | `--containers ztdf ztdf-ecwrap` | Which TDF container types to exercise. | | `--no-audit-logs` | Skip audit-log assertions for this run. CLI equivalent of `DISABLE_AUDIT_ASSERTIONS=1`. | -| `--sizes small,chunky` | Which payload sizes to parametrize over (`small` 128 B, `chunky` 5 MiB, `large` 5 GiB). Defaults to `small`. Every extra size fans out every test taking `pt_file`. `--large` is a deprecated alias for `small,large`. | +| `--sizes small,chunky` | Which payload sizes to parametrize over (`small` 128 B, `chunky` 5 MiB, `medium` 2.1 GiB, `large` 5 GiB). Defaults to `small`. Every extra size fans out every test taking `pt_file`. `--large` is a deprecated alias for `small,large`. | ## Environment Variables @@ -35,7 +35,7 @@ Beyond the repo-wide ones in `../AGENTS.md`: | Variable | Purpose | |----------|---------| -| `XT_TMP_DIR` | Root for generated fixtures and ciphertexts (default `tmp/`). Point at a large volume for `large` runs. | +| `XT_TMP_DIR` | Root for generated fixtures and ciphertexts (default `tmp/`). Point at a large volume for `medium`/`large` runs. | | `XT_FORCE_SUPPORTS` | Comma-separated features to treat as supported, bypassing the `cli.sh supports` gate. For evaluating a fix before it releases — see `../AGENTS.md`. Unknown names raise. | ## Authoring a New Test diff --git a/xtest/conftest.py b/xtest/conftest.py index 4a19f132..3dfc865f 100644 --- a/xtest/conftest.py +++ b/xtest/conftest.py @@ -151,7 +151,8 @@ def resolve_sizes(config: pytest.Config) -> list[str]: "deprecated spelling of --sizes small,large" ) warnings.warn( - "--large is deprecated; use --sizes small,large", + "--large is deprecated; use --sizes small,large (or --sizes medium " + "for the 2-4 GiB ZIP64 band, which --large steps straight over)", DeprecationWarning, stacklevel=2, ) @@ -428,23 +429,51 @@ def pytest_configure(config: pytest.Config): ) +def _item_exercises_zip64_window(item: pytest.Item, session_sizes: list[str]) -> bool: + """Whether this item has a payload large enough for the ZIP64 tests. + + Size-aware items must be judged by their own parametrized value. Marked + items without a ``size`` parameter retain the session-level behaviour so + a future ZIP64 test with a purpose-built fixture is not dropped merely + because it does not use :func:`pt_file`. + """ + callspec = getattr(item, "callspec", None) + item_size = callspec.params.get("size") if callspec is not None else None + if isinstance(item_size, str): + return sizes.exercises_zip64_window(item_size) + return any(sizes.exercises_zip64_window(size) for size in session_sizes) + + def pytest_collection_modifyitems( config: pytest.Config, items: list[pytest.Item] ) -> None: - """Drop the benchmark cells entirely unless --bench asked for them. - - Deselected rather than skipped: a 20-minute cell has no business in the - regular integration matrix, and a skip would report it as a test that - exists and was declined rather than one that was never in scope. + """Drop cells the session did not ask for. + + Two groups, deselected rather than skipped for the same reason: neither a + 20-minute benchmark nor a 2.1 GiB roundtrip has any business in the + regular integration matrix, and a skip would report them as tests that + exist and were declined rather than ones that were never in scope. + + - ``benchmark``: needs --bench. + - ``zip64``: needs a payload size that can reach the 2**31 boundary. At + the default 128 bytes these tests cannot exercise anything, and the one + thing worse than not running them is running them green on a payload + that never touches the code path. """ - if config.getoption("--bench", default=False): - return - keep, drop = [], [] + drop: list[pytest.Item] = [] + want_bench = bool(config.getoption("--bench", default=False)) + session_sizes = resolve_sizes(config) for item in items: - (drop if item.get_closest_marker("benchmark") else keep).append(item) + if not want_bench and item.get_closest_marker("benchmark"): + drop.append(item) + elif item.get_closest_marker("zip64") and not _item_exercises_zip64_window( + item, session_sizes + ): + drop.append(item) if drop: + dropped = set(map(id, drop)) config.hook.pytest_deselected(items=drop) - items[:] = keep + items[:] = [i for i in items if id(i) not in dropped] def pytest_sessionfinish(session: pytest.Session, exitstatus: int): @@ -584,8 +613,9 @@ def pt_file(tmp_dir: Path, size: str) -> Path: Args: tmp_dir: Temporary directory for test files size: a key of :data:`sizes.SIZES` -- 'small' (128 bytes), - 'chunky' (5 MiB, several default-sized segments), or - 'large' (5 GiB) + 'chunky' (5 MiB, several default-sized segments), + 'medium' (2.1 GiB, inside the ZIP64 broken window), or + 'large' (5 GiB, above it) Returns: Path to the generated plaintext file diff --git a/xtest/pyproject.toml b/xtest/pyproject.toml index 5884dea1..d2f13502 100644 --- a/xtest/pyproject.toml +++ b/xtest/pyproject.toml @@ -89,6 +89,7 @@ known-first-party = [ "fixtures", "perf", "sizes", + "zipinspect", ] [tool.ruff.format] @@ -113,4 +114,5 @@ addopts = "-ra -v" markers = [ "benchmark: paired A/B performance cell; only collected under --bench", "no_audit_logs: opt this test out of the default audit-log assertions", + "zip64: multi-GiB ZIP64 boundary cell; only collected when --sizes reaches 2**31", ] diff --git a/xtest/sizes.py b/xtest/sizes.py index e9d5cf15..6f44551a 100644 --- a/xtest/sizes.py +++ b/xtest/sizes.py @@ -1,13 +1,53 @@ -"""Plaintext payload sizes for cross-SDK test fixtures. +"""Plaintext payload sizes, and the ZIP64 window they are chosen around. Kept free of pytest and of ``tdfs`` so that both ``conftest.py`` and the test modules can name a size without importing each other. + +The ZIP central directory stores local-header offsets and entry sizes in 32-bit +fields that are *unsigned on the wire*. Three regimes follow, and only one of +them can expose a signed-widening bug: + +=========================== ========================================== +value what a reader sees +=========================== ========================================== +``v < 2**31`` a signed read and an unsigned read agree +``2**31 <= v < 2**32`` a signed read comes back negative +``v >= 2**32`` ZIP64 sentinel; the 32-bit field is never + populated with a real value, so the bug + cannot fire +=========================== ========================================== + +That middle row is the only broken window, and it is exactly what +:data:`SIZES`'s ``medium`` entry exists to land a TDF's manifest offset in. """ from __future__ import annotations -#: 5 MiB. This is the smallest size at which *every* SDK's writer emits more -#: than one **default-sized** segment. +#: Smallest value a 32-bit field must be read as unsigned to survive. +ZIP64_WINDOW_LOW = 2**31 + +#: At and above this the format requires the ZIP64 sentinel plus an extra +#: field, so the 32-bit field holds 0xFFFFFFFF rather than a real value. +ZIP64_WINDOW_HIGH = 2**32 + +#: 2.1 GiB. Sits ~102 MiB inside the low edge of the broken window. +#: +#: The margin is the point. A TDF writes ``0.payload`` first and +#: ``0.manifest.json`` after it, so the manifest's local-header offset is +#: roughly the payload size -- and that offset is the value under test. The +#: gap to 2**31 has to be wider than anything that could shift it: segment +#: padding, manifest length, per-entry header overhead. 102 MiB is not a +#: round number because it does not need to be; it needs to be unarguably +#: larger than those. +#: +#: Shrinking this below 2**31 does not make the test cheaper, it makes it +#: vacuous -- every SDK takes the safe path and the test passes without +#: exercising anything. See the assertion in test_zip64.py that fails loudly +#: rather than letting that happen quietly. +MEDIUM_BYTES = 2_254_857_830 + +#: 5 MiB. Nothing to do with the ZIP64 window -- this is the smallest size at +#: which *every* SDK's writer emits more than one **default-sized** segment. #: #: A segment only exercises the ``chunky`` path if its size equals the #: manifest-level default, because that is precisely the case web-sdk omits @@ -23,8 +63,25 @@ SIZES: dict[str, int] = { "small": 128, "chunky": CHUNKY_BYTES, + "medium": MEDIUM_BYTES, "large": 5 * 2**30, } #: Order to emit parametrized sizes in, cheapest first. -SIZE_ORDER: tuple[str, ...] = ("small", "chunky", "large") +SIZE_ORDER: tuple[str, ...] = ("small", "chunky", "medium", "large") + + +def in_zip64_window(n: int) -> bool: + """True for values a signed 32-bit read would mangle.""" + return ZIP64_WINDOW_LOW <= n < ZIP64_WINDOW_HIGH + + +def exercises_zip64_window(size: str) -> bool: + """True if a payload of this size can put a real value in the broken window. + + Note this is ``>=`` the low edge rather than :func:`in_zip64_window`: a + 5 GiB payload does not itself land in the window, but the run that asked + for it is plainly a large-file run and the zip64 module has something to + say about its ZIP64 encoding too. + """ + return SIZES[size] >= ZIP64_WINDOW_LOW diff --git a/xtest/tdfs.py b/xtest/tdfs.py index 392bd31b..c16abc72 100644 --- a/xtest/tdfs.py +++ b/xtest/tdfs.py @@ -883,8 +883,9 @@ def skip_chunky_skew(ct_file: Path, decrypt_sdk: SDK): """Skip if ``ct_file`` needs segment-size defaulting and the reader lacks it. A skip and not an xfail: this cell runs on the PR gate, where a - permanently-red job trains people to ignore it, and it needs no dated - guess about which release carries the fix. + permanently-red job trains people to ignore it, and unlike + :func:`zip64_reader_xfail` it needs no dated guess about which release + carries the fix. The cost is that it stays skipped until somebody edits ``sdk/{go,java}/cli.sh`` to answer yes -- the ``supports`` case statement @@ -906,6 +907,44 @@ def skip_chunky_skew(ct_file: Path, decrypt_sdk: SDK): ) +#: First java-sdk release containing java-sdk#393. +#: +#: Before it, ``ZipReader.readInt()`` sign-extends, so a central-directory +#: offset in ``[2**31, 2**32)`` comes back negative and the read fails or +#: seeks to nonsense. A 2.1 GiB payload puts the manifest's offset exactly +#: there. See DSPX-4592. +#: +#: Keep this honest. Set too high, a fixed release keeps reporting XFAIL and +#: a genuine regression hides behind it; set too low, the strict xfail turns +#: every pre-fix cell into a hard failure. Update it when the release with +#: #393 actually ships, not when the PR merges. +JAVA_ZIP64_READER_FIX = (0, 19, 0) + + +def zip64_reader_xfail(decrypt_sdk: SDK) -> pytest.MarkDecorator | None: + """An xfail marker for decryptors known to mishandle the 2-4 GiB band. + + ``strict=True`` deliberately. The point of this test is to flip to green + when the sibling fixes land: an XPASS here means a build we believed + broken now reads the container correctly, and that should fail the run so + somebody comes and deletes this predicate rather than leaving a + permanently-XFAIL cell that nobody reads. + + Branch builds (``main``) have no semver and are never marked -- they are + the builds expected to carry the fix. + """ + sv = decrypt_sdk.semver() + if decrypt_sdk.sdk == "java" and sv is not None and sv < JAVA_ZIP64_READER_FIX: + return pytest.mark.xfail( + strict=True, + reason=( + f"DSPX-4592: {decrypt_sdk} predates java-sdk#393; readInt() " + "sign-extends the manifest's central-directory offset" + ), + ) + return None + + def _parse_semver(version: str) -> tuple[int, int, int] | None: """Parse a version string (with optional 'v' prefix) into (major, minor, patch).""" m = _version_re.match(version.lstrip("v")) diff --git a/xtest/test_sizes_units.py b/xtest/test_sizes_units.py index b91a8911..67db8363 100644 --- a/xtest/test_sizes_units.py +++ b/xtest/test_sizes_units.py @@ -12,6 +12,41 @@ class TestSizes: + def test_medium_is_inside_the_broken_window(self): + """The whole ticket rests on this one number being in the band.""" + assert sizes.in_zip64_window(sizes.MEDIUM_BYTES) + + def test_medium_has_margin_below_the_low_edge(self): + """Manifest size and segment padding must not push the offset back under 2**31. + + The manifest is written after the payload, so its local-header offset + is the payload size plus header overhead -- but the assertion that + matters is the reverse: the payload alone must already clear the + boundary by more than any plausible overhead. + """ + margin = sizes.MEDIUM_BYTES - sizes.ZIP64_WINDOW_LOW + assert margin > 100 * 2**20, ( + f"only {margin} bytes of margin above 2**31; segment padding and " + "manifest size could push the interesting offset back below it" + ) + + def test_small_and_large_sit_outside_the_window(self): + """The two pre-existing sizes are exactly why this ticket exists.""" + assert sizes.SIZES["small"] < sizes.ZIP64_WINDOW_LOW + assert sizes.SIZES["large"] >= sizes.ZIP64_WINDOW_HIGH + assert not sizes.in_zip64_window(sizes.SIZES["small"]) + assert not sizes.in_zip64_window(sizes.SIZES["large"]) + + # Named size_name, not size: `size` is parametrized session-wide by + # conftest's pytest_generate_tests, and reusing it here is a collection + # error rather than a shadow. + @pytest.mark.parametrize( + ("size_name", "expected"), + [("small", False), ("chunky", False), ("medium", True), ("large", True)], + ) + def test_which_sizes_select_the_zip64_tests(self, size_name: str, expected: bool): + assert sizes.exercises_zip64_window(size_name) is expected + def test_chunky_clears_every_sdk_default_segment(self): """5 MiB has to buy more than one *default-sized* segment, everywhere. @@ -25,6 +60,11 @@ def test_chunky_clears_every_sdk_default_segment(self): largest_known_default = 2 * 2**20 assert sizes.CHUNKY_BYTES > 2 * largest_known_default + def test_chunky_stays_cheap(self): + """It runs on the PR gate, so it must not creep toward the nightly's cost.""" + assert sizes.CHUNKY_BYTES < 64 * 2**20 + assert not sizes.in_zip64_window(sizes.CHUNKY_BYTES) + class TestSizesOptionParsing: def test_dedups_and_orders_cheapest_first(self): diff --git a/xtest/test_tdfs_units.py b/xtest/test_tdfs_units.py index b6b9b5f2..8bb22ad0 100644 --- a/xtest/test_tdfs_units.py +++ b/xtest/test_tdfs_units.py @@ -1,20 +1,24 @@ -"""Offline tests for tdfs.py's XT_FORCE_SUPPORTS override and chunky gating (DSPX-4638, DSPX-4589). +"""Offline tests for tdfs.py's anti-vacuous-green machinery (DSPX-4592, DSPX-4638). -No platform, no SDK, no subprocess. ``_parse_forced_supports`` and -``skip_chunky_skew`` are both safeguards built specifically to stop a real -regression from hiding behind a skip -- so they are worth testing on their -own. +No platform, no SDK, no subprocess. ``_parse_forced_supports``, +``zip64_reader_xfail``, and ``skip_chunky_skew`` are all safeguards built +specifically to stop a real regression from hiding behind a skip or a stale +xfail -- so they are worth testing on their own, the same way the ZIP64 +parser they sit next to is tested in ``test_zip64_units.py``. """ import json import zipfile from pathlib import Path +from types import SimpleNamespace from typing import cast import pytest import tdfs +# --- tdfs._parse_forced_supports --------------------------------------------- + class TestParseForcedSupports: def test_parses_comma_and_whitespace_separated_names(self): @@ -32,6 +36,35 @@ def test_unknown_name_raises(self): tdfs._parse_forced_supports("hexles") +# --- tdfs.zip64_reader_xfail -------------------------------------------------- + + +def _stub_sdk(sdk: str, semver: tuple[int, int, int] | None) -> tdfs.SDK: + """A duck-typed stand-in exposing only what zip64_reader_xfail reads.""" + return cast(tdfs.SDK, SimpleNamespace(sdk=sdk, semver=lambda: semver)) + + +class TestZip64ReaderXfail: + def test_pre_fix_java_gets_a_strict_xfail(self): + stub = _stub_sdk("java", (0, 18, 0)) + marker = tdfs.zip64_reader_xfail(stub) + assert marker is not None + assert marker.mark.kwargs["strict"] is True + + def test_post_fix_java_is_not_marked(self): + stub = _stub_sdk("java", tdfs.JAVA_ZIP64_READER_FIX) + assert tdfs.zip64_reader_xfail(stub) is None + + def test_non_java_sdk_is_never_marked(self): + stub = _stub_sdk("go", (0, 1, 0)) + assert tdfs.zip64_reader_xfail(stub) is None + + def test_branch_build_is_never_marked(self): + """A branch build (e.g. 'main') has no semver and is expected to carry the fix.""" + stub = _stub_sdk("java", None) + assert tdfs.zip64_reader_xfail(stub) is None + + # --- tdfs.elides_segment_sizes / tdfs.skip_chunky_skew ------------------------ diff --git a/xtest/test_zip64.py b/xtest/test_zip64.py new file mode 100644 index 00000000..01eabefe --- /dev/null +++ b/xtest/test_zip64.py @@ -0,0 +1,142 @@ +"""Cross-SDK coverage for the ZIP64 boundary at 2 GiB (DSPX-4592). + +Every test here is marked ``zip64`` and is deselected unless the session asks +for a payload size that can reach ``2**31`` -- see the size table in +``sizes.py`` for why 2.1 GiB and not something rounder, and +``pytest_collection_modifyitems`` in ``conftest.py`` for the deselection. + +Run it with:: + + uv run pytest test_zip64.py --sizes medium --sdks "go java js" -v + +These are separate from ``test_tdfs.py`` for three reasons that all come down +to the payload size: the decrypted output is deleted as soon as it has been +compared rather than accumulating at 2.1 GiB a time, the container's ZIP +encoding is asserted on directly, and CI can select them without dragging the +rest of the suite up to multi-GiB payloads. +""" + +import filecmp +import logging +from pathlib import Path + +import pytest + +import tdfs +import zipinspect +from abac import Attribute +from fixtures.encryption import EncryptFactory +from sizes import SIZES, ZIP64_WINDOW_LOW + +logger = logging.getLogger(__name__) + +# ``no_audit_logs`` is inert today -- nothing here requests the ``audit_logs`` +# fixture, and it is not autouse -- but it states the intent: this module tests +# the container encoding, and a rewrap audit event says nothing about that. +pytestmark = [pytest.mark.zip64, pytest.mark.no_audit_logs] + + +def _assert_reaches_the_window( + entries: list[zipinspect.CentralDirectoryEntry], + pt_file: Path, + encrypt_sdk: tdfs.SDK, +) -> None: + """Fail unless some entry actually landed at or above 2**31. + + This is the load-bearing assertion in the module. Everything else here + tests how an SDK handles a value in the broken window; if no value got + there, the rest of the test passes without exercising a single line of + the code under test, and reports success for it. + + That is the exact failure this ticket exists to close -- the suite already + had a large-file path that stepped over the window -- so it is an + assertion rather than a skip. + """ + biggest = max((e.local_header_offset for e in entries), default=0) + assert biggest >= ZIP64_WINDOW_LOW, ( + f"{encrypt_sdk} wrote a container whose largest local-header offset is " + f"{biggest}, below 2**31 ({ZIP64_WINDOW_LOW}), from a " + f"{pt_file.stat().st_size}-byte payload. Nothing in this test is " + f"exercising the 2-4 GiB band.\n" + zipinspect.describe(entries) + ) + + +def test_zip64_band_roundtrip( + request: pytest.FixtureRequest, + encrypt_sdk: tdfs.SDK, + decrypt_sdk: tdfs.SDK, + pt_file: Path, + size: str, + in_focus: set[tdfs.SDK], + attribute_default_rsa: Attribute, + encrypted_tdf: EncryptFactory, +): + """Encrypt and decrypt a payload whose manifest offset is in the broken window. + + The whole cross-SDK matrix runs against one payload: writer defects and + reader defects both surface as a failure to round-trip, and which SDK is + at fault is what the structural assertions below disambiguate. + """ + if not in_focus & {encrypt_sdk, decrypt_sdk}: + pytest.skip("Not in focus") + tdfs.skip_hexless_skew(encrypt_sdk, decrypt_sdk) + + ct_file = encrypted_tdf( + encrypt_sdk, + target_mode=tdfs.select_target_version(encrypt_sdk, decrypt_sdk), + attr_values=attribute_default_rsa.value_fqns, + ) + + entries = zipinspect.central_directory(ct_file) + logger.info( + "%s wrote %s at size=%s (%d bytes):\n%s", + encrypt_sdk, + ct_file.name, + size, + SIZES[size], + zipinspect.describe(entries), + ) + + # Writer conformance first, and outside the reader's xfail below. A + # writer regression must not hide behind a known reader bug: if these + # fail under an xfail marker the cell reports XFAIL and nobody looks. + _assert_reaches_the_window(entries, pt_file, encrypt_sdk) + zipinspect.assert_zip64_above_4gib(entries) + + in_window = zipinspect.entries_in_window(entries) + logger.info( + "%s: %d entr%s in [2**31, 2**32); zip64 extra field used for %s", + encrypt_sdk, + len(in_window), + "y" if len(in_window) == 1 else "ies", + [e.name for e in in_window if e.has_zip64_extra] or "none", + ) + + # Keep the independent segment-defaulting incompatibility out of the + # ZIP64 result. In particular, web-sdk uses ZIP64 sentinels in this band, + # so those containers do not exercise Java's signed 32-bit read defect. + tdfs.skip_chunky_skew(ct_file, decrypt_sdk) + + # Apply the reader xfail only when a real 32-bit value (not the sentinel) + # exercises the signed-risk window. Writer conformance has already been + # checked above, so a failure from this point belongs to the reader. + if zipinspect.entries_with_raw_values_in_window(entries): + if mark := tdfs.zip64_reader_xfail(decrypt_sdk): + request.node.add_marker(mark) + + rt_file = encrypted_tdf.rt_file(ct_file, decrypt_sdk) + try: + decrypt_sdk.decrypt(ct_file, rt_file, "ztdf") + # shallow=False explicitly: the default compares a stat signature + # first and only falls through to a byte compare because the mtimes + # happen to differ. At this size the difference between checking the + # bytes and checking the size is worth not leaving to chance. + assert filecmp.cmp(pt_file, rt_file, shallow=False), ( + f"{decrypt_sdk} decrypted {ct_file.name} without error but the " + f"output does not match the {pt_file.stat().st_size}-byte input" + ) + finally: + # 2.1 GiB per pair. The ciphertext is session-cached and shared, but + # these are not, and a full matrix would fill the runner's disk long + # before the last cell. + rt_file.unlink(missing_ok=True) diff --git a/xtest/test_zip64_units.py b/xtest/test_zip64_units.py new file mode 100644 index 00000000..bebc679f --- /dev/null +++ b/xtest/test_zip64_units.py @@ -0,0 +1,434 @@ +"""Offline tests for the ZIP64 boundary machinery (DSPX-4592). + +No platform, no SDK, no subprocess. These run in ``check.yml`` on every PR, +because the multi-GiB test they support runs only on a nightly cron -- a +parser bug found six weeks later, in a job nobody watches, on a fixture that +takes twenty minutes to reproduce, is a bad trade against a few seconds here. + +The central directories are synthesized byte by byte rather than produced by +``zipfile``. A real 2.1 GiB container is exactly what cannot be built in a +unit test, and ``zipfile`` will not emit a 32-bit field holding a value in +``[2**31, 2**32)`` on request -- which is the encoding under test. +""" + +import struct +import zipfile +from pathlib import Path +from types import SimpleNamespace +from typing import cast + +import pytest + +import conftest +import zipinspect +from sizes import MEDIUM_BYTES, ZIP64_WINDOW_HIGH, ZIP64_WINDOW_LOW +from zipinspect import ZIP64_SENTINEL_32, MalformedZipError + +# --- Synthetic container construction --------------------------------------- + + +def cen_record( + name: str, + *, + raw_offset: int, + raw_usize: int = 0, + raw_csize: int = 0, + zip64_offset: int | None = None, + zip64_usize: int | None = None, + zip64_csize: int | None = None, +) -> bytes: + """One central-directory header, with an optional ZIP64 extra field. + + ``zip64_*`` values are written into the extra field in APPNOTE 4.5.3's + fixed order (uncompressed, compressed, offset); pass them only for the + fields whose 32-bit slot holds the sentinel, which is the same contract + the parser relies on. + """ + extra = b"" + body = b"" + if zip64_usize is not None: + body += struct.pack(" Path: + """Write a container that is nothing but a central directory and an EOCD. + + The parser never reads entry data, so leaving it out keeps these tests + instant while exercising every field it does read. + """ + cd = b"".join(records) + cd_offset = 0 + eocd = ( + b"PK\x05\x06" + + struct.pack(" Path: + """Same, but located through a ZIP64 EOCD record and its locator. + + The 32-bit EOCD carries sentinels, so a reader that stops there sees + 0xFFFF entries at offset 0xFFFFFFFF. This is how a container whose + central directory sits past 4 GiB has to be read. + """ + cd = b"".join(records) + cd_offset = 0 + eocd64 = ( + b"PK\x06\x06" + + struct.pack("4 GiB offset with no ZIP64 encoding, + # which is the state a non-conformant writer would leave behind. + broken = [ + zipinspect.CentralDirectoryEntry( + name="0.manifest.json", + raw_compressed_size=0, + raw_uncompressed_size=0, + raw_local_header_offset=12345, + compressed_size=0, + uncompressed_size=0, + local_header_offset=5 * 2**30, + has_zip64_extra=False, + ) + ] + zipinspect.assert_zip64_above_4gib(entries) # the conformant one passes + with pytest.raises(AssertionError, match="ZIP64 sentinel"): + zipinspect.assert_zip64_above_4gib(broken) + + def test_above_4gib_uncompressed_size_without_the_sentinel_fails(self): + """The size branches had no test of their own; the offset test above doesn't touch them.""" + broken = [ + zipinspect.CentralDirectoryEntry( + name="0.payload", + raw_compressed_size=0, + raw_uncompressed_size=12345, + raw_local_header_offset=0, + compressed_size=0, + uncompressed_size=5 * 2**30, + local_header_offset=0, + has_zip64_extra=False, + ) + ] + with pytest.raises(AssertionError, match="ZIP64 sentinel"): + zipinspect.assert_zip64_above_4gib(broken) + + def test_above_4gib_compressed_size_without_the_sentinel_fails(self): + """Compressed size must be checked against its own raw field, not the uncompressed one. + + A TDF is STORED, not DEFLATEd, so the compressed field is at least as + likely to cross 2**32 as the uncompressed one -- but a check that only + looks at ``uses_zip64_for_sizes`` (an OR over both raw fields) would + let a correctly-sentineled uncompressed field paper over a broken + compressed one. This entry has exactly that shape. + """ + broken = [ + zipinspect.CentralDirectoryEntry( + name="0.payload", + raw_compressed_size=12345, + raw_uncompressed_size=ZIP64_SENTINEL_32, + raw_local_header_offset=0, + compressed_size=5 * 2**30, + uncompressed_size=5 * 2**30, + local_header_offset=0, + has_zip64_extra=True, + ) + ] + with pytest.raises(AssertionError, match="compressed-size field"): + zipinspect.assert_zip64_above_4gib(broken) + + def test_above_4gib_sizes_with_the_sentinel_pass(self, tmp_path: Path): + """The positive counterpart: both size fields correctly ZIP64-encoded.""" + p = synth_zip( + tmp_path / "big-sizes.zip", + [ + cen_record( + "0.payload", + raw_offset=0, + raw_usize=ZIP64_SENTINEL_32, + raw_csize=ZIP64_SENTINEL_32, + zip64_usize=5 * 2**30, + zip64_csize=5 * 2**30 + 1, + ) + ], + ) + entries = zipinspect.central_directory(p) + zipinspect.assert_zip64_above_4gib(entries) + + def test_window_entries_are_reported_for_either_encoding(self, tmp_path: Path): + """Both a raw value and a sentinel in the band are legal and both are listed.""" + p = synth_zip( + tmp_path / "mixed.zip", + [ + cen_record("raw", raw_offset=MEDIUM_BYTES), + cen_record( + "sentinel", + raw_offset=ZIP64_SENTINEL_32, + zip64_offset=MEDIUM_BYTES + 1024, + ), + cen_record("small", raw_offset=1024), + ], + ) + entries = zipinspect.central_directory(p) + assert {e.name for e in zipinspect.entries_in_window(entries)} == { + "raw", + "sentinel", + } + + def test_only_raw_window_values_exercise_signed_read(self, tmp_path: Path): + """The sentinel redirects to ZIP64 data and is not a signed read risk.""" + p = synth_zip( + tmp_path / "mixed.zip", + [ + cen_record("raw", raw_offset=MEDIUM_BYTES), + cen_record( + "sentinel", + raw_offset=ZIP64_SENTINEL_32, + zip64_offset=MEDIUM_BYTES + 1024, + ), + ], + ) + entries = zipinspect.central_directory(p) + assert { + e.name for e in zipinspect.entries_with_raw_values_in_window(entries) + } == {"raw"} + + def test_describe_includes_the_numbers_needed_to_debug(self, tmp_path: Path): + p = synth_zip( + tmp_path / "d.zip", [cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES)] + ) + text = zipinspect.describe(zipinspect.central_directory(p)) + assert "0.manifest.json" in text + assert str(MEDIUM_BYTES) in text diff --git a/xtest/zipinspect.py b/xtest/zipinspect.py new file mode 100644 index 00000000..87977222 --- /dev/null +++ b/xtest/zipinspect.py @@ -0,0 +1,350 @@ +"""Raw ZIP central-directory reader, for asserting on the *encoding*. + +``zipfile`` cannot be used for this. It normalises ZIP64 away -- ask it for an +entry's header offset and you get the resolved value, whether that came from +the 32-bit field or from a ZIP64 extra field. The distinction it discards is +precisely what these tests are about, so the bytes are parsed here instead. + +Only the tail of the file plus the central directory is read, so this stays +cheap on a multi-GiB container. + +Reference: APPNOTE.TXT 4.3.12 (central directory header), 4.3.16 (end of +central directory), 4.5.3 (the ZIP64 extended information extra field). +""" + +from __future__ import annotations + +import struct +from dataclasses import dataclass +from pathlib import Path + +from sizes import ZIP64_WINDOW_HIGH, in_zip64_window + +# Signatures, little-endian. +_CEN_SIG = b"PK\x01\x02" +_EOCD_SIG = b"PK\x05\x06" +_EOCD64_SIG = b"PK\x06\x06" +_EOCD64_LOCATOR_SIG = b"PK\x06\x07" + +#: Written into a 32-bit field to mean "the real value is in the ZIP64 extra +#: field". APPNOTE 4.4.1.4. +ZIP64_SENTINEL_32 = 0xFFFFFFFF +ZIP64_SENTINEL_16 = 0xFFFF + +#: Header ID of the ZIP64 extended information extra field. APPNOTE 4.5.3. +ZIP64_EXTRA_ID = 0x0001 + +_EOCD_SIZE = 22 +_EOCD64_LOCATOR_SIZE = 20 +#: A ZIP comment is a 16-bit length, so the EOCD cannot start further back +#: than this from the end of the file. +_MAX_EOCD_SEARCH = _EOCD_SIZE + 0xFFFF + + +class MalformedZipError(Exception): + """The container is not a ZIP we can parse at all.""" + + +@dataclass(frozen=True, slots=True) +class CentralDirectoryEntry: + """One central-directory record, with the raw fields kept alongside. + + ``raw_*`` are the 32-bit values exactly as they appear on the wire. + The unprefixed attributes are the resolved values, ZIP64 extra field + applied where present. Comparing the two is how a caller tells "this + writer emitted a real 2.1 GiB value in a 32-bit field" from "this writer + emitted the sentinel and put the value in the extra field". + """ + + name: str + raw_compressed_size: int + raw_uncompressed_size: int + raw_local_header_offset: int + compressed_size: int + uncompressed_size: int + local_header_offset: int + has_zip64_extra: bool + + @property + def uses_zip64_for_offset(self) -> bool: + return self.raw_local_header_offset == ZIP64_SENTINEL_32 + + @property + def uses_zip64_for_sizes(self) -> bool: + return ZIP64_SENTINEL_32 in ( + self.raw_compressed_size, + self.raw_uncompressed_size, + ) + + def signed_read_of_offset(self) -> int: + """What a reader that sign-extends a 32-bit read would compute. + + The defect this module exists to catch, expressed directly: for a raw + value at or above 2**31 this returns a negative number, and a seek to + it fails or lands on nonsense. + """ + return struct.unpack(" int: + """Offset of the EOCD record within the tail buffer. + + Searched backwards: the signature can legitimately appear inside a file + comment, and the last occurrence is the real one. + """ + idx = data.rfind(_EOCD_SIG) + if idx < 0: + raise MalformedZipError("no end-of-central-directory record found") + return idx + + +def _parse_zip64_extra( + extra: bytes, + *, + want_uncompressed: bool, + want_compressed: bool, + want_offset: bool, +) -> tuple[bool, int | None, int | None, int | None]: + """Pull the 64-bit values out of the ZIP64 extended information field. + + The field is positional, not tagged: values appear only for the 32-bit + fields that held the sentinel, in a fixed order (uncompressed size, + compressed size, local header offset, disk start). So which values are + present depends on the record that referenced it, which is what the + ``want_*`` flags carry in. + + Returns ``(present, uncompressed, compressed, offset)``; the values are + None when the corresponding 32-bit field did not hold the sentinel. + """ + pos = 0 + while pos + 4 <= len(extra): + header_id, size = struct.unpack_from(" len(extra): + break + if header_id != ZIP64_EXTRA_ID: + pos += size + continue + body = extra[pos : pos + size] + # Read the 64-bit values in APPNOTE order, consuming one only for each + # 32-bit field that actually held the sentinel. A truncated field + # yields None rather than raising: a malformed extra field is a + # finding for the caller's assertions, not a parse error. + values: list[int | None] = [] + at = 0 + for want in (want_uncompressed, want_compressed, want_offset): + if want and at + 8 <= len(body): + values.append(struct.unpack_from(" list[CentralDirectoryEntry]: + """Parse every central-directory record in ``path``. + + Reads the tail of the file to locate the directory, then the directory + itself. The payload is never touched, so cost is independent of container + size. + """ + size = path.stat().st_size + with path.open("rb") as f: + tail_len = min(size, _MAX_EOCD_SEARCH) + f.seek(size - tail_len) + tail = f.read(tail_len) + + eocd_at = _find_eocd(tail) + ( + cd_entries_this_disk, + cd_entries_total, + cd_size, + cd_offset, + ) = struct.unpack_from("= 0 and tail[locator_at : locator_at + 4] == _EOCD64_LOCATOR_SIG: + (eocd64_offset,) = struct.unpack_from(" str: + """One line per entry, for attaching to a failure message. + + A structural failure is nearly unreadable without the actual numbers, and + reproducing it costs a multi-GiB encrypt. + """ + header = ( + f"{'entry':<20} {'offset':>14} {'raw':>12} " + f"{'usize':>14} {'csize':>14} {'zip64':>6} {'signed':>14}" + ) + return "\n".join( + [header] + + [ + f"{e.name:<20} {e.local_header_offset:>14} " + f"{e.raw_local_header_offset:>12} {e.uncompressed_size:>14} " + f"{e.compressed_size:>14} " + f"{str(e.has_zip64_extra):>6} {e.signed_read_of_offset():>14}" + for e in entries + ] + ) + + +def assert_zip64_above_4gib(entries: list[CentralDirectoryEntry]) -> None: + """Every value at or above 2**32 must use the ZIP64 sentinel plus extra field. + + Unlike the 2-4 GiB band, there is no latitude here: a 32-bit field + physically cannot hold the value, so a writer that does not emit the + sentinel has produced a container whose stated offsets are wrong. + """ + for e in entries: + if e.local_header_offset >= ZIP64_WINDOW_HIGH: + assert e.uses_zip64_for_offset and e.has_zip64_extra, ( + f"entry {e.name!r} is at offset {e.local_header_offset}, at or " + f"above 2**32, but its 32-bit field holds " + f"{e.raw_local_header_offset} rather than the ZIP64 sentinel\n" + + describe(entries) + ) + if e.uncompressed_size >= ZIP64_WINDOW_HIGH: + assert e.raw_uncompressed_size == ZIP64_SENTINEL_32 and e.has_zip64_extra, ( + f"entry {e.name!r} is {e.uncompressed_size} bytes, at or above " + f"2**32, but its 32-bit size field holds " + f"{e.raw_uncompressed_size} rather than the ZIP64 sentinel\n" + + describe(entries) + ) + if e.compressed_size >= ZIP64_WINDOW_HIGH: + assert e.raw_compressed_size == ZIP64_SENTINEL_32 and e.has_zip64_extra, ( + f"entry {e.name!r} is {e.compressed_size} bytes compressed, at " + f"or above 2**32, but its 32-bit compressed-size field holds " + f"{e.raw_compressed_size} rather than the ZIP64 sentinel\n" + + describe(entries) + ) + + +def entries_in_window( + entries: list[CentralDirectoryEntry], +) -> list[CentralDirectoryEntry]: + """Entries with an offset or size in ``[2**31, 2**32)``. + + These are the records a sign-extending reader mishandles. Both encodings + -- a real unsigned 32-bit value, or the ZIP64 sentinel -- are legal here, + which is why this returns them for reporting rather than asserting on + which one the writer chose. + """ + return [ + e + for e in entries + if in_zip64_window(e.local_header_offset) + or in_zip64_window(e.uncompressed_size) + ] + + +def entries_with_raw_values_in_window( + entries: list[CentralDirectoryEntry], +) -> list[CentralDirectoryEntry]: + """Entries that exercise unsigned reads of real 32-bit values in the window. + + ``0xffffffff`` is numerically in the window, but it is a sentinel directing + the reader to the ZIP64 extra field. It therefore does not exercise the + signed 32-bit read defect this predicate identifies. + """ + return [ + e + for e in entries + if any( + value != ZIP64_SENTINEL_32 and in_zip64_window(value) + for value in ( + e.raw_compressed_size, + e.raw_uncompressed_size, + e.raw_local_header_offset, + ) + ) + ] From 507553922085b5ad28bdb0130729ec80e8073bdc Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 9 Sep 2026 16:29:34 -0400 Subject: [PATCH 2/3] Apply batched suggestions from code review Co-authored-by: Dave Mihalcik --- .github/workflows/check.yml | 5 ----- .github/workflows/xtest.yml | 20 +++++++------------- 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index a77faeb0..31b7d516 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -40,11 +40,6 @@ jobs: # No platform and no SDK builds required, so the part that has to be # *correct* is checked on every PR rather than only when the nightly runs. # - # test_zip64_units.py matters disproportionately here: the nightly zip64 - # job's verdict is only as good as this parser, and a parser bug would - # report a conformant container as broken (or the reverse) after an hour - # of multi-GiB IO that nobody wants to repeat to debug it. - # # --frozen --no-build: resolve nothing and build nothing, so a # dependency cannot slip in an unlocked version or a setup script on a # runner that already has everything installed from the step above. diff --git a/.github/workflows/xtest.yml b/.github/workflows/xtest.yml index ebbda99f..5a8848bc 100644 --- a/.github/workflows/xtest.yml +++ b/.github/workflows/xtest.yml @@ -42,12 +42,12 @@ on: required: false type: boolean default: false - description: "Run the 2.1 GiB ZIP64 boundary tests (DSPX-4592; adds ~60m per encrypting SDK). Set java-ref to 'main latest' to also exercise the pre-fix java reader, which is the defect this covers." + description: "Run 2.1 GiB ZIP64 boundary tests." force-supports: required: false type: string default: "" - description: "Comma-separated feature names to treat as supported regardless of what each SDK's `cli.sh supports` reports (e.g. 'chunky'). Use when evaluating a fix that has not been released yet: the version gates live in this repo and answer 'no' for exactly those unreleased builds, so the cells would otherwise skip. An unknown name fails the run rather than being ignored." + description: "Comma-separated feature names to treat as supported regardless of what each SDK's `cli.sh supports` reports. Use when evaluating a fix that has not been released yet: the version gates live in this repo and answer 'no' for exactly those unreleased builds, so the cells would otherwise skip. An unknown name fails the run rather than being ignored." workflow_call: inputs: platform-ref: @@ -211,11 +211,8 @@ jobs: } } - // Bench and ZIP64 hold the server on platform main independently - // of the platform lanes under test. Resolve that moving ref once - // here so every matrix job uses the same commit. Reuse the normal - // resolution when main was already requested; otherwise look up - // the branch without adding it to platform-tag-list. + // SDK focused jobs (`bench` and `zip64`) use platform main, not + // the platform lanes under test for `otdfctl` and `sdk`. let platformMainSha = versionData.platform ?.find(({ tag, sha, err }) => tag === 'main' && sha && !err) ?.sha; @@ -1068,7 +1065,7 @@ jobs: # test_zip64.py asserts that an offset actually landed in the window rather # than skipping, precisely so a mis-sized payload fails instead of passing. # - # Never on pull requests: an hour of multi-GiB IO per SDK is not a PR gate. + # Never on pull requests: an hour of multi-GiB IO per SDK is too slow for a PR gate. zip64: timeout-minutes: 90 runs-on: ubuntu-latest @@ -1279,11 +1276,8 @@ jobs: # holding a 2.1 GiB plaintext, ciphertext and decrypt output would # exhaust the scratch volume long before the last cell. # - # Note what is *not* here: --skip-released-pairs. The xct job derives - # that flag from SKIP_RELEASED_PAIRS to avoid re-testing release-against- - # release combinations, but a released java decryptor reading a 2.1 GiB - # container is the exact defect under test. Skipping it would leave this - # job green and blind. + # Note: `--skip-released-pairs` is not present, since these overnight jobs explicitly + # exist to find bugs in releases. - name: Run ZIP64 boundary tests id: zip64 run: |- From beccd3a5af6a060d29006a11f68c2c5b4755307c Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 9 Sep 2026 16:34:59 -0400 Subject: [PATCH 3/3] fix(xtest): close the silent-failure gaps in the ZIP64 test machinery The suite added for DSPX-4592 exists to make a silent gap loud, but its own tooling could still fail quietly. A review reproduced five such cases; this fixes those, the CI rate-limit failure blocking the branch, and one more gap the first live runs exposed: the writer half of DSPX-4590 finding 1 had no failing cell anywhere in the suite. zipinspect.py - Validate the ZIP64 extra field's declared length against the number of values the sentinels ask for. APPNOTE 4.5.3 makes the field positional, so a writer that sentinels only the offset but emits all three 64-bit values was decoded as offset=2254857830 instead of 2254857958 -- silently. Also reject an empty 0x0001 record, which reported has_zip64_extra=True with no values and then tripped the sentinel guard with a misleading message. - Guard every buffer read. A truncated EOCD, central-directory record, or ZIP64 EOCD leaked struct.error/OverflowError; the last also drove an unbounded f.read(cd_size) from unchecked bytes. All raise MalformedZipError. - central_directory() returns a CentralDirectory carrying cd_offset, cd_size and file_size. assert_zip64_above_4gib was a tautology -- a parsed value >= 2**32 can only have come from the extra field, so all three branches were unreachable as failures. The defect worth catching is a writer that truncates an offset mod 2**32, which parses perfectly; the new assert_offsets_are_consistent catches it against that ground truth. test_zip64.py / tdfs.py - Replace the dynamic xfail(strict=True) with an explicit pytest.raises branch. The marker worked, but a *node* marker absorbs every failure from that point on -- a KAS 500, a fixture error, a full disk all reported XFAIL. zip64_reader_xfail becomes zip64_reader_is_broken() -> bool. - Add the zip64-at-2gib feature and assert on it in the writer-conformance block: no entry may carry a *real* 32-bit value in [2**31, 2**32), the encoding every deployed signed-widening reader mishandles. It has to be gated rather than asserted outright, because that encoding is legal -- these fields are unsigned, and switching to the sentinel at 2 GiB is the convention java-sdk#393 adopted and web-sdk has always followed, not a spec rule. Without the gate DSPX-4590 finding 1 has no failing cell: when go starts sentineling, test_zip64_band_roundtrip merely switches which branch it asserts and stays green either way. js answers yes, so the check is live today; go answers no (platform#3981 open) and java answers no (#393 merged but unreleased, and a branch build reports v0.18.0 here), so an unreleased build is evaluated with XT_FORCE_SUPPORTS=zip64-at-2gib. sizes.py / conftest.py - Derive SIZE_ORDER from SIZES. --sizes validated names against SIZES while resolve_sizes filtered through SIZE_ORDER, so a name in one and not the other was accepted and then dropped: 8 skipped, 68 deselected, exit 0. A name that survives option validation but not the filter now raises. CI - Export GITHUB_TOKEN to the version-info step. github-script authenticates its own client, but the token never reaches the otdf-sdk-mgr child process it spawns, so all four resolutions shared the unauthenticated 60/hour per-IP budget and java, resolved last and paginating /releases, ran out. - Raise the zip64 execution guard from a floor of one to ZIP64_MIN_CELLS=3, counting only cells that ran. A floor of one passes when two of three decryptors have quietly stopped being exercised. - Assert /mnt has room before the job spends 40 minutes finding out otherwise. Verified: 76 offline tests pass; ruff and pyright clean. Each of the four reproduced defects now fails a test that was green before -- the transposed size decode, the SIZE_ORDER drift, a dropped bounds guard, and the extra-field length check. --- .github/workflows/xtest.yml | 68 ++++- spec/DSPX-4592.md | 100 +++++-- xtest/conftest.py | 14 +- xtest/sdk/go/cli.sh | 13 + xtest/sdk/java/cli.sh | 14 + xtest/sdk/js/cli.sh | 6 + xtest/sizes.py | 9 +- xtest/tdfs.py | 57 ++-- xtest/test_sizes_units.py | 32 ++ xtest/test_tdfs_units.py | 55 ++-- xtest/test_zip64.py | 71 ++++- xtest/test_zip64_units.py | 564 ++++++++++++++++++++++++++++++------ xtest/zipinspect.py | 299 ++++++++++++++----- 13 files changed, 1039 insertions(+), 263 deletions(-) diff --git a/.github/workflows/xtest.yml b/.github/workflows/xtest.yml index 5a8848bc..6bbd042b 100644 --- a/.github/workflows/xtest.yml +++ b/.github/workflows/xtest.yml @@ -42,7 +42,7 @@ on: required: false type: boolean default: false - description: "Run 2.1 GiB ZIP64 boundary tests." + description: "Run 2.1 GiB ZIP64 boundary tests (one runner per encrypting SDK, up to the job's 90m timeout each)." force-supports: required: false type: string @@ -167,6 +167,14 @@ jobs: - uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 - id: version-info uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 #v9.0.0 + # github-script authenticates its own `github` client, but the token + # never reaches the `otdf-sdk-mgr versions resolve` child process + # spawned below -- it reads GITHUB_TOKEN from the environment. Without + # this the four resolutions share the unauthenticated 60 requests/hour + # allowed per runner IP, and `java`, resolved last and paginating + # /releases, is the one that runs out. + env: + GITHUB_TOKEN: ${{ github.token }} with: script: | function htmlEscape(str) { @@ -1084,7 +1092,9 @@ jobs: # runner therefore installs every SDK, and the split is about disk and # wall clock rather than about what is installed: one encryptor per # runner means at most two cached 2.1 GiB ciphertexts (one per - # negotiated target_mode), not three. + # negotiated target_mode), not three. Those sit alongside the shared + # plaintext fixture and one decrypt output -- see the scratch-volume + # step below for the resulting budget. fail-fast: false matrix: sdk: [go, java, js] @@ -1095,12 +1105,19 @@ jobs: path: otdftests persist-credentials: false - # Plaintext 2.1 GiB + one cached ciphertext 2.1 GiB + one decrypt output - # at a time 2.1 GiB is ~6.5 GiB, which the workspace volume cannot be - # relied on to hold alongside three SDK toolchains. /mnt is the runner's - # large ephemeral disk; XT_TMP_DIR moves the fixtures there. + # Three 2.1 GiB files are live at once -- the plaintext fixture, the + # cached ciphertext, and the decrypt output -- so ~6.5 GiB, which the + # workspace volume cannot be relied on to hold alongside three SDK + # toolchains. /mnt is the runner's large ephemeral disk; XT_TMP_DIR moves + # the fixtures there. Keep this figure in step with the strategy comment + # above and with spec/DSPX-4592.md, which cites this step as the source. - name: Reclaim disk and stage a scratch volume id: scratch + env: + # ~6.5 GiB of fixtures plus headroom for the second cached ciphertext + # a target_mode renegotiation produces. Below this the job fails 40 + # minutes in, on a write, with no indication that disk was the cause. + ZIP64_MIN_SCRATCH_GIB: "10" run: |- # Toolchains this job does not use. Removing them buys ~25 GiB on the # workspace volume, which the platform containers and three SDK @@ -1110,6 +1127,14 @@ jobs: sudo mkdir -p /mnt/xtest-tmp sudo chown "$(id -u):$(id -g)" /mnt/xtest-tmp df -h / /mnt + avail=$(df -B1 --output=avail /mnt | tail -1) + need=$((ZIP64_MIN_SCRATCH_GIB * 1024 * 1024 * 1024)) + if [ "$avail" -lt "$need" ]; then + echo "::error::/mnt has $((avail / 1024 / 1024 / 1024)) GiB free," \ + "below the ${ZIP64_MIN_SCRATCH_GIB} GiB this job needs for" \ + "2.1 GiB fixtures. Failing now rather than mid-encrypt." + exit 1 + fi - name: load extra keys from file id: load-extra-keys @@ -1173,6 +1198,13 @@ jobs: ######## INSTALL EVERY SDK ############# # The go install doubles as otdfctl, which conftest.py loads at import # time to provision attributes and the KAS registry. + # + # Each of the three runners builds all three SDKs, so this job alone + # triples the nightly's BSR request rate. buf.build has already answered + # `resource_exhausted: too many requests` during a java prepare on the + # regular matrix; that flake is pre-existing, but it gets likelier here. + # If it becomes routine, retry inside setup-cli-tool rather than adding a + # wrapper per step -- the rate limit is per-org, not per-step. - name: Configure go sdk id: configure-go uses: ./otdftests/xtest/setup-cli-tool @@ -1316,11 +1348,18 @@ jobs: # ticket exists to close, and it is invisible in the job status. Parse # the junit XML rather than grepping the log: a skipped cell and a cell # that printed the word "skipped" are different things. + # + # The bar is a count, not "at least one". This runner encrypts once and + # decrypts with all three SDKs, so a healthy run is three cells; a floor + # of one passes when two of the three decryptors quietly stopped being + # exercised, which is the same erosion in slower motion. - name: Confirm the ZIP64 cells actually ran if: success() || failure() working-directory: otdftests/xtest env: ZIP64_SDK: ${{ matrix.sdk }} + # One cell per decrypting SDK. Raise this alongside --sdks-decrypt. + ZIP64_MIN_CELLS: "3" run: |- python3 - <<'PY' import os @@ -1328,6 +1367,7 @@ jobs: import xml.etree.ElementTree as ET sdk = os.environ["ZIP64_SDK"] + minimum = int(os.environ["ZIP64_MIN_CELLS"]) path = f"test-results/zip64-{sdk}.xml" try: cases = ET.parse(path).getroot().iter("testcase") @@ -1340,9 +1380,6 @@ jobs: s = c.find("skipped") if s is None: ran.append(name) - # pytest files xfail under , but an xfailed cell did - # encrypt 2.1 GiB and did attempt the decrypt -- it exercised the - # defect and predicted the outcome. That is coverage, not a gap. elif s.get("type") == "pytest.xfail": xfailed.append(name) else: @@ -1354,10 +1391,17 @@ jobs: for n in skipped: print(f" skipped: {n}") - if not ran and not xfailed: + # Only `ran` counts. test_zip64.py now asserts the reader failure + # inline instead of marking the node xfail, so an xfail here is no + # longer the expected shape of a pre-fix decryptor -- it is some + # other cell opting out, and opting out is what this step exists to + # catch. skip_chunky_skew can still produce genuine skips. + if len(ran) < minimum: sys.exit( - "::error::no ZIP64 cell executed. The job is green because " - "nothing ran, not because the 2-4 GiB band is conformant." + f"::error::only {len(ran)} of {minimum} expected ZIP64 cells " + f"executed ({len(xfailed)} xfailed, {len(skipped)} skipped, " + "listed above). A job this green because cells declined to " + "run says nothing about the 2-4 GiB band." ) PY diff --git a/spec/DSPX-4592.md b/spec/DSPX-4592.md index 9db9efdf..e84c18f3 100644 --- a/spec/DSPX-4592.md +++ b/spec/DSPX-4592.md @@ -156,16 +156,31 @@ class CentralDirectoryEntry: uses_zip64_for_offset / uses_zip64_for_sizes: bool # properties def signed_read_of_offset(self) -> int # reproduces the defect -def central_directory(path: Path) -> list[CentralDirectoryEntry] +@dataclass(frozen=True) +class CentralDirectory: + entries: list[CentralDirectoryEntry] + cd_offset / cd_size / file_size: int # ground truth from the container + +def central_directory(path: Path) -> CentralDirectory def entries_in_window(entries) -> list[CentralDirectoryEntry] -def assert_zip64_above_4gib(entries) -> None +def assert_offsets_are_consistent(cd: CentralDirectory) -> None +def assert_zip64_above_4gib(cd: CentralDirectory) -> None def describe(entries) -> str # xtest/tdfs.py JAVA_ZIP64_READER_FIX: tuple[int, int, int] -def zip64_reader_xfail(decrypt_sdk: SDK) -> pytest.MarkDecorator | None +def zip64_reader_is_broken(decrypt_sdk: SDK) -> bool ``` +`central_directory` returns the container rather than a bare list because the +only defect the `>= 2**32` check cannot see — a writer truncating an offset +mod `2**32` — is invisible in the records alone. Such a value parses perfectly; +what gives it away is that the entry's data would have to end after the central +directory begins, and `cd_offset` / `file_size` are the ground truth that says +so. `assert_offsets_are_consistent` is that check. Every malformed input the +parser can be handed raises `MalformedZipError` rather than leaking a +`struct.error` from a partial read. + Marker: `zip64`, deselected (not skipped) unless the session's resolved sizes reach `ZIP64_WINDOW_LOW`. Deselected, because a multi-GiB roundtrip has no business in the PR matrix and a skip would report it as a test that exists and @@ -190,17 +205,26 @@ one go-sdk emits is what the sibling ticket changes. So the test records and logs which was chosen and asserts only on the `>= 2**32` case, where there is no latitude. -**Writer checks run outside the reader's xfail.** A writer regression must not -hide behind a known reader bug: assertions under an xfail marker report XFAIL -and nobody looks. `add_marker` is therefore applied after the structural -assertions and immediately before the decrypt. +**A writer regression must not hide behind a known reader bug.** The first +cut applied `pytest.mark.xfail(strict=True)` to the node after the structural +assertions. That works, but a node marker absorbs *everything* from that point +on — a KAS 500, a fixture error, a full disk all report XFAIL, and nobody +looks at an XFAIL. So the reader defect is now asserted rather than marked: +when the decryptor is a pre-fix java and some entry carries a real 32-bit +value in the window, the test requires `subprocess.CalledProcessError` from +the decrypt via `pytest.raises`. Anything else — a success, or a failure +anywhere but the decrypt — is a failure, which is what the strict marker was +reaching for. `tdfs.zip64_reader_is_broken()` supplies the predicate. **Disk, not CPU, is binding.** `fixtures/encryption.py` caches ciphertexts session-wide and never deletes them, and `rt_file()` outputs were never deleted -either. One encryptor per runner plus `rt_file.unlink()` in a `finally` gives -~6.5 GiB peak; without both it is 15 GiB+ and the runner fails. `XT_TMP_DIR` -puts that on the runner's large ephemeral volume. If disk still binds, take a -larger runner. +either. One encryptor per runner plus `rt_file.unlink()` in a `finally` keeps +the peak to three live 2.1 GiB files; without both it is 15 GiB+ and the runner +fails. `XT_TMP_DIR` puts that on the runner's large ephemeral volume, and the +`zip64` job's scratch-volume step in `.github/workflows/xtest.yml` is the +source of the actual figure and asserts the volume can hold it — read the +budget there rather than restating it here. If disk still binds, take a larger +runner. **JVM and node heap.** The java shim pipes CLI stdout to a file with no `-Xmx` and the js shim is a bare `npx`. If either buffers the payload rather than @@ -215,9 +239,11 @@ combinations; here a released java decryptor is the entire point. ## Out of Scope -- The fixes themselves. go-sdk's 4 GiB → `MaxInt32` threshold change and - java-sdk #393 are the sibling tickets; this ticket only has to make them - demonstrable. +- The fixes themselves. go-sdk's 4 GiB → `MaxInt32` threshold change + (platform #3981) and java-sdk #393 are the sibling tickets; this ticket only + has to make them demonstrable — which for the writer half means the + `zip64-at-2gib` gate below, since the pre-fix encoding is legal and so + cannot be a standing assertion. - Sizes above `2**32`. `large` still exists and still works, but nothing new is asserted about it beyond the existing mandatory-ZIP64 check. - Non-ZTDF containers. `nano` has its own framing and none of this applies. @@ -237,20 +263,37 @@ Verified offline (no platform required): deflate ratio 1.000 over the first 64 MiB. - [x] The container's ZIP encoding is asserted on directly, so a failure names the SDK at fault instead of reporting "decrypt failed". -- [x] `zipinspect`'s own unit tests (18) run offline on every PR via +- [x] The supporting machinery's own unit tests run offline on every PR via `check.yml`, so the nightly's verdict does not rest on an unverified - parser. + parser: 40 in `test_zip64_units.py` (the parser, its malformed-input + handling, the conformance assertions, and the collection filter) and 20 + in `test_sizes_units.py`, in well under a second. - [x] The coverage runs on the nightly cron and via `workflow_dispatch` (`run-zip64`), never on a PR, with its own `timeout-minutes: 90`. - [x] Execution is confirmed by parsing the junit XML, not by grepping the - log; the job fails if no cell executed. Verified against a sample - report, including that `xfail` is counted as executed and a plain - `skip` is not. + log; the job fails unless `ZIP64_MIN_CELLS` (3, one per decrypting SDK) + cells actually ran. A floor of one would pass a run in which two of the + three decryptors had quietly stopped being exercised. Verified against + sample reports in both directions. +- [x] The scratch volume is asserted to have room before the job spends 40 + minutes discovering otherwise mid-encrypt. - [x] The multi-segment defect the 2.1 GiB run turned up is covered on the **PR gate**, not only the nightly: `chunky` (5 MiB) is a `feature_type` and a size, and `test_chunky_roundtrip` runs in the standard `xct` job with no CI change. It costs one 5 MiB encrypt and decrypt per pair; the fixture generates in 0.05 s. +- [x] The writer half of the interop defect has a red-to-green witness. + `zip64-at-2gib` is a `feature_type` gating an assertion that no entry + carries a *real* 32-bit value in `[2**31, 2**32)` — the encoding every + deployed signed-widening reader mishandles. It has to be gated rather + than asserted outright, because such a value is legal: these fields are + unsigned, and switching at 2 GiB is the convention java-sdk #393 adopted + and web-sdk has always followed, not a spec rule. web-sdk answers yes; + go and java answer no until the fix releases, so + `XT_FORCE_SUPPORTS=zip64-at-2gib` against a branch build is what turns + the cell into a real verdict. Without this, DSPX-4590 finding 1 has no + failing cell anywhere in xtest — `test_zip64_band_roundtrip` merely + switches which branch it asserts, and stays green either way. Verified by the first live run ([33771257397](https://github.com/opentdf/tests/actions/runs/33771257397), @@ -262,10 +305,12 @@ Verified by the first live run - [x] Every writer put an offset in the window — 2254888013 (go), 2254888021 (java), 2254918149 (js) — so `_assert_reaches_the_window` held in practice, not just by construction. -- [x] A pre-#393 java decryptor reports `XFAIL` with the DSPX-4592 reason - against all three writers. The failure is the sign-extension path - (`FileChannel.position()` throwing `IllegalArgumentException` on a - negative offset), not an OOM. +- [x] A pre-#393 java decryptor fails to read all three writers' containers, + and the failure is the sign-extension path (`FileChannel.position()` + throwing `IllegalArgumentException` on a negative offset), not an OOM. + That run reported it as `XFAIL`; the test now asserts the failure + instead, so the same outcome reports as a pass — see "A writer + regression must not hide behind a known reader bug" above. - [x] No OOM in either shim with the heap headroom the job sets. - [x] `xct (main, go@main)` passed on the same run, so the `--sizes` refactor did not disturb the default path. @@ -278,7 +323,8 @@ the ticket anticipated. **java main is still pre-fix**, as expected — java-sdk#393 is open, not merged (checked 2026-09-03). So main is the baseline, and its writer emits the -identical container to v0.18.0: +identical container to v0.18.0 (an excerpt of `zipinspect.describe()`; its +`csize` column is elided here because it was not transcribed from the run): ``` entry offset raw usize zip64 signed @@ -294,9 +340,9 @@ its own container at `ZipReader$Entry.getData:167`, and go's at `ZipReader.:294`, both `FileChannel.position()` rejecting a negative argument. -Because `zip64_reader_xfail` keys on a released semver, java@main is *not* -xfailed and fails hard. That is the honest report — main is broken — but it -means the nightly stays red until #393 actually lands. +Because `zip64_reader_is_broken` keys on a released semver, java@main is *not* +treated as a known-broken reader and fails hard. That is the honest report — +main is broken — but it means the nightly stays red until #393 actually lands. **web-sdk is conformant, and confirms the parser.** js emits the `0xFFFFFFFF` sentinel plus an extra field for both entries, and `zipinspect` diff --git a/xtest/conftest.py b/xtest/conftest.py index 3dfc865f..c2b8d5c7 100644 --- a/xtest/conftest.py +++ b/xtest/conftest.py @@ -126,7 +126,19 @@ def sizes_opt_type(v: str) -> list[str]: ) # Cheapest first, so a fan-out run reports its fast cells before spending # minutes on a multi-GiB one. - return [n for n in sizes.SIZE_ORDER if n in set(names)] + ordered = [n for n in sizes.SIZE_ORDER if n in set(names)] + # SIZE_ORDER is derived from SIZES, so this cannot fire today. It is here + # because the failure it guards is invisible: a name validated against + # SIZES but absent from SIZE_ORDER is dropped here, which empties the + # parameter set, which pytest reports as "got empty parameter set" -- a + # *skip*, exit 0. A whole matrix disappears and the run stays green. + dropped = sorted(set(names) - set(ordered)) + if dropped: + raise argparse.ArgumentTypeError( + f"size(s) {', '.join(dropped)} are in SIZES but missing from " + "SIZE_ORDER; they would be silently dropped from the run" + ) + return ordered _SIZES_KEY = pytest.StashKey[list[str]]() diff --git a/xtest/sdk/go/cli.sh b/xtest/sdk/go/cli.sh index 2b2a8638..c127b289 100755 --- a/xtest/sdk/go/cli.sh +++ b/xtest/sdk/go/cli.sh @@ -132,6 +132,19 @@ if [ "$1" == "supports" ]; then echo "chunky unsupported: see DSPX-4590" exit 1 ;; + zip64-at-2gib) + # Switch to the ZIP64 sentinel plus extra field at 2 GiB rather than at + # 4 GiB, so a reader that widens the central-directory fields with a + # signed read can still open the container. Every go build to date gates + # on ^uint32(0) and so writes a real 32-bit value across the whole + # 2-4 GiB band. Fix tracked as DSPX-4590 finding 1 (platform#3981, open); + # turn this into a version gate when it releases. + # + # Explicit rather than falling through to "Unknown feature" so that a + # typo'd feature name in tdfs.py cannot pass for a known-missing one. + echo "zip64-at-2gib unsupported: see DSPX-4590" + exit 1 + ;; *) echo "Unknown feature: $2" exit 2 diff --git a/xtest/sdk/java/cli.sh b/xtest/sdk/java/cli.sh index afdca268..b701f950 100755 --- a/xtest/sdk/java/cli.sh +++ b/xtest/sdk/java/cli.sh @@ -160,6 +160,20 @@ if [ "$1" == "supports" ]; then echo "chunky unsupported: see DSPX-4589" exit 1 ;; + zip64-at-2gib) + # Switch to the ZIP64 sentinel plus extra field at 2 GiB rather than at + # 4 GiB. java-sdk adopted MAX_NON_ZIP64_VALUE = Integer.MAX_VALUE in + # java-sdk#393, merged 2026-09-03 and not in any release through v0.18.0 + # -- and a branch build reports the last released version here, so this + # answers no for java@main too. Evaluate such a build with + # XT_FORCE_SUPPORTS=zip64-at-2gib; turn this into a version gate when the + # fix releases. + # + # Explicit rather than falling through to "Unknown feature" so that a + # typo'd feature name in tdfs.py cannot pass for a known-missing one. + echo "zip64-at-2gib unsupported: needs the release carrying java-sdk#393" + exit 1 + ;; *) echo "Unknown feature: $2" exit 2 diff --git a/xtest/sdk/js/cli.sh b/xtest/sdk/js/cli.sh index f805e967..f2d0b111 100755 --- a/xtest/sdk/js/cli.sh +++ b/xtest/sdk/js/cli.sh @@ -126,6 +126,12 @@ if [[ "$1" == "supports" ]]; then # test. See DSPX-4591. exit 0 ;; + zip64-at-2gib) + # web-sdk writes the ZIP64 sentinel unconditionally, so it is trivially + # on the right side of the 2 GiB switch point. Predates any version we + # test. See DSPX-4591. + exit 0 + ;; *) echo "Unknown feature: $2" exit 2 diff --git a/xtest/sizes.py b/xtest/sizes.py index 6f44551a..32184049 100644 --- a/xtest/sizes.py +++ b/xtest/sizes.py @@ -60,6 +60,7 @@ #: largest of them with room to spare. 2 MiB would only do it for web-sdk. CHUNKY_BYTES = 5 * 2**20 +#: Declared cheapest first, because :data:`SIZE_ORDER` is derived from it. SIZES: dict[str, int] = { "small": 128, "chunky": CHUNKY_BYTES, @@ -68,7 +69,13 @@ } #: Order to emit parametrized sizes in, cheapest first. -SIZE_ORDER: tuple[str, ...] = ("small", "chunky", "medium", "large") +#: +#: Derived, not restated. ``resolve_sizes`` filters the requested sizes +#: through this while ``--sizes`` validates them against :data:`SIZES`, so a +#: name in one and not the other is accepted on the command line and then +#: silently dropped -- which empties the parameter set and reports +#: ``got empty parameter set`` as a *skip*, exit 0. +SIZE_ORDER: tuple[str, ...] = tuple(SIZES) def in_zip64_window(n: int) -> bool: diff --git a/xtest/tdfs.py b/xtest/tdfs.py index c16abc72..3aeafcda 100644 --- a/xtest/tdfs.py +++ b/xtest/tdfs.py @@ -169,6 +169,21 @@ def is_sdk_type(val: str) -> TypeIs[sdk_type]: "multikao", "ns_grants", "obligations", + # Writer-side: switch to the ZIP64 sentinel plus extra field at 2 GiB + # rather than at 4 GiB. + # + # A real 32-bit value in [2**31, 2**32) is *legal* -- the central-directory + # size and offset fields are unsigned -- so this is a cross-SDK interop + # convention rather than a spec rule, which is why it is a feature gate and + # not an unconditional assertion on every writer. A reader that widens + # those fields with a signed read sees a negative number, and that + # describes every java-sdk released to date. web-sdk always writes ZIP64; + # java-sdk adopted the 2 GiB switch in java-sdk#393; go-sdk still switches + # at 4 GiB. See DSPX-4590 finding 1. + # + # Only observable from a payload that reaches the window; hence + # sizes.MEDIUM_BYTES. + "zip64-at-2gib", ] @@ -882,9 +897,9 @@ def elides_segment_sizes(ct_file: Path) -> bool: def skip_chunky_skew(ct_file: Path, decrypt_sdk: SDK): """Skip if ``ct_file`` needs segment-size defaulting and the reader lacks it. - A skip and not an xfail: this cell runs on the PR gate, where a + A skip and not an asserted failure: this cell runs on the PR gate, where a permanently-red job trains people to ignore it, and unlike - :func:`zip64_reader_xfail` it needs no dated guess about which release + :func:`zip64_reader_is_broken` it needs no dated guess about which release carries the fix. The cost is that it stays skipped until somebody edits @@ -914,35 +929,29 @@ def skip_chunky_skew(ct_file: Path, decrypt_sdk: SDK): #: seeks to nonsense. A 2.1 GiB payload puts the manifest's offset exactly #: there. See DSPX-4592. #: -#: Keep this honest. Set too high, a fixed release keeps reporting XFAIL and -#: a genuine regression hides behind it; set too low, the strict xfail turns -#: every pre-fix cell into a hard failure. Update it when the release with -#: #393 actually ships, not when the PR merges. +#: Keep this honest, and note that both directions of getting it wrong fail +#: the run rather than hiding: set too high, a release that does carry the fix +#: reads the container correctly and the "must fail" assertion fires; set too +#: low, a pre-fix release is expected to succeed and its real failure is +#: reported as a defect. Update it when the release with #393 actually ships, +#: not when the PR merges. JAVA_ZIP64_READER_FIX = (0, 19, 0) -def zip64_reader_xfail(decrypt_sdk: SDK) -> pytest.MarkDecorator | None: - """An xfail marker for decryptors known to mishandle the 2-4 GiB band. +def zip64_reader_is_broken(decrypt_sdk: SDK) -> bool: + """True for decryptors known to mishandle a real 32-bit value in the band. - ``strict=True`` deliberately. The point of this test is to flip to green - when the sibling fixes land: an XPASS here means a build we believed - broken now reads the container correctly, and that should fail the run so - somebody comes and deletes this predicate rather than leaving a - permanently-XFAIL cell that nobody reads. + The caller asserts the decrypt *fails* for these, rather than marking the + cell xfail. That is deliberate on both counts: a node-level xfail would + swallow every unrelated failure in the rest of the cell, and asserting the + failure means a build that has quietly been fixed turns the cell red so + somebody comes and deletes this predicate. - Branch builds (``main``) have no semver and are never marked -- they are - the builds expected to carry the fix. + Branch builds (``main``) have no semver and are never assumed broken -- + they are the builds expected to carry the fix. """ sv = decrypt_sdk.semver() - if decrypt_sdk.sdk == "java" and sv is not None and sv < JAVA_ZIP64_READER_FIX: - return pytest.mark.xfail( - strict=True, - reason=( - f"DSPX-4592: {decrypt_sdk} predates java-sdk#393; readInt() " - "sign-extends the manifest's central-directory offset" - ), - ) - return None + return decrypt_sdk.sdk == "java" and sv is not None and sv < JAVA_ZIP64_READER_FIX def _parse_semver(version: str) -> tuple[int, int, int] | None: diff --git a/xtest/test_sizes_units.py b/xtest/test_sizes_units.py index 67db8363..dde62a47 100644 --- a/xtest/test_sizes_units.py +++ b/xtest/test_sizes_units.py @@ -66,6 +66,38 @@ def test_chunky_stays_cheap(self): assert not sizes.in_zip64_window(sizes.CHUNKY_BYTES) +class TestSizeOrder: + """The two tables that must not drift apart. + + ``--sizes`` validates names against ``SIZES``; ``sizes_opt_type`` then + orders them through ``SIZE_ORDER``. A name in the first and not the second + is accepted on the command line and dropped immediately after, which + empties the parameter set -- and pytest reports an empty parameter set as + a *skip*, exit 0. An entire matrix disappears and the run stays green. + """ + + def test_every_size_is_ordered(self): + assert set(sizes.SIZE_ORDER) == set(sizes.SIZES) + + def test_ordering_is_cheapest_first(self): + """The order is the run order; an expensive size must not go first.""" + by_bytes = [sizes.SIZES[n] for n in sizes.SIZE_ORDER] + assert by_bytes == sorted(by_bytes) + + def test_a_size_missing_from_the_order_is_rejected_not_dropped( + self, monkeypatch: pytest.MonkeyPatch + ): + """The drift above, made to happen, so the failure mode is a message. + + Patching ``SIZE_ORDER`` short is the only way to reach this branch -- + it is derived from ``SIZES`` precisely so the drift cannot occur -- but + the guard is what makes a future hand-written ``SIZE_ORDER`` loud. + """ + monkeypatch.setattr(sizes, "SIZE_ORDER", ("small", "chunky", "large")) + with pytest.raises(argparse.ArgumentTypeError, match="missing from"): + conftest.sizes_opt_type("small,medium") + + class TestSizesOptionParsing: def test_dedups_and_orders_cheapest_first(self): assert conftest.sizes_opt_type("large,small,small") == ["small", "large"] diff --git a/xtest/test_tdfs_units.py b/xtest/test_tdfs_units.py index 8bb22ad0..9774ee6e 100644 --- a/xtest/test_tdfs_units.py +++ b/xtest/test_tdfs_units.py @@ -1,7 +1,7 @@ """Offline tests for tdfs.py's anti-vacuous-green machinery (DSPX-4592, DSPX-4638). No platform, no SDK, no subprocess. ``_parse_forced_supports``, -``zip64_reader_xfail``, and ``skip_chunky_skew`` are all safeguards built +``zip64_reader_is_broken``, and ``skip_chunky_skew`` are all safeguards built specifically to stop a real regression from hiding behind a skip or a stale xfail -- so they are worth testing on their own, the same way the ZIP64 parser they sit next to is tested in ``test_zip64_units.py``. @@ -36,33 +36,50 @@ def test_unknown_name_raises(self): tdfs._parse_forced_supports("hexles") -# --- tdfs.zip64_reader_xfail -------------------------------------------------- +# --- tdfs.zip64_reader_is_broken ---------------------------------------------- def _stub_sdk(sdk: str, semver: tuple[int, int, int] | None) -> tdfs.SDK: - """A duck-typed stand-in exposing only what zip64_reader_xfail reads.""" + """A duck-typed stand-in exposing only what zip64_reader_is_broken reads.""" return cast(tdfs.SDK, SimpleNamespace(sdk=sdk, semver=lambda: semver)) -class TestZip64ReaderXfail: - def test_pre_fix_java_gets_a_strict_xfail(self): - stub = _stub_sdk("java", (0, 18, 0)) - marker = tdfs.zip64_reader_xfail(stub) - assert marker is not None - assert marker.mark.kwargs["strict"] is True +class TestZip64ReaderIsBroken: + """Which decryptors ``test_zip64.py`` requires a decrypt failure from. - def test_post_fix_java_is_not_marked(self): - stub = _stub_sdk("java", tdfs.JAVA_ZIP64_READER_FIX) - assert tdfs.zip64_reader_xfail(stub) is None + Only java before #393, and only when it reports a release version. Each + "False" here is a build the test holds to a successful roundtrip, so a + predicate that were too generous would turn a real regression into an + expected failure. + """ - def test_non_java_sdk_is_never_marked(self): - stub = _stub_sdk("go", (0, 1, 0)) - assert tdfs.zip64_reader_xfail(stub) is None + def test_pre_fix_java_is_broken(self): + assert tdfs.zip64_reader_is_broken(_stub_sdk("java", (0, 18, 0))) - def test_branch_build_is_never_marked(self): - """A branch build (e.g. 'main') has no semver and is expected to carry the fix.""" - stub = _stub_sdk("java", None) - assert tdfs.zip64_reader_xfail(stub) is None + def test_the_fix_release_itself_is_not(self): + """Boundary: the constant names the first release *with* the fix.""" + assert not tdfs.zip64_reader_is_broken( + _stub_sdk("java", tdfs.JAVA_ZIP64_READER_FIX) + ) + + def test_a_later_java_is_not(self): + major, minor, patch = tdfs.JAVA_ZIP64_READER_FIX + assert not tdfs.zip64_reader_is_broken( + _stub_sdk("java", (major, minor, patch + 1)) + ) + + def test_another_sdk_is_never_broken(self): + """The defect is java's ZipReader; an old go is not a stand-in for it.""" + assert not tdfs.zip64_reader_is_broken(_stub_sdk("go", (0, 1, 0))) + + def test_a_branch_build_is_not(self): + """A branch build (e.g. 'main') has no semver and is expected to carry the fix. + + If it does not, the cell fails loudly -- which is the correct report + for a branch that has regressed, and is exactly what happened on the + first live run against java@main. + """ + assert not tdfs.zip64_reader_is_broken(_stub_sdk("java", None)) # --- tdfs.elides_segment_sizes / tdfs.skip_chunky_skew ------------------------ diff --git a/xtest/test_zip64.py b/xtest/test_zip64.py index 01eabefe..583852ea 100644 --- a/xtest/test_zip64.py +++ b/xtest/test_zip64.py @@ -18,6 +18,7 @@ import filecmp import logging +import subprocess from pathlib import Path import pytest @@ -62,7 +63,6 @@ def _assert_reaches_the_window( def test_zip64_band_roundtrip( - request: pytest.FixtureRequest, encrypt_sdk: tdfs.SDK, decrypt_sdk: tdfs.SDK, pt_file: Path, @@ -87,7 +87,8 @@ def test_zip64_band_roundtrip( attr_values=attribute_default_rsa.value_fqns, ) - entries = zipinspect.central_directory(ct_file) + cd = zipinspect.central_directory(ct_file) + entries = cd.entries logger.info( "%s wrote %s at size=%s (%d bytes):\n%s", encrypt_sdk, @@ -97,35 +98,77 @@ def test_zip64_band_roundtrip( zipinspect.describe(entries), ) - # Writer conformance first, and outside the reader's xfail below. A - # writer regression must not hide behind a known reader bug: if these - # fail under an xfail marker the cell reports XFAIL and nobody looks. + # Writer conformance first, and outside the reader branch below. A writer + # regression must not be reported as the known reader bug. _assert_reaches_the_window(entries, pt_file, encrypt_sdk) - zipinspect.assert_zip64_above_4gib(entries) + zipinspect.assert_offsets_are_consistent(cd) + zipinspect.assert_zip64_above_4gib(cd) in_window = zipinspect.entries_in_window(entries) logger.info( - "%s: %d entr%s in [2**31, 2**32); zip64 extra field used for %s", + "%s: %d entr%s in [2**31, 2**32); ZIP64 sentinel used for the offset of %s", encrypt_sdk, len(in_window), "y" if len(in_window) == 1 else "ies", - [e.name for e in in_window if e.has_zip64_extra] or "none", + [e.name for e in in_window if e.uses_zip64_for_offset] or "none", ) + # A real 32-bit value in the window is legal -- these fields are unsigned + # -- so this is gated rather than asserted outright. It is the cross-SDK + # convention java-sdk#393 adopted and web-sdk has always followed: sentinel + # from 2 GiB up, so a reader that widens the field with a signed read still + # gets a usable number. Held only against a writer that claims to do it, + # which is what makes this cell a red-to-green witness for the fix rather + # than a standing failure against a writer that has not landed it yet. + raw_in_window = zipinspect.entries_with_raw_values_in_window(entries) + if encrypt_sdk.supports("zip64-at-2gib"): + assert not raw_in_window, ( + f"{encrypt_sdk} reports the 2 GiB ZIP64 switch but wrote " + f"{len(raw_in_window)} entr" + f"{'y' if len(raw_in_window) == 1 else 'ies'} carrying a real " + f"32-bit value in [2**31, 2**32): " + f"{[e.name for e in raw_in_window]}. Every deployed reader that " + f"widens these fields signed reads those as negative.\n" + + zipinspect.describe(entries) + ) + # Keep the independent segment-defaulting incompatibility out of the # ZIP64 result. In particular, web-sdk uses ZIP64 sentinels in this band, # so those containers do not exercise Java's signed 32-bit read defect. tdfs.skip_chunky_skew(ct_file, decrypt_sdk) - # Apply the reader xfail only when a real 32-bit value (not the sentinel) - # exercises the signed-risk window. Writer conformance has already been - # checked above, so a failure from this point belongs to the reader. - if zipinspect.entries_with_raw_values_in_window(entries): - if mark := tdfs.zip64_reader_xfail(decrypt_sdk): - request.node.add_marker(mark) + # Expect the reader defect only when a real 32-bit value (not the + # sentinel) exercises the signed-risk window. Writer conformance has + # already been checked above, so a failure from this point is the + # reader's. + # + # Asserted rather than xfailed on purpose. A dynamic + # ``xfail(strict=True)`` marker does work here, but it is a *node* marker: + # it would absorb every remaining failure in the cell -- a KAS error, a + # timeout, a full scratch volume -- and report the lot as a confirmed + # prediction about ZIP64. + expect_reader_defect = bool(raw_in_window) and tdfs.zip64_reader_is_broken( + decrypt_sdk + ) rt_file = encrypted_tdf.rt_file(ct_file, decrypt_sdk) try: + if expect_reader_defect: + # No assertion on the message. There is no pre-fix java build in + # this repo to check the wording against, and a guessed pattern + # would fail the first real run for the wrong reason. Once the + # nightly has produced one, tighten this to match it. + with pytest.raises(subprocess.CalledProcessError) as failure: + decrypt_sdk.decrypt(ct_file, rt_file, "ztdf", expect_error=True) + logger.info( + "%s failed to read %s as expected -- it holds a real 32-bit " + "value in [2**31, 2**32) and this build predates the fix:\n%s", + decrypt_sdk, + ct_file.name, + (failure.value.output or b"").decode(errors="replace"), + ) + return + decrypt_sdk.decrypt(ct_file, rt_file, "ztdf") # shallow=False explicitly: the default compares a stat signature # first and only falls through to a byte compare because the mtimes diff --git a/xtest/test_zip64_units.py b/xtest/test_zip64_units.py index bebc679f..a935e294 100644 --- a/xtest/test_zip64_units.py +++ b/xtest/test_zip64_units.py @@ -36,6 +36,9 @@ def cen_record( zip64_offset: int | None = None, zip64_usize: int | None = None, zip64_csize: int | None = None, + zip64_disk_start: int | None = None, + extra_prefix: bytes = b"", + zip64_extra_len_override: int | None = None, ) -> bytes: """One central-directory header, with an optional ZIP64 extra field. @@ -43,6 +46,11 @@ def cen_record( fixed order (uncompressed, compressed, offset); pass them only for the fields whose 32-bit slot holds the sentinel, which is the same contract the parser relies on. + + ``extra_prefix`` puts another extra-field record ahead of the ZIP64 one, + and ``zip64_extra_len_override`` lies about the ZIP64 record's length. + Both exist to build inputs a conformant writer would not: the first is + what real writers actually emit, the second is the misparse under test. """ extra = b"" body = b"" @@ -52,8 +60,14 @@ def cen_record( body += struct.pack(" Path: +def _write_at(path: Path, cd_offset: int, trailer: bytes) -> Path: + """Write ``trailer`` at ``cd_offset``, leaving the gap before it sparse. + + The gap stands in for entry data the parser never reads. Making it a hole + rather than real bytes is what lets these tests build a container whose + central directory genuinely sits past 4 GiB -- ``file_size`` and + ``cd_offset`` are then real ground truth for the consistency assertions, + at a cost of one filesystem block. Sparse files are supported on both APFS + and ext4, which is macOS dev boxes and the CI runners. + """ + with path.open("wb") as f: + if cd_offset: + f.truncate(cd_offset) + f.seek(cd_offset) + f.write(trailer) + return path + + +def synth_zip(path: Path, records: list[bytes], *, cd_offset: int = 0) -> Path: """Write a container that is nothing but a central directory and an EOCD. The parser never reads entry data, so leaving it out keeps these tests - instant while exercising every field it does read. + instant while exercising every field it does read. Pass ``cd_offset`` when + the test needs the directory to sit at a plausible place after the entry + data, which the consistency assertions check against. """ + if cd_offset >= zipinspect.ZIP64_SENTINEL_32: + raise ValueError( + f"cd_offset {cd_offset} does not fit the 32-bit EOCD; a directory " + "this far into the file has to be located through the ZIP64 EOCD, " + "so use synth_zip64_eocd" + ) cd = b"".join(records) - cd_offset = 0 eocd = ( b"PK\x05\x06" + struct.pack(" Path: """Same, but located through a ZIP64 EOCD record and its locator. @@ -100,18 +139,20 @@ def synth_zip64_eocd( central directory sits past 4 GiB has to be read. """ cd = b"".join(records) - cd_offset = 0 eocd64 = ( b"PK\x06\x06" + struct.pack(" object: + return self._options.get(name, default) + + +def _fake_item(name: str, *, marker: str | None = None, size: str | None = None): + params = {} if size is None else {"size": size} + return cast( + pytest.Item, + SimpleNamespace( + name=name, + callspec=SimpleNamespace(params=params), + get_closest_marker=lambda want, m=marker: want if want == m else None, + ), + ) + + +def _run_filter(config: _FakeConfig, items: list[pytest.Item]) -> list[str]: + conftest.pytest_collection_modifyitems(cast(pytest.Config, config), items) + return [i.name for i in items] + + +class TestZip64Deselection: + """The collection filter itself, not just the predicate it delegates to.""" + + def test_a_default_run_drops_the_zip64_cells(self): + items = [_fake_item("zip64[small]", marker="zip64", size="small")] + config = _FakeConfig() + + assert _run_filter(config, items) == [] + assert [i.name for i in config.deselected] == ["zip64[small]"] + + def test_a_medium_run_keeps_them(self): + items = [_fake_item("zip64[medium]", marker="zip64", size="medium")] + config = _FakeConfig(sizes=["medium"]) + + assert _run_filter(config, items) == ["zip64[medium]"] + assert config.deselected == [] + + def test_a_mixed_run_keeps_only_the_cells_that_reach_the_window(self): + """The reason the filter is per-item rather than per-session. + + ``--sizes small,medium`` collects both arms of every size-parametrized + zip64 test. Judging by the session would keep the 128-byte arm, which + passes green without touching the code path under test. + """ + items = [ + _fake_item("zip64[small]", marker="zip64", size="small"), + _fake_item("zip64[medium]", marker="zip64", size="medium"), + _fake_item("roundtrip[small]", size="small"), + ] + config = _FakeConfig(sizes=["small", "medium"]) + + assert _run_filter(config, items) == ["zip64[medium]", "roundtrip[small]"] + assert [i.name for i in config.deselected] == ["zip64[small]"] + + def test_benchmarks_need_their_own_opt_in(self): + """A medium run is not a benchmark run; the two markers are independent.""" + items = [ + _fake_item("bench", marker="benchmark", size="medium"), + _fake_item("zip64", marker="zip64", size="medium"), + ] + config = _FakeConfig(sizes=["medium"]) + + assert _run_filter(config, items) == ["zip64"] + assert [i.name for i in config.deselected] == ["bench"] + + # --- zipinspect.py ----------------------------------------------------------- @@ -159,11 +285,16 @@ def test_reads_a_real_zip(self, tmp_path: Path): z.writestr("0.payload", b"a" * 4096) z.writestr("0.manifest.json", b"{}") - entries = zipinspect.central_directory(p) - assert [e.name for e in entries] == ["0.payload", "0.manifest.json"] + cd = zipinspect.central_directory(p) + assert [e.name for e in cd.entries] == ["0.payload", "0.manifest.json"] with zipfile.ZipFile(p) as z: expected = {i.filename: i.header_offset for i in z.infolist()} - assert {e.name: e.local_header_offset for e in entries} == expected + assert {e.name: e.local_header_offset for e in cd.entries} == expected + # The ground truth the consistency assertions need, against a + # container built by something other than this file's helpers. + assert cd.file_size == p.stat().st_size + assert cd.cd_offset + cd.cd_size < cd.file_size + zipinspect.assert_offsets_are_consistent(cd) def test_local_header_zip64_is_not_mistaken_for_central_directory_zip64( self, tmp_path: Path @@ -181,7 +312,7 @@ def test_local_header_zip64_is_not_mistaken_for_central_directory_zip64( with z.open("0.payload", "w", force_zip64=True) as f: f.write(b"b" * 8192) - (entry,) = zipinspect.central_directory(p) + (entry,) = zipinspect.central_directory(p).entries assert entry.uncompressed_size == 8192 assert not entry.has_zip64_extra assert not entry.uses_zip64_for_sizes @@ -198,9 +329,9 @@ def test_reads_a_zip64_end_of_central_directory(self, tmp_path: Path): cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES), ] p = synth_zip64_eocd(tmp_path / "z64-eocd.zip", records) - entries = zipinspect.central_directory(p) - assert [e.name for e in entries] == ["0.payload", "0.manifest.json"] - assert entries[1].raw_local_header_offset == MEDIUM_BYTES + cd = zipinspect.central_directory(p) + assert [e.name for e in cd.entries] == ["0.payload", "0.manifest.json"] + assert cd.entries[1].raw_local_header_offset == MEDIUM_BYTES def test_rejects_a_locator_pointing_at_nothing(self, tmp_path: Path): p = synth_zip64_eocd( @@ -226,8 +357,7 @@ def test_raw_value_in_the_window_is_preserved(self, tmp_path: Path): cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES + 64), ], ) - entries = zipinspect.central_directory(p) - manifest = entries[1] + manifest = zipinspect.central_directory(p).entries[1] assert manifest.raw_local_header_offset == MEDIUM_BYTES + 64 assert manifest.local_header_offset == MEDIUM_BYTES + 64 assert not manifest.has_zip64_extra @@ -244,7 +374,7 @@ def test_signed_read_of_a_windowed_offset_goes_negative(self, tmp_path: Path): tmp_path / "signed.zip", [cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES)], ) - (entry,) = zipinspect.central_directory(p) + (entry,) = zipinspect.central_directory(p).entries assert entry.signed_read_of_offset() < 0 assert entry.signed_read_of_offset() == MEDIUM_BYTES - ZIP64_WINDOW_HIGH @@ -254,7 +384,7 @@ def test_signed_read_is_harmless_below_the_window(self, tmp_path: Path): p = synth_zip( tmp_path / "safe.zip", [cen_record("0.manifest.json", raw_offset=offset)] ) - (entry,) = zipinspect.central_directory(p) + (entry,) = zipinspect.central_directory(p).entries assert entry.signed_read_of_offset() == offset def test_sentinel_resolves_through_the_extra_field(self, tmp_path: Path): @@ -270,11 +400,62 @@ def test_sentinel_resolves_through_the_extra_field(self, tmp_path: Path): ) ], ) - (entry,) = zipinspect.central_directory(p) + (entry,) = zipinspect.central_directory(p).entries assert entry.local_header_offset == true_offset assert entry.uses_zip64_for_offset assert entry.has_zip64_extra + def test_each_zip64_value_lands_in_its_own_field(self, tmp_path: Path): + """The extra field is positional, so a transposed decode must be caught. + + Every value here is distinct and none is a round number: if the + uncompressed and compressed slots were swapped, or the offset read + from the wrong one, the numbers below would not match. An earlier + version of this suite asserted only that parsing *succeeded*, and a + deliberate transposition of the two size fields kept every test green. + """ + p = synth_zip( + tmp_path / "all-three.zip", + [ + cen_record( + "0.payload", + raw_offset=ZIP64_SENTINEL_32, + raw_usize=ZIP64_SENTINEL_32, + raw_csize=ZIP64_SENTINEL_32, + zip64_usize=5 * 2**30 + 11, + zip64_csize=5 * 2**30 + 22, + zip64_offset=5 * 2**30 + 33, + ) + ], + ) + (entry,) = zipinspect.central_directory(p).entries + assert entry.uncompressed_size == 5 * 2**30 + 11 + assert entry.compressed_size == 5 * 2**30 + 22 + assert entry.local_header_offset == 5 * 2**30 + 33 + + def test_skips_a_foreign_extra_field_record(self, tmp_path: Path): + """A ZIP64 record after an unrelated one must still be found. + + Real writers put an extended-timestamp record (0x5455) in the extra + field, so the skip-and-continue path is the common case in the wild + rather than an edge case. + """ + timestamp = struct.pack("4 GiB offset with no ZIP64 encoding, - # which is the state a non-conformant writer would leave behind. - broken = [ - zipinspect.CentralDirectoryEntry( - name="0.manifest.json", - raw_compressed_size=0, - raw_uncompressed_size=0, - raw_local_header_offset=12345, - compressed_size=0, - uncompressed_size=0, - local_header_offset=5 * 2**30, - has_zip64_extra=False, - ) - ] - zipinspect.assert_zip64_above_4gib(entries) # the conformant one passes - with pytest.raises(AssertionError, match="ZIP64 sentinel"): - zipinspect.assert_zip64_above_4gib(broken) - - def test_above_4gib_uncompressed_size_without_the_sentinel_fails(self): - """The size branches had no test of their own; the offset test above doesn't touch them.""" - broken = [ - zipinspect.CentralDirectoryEntry( - name="0.payload", - raw_compressed_size=0, - raw_uncompressed_size=12345, - raw_local_header_offset=0, - compressed_size=0, - uncompressed_size=5 * 2**30, - local_header_offset=0, - has_zip64_extra=False, - ) - ] - with pytest.raises(AssertionError, match="ZIP64 sentinel"): - zipinspect.assert_zip64_above_4gib(broken) + with pytest.raises(MalformedZipError, match="claims 64 bytes"): + zipinspect.central_directory(p) - def test_above_4gib_compressed_size_without_the_sentinel_fails(self): - """Compressed size must be checked against its own raw field, not the uncompressed one. + def test_tolerates_the_trailing_disk_start_field(self, tmp_path: Path): + """APPNOTE 4.5.3 allows a 4-byte disk-start value after the 64-bit ones. - A TDF is STORED, not DEFLATEd, so the compressed field is at least as - likely to cross 2**32 as the uncompressed one -- but a check that only - looks at ``uses_zip64_for_sizes`` (an OR over both raw fields) would - let a correctly-sentineled uncompressed field paper over a broken - compressed one. This entry has exactly that shape. + It is the one length the check has to be lax about, so it gets a test + rather than being left to the reviewer to notice in the ``+ 4``. """ - broken = [ - zipinspect.CentralDirectoryEntry( - name="0.payload", - raw_compressed_size=12345, - raw_uncompressed_size=ZIP64_SENTINEL_32, - raw_local_header_offset=0, - compressed_size=5 * 2**30, - uncompressed_size=5 * 2**30, - local_header_offset=0, - has_zip64_extra=True, - ) - ] - with pytest.raises(AssertionError, match="compressed-size field"): - zipinspect.assert_zip64_above_4gib(broken) + p = synth_zip( + tmp_path / "disk-start.zip", + [ + cen_record( + "0.manifest.json", + raw_offset=ZIP64_SENTINEL_32, + zip64_offset=5 * 2**30, + zip64_disk_start=0, + ) + ], + ) + (entry,) = zipinspect.central_directory(p).entries + assert entry.local_header_offset == 5 * 2**30 + assert entry.has_zip64_extra + + +class TestTruncatedContainers: + """A malformed container must arrive as MalformedZipError, not struct.error. + + These inputs are SDK output and hand-built fixtures, so hitting one is an + expected case. A bare ``struct.error`` from the middle of the parse names + neither the file nor the field and reads like a bug in the test harness. + """ + + def test_truncated_eocd(self, tmp_path: Path): + p = synth_zip(tmp_path / "t.zip", [cen_record("a", raw_offset=0)]) + p.write_bytes(p.read_bytes()[:-6]) + with pytest.raises(MalformedZipError): + zipinspect.central_directory(p) + + def test_truncated_central_directory_record(self, tmp_path: Path): + record = cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES) + p = tmp_path / "short-cen.zip" + cd = record[:20] + eocd = ( + b"PK\x05\x06" + + struct.pack(" ZIP64_WINDOW_HIGH + with pytest.raises(AssertionError, match="not one of its 1 entries"): + zipinspect.assert_zip64_above_4gib(cd) + + def test_a_container_past_4gib_with_zip64_passes(self, tmp_path: Path): + cd_offset = 5 * 2**30 + p = synth_zip64_eocd( + tmp_path / "big-with-zip64.zip", + [ + cen_record( + "0.payload", + raw_offset=ZIP64_SENTINEL_32, + zip64_offset=cd_offset - 1024, + ) + ], + cd_offset=cd_offset, + ) + zipinspect.assert_zip64_above_4gib(zipinspect.central_directory(p)) + + def test_a_container_below_4gib_is_not_required_to_use_zip64(self, tmp_path: Path): + """The 2-4 GiB band has latitude; only above 2**32 is ZIP64 mandatory.""" p = synth_zip( - tmp_path / "big-sizes.zip", + tmp_path / "medium.zip", + [cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES)], + cd_offset=MEDIUM_BYTES + 128, + ) + zipinspect.assert_zip64_above_4gib(zipinspect.central_directory(p)) + + def test_an_offset_truncated_mod_2_32_is_caught(self, tmp_path: Path): + """The defect ``assert_zip64_above_4gib`` structurally cannot see. + + A writer that drops the high bits of a 5 GiB offset emits a value that + parses perfectly and looks like an ordinary small offset. What gives + it away is that the entry's data would then have to end well after the + central directory starts. + """ + true_offset = 5 * 2**30 + cd_offset = true_offset + 4096 + p = synth_zip64_eocd( + tmp_path / "truncated-offset.zip", [ cen_record( "0.payload", - raw_offset=0, + raw_offset=true_offset % ZIP64_WINDOW_HIGH, raw_usize=ZIP64_SENTINEL_32, raw_csize=ZIP64_SENTINEL_32, - zip64_usize=5 * 2**30, - zip64_csize=5 * 2**30 + 1, + zip64_usize=true_offset, + zip64_csize=true_offset, ) ], + cd_offset=cd_offset, ) - entries = zipinspect.central_directory(p) - zipinspect.assert_zip64_above_4gib(entries) + cd = zipinspect.central_directory(p) + with pytest.raises(AssertionError, match="truncated mod 2\\*\\*32"): + zipinspect.assert_offsets_are_consistent(cd) + + def test_an_entry_at_or_after_the_central_directory_is_caught(self, tmp_path: Path): + p = synth_zip( + tmp_path / "entry-after-cd.zip", + [cen_record("0.payload", raw_offset=9000)], + cd_offset=4096, + ) + cd = zipinspect.central_directory(p) + with pytest.raises(AssertionError, match="at or after the central directory"): + zipinspect.assert_offsets_are_consistent(cd) + + def test_two_entries_claiming_one_offset_are_caught(self, tmp_path: Path): + p = synth_zip( + tmp_path / "dup-offset.zip", + [ + cen_record("0.payload", raw_offset=512), + cen_record("0.manifest.json", raw_offset=512), + ], + cd_offset=4096, + ) + cd = zipinspect.central_directory(p) + with pytest.raises(AssertionError, match="both claim a local header"): + zipinspect.assert_offsets_are_consistent(cd) + + def test_a_well_formed_container_is_consistent(self, tmp_path: Path): + p = synth_zip( + tmp_path / "fine.zip", + [ + cen_record("0.payload", raw_offset=0, raw_csize=2048), + cen_record("0.manifest.json", raw_offset=2100, raw_csize=64), + ], + cd_offset=4096, + ) + zipinspect.assert_offsets_are_consistent(zipinspect.central_directory(p)) def test_window_entries_are_reported_for_either_encoding(self, tmp_path: Path): """Both a raw value and a sentinel in the band are legal and both are listed.""" @@ -401,12 +766,21 @@ def test_window_entries_are_reported_for_either_encoding(self, tmp_path: Path): cen_record("small", raw_offset=1024), ], ) - entries = zipinspect.central_directory(p) + entries = zipinspect.central_directory(p).entries assert {e.name for e in zipinspect.entries_in_window(entries)} == { "raw", "sentinel", } + def test_a_compressed_size_in_the_window_is_reported(self, tmp_path: Path): + """The compressed size is a 32-bit field too, and was being skipped.""" + p = synth_zip( + tmp_path / "csize-window.zip", + [cen_record("0.payload", raw_offset=0, raw_csize=MEDIUM_BYTES)], + ) + entries = zipinspect.central_directory(p).entries + assert [e.name for e in zipinspect.entries_in_window(entries)] == ["0.payload"] + def test_only_raw_window_values_exercise_signed_read(self, tmp_path: Path): """The sentinel redirects to ZIP64 data and is not a signed read risk.""" p = synth_zip( @@ -420,7 +794,7 @@ def test_only_raw_window_values_exercise_signed_read(self, tmp_path: Path): ), ], ) - entries = zipinspect.central_directory(p) + entries = zipinspect.central_directory(p).entries assert { e.name for e in zipinspect.entries_with_raw_values_in_window(entries) } == {"raw"} @@ -429,6 +803,6 @@ def test_describe_includes_the_numbers_needed_to_debug(self, tmp_path: Path): p = synth_zip( tmp_path / "d.zip", [cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES)] ) - text = zipinspect.describe(zipinspect.central_directory(p)) + text = zipinspect.describe(zipinspect.central_directory(p).entries) assert "0.manifest.json" in text assert str(MEDIUM_BYTES) in text diff --git a/xtest/zipinspect.py b/xtest/zipinspect.py index 87977222..de3faf31 100644 --- a/xtest/zipinspect.py +++ b/xtest/zipinspect.py @@ -8,8 +8,10 @@ Only the tail of the file plus the central directory is read, so this stays cheap on a multi-GiB container. -Reference: APPNOTE.TXT 4.3.12 (central directory header), 4.3.16 (end of -central directory), 4.5.3 (the ZIP64 extended information extra field). +Reference: APPNOTE.TXT 4.3.12 (central directory header), 4.3.14 (zip64 end of +central directory record), 4.3.15 (zip64 end of central directory locator), +4.3.16 (end of central directory), 4.5.3 (the ZIP64 extended information extra +field). """ from __future__ import annotations @@ -17,6 +19,7 @@ import struct from dataclasses import dataclass from pathlib import Path +from typing import NamedTuple from sizes import ZIP64_WINDOW_HIGH, in_zip64_window @@ -29,13 +32,15 @@ #: Written into a 32-bit field to mean "the real value is in the ZIP64 extra #: field". APPNOTE 4.4.1.4. ZIP64_SENTINEL_32 = 0xFFFFFFFF -ZIP64_SENTINEL_16 = 0xFFFF #: Header ID of the ZIP64 extended information extra field. APPNOTE 4.5.3. ZIP64_EXTRA_ID = 0x0001 _EOCD_SIZE = 22 +_EOCD64_SIZE = 56 _EOCD64_LOCATOR_SIZE = 20 +#: Bytes of a central-directory record before the variable-length name. +_CEN_FIXED_SIZE = 46 #: A ZIP comment is a 16-bit length, so the EOCD cannot start further back #: than this from the end of the file. _MAX_EOCD_SEARCH = _EOCD_SIZE + 0xFFFF @@ -71,6 +76,13 @@ def uses_zip64_for_offset(self) -> bool: @property def uses_zip64_for_sizes(self) -> bool: + """True if *either* size field defers to the ZIP64 extra field. + + Deliberately an OR, for the "did this writer opt into ZIP64 at all" + question. It is the wrong predicate for checking one field's encoding: + a correctly-sentineled uncompressed size would paper over a broken + compressed one. + """ return ZIP64_SENTINEL_32 in ( self.raw_compressed_size, self.raw_uncompressed_size, @@ -89,8 +101,11 @@ def signed_read_of_offset(self) -> int: def _find_eocd(data: bytes) -> int: """Offset of the EOCD record within the tail buffer. - Searched backwards: the signature can legitimately appear inside a file - comment, and the last occurrence is the real one. + Searched backwards because the signature can also occur *before* the real + record -- inside entry data, or in a central-directory record's name or + extra field -- and only the last occurrence can be the EOCD itself. (A + signature planted inside the trailing file comment would defeat this, but + it would defeat every ZIP reader; the format is genuinely ambiguous there.) """ idx = data.rfind(_EOCD_SIG) if idx < 0: @@ -98,13 +113,28 @@ def _find_eocd(data: bytes) -> int: return idx +class Zip64Extra(NamedTuple): + """Decoded ZIP64 extended information field. A tuple, but a named one. + + The bare 4-tuple this replaces was two adjacent ``int | None`` size fields + in a fixed order, which is exactly the shape where transposing the decode + goes unnoticed -- both call sites and both mutations type-check. + """ + + present: bool + uncompressed_size: int | None + compressed_size: int | None + local_header_offset: int | None + + def _parse_zip64_extra( extra: bytes, *, + name: str, want_uncompressed: bool, want_compressed: bool, want_offset: bool, -) -> tuple[bool, int | None, int | None, int | None]: +) -> Zip64Extra: """Pull the 64-bit values out of the ZIP64 extended information field. The field is positional, not tagged: values appear only for the 32-bit @@ -113,49 +143,96 @@ def _parse_zip64_extra( present depends on the record that referenced it, which is what the ``want_*`` flags carry in. - Returns ``(present, uncompressed, compressed, offset)``; the values are - None when the corresponding 32-bit field did not hold the sentinel. + Because it is positional, a length that does not match the record's + sentinels makes *every* value in it ambiguous, and decoding it anyway + yields a plausible wrong number rather than an obvious one -- a writer + that sentinels only the offset but emits all three values would have its + uncompressed size read back as the offset. So the length is checked + against the ``want_*`` flags and a mismatch raises. The only slack is the + trailing 4-byte disk-start field, which is cheap to tolerate. """ + wants = (want_uncompressed, want_compressed, want_offset) + expected = 8 * sum(wants) pos = 0 while pos + 4 <= len(extra): header_id, size = struct.unpack_from(" len(extra): - break + raise MalformedZipError( + f"entry {name!r}: extra field record {header_id:#06x} claims " + f"{size} bytes but only {len(extra) - pos} remain" + ) if header_id != ZIP64_EXTRA_ID: pos += size continue + if size == 0 or size not in (expected, expected + 4): + raise MalformedZipError( + f"entry {name!r}: ZIP64 extra field is {size} bytes, but the " + f"record's 32-bit fields call for {expected} " + f"(uncompressed={want_uncompressed}, " + f"compressed={want_compressed}, offset={want_offset}). " + "APPNOTE 4.5.3 makes the field positional, so a length " + "mismatch leaves every value in it ambiguous." + ) body = extra[pos : pos + size] # Read the 64-bit values in APPNOTE order, consuming one only for each - # 32-bit field that actually held the sentinel. A truncated field - # yields None rather than raising: a malformed extra field is a - # finding for the caller's assertions, not a parse error. + # 32-bit field that actually held the sentinel. The length check above + # guarantees there are exactly as many as the flags asked for. values: list[int | None] = [] at = 0 - for want in (want_uncompressed, want_compressed, want_offset): - if want and at + 8 <= len(body): + for want in wants: + if want: values.append(struct.unpack_from(" list[CentralDirectoryEntry]: + +def central_directory(path: Path) -> CentralDirectory: """Parse every central-directory record in ``path``. Reads the tail of the file to locate the directory, then the directory itself. The payload is never touched, so cost is independent of container size. + + Every read out of those two buffers is bounds-checked first. The inputs + here are SDK output under test and hand-built byte fixtures, so a + malformed one is an expected case: it must arrive as + :class:`MalformedZipError` naming the problem, not as a bare + ``struct.error`` from somewhere in the middle of the parse. """ - size = path.stat().st_size + file_size = path.stat().st_size with path.open("rb") as f: - tail_len = min(size, _MAX_EOCD_SEARCH) - f.seek(size - tail_len) + tail_len = min(file_size, _MAX_EOCD_SEARCH) + f.seek(file_size - tail_len) tail = f.read(tail_len) eocd_at = _find_eocd(tail) + if eocd_at + _EOCD_SIZE > len(tail): + raise MalformedZipError( + f"end-of-central-directory record at {eocd_at} is truncated: " + f"{len(tail) - eocd_at} of {_EOCD_SIZE} bytes" + ) ( cd_entries_this_disk, cd_entries_total, @@ -170,8 +247,13 @@ def central_directory(path: Path) -> list[CentralDirectoryEntry]: locator_at = eocd_at - _EOCD64_LOCATOR_SIZE if locator_at >= 0 and tail[locator_at : locator_at + 4] == _EOCD64_LOCATOR_SIG: (eocd64_offset,) = struct.unpack_from(" file_size: + raise MalformedZipError( + f"zip64 locator points at {eocd64_offset}, past the end of " + f"a {file_size}-byte file" + ) f.seek(eocd64_offset) - eocd64 = f.read(56) + eocd64 = f.read(_EOCD64_SIZE) if eocd64[:4] != _EOCD64_SIG: raise MalformedZipError( f"zip64 locator points at {eocd64_offset}, which is not a " @@ -179,6 +261,13 @@ def central_directory(path: Path) -> list[CentralDirectoryEntry]: ) entry_count, cd_size, cd_offset = struct.unpack_from(" file_size: + raise MalformedZipError( + f"central directory claims {cd_size} bytes at offset " + f"{cd_offset}, past the end of a {file_size}-byte file" + ) f.seek(cd_offset) cd = f.read(cd_size) @@ -189,6 +278,11 @@ def central_directory(path: Path) -> list[CentralDirectoryEntry]: raise MalformedZipError( f"expected a central-directory header at {cd_offset + pos}" ) + if pos + _CEN_FIXED_SIZE > len(cd): + raise MalformedZipError( + f"central-directory header at {cd_offset + pos} is truncated: " + f"{len(cd) - pos} of {_CEN_FIXED_SIZE} bytes" + ) ( raw_compressed, raw_uncompressed, @@ -198,41 +292,49 @@ def central_directory(path: Path) -> list[CentralDirectoryEntry]: ) = struct.unpack_from(" len(cd): + raise MalformedZipError( + f"central-directory header at {cd_offset + pos} claims " + f"{name_len}+{extra_len}+{comment_len} bytes of name, extra " + f"and comment, but only {len(cd) - name_at} remain" + ) name = cd[name_at:extra_at].decode("utf-8", errors="replace") extra = cd[extra_at : extra_at + extra_len] want_uncompressed = raw_uncompressed == ZIP64_SENTINEL_32 want_compressed = raw_compressed == ZIP64_SENTINEL_32 want_offset = raw_offset == ZIP64_SENTINEL_32 - has_extra, z_uncompressed, z_compressed, z_offset = _parse_zip64_extra( + z = _parse_zip64_extra( extra, + name=name, want_uncompressed=want_uncompressed, want_compressed=want_compressed, want_offset=want_offset, ) # A 32-bit field holding the sentinel promises the real value lives in - # the extra field. If it doesn't -- missing entirely, or truncated -- - # that is the malformed-container case _parse_zip64_extra's docstring - # describes, not a legitimate "no ZIP64 here" reading. Falling back to - # the raw sentinel (0xFFFFFFFF) below would mislabel it as an ordinary - # in-window value instead. - if want_uncompressed and z_uncompressed is None: + # the extra field. If the field is not there at all, that is a + # malformed container, not a legitimate "no ZIP64 here" reading: + # falling back to the raw sentinel (0xFFFFFFFF) below would mislabel it + # as an ordinary in-window value. (A field that *is* there but the + # wrong length has already raised inside _parse_zip64_extra.) + if want_uncompressed and z.uncompressed_size is None: raise MalformedZipError( f"entry {name!r}: uncompressed size holds the ZIP64 sentinel " - "but its extra field is missing or truncated" + "but there is no ZIP64 extra field to resolve it" ) - if want_compressed and z_compressed is None: + if want_compressed and z.compressed_size is None: raise MalformedZipError( f"entry {name!r}: compressed size holds the ZIP64 sentinel " - "but its extra field is missing or truncated" + "but there is no ZIP64 extra field to resolve it" ) - if want_offset and z_offset is None: + if want_offset and z.local_header_offset is None: raise MalformedZipError( f"entry {name!r}: local header offset holds the ZIP64 sentinel " - "but its extra field is missing or truncated" + "but there is no ZIP64 extra field to resolve it" ) entries.append( @@ -242,18 +344,31 @@ def central_directory(path: Path) -> list[CentralDirectoryEntry]: raw_uncompressed_size=raw_uncompressed, raw_local_header_offset=raw_offset, compressed_size=( - z_compressed if z_compressed is not None else raw_compressed + z.compressed_size + if z.compressed_size is not None + else raw_compressed ), uncompressed_size=( - z_uncompressed if z_uncompressed is not None else raw_uncompressed + z.uncompressed_size + if z.uncompressed_size is not None + else raw_uncompressed ), - local_header_offset=z_offset if z_offset is not None else raw_offset, - has_zip64_extra=has_extra, + local_header_offset=( + z.local_header_offset + if z.local_header_offset is not None + else raw_offset + ), + has_zip64_extra=z.present, ) ) - pos = extra_at + extra_len + comment_len + pos = end - return entries + return CentralDirectory( + entries=entries, + cd_offset=cd_offset, + cd_size=cd_size, + file_size=file_size, + ) def describe(entries: list[CentralDirectoryEntry]) -> str: @@ -278,41 +393,79 @@ def describe(entries: list[CentralDirectoryEntry]) -> str: ) -def assert_zip64_above_4gib(entries: list[CentralDirectoryEntry]) -> None: - """Every value at or above 2**32 must use the ZIP64 sentinel plus extra field. +def assert_offsets_are_consistent(cd: CentralDirectory) -> None: + """Cross-check the directory against where it actually sits in the file. + + This is what catches a writer that truncates an offset mod 2**32. Such a + value parses perfectly -- an entry genuinely at 5 GiB written as + ``5 GiB - 2**32`` looks exactly like an entry at 0.7 GiB -- so no amount + of inspecting the record on its own will find it. What gives it away is + that the container disagrees with itself: the entry's data would then have + to end long after the central directory begins. - Unlike the 2-4 GiB band, there is no latitude here: a 32-bit field - physically cannot hold the value, so a writer that does not emit the - sentinel has produced a container whose stated offsets are wrong. + Every clause here is true of any conformant ZIP, so a failure is a real + writer defect and not a quirk this suite has decided to dislike. """ - for e in entries: - if e.local_header_offset >= ZIP64_WINDOW_HIGH: - assert e.uses_zip64_for_offset and e.has_zip64_extra, ( - f"entry {e.name!r} is at offset {e.local_header_offset}, at or " - f"above 2**32, but its 32-bit field holds " - f"{e.raw_local_header_offset} rather than the ZIP64 sentinel\n" - + describe(entries) - ) - if e.uncompressed_size >= ZIP64_WINDOW_HIGH: - assert e.raw_uncompressed_size == ZIP64_SENTINEL_32 and e.has_zip64_extra, ( - f"entry {e.name!r} is {e.uncompressed_size} bytes, at or above " - f"2**32, but its 32-bit size field holds " - f"{e.raw_uncompressed_size} rather than the ZIP64 sentinel\n" - + describe(entries) - ) - if e.compressed_size >= ZIP64_WINDOW_HIGH: - assert e.raw_compressed_size == ZIP64_SENTINEL_32 and e.has_zip64_extra, ( - f"entry {e.name!r} is {e.compressed_size} bytes compressed, at " - f"or above 2**32, but its 32-bit compressed-size field holds " - f"{e.raw_compressed_size} rather than the ZIP64 sentinel\n" - + describe(entries) - ) + assert cd.cd_offset + cd.cd_size <= cd.file_size, ( + f"central directory claims {cd.cd_size} bytes at offset " + f"{cd.cd_offset}, which runs past the end of the {cd.file_size}-byte " + f"container\n" + describe(cd.entries) + ) + + seen: dict[int, str] = {} + for e in cd.entries: + assert e.local_header_offset < cd.cd_offset, ( + f"entry {e.name!r} claims its local header is at " + f"{e.local_header_offset}, at or after the central directory at " + f"{cd.cd_offset}\n" + describe(cd.entries) + ) + # The local header and its data both precede the directory, so the + # data alone cannot overrun it. Compressed size is the right field: + # a TDF is STORED, so it is the number of bytes actually on disk. + end = e.local_header_offset + e.compressed_size + assert end <= cd.cd_offset, ( + f"entry {e.name!r} claims {e.compressed_size} bytes at offset " + f"{e.local_header_offset}, ending at {end}, past the central " + f"directory at {cd.cd_offset}. An offset truncated mod 2**32 " + f"looks exactly like this.\n" + describe(cd.entries) + ) + clash = seen.get(e.local_header_offset) + assert clash is None, ( + f"entries {clash!r} and {e.name!r} both claim a local header at " + f"{e.local_header_offset}\n" + describe(cd.entries) + ) + seen[e.local_header_offset] = e.name + + +def assert_zip64_above_4gib(cd: CentralDirectory) -> None: + """A container that crosses 4 GiB must use ZIP64 somewhere. + + Above 2**32 there is no latitude: a 32-bit field physically cannot hold + the value, so the format requires the sentinel plus an extra field. + + Note what this does *not* assert. "Every resolved value at or above 2**32 + uses the sentinel" is unfalsifiable on parser output -- a raw 32-bit field + tops out at 2**32-1, so a resolved value that large can only have come + from the extra field in the first place. The check has to be anchored to + the file's real size instead, which is why it takes the whole + :class:`CentralDirectory`. The writer defect it leaves uncovered -- + truncation mod 2**32 -- is :func:`assert_offsets_are_consistent`'s. + """ + if cd.file_size < ZIP64_WINDOW_HIGH: + return + assert any(e.uses_zip64_for_offset or e.uses_zip64_for_sizes for e in cd.entries), ( + f"the container is {cd.file_size} bytes, at or above 2**32, but not " + f"one of its {len(cd.entries)} entries uses a ZIP64 sentinel. Either " + f"a local header past 4 GiB or an entry whose data crosses it has to " + f"exist in a file this size, and either one requires ZIP64.\n" + + describe(cd.entries) + ) def entries_in_window( entries: list[CentralDirectoryEntry], ) -> list[CentralDirectoryEntry]: - """Entries with an offset or size in ``[2**31, 2**32)``. + """Entries with an offset or either size in ``[2**31, 2**32)``. These are the records a sign-extending reader mishandles. Both encodings -- a real unsigned 32-bit value, or the ZIP64 sentinel -- are legal here, @@ -322,8 +475,14 @@ def entries_in_window( return [ e for e in entries - if in_zip64_window(e.local_header_offset) - or in_zip64_window(e.uncompressed_size) + if any( + in_zip64_window(value) + for value in ( + e.local_header_offset, + e.uncompressed_size, + e.compressed_size, + ) + ) ]