Harden TQQQ trusted snapshot input boundary - #219
Conversation
Co-Authored-By: Codex <noreply@openai.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fa83ffa73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return verify_tqqq_r1_snapshot_fd(directory_fd, expected_manifest_sha256=expected_manifest_sha256) | ||
| finally: | ||
| os.close(directory_fd) |
There was a problem hiding this comment.
On Linux, verify_tqqq_r1_snapshot_fd() records /proc/self/fd/<directory_fd> as SnapshotResult.output_dir, but this wrapper closes that descriptor immediately afterward. Every successful path-based verification therefore returns a missing—or potentially reused—path instead of the caller's directory, breaking the existing result contract and the equality assertion in test_materialize_writes_deterministic_immutable_artifacts.
Useful? React with 👍 / 👎.
|
|
||
| manifest = _package_manifest(read_strict_json(package_manifest_payload, "package manifest")) | ||
| receipt = _receipt(read_strict_json(receipt_payload, "receipt")) | ||
| calendar = _calendar(read_strict_json(calendar_payload, "calendar evidence")) |
There was a problem hiding this comment.
When the calendar file is tampered with while the externally anchored package manifest and receipt remain valid, this line decodes and validates the unauthenticated calendar before the following digest comparison. A deeply nested payload under the size limit can consequently raise an uncaught RecursionError, and large valid structures consume parsing work even though their hash is already known to be wrong; compare calendar_sha256 with the authenticated manifest and receipt bindings before invoking read_strict_json().
Useful? React with 👍 / 👎.
| except (UnicodeDecodeError, ValueError, json.JSONDecodeError) as exc: | ||
| raise TrustedSnapshotPackageError(f"invalid strict JSON: {name}") from exc |
There was a problem hiding this comment.
For a sufficiently deeply nested JSON payload, json.loads() raises RecursionError, which is not covered by this exception normalization. Calls to the public read_strict_json()—and package loads whose authenticated evidence is malformed this way—therefore leak an unexpected runtime exception instead of the documented TrustedSnapshotPackageError; include recursive parser failures in the normalized error path.
Useful? React with 👍 / 👎.
| descriptor = self._snapshot_fd | ||
| if descriptor >= 0: | ||
| try: | ||
| os.close(descriptor) |
There was a problem hiding this comment.
If two threads call close() concurrently, both can read the same nonnegative descriptor before either call stores -1; after the first os.close() releases it, the operating system may reuse that number and the second call can close an unrelated resource. Atomically claim the descriptor by storing -1 before calling os.close(), or protect this transition with a lock.
Useful? React with 👍 / 👎.
|
Fail-closed closure evidence: required CI |
1 participant
Scope
Validation
PYTHONPATH=src python3 -m pytest -q tests/test_tqqq_r1_snapshot.py tests/test_tqqq_trusted_snapshot_package.pyPYTHONPATH=src python3 -m pytest -qPYTHONPATH=src python3 -m ruff check(exact four paths)PYTHONPATH=src python3 -m compileall -q srcImmutable review head:
2fa83ffa73d12054c3b41195a9d711527e1fb345