From b3f92fcf0f37525d93f0f2a77c02a754d23f72fc Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 2 Mar 2023 22:37:52 +0000 Subject: [PATCH 1/3] C++: Add FN caused by missing static local initialization in SSA. --- .../library-tests/dataflow/dataflow-tests/test.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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..edaaa3ade777 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 MISSING: ir +} \ No newline at end of file From 2963dc1cb13b5da51f577fd7577383440fcec223 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 2 Mar 2023 22:38:43 +0000 Subject: [PATCH 2/3] C++: Include phi read nodes in SSA. There's a small fix to the mapping from 'global def -> use'. Finally, this commit also accepts a test failure related to new missing types for phi nodes. The fix for that is in the next commit. --- .../cpp/ir/dataflow/internal/SsaInternals.qll | 58 +++++++++++-------- .../dataflow/internal/ssa0/SsaInternals.qll | 13 +++-- .../dataflow/dataflow-tests/test.cpp | 2 +- .../dataflow-tests/type-bugs.expected | 5 ++ 4 files changed, 50 insertions(+), 28 deletions(-) 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..5db883362b44 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 @@ -530,15 +530,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 +550,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 +673,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 +709,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 +718,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 +803,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 +813,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 +825,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 +974,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 edaaa3ade777..351fc386cdf2 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -626,5 +626,5 @@ void test_def_via_phi_read(bool b) use(buffer); } intPointerSource(buffer); - sink(buffer); // $ ast MISSING: ir + sink(buffer); // $ ast,ir } \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/type-bugs.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/type-bugs.expected index a77f3044647a..dae4b7895eb4 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/type-bugs.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/type-bugs.expected @@ -1,3 +1,8 @@ failures astTypeBugs irTypeBugs +| test.cpp:15:3:15:6 | test.cpp:15:3:15:6 | test.cpp:15:3:15:6 | Phi | +| test.cpp:43:10:43:20 | test.cpp:43:10:43:20 | test.cpp:43:10:43:20 | Phi | +| test.cpp:43:10:43:20 | test.cpp:43:10:43:20 | test.cpp:43:10:43:20 | Phi | +| test.cpp:628:3:628:18 | test.cpp:628:3:628:18 | test.cpp:628:3:628:18 | Phi | +| test.cpp:628:3:628:18 | test.cpp:628:3:628:18 | test.cpp:628:3:628:18 | Phi | From 959237e8d21749c8079f0e43c7a077aa859eb6a4 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 2 Mar 2023 22:45:32 +0000 Subject: [PATCH 3/3] C++: Fix missing type for Phi nodes. --- .../ir/dataflow/internal/DataFlowPrivate.qll | 2 +- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 9 ++++++++- .../cpp/ir/dataflow/internal/SsaInternals.qll | 18 ++++++++++++++++++ .../dataflow/dataflow-tests/type-bugs.expected | 5 ----- 4 files changed, 27 insertions(+), 7 deletions(-) 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 5db883362b44..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) } } } diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/type-bugs.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/type-bugs.expected index dae4b7895eb4..a77f3044647a 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/type-bugs.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/type-bugs.expected @@ -1,8 +1,3 @@ failures astTypeBugs irTypeBugs -| test.cpp:15:3:15:6 | test.cpp:15:3:15:6 | test.cpp:15:3:15:6 | Phi | -| test.cpp:43:10:43:20 | test.cpp:43:10:43:20 | test.cpp:43:10:43:20 | Phi | -| test.cpp:43:10:43:20 | test.cpp:43:10:43:20 | test.cpp:43:10:43:20 | Phi | -| test.cpp:628:3:628:18 | test.cpp:628:3:628:18 | test.cpp:628:3:628:18 | Phi | -| test.cpp:628:3:628:18 | test.cpp:628:3:628:18 | test.cpp:628:3:628:18 | Phi |