Skip to content

Java: Adjust caching of BasicBlocks, BaseSSA, and CompileTimeConstants #19093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions java/ql/lib/semmle/code/java/Expr.qll
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ class CompileTimeConstantExpr extends Expr {
/**
* Gets the string value of this expression, where possible.
*/
pragma[nomagic]
cached
string getStringValue() {
result = this.(StringLiteral).getValue()
or
Expand All @@ -205,7 +205,7 @@ class CompileTimeConstantExpr extends Expr {
/**
* Gets the boolean value of this expression, where possible.
*/
pragma[nomagic]
cached
boolean getBooleanValue() {
// Literal value.
result = this.(BooleanLiteral).getBooleanValue()
Expand Down
32 changes: 28 additions & 4 deletions java/ql/lib/semmle/code/java/controlflow/BasicBlocks.qll
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,31 @@
import java
import Dominance

cached
private module BasicBlockStage {
cached
predicate ref() { any() }

cached
predicate backref() {
(exists(any(BasicBlock bb).getABBSuccessor()) implies any()) and
(exists(any(BasicBlock bb).getNode(_)) implies any()) and
(exists(any(BasicBlock bb).length()) implies any())
}
}

/**
* A control-flow node that represents the start of a basic block.
*
* A basic block is a series of nodes with no control-flow branching, which can
* often be treated as a unit in analyses.
*/
class BasicBlock extends ControlFlowNode {
cached
BasicBlock() {
not exists(this.getAPredecessor()) and exists(this.getASuccessor())
BasicBlockStage::ref() and
not exists(this.getAPredecessor()) and
exists(this.getASuccessor())
or
strictcount(this.getAPredecessor()) > 1
or
Expand All @@ -24,7 +40,10 @@

/** Gets an immediate successor of this basic block. */
cached
BasicBlock getABBSuccessor() { result = this.getLastNode().getASuccessor() }
BasicBlock getABBSuccessor() {

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in getABBSuccessor should be PascalCase/camelCase.
BasicBlockStage::ref() and
result = this.getLastNode().getASuccessor()
}

/** Gets an immediate predecessor of this basic block. */
BasicBlock getABBPredecessor() { result.getABBSuccessor() = this }
Expand All @@ -35,7 +54,9 @@
/** Gets the control-flow node at a specific (zero-indexed) position in this basic block. */
cached
ControlFlowNode getNode(int pos) {
result = this and pos = 0
BasicBlockStage::ref() and
result = this and
pos = 0
or
exists(ControlFlowNode mid, int mid_pos | pos = mid_pos + 1 |
this.getNode(mid_pos) = mid and
Expand All @@ -52,7 +73,10 @@

/** Gets the number of control-flow nodes contained in this basic block. */
cached
int length() { result = strictcount(this.getANode()) }
int length() {
BasicBlockStage::ref() and
result = strictcount(this.getANode())
}

/** Holds if this basic block strictly dominates `node`. */
predicate bbStrictlyDominates(BasicBlock node) { bbStrictlyDominates(this, node) }
Expand Down
21 changes: 20 additions & 1 deletion java/ql/lib/semmle/code/java/dataflow/internal/BaseSSA.qll
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,26 @@
import java
private import codeql.ssa.Ssa as SsaImplCommon

cached
private module BaseSsaStage {
cached
predicate ref() { any() }

cached
predicate backref() {
(exists(TLocalVar(_, _)) implies any()) and
(exists(any(BaseSsaSourceVariable v).getAnAccess()) implies any()) and
(exists(getAUse(_)) implies any())
}
}

cached
private newtype TBaseSsaSourceVariable =
TLocalVar(Callable c, LocalScopeVariable v) {
c = v.getCallable() or c = v.getAnAccess().getEnclosingCallable()
BaseSsaStage::ref() and
c = v.getCallable()
or
c = v.getAnAccess().getEnclosingCallable()
}

/**
Expand All @@ -31,6 +48,7 @@ class BaseSsaSourceVariable extends TBaseSsaSourceVariable {
*/
cached
VarAccess getAnAccess() {
BaseSsaStage::ref() and
exists(LocalScopeVariable v, Callable c |
this = TLocalVar(c, v) and result = v.getAnAccess() and result.getEnclosingCallable() = c
)
Expand Down Expand Up @@ -188,6 +206,7 @@ cached
private module Cached {
cached
VarRead getAUse(Impl::Definition def) {
BaseSsaStage::ref() and
exists(BaseSsaSourceVariable v, BasicBlock bb, int i |
Impl::ssaDefReachesRead(v, def, bb, i) and
result.getControlFlowNode() = bb.getNode(i) and
Expand Down
18 changes: 12 additions & 6 deletions java/ql/lib/semmle/code/java/environment/SystemProperty.qll
Original file line number Diff line number Diff line change
Expand Up @@ -269,18 +269,24 @@ private MethodCall getSystemPropertyFromSpringProperties(string propertyName) {
* for final variables.
*/
private predicate localExprFlowPlusInitializers(Expr e1, Expr e2) {
e1 = e2 or
localFlowPlusInitializers(DataFlow::exprNode(e1), DataFlow::exprNode(e2))
}

private predicate localFlowPlusInitializers(DataFlow::Node pred, DataFlow::Node succ) =
fastTC(localFlowStepPlusInitializers/2)(pred, succ)

/**
* Holds if data can flow from `pred` to `succ` in zero or more
* local (intra-procedural) steps or via instance or static variable intializers
* Holds if data can flow from `pred` to `succ` in a
* local (intra-procedural) step or via instance or static variable intializers
* for final variables.
*/
private predicate localFlowPlusInitializers(DataFlow::Node pred, DataFlow::Node succ) {
exists(Variable v | v.isFinal() and pred.asExpr() = v.getInitializer() |
DataFlow::localFlow(DataFlow::exprNode(v.getAnAccess()), succ)
private predicate localFlowStepPlusInitializers(DataFlow::Node pred, DataFlow::Node succ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this rewrite semantics preserving? It looks like before it would calculate flow as

pred --initializer--> access --local--*> succ

and now it is

pred --initializer/local--*> succ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, it's not 100% semantics preserving, but I think the tweaked version makes more sense: If we're doing local flow plus initialization of final fields, then it seems like a mistake to rule out local flow prior to the initialization, like e.g. a cast or something like that.

exists(Variable v |
v.isFinal() and
pred.asExpr() = v.getInitializer() and
succ.asExpr() = v.getAnAccess()
)
or
DataFlow::localFlow(pred, succ)
DataFlow::localFlowStep(pred, succ)
}