Conversation
…pleted A rule file passed through --yara-rules-dir that YARA cannot compile, or that SkillSpector cannot decode as UTF-8/base64, is dropped whole with no signal above debug-level logging. _load_rules already counted these (materialize_skipped + compile_skipped) but only logged the total; node() never saw it, so every scanned component could still report COMPLETED and the recommendation stayed SAFE, because the rule that would have flagged something simply never ran. --fail-on-incomplete correctly has nothing to key off, so it exits 0. Kept _load_rules's existing single-value signature: every current monkeypatch.setattr(static_yara, "_load_rules", ...) test double in the suite returns a bare yara.Rules object, and changing the return shape to a tuple would have broken all 15 of them for an internal detail those tests don't exercise. The skip count is instead recorded on the same module-level cache the compiled rules already live on, read back via the new rules_skipped_count(), and folded into a PARTIAL ledger event scoped to the rule set (not a scanned skill file, hence the synthetic "yara_rules/" path and LedgerRecordType.SYSTEM) using the existing READ_ERROR reason. That event flows through node()'s existing degraded/completed decision unchanged, so --fail-on-incomplete now has something real to key off. Test builds a valid rule and a syntactically broken one in the same --yara-rules-dir (a real YARA syntax error, not a decode failure, to match the issue's own repro), asserts the valid rule still fires, the analyzer status is not "completed", and the ledger records the drop. Negative control: reverting only the source fails with status == "completed" — the exact false-SAFE the issue reports. Fixes NVIDIA#554 Signed-off-by: Souptik Chakraborty <62941615+Souptik96@users.noreply.github.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed current head 4e753fe71cae2a3ecfe7df258c760115a1ed3f6c, including the complete two-file diff, surrounding rule-cache and analyzer-status logic, tests, existing discussion, and exact-head checks. The mixed valid/invalid-rule case is now surfaced in the ledger, and all five hosted checks pass.
Changes are requested because the skipped-rule count is stored in a module global and read separately after _load_rules returns. Concurrent scans with different rule directories can interleave those operations, so one scan can consume another rule set's count and still report completed after its own rule was dropped. Bind the skip metadata atomically to the returned/cached compiled rule set (or protect the load-and-read operation with appropriate synchronization) and add a deterministic concurrency regression.
| return _rule_limit_response(exc.reason, dict(exc.metrics)) | ||
| finally: | ||
| _RULE_LOAD_DEADLINE.reset(deadline_token) | ||
| rules_skipped = rules_skipped_count() |
There was a problem hiding this comment.
[P1] Bind skipped-rule metadata to the returned rules
rules and rules_skipped are obtained from two separately mutable module globals. Two concurrent MCP/graph scans can interleave after _load_rules(A) returns: scan B can load rules B and overwrite _rules_skipped_count before scan A calls rules_skipped_count(). Scan A then runs rules A with B's count, potentially reporting completed even though an A rule was dropped. Return/cache the compiled rules and their skip metadata as one value, or lock the load-and-read transaction, and add a regression that forces this interleaving.
yashrajp22
left a comment
yashrajp22
left a comment
There was a problem hiding this comment.
The review of head 4e753fe71cae2a3ecfe7df258c760115a1ed3f6c is complete. Two additional fixes are needed: the synthetic rule-load event can collide with a real file, and rejected rules still lack the default-level diagnostics requested in #554. The existing skip-count concurrency finding also remains reproducible; I have not duplicated that comment.
The ordinary mixed valid/invalid-rule case now correctly produces a nonfatal partial report, strict CLI exit 1, and safe_to_install=false through programmatic MCP.
Validation used fresh wheels and pinned source for base c13f70ebf14905912c616a58c9a8cb8112ef94a4 and this head. All 12 complete sample directories ran in all four combinations (48 scans), with matching source/wheel reports. The 98 selected tests passed in each HEAD mode. Focused checks covered malformed syntax/encoding/BOM, cache transitions, concurrency, ledger identity, CLI/MCP, suppression and all report formats, resource/failure precedence, and recursive/transitive aggregation. Greptile's cache-metadata observation was independently reproduced and grouped with the existing shared-metadata finding.
Scope: Linux and offline checks; transitive remote targets were mapped to local fixtures. Nine sample reports remain partial for unrelated reference/obfuscation limitations, so this is not an all-rule accuracy or live-provider result. The PR contribution is based on 2e9ae8d1cfa6e339f7035f876d3ac2e1c6ce24e6; unrelated AS3 differences from newer main were kept separate.
| # set itself. Ledger paths must be relative POSIX paths, and | ||
| # the real rules directory (builtin or --yara-rules-dir) is | ||
| # absolute, so it cannot be used here. | ||
| path="yara_rules/", |
There was a problem hiding this comment.
Could we give rule-load events a work ID that cannot overlap with component work? With a valid file named yara_rules and one rejected custom rule, the ledger normalizes this path to yara_rules, so both events have the same static_yara work ID. I reproduced fatal unaccounted_work, execution_successful=false, and CLI exit 2. This should remain a nonfatal partial scan (strict exit 1). Changing only the synthetic filename would still allow another valid filename to collide.
| sources, materialize_skipped = _build_namespace_map(rule_files, raw_cache=raw_cache) | ||
| compiled, compile_skipped = _compile_rules(sources) | ||
| skipped = materialize_skipped + compile_skipped | ||
| _rules_skipped_count = skipped |
There was a problem hiding this comment.
Could we also report each rejected rule at the default WARNING level, including its filename and a bounded decode/compile reason, as #554 requests? A malformed acme.yar, a BOM rule, and a non-UTF-8 .yar still produce no warning. This count now makes ordinary scans partial, but the public event only says File content could not be read for yara_rules, so the user cannot identify or repair the dropped detector. The rejection handlers remain at DEBUG.
…iles Addresses the three review findings on NVIDIA#557. All three share one shape: the dropped-rule total was reported through a channel not tied to the scan that produced it. 1. Skip count raced across concurrent scans (rng1995, P1) `node()` called `_load_rules()` and then read `rules_skipped_count()` as a separate step. Two concurrent MCP/graph scans can interleave between those: scan B loads its own rule set and overwrites `_rules_skipped_count` before scan A reads it, so A runs rules A while reporting B's total. If B skipped nothing, A reports `completed` even though one of A's own rules was dropped -- the false-clean result NVIDIA#554 exists to prevent. Adds `load_rules_with_skips()`, which returns the rules and their own skip count from one transaction guarded by a reentrant `_RULES_LOCK`, and switches `node()` to it. `_load_rules()` keeps its single-value signature, and `load_rules_with_skips` calls it through the module global, so every existing `monkeypatch.setattr(static_yara, "_load_rules", ...)` double still applies. `rules_skipped_count()` is retained for single-threaded callers and now reads under the lock. The three cache globals are documented as one logical value that must only be written or read as a set. The lock serializes rule compilation across concurrent scans. That is a deliberate trade: compilation is cached and already deadline-bounded, and a scanner reporting a false clean is worse than one loading rules serially. 2. Rule-load event collided with a component of the same name (yashrajp22) `ledger_event` derives the work identity as `analyzer_id or f"{record_type}:{phase}"`, and the synthetic `yara_rules/` scope normalizes to `yara_rules`. Passing `analyzer_id=ANALYZER_ID` therefore produced the same work ID as the planned work item for a scanned component literally named `yara_rules`: both planned targets resolved to two matching events, and reconciliation raised a fatal `unaccounted_work` with `execution_successful=false` and CLI exit 2, instead of the nonfatal partial scan this event is meant to record. Omits `analyzer_id` on that one event so the identity falls back to `system:static`, which is disjoint from every analyzer work item by construction. As the review noted, renaming the synthetic path alone would only move the collision to the next unlucky filename. 3. Rejected rules were invisible at default log level (yashrajp22, NVIDIA#554) Both rejection handlers logged at DEBUG, so a malformed `acme.yar`, a BOM rule, or a non-UTF-8 `.yar` produced no default-level warning, and the public ledger event is scoped to the rule set rather than the file. The operator could see that a detector was dropped but not which one to repair. Both handlers now log at WARNING, naming the file and a bounded reason. `_build_namespace_map` optionally fills a `{namespace: filename}` map -- passed in rather than returned, to keep its two-value signature -- so the compile path can name `acme.yar` instead of the extension-stripped namespace `acme`. `_bounded_rejection_reason` collapses newlines and caps the echoed text at 200 characters, because rule sources are attacker-influenced when `--yara-rules-dir` points at untrusted content and YARA errors can quote the offending source line. Tests New `TestRuleSkipAccounting` (9 tests): a deterministic pairing test, a serialization test that asserts the lock is genuinely held for the whole load-and-read transaction rather than racing and hoping, a contended two-thread test over 50 observations, the `yara_rules` work-ID collision case asserting both event and planned-work IDs stay distinct, three parametrized rejection-diagnostic cases (malformed, BOM, non-UTF-8), and two bounding tests. The contended test surfaces worker-thread exceptions and asserts an observation count, so it cannot pass vacuously when the scans never ran. The autouse cache fixture now also resets `_rules_skipped_count`, which is part of that cache and would otherwise leak between tests. Verification - Negative control: all 9 new tests fail with the source change reverted and the tests kept; 9/9 pass with it. - `tests/nodes/analyzers/test_static_yara.py`: 96 passed. - Full suite: 18 pre-existing failures, byte-identical to the same run on unmodified `4e753fe` (build_context, compare_scan_accuracy, create_github_release, input_handler, json_container_ownership, security_end_to_end -- all environmental, none in the touched files). - `ruff check`, `ruff format --check`, and `mypy` clean on both files. - Windows / Python 3.13 only; the pre-existing failures above are consistent with that environment rather than with this change. Signed-off-by: Souptik Chakraborty <62941615+Souptik96@users.noreply.github.com>
|
Thanks both — the reviews were specific enough to fix directly, and @rng1995's point about the two globals was the one I should have caught myself. All three turned out to be the same shape: the dropped-rule total was reported through a channel that wasn't tied to the scan that produced it — a module global read after the fact, a ledger work ID shared with component work, and a DEBUG log nobody sees at default verbosity. ① Skip count bound to its rules — @rng1995 [P1]
Done both. New
Confirmed the interleaving before fixing it — scan A drops one rule, scan B loads a clean set, A then reads B's count: For the regression you asked for, I did not want a test that passes on timing luck, so there are two. One trade to flag explicitly: the lock serializes rule compilation across concurrent scans. I judged that acceptable because compilation is cached and already deadline-bounded, and a scanner reporting a false clean is worse than one loading rules serially. If you would rather not serialize compilation, the alternative is caching ② Rule-load work ID can no longer collide — @yashrajp22
Agreed, and that last sentence is why I did not just rename the path. Reproduced your exact case first (a component named The regression asserts both the event IDs and the advertised ③ Rejected rules named at default level — @yashrajp22, #554
Both handlers now log at WARNING with the filename and a bounded reason. Your three cases: This needed one change beyond the log level, which is worth calling out: the compile path only had the namespace, and
Verification
One thing I found in my own tests rather than let it sit: the contended-threads test initially passed vacuously, because an exception inside a worker thread does not fail a pytest test. It now captures worker exceptions and asserts an observation count of 50, so it cannot pass when the scans never ran. Also extended the autouse cache fixture to reset Disclosure: written with AI assistance under my direction. I reproduced each finding before fixing it, ran the negative control and full-suite comparison myself, and reviewed this comment before posting. |
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head 6e07493c9956dcf2ad7b2f032f58b125cc978fa9 against all three prior threads, the complete rule-cache/ledger/logging diff, concurrency tests, surrounding no-rules paths, and exact-head checks.
The lock now makes a successful load-and-count transaction atomic, and the work-ID collision plus default-level rejected-file reporting are addressed. One cache-integrity path remains. _load_rules() sets _rules_skipped_count and returns without replacing or clearing _compiled_rules / _rules_hash when no rule files exist or compilation yields no rules. A later request for the previously cached hash then returns those cached rules paired with the intervening load's count. For example, load A with one valid and one rejected rule, load an empty/all-rejected set B, then load A again: the A cache hit can report B's count (including zero), recreating a false-complete YARA scan. Keep rules, hash, and skip count as one immutable cache entry or invalidate the cached rules/hash on every non-populating path; add this A→B→A sequence as a regression.
All six exact-head checks pass, but this remaining completeness-accounting defect and active change requests block merging.
Priority: P0 — incorrect rule-drop accounting can make an incomplete malware scan look complete.
3 participants
Fixes #554
What was wrong
A rule file passed through
--yara-rules-dirthat YARA cannot compile, or that SkillSpector cannot decode as UTF-8/base64, is dropped whole with only debug-level logging._load_rulesalready counted these (materialize_skipped + compile_skipped), but only logged the total —node()never saw it, so every scanned component could still reportCOMPLETED,analysis_completeness: complete, and the recommendation stayed SAFE, because the rule that would have flagged something simply never ran.--fail-on-incompletecorrectly has nothing to key off, so it exits 0.Reproduced with the issue's own scenario: a workspace with a valid custom rule and a syntactically broken one in the same
--yara-rules-dir. The good rule fires, but the run reports a clean scan regardless.What this changes, and a design choice I want to flag
_rules_skipped_count, read back via the newrules_skipped_count()), and folded into aPARTIALledger event innode(), using the existingREAD_ERRORreason andLedgerRecordType.SYSTEM(it isn't scoped to a scanned skill file, so I used a synthetic"yara_rules/"path — ledger paths must be relative POSIX, and the real rules directory is absolute).degraded/completeddecision innode()unchanged, so this is additive to the existing status machinery rather than a new mechanism._load_rules's return signature. My first pass returned(compiled_rules, skipped_count)as a tuple, which is the more obvious API, but 15 tests intest_static_yara.pydomonkeypatch.setattr(static_yara, "_load_rules", lambda _extra_dir: rules), returning a bareyara.Rulesobject — all of them would have silently broken by unpacking ayara.Rulesas a 2-tuple. I chose the module-global read-back instead specifically to avoid that blast radius for an internal detail those tests don't exercise. Happy to go the tuple route instead if you'd rather have the cleaner API and take the test-file diff — just say so.SYNTAX_ERROR(already reserved for "Python source could not be parsed" perREASON_MESSAGES) or invent a newLedgerReasonfor this;READ_ERROR's existing message ("File content could not be read") is generic enough to cover both the decode and compile failure cases the count already sums together.Testing
New test builds a valid rule and a syntactically broken one (missing closing brace, a real YARA syntax error, matching the issue's own repro rather than a decode failure) in the same
--yara-rules-dir, asserts the valid rule still fires, the analyzer status is not"completed", and the ledger records the drop withobserved_artifacts=1.Negative control, reverting only
static_yara.pyand keeping the test:Restoring the fix, all 87 tests in the file pass again.
I did not reproduce this on Windows (the issue notes the same result on Windows 10 and Ubuntu/WSL2) — tested on Linux only, CPython 3.12.14, yara-python==4.5.4 (same version the issue reports).