diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 0c6fc424..31b7d516 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -36,9 +36,9 @@ 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. # # --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 +48,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..6bbd042b 100644 --- a/.github/workflows/xtest.yml +++ b/.github/workflows/xtest.yml @@ -38,6 +38,11 @@ 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 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 @@ -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 }} @@ -157,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) { @@ -201,6 +219,24 @@ jobs: } } + // 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; + 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 +860,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 +982,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 +1059,370 @@ 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 too slow for 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. 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] + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + repository: opentdf/tests + path: otdftests + persist-credentials: false + + # 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 + # 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 + 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 + 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. + # + # 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 + 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: `--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: |- + 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. + # + # 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 + import sys + 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") + 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) + 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}") + + # 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( + 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 + + - 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..e84c18f3 --- /dev/null +++ b/spec/DSPX-4592.md @@ -0,0 +1,398 @@ +--- +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 + +@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_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_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 +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. + +**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` 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 +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 + (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. +- 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] 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: 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 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), +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 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. +- [ ] 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 (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 +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_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` +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..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]]() @@ -151,7 +163,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 +441,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 +625,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/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 e9d5cf15..32184049 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 @@ -20,11 +60,35 @@ #: 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, + "medium": MEDIUM_BYTES, "large": 5 * 2**30, } #: Order to emit parametrized sizes in, cheapest first. -SIZE_ORDER: tuple[str, ...] = ("small", "chunky", "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: + """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..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,10 @@ 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 - permanently-red job trains people to ignore it, and it needs no dated - guess about which release carries the fix. + 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_is_broken` 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 +922,38 @@ 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, 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_is_broken(decrypt_sdk: SDK) -> bool: + """True for decryptors known to mishandle a real 32-bit value in the band. + + 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 assumed broken -- + they are the builds expected to carry the fix. + """ + sv = decrypt_sdk.semver() + 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: """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..dde62a47 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,43 @@ 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 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): diff --git a/xtest/test_tdfs_units.py b/xtest/test_tdfs_units.py index b6b9b5f2..9774ee6e 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_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``. """ 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,52 @@ def test_unknown_name_raises(self): tdfs._parse_forced_supports("hexles") +# --- 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_is_broken reads.""" + return cast(tdfs.SDK, SimpleNamespace(sdk=sdk, semver=lambda: semver)) + + +class TestZip64ReaderIsBroken: + """Which decryptors ``test_zip64.py`` requires a decrypt failure from. + + 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_pre_fix_java_is_broken(self): + assert tdfs.zip64_reader_is_broken(_stub_sdk("java", (0, 18, 0))) + + 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 new file mode 100644 index 00000000..583852ea --- /dev/null +++ b/xtest/test_zip64.py @@ -0,0 +1,185 @@ +"""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 +import subprocess +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( + 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, + ) + + cd = zipinspect.central_directory(ct_file) + entries = cd.entries + 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 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_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 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.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) + + # 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 + # 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..a935e294 --- /dev/null +++ b/xtest/test_zip64_units.py @@ -0,0 +1,808 @@ +"""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, + 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. + + ``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_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"" + if zip64_usize is not None: + body += struct.pack(" 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. 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) + 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) + 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 ----------------------------------------------------------- + + +class TestCentralDirectory: + def test_reads_a_real_zip(self, tmp_path: Path): + """Agreement with zipfile on an ordinary container, as a sanity floor.""" + p = tmp_path / "ordinary.zip" + with zipfile.ZipFile(p, "w") as z: + z.writestr("0.payload", b"a" * 4096) + z.writestr("0.manifest.json", b"{}") + + 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 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 + ): + """``force_zip64`` is a local-header decision and must not be read as a CD one. + + The two are independent: a writer can emit the ZIP64 extra field in + the local header while the central directory's values still fit in 32 + bits, which is exactly what this produces. Reporting + ``has_zip64_extra`` for it would make the conformance assertions think + a writer had opted into ZIP64 for a field it had not. + """ + p = tmp_path / "z64-local.zip" + with zipfile.ZipFile(p, "w") as z: + with z.open("0.payload", "w", force_zip64=True) as f: + f.write(b"b" * 8192) + + (entry,) = zipinspect.central_directory(p).entries + assert entry.uncompressed_size == 8192 + assert not entry.has_zip64_extra + assert not entry.uses_zip64_for_sizes + + def test_reads_a_zip64_end_of_central_directory(self, tmp_path: Path): + """When the EOCD holds sentinels, the real values come from the ZIP64 EOCD. + + A container whose central directory starts past 4 GiB -- which the + 'large' size produces -- can only be located this way, so the branch + is on the path for the very sizes this module exists to cover. + """ + records = [ + cen_record("0.payload", raw_offset=0), + cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES), + ] + p = synth_zip64_eocd(tmp_path / "z64-eocd.zip", records) + 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( + tmp_path / "bad-locator.zip", + [cen_record("0.payload", raw_offset=0)], + eocd64_offset_override=1, + ) + with pytest.raises(MalformedZipError, match="zip64 locator"): + zipinspect.central_directory(p) + + def test_raw_value_in_the_window_is_preserved(self, tmp_path: Path): + """A 32-bit field holding a real 2.1 GiB value must not be normalised away. + + This is the go-writer shape: legal APPNOTE, and the input that a + sign-extending reader mishandles. If the parser resolved it through + the ZIP64 path the test would lose the ability to tell the two + encodings apart. + """ + p = synth_zip( + tmp_path / "window.zip", + [ + cen_record("0.payload", raw_offset=0, raw_usize=MEDIUM_BYTES), + cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES + 64), + ], + ) + 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 + assert not manifest.uses_zip64_for_offset + + def test_signed_read_of_a_windowed_offset_goes_negative(self, tmp_path: Path): + """The defect itself, reproduced arithmetically. + + java-sdk's pre-#393 ``readInt()`` widens this field with a signed + read. Anything at or above 2**31 comes back negative and the + subsequent seek fails or lands on nonsense. + """ + p = synth_zip( + tmp_path / "signed.zip", + [cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES)], + ) + (entry,) = zipinspect.central_directory(p).entries + assert entry.signed_read_of_offset() < 0 + assert entry.signed_read_of_offset() == MEDIUM_BYTES - ZIP64_WINDOW_HIGH + + def test_signed_read_is_harmless_below_the_window(self, tmp_path: Path): + """Below 2**31 the two reads agree, which is why smaller payloads miss this.""" + offset = ZIP64_WINDOW_LOW - 1 + p = synth_zip( + tmp_path / "safe.zip", [cen_record("0.manifest.json", raw_offset=offset)] + ) + (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): + """The web-sdk shape: always ZIP64, so the 32-bit field is 0xFFFFFFFF.""" + true_offset = 6 * 2**30 + p = synth_zip( + tmp_path / "sentinel.zip", + [ + cen_record( + "0.manifest.json", + raw_offset=ZIP64_SENTINEL_32, + zip64_offset=true_offset, + ) + ], + ) + (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(" 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 / "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=true_offset % ZIP64_WINDOW_HIGH, + raw_usize=ZIP64_SENTINEL_32, + raw_csize=ZIP64_SENTINEL_32, + zip64_usize=true_offset, + zip64_csize=true_offset, + ) + ], + cd_offset=cd_offset, + ) + 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.""" + 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).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( + 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).entries + 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).entries) + 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..de3faf31 --- /dev/null +++ b/xtest/zipinspect.py @@ -0,0 +1,509 @@ +"""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.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 + +import struct +from dataclasses import dataclass +from pathlib import Path +from typing import NamedTuple + +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 + +#: 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 + + +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: + """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, + ) + + 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 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: + raise MalformedZipError("no end-of-central-directory record found") + 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, +) -> 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 + 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. + + 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): + 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. The length check above + # guarantees there are exactly as many as the flags asked for. + values: list[int | None] = [] + at = 0 + for want in wants: + if want: + values.append(struct.unpack_from(" 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. + """ + file_size = path.stat().st_size + with path.open("rb") as f: + 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, + cd_size, + cd_offset, + ) = struct.unpack_from("= 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(_EOCD64_SIZE) + if eocd64[:4] != _EOCD64_SIG: + raise MalformedZipError( + f"zip64 locator points at {eocd64_offset}, which is not a " + "zip64 end-of-central-directory record" + ) + 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) + + entries: list[CentralDirectoryEntry] = [] + pos = 0 + for _ in range(entry_count): + if cd[pos : pos + 4] != _CEN_SIG: + 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, + name_len, + extra_len, + comment_len, + ) = 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 + 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 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 there is no ZIP64 extra field to resolve it" + ) + if want_compressed and z.compressed_size is None: + raise MalformedZipError( + f"entry {name!r}: compressed size holds the ZIP64 sentinel " + "but there is no ZIP64 extra field to resolve it" + ) + if want_offset and z.local_header_offset is None: + raise MalformedZipError( + f"entry {name!r}: local header offset holds the ZIP64 sentinel " + "but there is no ZIP64 extra field to resolve it" + ) + + entries.append( + CentralDirectoryEntry( + name=name, + raw_compressed_size=raw_compressed, + raw_uncompressed_size=raw_uncompressed, + raw_local_header_offset=raw_offset, + compressed_size=( + z.compressed_size + if z.compressed_size is not None + else raw_compressed + ), + uncompressed_size=( + z.uncompressed_size + if z.uncompressed_size is not None + else raw_uncompressed + ), + local_header_offset=( + z.local_header_offset + if z.local_header_offset is not None + else raw_offset + ), + has_zip64_extra=z.present, + ) + ) + pos = end + + return CentralDirectory( + entries=entries, + cd_offset=cd_offset, + cd_size=cd_size, + file_size=file_size, + ) + + +def describe(entries: list[CentralDirectoryEntry]) -> 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_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. + + 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. + """ + 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 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, + which is why this returns them for reporting rather than asserting on + which one the writer chose. + """ + return [ + e + for e in entries + if any( + in_zip64_window(value) + for value in ( + e.local_header_offset, + e.uncompressed_size, + e.compressed_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, + ) + ) + ]