Conversation
A hash signature keeps its name as a pooled string plus an 8-byte entry in a parallel virusnames[] array. On a 424 MB signature set that is 80.7 MB of name bytes and 26.4 MB of pointers against 53.2 MB of hash payload, so the names cost more than twice what they name. Interning them is worthless - 3,290,969 of 3,292,090 are distinct - but they share long prefixes: sorted neighbours agree on 21.7 bytes of a 23.5 byte average. Sorting them and front coding each entry as [lcp][len][tail], with a restart point every 16 entries so a decode has somewhere to begin, stores the same names in 18.0 MB. name_idx[] then replaces the pointer array with one 32-bit entry number per signature. Names are decoded on match into a per-root cache that lives as long as the engine, which is the lifetime evidence_get_last_alert() documents for the pointer it hands out. hm_addhash_str() and hm_addhash_bin() copy the name they are given rather than taking ownership, so cli_loadhash() and openioc_parse() pass an ordinary cli_virname() and free it. The sort is preceded by a counting sort on the name arena offset. Entry order arrives from a hash-table walk, so comparing names in that order makes every merge pass a random walk over the arena; on this set that alone is 1.6 s of the load time below. hm_addhash_bin() still grows both arrays one entry at a time. Growing them geometrically and shrinking to size at flush - what issue Cisco-Talos#1118 asks for - was tried first and gave up most of the saving: mpool does not return an over-allocated tail to the OS, and mpool_realloc() shrinking across a size class allocates the smaller block before freeing the larger one into a free list, so the shrink grows the arena. On the build pair that was measured, per-entry growth saved 99,630 KB of compiled RSS where geometric growth saved 5,918 KB. RSS after cl_engine_compile(), 3,849,249 signatures, 3 runs per arm, spread under 0.01 %: mpool (as shipped) 1,135,465 KB -> 1,036,364 KB -99,101 (-8.7%) DISABLE_MPOOL=ON 1,392,657 KB -> 1,122,487 KB -270,170 (-19.4%) Peak RSS falls with it, 1,135,465 -> 1,129,584 KB, although the table is built while the load-time name arena is still resident. Engine load time rises 10.21 s -> 11.87 s (+16 %) on that set, from sorting 3.3 M names. Scan time over a 2,401 file tree is unchanged within the spread of the measurement, -0.6 +/- 1.1 s on 127 s. hm_flush() returns cl_error_t so that a failed allocation while building the table is not swallowed.
![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: a94a1b4a87
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Materialising a hash signature's name can fail now that the name is decoded on demand, so cli_hm_scan() and cli_hm_scan_wild() can return CL_EMEM where before they returned only CL_CLEAN or CL_VIRUS. Their callers were written against the old contract and test for CL_VIRUS alone, so that error is discarded. In cli_scan_fmap() the size-based result is overwritten by the wildcard scan two lines later, and the section and import-hash paths in pe.c fall through to the next hash type. A signature that did match is then reported clean. Each caller now separates the three outcomes. cli_check_fp() treats a failed lookup the way it already treats a failed hash calculation, by keeping the alert rather than trusting a file it could not check against the false positive signatures. The authenticode path in pe.c is unchanged: it passes a NULL virname, so it never materialises a name. Also assigns the return value of the import-hash wildcard cli_append_virus() call, which was dropped while the line below it tested the previous value.
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.
A hash signature keeps its name as a pooled string plus an 8-byte entry in a
parallel
virusnames[]array. On a 424 MB signature set that is 80.7 MB of namebytes and 26.4 MB of pointers against 53.2 MB of hash payload — the names cost
more than twice what they name.
Interning them is worthless: 3,290,969 of 3,292,090 are distinct. But they share
long prefixes — sorted neighbours agree on 21.7 bytes of a 23.5 byte average — so
sorting them and front coding each entry as
[lcp][len][tail]stores the samenames in 18.0 MB. A restart point every 16 entries gives a decode somewhere to
begin, and
name_idx[]replaces the pointer array with one 32-bit entry numberper signature.
Names are decoded on match into a per-root cache that lives as long as the
engine, which is the lifetime
evidence_get_last_alert()documents for thepointer it hands out.
hm_addhash_str()andhm_addhash_bin()copy the namethey are given rather than taking ownership, so
cli_loadhash()andopenioc_parse()pass an ordinarycli_virname()and free it.The sort is preceded by a counting sort on the name arena offset. Entry order
arrives from a hash-table walk, so comparing names in that order makes every
merge pass a random walk over a 77 MB arena; on this set that pre-pass is
1.6 s of the load time below.
Measured
424 MB signature set (3,849,249 signatures), RSS after
cl_engine_compile(),3 runs per arm, spread under 0.01 %:
DISABLE_MPOOL=ONPeak RSS falls with it, 1,135,465 → 1,129,584 KB, even though the table is built
while the load-time name arena is still resident.
Engine load time rises 10.21 s → 11.87 s (+16 %) on that set, from sorting
3.3 M names; both arms' run-to-run spread is under 1.5 %. Scan time over a
2,401-file tree is unchanged within the spread of the measurement, -0.6 ± 1.1 s
on 127 s.
Behaviour
Scan output is byte-identical over that tree, EICAR control included. The one new
failure mode is an out-of-memory while decoding a name, which makes
cli_hm_scan()returnCL_EMEMrather than report a match it cannot name;hm_flush()returnscl_error_tfor the same reason, so a failed allocationwhile building the table is not swallowed.
Verified with the test suite (same failure set as the base), a
byte-for-byte scan-output comparison over 2,401 files, an OpenIOC database
load, an 8-thread concurrent-decode run, the escaped-length paths at names up
to 608 bytes, and AddressSanitizer with a positive control.