Skip to content

Commit a04830b

Browse files
MathiasVPMathiasVP
authored
Merge pull request #14697 from MathiasVP/range-analysis-simplify-conversions
C++: Simplify the definition of `SemExpr` for range analysis
2 parents b2512eb + 4455ed9 commit a04830b

File tree

7 files changed

+38
-113
lines changed

7 files changed

+38
-113
lines changed

‎cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/RangeAnalysis.qll

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,7 @@ private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
1717
* `upper` is true, and can be traced back to a guard represented by `reason`.
1818
*/
1919
predicate bounded(Expr e, Bound b, float delta, boolean upper, Reason reason) {
20-
exists(SemanticExprConfig::Expr semExpr |
21-
semExpr.getUnconverted().getUnconvertedResultExpression() = e
22-
|
20+
exists(SemanticExprConfig::Expr semExpr | semExpr.getUnconvertedResultExpression() = e |
2321
semBounded(semExpr, b, delta, upper, reason)
2422
)
2523
}
@@ -30,9 +28,7 @@ predicate bounded(Expr e, Bound b, float delta, boolean upper, Reason reason) {
3028
* The `Expr` may be a conversion.
3129
*/
3230
predicate convertedBounded(Expr e, Bound b, float delta, boolean upper, Reason reason) {
33-
exists(SemanticExprConfig::Expr semExpr |
34-
semExpr.getConverted().getConvertedResultExpression() = e
35-
|
31+
exists(SemanticExprConfig::Expr semExpr | semExpr.getConvertedResultExpression() = e |
3632
semBounded(semExpr, b, delta, upper, reason)
3733
)
3834
}

‎cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/SimpleRangeAnalysis.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ predicate exprMightOverflowNegatively(Expr expr) {
100100
lowerBound(expr) < exprMinVal(expr)
101101
or
102102
exists(SemanticExprConfig::Expr semExpr |
103-
semExpr.getUnconverted().getAst() = expr and
103+
semExpr.getAst() = expr and
104104
ConstantStage::potentiallyOverflowingExpr(false, semExpr) and
105105
not ConstantStage::initialBounded(semExpr, _, _, false, _, _, _)
106106
)
@@ -126,7 +126,7 @@ predicate exprMightOverflowPositively(Expr expr) {
126126
upperBound(expr) > exprMaxVal(expr)
127127
or
128128
exists(SemanticExprConfig::Expr semExpr |
129-
semExpr.getUnconverted().getAst() = expr and
129+
semExpr.getAst() = expr and
130130
ConstantStage::potentiallyOverflowingExpr(true, semExpr) and
131131
not ConstantStage::initialBounded(semExpr, _, _, true, _, _, _)
132132
)

‎cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticExpr.qll

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
private import Semantic
66
private import SemanticExprSpecific::SemanticExprConfig as Specific
7+
private import SemanticType
78

89
/**
910
* An language-neutral expression.
@@ -241,8 +242,21 @@ class SemConvertExpr extends SemUnaryExpr {
241242
SemConvertExpr() { opcode instanceof Opcode::Convert }
242243
}
243244

245+
private import semmle.code.cpp.ir.IR as IR
246+
247+
/** A conversion instruction which is guaranteed to not overflow. */
248+
private class SafeConversion extends IR::ConvertInstruction {
249+
SafeConversion() {
250+
exists(SemType tFrom, SemType tTo |
251+
tFrom = getSemanticType(super.getUnary().getResultIRType()) and
252+
tTo = getSemanticType(super.getResultIRType()) and
253+
conversionCannotOverflow(tFrom, tTo)
254+
)
255+
}
256+
}
257+
244258
class SemCopyValueExpr extends SemUnaryExpr {
245-
SemCopyValueExpr() { opcode instanceof Opcode::CopyValue }
259+
SemCopyValueExpr() { opcode instanceof Opcode::CopyValue or this instanceof SafeConversion }
246260
}
247261

248262
class SemNegateExpr extends SemUnaryExpr {

‎cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticExprSpecific.qll

Lines changed: 10 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -12,87 +12,10 @@ private import semmle.code.cpp.ir.ValueNumbering
1212
module SemanticExprConfig {
1313
class Location = Cpp::Location;
1414

15-
/** A `ConvertInstruction` or a `CopyValueInstruction`. */
16-
private class Conversion extends IR::UnaryInstruction {
17-
Conversion() {
18-
this instanceof IR::CopyValueInstruction
19-
or
20-
this instanceof IR::ConvertInstruction
21-
}
22-
23-
/** Holds if this instruction converts a value of type `tFrom` to a value of type `tTo`. */
24-
predicate converts(SemType tFrom, SemType tTo) {
25-
tFrom = getSemanticType(this.getUnary().getResultIRType()) and
26-
tTo = getSemanticType(this.getResultIRType())
27-
}
28-
}
29-
30-
/**
31-
* Gets a conversion-like instruction that consumes `op`, and
32-
* which is guaranteed to not overflow.
33-
*/
34-
private IR::Instruction safeConversion(IR::Operand op) {
35-
exists(Conversion conv, SemType tFrom, SemType tTo |
36-
conv.converts(tFrom, tTo) and
37-
conversionCannotOverflow(tFrom, tTo) and
38-
conv.getUnaryOperand() = op and
39-
result = conv
40-
)
41-
}
42-
43-
/** Holds if `i1 = i2` or if `i2` is a safe conversion that consumes `i1`. */
44-
private predicate idOrSafeConversion(IR::Instruction i1, IR::Instruction i2) {
45-
not i1.getResultIRType() instanceof IR::IRVoidType and
46-
(
47-
i1 = i2
48-
or
49-
i2 = safeConversion(i1.getAUse()) and
50-
i1.getBlock() = i2.getBlock()
51-
)
52-
}
53-
54-
module Equiv = QlBuiltins::EquivalenceRelation<IR::Instruction, idOrSafeConversion/2>;
55-
5615
/**
5716
* The expressions on which we perform range analysis.
5817
*/
59-
class Expr extends Equiv::EquivalenceClass {
60-
/** Gets the n'th instruction in this equivalence class. */
61-
private IR::Instruction getInstruction(int n) {
62-
result =
63-
rank[n + 1](IR::Instruction instr, int i, IR::IRBlock block |
64-
this = Equiv::getEquivalenceClass(instr) and block.getInstruction(i) = instr
65-
|
66-
instr order by i
67-
)
68-
}
69-
70-
/** Gets a textual representation of this element. */
71-
string toString() { result = this.getUnconverted().toString() }
72-
73-
/** Gets the basic block of this expression. */
74-
IR::IRBlock getBlock() { result = this.getUnconverted().getBlock() }
75-
76-
/** Gets the unconverted instruction associated with this expression. */
77-
IR::Instruction getUnconverted() { result = this.getInstruction(0) }
78-
79-
/**
80-
* Gets the final instruction associated with this expression. This
81-
* represents the result after applying all the safe conversions.
82-
*/
83-
IR::Instruction getConverted() {
84-
exists(int n |
85-
result = this.getInstruction(n) and
86-
not exists(this.getInstruction(n + 1))
87-
)
88-
}
89-
90-
/** Gets the type of the result produced by this instruction. */
91-
IR::IRType getResultIRType() { result = this.getConverted().getResultIRType() }
92-
93-
/** Gets the location of the source code for this expression. */
94-
Location getLocation() { result = this.getUnconverted().getLocation() }
95-
}
18+
class Expr = IR::Instruction;
9619

9720
SemBasicBlock getExprBasicBlock(Expr e) { result = getSemanticBasicBlock(e.getBlock()) }
9821

@@ -139,12 +62,12 @@ module SemanticExprConfig {
13962

14063
predicate stringLiteral(Expr expr, SemType type, string value) {
14164
anyConstantExpr(expr, type, value) and
142-
expr.getUnconverted() instanceof IR::StringConstantInstruction
65+
expr instanceof IR::StringConstantInstruction
14366
}
14467

14568
predicate binaryExpr(Expr expr, Opcode opcode, SemType type, Expr leftOperand, Expr rightOperand) {
14669
exists(IR::BinaryInstruction instr |
147-
instr = expr.getUnconverted() and
70+
instr = expr and
14871
type = getSemanticType(instr.getResultIRType()) and
14972
leftOperand = getSemanticExpr(instr.getLeft()) and
15073
rightOperand = getSemanticExpr(instr.getRight()) and
@@ -154,14 +77,14 @@ module SemanticExprConfig {
15477
}
15578

15679
predicate unaryExpr(Expr expr, Opcode opcode, SemType type, Expr operand) {
157-
exists(IR::UnaryInstruction instr | instr = expr.getUnconverted() |
80+
exists(IR::UnaryInstruction instr | instr = expr |
15881
type = getSemanticType(instr.getResultIRType()) and
15982
operand = getSemanticExpr(instr.getUnary()) and
16083
// REVIEW: Merge the two operand types.
16184
opcode.toString() = instr.getOpcode().toString()
16285
)
16386
or
164-
exists(IR::StoreInstruction instr | instr = expr.getUnconverted() |
87+
exists(IR::StoreInstruction instr | instr = expr |
16588
type = getSemanticType(instr.getResultIRType()) and
16689
operand = getSemanticExpr(instr.getSourceValue()) and
16790
opcode instanceof Opcode::Store
@@ -170,13 +93,13 @@ module SemanticExprConfig {
17093

17194
predicate nullaryExpr(Expr expr, Opcode opcode, SemType type) {
17295
exists(IR::LoadInstruction load |
173-
load = expr.getUnconverted() and
96+
load = expr and
17497
type = getSemanticType(load.getResultIRType()) and
17598
opcode instanceof Opcode::Load
17699
)
177100
or
178101
exists(IR::InitializeParameterInstruction init |
179-
init = expr.getUnconverted() and
102+
init = expr and
180103
type = getSemanticType(init.getResultIRType()) and
181104
opcode instanceof Opcode::InitializeParameter
182105
)
@@ -290,9 +213,9 @@ module SemanticExprConfig {
290213
}
291214

292215
Expr getAUse(SsaVariable v) {
293-
result.getUnconverted().(IR::LoadInstruction).getSourceValue() = v.asInstruction()
216+
result.(IR::LoadInstruction).getSourceValue() = v.asInstruction()
294217
or
295-
result.getUnconverted() = v.asPointerArithGuard().getAnInstruction()
218+
result = v.asPointerArithGuard().getAnInstruction()
296219
}
297220

298221
SemType getSsaVariableType(SsaVariable v) {
@@ -433,7 +356,7 @@ module SemanticExprConfig {
433356
}
434357

435358
/** Gets the expression associated with `instr`. */
436-
SemExpr getSemanticExpr(IR::Instruction instr) { result = Equiv::getEquivalenceClass(instr) }
359+
SemExpr getSemanticExpr(IR::Instruction instr) { result = instr }
437360
}
438361

439362
predicate getSemanticExpr = SemanticExprConfig::getSemanticExpr/1;

‎cpp/ql/test/library-tests/ir/range-analysis/SimpleRangeAnalysis_tests.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ int test2(struct List* p) {
1818
int count = 0;
1919
for (; p; p = p->next) {
2020
count = (count+1) % 10;
21-
range(count); // $ range=<=9 range=>=-9 range="<=Phi: p | Store: count+1"
21+
range(count); // $ range=<=9 range=>=-9
2222
}
2323
range(count); // $ range=>=-9 range=<=9
2424
return count;
@@ -29,7 +29,7 @@ int test3(struct List* p) {
2929
for (; p; p = p->next) {
3030
range(count++); // $ range=>=-9 range=<=9
3131
count = count % 10;
32-
range(count); // $ range=<=9 range=>=-9 range="<=Store: ... +++0" range="<=Phi: p | Store: count+1"
32+
range(count); // $ range=<=9 range=>=-9
3333
}
3434
range(count); // $ range=>=-9 range=<=9
3535
return count;
@@ -317,7 +317,7 @@ int test_mult01(int a, int b) {
317317
range(b); // $ range=<=23 range=>=-13
318318
int r = a*b; // $ overflow=+- -143 .. 253
319319
range(r);
320-
total += r; // $ overflow=+
320+
total += r; // $ overflow=+-
321321
range(total); // $ MISSING: range=">=... * ...+0"
322322
}
323323
if (3 <= a && a <= 11 && -13 <= b && b <= 0) {
@@ -365,7 +365,7 @@ int test_mult02(int a, int b) {
365365
range(b); // $ range=<=23 range=>=-13
366366
int r = a*b; // $ overflow=+- -143 .. 253
367367
range(r);
368-
total += r; // $ overflow=+
368+
total += r; // $ overflow=+-
369369
range(total); // $ MISSING: range=">=... * ...+0"
370370
}
371371
if (0 <= a && a <= 11 && -13 <= b && b <= 0) {
@@ -460,7 +460,7 @@ int test_mult04(int a, int b) {
460460
range(b); // $ range=<=23 range=>=-13
461461
int r = a*b; // $ overflow=+- -391 .. 221
462462
range(r);
463-
total += r; // $ overflow=-
463+
total += r; // $ overflow=+-
464464
range(total); // $ MISSING: range="<=... * ...+0"
465465
}
466466
if (-17 <= a && a <= 0 && -13 <= b && b <= 0) {
@@ -508,7 +508,7 @@ int test_mult05(int a, int b) {
508508
range(b); // $ range=<=23 range=>=-13
509509
int r = a*b; // $ overflow=+- -391 .. 221
510510
range(r);
511-
total += r; // $ overflow=-
511+
total += r; // $ overflow=+-
512512
range(total); // $ MISSING: range="<=... * ...+0"
513513
}
514514
if (-17 <= a && a <= -2 && -13 <= b && b <= 0) {
@@ -974,7 +974,7 @@ void test_mod_neg(int s) {
974974

975975
void test_mod_ternary(int s, bool b) {
976976
int s2 = s % (b ? 5 : 500);
977-
range(s2); // $ range=>=-499 range=<=499 range="<=Phi: ... ? ... : ...-1"
977+
range(s2); // $ range=>=-499 range=<=499
978978
}
979979

980980
void test_mod_ternary2(int s, bool b1, bool b2) {

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

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,6 @@ edges
3030
| test.cpp:206:17:206:23 | ... + ... | test.cpp:206:17:206:23 | ... + ... |
3131
| test.cpp:206:17:206:23 | ... + ... | test.cpp:213:5:213:13 | ... = ... |
3232
| test.cpp:206:17:206:23 | ... + ... | test.cpp:213:5:213:13 | ... = ... |
33-
| test.cpp:231:18:231:30 | new[] | test.cpp:232:3:232:20 | ... = ... |
34-
| test.cpp:238:20:238:32 | new[] | test.cpp:239:5:239:22 | ... = ... |
3533
| test.cpp:260:13:260:24 | new[] | test.cpp:261:14:261:21 | ... + ... |
3634
| test.cpp:261:14:261:21 | ... + ... | test.cpp:261:14:261:21 | ... + ... |
3735
| test.cpp:261:14:261:21 | ... + ... | test.cpp:264:13:264:14 | * ... |
@@ -135,10 +133,6 @@ nodes
135133
| test.cpp:206:17:206:23 | ... + ... | semmle.label | ... + ... |
136134
| test.cpp:206:17:206:23 | ... + ... | semmle.label | ... + ... |
137135
| test.cpp:213:5:213:13 | ... = ... | semmle.label | ... = ... |
138-
| test.cpp:231:18:231:30 | new[] | semmle.label | new[] |
139-
| test.cpp:232:3:232:20 | ... = ... | semmle.label | ... = ... |
140-
| test.cpp:238:20:238:32 | new[] | semmle.label | new[] |
141-
| test.cpp:239:5:239:22 | ... = ... | semmle.label | ... = ... |
142136
| test.cpp:260:13:260:24 | new[] | semmle.label | new[] |
143137
| test.cpp:261:14:261:21 | ... + ... | semmle.label | ... + ... |
144138
| test.cpp:261:14:261:21 | ... + ... | semmle.label | ... + ... |
@@ -222,8 +216,6 @@ subpaths
222216
| test.cpp:67:9:67:14 | ... = ... | test.cpp:52:19:52:37 | call to malloc | test.cpp:67:9:67:14 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:52:19:52:37 | call to malloc | call to malloc | test.cpp:53:20:53:23 | size | size |
223217
| test.cpp:201:5:201:19 | ... = ... | test.cpp:194:15:194:33 | call to malloc | test.cpp:201:5:201:19 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:194:15:194:33 | call to malloc | call to malloc | test.cpp:195:21:195:23 | len | len |
224218
| test.cpp:213:5:213:13 | ... = ... | test.cpp:205:15:205:33 | call to malloc | test.cpp:213:5:213:13 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:205:15:205:33 | call to malloc | call to malloc | test.cpp:206:21:206:23 | len | len |
225-
| test.cpp:232:3:232:20 | ... = ... | test.cpp:231:18:231:30 | new[] | test.cpp:232:3:232:20 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:231:18:231:30 | new[] | new[] | test.cpp:232:11:232:15 | index | index |
226-
| test.cpp:239:5:239:22 | ... = ... | test.cpp:238:20:238:32 | new[] | test.cpp:239:5:239:22 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:238:20:238:32 | new[] | new[] | test.cpp:239:13:239:17 | index | index |
227219
| test.cpp:264:13:264:14 | * ... | test.cpp:260:13:260:24 | new[] | test.cpp:264:13:264:14 | * ... | 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 |
228220
| test.cpp:274:5:274:10 | ... = ... | test.cpp:270:13:270:24 | new[] | test.cpp:274:5:274:10 | ... = ... | 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 |
229221
| test.cpp:358:14:358:26 | end_plus_one indirection | test.cpp:355:14:355:27 | new[] | test.cpp:358:14:358:26 | end_plus_one indirection | 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 |

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,14 +229,14 @@ void test15(unsigned index) {
229229
return;
230230
}
231231
int* newname = new int[size];
232-
newname[index] = 0; // $ alloc=L231 deref=L232 // GOOD [FALSE POSITIVE]
232+
newname[index] = 0; // GOOD
233233
}
234234

235235
void test16(unsigned index) {
236236
unsigned size = index + 13;
237237
if(size >= index) {
238238
int* newname = new int[size];
239-
newname[index] = 0; // $ alloc=L238 deref=L239 // GOOD [FALSE POSITIVE]
239+
newname[index] = 0; // GOOD
240240
}
241241
}
242242

0 commit comments

Comments
 (0)