Repository navigation
Inspect stacked pickle streams incrementally in is_likely_safe() - #345
Open
professor-moody wants to merge 1 commit into
Open
professor-moody wants to merge 1 commit into
professor-moody wants to merge 1 commit into
Conversation
professor-moody
requested a review
from thomas-chauchefoin-tob
as a code owner
November 2, 2026 17:21
dguido
requested changes
Nov 7, 2026
dguido
left a comment
Member
There was a problem hiding this comment.
[P1] fickling/analysis.py:690 loads every stacked stream before all() can reject an unsafe first stream. In a local check, 100,000 tiny trailing streams (about 200 KB) increased memory use by roughly 47 MB. Please parse and check streams incrementally, returning on the first unsafe result.
Check each stream before parsing the next one. Return False immediately for an invalid opcode or a safety result other than LIKELY_SAFE, without materializing trailing streams. Preserve the empty-file exception. Retain the unsafe-second-stream and benign controls, and add regressions for early rejection with complete or truncated trailing streams, invalid first streams, and empty files.
professor-moody
force-pushed
the
fix-is-likely-safe-stacked-pickles
branch
from
November 7, 2026 17:18
1c4d219 to
31f1f27
Compare
Author
|
Thanks @dguido! Updated |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
is_likely_safe()previously based its answer on the first pickle stream. This change checks stacked streams incrementally and returnsFalseas soon as a stream has an invalid opcode or is not ratedLIKELY_SAFE. It does not parse trailing streams after that decision or collect the streams in a list.This also addresses the eager parsing noted in the review of this PR. The regression test uses harmless serialized dictionaries and a controlled unsafe verdict, then asserts that the parser was called exactly once with either complete or truncated trailing streams. Additional tests cover an invalid first stream and the empty-file exception. The existing real-analysis unsafe-second-stream test and benign single/stacked-stream controls are retained.
Validation on Python 3.12.15: the focused file passes all 5 tests, and the full suite passes 157 tests plus 151 subtests with no skips. Formatting and Ruff lint checks pass. The advisory type check reports the same 62 diagnostics before and after the change. The focused tests execute every statement and both reported branches of the updated helper.
Files previously accepted from their first stream alone can now be rejected by a later stream. In particular, legacy PyTorch files contain multiple streams, so later streams are now subject to the same safety ratings. This retains the intended behavior change of the original PR.
AI assistance was used in drafting and validating this change; the patch and tests were reviewed and verified by the author.