Skip to content

Commit 2aace0d

Browse files
MathiasVPMathiasVP
authored
Merge pull request #11743 from MathiasVP/fix-pointer-to-const-nodes
C++: `PostUpdateNode`s for const-pointer arguments
2 parents 8b01dfe + cdd9567 commit 2aace0d

8 files changed

Lines changed: 98 additions & 19 deletions

File tree

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,7 @@ private newtype TIRDataFlowNode =
3939
} or
4040
TSsaPhiNode(Ssa::PhiNode phi) or
4141
TIndirectArgumentOutNode(ArgumentOperand operand, int indirectionIndex) {
42-
Ssa::isModifiableByCall(operand) and
43-
indirectionIndex = [1 .. Ssa::countIndirectionsForCppType(operand.getLanguageType())]
42+
Ssa::isModifiableByCall(operand, indirectionIndex)
4443
} or
4544
TRawIndirectOperand(Operand op, int indirectionIndex) {
4645
Ssa::hasRawIndirectOperand(op, indirectionIndex)
@@ -374,11 +373,13 @@ class OperandNode extends Node, Node0 {
374373
}
375374

376375
/**
376+
* INTERNAL: Do not use.
377+
*
377378
* Returns `t`, but stripped of the outermost pointer, reference, etc.
378379
*
379380
* For example, `stripPointers(int*&)` is `int*` and `stripPointers(int*)` is `int`.
380381
*/
381-
private Type stripPointer(Type t) {
382+
Type stripPointer(Type t) {
382383
result = any(Ssa::Indirection ind | ind.getType() = t).getBaseType()
383384
or
384385
// These types have a sensible base type, but don't receive additional
@@ -665,19 +666,24 @@ private class PostIndirectReturnOutNode extends IndirectReturnOutNode, PostUpdat
665666
override Node getPreUpdateNode() { result = this }
666667
}
667668

668-
private Type getTypeImpl(Type t, int indirectionIndex) {
669+
/**
670+
* INTERNAL: Do not use.
671+
*
672+
* Returns `t`, but stripped of the outer-most `indirectionIndex` number of indirections.
673+
*/
674+
Type getTypeImpl(Type t, int indirectionIndex) {
669675
indirectionIndex = 0 and
670676
result = t
671677
or
672678
indirectionIndex > 0 and
673679
exists(Type stripped |
674-
stripped = stripPointer(t) and
680+
stripped = stripPointer(t.stripTopLevelSpecifiers()) and
675681
// We need to avoid the case where `stripPointer(t) = t` (which can happen on
676682
// iterators that specify a `value_type` that is the iterator itself). Such a type
677683
// would create an infinite loop otherwise. For these cases we simply don't produce
678684
// a result for `getType`.
679685
stripped.getUnspecifiedType() != t.getUnspecifiedType() and
680-
result = getTypeImpl(stripPointer(t), indirectionIndex - 1)
686+
result = getTypeImpl(stripped, indirectionIndex - 1)
681687
)
682688
}
683689

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll‎

Lines changed: 55 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import semmle.code.cpp.ir.internal.IRCppLanguage
44
private import semmle.code.cpp.ir.implementation.raw.internal.SideEffects as SideEffects
55
private import DataFlowImplCommon as DataFlowImplCommon
66
private import DataFlowUtil
7+
private import semmle.code.cpp.models.interfaces.PointerWrapper
78
private import DataFlowPrivate
89

910
/**
@@ -137,7 +138,7 @@ abstract class Indirection extends Type {
137138
Type baseType;
138139

139140
/** Gets the type of this indirection. */
140-
final Type getType() { result = super.getUnspecifiedType() }
141+
final Type getType() { result = this }
141142

142143
/**
143144
* Gets the number of indirections supported by this type.
@@ -166,7 +167,7 @@ abstract class Indirection extends Type {
166167
*
167168
* For example, the base type of `int*&` is `int*`, and the base type of `int*` is `int`.
168169
*/
169-
final Type getBaseType() { result = baseType.getUnspecifiedType() }
170+
final Type getBaseType() { result = baseType }
170171

171172
/** Holds if there should be an additional taint step from `node1` to `node2`. */
172173
predicate isAdditionalTaintStep(Node node1, Node node2) { none() }
@@ -181,7 +182,9 @@ abstract class Indirection extends Type {
181182
private class PointerOrReferenceTypeIndirection extends Indirection instanceof PointerOrReferenceType {
182183
PointerOrReferenceTypeIndirection() { baseType = PointerOrReferenceType.super.getBaseType() }
183184

184-
override int getNumberOfIndirections() { result = 1 + countIndirections(this.getBaseType()) }
185+
override int getNumberOfIndirections() {
186+
result = 1 + countIndirections(this.getBaseType().getUnspecifiedType())
187+
}
185188

186189
override predicate isAdditionalDereference(Instruction deref, Operand address) { none() }
187190

@@ -198,7 +201,9 @@ private module IteratorIndirections {
198201
not this instanceof PointerOrReferenceTypeIndirection and baseType = super.getValueType()
199202
}
200203

201-
override int getNumberOfIndirections() { result = 1 + countIndirections(this.getBaseType()) }
204+
override int getNumberOfIndirections() {
205+
result = 1 + countIndirections(this.getBaseType().getUnspecifiedType())
206+
}
202207

203208
override predicate isAdditionalDereference(Instruction deref, Operand address) {
204209
exists(CallInstruction call |
@@ -352,8 +357,9 @@ class BaseCallVariable extends BaseSourceVariable, TBaseCallVariable {
352357
* Holds if the value pointed to by `operand` can potentially be
353358
* modified be the caller.
354359
*/
355-
predicate isModifiableByCall(ArgumentOperand operand) {
360+
predicate isModifiableByCall(ArgumentOperand operand, int indirectionIndex) {
356361
exists(CallInstruction call, int index, CppType type |
362+
indirectionIndex = [1 .. countIndirectionsForCppType(type)] and
357363
type = getLanguageType(operand) and
358364
call.getArgumentOperand(index) = operand and
359365
if index = -1
@@ -385,13 +391,50 @@ predicate isModifiableByCall(ArgumentOperand operand) {
385391
else any()
386392
else
387393
// An argument is modifiable if it's a non-const pointer or reference type.
388-
exists(Type t, boolean isGLValue | type.hasType(t, isGLValue) |
389-
// If t is a glvalue it means that t is always a pointer-like type.
390-
isGLValue = true
391-
or
392-
t instanceof PointerOrReferenceType and
393-
not SideEffects::isConstPointerLike(t)
394-
)
394+
isModifiableAt(type, indirectionIndex)
395+
)
396+
}
397+
398+
/**
399+
* Holds if `t` is a pointer or reference type that supports at least `indirectionIndex` number
400+
* of indirections, and the `indirectionIndex` indirection cannot be modfiied by passing a
401+
* value of `t` to a function.
402+
*/
403+
private predicate isModifiableAtImpl(CppType cppType, int indirectionIndex) {
404+
indirectionIndex = [1 .. countIndirectionsForCppType(cppType)] and
405+
(
406+
exists(PointerOrReferenceType pointerType, Type base, Type t |
407+
cppType.hasType(t, _) and
408+
pointerType = t.getUnderlyingType() and
409+
base = getTypeImpl(pointerType, indirectionIndex)
410+
|
411+
// The value cannot be modified if it has a const specifier,
412+
not base.isConst()
413+
or
414+
// but in the case of a class type, it may be the case that
415+
// one of the members was modified.
416+
exists(base.stripType().(Cpp::Class).getAField())
417+
)
418+
or
419+
// If the `indirectionIndex`'th dereference of a type can be modified
420+
// then so can the `indirectionIndex + 1`'th dereference.
421+
isModifiableAtImpl(cppType, indirectionIndex - 1)
422+
)
423+
}
424+
425+
/**
426+
* Holds if `t` is a type with at least `indirectionIndex` number of indirections,
427+
* and the `indirectionIndex` indirection can be modified by passing a value of
428+
* type `t` to a function function.
429+
*/
430+
bindingset[indirectionIndex]
431+
private predicate isModifiableAt(CppType cppType, int indirectionIndex) {
432+
isModifiableAtImpl(cppType, indirectionIndex)
433+
or
434+
exists(PointerWrapper pw, Type t |
435+
cppType.hasType(t, _) and
436+
t.stripType() = pw and
437+
not pw.pointsToConst()
395438
)
396439
}
397440

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/AllocMultiplicationOverflow.expected‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ edges
66
| test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 |
77
| test.cpp:37:24:37:27 | size | test.cpp:37:46:37:49 | size |
88
| test.cpp:45:36:45:40 | ... * ... | test.cpp:37:24:37:27 | size |
9+
| test.cpp:45:36:45:40 | ... * ... | test.cpp:45:36:45:40 | ... * ... |
10+
| test.cpp:45:36:45:40 | ... * ... | test.cpp:45:36:45:40 | ... * ... |
11+
| test.cpp:46:36:46:40 | ... * ... | test.cpp:46:36:46:40 | ... * ... |
912
nodes
1013
| test.cpp:13:33:13:37 | ... * ... | semmle.label | ... * ... |
1114
| test.cpp:13:33:13:37 | ... * ... | semmle.label | ... * ... |
@@ -25,6 +28,10 @@ nodes
2528
| test.cpp:37:46:37:49 | size | semmle.label | size |
2629
| test.cpp:45:36:45:40 | ... * ... | semmle.label | ... * ... |
2730
| test.cpp:45:36:45:40 | ... * ... | semmle.label | ... * ... |
31+
| test.cpp:45:36:45:40 | ... * ... | semmle.label | ... * ... |
32+
| test.cpp:45:36:45:40 | ... * ... | semmle.label | ... * ... |
33+
| test.cpp:46:36:46:40 | ... * ... | semmle.label | ... * ... |
34+
| test.cpp:46:36:46:40 | ... * ... | semmle.label | ... * ... |
2835
| test.cpp:46:36:46:40 | ... * ... | semmle.label | ... * ... |
2936
subpaths
3037
#select
@@ -42,5 +49,10 @@ subpaths
4249
| test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:30:27:30:31 | ... * ... | multiplication |
4350
| test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:31:27:31:31 | ... * ... | multiplication |
4451
| test.cpp:37:46:37:49 | size | test.cpp:45:36:45:40 | ... * ... | test.cpp:37:46:37:49 | size | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:45:36:45:40 | ... * ... | multiplication |
52+
| test.cpp:37:46:37:49 | size | test.cpp:45:36:45:40 | ... * ... | test.cpp:37:46:37:49 | size | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:45:36:45:40 | ... * ... | multiplication |
4553
| test.cpp:45:36:45:40 | ... * ... | test.cpp:45:36:45:40 | ... * ... | test.cpp:45:36:45:40 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:45:36:45:40 | ... * ... | multiplication |
54+
| test.cpp:45:36:45:40 | ... * ... | test.cpp:45:36:45:40 | ... * ... | test.cpp:45:36:45:40 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:45:36:45:40 | ... * ... | multiplication |
55+
| test.cpp:45:36:45:40 | ... * ... | test.cpp:45:36:45:40 | ... * ... | test.cpp:45:36:45:40 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:45:36:45:40 | ... * ... | multiplication |
56+
| test.cpp:46:36:46:40 | ... * ... | test.cpp:46:36:46:40 | ... * ... | test.cpp:46:36:46:40 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:46:36:46:40 | ... * ... | multiplication |
57+
| test.cpp:46:36:46:40 | ... * ... | test.cpp:46:36:46:40 | ... * ... | test.cpp:46:36:46:40 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:46:36:46:40 | ... * ... | multiplication |
4658
| test.cpp:46:36:46:40 | ... * ... | test.cpp:46:36:46:40 | ... * ... | test.cpp:46:36:46:40 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:46:36:46:40 | ... * ... | multiplication |

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,9 @@ postWithInFlow
9696
| test.cpp:519:3:519:15 | access to array [post update] | PostUpdateNode should not be the target of local flow. |
9797
| test.cpp:520:3:520:12 | stackArray [inner post update] | PostUpdateNode should not be the target of local flow. |
9898
| test.cpp:520:3:520:15 | access to array [post update] | PostUpdateNode should not be the target of local flow. |
99+
| test.cpp:526:3:526:4 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
100+
| test.cpp:526:4:526:4 | e [inner post update] | PostUpdateNode should not be the target of local flow. |
101+
| test.cpp:531:40:531:40 | e [inner post update] | PostUpdateNode should not be the target of local flow. |
99102
viableImplInCallContextTooLarge
100103
uniqueParameterNodeAtPosition
101104
uniqueParameterNodePosition

‎cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -519,4 +519,15 @@ void uncertain_definition() {
519519
stackArray[0] = source();
520520
stackArray[1] = clean;
521521
sink(stackArray[0]); // $ ast=519:19 ir SPURIOUS: ast=517:7
522+
}
523+
524+
void set_through_const_pointer(int x, const int **e)
525+
{
526+
*e = &x;
527+
}
528+
529+
void test_set_through_const_pointer(int *e)
530+
{
531+
set_through_const_pointer(source(), &e);
532+
sink(*e); // $ ir MISSING: ast
522533
}

‎cpp/ql/test/library-tests/dataflow/fields/partial-definition-diff.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,8 @@
153153
| by_reference.cpp:16:11:16:11 | a | AST only |
154154
| by_reference.cpp:32:15:32:15 | s | IR only |
155155
| by_reference.cpp:36:18:36:18 | this | IR only |
156+
| by_reference.cpp:44:26:44:29 | this | IR only |
157+
| by_reference.cpp:69:22:69:23 | & ... | IR only |
156158
| by_reference.cpp:84:10:84:10 | a | AST only |
157159
| by_reference.cpp:88:9:88:9 | a | AST only |
158160
| by_reference.cpp:92:3:92:5 | * ... | AST only |

‎cpp/ql/test/library-tests/dataflow/fields/partial-definition-ir.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,7 @@
333333
| by_reference.cpp:32:12:32:12 | s |
334334
| by_reference.cpp:36:12:36:15 | this |
335335
| by_reference.cpp:40:12:40:15 | this |
336+
| by_reference.cpp:44:26:44:29 | this |
336337
| by_reference.cpp:50:3:50:3 | s |
337338
| by_reference.cpp:50:17:50:26 | call to user_input |
338339
| by_reference.cpp:51:8:51:8 | s |
@@ -348,6 +349,7 @@
348349
| by_reference.cpp:68:17:68:18 | & ... |
349350
| by_reference.cpp:68:21:68:30 | call to user_input |
350351
| by_reference.cpp:69:8:69:20 | call to nonMemberGetA |
352+
| by_reference.cpp:69:22:69:23 | & ... |
351353
| by_reference.cpp:84:3:84:7 | inner |
352354
| by_reference.cpp:88:3:88:7 | inner |
353355
| by_reference.cpp:102:21:102:39 | & ... |

‎cpp/ql/test/library-tests/dataflow/source-sink-tests/sources-and-sinks.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ int readv(int, const struct iovec*, int);
2323
int writev(int, const struct iovec*, int);
2424

2525
void test_readv_and_writev(iovec* iovs) {
26-
readv(0, iovs, 16); // $ MISSING: remote_source
26+
readv(0, iovs, 16); // $ remote_source
2727
writev(0, iovs, 16); // $ remote_sink
2828
}
2929

0 commit comments

Comments
 (0)