Bound the cost of identify() on very large files - #77
Open
adepasquale wants to merge 4 commits into
Open
Conversation
is_executable() and the recognized-extension check in identify() only need a prefix to decide (a magic-byte startswith), but both read f.contents, which materializes the entire file into memory. Use the already-cached f.header (first 1MB) instead, so a large executable or a large file with a recognized extension no longer forces a full read before identify() returns.
File.contents reads and caches the entire file; several identifiers in ident.py only need to sniff the head of a file to classify it, but capping .contents itself is not safe (File.stream, File.read(), and several unpackers/tests rely on it being the full buffer). Add a separate accessor, mirroring the existing File.header pattern, that never reads past MAX_IDENT_SCAN_SIZE regardless of file size. Not wired into any identifier yet.
nodejs() ran 24 unbounded regex scans over the entire file for every non-executable, non-trusted-archive file that reaches identify(). Most of these patterns are \b-anchored, which defeats re's literal-prefix optimization, so the regex VM steps through every byte offset (~60 MB/s) instead of skipping via a fast literal search (~3500+ MB/s). Measured on a 250MB sample, nodejs() alone accounted for ~95% of identify()'s ~190s runtime. Pair each pattern with the literal substring(s) that must appear in any string it can match, and only run the regex when that cheap check passes first. This is behavior-preserving (identical results, same count>=3 threshold) and drops nodejs()'s cost to near zero on files that don't contain any node.js markers, which is the overwhelming majority of large files reaching this identifier. Also switch nodejs() to the new capped f.scan_buffer instead of f.contents, so a match late in a very large file can't force a full read, and pattern matching stays bounded even if new patterns are added later.
- test_nodejs_literal_prefilter: for every (literals, pattern) pair in nodejs_patterns, checks a minimal matching sample passes the literal gate and the regex itself, and that every pattern is covered. Guards against a pattern being added/edited without a matching literal, which would silently disable it. - test_nodejs_scan_buffer_cap_contents / _stream: verify nodejs() correctly returns "nodejs" for markers within MAX_IDENT_SCAN_SIZE and None for markers placed past the cap, via both the in-memory contents path and the on-disk stream path of File.scan_buffer.
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
1 participant
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.
Problem
sflock.ident.identify()takes ~190–195 seconds on a 250MB non-archivesample. Callers reasonably expect file-type identification to be a fast,
near-constant-time operation.
This was root-caused in production via
py-spyagainst a stuck CAPEv2 worker:demux_sflock()inlib/cuckoo/common/demux.pycallssflock.unpack()synchronously before task creation, and the thread was parked inside
ident.nodejs()at every sample point across a ~95s profiling window.Two independent causes:
1.
nodejs()accounted for ~95% of the runtime. It ran 24re.search()calls over the entire file buffer unconditionally, with the
count >= 3threshold only checked after the full sweep. The mechanism is specific: 20 of
the 24 patterns begin with
\b, which defeats CPythonre's literal-prefixoptimization, so the regex VM steps every byte offset (measured ~60 MB/s per
pattern) instead of using a fast literal search (~3,500 MB/s for a plain
substring scan). That ~60x per-scan gap is why
nodejs()alone dwarfed theother ~20 content identifiers combined (~4.7s).
2.
identify()'s two early-return fast paths read the whole file first.is_executable()and the recognized-extension check are pure prefix tests, butboth went through
f.contents, materializing the entire file before eithercould return.
Changes
Four commits, each independently reviewable:
e25ac81is_executable()and the extension fast path usef.header(first 1MB, already cached) instead off.contents. Identical semantics — both arestartswith()prefix tests.6de0ff8File.scan_bufferaccessor, mirroring the existingFile.headerpattern, bounded byMAX_IDENT_SCAN_SIZE(16MB, new constant inconfig.py).7a84f97nodejs()pattern with the literal substring(s) mandatory for any string it can match; run the regex only when the cheap literal check passes. Switch toscan_buffer, and return as soon ascount >= 3.31a4a02File.contentsis deliberately not capped — it is load-bearing forFile.stream, the publicFile.read(),unpack/mso.py,unpack/eml.py, and anumber of existing test assertions that check exact lengths and hashes. Hence
the separate accessor.
Results
nodejs()on the 250MB sample: ~86–100s → 0.265sidentify()end to end, worst case (file falls through to the full identifierloop): ~190s → ~4.7s
identify()on a large PE/ELF or any file with a recognized extension:returns without reading past the first 1MB
The residual ~4.7s is the other substring-based identifiers (
visualbasic,javascript,powershell, and the.count()-based ones), untouched here.Behavior notes for reviewers
nodejs()literal gating is exactly behavior-preserving: the literalsare mandatory substrings, so gating cannot change which patterns match. The
early
returnis equivalent because the function only ever comparescountagainst 3.
nodejs()now inspects at most the first16MB. A file whose only Node.js markers appear past 16MB is no longer
detected. This is the accuracy-for-bounded-runtime tradeoff, and 16MB is
configurable in one place.