Skip to content

Commit f147b63

Browse files
jbjjbj
authored
Merge pull request #654 from geoffw0/lossyresultcast
CPP: Work on Lossy function result cast query
2 parents beed519 + f2e68da commit f147b63

File tree

7 files changed

+233
-5
lines changed

7 files changed

+233
-5
lines changed

‎change-notes/1.20/analysis-cpp.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,14 @@
88

99
| **Query** | **Tags** | **Purpose** |
1010
|-----------------------------|-----------|--------------------------------------------------------------------|
11+
| Lossy function result cast (`cpp/lossy-function-result-cast`) | correctness | Finds function calls whose result type is a floating point type, which are implicitly cast to an integral type. Newly available but not displayed by default on LGTM. |
1112

1213
## Changes to existing queries
1314

1415
| **Query** | **Expected impact** | **Change** |
1516
|----------------------------|------------------------|------------------------------------------------------------------|
1617
| Suspicious pointer scaling (`cpp/suspicious-pointer-scaling`) | Fewer false positives | False positives involving types that are not uniquely named in the snapshot have been fixed. |
18+
| Lossy function result cast (`cpp/lossy-function-result-cast`) | Fewer false positive results | The whitelist of rounding functions built into this query has been expanded. |
1719
| Unused static variable (`cpp/unused-static-variable`) | Fewer false positive results | Variables with the attribute `unused` are now excluded from the query. |
1820
| Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Fix false positives where a resource is released via a virtual method call. |
1921

‎cpp/ql/src/Likely Bugs/Conversion/LossyFunctionResultCast.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
double getWidth();
2+
3+
void f() {
4+
int width = getWidth();
5+
6+
// ...
7+
}

‎cpp/ql/src/Likely Bugs/Conversion/LossyFunctionResultCast.qhelp‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>This rule finds function calls whose result type is a floating point type, which are implicitly cast to an integral type. Such code may not behave as intended when the floating point return value has a fractional part, or takes an extreme value outside the range that can be represented by the integer type.</p>
8+
9+
</overview>
10+
<recommendation>
11+
<p>Consider changing the surrounding expression to match the floating point type. If rounding is intended, explicitly round using a standard function such as `trunc`, `floor` or `round`.</p>
12+
13+
</recommendation>
14+
<example><sample src="LossyFunctionResultCast.cpp" />
15+
<p>In this example, the result of the call to <code>getWidth()</code> is implicitly cast to <code>int</code>, resulting in an unintended loss of accuracy. To fix this, the type of variable <code>width</code> could be changed from <code>int</code> to <code>double</code>.</p>
16+
17+
</example>
18+
<references>
19+
20+
<li>
21+
Microsoft Visual C++ Documentation: <a href="https://docs.microsoft.com/en-us/cpp/cpp/type-conversions-and-type-safety-modern-cpp?view=vs-2017">Type Conversions and Type Safety (Modern C++)</a>.
22+
</li>
23+
<li>
24+
Cplusplus.com: <a href="http://www.cplusplus.com/doc/tutorial/typecasting/">Type conversions</a>.
25+
</li>
26+
27+
</references>
28+
</qhelp>

‎cpp/ql/src/Likely Bugs/Conversion/LossyFunctionResultCast.ql‎

Lines changed: 55 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,68 @@
11
/**
22
* @name Lossy function result cast
33
* @description Finds function calls whose result type is a floating point type, and which are casted to an integral type.
4-
* Includes only expressions with implicit cast and excludes function calls to ceil, floor and round. {This is a gcc check; doesn't seem wildly useful.}
4+
* Includes only expressions with implicit cast and excludes function calls to ceil, floor and round.
55
* @kind problem
66
* @id cpp/lossy-function-result-cast
77
* @problem.severity warning
8+
* @precision medium
9+
* @tags correctness
810
*/
11+
912
import cpp
13+
import semmle.code.cpp.dataflow.DataFlow
14+
15+
predicate whitelist(Function f) {
16+
exists(string fName |
17+
fName = f.getName() and
18+
(
19+
fName = "ceil" or
20+
fName = "ceilf" or
21+
fName = "ceill" or
22+
fName = "floor" or
23+
fName = "floorf" or
24+
fName = "floorl" or
25+
fName = "nearbyint" or
26+
fName = "nearbyintf" or
27+
fName = "nearbyintl" or
28+
fName = "rint" or
29+
fName = "rintf" or
30+
fName = "rintl" or
31+
fName = "round" or
32+
fName = "roundf" or
33+
fName = "roundl" or
34+
fName = "trunc" or
35+
fName = "truncf" or
36+
fName = "truncl" or
37+
fName.matches("__builtin_%")
38+
)
39+
)
40+
}
41+
42+
predicate whitelistPow(FunctionCall fc) {
43+
(
44+
fc.getTarget().getName() = "pow" or
45+
fc.getTarget().getName() = "powf" or
46+
fc.getTarget().getName() = "powl"
47+
) and exists(float value |
48+
value = fc.getArgument(0).getValue().toFloat() and
49+
(value.floor() - value).abs() < 0.001
50+
)
51+
}
52+
53+
predicate whiteListWrapped(FunctionCall fc) {
54+
whitelist(fc.getTarget()) or
55+
whitelistPow(fc) or
56+
exists(Expr e, ReturnStmt rs |
57+
whiteListWrapped(e) and
58+
DataFlow::localFlow(DataFlow::exprNode(e), DataFlow::exprNode(rs.getExpr())) and
59+
fc.getTarget() = rs.getEnclosingFunction()
60+
)
61+
}
1062

1163
from FunctionCall c, FloatingPointType t1, IntegralType t2
1264
where t1 = c.getTarget().getType().getUnderlyingType() and
1365
t2 = c.getActualType() and
1466
c.hasImplicitConversion() and
15-
not c.getTarget().getName() = "ceil" and
16-
not c.getTarget().getName() = "floor" and
17-
not c.getTarget().getName() = "round"
18-
select c
67+
not whiteListWrapped(c)
68+
select c, "Return value of type " + t1.toString() + " is implicitly converted to " + t2.toString() + " here."

‎cpp/ql/test/query-tests/Likely Bugs/Conversion/LossyFunctionResultCast/ImplicitDowncastFromBitfield.expected‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
| test.cpp:33:6:33:13 | call to getFloat | Return value of type float is implicitly converted to bool here. |
2+
| test.cpp:35:13:35:20 | call to getFloat | Return value of type float is implicitly converted to int here. |
3+
| test.cpp:38:6:38:14 | call to getDouble | Return value of type double is implicitly converted to bool here. |
4+
| test.cpp:40:13:40:21 | call to getDouble | Return value of type double is implicitly converted to int here. |
5+
| test.cpp:43:6:43:12 | call to getMyLD | Return value of type long double is implicitly converted to bool here. |
6+
| test.cpp:45:13:45:19 | call to getMyLD | Return value of type long double is implicitly converted to int here. |
7+
| test.cpp:101:10:101:12 | call to pow | Return value of type double is implicitly converted to int here. |
8+
| test.cpp:103:10:103:12 | call to pow | Return value of type double is implicitly converted to int here. |
9+
| test.cpp:105:10:105:12 | call to pow | Return value of type double is implicitly converted to int here. |

‎cpp/ql/test/query-tests/Likely Bugs/Conversion/LossyFunctionResultCast/ImplicitDowncastFromBitfield.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Likely Bugs/Conversion/LossyFunctionResultCast.ql

‎cpp/ql/test/query-tests/Likely Bugs/Conversion/LossyFunctionResultCast/test.cpp‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
2+
typedef long double MYLD;
3+
4+
bool getBool();
5+
int getInt();
6+
float getFloat();
7+
double getDouble();
8+
MYLD getMyLD();
9+
float *getFloatPtr();
10+
float &getFloatRef();
11+
const float &getConstFloatRef();
12+
13+
void setPosInt(int x);
14+
void setPosFloat(float x);
15+
16+
double round(double x);
17+
float roundf(float x);
18+
19+
void test1()
20+
{
21+
// simple
22+
23+
if (getBool())
24+
{
25+
setPosInt(getBool());
26+
setPosFloat(getBool());
27+
}
28+
if (getInt())
29+
{
30+
setPosInt(getInt());
31+
setPosFloat(getInt());
32+
}
33+
if (getFloat()) // BAD
34+
{
35+
setPosInt(getFloat()); // BAD
36+
setPosFloat(getFloat());
37+
}
38+
if (getDouble()) // BAD
39+
{
40+
setPosInt(getDouble()); // BAD
41+
setPosFloat(getDouble());
42+
}
43+
if (getMyLD()) // BAD
44+
{
45+
setPosInt(getMyLD()); // BAD
46+
setPosFloat(getMyLD());
47+
}
48+
if (getFloatPtr())
49+
{
50+
// ...
51+
}
52+
if (getFloatRef()) // BAD [NOT DETECTED]
53+
{
54+
setPosInt(getFloatRef()); // BAD [NOT DETECTED]
55+
setPosFloat(getFloatRef());
56+
}
57+
if (getConstFloatRef()) // BAD [NOT DETECTED]
58+
{
59+
setPosInt(getConstFloatRef()); // BAD [NOT DETECTED]
60+
setPosFloat(getConstFloatRef());
61+
}
62+
63+
// explicit cast
64+
65+
if ((bool)getInt())
66+
{
67+
setPosInt(getInt());
68+
setPosFloat((float)getInt());
69+
}
70+
if ((bool)getFloat())
71+
{
72+
setPosInt((int)getFloat());
73+
setPosFloat(getFloat());
74+
}
75+
76+
// explicit rounding
77+
78+
if (roundf(getFloat()))
79+
{
80+
setPosInt(roundf(getFloat()));
81+
setPosFloat(roundf(getFloat()));
82+
}
83+
if (round(getDouble()))
84+
{
85+
setPosInt(round(getDouble()));
86+
setPosFloat(round(getDouble()));
87+
}
88+
}
89+
90+
double pow(double x, double y);
91+
92+
int test2(double v, double w, int n)
93+
{
94+
switch (n)
95+
{
96+
case 1:
97+
return pow(2, v); // GOOD
98+
case 2:
99+
return pow(10, v); // GOOD
100+
case 3:
101+
return pow(2.5, v); // BAD
102+
case 4:
103+
return pow(v, 2); // BAD
104+
case 5:
105+
return pow(v, w); // BAD
106+
};
107+
}
108+
109+
double myRound1(double v)
110+
{
111+
return round(v);
112+
}
113+
114+
double myRound2(double v)
115+
{
116+
double result = round(v);
117+
118+
return result;
119+
}
120+
121+
double myRound3(double v)
122+
{
123+
return (v > 0) ? round(v) : 0;
124+
}
125+
126+
void test3()
127+
{
128+
int i = myRound1(1.5); // GOOD
129+
int j = myRound2(2.5); // GOOD
130+
int k = myRound3(3.5); // GOOD
131+
}

0 commit comments

Comments
 (0)