Skip to content

Commit dd138b0

Browse files
hvitvedhvitved
committed
Address review comments
1 parent ec5d8ab commit dd138b0

37 files changed

+767
-727
lines changed

‎cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll

Lines changed: 29 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,8 @@ private class ParamNodeEx extends NodeEx {
244244
}
245245

246246
int getPosition() { this.isParameterOf(_, result) }
247+
248+
predicate allowParameterReturnInSelf() { allowParameterReturnInSelfCached(this.asNode()) }
247249
}
248250

249251
private class RetNodeEx extends NodeEx {
@@ -252,8 +254,6 @@ private class RetNodeEx extends NodeEx {
252254
ReturnPosition getReturnPosition() { result = getReturnPosition(this.asNode()) }
253255

254256
ReturnKindExt getKind() { result = this.asNode().(ReturnNodeExt).getKind() }
255-
256-
predicate allowFlowThroughParameter() { allowFlowThroughParameterCached(this.asNode()) }
257257
}
258258

259259
private predicate inBarrier(NodeEx node, Configuration config) {
@@ -727,16 +727,12 @@ private module Stage1 {
727727
/** Holds if flow may return from `callable`. */
728728
pragma[nomagic]
729729
private predicate returnFlowCallableNodeCand(
730-
DataFlowCallable callable, ReturnKindExt kind, boolean allowFlowThroughParameter,
731-
Configuration config
730+
DataFlowCallable callable, ReturnKindExt kind, Configuration config
732731
) {
733732
exists(RetNodeEx ret |
734733
throughFlowNodeCand(ret, config) and
735734
callable = ret.getEnclosingCallable() and
736-
kind = ret.getKind() and
737-
if ret.allowFlowThroughParameter()
738-
then allowFlowThroughParameter = true
739-
else allowFlowThroughParameter = false
735+
kind = ret.getKind()
740736
)
741737
}
742738

@@ -745,16 +741,17 @@ private module Stage1 {
745741
* candidate for the origin of a summary.
746742
*/
747743
predicate parameterMayFlowThrough(ParamNodeEx p, DataFlowCallable c, Ap ap, Configuration config) {
748-
exists(ReturnKindExt kind, boolean allowFlowThroughParameter |
744+
exists(ReturnKindExt kind |
749745
throughFlowNodeCand(p, config) and
750-
returnFlowCallableNodeCand(c, kind, allowFlowThroughParameter, config) and
746+
returnFlowCallableNodeCand(c, kind, config) and
751747
p.getEnclosingCallable() = c and
748+
exists(ap) and
749+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
752750
(
753-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
754-
then allowFlowThroughParameter = true
755-
else any()
756-
) and
757-
exists(ap)
751+
not kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
752+
or
753+
p.allowParameterReturnInSelf()
754+
)
758755
)
759756
}
760757

@@ -1403,10 +1400,11 @@ private module Stage2 {
14031400
fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
14041401
kind = ret.getKind() and
14051402
p.getPosition() = pos and
1403+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
14061404
(
1407-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
1408-
then ret.allowFlowThroughParameter()
1409-
else any()
1405+
not kind.(ParamUpdateReturnKind).getPosition() = pos
1406+
or
1407+
p.allowParameterReturnInSelf()
14101408
)
14111409
)
14121410
}
@@ -2095,10 +2093,11 @@ private module Stage3 {
20952093
fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
20962094
kind = ret.getKind() and
20972095
p.getPosition() = pos and
2096+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
20982097
(
2099-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
2100-
then ret.allowFlowThroughParameter()
2101-
else any()
2098+
not kind.(ParamUpdateReturnKind).getPosition() = pos
2099+
or
2100+
p.allowParameterReturnInSelf()
21022101
)
21032102
)
21042103
}
@@ -2858,10 +2857,11 @@ private module Stage4 {
28582857
fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
28592858
kind = ret.getKind() and
28602859
p.getPosition() = pos and
2860+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
28612861
(
2862-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
2863-
then ret.allowFlowThroughParameter()
2864-
else any()
2862+
not kind.(ParamUpdateReturnKind).getPosition() = pos
2863+
or
2864+
p.allowParameterReturnInSelf()
28652865
)
28662866
)
28672867
}
@@ -2935,7 +2935,7 @@ private class SummaryCtxSome extends SummaryCtx, TSummaryCtxSome {
29352935

29362936
int getParameterPos() { p.isParameterOf(_, result) }
29372937

2938-
ParameterNode getParameterNode() { result = p.asNode() }
2938+
ParamNodeEx getParamNode() { result = p }
29392939

29402940
override string toString() { result = p + ": " + ap }
29412941

@@ -3637,10 +3637,11 @@ private predicate paramFlowsThrough(
36373637
ap = mid.getAp() and
36383638
apa = ap.getApprox() and
36393639
pos = sc.getParameterPos() and
3640+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
36403641
(
3641-
if kind.(ParamUpdateReturnKind).getPosition() = pos
3642-
then ret.allowFlowThroughParameter()
3643-
else any()
3642+
not kind.(ParamUpdateReturnKind).getPosition() = pos
3643+
or
3644+
sc.getParamNode().allowParameterReturnInSelf()
36443645
)
36453646
)
36463647
}

‎cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowImpl2.qll

Lines changed: 29 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,8 @@ private class ParamNodeEx extends NodeEx {
244244
}
245245

246246
int getPosition() { this.isParameterOf(_, result) }
247+
248+
predicate allowParameterReturnInSelf() { allowParameterReturnInSelfCached(this.asNode()) }
247249
}
248250

249251
private class RetNodeEx extends NodeEx {
@@ -252,8 +254,6 @@ private class RetNodeEx extends NodeEx {
252254
ReturnPosition getReturnPosition() { result = getReturnPosition(this.asNode()) }
253255

254256
ReturnKindExt getKind() { result = this.asNode().(ReturnNodeExt).getKind() }
255-
256-
predicate allowFlowThroughParameter() { allowFlowThroughParameterCached(this.asNode()) }
257257
}
258258

259259
private predicate inBarrier(NodeEx node, Configuration config) {
@@ -727,16 +727,12 @@ private module Stage1 {
727727
/** Holds if flow may return from `callable`. */
728728
pragma[nomagic]
729729
private predicate returnFlowCallableNodeCand(
730-
DataFlowCallable callable, ReturnKindExt kind, boolean allowFlowThroughParameter,
731-
Configuration config
730+
DataFlowCallable callable, ReturnKindExt kind, Configuration config
732731
) {
733732
exists(RetNodeEx ret |
734733
throughFlowNodeCand(ret, config) and
735734
callable = ret.getEnclosingCallable() and
736-
kind = ret.getKind() and
737-
if ret.allowFlowThroughParameter()
738-
then allowFlowThroughParameter = true
739-
else allowFlowThroughParameter = false
735+
kind = ret.getKind()
740736
)
741737
}
742738

@@ -745,16 +741,17 @@ private module Stage1 {
745741
* candidate for the origin of a summary.
746742
*/
747743
predicate parameterMayFlowThrough(ParamNodeEx p, DataFlowCallable c, Ap ap, Configuration config) {
748-
exists(ReturnKindExt kind, boolean allowFlowThroughParameter |
744+
exists(ReturnKindExt kind |
749745
throughFlowNodeCand(p, config) and
750-
returnFlowCallableNodeCand(c, kind, allowFlowThroughParameter, config) and
746+
returnFlowCallableNodeCand(c, kind, config) and
751747
p.getEnclosingCallable() = c and
748+
exists(ap) and
749+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
752750
(
753-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
754-
then allowFlowThroughParameter = true
755-
else any()
756-
) and
757-
exists(ap)
751+
not kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
752+
or
753+
p.allowParameterReturnInSelf()
754+
)
758755
)
759756
}
760757

@@ -1403,10 +1400,11 @@ private module Stage2 {
14031400
fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
14041401
kind = ret.getKind() and
14051402
p.getPosition() = pos and
1403+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
14061404
(
1407-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
1408-
then ret.allowFlowThroughParameter()
1409-
else any()
1405+
not kind.(ParamUpdateReturnKind).getPosition() = pos
1406+
or
1407+
p.allowParameterReturnInSelf()
14101408
)
14111409
)
14121410
}
@@ -2095,10 +2093,11 @@ private module Stage3 {
20952093
fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
20962094
kind = ret.getKind() and
20972095
p.getPosition() = pos and
2096+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
20982097
(
2099-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
2100-
then ret.allowFlowThroughParameter()
2101-
else any()
2098+
not kind.(ParamUpdateReturnKind).getPosition() = pos
2099+
or
2100+
p.allowParameterReturnInSelf()
21022101
)
21032102
)
21042103
}
@@ -2858,10 +2857,11 @@ private module Stage4 {
28582857
fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
28592858
kind = ret.getKind() and
28602859
p.getPosition() = pos and
2860+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
28612861
(
2862-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
2863-
then ret.allowFlowThroughParameter()
2864-
else any()
2862+
not kind.(ParamUpdateReturnKind).getPosition() = pos
2863+
or
2864+
p.allowParameterReturnInSelf()
28652865
)
28662866
)
28672867
}
@@ -2935,7 +2935,7 @@ private class SummaryCtxSome extends SummaryCtx, TSummaryCtxSome {
29352935

29362936
int getParameterPos() { p.isParameterOf(_, result) }
29372937

2938-
ParameterNode getParameterNode() { result = p.asNode() }
2938+
ParamNodeEx getParamNode() { result = p }
29392939

29402940
override string toString() { result = p + ": " + ap }
29412941

@@ -3637,10 +3637,11 @@ private predicate paramFlowsThrough(
36373637
ap = mid.getAp() and
36383638
apa = ap.getApprox() and
36393639
pos = sc.getParameterPos() and
3640+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
36403641
(
3641-
if kind.(ParamUpdateReturnKind).getPosition() = pos
3642-
then ret.allowFlowThroughParameter()
3643-
else any()
3642+
not kind.(ParamUpdateReturnKind).getPosition() = pos
3643+
or
3644+
sc.getParamNode().allowParameterReturnInSelf()
36443645
)
36453646
)
36463647
}

‎cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowImpl3.qll

Lines changed: 29 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,8 @@ private class ParamNodeEx extends NodeEx {
244244
}
245245

246246
int getPosition() { this.isParameterOf(_, result) }
247+
248+
predicate allowParameterReturnInSelf() { allowParameterReturnInSelfCached(this.asNode()) }
247249
}
248250

249251
private class RetNodeEx extends NodeEx {
@@ -252,8 +254,6 @@ private class RetNodeEx extends NodeEx {
252254
ReturnPosition getReturnPosition() { result = getReturnPosition(this.asNode()) }
253255

254256
ReturnKindExt getKind() { result = this.asNode().(ReturnNodeExt).getKind() }
255-
256-
predicate allowFlowThroughParameter() { allowFlowThroughParameterCached(this.asNode()) }
257257
}
258258

259259
private predicate inBarrier(NodeEx node, Configuration config) {
@@ -727,16 +727,12 @@ private module Stage1 {
727727
/** Holds if flow may return from `callable`. */
728728
pragma[nomagic]
729729
private predicate returnFlowCallableNodeCand(
730-
DataFlowCallable callable, ReturnKindExt kind, boolean allowFlowThroughParameter,
731-
Configuration config
730+
DataFlowCallable callable, ReturnKindExt kind, Configuration config
732731
) {
733732
exists(RetNodeEx ret |
734733
throughFlowNodeCand(ret, config) and
735734
callable = ret.getEnclosingCallable() and
736-
kind = ret.getKind() and
737-
if ret.allowFlowThroughParameter()
738-
then allowFlowThroughParameter = true
739-
else allowFlowThroughParameter = false
735+
kind = ret.getKind()
740736
)
741737
}
742738

@@ -745,16 +741,17 @@ private module Stage1 {
745741
* candidate for the origin of a summary.
746742
*/
747743
predicate parameterMayFlowThrough(ParamNodeEx p, DataFlowCallable c, Ap ap, Configuration config) {
748-
exists(ReturnKindExt kind, boolean allowFlowThroughParameter |
744+
exists(ReturnKindExt kind |
749745
throughFlowNodeCand(p, config) and
750-
returnFlowCallableNodeCand(c, kind, allowFlowThroughParameter, config) and
746+
returnFlowCallableNodeCand(c, kind, config) and
751747
p.getEnclosingCallable() = c and
748+
exists(ap) and
749+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
752750
(
753-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
754-
then allowFlowThroughParameter = true
755-
else any()
756-
) and
757-
exists(ap)
751+
not kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
752+
or
753+
p.allowParameterReturnInSelf()
754+
)
758755
)
759756
}
760757

@@ -1403,10 +1400,11 @@ private module Stage2 {
14031400
fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
14041401
kind = ret.getKind() and
14051402
p.getPosition() = pos and
1403+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
14061404
(
1407-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
1408-
then ret.allowFlowThroughParameter()
1409-
else any()
1405+
not kind.(ParamUpdateReturnKind).getPosition() = pos
1406+
or
1407+
p.allowParameterReturnInSelf()
14101408
)
14111409
)
14121410
}
@@ -2095,10 +2093,11 @@ private module Stage3 {
20952093
fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
20962094
kind = ret.getKind() and
20972095
p.getPosition() = pos and
2096+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
20982097
(
2099-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
2100-
then ret.allowFlowThroughParameter()
2101-
else any()
2098+
not kind.(ParamUpdateReturnKind).getPosition() = pos
2099+
or
2100+
p.allowParameterReturnInSelf()
21022101
)
21032102
)
21042103
}
@@ -2858,10 +2857,11 @@ private module Stage4 {
28582857
fwdFlow(ret, any(CcCall ccc), apSome(ap), ap0, config) and
28592858
kind = ret.getKind() and
28602859
p.getPosition() = pos and
2860+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
28612861
(
2862-
if kind.(ParamUpdateReturnKind).getPosition() = p.getPosition()
2863-
then ret.allowFlowThroughParameter()
2864-
else any()
2862+
not kind.(ParamUpdateReturnKind).getPosition() = pos
2863+
or
2864+
p.allowParameterReturnInSelf()
28652865
)
28662866
)
28672867
}
@@ -2935,7 +2935,7 @@ private class SummaryCtxSome extends SummaryCtx, TSummaryCtxSome {
29352935

29362936
int getParameterPos() { p.isParameterOf(_, result) }
29372937

2938-
ParameterNode getParameterNode() { result = p.asNode() }
2938+
ParamNodeEx getParamNode() { result = p }
29392939

29402940
override string toString() { result = p + ": " + ap }
29412941

@@ -3637,10 +3637,11 @@ private predicate paramFlowsThrough(
36373637
ap = mid.getAp() and
36383638
apa = ap.getApprox() and
36393639
pos = sc.getParameterPos() and
3640+
// we don't expect a parameter to return stored in itself, unless explicitly allowed
36403641
(
3641-
if kind.(ParamUpdateReturnKind).getPosition() = pos
3642-
then ret.allowFlowThroughParameter()
3643-
else any()
3642+
not kind.(ParamUpdateReturnKind).getPosition() = pos
3643+
or
3644+
sc.getParamNode().allowParameterReturnInSelf()
36443645
)
36453646
)
36463647
}

0 commit comments

Comments
 (0)