From 7439de37a30f27b36971b416e7b5a254a826342b Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Feb 2023 16:23:32 +0000 Subject: [PATCH 1/4] C++: Add a new test that demonstrates multiple out nodes. --- .../test-number-of-outnodes.expected | 3 ++ .../dataflow-tests/test-number-of-outnodes.ql | 46 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected create mode 100644 cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.ql diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected new file mode 100644 index 00000000000..1ec62f02ba8 --- /dev/null +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected @@ -0,0 +1,3 @@ +| dispatch.cpp:60:18:60:29 | Call: new | Unexpected result: ir-count(indirect return)=3 | +| dispatch.cpp:61:18:61:29 | Call: new | Unexpected result: ir-count(indirect return)=3 | +| dispatch.cpp:65:10:65:21 | Call: new | Unexpected result: ir-count(indirect return)=3 | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.ql b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.ql new file mode 100644 index 00000000000..dbaf015630c --- /dev/null +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.ql @@ -0,0 +1,46 @@ +import TestUtilities.InlineExpectationsTest +import cpp + +module AstTest { + private import semmle.code.cpp.dataflow.old.DataFlow::DataFlow + private import semmle.code.cpp.dataflow.old.internal.DataFlowPrivate + + class ASTMultipleOutNodesTest extends InlineExpectationsTest { + ASTMultipleOutNodesTest() { this = "ASTMultipleOutNodesTest" } + + override string getARelevantTag() { result = "ast-count(" + any(ReturnKind k).toString() + ")" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(DataFlowCall call, int n, ReturnKind kind | + call.getLocation() = location and + n = strictcount(getAnOutNode(call, kind)) and + n > 1 and + element = call.toString() and + tag = "ast-count(" + kind.toString() + ")" and + value = n.toString() + ) + } + } +} + +module IRTest { + private import semmle.code.cpp.ir.dataflow.DataFlow + private import semmle.code.cpp.ir.dataflow.internal.DataFlowPrivate + + class IRMultipleOutNodesTest extends InlineExpectationsTest { + IRMultipleOutNodesTest() { this = "IRMultipleOutNodesTest" } + + override string getARelevantTag() { result = "ir-count(" + any(ReturnKind k).toString() + ")" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(DataFlowCall call, int n, ReturnKind kind | + call.getLocation() = location and + n = strictcount(getAnOutNode(call, kind)) and + n > 1 and + element = call.toString() and + tag = "ir-count(" + kind.toString() + ")" and + value = n.toString() + ) + } + } +} From 6b851d0529085dbc34d5ae02797e08f7845a7de2 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Feb 2023 16:55:19 +0000 Subject: [PATCH 2/4] C++: Fix an inconsistency with too many out nodes. --- .../ir/dataflow/internal/DataFlowPrivate.qll | 47 +++++++++++++++---- .../test-number-of-outnodes.expected | 3 -- .../fields/dataflow-ir-consistency.expected | 20 -------- .../dataflow/fields/ir-path-flow.expected | 1 - 4 files changed, 39 insertions(+), 32 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 2493a617cb3..f5cf73fb76d 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 @@ -367,7 +367,7 @@ class ReturnIndirectionNode extends IndirectReturnNode, ReturnNode { } } -private Operand fullyConvertedCallStep(Operand op) { +private Operand fullyConvertedCallStepImpl(Operand op) { not exists(getANonConversionUse(op)) and exists(Instruction instr | conversionFlow(op, instr, _, _) and @@ -375,6 +375,10 @@ private Operand fullyConvertedCallStep(Operand op) { ) } +private Operand fullyConvertedCallStep(Operand op) { + result = unique( | | fullyConvertedCallStepImpl(op)) +} + /** * Gets the instruction that uses this operand, if the instruction is not * ignored for dataflow purposes. @@ -401,10 +405,21 @@ private Instruction getANonConversionUse(Operand operand) { } /** - * Gets the operand that represents the first use of the value of `call` following + * Gets an operand that represents the use of the value of `call` following * a sequence of conversion-like instructions. + * + * Note that `operand` is not functionally determined by `call` since there + * can be multiple sequences of disjoint conversions following a call. For example, + * consider an example like: + * ```cpp + * long f(); + * int y; + * long x = (long)(y = (int)f()); + * ``` + * in this case, there'll be a long-to-int conversion on `f()` before the value is assigned to `y`, + * and there will be an int-to-long conversion on `(int)f()` before the value is assigned to `x`. */ -predicate operandForFullyConvertedCall(Operand operand, CallInstruction call) { +private predicate operandForFullyConvertedCallImpl(Operand operand, CallInstruction call) { exists(getANonConversionUse(operand)) and ( operand = getAUse(call) @@ -413,6 +428,25 @@ predicate operandForFullyConvertedCall(Operand operand, CallInstruction call) { ) } +/** + * Gets the operand that represents the use of the value of `call` following + * a sequence of conversion-like instructions, if a unique operand exists. + */ +predicate operandForFullyConvertedCall(Operand operand, CallInstruction call) { + operand = unique(Operand cand | operandForFullyConvertedCallImpl(cand, call)) +} + +private predicate instructionForFullyConvertedCallWithConversions( + Instruction instr, CallInstruction call +) { + // Otherwise, flow to the first non-conversion use. + instr = + getUse(unique(Operand operand | + operand = fullyConvertedCallStep*(getAUse(call)) and + not exists(fullyConvertedCallStep(operand)) + )) +} + /** * Gets the instruction that represents the first use of the value of `call` following * a sequence of conversion-like instructions. @@ -424,13 +458,10 @@ predicate instructionForFullyConvertedCall(Instruction instr, CallInstruction ca not operandForFullyConvertedCall(_, call) and ( // If there is no use of the call then we pick the call instruction - not exists(getAUse(call)) and + not instructionForFullyConvertedCallWithConversions(_, call) and instr = call or - // Otherwise, flow to the first non-conversion use. - exists(Operand operand | operand = fullyConvertedCallStep*(getAUse(call)) | - instr = getANonConversionUse(operand) - ) + instructionForFullyConvertedCallWithConversions(instr, call) ) } diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected index 1ec62f02ba8..e69de29bb2d 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test-number-of-outnodes.expected @@ -1,3 +0,0 @@ -| dispatch.cpp:60:18:60:29 | Call: new | Unexpected result: ir-count(indirect return)=3 | -| dispatch.cpp:61:18:61:29 | Call: new | Unexpected result: ir-count(indirect return)=3 | -| dispatch.cpp:65:10:65:21 | Call: new | Unexpected result: ir-count(indirect return)=3 | diff --git a/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected b/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected index 1e9c150b01b..f5e53c2d7a3 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected @@ -12,16 +12,6 @@ compatibleTypesReflexive unreachableNodeCCtx localCallNodes postIsNotPre -| A.cpp:98:12:98:18 | new indirection | PostUpdateNode should not equal its pre-update node. | -| B.cpp:6:15:6:24 | new indirection | PostUpdateNode should not equal its pre-update node. | -| B.cpp:15:15:15:27 | new indirection | PostUpdateNode should not equal its pre-update node. | -| C.cpp:22:12:22:21 | new indirection | PostUpdateNode should not equal its pre-update node. | -| C.cpp:24:16:24:25 | new indirection | PostUpdateNode should not equal its pre-update node. | -| D.cpp:28:15:28:24 | new indirection | PostUpdateNode should not equal its pre-update node. | -| D.cpp:35:15:35:24 | new indirection | PostUpdateNode should not equal its pre-update node. | -| D.cpp:42:15:42:24 | new indirection | PostUpdateNode should not equal its pre-update node. | -| D.cpp:49:15:49:24 | new indirection | PostUpdateNode should not equal its pre-update node. | -| D.cpp:56:15:56:24 | new indirection | PostUpdateNode should not equal its pre-update node. | postHasUniquePre uniquePostUpdate | aliasing.cpp:70:11:70:11 | definition of w indirection | Node has multiple PostUpdateNodes. | @@ -42,16 +32,6 @@ postIsInSameCallable reverseRead argHasPostUpdate postWithInFlow -| A.cpp:98:12:98:18 | new indirection | PostUpdateNode should not be the target of local flow. | -| B.cpp:6:15:6:24 | new indirection | PostUpdateNode should not be the target of local flow. | -| B.cpp:15:15:15:27 | new indirection | PostUpdateNode should not be the target of local flow. | -| C.cpp:22:12:22:21 | new indirection | PostUpdateNode should not be the target of local flow. | -| C.cpp:24:16:24:25 | new indirection | PostUpdateNode should not be the target of local flow. | -| D.cpp:28:15:28:24 | new indirection | PostUpdateNode should not be the target of local flow. | -| D.cpp:35:15:35:24 | new indirection | PostUpdateNode should not be the target of local flow. | -| D.cpp:42:15:42:24 | new indirection | PostUpdateNode should not be the target of local flow. | -| D.cpp:49:15:49:24 | new indirection | PostUpdateNode should not be the target of local flow. | -| D.cpp:56:15:56:24 | new indirection | PostUpdateNode should not be the target of local flow. | | realistic.cpp:54:16:54:47 | memcpy output argument | PostUpdateNode should not be the target of local flow. | | realistic.cpp:60:16:60:18 | memcpy output argument | PostUpdateNode should not be the target of local flow. | viableImplInCallContextTooLarge diff --git a/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected b/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected index c238e7aac96..ddc7c397f1e 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected @@ -31,7 +31,6 @@ edges | A.cpp:57:11:57:24 | call to B [c] | A.cpp:57:11:57:24 | new indirection [c] | | A.cpp:57:11:57:24 | new indirection [c] | A.cpp:28:8:28:10 | this indirection [c] | | A.cpp:57:11:57:24 | new indirection [c] | A.cpp:57:10:57:32 | call to get | -| A.cpp:57:11:57:24 | new indirection [c] | A.cpp:57:11:57:24 | new indirection [c] | | A.cpp:57:17:57:23 | new | A.cpp:23:10:23:10 | c | | A.cpp:57:17:57:23 | new | A.cpp:57:11:57:24 | call to B [c] | | A.cpp:57:17:57:23 | new | A.cpp:57:17:57:23 | new | From 981c97675450e9ea8249f84e69faa9b2ba66cb74 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 9 Feb 2023 17:10:07 +0000 Subject: [PATCH 3/4] C++: Expand comments. --- .../semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 f5cf73fb76d..007f0592c0d 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 @@ -439,7 +439,6 @@ predicate operandForFullyConvertedCall(Operand operand, CallInstruction call) { private predicate instructionForFullyConvertedCallWithConversions( Instruction instr, CallInstruction call ) { - // Otherwise, flow to the first non-conversion use. instr = getUse(unique(Operand operand | operand = fullyConvertedCallStep*(getAUse(call)) and @@ -455,12 +454,14 @@ private predicate instructionForFullyConvertedCallWithConversions( * conversion instruction) to use to represent the value of `call` after conversions. */ predicate instructionForFullyConvertedCall(Instruction instr, CallInstruction call) { + // Only pick an instruction for the call if we cannot pick a unique operand. not operandForFullyConvertedCall(_, call) and ( // If there is no use of the call then we pick the call instruction not instructionForFullyConvertedCallWithConversions(_, call) and instr = call or + // Otherwise, flow to the first instruction that defines multiple operands. instructionForFullyConvertedCallWithConversions(instr, call) ) } From 4719fd5235169e9c3fd4194b64d5ab745d6b41d7 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Fri, 10 Feb 2023 08:38:46 +0000 Subject: [PATCH 4/4] C++: Accept more test changes. --- .../syntax-zoo/dataflow-ir-consistency.expected | 10 ---------- 1 file changed, 10 deletions(-) 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 2d9721e86a5..5e3dcaba931 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 @@ -223,11 +223,6 @@ compatibleTypesReflexive unreachableNodeCCtx localCallNodes postIsNotPre -| condition_decls.cpp:3:13:3:22 | new indirection | PostUpdateNode should not equal its pre-update node. | -| defdestructordeleteexpr.cpp:4:9:4:15 | new indirection | PostUpdateNode should not equal its pre-update node. | -| deleteexpr.cpp:7:9:7:15 | new indirection | PostUpdateNode should not equal its pre-update node. | -| ir.cpp:943:3:943:11 | new indirection | PostUpdateNode should not equal its pre-update node. | -| ir.cpp:947:3:947:25 | new indirection | PostUpdateNode should not equal its pre-update node. | postHasUniquePre uniquePostUpdate | cpp11.cpp:82:17:82:17 | this indirection | Node has multiple PostUpdateNodes. | @@ -251,19 +246,14 @@ postIsInSameCallable reverseRead argHasPostUpdate postWithInFlow -| condition_decls.cpp:3:13:3:22 | new indirection | PostUpdateNode should not be the target of local flow. | | cpp11.cpp:77:19:77:21 | call to Val | PostUpdateNode should not be the target of local flow. | | cpp11.cpp:82:11:82:14 | call to Val | PostUpdateNode should not be the target of local flow. | | cpp11.cpp:82:45:82:48 | call to Val | PostUpdateNode should not be the target of local flow. | | cpp11.cpp:82:51:82:51 | call to Val | PostUpdateNode should not be the target of local flow. | -| defdestructordeleteexpr.cpp:4:9:4:15 | new indirection | PostUpdateNode should not be the target of local flow. | -| deleteexpr.cpp:7:9:7:15 | new indirection | PostUpdateNode should not be the target of local flow. | | ir.cpp:809:7:809:13 | call to Base | PostUpdateNode should not be the target of local flow. | | ir.cpp:810:7:810:26 | call to Base | PostUpdateNode should not be the target of local flow. | | ir.cpp:823:7:823:13 | call to Base | PostUpdateNode should not be the target of local flow. | | ir.cpp:824:7:824:26 | call to Base | PostUpdateNode should not be the target of local flow. | -| ir.cpp:943:3:943:11 | new indirection | PostUpdateNode should not be the target of local flow. | -| ir.cpp:947:3:947:25 | new indirection | PostUpdateNode should not be the target of local flow. | | try_catch.cpp:7:8:7:8 | call to exception | PostUpdateNode should not be the target of local flow. | viableImplInCallContextTooLarge uniqueParameterNodeAtPosition