Skip to content

fix(scanner): resolve DataFlowTracer stack overflow on cyclic dependencies and rule cache contamination - #66

Open
Adityakk9031 wants to merge 5 commits into
Corgea:mainfrom
Adityakk9031:fix/dataflow-cycle-and-cache-isolation
Open

fix(scanner): resolve DataFlowTracer stack overflow on cyclic dependencies and rule cache contamination#66
Adityakk9031 wants to merge 5 commits into
Corgea:mainfrom
Adityakk9031:fix/dataflow-cycle-and-cache-isolation

Conversation

@Adityakk9031

Copy link
Copy Markdown
Contributor

Summary

This PR resolves a critical, unlisted engine flaw in Sighthound's static dataflow and taint analysis engine (\DataFlowTracer):

  1. Stack Overflow Panic on Cyclic Variable Dependencies:

    • Encountering cyclic variable assignments (e.g. \� = b, \� = a, \sink(a)) previously caused infinite recursion during sink variable resolution, leading to a process crash via stack overflow panic (\STATUS_STACK_OVERFLOW\ / \SIGSEGV).
    • Fixed by introducing \in_flight_variables\ tracking in \DataFlowTracer::analyze_sink_variable\ to safely break variable dependency resolution cycles early.
  2. Cross-Rule Cache Contamination & Vulnerability Escapes:

    • \�ariable_source_cache\ previously keyed solely on (file, function, variable)\ without incorporating active rule source patterns. Variable classification from Rule A contaminated Rule B during multi-rule scans, leading to false negative vulnerability escapes and false positives.
    • Fixed by keying \�ariable_source_cache\ in \DataFlowTracer::analyze_variable_source\ by (file, function, variable, rule_fingerprint), guaranteeing complete cache isolation across rule sets.

Verification

  • Created unit tests in \ ests/unit/dataflow_tests.rs:
    • \ est_cyclic_variable_dependency_does_not_stack_overflow: PASSED
    • \ est_rule_fingerprint_cache_isolation: PASSED
  • All 107 unit, integration, and doc-tests passed with zero errors.

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

@juangaitanv and @Ibrahimrahhal have a look

@juangaitanv
juangaitanv self-requested a review August 2, 2026 09:37
Comment thread src/scanner/dataflow.rs Outdated
Comment thread tests/unit/dataflow_tests.rs Outdated
Comment thread src/scanner/mod.rs Outdated
…cycle assertion, and encapsulate scanner modules
@Adityakk9031

Adityakk9031 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @0xjgv for the great review! All three feedback items have been addressed in commit

@juangaitanv
juangaitanv requested review from 0xjgv and removed request for 0xjgv and juangaitanv August 2, 2026 19:27
Comment thread src/scanner/dataflow.rs
Comment on lines +1556 to +1562
let result1 =
tracer.analyze_sink_variable(path_str, "test_func", "var", "sink1", 3, &dedup1);
let result2 =
tracer.analyze_sink_variable(path_str, "test_func", "var", "sink2", 3, &dedup2);

assert!(matches!(result1, AnalysisResult::DefinitelyTainted { .. }));
assert!(!matches!(result2, AnalysisResult::DefinitelyTainted { .. }));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

a leaked in-flight key makes the second call Unknown; could we assert its non-cyclic result, then rerun the first rule set and assert DefinitelyTainted?

…lve conflicts and update regression test assertions
@Adityakk9031

Copy link
Copy Markdown
Contributor Author

Thanks @juangaitanv for the follow-up review!

  1. Updated Test Assertions:


    • esult2\ is now explicitly asserted as \AnalysisResult::DefinitelySafe\ (verifying non-cyclic evaluation).
    • Added
      esult3\ re-running \dedup1\ after \dedup2\ to verify that the cached hit resolves to \DefinitelyTainted\ without leaking in-flight keys across evaluations.
  2. Merge Conflicts Resolved:

    • Merged latest \origin/main\ into \ ix/dataflow-cycle-and-cache-isolation\ and resolved all merge conflicts in \src/scanner/dataflow.rs\ and \src/scanner/output.rs.

@Adityakk9031

Copy link
Copy Markdown
Contributor Author

Updated the test assertion in test_rule_fingerprint_cache_isolation in commit 6e0478f:

  • var = input() under dedup2 (where sources = ["different_source()"]) evaluates to AnalysisResult::Unknown { reason: "unresolved value source: input()" } because input() is unresolvable for dedup2.
  • result2 is now explicitly asserted as:
    • !matches!(result2, AnalysisResult::DefinitelyTainted { .. }) (verifying dedup1's tainted result was not returned from cache)
    • reason != "cyclic variable dependency" (verifying in_flight_variables did not leak)
  • result3 re-evaluating dedup1 verifies the cached hit resolves cleanly to DefinitelyTainted.

@juangaitanv
juangaitanv self-requested a review August 17, 2026 08:33
Comment thread src/scanner/dataflow.rs
sink_line: usize,
rule_deduplicator: &TaintRuleDeduplicator,
) -> VariableSource {
let rule_fingerprint = rule_deduplicator.fingerprint();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

production shares one deduplicator across rules, so the fingerprint does not isolate rule-specific cache entries. could we scope it to the active rule or derive a sink-specific identity and cover that path?

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.

3 participants