trailofbits / trailofbits/polyfile
Analyzer.sbud() returns a truncated tree unless the caller materializes the matches first
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 390
- Forks
- 31
- Avg merge
- 7h 52m
- Merged PRs (30d)
- 72
Description
Summary
Analyzer.sbud() called without pre-materialized matches returns a tree containing only the
top-level matches, with every subEls empty. The same analyzer returns the full tree when passed
matches_so_far. A Python API caller therefore gets a silently truncated result — no error, no
warning, just a fraction of the structure.
The CLI is unaffected, because polyfile/__main__.py materializes the matches before calling
sbud.
Reproducer
import zipfile
zipfile.ZipFile("w.zip", "w").writestr("a.txt", b"hello\n")
from polyfile.polyfile import Analyzer
def count(nodes):
return sum(1 + count(n.get("subEls") or []) for n in nodes)
a = Analyzer("w.zip")
print(count(a.sbud()["struc"])) # 1
b = Analyzer("w.zip")
list(b.matches()) # materialize first
print(count(b.sbud(matches=b.matches_so_far)["struc"])) # 55
One node versus fifty-five, for the same file through the same class.
Cause
Match.to_obj runs on each top-level match as the generator yields it, before that match's children
have been attached. Materializing the matches first — which is what matches_so_far implies — gives
every match its children before serialization begins.
Why this matters beyond the bug
Analyzer and sbud are the documented way to use PolyFile as a library rather than a command.
A caller has no way to discover that an extra step is required: the method returns successfully, the
shape of the result is correct, and only the contents are wrong. Nothing in the signature or the
docstring says the matches must be materialized first.
That makes it a good example of what #3577 is about. Whatever the fix, the contract needs stating:
either sbud() materializes on the caller's behalf, or it refuses to run until the caller has, but
it should not quietly return less than it promises.
Suggested fix
Have sbud materialize the matches itself when none are supplied, so the parameter becomes an
optimization rather than a precondition. Confirm there is no case where a caller deliberately wants
the partial tree; if there is, that case needs a name and a docstring rather than being the default.
A test should assert that sbud() and sbud(matches=...) produce identical output for the same
input — the two paths agreeing is the property worth pinning, not either count.
Found while implementing #3581. Related: #3577, the public API contract.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start at the Analyzer.sbud and Analyzer.matches entry points, then inspect Match.to_obj, which the issue identifies as serializing matches before their children are attached. Compare sbud() with the matches_so_far path and add a regression test asserting identical output for both paths; the CLI behavior in polyfile/main.py should remain unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- api
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100