From 2b5d15082965ddfc5a79456cbb3aaad8a9f57b7d Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 24 Sep 2018 14:29:51 +0200 Subject: [PATCH 1/3] C++: Test for IntMultToLong on char-typed numbers --- .../Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c | 4 ++++ .../Arithmetic/IntMultToLong/IntMultToLong.expected | 1 + 2 files changed, 5 insertions(+) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c index 603fd50b7747..4afd40f5839b 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c @@ -88,3 +88,7 @@ void use_printf(float f, double d) // ^ there's a float -> double varargs promotion here, but it's unlikely that the author anticipates requiring a double printf("%f", d * d); // safe } + +size_t three_chars(unsigned char a, unsigned char b, unsigned char c) { + return a * b * c; // at most 16581375 [FALSE POSITIVE] +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected index 4a5edf213790..25cef782aeec 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected @@ -7,3 +7,4 @@ | IntMultToLong.c:61:23:61:33 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'long long'. | | IntMultToLong.c:63:23:63:40 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'long long'. | | IntMultToLong.c:75:9:75:13 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'size_t'. | +| IntMultToLong.c:93:12:93:20 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'size_t'. | From 4d2e4c53f155ddff378df6e3235b3772d2c9faae Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 24 Sep 2018 14:30:44 +0200 Subject: [PATCH 2/3] C++: Suppress IntMultToLong alert on char --- cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql | 2 ++ .../Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c | 2 +- .../Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected | 1 - 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql b/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql index ac2bed3d6f99..4ab00c2d2804 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql @@ -21,6 +21,7 @@ import semmle.code.cpp.controlflow.SSA /** * Holds if `e` is either: * - a constant + * - a char-typed expression, meaning it's a small number * - an array access to an array of constants * - flows from one of the above * In these cases the value of `e` is likely to be small and @@ -28,6 +29,7 @@ import semmle.code.cpp.controlflow.SSA */ predicate effectivelyConstant(Expr e) { e.isConstant() or + e.getType().getSize() <= 1 or e.(ArrayExpr).getArrayBase().getType().(ArrayType).getBaseType().isConst() or exists(SsaDefinition def, Variable v | def.getAUse(v) = e and diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c index 4afd40f5839b..76ab0e83c599 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.c @@ -90,5 +90,5 @@ void use_printf(float f, double d) } size_t three_chars(unsigned char a, unsigned char b, unsigned char c) { - return a * b * c; // at most 16581375 [FALSE POSITIVE] + return a * b * c; // at most 16581375 } diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected index 25cef782aeec..4a5edf213790 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/IntMultToLong/IntMultToLong.expected @@ -7,4 +7,3 @@ | IntMultToLong.c:61:23:61:33 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'long long'. | | IntMultToLong.c:63:23:63:40 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'long long'. | | IntMultToLong.c:75:9:75:13 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'size_t'. | -| IntMultToLong.c:93:12:93:20 | ... * ... | Multiplication result may overflow 'int' before it is converted to 'size_t'. | From a56376a2df90b4c28335303779d9187f84818251 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 24 Sep 2018 14:31:19 +0200 Subject: [PATCH 3/3] C++: rename effectivelyConstant to likelySmall This reflects the existing QLDoc better and makes it more clear why it includes char-typed expressions. --- cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql b/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql index 4ab00c2d2804..885039e9d58d 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/IntMultToLong.ql @@ -27,13 +27,13 @@ import semmle.code.cpp.controlflow.SSA * In these cases the value of `e` is likely to be small and * controlled, so we consider it less likely to cause an overflow. */ -predicate effectivelyConstant(Expr e) { +predicate likelySmall(Expr e) { e.isConstant() or e.getType().getSize() <= 1 or e.(ArrayExpr).getArrayBase().getType().(ArrayType).getBaseType().isConst() or exists(SsaDefinition def, Variable v | def.getAUse(v) = e and - effectivelyConstant(def.getDefiningValue(v)) + likelySmall(def.getDefiningValue(v)) ) } @@ -58,7 +58,7 @@ int getEffectiveMulOperands(MulExpr me) { result = count(Expr op | op = getMulOperand*(me) and not op instanceof MulExpr and - not effectivelyConstant(op) + not likelySmall(op) ) }