From e8afec413aabf7407e5502bf1245d50aacb22af9 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 14 Jan 2022 13:34:27 +0000 Subject: [PATCH 1/3] C++: Add testcase that demonstrates a FP caused by spurious flow through phi nodes in IR dataflow. --- .../dataflow-ir-consistency.expected | 3 +++ .../library-tests/dataflow/dataflow-tests/test.cpp | 14 +++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected index 52511babde2c..1d9d16d9086b 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected @@ -570,6 +570,9 @@ postWithInFlow | test.cpp:481:24:481:30 | FieldAddress [post update] | PostUpdateNode should not be the target of local flow. | | test.cpp:481:24:481:30 | content [post update] | PostUpdateNode should not be the target of local flow. | | test.cpp:482:8:482:16 | VariableAddress [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:489:7:489:7 | VariableAddress [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:491:5:491:5 | x [post update] | PostUpdateNode should not be the target of local flow. | +| test.cpp:494:5:494:5 | x [post update] | PostUpdateNode should not be the target of local flow. | | true_upon_entry.cpp:9:7:9:7 | VariableAddress [post update] | PostUpdateNode should not be the target of local flow. | | true_upon_entry.cpp:10:12:10:12 | VariableAddress [post update] | PostUpdateNode should not be the target of local flow. | | true_upon_entry.cpp:10:27:10:27 | VariableAddress [post update] | PostUpdateNode should not be the target of local flow. | 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 e4a2eb4a0b86..697dbbf7c1f1 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -481,4 +481,16 @@ void local_field_flow_def_by_ref_steps_with_local_flow(MyStruct * s) { writes_to_content(s->content); int* p_content = s->content; sink(*p_content); -} \ No newline at end of file +} + +bool unknown(); + +void regression_with_phi_flow(int clean1) { + int x = 0; + while (unknown()) { + x = clean1; + if (unknown()) { } + sink(x); // $ SPURIOUS: ir + x = source(); + } +} From 25253c7b8df9a5bf800841881575fc9803f8d7c0 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 14 Jan 2022 13:39:57 +0000 Subject: [PATCH 2/3] C++: Don't count write operations as uses for IR dataflow. Accept test changes. --- .../code/cpp/ir/dataflow/internal/SsaInternals.qll | 14 ++++++++++---- .../library-tests/dataflow/dataflow-tests/test.cpp | 2 +- 2 files changed, 11 insertions(+), 5 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 18ba4c3a607e..24b51f83aca0 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 @@ -170,10 +170,16 @@ private class ReturnParameterIndirection extends Use, TReturnParamIndirection { } private predicate isExplicitUse(Operand op) { - op.getDef() instanceof VariableAddressInstruction and - not exists(LoadInstruction load | - load.getSourceAddressOperand() = op and - load.getAUse().getUse() instanceof InitializeIndirectionInstruction + exists(VariableAddressInstruction vai | vai = op.getDef() | + // Don't include this operand as a use if it only exists to initialize the + // indirection of a parameter. + not exists(LoadInstruction load | + load.getSourceAddressOperand() = op and + load.getAUse().getUse() instanceof InitializeIndirectionInstruction + ) and + // Don't include this operand as a use if the only use of the address is for a write + // that definately overrides a variable. + not (explicitWrite(true, _, vai) and exists(unique( | | vai.getAUse()))) ) } 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 697dbbf7c1f1..5e5c5279f160 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -490,7 +490,7 @@ void regression_with_phi_flow(int clean1) { while (unknown()) { x = clean1; if (unknown()) { } - sink(x); // $ SPURIOUS: ir + sink(x); // clean x = source(); } } From e1598aba5eedbf9652280f8323b8025063b6c43a Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 18 Jan 2022 09:44:36 +0000 Subject: [PATCH 3/3] C++: Fix spelling. --- .../lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 24b51f83aca0..7aefc3893f20 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 @@ -178,7 +178,7 @@ private predicate isExplicitUse(Operand op) { load.getAUse().getUse() instanceof InitializeIndirectionInstruction ) and // Don't include this operand as a use if the only use of the address is for a write - // that definately overrides a variable. + // that definitely overrides a variable. not (explicitWrite(true, _, vai) and exists(unique( | | vai.getAUse()))) ) }