diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index e882ae3628b7..c6c181d2c304 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -43,7 +43,7 @@ class Node0Impl extends TIRDataFlowNode0 { /** * Gets the type of this node. * - * If `asInstruction().isGLValue()` holds, then the type of this node + * If `isGLValue()` holds, then the type of this node * should be thought of as "pointer to `getType()`". */ DataFlowType getType() { none() } // overridden in subclasses diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index 4a6dfde9e3b0..0f89d9a10019 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -498,7 +498,14 @@ class SsaPhiNode extends Node, TSsaPhiNode { override Declaration getFunction() { result = phi.getBasicBlock().getEnclosingFunction() } - override DataFlowType getType() { result = this.getAnInput().getType().getUnspecifiedType() } + override DataFlowType getType() { + exists(Ssa::SourceVariable sv | + this.getPhiNode().definesAt(sv, _, _, _) and + result = sv.getType() + ) + } + + override predicate isGLValue() { phi.getSourceVariable().isGLValue() } final override Location getLocationImpl() { result = phi.getBasicBlock().getLocation() } diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll index 243b18e9ac19..ee958431b691 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll @@ -48,6 +48,16 @@ private module SourceVariables { * indirections) of this source variable. */ abstract BaseSourceVariable getBaseVariable(); + + /** Holds if this variable is a glvalue. */ + predicate isGLValue() { none() } + + /** + * Gets the type of this source variable. If `isGLValue()` holds, then + * the type of this source variable should be thought of as "pointer + * to `getType()`". + */ + abstract DataFlowType getType(); } class SourceIRVariable extends SourceVariable, TSourceIRVariable { @@ -66,6 +76,12 @@ private module SourceVariables { ind > 0 and result = this.getIRVariable().toString() + " indirection" } + + override predicate isGLValue() { ind = 0 } + + override DataFlowType getType() { + if ind = 0 then result = var.getType() else result = getTypeImpl(var.getType(), ind - 1) + } } class CallVariable extends SourceVariable, TCallVariable { @@ -84,6 +100,8 @@ private module SourceVariables { ind > 0 and result = "Call indirection" } + + override DataFlowType getType() { result = getTypeImpl(call.getResultType(), ind) } } } @@ -530,15 +548,15 @@ predicate adjacentDefRead(DefOrUse defOrUse1, UseOrPhi use) { exists(IRBlock bb1, int i1, SourceVariable v | defOrUse1.asDefOrUse().hasIndexInBlock(bb1, i1, v) | - exists(IRBlock bb2, int i2, Definition def | - adjacentDefRead(pragma[only_bind_into](def), pragma[only_bind_into](bb1), + exists(IRBlock bb2, int i2, DefinitionExt def | + adjacentDefReadExt(pragma[only_bind_into](def), pragma[only_bind_into](bb1), pragma[only_bind_into](i1), pragma[only_bind_into](bb2), pragma[only_bind_into](i2)) and def.getSourceVariable() = v and use.asDefOrUse().(UseImpl).hasIndexInBlock(bb2, i2, v) ) or exists(PhiNode phi | - lastRefRedef(_, bb1, i1, phi) and + lastRefRedefExt(_, bb1, i1, phi) and use.asPhi() = phi and phi.getSourceVariable() = pragma[only_bind_into](v) ) @@ -550,11 +568,18 @@ predicate adjacentDefRead(DefOrUse defOrUse1, UseOrPhi use) { * flows to `useOrPhi`. */ private predicate globalDefToUse(GlobalDef globalDef, UseOrPhi useOrPhi) { - exists(IRBlock bb1, int i1, IRBlock bb2, int i2, SourceVariable v | - globalDef.hasIndexInBlock(bb1, i1, v) and - adjacentDefRead(_, pragma[only_bind_into](bb1), pragma[only_bind_into](i1), - pragma[only_bind_into](bb2), pragma[only_bind_into](i2)) and - useOrPhi.asDefOrUse().hasIndexInBlock(bb2, i2, v) + exists(IRBlock bb1, int i1, SourceVariable v | globalDef.hasIndexInBlock(bb1, i1, v) | + exists(IRBlock bb2, int i2 | + adjacentDefReadExt(_, pragma[only_bind_into](bb1), pragma[only_bind_into](i1), + pragma[only_bind_into](bb2), pragma[only_bind_into](i2)) and + useOrPhi.asDefOrUse().hasIndexInBlock(bb2, i2, v) + ) + or + exists(PhiNode phi | + lastRefRedefExt(_, bb1, i1, phi) and + useOrPhi.asPhi() = phi and + phi.getSourceVariable() = pragma[only_bind_into](v) + ) ) } @@ -666,17 +691,17 @@ private predicate ssaFlowImpl(SsaDefOrUse defOrUse, Node nodeFrom, Node nodeTo, * Holds if `def` is the corresponding definition of * the SSA library's `definition`. */ -private Definition ssaDefinition(Def def) { +private DefinitionExt ssaDefinition(Def def) { exists(IRBlock block, int i, SourceVariable sv | def.hasIndexInBlock(block, i, sv) and - result.definesAt(sv, block, i) + result.definesAt(sv, block, i, _) ) } /** Gets a node that represents the prior definition of `node`. */ private Node getAPriorDefinition(SsaDefOrUse defOrUse) { - exists(IRBlock bb, int i, SourceVariable sv, Definition def, DefOrUse defOrUse0 | - SsaCached::lastRefRedef(pragma[only_bind_into](def), pragma[only_bind_into](bb), + exists(IRBlock bb, int i, SourceVariable sv, DefinitionExt def, DefOrUse defOrUse0 | + lastRefRedefExt(pragma[only_bind_into](def), pragma[only_bind_into](bb), pragma[only_bind_into](i), ssaDefinition(defOrUse)) and def.getSourceVariable() = sv and defOrUse0.hasIndexInBlock(bb, i, sv) and @@ -702,7 +727,7 @@ pragma[nomagic] private predicate fromPhiNodeToUse(PhiNode phi, SourceVariable sv, IRBlock bb1, int i1, UseOrPhi use) { exists(IRBlock bb2, int i2 | use.asDefOrUse().hasIndexInBlock(bb2, i2, sv) and - adjacentDefRead(pragma[only_bind_into](phi), pragma[only_bind_into](bb1), + adjacentDefReadExt(pragma[only_bind_into](phi), pragma[only_bind_into](bb1), pragma[only_bind_into](i1), pragma[only_bind_into](bb2), pragma[only_bind_into](i2)) ) } @@ -711,13 +736,13 @@ private predicate fromPhiNodeToUse(PhiNode phi, SourceVariable sv, IRBlock bb1, predicate fromPhiNode(SsaPhiNode nodeFrom, Node nodeTo) { exists(PhiNode phi, SourceVariable sv, IRBlock bb1, int i1, UseOrPhi use | phi = nodeFrom.getPhiNode() and - phi.definesAt(sv, bb1, i1) and + phi.definesAt(sv, bb1, i1, _) and useToNode(use, nodeTo) | fromPhiNodeToUse(phi, sv, bb1, i1, use) or exists(PhiNode phiTo | - lastRefRedef(phi, _, _, phiTo) and + lastRefRedefExt(phi, _, _, phiTo) and nodeTo.(SsaPhiNode).getPhiNode() = phiTo ) ) @@ -796,8 +821,8 @@ module SsaCached { * path between them without any read of `def`. */ cached - predicate adjacentDefRead(Definition def, IRBlock bb1, int i1, IRBlock bb2, int i2) { - SsaImpl::adjacentDefRead(def, bb1, i1, bb2, i2) + predicate adjacentDefReadExt(DefinitionExt def, IRBlock bb1, int i1, IRBlock bb2, int i2) { + SsaImpl::adjacentDefReadExt(def, _, bb1, i1, bb2, i2) } /** @@ -806,8 +831,8 @@ module SsaCached { * without passing through another read or write. */ cached - predicate lastRefRedef(Definition def, IRBlock bb, int i, Definition next) { - SsaImpl::lastRefRedef(def, bb, i, next) + predicate lastRefRedefExt(DefinitionExt def, IRBlock bb, int i, DefinitionExt next) { + SsaImpl::lastRefRedefExt(def, _, bb, i, next) } } @@ -818,8 +843,8 @@ private newtype TSsaDefOrUse = or // Like in the pruning stage, we only include definition that's live after the // write as the final definitions computed by SSA. - exists(Definition def, SourceVariable sv, IRBlock bb, int i | - def.definesAt(sv, bb, i) and + exists(DefinitionExt def, SourceVariable sv, IRBlock bb, int i | + def.definesAt(sv, bb, i, _) and defOrUse.(DefImpl).hasIndexInBlock(bb, i, sv) ) } or @@ -967,9 +992,14 @@ class Def extends DefOrUse { private module SsaImpl = SsaImplCommon::Make; -class PhiNode = SsaImpl::PhiNode; +class PhiNode extends SsaImpl::DefinitionExt { + PhiNode() { + this instanceof SsaImpl::PhiNode or + this instanceof SsaImpl::PhiReadNode + } +} -class Definition = SsaImpl::Definition; +class DefinitionExt = SsaImpl::DefinitionExt; class UncertainWriteDefinition = SsaImpl::UncertainWriteDefinition; diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ssa0/SsaInternals.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ssa0/SsaInternals.qll index 7997bf7dee42..aa6a43a2580d 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ssa0/SsaInternals.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ssa0/SsaInternals.qll @@ -225,8 +225,8 @@ private newtype TSsaDefOrUse = or // If `defOrUse` is a definition we only include it if the // SSA library concludes that it's live after the write. - exists(Definition def, SourceVariable sv, IRBlock bb, int i | - def.definesAt(sv, bb, i) and + exists(DefinitionExt def, SourceVariable sv, IRBlock bb, int i | + def.definesAt(sv, bb, i, _) and defOrUse.(DefImpl).hasIndexInBlock(bb, i, sv) ) } or @@ -301,6 +301,11 @@ class Def extends DefOrUse { private module SsaImpl = SsaImplCommon::Make; -class PhiNode = SsaImpl::PhiNode; +class PhiNode extends SsaImpl::DefinitionExt { + PhiNode() { + this instanceof SsaImpl::PhiNode or + this instanceof SsaImpl::PhiReadNode + } +} -class Definition = SsaImpl::Definition; +class DefinitionExt = SsaImpl::DefinitionExt; diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp index ccfc3cd69a20..351fc386cdf2 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -615,3 +615,16 @@ void test_flow_through_void_double_pointer(int *p) // $ ast-def=p void* q = (void*)&p; sink(**(int**)q); // $ ir MISSING: ast } + +void use(int *); + +void test_def_via_phi_read(bool b) +{ + static int buffer[10]; // This is missing an initialisation in IR dataflow + if (b) + { + use(buffer); + } + intPointerSource(buffer); + sink(buffer); // $ ast,ir +} \ No newline at end of file