From ea664e08d1d79eef07760289e122a11e519d4d86 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 1 Sep 2025 15:00:34 +0200 Subject: [PATCH 1/3] Go: Fix some Ql4Ql violations. --- go/ql/lib/semmle/go/StringOps.qll | 7 +- .../go/dataflow/internal/DataFlowUtil.qll | 4 +- .../IntegerOverflow/RangeAnalysis.qll | 311 +++++++++--------- 3 files changed, 156 insertions(+), 166 deletions(-) diff --git a/go/ql/lib/semmle/go/StringOps.qll b/go/ql/lib/semmle/go/StringOps.qll index 351617abf9de..3463e4fdf878 100644 --- a/go/ql/lib/semmle/go/StringOps.qll +++ b/go/ql/lib/semmle/go/StringOps.qll @@ -306,11 +306,12 @@ module StringOps { */ class StringFormatCall extends DataFlow::CallNode { string fmt; - Range f; StringFormatCall() { - this = f.getACall() and - fmt = this.getArgument(f.getFormatStringIndex()).getStringValue() and + exists(Range f | + this = f.getACall() and + fmt = this.getArgument(f.getFormatStringIndex()).getStringValue() + ) and fmt.regexpMatch(getFormatComponentRegex() + "*") } diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll index 2cd1bbcc7476..14ff455646c9 100644 --- a/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll +++ b/go/ql/lib/semmle/go/dataflow/internal/DataFlowUtil.qll @@ -367,7 +367,7 @@ module BarrierGuard { } /** - * Holds if `guard` marks a point in the control-flow graph where this node + * Holds if `guard` marks a point in the control-flow graph where `g` * is known to validate `nd`, which is represented by `ap`. * * This predicate exists to enforce a good join order in `getAGuardedNode`. @@ -378,7 +378,7 @@ module BarrierGuard { } /** - * Holds if `guard` marks a point in the control-flow graph where this node + * Holds if `guard` marks a point in the control-flow graph where `g` * is known to validate `nd`. */ private predicate guards(Node g, ControlFlow::ConditionGuardNode guard, Node nd) { diff --git a/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll b/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll index eccb0b735896..828fc392a2df 100644 --- a/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll +++ b/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll @@ -347,91 +347,86 @@ float getALowerBound(Expr expr) { * Gets a possible upper bound of SSA definition `def`. */ float getAnSsaUpperBound(SsaDefinition def) { - if recursiveSelfDef(def) - then none() - else ( - if def instanceof SsaExplicitDefinition - then - exists(SsaExplicitDefinition explicitDef | explicitDef = def | - //SSA definition coresponding to a `SimpleAssignStmt` - if explicitDef.getInstruction() instanceof IR::AssignInstruction - then - exists(IR::AssignInstruction assignInstr, SimpleAssignStmt simpleAssign | - assignInstr = explicitDef.getInstruction() and - assignInstr.getRhs().(IR::EvalInstruction).getExpr() = simpleAssign.getRhs() and - result = getAnUpperBound(simpleAssign.getRhs()) - ) - or - //SSA definition coresponding to a ValueSpec(used in a variable declaration) - exists(IR::AssignInstruction declInstr, ValueSpec vs, int i, Expr init | - declInstr = explicitDef.getInstruction() and - declInstr = IR::initInstruction(vs, i) and - init = vs.getInit(i) and - result = getAnUpperBound(init) - ) - or - //SSA definition coresponding to an `AddAssignStmt` (x += y) or `SubAssignStmt` (x -= y) - exists( - IR::AssignInstruction assignInstr, SsaExplicitDefinition prevDef, - CompoundAssignStmt compoundAssign, float prevBound, float delta - | - assignInstr = explicitDef.getInstruction() and - getAUse(prevDef) = compoundAssign.getLhs() and - assignInstr = IR::assignInstruction(compoundAssign, 0) and - prevBound = getAnSsaUpperBound(prevDef) and - if compoundAssign instanceof AddAssignStmt - then - delta = getAnUpperBound(compoundAssign.getRhs()) and - result = addRoundingUp(prevBound, delta) - else - if compoundAssign instanceof SubAssignStmt - then - delta = getALowerBound(compoundAssign.getRhs()) and - result = addRoundingUp(prevBound, -delta) - else none() - ) - else - //SSA definition coresponding to an `IncDecStmt` - if explicitDef.getInstruction() instanceof IR::IncDecInstruction + not recursiveSelfDef(def) and + ( + def instanceof SsaExplicitDefinition and + exists(SsaExplicitDefinition explicitDef | explicitDef = def | + //SSA definition coresponding to a `SimpleAssignStmt` + if explicitDef.getInstruction() instanceof IR::AssignInstruction + then + exists(IR::AssignInstruction assignInstr, SimpleAssignStmt simpleAssign | + assignInstr = explicitDef.getInstruction() and + assignInstr.getRhs().(IR::EvalInstruction).getExpr() = simpleAssign.getRhs() and + result = getAnUpperBound(simpleAssign.getRhs()) + ) + or + //SSA definition coresponding to a ValueSpec(used in a variable declaration) + exists(IR::AssignInstruction declInstr, ValueSpec vs, int i, Expr init | + declInstr = explicitDef.getInstruction() and + declInstr = IR::initInstruction(vs, i) and + init = vs.getInit(i) and + result = getAnUpperBound(init) + ) + or + //SSA definition coresponding to an `AddAssignStmt` (x += y) or `SubAssignStmt` (x -= y) + exists( + IR::AssignInstruction assignInstr, SsaExplicitDefinition prevDef, + CompoundAssignStmt compoundAssign, float prevBound, float delta + | + assignInstr = explicitDef.getInstruction() and + getAUse(prevDef) = compoundAssign.getLhs() and + assignInstr = IR::assignInstruction(compoundAssign, 0) and + prevBound = getAnSsaUpperBound(prevDef) and + if compoundAssign instanceof AddAssignStmt then - exists(IncDecStmt incOrDec, IR::IncDecInstruction instr, float exprLB | - instr = explicitDef.getInstruction() and - exprLB = getAnUpperBound(incOrDec.getOperand()) and - instr.getRhs().(IR::EvalIncDecRhsInstruction).getStmt() = incOrDec and - ( - //IncStmt(x++) - exists(IncStmt inc | - inc = incOrDec and - result = addRoundingUp(exprLB, 1) - ) - or - //DecStmt(x--) - exists(DecStmt dec | - dec = incOrDec and - result = addRoundingUp(exprLB, -1) - ) - ) - ) + delta = getAnUpperBound(compoundAssign.getRhs()) and + result = addRoundingUp(prevBound, delta) else - //SSA definition coreponding to the init of the parameter - if explicitDef.getInstruction() instanceof IR::InitParameterInstruction + if compoundAssign instanceof SubAssignStmt then - exists(IR::InitParameterInstruction instr, Parameter p | - instr = explicitDef.getInstruction() and - IR::initParamInstruction(p) = instr and - result = typeMaxValue(p.getType()) - ) + delta = getALowerBound(compoundAssign.getRhs()) and + result = addRoundingUp(prevBound, -delta) else none() - ) - else - //this SSA definition is a phi node. - if def instanceof SsaPhiNode - then - exists(SsaPhiNode phi | - phi = def and - result = getAnSsaUpperBound(phi.getAnInput().getDefinition()) ) - else none() + else + //SSA definition coresponding to an `IncDecStmt` + if explicitDef.getInstruction() instanceof IR::IncDecInstruction + then + exists(IncDecStmt incOrDec, IR::IncDecInstruction instr, float exprLB | + instr = explicitDef.getInstruction() and + exprLB = getAnUpperBound(incOrDec.getOperand()) and + instr.getRhs().(IR::EvalIncDecRhsInstruction).getStmt() = incOrDec and + ( + //IncStmt(x++) + exists(IncStmt inc | + inc = incOrDec and + result = addRoundingUp(exprLB, 1) + ) + or + //DecStmt(x--) + exists(DecStmt dec | + dec = incOrDec and + result = addRoundingUp(exprLB, -1) + ) + ) + ) + else ( + //SSA definition coreponding to the init of the parameter + explicitDef.getInstruction() instanceof IR::InitParameterInstruction and + exists(IR::InitParameterInstruction instr, Parameter p | + instr = explicitDef.getInstruction() and + IR::initParamInstruction(p) = instr and + result = typeMaxValue(p.getType()) + ) + ) + ) + or + //this SSA definition is a phi node. + def instanceof SsaPhiNode and + exists(SsaPhiNode phi | + phi = def and + result = getAnSsaUpperBound(phi.getAnInput().getDefinition()) + ) ) } @@ -439,91 +434,85 @@ float getAnSsaUpperBound(SsaDefinition def) { * Gets a possible lower bound of SSA definition `def`. */ float getAnSsaLowerBound(SsaDefinition def) { - if recursiveSelfDef(def) - then none() - else ( - if def instanceof SsaExplicitDefinition - then - exists(SsaExplicitDefinition explicitDef | explicitDef = def | - if explicitDef.getInstruction() instanceof IR::AssignInstruction - then - //SimpleAssignStmt - exists(IR::AssignInstruction assignInstr, SimpleAssignStmt simpleAssign | - assignInstr = explicitDef.getInstruction() and - assignInstr.getRhs().(IR::EvalInstruction).getExpr() = simpleAssign.getRhs() and - result = getALowerBound(simpleAssign.getRhs()) - ) - or - //ValueSpec - exists(IR::AssignInstruction declInstr, ValueSpec vs, int i, Expr init | - declInstr = explicitDef.getInstruction() and - declInstr = IR::initInstruction(vs, i) and - init = vs.getInit(i) and - result = getALowerBound(init) - ) - or - //AddAssignStmt(x += y) - exists( - IR::AssignInstruction assignInstr, SsaExplicitDefinition prevDef, - CompoundAssignStmt compoundAssign, float prevBound, float delta - | - assignInstr = explicitDef.getInstruction() and - getAUse(prevDef) = compoundAssign.getLhs() and - assignInstr = IR::assignInstruction(compoundAssign, 0) and - prevBound = getAnSsaLowerBound(prevDef) and - if compoundAssign instanceof AddAssignStmt - then - delta = getALowerBound(compoundAssign.getRhs()) and - result = addRoundingDown(prevBound, delta) - else - if compoundAssign instanceof SubAssignStmt - then - delta = getAnUpperBound(compoundAssign.getRhs()) and - result = addRoundingDown(prevBound, -delta) - else none() + not recursiveSelfDef(def) and + ( + def instanceof SsaExplicitDefinition and + exists(SsaExplicitDefinition explicitDef | explicitDef = def | + if explicitDef.getInstruction() instanceof IR::AssignInstruction + then + //SimpleAssignStmt + exists(IR::AssignInstruction assignInstr, SimpleAssignStmt simpleAssign | + assignInstr = explicitDef.getInstruction() and + assignInstr.getRhs().(IR::EvalInstruction).getExpr() = simpleAssign.getRhs() and + result = getALowerBound(simpleAssign.getRhs()) + ) + or + //ValueSpec + exists(IR::AssignInstruction declInstr, ValueSpec vs, int i, Expr init | + declInstr = explicitDef.getInstruction() and + declInstr = IR::initInstruction(vs, i) and + init = vs.getInit(i) and + result = getALowerBound(init) + ) + or + //AddAssignStmt(x += y) + exists( + IR::AssignInstruction assignInstr, SsaExplicitDefinition prevDef, + CompoundAssignStmt compoundAssign, float prevBound, float delta + | + assignInstr = explicitDef.getInstruction() and + getAUse(prevDef) = compoundAssign.getLhs() and + assignInstr = IR::assignInstruction(compoundAssign, 0) and + prevBound = getAnSsaLowerBound(prevDef) and + ( + compoundAssign instanceof AddAssignStmt and + delta = getALowerBound(compoundAssign.getRhs()) and + result = addRoundingDown(prevBound, delta) + or + compoundAssign instanceof SubAssignStmt and + delta = getAnUpperBound(compoundAssign.getRhs()) and + result = addRoundingDown(prevBound, -delta) ) - else - //IncDecStmt - if explicitDef.getInstruction() instanceof IR::IncDecInstruction - then - exists(IncDecStmt incOrDec, IR::IncDecInstruction instr, float exprLB | - instr = explicitDef.getInstruction() and - exprLB = getALowerBound(incOrDec.getOperand()) and - instr.getRhs().(IR::EvalIncDecRhsInstruction).getStmt() = incOrDec and - ( - //IncStmt(x++) - exists(IncStmt inc | - inc = incOrDec and - result = addRoundingDown(exprLB, 1) - ) - or - //DecStmt(x--) - exists(DecStmt dec | - dec = incOrDec and - result = addRoundingDown(exprLB, -1) - ) + ) + else + //IncDecStmt + if explicitDef.getInstruction() instanceof IR::IncDecInstruction + then + exists(IncDecStmt incOrDec, IR::IncDecInstruction instr, float exprLB | + instr = explicitDef.getInstruction() and + exprLB = getALowerBound(incOrDec.getOperand()) and + instr.getRhs().(IR::EvalIncDecRhsInstruction).getStmt() = incOrDec and + ( + //IncStmt(x++) + exists(IncStmt inc | + inc = incOrDec and + result = addRoundingDown(exprLB, 1) ) - ) - else - //init of the function parameter - if explicitDef.getInstruction() instanceof IR::InitParameterInstruction - then - exists(IR::InitParameterInstruction instr, Parameter p | - instr = explicitDef.getInstruction() and - IR::initParamInstruction(p) = instr and - result = typeMinValue(p.getType()) + or + //DecStmt(x--) + exists(DecStmt dec | + dec = incOrDec and + result = addRoundingDown(exprLB, -1) ) - else none() - ) - else - //phi node - if def instanceof SsaPhiNode - then - exists(SsaPhiNode phi | - phi = def and - result = getAnSsaLowerBound(phi.getAnInput().getDefinition()) + ) + ) + else ( + //init of the function parameter + explicitDef.getInstruction() instanceof IR::InitParameterInstruction and + exists(IR::InitParameterInstruction instr, Parameter p | + instr = explicitDef.getInstruction() and + IR::initParamInstruction(p) = instr and + result = typeMinValue(p.getType()) + ) ) - else none() + ) + or + //phi node + def instanceof SsaPhiNode and + exists(SsaPhiNode phi | + phi = def and + result = getAnSsaLowerBound(phi.getAnInput().getDefinition()) + ) ) } From d0323a6425c380cac284aca0c7207ced4b424a2f Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Tue, 2 Sep 2025 09:42:05 +0200 Subject: [PATCH 2/3] Fix one more violation. --- .../IntegerOverflow/RangeAnalysis.qll | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll b/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll index 828fc392a2df..c53d04c737a0 100644 --- a/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll +++ b/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll @@ -377,16 +377,15 @@ float getAnSsaUpperBound(SsaDefinition def) { getAUse(prevDef) = compoundAssign.getLhs() and assignInstr = IR::assignInstruction(compoundAssign, 0) and prevBound = getAnSsaUpperBound(prevDef) and - if compoundAssign instanceof AddAssignStmt - then + ( + compoundAssign instanceof AddAssignStmt and delta = getAnUpperBound(compoundAssign.getRhs()) and result = addRoundingUp(prevBound, delta) - else - if compoundAssign instanceof SubAssignStmt - then - delta = getALowerBound(compoundAssign.getRhs()) and - result = addRoundingUp(prevBound, -delta) - else none() + or + compoundAssign instanceof SubAssignStmt and + delta = getALowerBound(compoundAssign.getRhs()) and + result = addRoundingUp(prevBound, -delta) + ) ) else //SSA definition coresponding to an `IncDecStmt` From 55e5281429fac51505945a966f06a1946ca408e8 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Tue, 2 Sep 2025 10:47:36 +0200 Subject: [PATCH 3/3] Go: Fix a couple more spelling errors. --- .../IntegerOverflow/RangeAnalysis.qll | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll b/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll index c53d04c737a0..f3f4c15f0085 100644 --- a/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll +++ b/go/ql/src/experimental/IntegerOverflow/RangeAnalysis.qll @@ -74,7 +74,7 @@ float getAnUpperBound(Expr expr) { result = getAnUpperBound(greater.asExpr()) + bias ) else - //If not, find the coresponding `SsaDefinition`, then call `getAnSsaUpperBound` on it. + //If not, find the corresponding `SsaDefinition`, then call `getAnSsaUpperBound` on it. result = getAnSsaUpperBound(v.getDefinition()) ) ) @@ -231,7 +231,7 @@ float getALowerBound(Expr expr) { result = lbs - bias ) else - //find coresponding SSA definition and calls `getAnSsaLowerBound` on it. + //find corresponding SSA definition and calls `getAnSsaLowerBound` on it. result = getAnSsaLowerBound(v.getDefinition()) ) ) @@ -351,7 +351,7 @@ float getAnSsaUpperBound(SsaDefinition def) { ( def instanceof SsaExplicitDefinition and exists(SsaExplicitDefinition explicitDef | explicitDef = def | - //SSA definition coresponding to a `SimpleAssignStmt` + //SSA definition corresponding to a `SimpleAssignStmt` if explicitDef.getInstruction() instanceof IR::AssignInstruction then exists(IR::AssignInstruction assignInstr, SimpleAssignStmt simpleAssign | @@ -360,7 +360,7 @@ float getAnSsaUpperBound(SsaDefinition def) { result = getAnUpperBound(simpleAssign.getRhs()) ) or - //SSA definition coresponding to a ValueSpec(used in a variable declaration) + //SSA definition corresponding to a ValueSpec(used in a variable declaration) exists(IR::AssignInstruction declInstr, ValueSpec vs, int i, Expr init | declInstr = explicitDef.getInstruction() and declInstr = IR::initInstruction(vs, i) and @@ -368,7 +368,7 @@ float getAnSsaUpperBound(SsaDefinition def) { result = getAnUpperBound(init) ) or - //SSA definition coresponding to an `AddAssignStmt` (x += y) or `SubAssignStmt` (x -= y) + //SSA definition corresponding to an `AddAssignStmt` (x += y) or `SubAssignStmt` (x -= y) exists( IR::AssignInstruction assignInstr, SsaExplicitDefinition prevDef, CompoundAssignStmt compoundAssign, float prevBound, float delta @@ -388,7 +388,7 @@ float getAnSsaUpperBound(SsaDefinition def) { ) ) else - //SSA definition coresponding to an `IncDecStmt` + //SSA definition corresponding to an `IncDecStmt` if explicitDef.getInstruction() instanceof IR::IncDecInstruction then exists(IncDecStmt incOrDec, IR::IncDecInstruction instr, float exprLB | @@ -521,13 +521,13 @@ float getAnSsaLowerBound(SsaDefinition def) { * The structure of this function needs to be same as `getAnSsaLowerBound` */ predicate ssaDependsOnSsa(SsaDefinition nextDef, SsaDefinition prevDef) { - //SSA definition coresponding to a `SimpleAssignStmt` + //SSA definition corresponding to a `SimpleAssignStmt` exists(SimpleAssignStmt simpleAssign | nextDef.(SsaExplicitDefinition).getInstruction() = IR::assignInstruction(simpleAssign, _) and ssaDependsOnExpr(prevDef, simpleAssign.getRhs()) ) or - //SSA definition coresponding to a `ValueSpec`(used in variable declaration) + //SSA definition corresponding to a `ValueSpec`(used in variable declaration) exists(IR::AssignInstruction declInstr, ValueSpec vs, int i, Expr init | declInstr = nextDef.(SsaExplicitDefinition).getInstruction() and declInstr = IR::initInstruction(vs, i) and @@ -535,7 +535,7 @@ predicate ssaDependsOnSsa(SsaDefinition nextDef, SsaDefinition prevDef) { ssaDependsOnExpr(prevDef, init) ) or - //SSA definition coresponding to a `AddAssignStmt` or `SubAssignStmt` + //SSA definition corresponding to a `AddAssignStmt` or `SubAssignStmt` exists(CompoundAssignStmt compoundAssign | (compoundAssign instanceof AddAssignStmt or compoundAssign instanceof SubAssignStmt) and nextDef.(SsaExplicitDefinition).getInstruction() = IR::assignInstruction(compoundAssign, 0) and @@ -545,7 +545,7 @@ predicate ssaDependsOnSsa(SsaDefinition nextDef, SsaDefinition prevDef) { ) ) or - //SSA definition coresponding to a `IncDecStmt` + //SSA definition corresponding to a `IncDecStmt` exists(IncDecStmt incDec | nextDef .(SsaExplicitDefinition) @@ -557,7 +557,7 @@ predicate ssaDependsOnSsa(SsaDefinition nextDef, SsaDefinition prevDef) { ssaDependsOnExpr(prevDef, incDec.getOperand()) ) or - //if `nextDef` coresponding to the init of a parameter, there is no coresponding `prevDef` + //if `nextDef` corresponding to the init of a parameter, there is no corresponding `prevDef` //if `nextDef` is a phi node and `prevDef` is one of the input of the phi node, then `nextDef` depends on `prevDef` directly. exists(SsaPhiNode phi | nextDef = phi and phi.getAnInput().getDefinition() = prevDef) }