Skip to content

Commit 315f09b

Browse files
mbaludambaluda
committed
Addressing review comments
1 parent c38fe8c commit 315f09b

File tree

3 files changed

+170
-102
lines changed

3 files changed

+170
-102
lines changed

‎c/cert/src/rules/ERR33-C/DetectAndHandleStandardLibraryErrors.ql

Lines changed: 157 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
/**
22
* @id c/cert/detect-and-handle-standard-library-errors
33
* @name ERR33-C: Detect and handle standard library errors
4-
* @description Detect and handle standard library errors.
4+
* @description Detect and handle standard library errors. Undetected failures can lead to
5+
* unexpected or undefined behavior.
56
* @kind problem
67
* @precision high
78
* @problem.severity error
@@ -14,39 +15,54 @@ import cpp
1415
import codingstandards.c.cert
1516
import semmle.code.cpp.commons.NULL
1617
import codingstandards.cpp.ReadErrorsAndEOF
17-
import semmle.code.cpp.dataflow.DataFlow
1818

1919
/**
2020
* Classifies error returning function calls based on the
2121
* type and value of the required checked
2222
*/
23-
class ExpectedErrReturn extends FunctionCall {
23+
abstract class ExpectedErrReturn extends FunctionCall {
2424
Expr errValue;
25-
string errOperator;
25+
ComparisonOperation errOperator;
2626

27-
ExpectedErrReturn() {
28-
errOperator = ["==", "!="] and
27+
Expr getErrValue() { result = errValue }
28+
29+
ComparisonOperation getErrOperator() { result = errOperator }
30+
}
31+
32+
class ExpectedErrReturnEqZero extends ExpectedErrReturn {
33+
ExpectedErrReturnEqZero() {
34+
errOperator instanceof EqualityOperation and
35+
errValue.(Literal).getValue() = "0" and
36+
this.getTarget()
37+
.hasName([
38+
"asctime_s", "at_quick_exit", "atexit", "ctime_s", "fgetpos", "fopen_s", "freopen_s",
39+
"fseek", "fsetpos", "mbsrtowcs_s", "mbstowcs_s", "raise", "remove", "rename", "setvbuf",
40+
"strerror_s", "strftime", "strtod", "strtof", "strtold", "timespec_get", "tmpfile_s",
41+
"tmpnam_s", "tss_get", "wcsftime", "wcsrtombs_s", "wcstod", "wcstof", "wcstold",
42+
"wcstombs_s", "wctrans", "wctype"
43+
])
44+
}
45+
}
46+
47+
class ExpectedErrReturnEqNull extends ExpectedErrReturn {
48+
ExpectedErrReturnEqNull() {
49+
errOperator instanceof EqualityOperation and
50+
errValue instanceof NULL and
51+
this.getTarget()
52+
.hasName([
53+
"aligned_alloc", "bsearch_s", "bsearch", "calloc", "fgets", "fopen", "freopen",
54+
"getenv_s", "getenv", "gets_s", "gmtime_s", "gmtime", "localtime_s", "localtime",
55+
"malloc", "memchr", "realloc", "setlocale", "strchr", "strpbrk", "strrchr", "strstr",
56+
"strtok_s", "strtok", "tmpfile", "tmpnam", "wcschr", "wcspbrk", "wcsrchr", "wcsstr",
57+
"wcstok_s", "wcstok", "wmemchr"
58+
])
59+
}
60+
}
61+
62+
class ExpectedErrReturnEqEofWeof extends ExpectedErrReturn {
63+
ExpectedErrReturnEqEofWeof() {
64+
errOperator instanceof EqualityOperation and
2965
(
30-
errValue.(Literal).getValue() = "0" and
31-
this.getTarget()
32-
.hasName([
33-
"asctime_s", "at_quick_exit", "atexit", "ctime_s", "fgetpos", "fopen_s", "freopen_s",
34-
"fseek", "fsetpos", "mbsrtowcs_s", "mbstowcs_s", "raise", "remove", "rename",
35-
"setvbuf", "strerror_s", "strftime", "strtod", "strtof", "strtold", "timespec_get",
36-
"tmpfile_s", "tmpnam_s", "tss_get", "wcsftime", "wcsrtombs_s", "wcstod", "wcstof",
37-
"wcstold", "wcstombs_s", "wctrans", "wctype"
38-
])
39-
or
40-
errValue instanceof NULL and
41-
this.getTarget()
42-
.hasName([
43-
"aligned_alloc", "bsearch_s", "bsearch", "calloc", "fgets", "fopen", "freopen",
44-
"getenv_s", "getenv", "gets_s", "gmtime_s", "gmtime", "localtime_s", "localtime",
45-
"malloc", "memchr", "realloc", "setlocale", "strchr", "strpbrk", "strrchr", "strstr",
46-
"strtok_s", "strtok", "tmpfile", "tmpnam", "wcschr", "wcspbrk", "wcsrchr", "wcsstr",
47-
"wcstok_s", "wcstok", "wmemchr"
48-
])
49-
or
5066
errValue = any(EOFInvocation i).getExpr() and
5167
this.getTarget()
5268
.hasName([
@@ -62,7 +78,14 @@ class ExpectedErrReturn extends FunctionCall {
6278
.hasName([
6379
"btowc", "fgetwc", "fputwc", "getwc", "getwchar", "putwc", "ungetwc", "putwchar"
6480
])
65-
or
81+
)
82+
}
83+
}
84+
85+
class ExpectedErrReturnEqEnumConstant extends ExpectedErrReturn {
86+
ExpectedErrReturnEqEnumConstant() {
87+
errOperator instanceof EqualityOperation and
88+
(
6689
errValue = any(EnumConstantAccess i | i.toString() = "thrd_error") and
6790
this.getTarget()
6891
.hasName([
@@ -79,7 +102,14 @@ class ExpectedErrReturn extends FunctionCall {
79102
or
80103
errValue = any(EnumConstantAccess i | i.toString() = "thrd_busy") and
81104
this.getTarget().hasName(["mtx_trylock"])
82-
or
105+
)
106+
}
107+
}
108+
109+
class ExpectedErrReturnEqMacroInvocation extends ExpectedErrReturn {
110+
ExpectedErrReturnEqMacroInvocation() {
111+
errOperator instanceof EqualityOperation and
112+
(
83113
errValue = any(MacroInvocation i | i.getMacroName() = "UINTMAX_MAX").getExpr() and
84114
this.getTarget().hasName(["strtoumax", "wcstoumax"])
85115
or
@@ -100,55 +130,114 @@ class ExpectedErrReturn extends FunctionCall {
100130
or
101131
errValue = any(MacroInvocation i | i.getMacroName() = ["LLONG_MAX", "LLONG_MIN"]).getExpr() and
102132
this.getTarget().hasName(["strtoll", "wcstoll"])
103-
or
133+
)
134+
}
135+
}
136+
137+
class ExpectedErrReturnEqMinusOne extends ExpectedErrReturn {
138+
ExpectedErrReturnEqMinusOne() {
139+
errOperator instanceof EqualityOperation and
140+
(
104141
errValue.(UnaryMinusExpr).getOperand().(Literal).getValue() = "1" and
105142
this.getTarget()
106143
.hasName([
107144
"c16rtomb", "c32rtomb", "clock", "ftell", "mbrtoc16", "mbrtoc32", "mbsrtowcs",
108145
"mbstowcs", "mktime", "time", "wcrtomb", "wcsrtombs", "wcstombs"
109146
])
110147
or
148+
// functions that behave differently when the first argument is NULL
111149
errValue.(UnaryMinusExpr).getOperand().(Literal).getValue() = "1" and
112150
not this.getArgument(0) instanceof NULL and
113151
this.getTarget().hasName(["mblen", "mbrlen", "mbrtowc", "mbtowc", "wctomb_s", "wctomb"])
114-
or
115-
errValue.getType() instanceof IntType and
116-
this.getTarget().hasName(["fread", "fwrite"])
117-
)
118-
or
119-
errOperator = ["<", ">="] and
120-
(
121-
errValue.(Literal).getValue() = "0" and
122-
this.getTarget()
123-
.hasName([
124-
"fprintf_s", "fprintf", "fwprintf_s", "fwprintf", "printf_s", "snprintf_s",
125-
"snprintf", "sprintf_s", "sprintf", "swprintf_s", "swprintf", "thrd_sleep",
126-
"vfprintf_s", "vfprintf", "vfwprintf_s", "vfwprintf", "vprintf_s", "vsnprintf_s",
127-
"vsnprintf", "vsprintf_s", "vsprintf", "vswprintf_s", "vswprintf", "vwprintf_s",
128-
"wprintf_s", "printf", "vprintf", "wprintf", "vwprintf"
129-
])
130-
or
131-
errValue.getType() instanceof IntType and
132-
this.getTarget().hasName(["strxfrm", "wcsxfrm"])
133-
)
134-
or
135-
errOperator = "NA" and
136-
(
137-
errValue = any(Expr e) and
138-
this.getTarget()
139-
.hasName([
140-
"kill_dependency", "memcpy", "wmemcpy", "memmove", "wmemmove", "strcpy", "wcscpy",
141-
"strncpy", "wcsncpy", "strcat", "wcscat", "strncat", "wcsncat", "memset", "wmemset"
142-
])
143152
)
144153
}
154+
}
145155

146-
Expr getErrValue() { result = errValue }
156+
class ExpectedErrReturnEqInt extends ExpectedErrReturn {
157+
ExpectedErrReturnEqInt() {
158+
errOperator instanceof EqualityOperation and
159+
errValue.getType() instanceof IntType and
160+
this.getTarget().hasName(["fread", "fwrite"])
161+
}
162+
}
163+
164+
class ExpectedErrReturnLtZero extends ExpectedErrReturn {
165+
ExpectedErrReturnLtZero() {
166+
errOperator.getOperator() = ["<", ">="] and
167+
errValue.(Literal).getValue() = "0" and
168+
this.getTarget()
169+
.hasName([
170+
"fprintf_s", "fprintf", "fwprintf_s", "fwprintf", "printf_s", "snprintf_s", "snprintf",
171+
"sprintf_s", "sprintf", "swprintf_s", "swprintf", "thrd_sleep", "vfprintf_s",
172+
"vfprintf", "vfwprintf_s", "vfwprintf", "vprintf_s", "vsnprintf_s", "vsnprintf",
173+
"vsprintf_s", "vsprintf", "vswprintf_s", "vswprintf", "vwprintf_s", "wprintf_s",
174+
"printf", "vprintf", "wprintf", "vwprintf"
175+
])
176+
}
177+
}
178+
179+
class ExpectedErrReturnLtInt extends ExpectedErrReturn {
180+
ExpectedErrReturnLtInt() {
181+
errOperator.getOperator() = ["<", ">="] and
182+
errValue.getType() instanceof IntType and
183+
this.getTarget().hasName(["strxfrm", "wcsxfrm"])
184+
}
185+
}
186+
187+
class ExpectedErrReturnNA extends ExpectedErrReturn {
188+
ExpectedErrReturnNA() {
189+
errOperator.getOperator() = ["<", ">="] and
190+
errValue = any(Expr e) and
191+
this.getTarget()
192+
.hasName([
193+
"kill_dependency", "memcpy", "wmemcpy", "memmove", "wmemmove", "strcpy", "wcscpy",
194+
"strncpy", "wcsncpy", "strcat", "wcscat", "strncat", "wcsncat", "memset", "wmemset"
195+
])
196+
}
197+
}
198+
199+
/**
200+
* calls that can be verified using ferror() && feof()
201+
*/
202+
class FerrorFeofException extends FunctionCall {
203+
FerrorFeofException() {
204+
this.getTarget().hasName(["fgetc", "fgetwc", "getc", "getchar"])
205+
implies
206+
missingFeofFerrorChecks(this)
207+
}
208+
}
209+
210+
/**
211+
* calls that can be verified using ferror()
212+
*/
213+
class FerrorException extends FunctionCall {
214+
FerrorException() {
215+
this.getTarget().hasName(["fputc", "putc"])
216+
implies
217+
this.getEnclosingFunction() = ferrorNotchecked(this)
218+
}
219+
}
147220

148-
string getErrOperator() { result = errOperator }
221+
/**
222+
* ERR33-C-EX1: calls that must not be verified if cast to `void`
223+
*/
224+
class VoidCastException extends FunctionCall {
225+
VoidCastException() {
226+
this.getTarget()
227+
.hasName([
228+
"putchar", "putwchar", "puts", "printf", "vprintf", "wprintf", "vwprintf",
229+
"kill_dependency", "memcpy", "wmemcpy", "memmove", "wmemmove", "strcpy", "wcscpy",
230+
"strncpy", "wcsncpy", "strcat", "wcscat", "strncat", "wcsncat", "memset", "wmemset"
231+
])
232+
implies
233+
not this.getExplicitlyConverted() instanceof VoidConversion
234+
}
149235
}
150236

151-
// Nodes following a file write before a call to `ferror` is performed
237+
/**
238+
* CFG search:
239+
* Nodes following a file write before a call to `ferror` is performed
240+
*/
152241
ControlFlowNode ferrorNotchecked(FileWriteFunctionCall write) {
153242
result = write
154243
or
@@ -168,7 +257,7 @@ where
168257
// calls that must be verified using the return value
169258
not exists(ComparisonOperation op |
170259
DataFlow::localExprFlow(err, op.getAnOperand()) and
171-
(err.getErrOperator() != "NA" implies op.getOperator() = err.getErrOperator()) and
260+
op = err.getErrOperator() and
172261
op.getAnOperand() = err.getErrValue() and
173262
// special case for function `realloc` where the returned pointer
174263
// should not be invalidated
@@ -178,30 +267,9 @@ where
178267
err.getArgument(0).(VariableAccess).getTarget()
179268
)
180269
) and
181-
// EXCEPTIONS
182-
(
183-
// calls that can be verified using ferror() && feof()
184-
err.getTarget().hasName(["fgetc", "fgetwc", "getc", "getchar"])
185-
implies
186-
missingFeofFerrorChecks(err)
187-
) and
188-
(
189-
// calls that can be verified using ferror()
190-
err.getTarget().hasName(["fputc", "putc"])
191-
implies
192-
err.getEnclosingFunction() = ferrorNotchecked(err)
193-
) and
194-
(
195-
// ERR33-C-EX1: calls that can be ignored when cast to `void`
196-
err.getTarget()
197-
.hasName([
198-
"putchar", "putwchar", "puts", "printf", "vprintf", "wprintf", "vwprintf",
199-
"kill_dependency", "memcpy", "wmemcpy", "memmove", "wmemmove", "strcpy", "wcscpy",
200-
"strncpy", "wcsncpy", "strcat", "wcscat", "strncat", "wcsncat", "memset", "wmemset"
201-
])
202-
implies
203-
not err.getExplicitlyConverted() instanceof VoidConversion
204-
)
205-
select err,
206-
"Missing error detection for the call to function `" + err.getTarget() +
207-
"`. Undetected failures can lead to unexpected or undefined behavior."
270+
// ERR33-C-EX1: calls for which it is acceptable
271+
// to ignore the return value
272+
err instanceof FerrorFeofException and
273+
err instanceof FerrorException and
274+
err instanceof VoidCastException
275+
select err, "Missing error detection for the call to function `" + err.getTarget() + "`."

‎c/cert/test/rules/ERR33-C/DetectAndHandleStandardLibraryErrors.expected

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
| test.c:18:3:18:11 | call to setlocale | Missing error detection for the call to function `setlocale`. Undetected failures can lead to unexpected or undefined behavior. |
2-
| test.c:24:23:24:31 | call to setlocale | Missing error detection for the call to function `setlocale`. Undetected failures can lead to unexpected or undefined behavior. |
3-
| test.c:29:22:29:27 | call to calloc | Missing error detection for the call to function `calloc`. Undetected failures can lead to unexpected or undefined behavior. |
4-
| test.c:35:7:35:13 | call to realloc | Missing error detection for the call to function `realloc`. Undetected failures can lead to unexpected or undefined behavior. |
5-
| test.c:46:3:46:7 | call to fseek | Missing error detection for the call to function `fseek`. Undetected failures can lead to unexpected or undefined behavior. |
6-
| test.c:52:3:52:10 | call to snprintf | Missing error detection for the call to function `snprintf`. Undetected failures can lead to unexpected or undefined behavior. |
7-
| test.c:60:3:60:9 | call to putchar | Missing error detection for the call to function `putchar`. Undetected failures can lead to unexpected or undefined behavior. |
8-
| test.c:63:3:63:8 | call to printf | Missing error detection for the call to function `printf`. Undetected failures can lead to unexpected or undefined behavior. |
9-
| test.c:74:22:74:30 | call to localtime | Missing error detection for the call to function `localtime`. Undetected failures can lead to unexpected or undefined behavior. |
10-
| test.c:80:3:80:7 | call to mblen | Missing error detection for the call to function `mblen`. Undetected failures can lead to unexpected or undefined behavior. |
11-
| test.c:97:5:97:9 | call to fputc | Missing error detection for the call to function `fputc`. Undetected failures can lead to unexpected or undefined behavior. |
12-
| test.c:105:5:105:11 | call to getchar | Missing error detection for the call to function `getchar`. Undetected failures can lead to unexpected or undefined behavior. |
1+
| test.c:18:3:18:11 | call to setlocale | Missing error detection for the call to function `setlocale`. |
2+
| test.c:24:23:24:31 | call to setlocale | Missing error detection for the call to function `setlocale`. |
3+
| test.c:29:22:29:27 | call to calloc | Missing error detection for the call to function `calloc`. |
4+
| test.c:35:7:35:13 | call to realloc | Missing error detection for the call to function `realloc`. |
5+
| test.c:46:3:46:7 | call to fseek | Missing error detection for the call to function `fseek`. |
6+
| test.c:52:3:52:10 | call to snprintf | Missing error detection for the call to function `snprintf`. |
7+
| test.c:60:3:60:9 | call to putchar | Missing error detection for the call to function `putchar`. |
8+
| test.c:63:3:63:8 | call to printf | Missing error detection for the call to function `printf`. |
9+
| test.c:74:22:74:30 | call to localtime | Missing error detection for the call to function `localtime`. |
10+
| test.c:80:3:80:7 | call to mblen | Missing error detection for the call to function `mblen`. |
11+
| test.c:97:5:97:9 | call to fputc | Missing error detection for the call to function `fputc`. |
12+
| test.c:105:5:105:11 | call to getchar | Missing error detection for the call to function `getchar`. |

‎rule_packages/c/Contracts5.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
},
2424
"queries": [
2525
{
26-
"description": "Detect and handle standard library errors.",
26+
"description": "Detect and handle standard library errors. Undetected failures can lead to unexpected or undefined behavior.",
2727
"kind": "problem",
2828
"name": "Detect and handle standard library errors",
2929
"precision": "high",

0 commit comments

Comments
 (0)