C++: Don't use definitionByReference for data flow #1777
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,7 +105,7 @@ private module PartialDefinitions { | |
| ) | ||
| } or | ||
| TExplicitCallQualifier(Expr qualifier, Call call) { qualifier = call.getQualifier() } or | ||
| TReferenceArgument(Expr arg, VariableAccess va) { referenceArgument(va, arg) } | ||
|
|
||
| private predicate isInstanceFieldWrite(FieldAccess fa, ControlFlowNode node) { | ||
| not fa.getTarget().isStatic() and | ||
|
|
@@ -154,18 +154,48 @@ private module PartialDefinitions { | |
| } | ||
|
|
||
| /** | ||
| * A partial definition that's a definition by reference. | ||
| */ | ||
| class DefinitionByReference extends PartialDefinition, TReferenceArgument { | ||
| VariableAccess va; | ||
|
|
||
| DefinitionByReference() { | ||
| // `this` is not restricted in this charpred. That's because the full | ||
| // extent of this class includes the charpred of the superclass, which | ||
| // relates `this` to `definedExpr`, and `va` is functionally determined | ||
| // by `definedExpr`. | ||
| referenceArgument(va, definedExpr) | ||
| } | ||
|
|
||
| VariableAccess getVariableAccess() { result = va } | ||
|
|
||
| override predicate partiallyDefines(Variable v) { va = v.getAnAccess() } | ||
| } | ||
|
|
||
| private predicate referenceArgument(VariableAccess va, Expr argument) { | ||
| argument = any(Call c).getAnArgument() and | ||
| exists(Type argumentType | | ||
| argumentType = argument.getFullyConverted().getType().stripTopLevelSpecifiers() | ||
| | | ||
| argumentType instanceof ReferenceType and | ||
| not argumentType.(ReferenceType).getBaseType().isConst() and | ||
| va = argument | ||
| or | ||
| argumentType instanceof PointerType and | ||
| not argumentType.(PointerType).getBaseType().isConst() and | ||
| ( | ||
| // f(variable) | ||
| va = argument | ||
| or | ||
| // f(&variable) | ||
| va = argument.(AddressOfExpr).getOperand() | ||
| or | ||
| // f(&array[0]) | ||
|
Contributor
There was a problem hiding this comment. should this be generalized to handle multi-dimensional arrays?
Contributor
Author
There was a problem hiding this comment. Is it normal to pass a multi-dimensional array as Can you find other common cases that aren't handled right here? I wouldn't be surprised if there are more problems.
Contributor
There was a problem hiding this comment. I don't think it would be normal to do that, but I wouldn't be surprised if something like This implicitly covers
Contributor
There was a problem hiding this comment. Looking at this again, I'm not at all sure if
Contributor
Author
There was a problem hiding this comment. Thank you for the reminder. I didn't see the The In summary, I think |
||
| va.getType().getUnspecifiedType() instanceof ArrayType and | ||
| va = argument.(AddressOfExpr).getOperand().(ArrayExpr).getArrayBase() | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| import PartialDefinitions | ||
| private import FlowVar_internal | ||
|
|
@@ -236,7 +266,7 @@ module FlowVar_internal { | |
| not v instanceof Field and // Fields are interprocedural data flow, not local | ||
| reachable(sbb) and | ||
| ( | ||
| initializer(v, sbb.getANode()) | ||
| or | ||
| assignmentLikeOperation(sbb, v, _, _) | ||
| or | ||
|
|
@@ -353,7 +383,12 @@ module FlowVar_internal { | |
| assignmentLikeOperation(node, v, _, e) and | ||
| node = sbb | ||
| or | ||
| // We pick the defining `ControlFlowNode` of an `Initializer` to be its | ||
| // expression rather than the `Initializer` itself. That's because the | ||
| // `Initializer` of a `ConditionDeclExpr` is for historical reasons not | ||
| // part of the CFG and therefore ends up in the wrong basic block. | ||
| initializer(v, e) and | ||
| node = e and | ||
| node = sbb.getANode() | ||
| } | ||
|
|
||
|
|
@@ -711,13 +746,11 @@ module FlowVar_internal { | |
| } | ||
|
|
||
| /** | ||
| * Holds if `v` is initialized to have value `assignedExpr`. | ||
| */ | ||
| predicate initializer(LocalVariable v, Expr assignedExpr) | ||
| { | ||
| assignedExpr = v.getInitializer().getExpr() | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about rvalue references to non-const types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe those are ruled out with this predicate. The argument is restricted to be a
VariableAccess, and aVariableAccessis never an rvalue unless you send it throughstd::move(or cast it yourself, but that should be very rare).