diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll index ba608c3e1d1..73cfaf219da 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll @@ -13,9 +13,6 @@ private newtype TNode = TPostConstructorCallNode(ConstructorCall call) or TExplicitParameterNode(Parameter p) { exists(p.getFunction().getBlock()) } or TInstanceParameterNode(MemberFunction f) { exists(f.getBlock()) and not f.isStatic() } or - TDefinitionByReferenceNode(VariableAccess va, Expr argument) { - definitionByReference(va, argument) - } or TUninitializedNode(LocalVariable v) { not v.hasInitializer() } /** @@ -152,12 +149,18 @@ class ImplicitParameterNode extends ParameterNode, TInstanceParameterNode { * `DefinitionByReferenceNode` to represent the value of `x` after the call has * returned. This node will have its `getArgument()` equal to `&x`. */ -class DefinitionByReferenceNode extends Node, TDefinitionByReferenceNode { +class DefinitionByReferenceNode extends PartialDefNode { VariableAccess va; Expr argument; - DefinitionByReferenceNode() { this = TDefinitionByReferenceNode(va, argument) } + DefinitionByReferenceNode() { + exists(DefinitionByReference def | + def = this.getPartialDefinition() and + argument = def.getDefinedExpr() and + va = def.getVariableAccess() + ) + } override Function getFunction() { result = va.getEnclosingFunction() } diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll index 5c849fa1b2f..712bbb72c73 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll @@ -93,27 +93,20 @@ private module PartialDefinitions { isInstanceFieldWrite(fa) and qualifier = fa.getQualifier() } or TExplicitCallQualifier(Expr qualifier, Call call) { qualifier = call.getQualifier() } or - TReferenceArgument(Expr arg, Call call, int i) { isReferenceOrPointerArg(arg, call, i) } + TReferenceArgument(Expr arg, VariableAccess va) { definitionByReference(va, arg) } private predicate isInstanceFieldWrite(FieldAccess fa) { not fa.getTarget().isStatic() and assignmentLikeOperation(_, fa.getTarget(), fa, _) } - private predicate isReferenceOrPointerArg(Expr arg, Call call, int i) { - arg = call.getArgument(i) and - exists(Type t | t = arg.getFullyConverted().getType().getUnspecifiedType() | - t instanceof ReferenceType or t instanceof PointerType - ) - } - class PartialDefinition extends TPartialDefinition { Expr definedExpr; PartialDefinition() { this = TExplicitFieldStoreQualifier(definedExpr, _) or this = TExplicitCallQualifier(definedExpr, _) or - this = TReferenceArgument(definedExpr, _, _) + this = TReferenceArgument(definedExpr, _) } predicate partiallyDefines(Variable v) { definedExpr = v.getAnAccess() } @@ -135,6 +128,22 @@ private module PartialDefinitions { string toString() { result = "partial def of " + definedExpr } } + + /** + * A partial definition that's a definition by reference (in the sense of the + * `definitionByReference` predicate). + */ + class DefinitionByReference extends PartialDefinition, TReferenceArgument { + VariableAccess va; + + DefinitionByReference() { definitionByReference(va, definedExpr) } + + VariableAccess getVariableAccess() { result = va } + + override predicate partiallyDefines(Variable v) { va = v.getAnAccess() } + + override ControlFlowNode getSubBasicBlockStart() { result = va } + } } import PartialDefinitions private import FlowVar_internal @@ -159,13 +168,8 @@ module FlowVar_internal { */ predicate fullySupportedSsaVariable(Variable v) { v = any(SsaDefinition def).getAVariable() and - // After `foo(&x.field)` we need for there to be two definitions of `x`: - // the one that existed before the call to `foo` and the def-by-ref from - // the call. It's fundamental in SSA that each use is only associated with - // one def, so we can't easily get this effect with SSA. - not definitionByReference(v.getAnAccess(), _) and // A partially-defined variable is handled using the partial definitions logic. - not any(PartialDefinitions::PartialDefinition p).partiallyDefines(v) and + not any(PartialDefinition p).partiallyDefines(v) and // SSA variables do not exist before their first assignment, but one // feature of this data flow library is to track where uninitialized data // ends up. @@ -213,10 +217,7 @@ module FlowVar_internal { or assignmentLikeOperation(sbb, v, _, _) or - blockVarDefinedByReference(sbb, v, _) - or - sbb = any(PartialDefinitions::PartialDefinition p | p.partiallyDefines(v)) - .getSubBasicBlockStart() + sbb = any(PartialDefinition p | p.partiallyDefines(v)).getSubBasicBlockStart() or blockVarDefinedByVariable(sbb, v) ) @@ -328,11 +329,15 @@ module FlowVar_internal { } override predicate definedByReference(Expr arg) { - blockVarDefinedByReference(sbb, v, arg) + exists(DefinitionByReference def | + def.partiallyDefines(v) and + sbb = def.getSubBasicBlockStart() and + arg = def.getDefinedExpr() + ) } override predicate definedPartiallyAt(Expr e) { - exists(PartialDefinitions::PartialDefinition p | + exists(PartialDefinition p | p.partiallyDefines(v) and sbb = p.getSubBasicBlockStart() and e = p.getDefinedExpr() @@ -632,11 +637,6 @@ module FlowVar_internal { ) } - predicate blockVarDefinedByReference(ControlFlowNode node, Variable v, Expr argument) { - node = v.getAnAccess() and - definitionByReference(node, argument) - } - /** * Holds if `v` is initialized by `init` to have value `assignedExpr`. */ @@ -660,10 +660,7 @@ module FlowVar_internal { | assignmentLikeOperation(this, v, _, _) or - blockVarDefinedByReference(this, v, _) - or - this = any(PartialDefinitions::PartialDefinition p | p.partiallyDefines(v)) - .getSubBasicBlockStart() + this = any(PartialDefinition p | p.partiallyDefines(v)).getSubBasicBlockStart() // It is not necessary to cut the basic blocks at `Initializer` nodes // because the affected variable can have no _other_ value before its // initializer. It is not necessary to cut basic blocks at procedure diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected index 9ff45c0e172..a14cc1d0929 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected @@ -42,7 +42,7 @@ | test.cpp:431:12:431:13 | 0 | test.cpp:432:33:432:35 | tmp | | test.cpp:431:12:431:13 | 0 | test.cpp:433:8:433:10 | tmp | | test.cpp:432:10:432:13 | & ... | test.cpp:432:3:432:8 | call to memcpy | -| test.cpp:432:10:432:13 | & ... [post update] | test.cpp:432:3:432:8 | call to memcpy | +| test.cpp:432:10:432:13 | ref arg & ... | test.cpp:432:3:432:8 | call to memcpy | | test.cpp:432:10:432:13 | ref arg & ... | test.cpp:432:33:432:35 | tmp | | test.cpp:432:10:432:13 | ref arg & ... | test.cpp:433:8:433:10 | tmp | | test.cpp:432:17:432:23 | source1 | test.cpp:432:10:432:13 | ref arg & ... | @@ -54,7 +54,7 @@ | test.cpp:437:12:437:13 | 0 | test.cpp:440:8:440:10 | tmp | | test.cpp:437:12:437:13 | 0 | test.cpp:442:10:442:12 | tmp | | test.cpp:439:10:439:13 | & ... | test.cpp:439:3:439:8 | call to memcpy | -| test.cpp:439:10:439:13 | & ... [post update] | test.cpp:439:3:439:8 | call to memcpy | +| test.cpp:439:10:439:13 | ref arg & ... | test.cpp:439:3:439:8 | call to memcpy | | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:439:33:439:35 | tmp | | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:440:8:440:10 | tmp | | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:442:10:442:12 | tmp | diff --git a/cpp/ql/test/library-tests/dataflow/fields/flow.expected b/cpp/ql/test/library-tests/dataflow/fields/flow.expected index 2f5ea14557d..99c52b8f2f3 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/flow.expected @@ -24,12 +24,7 @@ edges | A.cpp:103:14:103:14 | c [a, ... (1)] | A.cpp:120:12:120:13 | c1 [a, ... (1)] | | A.cpp:107:12:107:13 | c1 [a, ... (1)] | A.cpp:107:16:107:16 | a | | A.cpp:120:12:120:13 | c1 [a, ... (1)] | A.cpp:120:16:120:16 | a | -| A.cpp:126:5:126:5 | b [post update] [c, ... (1)] | A.cpp:131:8:131:8 | b [post update] [c, ... (1)] | -| A.cpp:126:12:126:18 | new [void] | A.cpp:126:5:126:5 | b [post update] [c, ... (1)] | -| A.cpp:131:8:131:8 | b [post update] [c, ... (1)] | A.cpp:132:10:132:10 | b [c, ... (1)] | -| A.cpp:132:10:132:10 | b [c, ... (1)] | A.cpp:132:13:132:13 | c | | A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | A.cpp:143:7:143:31 | ... = ... [c, ... (1)] | -| A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | A.cpp:151:18:151:18 | b [post update] [c, ... (1)] | | A.cpp:142:7:142:20 | ... = ... [void] | A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | | A.cpp:142:14:142:20 | new [void] | A.cpp:142:7:142:20 | ... = ... [void] | | A.cpp:143:7:143:10 | this [post update] [b, ... (1)] | A.cpp:151:12:151:24 | call to D [post update] [b, ... (1)] | @@ -40,12 +35,10 @@ edges | A.cpp:150:12:150:18 | new [void] | A.cpp:151:18:151:18 | b [void] | | A.cpp:151:12:151:24 | call to D [post update] [b, ... (1)] | A.cpp:152:10:152:10 | d [b, ... (1)] | | A.cpp:151:12:151:24 | call to D [post update] [b, ... (2)] | A.cpp:153:10:153:10 | d [b, ... (2)] | -| A.cpp:151:18:151:18 | b [post update] [c, ... (1)] | A.cpp:154:10:154:10 | b [c, ... (1)] | | A.cpp:151:18:151:18 | b [void] | A.cpp:151:12:151:24 | call to D [post update] [b, ... (1)] | | A.cpp:152:10:152:10 | d [b, ... (1)] | A.cpp:152:13:152:13 | b | | A.cpp:153:10:153:10 | d [b, ... (2)] | A.cpp:153:13:153:13 | b [c, ... (1)] | | A.cpp:153:13:153:13 | b [c, ... (1)] | A.cpp:153:16:153:16 | c | -| A.cpp:154:10:154:10 | b [c, ... (1)] | A.cpp:154:13:154:13 | c | | A.cpp:159:12:159:18 | new [void] | A.cpp:160:29:160:29 | b [void] | | A.cpp:160:18:160:60 | call to MyList [post update] [head, ... (1)] | A.cpp:161:38:161:39 | l1 [head, ... (1)] | | A.cpp:160:29:160:29 | b [void] | A.cpp:160:18:160:60 | call to MyList [post update] [head, ... (1)] | @@ -114,11 +107,9 @@ edges | A.cpp:75:14:75:14 | c | A.cpp:73:25:73:32 | new [void] | A.cpp:75:14:75:14 | c | c flows from $@ | A.cpp:73:25:73:32 | new [void] | new [void] | | A.cpp:107:16:107:16 | a | A.cpp:98:12:98:18 | new [void] | A.cpp:107:16:107:16 | a | a flows from $@ | A.cpp:98:12:98:18 | new [void] | new [void] | | A.cpp:120:16:120:16 | a | A.cpp:98:12:98:18 | new [void] | A.cpp:120:16:120:16 | a | a flows from $@ | A.cpp:98:12:98:18 | new [void] | new [void] | -| A.cpp:132:13:132:13 | c | A.cpp:126:12:126:18 | new [void] | A.cpp:132:13:132:13 | c | c flows from $@ | A.cpp:126:12:126:18 | new [void] | new [void] | | A.cpp:152:13:152:13 | b | A.cpp:143:25:143:31 | new [void] | A.cpp:152:13:152:13 | b | b flows from $@ | A.cpp:143:25:143:31 | new [void] | new [void] | | A.cpp:152:13:152:13 | b | A.cpp:150:12:150:18 | new [void] | A.cpp:152:13:152:13 | b | b flows from $@ | A.cpp:150:12:150:18 | new [void] | new [void] | | A.cpp:153:16:153:16 | c | A.cpp:142:14:142:20 | new [void] | A.cpp:153:16:153:16 | c | c flows from $@ | A.cpp:142:14:142:20 | new [void] | new [void] | -| A.cpp:154:13:154:13 | c | A.cpp:142:14:142:20 | new [void] | A.cpp:154:13:154:13 | c | c flows from $@ | A.cpp:142:14:142:20 | new [void] | new [void] | | A.cpp:165:26:165:29 | head | A.cpp:159:12:159:18 | new [void] | A.cpp:165:26:165:29 | head | head flows from $@ | A.cpp:159:12:159:18 | new [void] | new [void] | | A.cpp:169:15:169:18 | head | A.cpp:159:12:159:18 | new [void] | A.cpp:169:15:169:18 | head | head flows from $@ | A.cpp:159:12:159:18 | new [void] | new [void] | | B.cpp:9:20:9:24 | elem1 | B.cpp:6:15:6:24 | new [void] | B.cpp:9:20:9:24 | elem1 | elem1 flows from $@ | B.cpp:6:15:6:24 | new [void] | new [void] | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index a8346219c26..875e5619c84 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -140,24 +140,17 @@ | taint.cpp:165:22:165:25 | {...} | taint.cpp:172:10:172:15 | buffer | | | taint.cpp:165:22:165:25 | {...} | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:165:24:165:24 | 0 | taint.cpp:165:22:165:25 | {...} | TAINT | -| taint.cpp:168:8:168:14 | tainted [post update] | taint.cpp:172:18:172:24 | tainted | | | taint.cpp:170:10:170:15 | buffer | taint.cpp:170:3:170:8 | call to strcpy | | -| taint.cpp:170:10:170:15 | buffer [post update] | taint.cpp:170:3:170:8 | call to strcpy | | -| taint.cpp:170:10:170:15 | buffer [post update] | taint.cpp:171:8:171:13 | buffer | | -| taint.cpp:170:10:170:15 | buffer [post update] | taint.cpp:172:10:172:15 | buffer | | -| taint.cpp:170:10:170:15 | buffer [post update] | taint.cpp:173:8:173:13 | buffer | | +| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:170:3:170:8 | call to strcpy | | | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:171:8:171:13 | buffer | | | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:172:10:172:15 | buffer | | | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:170:18:170:26 | Hello, | taint.cpp:170:10:170:15 | ref arg buffer | TAINT | -| taint.cpp:171:8:171:13 | buffer [post update] | taint.cpp:172:10:172:15 | buffer | | -| taint.cpp:171:8:171:13 | buffer [post update] | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:172:10:172:15 | buffer | | | taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:3:172:8 | call to strcat | | | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:10:172:15 | ref arg buffer | TAINT | -| taint.cpp:172:10:172:15 | buffer [post update] | taint.cpp:172:3:172:8 | call to strcat | | -| taint.cpp:172:10:172:15 | buffer [post update] | taint.cpp:173:8:173:13 | buffer | | +| taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:172:3:172:8 | call to strcat | | | taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:172:18:172:24 | tainted | taint.cpp:172:10:172:15 | ref arg buffer | TAINT | | taint.cpp:180:19:180:19 | p | taint.cpp:181:9:181:9 | p | | @@ -168,7 +161,7 @@ | taint.cpp:193:6:193:6 | x | taint.cpp:194:10:194:10 | x | | | taint.cpp:193:6:193:6 | x | taint.cpp:195:7:195:7 | x | | | taint.cpp:194:9:194:10 | & ... | taint.cpp:194:2:194:7 | call to memcpy | | -| taint.cpp:194:9:194:10 | & ... [post update] | taint.cpp:194:2:194:7 | call to memcpy | | +| taint.cpp:194:9:194:10 | ref arg & ... | taint.cpp:194:2:194:7 | call to memcpy | | | taint.cpp:194:9:194:10 | ref arg & ... | taint.cpp:195:7:195:7 | x | | | taint.cpp:194:10:194:10 | x | taint.cpp:194:9:194:10 | & ... | TAINT | | taint.cpp:194:13:194:18 | source | taint.cpp:194:9:194:10 | ref arg & ... | TAINT | @@ -183,7 +176,5 @@ | taint.cpp:208:6:208:6 | 0 | taint.cpp:216:7:216:7 | y | | | taint.cpp:213:12:213:12 | ref arg x | taint.cpp:215:7:215:7 | x | | | taint.cpp:213:12:213:12 | x | taint.cpp:213:15:213:15 | ref arg y | | -| taint.cpp:213:12:213:12 | x [post update] | taint.cpp:215:7:215:7 | x | | | taint.cpp:213:15:213:15 | ref arg y | taint.cpp:216:7:216:7 | y | | | taint.cpp:213:15:213:15 | y | taint.cpp:213:12:213:12 | ref arg x | | -| taint.cpp:213:15:213:15 | y [post update] | taint.cpp:216:7:216:7 | y | |