hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-24 04:36:35 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #13406 from MathiasVP/fix-++-problem

Browse Source 
C++: Fix the `++` problem
This commit is contained in:
Mathias Vorreiter Pedersen
2023-06-09 11:20:00 +01:00
committed by GitHub
parent 0cef5651e2 a357eeedac
commit 74ed9f535c
6 changed files with 76 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll
View File

@@ -364,7 +364,25 @@ abstract private class OperandBasedUse extends UseImpl {
OperandBasedUse() { any() }
final override predicate hasIndexInBlock(IRBlock block, int index) {
operand.getUse() = block.getInstruction(index)
// See the comment in `ssa0`'s `OperandBasedUse` for an explanation of this
// predicate's implementation.
exists(BaseSourceVariableInstruction base | base = this.getBase() |
if base.getAst() = any(Cpp::PostfixCrementOperation c).getOperand()
then
exists(Operand op, int indirectionIndex, int indirection |
indirectionIndex = this.getIndirectionIndex() and
indirection = this.getIndirection() and
op =
min(Operand cand, int i |
isUse(_, cand, base, indirection, indirectionIndex) and
block.getInstruction(i) = cand.getUse()
|
cand order by i
) and
block.getInstruction(index) = op.getUse()
)
else operand.getUse() = block.getInstruction(index)
)
}
final Operand getOperand() { result = operand }

41
cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ssa0/SsaInternals.qll
View File

@@ -122,7 +122,46 @@ abstract private class OperandBasedUse extends UseImpl {
override string toString() { result = operand.toString() }
final override predicate hasIndexInBlock(IRBlock block, int index) {
operand.getUse() = block.getInstruction(index)
// Ideally, this would just be implemented as:
// ```
// operand.getUse() = block.getInstruction(index)
// ```
// but because the IR generated for a snippet such as
// ```
// int x = *p++;
// ```
// looks like
// ```
// r1(glval<int>) = VariableAddress[x] :
// r2(glval<int *>) = VariableAddress[p] :
// r3(int *) = Load[p] : &:r2, m1
// r4(int) = Constant[1] :
// r5(int *) = PointerAdd[4] : r3, r4
// m3(int *) = Store[p] : &:r2, r5
// r6(int *) = CopyValue : r3
// r7(int) = Load[?] : &:r6, ~m2
// m2(int) = Store[x] : &:r1, r7
// ```
// we need to ensure that the `r3` operand of the `CopyValue` instruction isn't seen as a fresh use
// of `p` that happens after the increment. So if the base instruction of this use comes from a
// post-fix crement operation we set the index of the SSA use that wraps the `r3` operand at the
// `CopyValue` instruction to be the same index as the `r3` operand at the `PointerAdd` instruction.
// This ensures that the SSA library doesn't create flow from the `PointerAdd` to `r6`.
exists(BaseSourceVariableInstruction base | base = this.getBase() |
if base.getAst() = any(Cpp::PostfixCrementOperation c).getOperand()
then
exists(Operand op |
op =
min(Operand cand, int i |
isUse(_, cand, base, _, _) and
block.getInstruction(i) = cand.getUse()
|
cand order by i
) and
block.getInstruction(index) = op.getUse()
)
else operand.getUse() = block.getInstruction(index)
)
}
final override Cpp::Location getLocation() { result = operand.getLocation() }

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

@@ -664,11 +664,6 @@ edges
| test.cpp:338:8:338:15 | * ... | test.cpp:342:8:342:17 | * ... |
| test.cpp:341:8:341:17 | * ... | test.cpp:342:8:342:17 | * ... |
| test.cpp:347:14:347:27 | new[] | test.cpp:348:15:348:16 | xs |
| test.cpp:348:15:348:16 | xs | test.cpp:350:16:350:19 | ... ++ |
| test.cpp:348:15:348:16 | xs | test.cpp:350:16:350:19 | ... ++ |
| test.cpp:350:16:350:19 | ... ++ | test.cpp:350:15:350:19 | Load: * ... |
| test.cpp:350:16:350:19 | ... ++ | test.cpp:350:16:350:19 | ... ++ |
| test.cpp:350:16:350:19 | ... ++ | test.cpp:350:16:350:19 | ... ++ |
| test.cpp:355:14:355:27 | new[] | test.cpp:356:15:356:16 | xs |
| test.cpp:356:15:356:16 | xs | test.cpp:356:15:356:23 | ... + ... |
| test.cpp:356:15:356:16 | xs | test.cpp:356:15:356:23 | ... + ... |
@@ -1057,10 +1052,6 @@ nodes
| test.cpp:342:8:342:17 | * ... | semmle.label | * ... |
| test.cpp:347:14:347:27 | new[] | semmle.label | new[] |
| test.cpp:348:15:348:16 | xs | semmle.label | xs |
| test.cpp:350:15:350:19 | Load: * ... | semmle.label | Load: * ... |
| test.cpp:350:16:350:19 | ... ++ | semmle.label | ... ++ |
| test.cpp:350:16:350:19 | ... ++ | semmle.label | ... ++ |
| test.cpp:350:16:350:19 | ... ++ | semmle.label | ... ++ |
| test.cpp:355:14:355:27 | new[] | semmle.label | new[] |
| test.cpp:356:15:356:16 | xs | semmle.label | xs |
| test.cpp:356:15:356:23 | ... + ... | semmle.label | ... + ... |
@@ -1118,7 +1109,6 @@ subpaths
| 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 |
| 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 |
| 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 | ... + ... | ... + ... |
| 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 |
| 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 |
| 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 |
| 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 |

2
cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp
View File

@@ -347,7 +347,7 @@ void test24(unsigned size) {
char *xs = new char[size];
char *end = xs + size;
if (xs < end) {
int val = *xs++; // GOOD [FALSE POSITIVE]
int val = *xs++; // GOOD
}
}

7
cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
View File

@@ -6584,6 +6584,13 @@
| taint.cpp:691:18:691:18 | s [post update] | taint.cpp:695:7:695:7 | s | |
| taint.cpp:691:20:691:20 | ref arg x | taint.cpp:694:9:694:9 | x | |
| taint.cpp:694:7:694:7 | s [post update] | taint.cpp:695:7:695:7 | s | |
| taint.cpp:700:13:700:18 | call to source | taint.cpp:702:11:702:11 | s | |
| taint.cpp:701:9:701:9 | p | taint.cpp:702:4:702:4 | p | |
| taint.cpp:702:4:702:4 | p | taint.cpp:702:4:702:6 | ... ++ | |
| taint.cpp:702:4:702:6 | ... ++ | taint.cpp:702:3:702:6 | * ... | TAINT |
| taint.cpp:702:4:702:6 | ... ++ | taint.cpp:703:8:703:8 | p | TAINT |
| taint.cpp:702:10:702:11 | * ... | taint.cpp:702:3:702:11 | ... = ... | |
| taint.cpp:702:11:702:11 | s | taint.cpp:702:10:702:11 | * ... | TAINT |
| vector.cpp:16:43:16:49 | source1 | vector.cpp:17:26:17:32 | source1 | |
| vector.cpp:16:43:16:49 | source1 | vector.cpp:31:38:31:44 | source1 | |
| vector.cpp:17:21:17:33 | call to vector | vector.cpp:19:14:19:14 | v | |

9
cpp/ql/test/library-tests/dataflow/taint-tests/taint.cpp
View File

@@ -693,4 +693,13 @@ void test_argument_source_field_to_obj() {
sink(s); // $ SPURIOUS: ast,ir
sink(s.x); // $ ast,ir
sink(s.y); // clean
}
namespace strings {
void test_write_to_read_then_incr_then_deref() {
char* s = source();
char* p;
*p++ = *s;
sink(p); // $ ast ir
}
}