hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-20 10:46:30 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #1056 from jbj/SimpleRangeAnalysis-use-after-cast

Browse Source 
C++: Fix use-after-cast bug in SimpleRangeAnalysis
This commit is contained in:
Geoffrey White
2019-04-10 11:04:20 +01:00
committed by GitHub
parent 75ab311c3a 01fc721497
commit 5101a5bc3d
8 changed files with 129 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql
View File

@@ -38,7 +38,7 @@ where
// a comparison between an enum and zero is always valid because whether
// the underlying type of an enum is signed is compiler-dependent
not exists (Expr e, ConstantZero z
| relOpWithSwap(cmp, e, z, _, _) and
| relOpWithSwap(cmp, e.getFullyConverted(), z, _, _) and
e.getUnderlyingType() instanceof Enum) and
// Construct a reason for the message. Something like: x >= 5 and 3 >= y.

60
cpp/ql/src/semmle/code/cpp/rangeanalysis/RangeAnalysisUtils.qll
View File

@@ -36,7 +36,7 @@ boolean directionIsLesser(RelationDirection dir) {
/**
* Holds if `rel` is a relational operation (`<`, `>`, `<=` or `>=`)
* with left operand `lhs` and right operand `rhs`, described by
* with fully-converted children `lhs` and `rhs`, described by
* `dir` and `strict`.
*
* For example, if `rel` is `x < 5` then
@@ -47,8 +47,8 @@ predicate relOp(
RelationalOperation rel, Expr lhs, Expr rhs,
RelationDirection dir, RelationStrictness strict
) {
lhs = rel.getLeftOperand() and
rhs = rel.getRightOperand() and
lhs = rel.getLeftOperand().getFullyConverted() and
rhs = rel.getRightOperand().getFullyConverted() and
((rel instanceof LTExpr and dir = Lesser() and strict = Strict()) or
(rel instanceof LEExpr and dir = Lesser() and strict = Nonstrict()) or
(rel instanceof GTExpr and dir = Greater() and strict = Strict()) or
@@ -57,7 +57,7 @@ predicate relOp(
/**
* Holds if `rel` is a relational operation (`<`, `>`, `<=` or `>=`)
* with children `a` and `b`, described by `dir` and `strict`.
* with fully-converted children `a` and `b`, described by `dir` and `strict`.
*
* This allows for the relation to be either as written, or with its
* arguments reversed; for example, if `rel` is `x < 5` then both
@@ -74,7 +74,7 @@ predicate relOpWithSwap(
/**
* Holds if `rel` is a comparison operation (`<`, `>`, `<=` or `>=`)
* with children `a` and `b`, described by `dir` and `strict`, with
* with fully-converted children `a` and `b`, described by `dir` and `strict`, with
* result `branch`.
*
* This allows for the relation to be either as written, or with its
@@ -95,8 +95,8 @@ predicate relOpWithSwapAndNegate(
}
/**
* Holds if `cmp` is an equality operation (`==` or `!=`) with left
* operand `lhs`, right operand `rhs`, and `isEQ` is true if `cmp` is an
* Holds if `cmp` is an equality operation (`==` or `!=`) with fully-converted
* children `lhs` and `rhs`, and `isEQ` is true if `cmp` is an
* `==` operation and false if it is an `!=` operation.
*
* For example, if `rel` is `x == 5` then
@@ -104,15 +104,15 @@ predicate relOpWithSwapAndNegate(
*/
private
predicate eqOp(EqualityOperation cmp, Expr lhs, Expr rhs, boolean isEQ) {
lhs = cmp.getLeftOperand() and
rhs = cmp.getRightOperand() and
lhs = cmp.getLeftOperand().getFullyConverted() and
rhs = cmp.getRightOperand().getFullyConverted() and
((cmp instanceof EQExpr and isEQ = true) or
(cmp instanceof NEExpr and isEQ = false))
}
/**
* Holds if `cmp` is an equality operation (`==` or `!=`) with operands
* `a` and `b`, and `isEQ` is true if `cmp` is an `==` operation and
* Holds if `cmp` is an equality operation (`==` or `!=`) with fully-converted
* operands `a` and `b`, and `isEQ` is true if `cmp` is an `==` operation and
* false if it is an `!=` operation.
*
* This allows for the equality to be either as written, or with its
@@ -127,8 +127,8 @@ predicate eqOpWithSwap(EqualityOperation cmp, Expr a, Expr b, boolean isEQ) {
}
/**
* Holds if `cmp` is an equality operation (`==` or `!=`) with operands
* `a` and `b`, `isEQ` is true if `cmp` is an `==` operation and
* Holds if `cmp` is an equality operation (`==` or `!=`) with fully-converted
* children `a` and `b`, `isEQ` is true if `cmp` is an `==` operation and
* false if it is an `!=` operation, and the result is `branch`.
*
* This allows for the comparison to be either as written, or with its
@@ -201,12 +201,28 @@ predicate linearAccessImpl(Expr expr, VariableAccess v, float p, float q) {
or
// +(p*v+q) == p*v + q
exists (UnaryPlusExpr unaryPlusExpr
| linearAccess(unaryPlusExpr.getOperand(), v, p, q) and
| linearAccess(unaryPlusExpr.getOperand().getFullyConverted(), v, p, q) and
expr = unaryPlusExpr)
or
// (larger_type)(p*v+q) == p*v + q
exists (Cast cast, ArithmeticType sourceType, ArithmeticType targetType
| linearAccess(cast.getExpr(), v, p, q) and
sourceType = cast.getExpr().getType().getUnspecifiedType() and
targetType = cast.getType().getUnspecifiedType() and
// This allows conversion between signed and unsigned, which is technically
// lossy but common enough that we'll just have to assume the user knows
// what they're doing.
targetType.getSize() >= sourceType.getSize() and
expr = cast)
or
// (p*v+q) == p*v + q
exists (ParenthesisExpr paren
| linearAccess(paren.getExpr(), v, p, q) and
expr = paren)
or
// -(a*v+b) == -a*v + (-b)
exists (UnaryMinusExpr unaryMinusExpr, float a, float b
| linearAccess(unaryMinusExpr.getOperand(), v, a, b) and
| linearAccess(unaryMinusExpr.getOperand().getFullyConverted(), v, a, b) and
expr = unaryMinusExpr and
p = -a and
q = -b)
@@ -270,14 +286,14 @@ predicate cmpWithLinearBound(
}
/**
* Holds if `lb` and `ub` are the lower and upper bounds of type `t`
* respectively.
* Holds if `lb` and `ub` are the lower and upper bounds of the unspecified
* type `t`.
*
* For example, if `t` is a signed 32-bit type then holds if `lb` is
* `-2^31` and `ub` is `2^31 - 1`.
*/
private
predicate typeBounds(Type t, float lb, float ub) {
predicate typeBounds(ArithmeticType t, float lb, float ub) {
exists (IntegralType integralType, float limit
| integralType = t and limit = 2.pow(8 * integralType.getSize())
| if integralType instanceof BoolType
@@ -291,22 +307,22 @@ predicate typeBounds(Type t, float lb, float ub) {
}
/**
* Gets the lower bound for type `t`.
* Gets the lower bound for the unspecified type `t`.
*
* For example, if `t` is a signed 32-bit type then the result is
* `-2^31`.
*/
float typeLowerBound(Type t) {
float typeLowerBound(ArithmeticType t) {
typeBounds(t, result, _)
}
/**
* Gets the upper bound for type `t`.
* Gets the upper bound for the unspecified type `t`.
*
* For example, if `t` is a signed 32-bit type then the result is
* `2^31 - 1`.
*/
float typeUpperBound(Type t) {
float typeUpperBound(ArithmeticType t) {
typeBounds(t, _, result)
}

5
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/lowerBound.expected
View File

@@ -459,3 +459,8 @@
| test.cpp:67:13:67:14 | xb | 0.0 |
| test.cpp:69:10:69:10 | b | 0.0 |
| test.cpp:69:21:69:21 | t | 0.0 |
| test.cpp:74:30:74:30 | c | 0.0 |
| test.cpp:74:34:74:34 | c | 0.0 |
| test.cpp:75:22:75:30 | c_times_2 | 0.0 |
| test.cpp:77:5:77:13 | c_times_2 | 0.0 |
| test.cpp:79:3:79:11 | c_times_2 | 0.0 |

10
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/test.cpp
View File

@@ -68,3 +68,13 @@ bool test3(bool b, int x, int y) {
return b || (bool)t;
}
void use_after_cast(unsigned char c)
{
unsigned short c_times_2 = c + c;
if ((unsigned char)c_times_2 == 0)
{
c_times_2;
}
c_times_2;
}

5
cpp/ql/test/library-tests/rangeanalysis/SimpleRangeAnalysis/upperBound.expected
View File

@@ -459,3 +459,8 @@
| test.cpp:67:13:67:14 | xb | 1.0 |
| test.cpp:69:10:69:10 | b | 1.0 |
| test.cpp:69:21:69:21 | t | 3.0 |
| test.cpp:74:30:74:30 | c | 255.0 |
| test.cpp:74:34:74:34 | c | 255.0 |
| test.cpp:75:22:75:30 | c_times_2 | 510.0 |
| test.cpp:77:5:77:13 | c_times_2 | 510.0 |
| test.cpp:79:3:79:11 | c_times_2 | 510.0 |

54
cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.c
View File

@@ -207,7 +207,7 @@ enum my_enum {
};
int myFunction6(enum my_enum e) {
if (e < 0) {
if (e < 0) { // GOOD (suppressed because it's platform-dependent)
return 1;
}
return 0;
@@ -266,7 +266,57 @@ int negative_zero(double dbl) {
return 0;
}
int nan(double x) {
typedef unsigned char u8;
int widening_cast1(u8 c) {
if (c == 0) {
if ((int)c > 0) { // BAD
return 1;
}
}
return 0;
}
int widening_cast2(u8 c) {
if (c <= 10)
return -1;
else if ((c >= 11) /* BAD */ && (c <= 47))
return 0;
else
return 1;
}
int unsigned_implicit_conversion(unsigned int ui1) {
// These two comparisons are supported by the range analysis because the
// implicit signedness conversion is on the constants (0 and 5), not on the
// variables (ui1).
if (ui1 == 0) {
if (ui1 >= 5) { // BAD
return 1;
}
}
return 0;
}
int signedness_cast1(u8 c) {
if ((signed char)c == 0) {
if (c >= 5) { // BAD
return 1;
}
}
return 0;
}
int signedness_cast2(signed char c) {
if ((u8)c == 0) {
if (c >= 5) { // BAD
return 1;
}
}
return 0;
}
int nan1(double x) {
if (x < 0.0) {
return 100;
}

7
cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected
View File

@@ -32,6 +32,11 @@
| PointlessComparison.c:129:12:129:16 | ... > ... | Comparison is always false because a <= 3. |
| PointlessComparison.c:197:7:197:11 | ... < ... | Comparison is always false because x >= 0. |
| PointlessComparison.c:264:12:264:22 | ... >= ... | Comparison is always true because dbl >= 0 and -0 >= - .... |
| PointlessComparison.c:287:14:287:21 | ... >= ... | Comparison is always true because x >= 0. |
| PointlessComparison.c:273:9:273:18 | ... > ... | Comparison is always false because c <= 0. |
| PointlessComparison.c:283:13:283:19 | ... >= ... | Comparison is always true because c >= 11. |
| PointlessComparison.c:294:9:294:16 | ... >= ... | Comparison is always false because ui1 <= 0. |
| PointlessComparison.c:303:9:303:14 | ... >= ... | Comparison is always false because c <= 0. |
| PointlessComparison.c:312:9:312:14 | ... >= ... | Comparison is always false because c <= 0. |
| PointlessComparison.c:337:14:337:21 | ... >= ... | Comparison is always true because x >= 0. |
| RegressionTests.cpp:57:7:57:22 | ... <= ... | Comparison is always true because * ... <= 4294967295. |
| Templates.cpp:9:10:9:24 | ... <= ... | Comparison is always true because local <= 32767. |

12
cpp/ql/test/query-tests/Likely Bugs/Format/SnprintfOverflow/test.cpp
View File

@@ -103,3 +103,15 @@ void test7(const char *strings) // separated by \0, terminated by \0\0
strings += strlen(strings) + 1;
}
}
void concat_strings(char *buf, size_t buf_len, const char **strings, size_t n_strings) {
while (n_strings > 0) {
int ret = snprintf(buf, buf_len, "%s", *strings); // GOOD
if (ret > buf_len)
return;
buf_len -= ret;
buf += ret;
n_strings--;
strings++;
}
}