| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
| /** | ||
| * The given `read` reads multiple variables at once. `var` is bound to one of them. | ||
| */ |
These consistency violations originate from the VariableCapture instantation in C#, JS, Python, and Ruby.
The previously-added consistency errors are gone.
Unified also had consistency errors from its LocalSSA instantiation, due to its use of synthetic read nodes to represent post-update positions. Many variables can have a post-update at the same CFG node.
Many tests passed for the wrong reasons, due to the SSA bug. We need more library/operator modelling to actually find these flows.
Switched to TaintTracking and adds some very ad-hoc steps to recover most of the results. Some more tests pass and others fail; these are now consistent with what we actually model.
The C++ instantiation of DataFlowIntegration generated a bad join order
There was a problem hiding this comment.
Variable correlation remains incomplete in post-update, must-flow, and barrier joins, and the new Swift URL rule overmatches user-defined calls.
Review effort: Balanced
Findings: 1 · 1
This PR makes SSA data-flow nodes variable-specific when multiple variables share one CFG node and adds consistency coverage.
Changes:
| File | Description |
|---|---|
| shared/ssa/codeql/ssa/Ssa.qll | Qualifies expression nodes by variable and adds consistency checks. |
| shared/dataflow/codeql/dataflow/VariableCapture.qll | Exposes SSA ambiguity checks for variable capture. |
| unified/ql/consistency-queries/LocalSsaConsistency.ql | Enables the new unified consistency check. |
| unified/ql/lib/codeql/unified/internal/dataflow/DataFlowPluginSwift.qll | Adds Swift-specific taint steps. |
| unified/ql/src/queries/security/CWE-022/PathInjection.ql | Switches path injection to taint tracking. |
| unified/ql/test/library-tests/dataflow/test.swift | Adds a shared-CFG-node regression case. |
| unified/ql/test/library-tests/dataflow/test.expected | Updates generated data-flow expectations. |
| unified/ql/test/query-tests/security/CWE-022/PathInjection/testPathInjection.swift | Updates path-injection annotations. |
| unified/ql/test/query-tests/security/CWE-022/PathInjection/PathInjectionTest.expected | Updates generated path-injection results. |
| rust/ql/consistency-queries/SsaConsistency.ql | Enables SSA ambiguity checking for Rust. |
| ruby/ql/consistency-queries/SsaConsistency.ql | Enables SSA ambiguity checking for Ruby. |
| java/ql/consistency-queries/SsaConsistency.ql | Enables SSA ambiguity checking for Java. |
| csharp/ql/consistency-queries/SsaConsistency.ql | Enables SSA ambiguity checking for C#. |
| python/ql/test/library-tests/dataflow/variable-capture/dataflow-capture-consistency.expected | Updates generated consistency expectations. |
| javascript/ql/test/library-tests/FlowSummary/CaptureConsistency.expected | Updates generated consistency expectations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
| TExprNode(DfInput::Expr e, SourceVariable v, Boolean isPost) { | ||
| exists(Definition def | | ||
| def.getSourceVariable() = v and | ||
| e = DfInput::getARead(def) | ||
| ) |
| exists(CallExpr call | | ||
| call.getCallee().(Identifier).getValue() = ["URL", "NSURL"] and | ||
| node1.isResultValue(call.getNamedArgument("string")) and | ||
| step.taint() and | ||
| node2.isResultValue(call) |
| /** Gets the underlying expression. */ | ||
| DfInput::Expr getExpr() { result = e } | ||
|
|
||
| /** Holds if represents the access to `var` performed at `expr`. */ |
There was a problem hiding this comment.
grammar
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM! Thanks for fixing this.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fixes an issue with the SSA data-flow integration library that occurs when multiple variables are read at the same CFG node.
This situation happens when it is instantiated from the VariableCapture library. It also happens in unified due to how we use synthetic reads to mark post-update positions.
The assumption has a subtle presence in this bit of code:
Since this is only bound to the expression, the variableRead join can fan out to multiple variables when not uniquely determined by the CFG node. Even if Expr is constructed to be unique to the variable, the correlation is forgotten in this join and they get mixed up anyway. The fix was to make TExprNode unique to a specific variable.
The real culprit is the Expr class from the data-flow integration input. I propose we remove Expr entirely and replace ExprNode with ReadNode: a canonical representative for a (bb,i,v) triple from the variableRead input, and likewise for PostUpdateNode. But such a change is too large for this PR as it requires language-specific refactorings. But the fix is essentially a precursor to this solution: when Expr is a CFG node, ReadNode is effectively a (bb,i,v) triple as it rightfully should be.