From 5719967a8ed77088a94fbbec567819ba19ab348f Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 7 Apr 2020 12:03:28 +0200 Subject: [PATCH] C++: Remove single-field case from PostUpdateNode and accept tests --- .../ir/dataflow/internal/DataFlowPrivate.qll | 10 ++-- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 52 +++++++------------ .../dataflow-ir-consistency.expected | 1 - .../dataflow/fields/ir-flow.expected | 10 ++++ .../CWE-134/semmle/funcs/funcsLocal.expected | 1 + 5 files changed, 33 insertions(+), 41 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index ddacf2dcb30..f81be05c66b 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -131,12 +131,10 @@ private class ArrayContent extends Content, TArrayContent { * value of `node1`. */ predicate storeStep(Node node1, Content f, PostUpdateNode node2) { - exists(FieldAddressInstruction fa | - exists(StoreInstruction store | - node1.asInstruction() = store and - store.getDestinationAddress() = fa - ) and - node2.getPreUpdateNode().asInstruction() = fa.getObjectAddress() and + exists(FieldAddressInstruction fa, StoreInstruction store | + node1.asInstruction() = store and + store.getDestinationAddress() = fa and + node2.asInstruction().(ChiInstruction).getPartial() = store and f.(FieldContent).getField() = fa.getField() ) } diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index 88d221879fd..a7a5f9d6d86 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll @@ -264,22 +264,6 @@ private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode { override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() } } -private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNode { - override StoreInstruction instr; - FieldAddressInstruction field; - - ExplicitSingleFieldStoreQualifierNode() { - field = instr.getDestinationAddress() and - not exists(ChiInstruction chi | chi.getPartial() = instr) - } - - // Since there is no Chi instruction with a total operand for us to use we let the pre update node - // be the address of the object containing the field. - // Note that, unlike in the case where a struct has multiple fields (and thus has a `Chi` - // instruction), the pre update node will be an instruction with a register result. - override Node getPreUpdateNode() { result.asInstruction() = field.getObjectAddress() } -} - /** * A node that represents the value of a variable after a function call that * may have changed the variable because it's passed by reference. @@ -290,31 +274,31 @@ private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNod * returned. This node will have its `getArgument()` equal to `&x` and its * `getVariableAccess()` equal to `x`. */ -class DefinitionByReferenceNode extends PartialDefinitionNode { - override ChiInstruction instr; - WriteSideEffectInstruction write; - CallInstruction call; - - DefinitionByReferenceNode() { - not instr.isResultConflated() and - instr.getPartial() = write and - call = write.getPrimaryInstruction() - } - - // See the comment on ExplicitFieldStoreQualifierNode::getPreUpdateNode for comments on why - // this causes failures in DataFlowImplConsistency::Consistency. - override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() } +class DefinitionByReferenceNode extends InstructionNode { + override WriteSideEffectInstruction instr; /** Gets the argument corresponding to this node. */ Expr getArgument() { - result = call.getPositionalArgument(write.getIndex()).getUnconvertedResultExpression() + result = + instr + .getPrimaryInstruction() + .(CallInstruction) + .getPositionalArgument(instr.getIndex()) + .getUnconvertedResultExpression() or - result = call.getThisArgument().getUnconvertedResultExpression() and - write.getIndex() = -1 + result = + instr + .getPrimaryInstruction() + .(CallInstruction) + .getThisArgument() + .getUnconvertedResultExpression() and + instr.getIndex() = -1 } /** Gets the parameter through which this value is assigned. */ - Parameter getParameter() { result = call.getStaticCallTarget().getParameter(write.getIndex()) } + Parameter getParameter() { + exists(CallInstruction ci | result = ci.getStaticCallTarget().getParameter(instr.getIndex())) + } } /** 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 99bf7799ff2..3940c1e8389 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 @@ -31,7 +31,6 @@ postIsNotPre postHasUniquePre uniquePostUpdate | ref.cpp:83:5:83:17 | Chi | Node has multiple PostUpdateNodes. | -| ref.cpp:100:34:100:36 | InitializeIndirection | Node has multiple PostUpdateNodes. | | ref.cpp:109:5:109:22 | Chi | Node has multiple PostUpdateNodes. | postIsInSameCallable reverseRead diff --git a/cpp/ql/test/library-tests/dataflow/fields/ir-flow.expected b/cpp/ql/test/library-tests/dataflow/fields/ir-flow.expected index ad56e8af8a7..6d34e81e849 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/ir-flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/ir-flow.expected @@ -5,6 +5,10 @@ edges | aliasing.cpp:42:3:42:22 | Store : void | aliasing.cpp:43:13:43:14 | m1 | | aliasing.cpp:42:11:42:20 | call to user_input : void | aliasing.cpp:42:3:42:22 | Store : void | | aliasing.cpp:42:11:42:20 | call to user_input : void | aliasing.cpp:43:13:43:14 | m1 | +| aliasing.cpp:60:3:60:22 | Chi [m1] : void | aliasing.cpp:61:13:61:14 | Store [m1] : void | +| aliasing.cpp:60:3:60:22 | Store : void | aliasing.cpp:60:3:60:22 | Chi [m1] : void | +| aliasing.cpp:60:11:60:20 | call to user_input : void | aliasing.cpp:60:3:60:22 | Store : void | +| aliasing.cpp:61:13:61:14 | Store [m1] : void | aliasing.cpp:62:14:62:15 | m1 | | aliasing.cpp:79:3:79:22 | Store : void | aliasing.cpp:80:12:80:13 | m1 | | aliasing.cpp:79:11:79:20 | call to user_input : void | aliasing.cpp:79:3:79:22 | Store : void | | aliasing.cpp:79:11:79:20 | call to user_input : void | aliasing.cpp:80:12:80:13 | m1 | @@ -27,6 +31,11 @@ nodes | aliasing.cpp:42:3:42:22 | Store : void | semmle.label | Store : void | | aliasing.cpp:42:11:42:20 | call to user_input : void | semmle.label | call to user_input : void | | aliasing.cpp:43:13:43:14 | m1 | semmle.label | m1 | +| aliasing.cpp:60:3:60:22 | Chi [m1] : void | semmle.label | Chi [m1] : void | +| aliasing.cpp:60:3:60:22 | Store : void | semmle.label | Store : void | +| aliasing.cpp:60:11:60:20 | call to user_input : void | semmle.label | call to user_input : void | +| aliasing.cpp:61:13:61:14 | Store [m1] : void | semmle.label | Store [m1] : void | +| aliasing.cpp:62:14:62:15 | m1 | semmle.label | m1 | | aliasing.cpp:79:3:79:22 | Store : void | semmle.label | Store : void | | aliasing.cpp:79:11:79:20 | call to user_input : void | semmle.label | call to user_input : void | | aliasing.cpp:80:12:80:13 | m1 | semmle.label | m1 | @@ -45,6 +54,7 @@ nodes #select | aliasing.cpp:38:11:38:12 | m1 | aliasing.cpp:37:13:37:22 | call to user_input : void | aliasing.cpp:38:11:38:12 | m1 | m1 flows from $@ | aliasing.cpp:37:13:37:22 | call to user_input : void | call to user_input : void | | aliasing.cpp:43:13:43:14 | m1 | aliasing.cpp:42:11:42:20 | call to user_input : void | aliasing.cpp:43:13:43:14 | m1 | m1 flows from $@ | aliasing.cpp:42:11:42:20 | call to user_input : void | call to user_input : void | +| aliasing.cpp:62:14:62:15 | m1 | aliasing.cpp:60:11:60:20 | call to user_input : void | aliasing.cpp:62:14:62:15 | m1 | m1 flows from $@ | aliasing.cpp:60:11:60:20 | call to user_input : void | call to user_input : void | | aliasing.cpp:80:12:80:13 | m1 | aliasing.cpp:79:11:79:20 | call to user_input : void | aliasing.cpp:80:12:80:13 | m1 | m1 flows from $@ | aliasing.cpp:79:11:79:20 | call to user_input : void | call to user_input : void | | aliasing.cpp:87:12:87:13 | m1 | aliasing.cpp:86:10:86:19 | call to user_input : void | aliasing.cpp:87:12:87:13 | m1 | m1 flows from $@ | aliasing.cpp:86:10:86:19 | call to user_input : void | call to user_input : void | | aliasing.cpp:93:12:93:13 | m1 | aliasing.cpp:92:12:92:21 | call to user_input : void | aliasing.cpp:93:12:93:13 | m1 | m1 flows from $@ | aliasing.cpp:92:12:92:21 | call to user_input : void | call to user_input : void | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/funcs/funcsLocal.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/funcs/funcsLocal.expected index 2417990beb2..5ae6a003bed 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/funcs/funcsLocal.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/funcs/funcsLocal.expected @@ -5,3 +5,4 @@ | funcsLocal.c:37:9:37:10 | i5 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:36:7:36:8 | i5 | gets | | funcsLocal.c:42:9:42:10 | i6 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:41:13:41:16 | call to gets | gets | | funcsLocal.c:42:9:42:10 | i6 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:41:18:41:20 | i61 | gets | +| funcsLocal.c:58:9:58:10 | e1 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:16:8:16:9 | i1 | fread |