Skip to content

Commit 74ed9f5

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#13406 from MathiasVP/fix-++-problem
C++: Fix the `++` problem
2 parents 0cef565 + a357eee commit 74ed9f5

File tree

6 files changed

+76
-13
lines changed

6 files changed

+76
-13
lines changed

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

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,25 @@ abstract private class OperandBasedUse extends UseImpl {
364364
OperandBasedUse() { any() }
365365

366366
final override predicate hasIndexInBlock(IRBlock block, int index) {
367-
operand.getUse() = block.getInstruction(index)
367+
// See the comment in `ssa0`'s `OperandBasedUse` for an explanation of this
368+
// predicate's implementation.
369+
exists(BaseSourceVariableInstruction base | base = this.getBase() |
370+
if base.getAst() = any(Cpp::PostfixCrementOperation c).getOperand()
371+
then
372+
exists(Operand op, int indirectionIndex, int indirection |
373+
indirectionIndex = this.getIndirectionIndex() and
374+
indirection = this.getIndirection() and
375+
op =
376+
min(Operand cand, int i |
377+
isUse(_, cand, base, indirection, indirectionIndex) and
378+
block.getInstruction(i) = cand.getUse()
379+
|
380+
cand order by i
381+
) and
382+
block.getInstruction(index) = op.getUse()
383+
)
384+
else operand.getUse() = block.getInstruction(index)
385+
)
368386
}
369387

370388
final Operand getOperand() { result = operand }

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

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,46 @@ abstract private class OperandBasedUse extends UseImpl {
122122
override string toString() { result = operand.toString() }
123123

124124
final override predicate hasIndexInBlock(IRBlock block, int index) {
125-
operand.getUse() = block.getInstruction(index)
125+
// Ideally, this would just be implemented as:
126+
// ```
127+
// operand.getUse() = block.getInstruction(index)
128+
// ```
129+
// but because the IR generated for a snippet such as
130+
// ```
131+
// int x = *p++;
132+
// ```
133+
// looks like
134+
// ```
135+
// r1(glval<int>) = VariableAddress[x] :
136+
// r2(glval<int *>) = VariableAddress[p] :
137+
// r3(int *) = Load[p] : &:r2, m1
138+
// r4(int) = Constant[1] :
139+
// r5(int *) = PointerAdd[4] : r3, r4
140+
// m3(int *) = Store[p] : &:r2, r5
141+
// r6(int *) = CopyValue : r3
142+
// r7(int) = Load[?] : &:r6, ~m2
143+
// m2(int) = Store[x] : &:r1, r7
144+
// ```
145+
// we need to ensure that the `r3` operand of the `CopyValue` instruction isn't seen as a fresh use
146+
// of `p` that happens after the increment. So if the base instruction of this use comes from a
147+
// post-fix crement operation we set the index of the SSA use that wraps the `r3` operand at the
148+
// `CopyValue` instruction to be the same index as the `r3` operand at the `PointerAdd` instruction.
149+
// This ensures that the SSA library doesn't create flow from the `PointerAdd` to `r6`.
150+
exists(BaseSourceVariableInstruction base | base = this.getBase() |
151+
if base.getAst() = any(Cpp::PostfixCrementOperation c).getOperand()
152+
then
153+
exists(Operand op |
154+
op =
155+
min(Operand cand, int i |
156+
isUse(_, cand, base, _, _) and
157+
block.getInstruction(i) = cand.getUse()
158+
|
159+
cand order by i
160+
) and
161+
block.getInstruction(index) = op.getUse()
162+
)
163+
else operand.getUse() = block.getInstruction(index)
164+
)
126165
}
127166

128167
final override Cpp::Location getLocation() { result = operand.getLocation() }

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -664,11 +664,6 @@ edges
664664
| test.cpp:338:8:338:15 | * ... | test.cpp:342:8:342:17 | * ... |
665665
| test.cpp:341:8:341:17 | * ... | test.cpp:342:8:342:17 | * ... |
666666
| test.cpp:347:14:347:27 | new[] | test.cpp:348:15:348:16 | xs |
667-
| test.cpp:348:15:348:16 | xs | test.cpp:350:16:350:19 | ... ++ |
668-
| test.cpp:348:15:348:16 | xs | test.cpp:350:16:350:19 | ... ++ |
669-
| test.cpp:350:16:350:19 | ... ++ | test.cpp:350:15:350:19 | Load: * ... |
670-
| test.cpp:350:16:350:19 | ... ++ | test.cpp:350:16:350:19 | ... ++ |
671-
| test.cpp:350:16:350:19 | ... ++ | test.cpp:350:16:350:19 | ... ++ |
672667
| test.cpp:355:14:355:27 | new[] | test.cpp:356:15:356:16 | xs |
673668
| test.cpp:356:15:356:16 | xs | test.cpp:356:15:356:23 | ... + ... |
674669
| test.cpp:356:15:356:16 | xs | test.cpp:356:15:356:23 | ... + ... |
@@ -1057,10 +1052,6 @@ nodes
10571052
| test.cpp:342:8:342:17 | * ... | semmle.label | * ... |
10581053
| test.cpp:347:14:347:27 | new[] | semmle.label | new[] |
10591054
| test.cpp:348:15:348:16 | xs | semmle.label | xs |
1060-
| test.cpp:350:15:350:19 | Load: * ... | semmle.label | Load: * ... |
1061-
| test.cpp:350:16:350:19 | ... ++ | semmle.label | ... ++ |
1062-
| test.cpp:350:16:350:19 | ... ++ | semmle.label | ... ++ |
1063-
| test.cpp:350:16:350:19 | ... ++ | semmle.label | ... ++ |
10641055
| test.cpp:355:14:355:27 | new[] | semmle.label | new[] |
10651056
| test.cpp:356:15:356:16 | xs | semmle.label | xs |
10661057
| test.cpp:356:15:356:23 | ... + ... | semmle.label | ... + ... |
@@ -1118,7 +1109,6 @@ subpaths
11181109
| test.cpp:264:13:264:14 | Load: * ... | test.cpp:260:13:260:24 | new[] | test.cpp:264:13:264:14 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:260:13:260:24 | new[] | new[] | test.cpp:261:19:261:21 | len | len |
11191110
| test.cpp:274:5:274:10 | Store: ... = ... | test.cpp:270:13:270:24 | new[] | test.cpp:274:5:274:10 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:270:13:270:24 | new[] | new[] | test.cpp:271:19:271:21 | len | len |
11201111
| test.cpp:308:5:308:29 | Store: ... = ... | test.cpp:304:15:304:26 | new[] | test.cpp:308:5:308:29 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:304:15:304:26 | new[] | new[] | test.cpp:308:8:308:10 | ... + ... | ... + ... |
1121-
| test.cpp:350:15:350:19 | Load: * ... | test.cpp:347:14:347:27 | new[] | test.cpp:350:15:350:19 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:347:14:347:27 | new[] | new[] | test.cpp:348:20:348:23 | size | size |
11221112
| test.cpp:358:14:358:26 | Load: * ... | test.cpp:355:14:355:27 | new[] | test.cpp:358:14:358:26 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@ + 1. | test.cpp:355:14:355:27 | new[] | new[] | test.cpp:356:20:356:23 | size | size |
11231113
| test.cpp:359:14:359:32 | Load: * ... | test.cpp:355:14:355:27 | new[] | test.cpp:359:14:359:32 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@ + 2. | test.cpp:355:14:355:27 | new[] | new[] | test.cpp:356:20:356:23 | size | size |
11241114
| test.cpp:372:15:372:16 | Load: * ... | test.cpp:363:14:363:27 | new[] | test.cpp:372:15:372:16 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:363:14:363:27 | new[] | new[] | test.cpp:365:19:365:22 | size | size |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ void test24(unsigned size) {
347347
char *xs = new char[size];
348348
char *end = xs + size;
349349
if (xs < end) {
350-
int val = *xs++; // GOOD [FALSE POSITIVE]
350+
int val = *xs++; // GOOD
351351
}
352352
}
353353

‎cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6584,6 +6584,13 @@
65846584
| taint.cpp:691:18:691:18 | s [post update] | taint.cpp:695:7:695:7 | s | |
65856585
| taint.cpp:691:20:691:20 | ref arg x | taint.cpp:694:9:694:9 | x | |
65866586
| taint.cpp:694:7:694:7 | s [post update] | taint.cpp:695:7:695:7 | s | |
6587+
| taint.cpp:700:13:700:18 | call to source | taint.cpp:702:11:702:11 | s | |
6588+
| taint.cpp:701:9:701:9 | p | taint.cpp:702:4:702:4 | p | |
6589+
| taint.cpp:702:4:702:4 | p | taint.cpp:702:4:702:6 | ... ++ | |
6590+
| taint.cpp:702:4:702:6 | ... ++ | taint.cpp:702:3:702:6 | * ... | TAINT |
6591+
| taint.cpp:702:4:702:6 | ... ++ | taint.cpp:703:8:703:8 | p | TAINT |
6592+
| taint.cpp:702:10:702:11 | * ... | taint.cpp:702:3:702:11 | ... = ... | |
6593+
| taint.cpp:702:11:702:11 | s | taint.cpp:702:10:702:11 | * ... | TAINT |
65876594
| vector.cpp:16:43:16:49 | source1 | vector.cpp:17:26:17:32 | source1 | |
65886595
| vector.cpp:16:43:16:49 | source1 | vector.cpp:31:38:31:44 | source1 | |
65896596
| vector.cpp:17:21:17:33 | call to vector | vector.cpp:19:14:19:14 | v | |

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -693,4 +693,13 @@ void test_argument_source_field_to_obj() {
693693
sink(s); // $ SPURIOUS: ast,ir
694694
sink(s.x); // $ ast,ir
695695
sink(s.y); // clean
696+
}
697+
698+
namespace strings {
699+
void test_write_to_read_then_incr_then_deref() {
700+
char* s = source();
701+
char* p;
702+
*p++ = *s;
703+
sink(p); // $ ast ir
704+
}
696705
}

0 commit comments

Comments
 (0)