From 0963af2ee7827eedcf4f94f9a7196eb144f582c0 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Nov 2023 17:57:22 +0000 Subject: [PATCH 1/8] C++: Add failing tests. --- .../dataflow-consistency.expected | 4 ++ .../dataflow/dataflow-tests/test.cpp | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected index 01fc86f1a383..da59987d7421 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected @@ -23,6 +23,7 @@ argHasPostUpdate | lambdas.cpp:38:2:38:2 | d | ArgumentNode is missing PostUpdateNode. | | lambdas.cpp:45:2:45:2 | e | ArgumentNode is missing PostUpdateNode. | | test.cpp:67:29:67:35 | source1 | ArgumentNode is missing PostUpdateNode. | +| test.cpp:813:19:813:35 | * ... | ArgumentNode is missing PostUpdateNode. | postWithInFlow | BarrierGuard.cpp:49:6:49:6 | x [post update] | PostUpdateNode should not be the target of local flow. | | BarrierGuard.cpp:60:7:60:7 | x [post update] | PostUpdateNode should not be the target of local flow. | @@ -136,6 +137,9 @@ postWithInFlow | test.cpp:728:3:728:4 | * ... [post update] | PostUpdateNode should not be the target of local flow. | | test.cpp:728:4:728:4 | p [inner post update] | PostUpdateNode should not be the target of local flow. | | test.cpp:734:41:734:41 | x [inner post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:808:5:808:21 | * ... [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:808:6:808:21 | global_indirect1 [inner post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:832:5:832:17 | global_direct [post update] | PostUpdateNode should not be the target of local flow. | viableImplInCallContextTooLarge uniqueParameterNodeAtPosition uniqueParameterNodePosition 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 c5f7ffcf1603..fed95cdce9c1 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -796,4 +796,44 @@ void test() { MyStruct a; intPointerSource(a.content, a.content); indirect_sink(a.content); // $ ast ir +} + +namespace MoreGlobalTests { + int **global_indirect1; + int **global_indirect2; + int **global_direct; + + void set_indirect1() + { + *global_indirect1 = indirect_source(); + } + + void read_indirect1() { + sink(global_indirect1); // clean + indirect_sink(*global_indirect1); // $ MISSING: ir,ast + } + + void set_indirect2() + { + **global_indirect2 = source(); + } + + void read_indirect2() { + sink(global_indirect2); // clean + sink(**global_indirect2); // $ MISSING: ir,ast + } + + // overload source with a boolean parameter so + // that we can define a variant that return an int**. + int** source(bool); + + void set_direct() + { + global_direct = source(true); + } + + void read_direct() { + sink(global_direct); // $ ir MISSING: ast + indirect_sink(global_direct); // clean + } } \ No newline at end of file From bb5a78d3f1ac08c44bf77b8d292a86febbdd52cb Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Nov 2023 20:16:27 +0000 Subject: [PATCH 2/8] C++: Factor the IPA body of 'TGlobalUse' and 'TGlobalDef' out into predicates. --- .../cpp/ir/dataflow/internal/SsaInternals.qll | 38 ++++++++++++------- 1 file changed, 25 insertions(+), 13 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 b6afadfe0e1b..fa5566641824 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 @@ -113,22 +113,12 @@ private newtype TDefOrUseImpl = TGlobalUse(GlobalLikeVariable v, IRFunction f, int indirectionIndex) { // Represents a final "use" of a global variable to ensure that // the assignment to a global variable isn't ruled out as dead. - exists(VariableAddressInstruction vai, int defIndex | - vai.getEnclosingIRFunction() = f and - vai.getAstVariable() = v and - isDef(_, _, _, vai, _, defIndex) and - indirectionIndex = [0 .. defIndex] + 1 - ) + isGlobalUse(v, f, _, indirectionIndex) } or TGlobalDefImpl(GlobalLikeVariable v, IRFunction f, int indirectionIndex) { // Represents the initial "definition" of a global variable when entering // a function body. - exists(VariableAddressInstruction vai | - vai.getEnclosingIRFunction() = f and - vai.getAstVariable() = v and - isUse(_, _, vai, _, indirectionIndex) and - not isDef(_, _, vai.getAUse(), _, _, _) - ) + isGlobalDefImpl(v, f, _, indirectionIndex) } or TIteratorDef( Operand iteratorDerefAddress, BaseSourceVariableInstruction container, int indirectionIndex @@ -150,6 +140,28 @@ private newtype TDefOrUseImpl = ) } +private predicate isGlobalUse( + GlobalLikeVariable v, IRFunction f, int indirection, int indirectionIndex +) { + exists(VariableAddressInstruction vai, int defIndex | + vai.getEnclosingIRFunction() = f and + vai.getAstVariable() = v and + isDef(_, _, _, vai, indirection, defIndex) and + indirectionIndex = [0 .. defIndex] + 1 + ) +} + +private predicate isGlobalDefImpl( + GlobalLikeVariable v, IRFunction f, int indirection, int indirectionIndex +) { + exists(VariableAddressInstruction vai | + vai.getEnclosingIRFunction() = f and + vai.getAstVariable() = v and + isUse(_, _, vai, indirection, indirectionIndex) and + not isDef(_, _, vai.getAUse(), _, _, _) + ) +} + private predicate unspecifiedTypeIsModifiableAt(Type unspecified, int indirectionIndex) { indirectionIndex = [1 .. getIndirectionForUnspecifiedType(unspecified).getNumberOfIndirections()] and exists(CppType cppType | @@ -438,7 +450,7 @@ class GlobalUse extends UseImpl, TGlobalUse { override FinalGlobalValue getNode() { result.getGlobalUse() = this } - override int getIndirection() { result = ind + 1 } + override int getIndirection() { isGlobalUse(global, f, result, ind) } /** Gets the global variable associated with this use. */ GlobalLikeVariable getVariable() { result = global } From fd26ae18bf52e2b384add3d98a88532e5fab0f34 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Nov 2023 20:20:27 +0000 Subject: [PATCH 3/8] C++: Obtain the SSA variable of a 'GlobalUse' using the indirection instead of the index (like we do for non-global uses as well). --- .../semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll | 8 ++++++-- 1 file changed, 6 insertions(+), 2 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 fa5566641824..f0a8715dcef7 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 @@ -472,7 +472,9 @@ class GlobalUse extends UseImpl, TGlobalUse { ) } - override SourceVariable getSourceVariable() { sourceVariableIsGlobal(result, global, f, ind) } + override SourceVariable getSourceVariable() { + sourceVariableIsGlobal(result, global, f, this.getIndirection()) + } final override Cpp::Location getLocation() { result = f.getLocation() } @@ -513,9 +515,11 @@ class GlobalDefImpl extends DefOrUseImpl, TGlobalDefImpl { /** Gets the global variable associated with this definition. */ override SourceVariable getSourceVariable() { - sourceVariableIsGlobal(result, global, f, indirectionIndex) + sourceVariableIsGlobal(result, global, f, this.getIndirection()) } + int getIndirection() { result = indirectionIndex } + /** * Gets the type of this use after specifiers have been deeply stripped * and typedefs have been resolved. From 95bb70f577246591ceec157d63d3fb42d80a5444 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Nov 2023 20:25:29 +0000 Subject: [PATCH 4/8] C++: Also add a 'getIndirection' on 'GlobalDef' as well. This will be useful in the next commit. --- .../lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll | 3 +++ 1 file changed, 3 insertions(+) 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 f0a8715dcef7..0353c3e1ba64 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 @@ -1006,6 +1006,9 @@ class GlobalDef extends TGlobalDef, SsaDefOrUse { global.hasIndexInBlock(block, index, sv) } + /** Gets the indirection index of this definition. */ + int getIndirection() { result = global.getIndirection() } + /** Gets the indirection index of this definition. */ int getIndirectionIndex() { result = global.getIndirectionIndex() } From 976231350034f788a41314caa7b44da3bbbac0c7 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Nov 2023 20:22:57 +0000 Subject: [PATCH 5/8] C++: Implement jumpStep using the indirection instead of index. --- .../code/cpp/ir/dataflow/internal/DataFlowPrivate.qll | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 fa54c9c736a3..e3f30b07dad0 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 @@ -632,20 +632,20 @@ predicate jumpStep(Node n1, Node n2) { v = globalUse.getVariable() and n1.(FinalGlobalValue).getGlobalUse() = globalUse | - globalUse.getIndirectionIndex() = 1 and + globalUse.getIndirection() = 1 and v = n2.asVariable() or - v = n2.asIndirectVariable(globalUse.getIndirectionIndex()) + v = n2.asIndirectVariable(globalUse.getIndirection()) ) or exists(Ssa::GlobalDef globalDef | v = globalDef.getVariable() and n2.(InitialGlobalValue).getGlobalDef() = globalDef | - globalDef.getIndirectionIndex() = 1 and + globalDef.getIndirection() = 1 and v = n1.asVariable() or - v = n1.asIndirectVariable(globalDef.getIndirectionIndex()) + v = n1.asIndirectVariable(globalDef.getIndirection()) ) ) } From 86e791980cc70fd13cde90ed37f57fedb4dbc9bb Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Nov 2023 20:24:14 +0000 Subject: [PATCH 6/8] C++: Simplify 'isGlobalUse' and 'isGlobalDefImpl'. --- .../semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll | 7 +++---- 1 file changed, 3 insertions(+), 4 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 0353c3e1ba64..614ba67715f7 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 @@ -143,11 +143,10 @@ private newtype TDefOrUseImpl = private predicate isGlobalUse( GlobalLikeVariable v, IRFunction f, int indirection, int indirectionIndex ) { - exists(VariableAddressInstruction vai, int defIndex | + exists(VariableAddressInstruction vai | vai.getEnclosingIRFunction() = f and vai.getAstVariable() = v and - isDef(_, _, _, vai, indirection, defIndex) and - indirectionIndex = [0 .. defIndex] + 1 + isDef(_, _, _, vai, indirection, indirectionIndex) ) } @@ -158,7 +157,7 @@ private predicate isGlobalDefImpl( vai.getEnclosingIRFunction() = f and vai.getAstVariable() = v and isUse(_, _, vai, indirection, indirectionIndex) and - not isDef(_, _, vai.getAUse(), _, _, _) + not isDef(_, _, _, vai, _, indirectionIndex) ) } From eb1024c79b28915fcf891b1f1c678a0e0728e3ca Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Nov 2023 20:25:48 +0000 Subject: [PATCH 7/8] C++: Improve (and simplify) 'toString's. --- .../lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 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 614ba67715f7..76741dfc5cc8 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 @@ -525,7 +525,7 @@ class GlobalDefImpl extends DefOrUseImpl, TGlobalDefImpl { */ Type getUnspecifiedType() { result = global.getUnspecifiedType() } - override string toString() { result = "GlobalDef" } + override string toString() { result = "Def of " + this.getSourceVariable() } override Location getLocation() { result = f.getLocation() } @@ -995,7 +995,7 @@ class GlobalDef extends TGlobalDef, SsaDefOrUse { final override Location getLocation() { result = global.getLocation() } /** Gets a textual representation of this definition. */ - override string toString() { result = "GlobalDef" } + override string toString() { result = global.toString() } /** * Holds if this definition has index `index` in block `block`, and From 39b9d2ea83572a3f3bc04e3dcc07929863f064d9 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Nov 2023 18:11:43 +0000 Subject: [PATCH 8/8] C++: Accept test changes. --- cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp | 4 ++-- .../syntax-zoo/dataflow-ir-consistency.expected | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) 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 fed95cdce9c1..73c9fd28b933 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -810,7 +810,7 @@ namespace MoreGlobalTests { void read_indirect1() { sink(global_indirect1); // clean - indirect_sink(*global_indirect1); // $ MISSING: ir,ast + indirect_sink(*global_indirect1); // $ ir MISSING: ast } void set_indirect2() @@ -820,7 +820,7 @@ namespace MoreGlobalTests { void read_indirect2() { sink(global_indirect2); // clean - sink(**global_indirect2); // $ MISSING: ir,ast + sink(**global_indirect2); // $ ir MISSING: ast } // overload source with a boolean parameter so diff --git a/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected b/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected index 0065fe2648fc..4bb6a9e27363 100644 --- a/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected +++ b/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected @@ -4,7 +4,12 @@ uniqueType uniqueNodeLocation missingLocation uniqueNodeToString -| cpp11.cpp:50:15:50:16 | (no string representation) | Node should have one toString but has 0. | +| builtin.c:5:5:5:11 | (no string representation) | Node should have one toString but has 0. | +| misc.c:227:7:227:28 | (no string representation) | Node should have one toString but has 0. | +| static_init_templates.cpp:80:18:80:23 | (no string representation) | Node should have one toString but has 0. | +| static_init_templates.cpp:80:18:80:23 | (no string representation) | Node should have one toString but has 0. | +| static_init_templates.cpp:89:18:89:23 | (no string representation) | Node should have one toString but has 0. | +| static_init_templates.cpp:89:18:89:23 | (no string representation) | Node should have one toString but has 0. | parameterCallable localFlowIsLocal readStepIsLocal