Skip to content

Inspect stacked pickle streams incrementally in is_likely_safe() - #345

Open
professor-moody wants to merge 1 commit into
trailofbits:masterfrom
professor-moody:fix-is-likely-safe-stacked-pickles
Open

professor-moody wants to merge 1 commit into
trailofbits:masterfrom
professor-moody:fix-is-likely-safe-stacked-pickles

Conversation

@professor-moody

@professor-moody professor-moody commented Nov 2, 2026 •

Copy link
Copy Markdown

is_likely_safe() previously based its answer on the first pickle stream. This change checks stacked streams incrementally and returns False as soon as a stream has an invalid opcode or is not rated LIKELY_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.

@dguido dguido left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 professor-moody changed the title Inspect all stacked pickle streams in is_likely_safe() Inspect stacked pickle streams incrementally in is_likely_safe() Nov 7, 2026
@professor-moody
professor-moody force-pushed the fix-is-likely-safe-stacked-pickles branch from 1c4d219 to 31f1f27 Compare November 7, 2026 17:18
@professor-moody

Copy link
Copy Markdown
Author

Thanks @dguido! Updated is_likely_safe() in 31f1f27b10ef to parse and inspect stacked pickle streams incrementally so it returns immediately on the first unsafe stream without preloading trailing streams.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants