Skip to content

Commit fe98775

Browse files
danmardanmar
authored
Fix danmar#4626 (new check: potential NULL pointer dereference, when memory allocation fails) (danmar#7068)
1 parent e27a0e4 commit fe98775

File tree

17 files changed

+152
-17
lines changed

17 files changed

+152
-17
lines changed

‎lib/checknullpointer.cpp‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -442,6 +442,8 @@ void CheckNullPointer::nullPointerError(const Token *tok, const std::string &var
442442
reportError(tok, Severity::error, "nullPointer", "Null pointer dereference", CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
443443
reportError(tok, Severity::warning, "nullPointerDefaultArg", errmsgdefarg, CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
444444
reportError(tok, Severity::warning, "nullPointerRedundantCheck", errmsgcond, CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
445+
reportError(tok, Severity::warning, "nullPointerOutOfMemory", "Null pointer dereference", CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
446+
reportError(tok, Severity::warning, "nullPointerOutOfResources", "Null pointer dereference", CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
445447
return;
446448
}
447449

@@ -461,12 +463,29 @@ void CheckNullPointer::nullPointerError(const Token *tok, const std::string &var
461463
reportError(errorPath, Severity::warning, "nullPointerDefaultArg", errmsgdefarg, CWE_NULL_POINTER_DEREFERENCE, inconclusive || value->isInconclusive() ? Certainty::inconclusive : Certainty::normal);
462464
} else {
463465
std::string errmsg = std::string(value->isKnown() ? "Null" : "Possible null") + " pointer dereference";
466+
467+
std::string id = "nullPointer";
468+
if (value->unknownFunctionReturn == ValueFlow::Value::UnknownFunctionReturn::outOfMemory) {
469+
if (errmsg.compare(0, 9, "Possible ") == 0)
470+
errmsg = "If memory allocation fail: " + errmsg.substr(9);
471+
else
472+
errmsg = "If memory allocation fail: " + errmsg;
473+
id += "OutOfMemory";
474+
}
475+
else if (value->unknownFunctionReturn == ValueFlow::Value::UnknownFunctionReturn::outOfResources) {
476+
if (errmsg.compare(0, 9, "Possible ") == 0)
477+
errmsg = "If resource allocation fail: " + errmsg.substr(9);
478+
else
479+
errmsg = "If resource allocation fail: " + errmsg;
480+
id += "OutOfResources";
481+
}
482+
464483
if (!varname.empty())
465484
errmsg = "$symbol:" + varname + '\n' + errmsg + ": $symbol";
466485

467486
reportError(errorPath,
468487
value->isKnown() ? Severity::error : Severity::warning,
469-
"nullPointer",
488+
id.c_str(),
470489
errmsg,
471490
CWE_NULL_POINTER_DEREFERENCE, inconclusive || value->isInconclusive() ? Certainty::inconclusive : Certainty::normal);
472491
}
@@ -529,10 +548,21 @@ void CheckNullPointer::pointerArithmeticError(const Token* tok, const ValueFlow:
529548
} else {
530549
errmsg = "Pointer " + arithmetic + " with NULL pointer.";
531550
}
551+
552+
std::string id = "nullPointerArithmetic";
553+
if (value && value->unknownFunctionReturn == ValueFlow::Value::UnknownFunctionReturn::outOfMemory) {
554+
errmsg = "If memory allocation fail: " + ((char)std::tolower(errmsg[0]) + errmsg.substr(1));
555+
id += "OutOfMemory";
556+
}
557+
else if (value && value->unknownFunctionReturn == ValueFlow::Value::UnknownFunctionReturn::outOfResources) {
558+
errmsg = "If resource allocation fail: " + ((char)std::tolower(errmsg[0]) + errmsg.substr(1));
559+
id += "OutOfResources";
560+
}
561+
532562
const ErrorPath errorPath = getErrorPath(tok, value, "Null pointer " + arithmetic);
533563
reportError(errorPath,
534564
Severity::error,
535-
"nullPointerArithmetic",
565+
id.c_str(),
536566
errmsg,
537567
CWE_INCORRECT_CALCULATION,
538568
inconclusive ? Certainty::inconclusive : Certainty::normal);

‎lib/valueflow.cpp‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6954,11 +6954,26 @@ static void valueFlowSafeFunctions(const TokenList& tokenlist, const SymbolDatab
69546954

69556955
static void valueFlowUnknownFunctionReturn(TokenList& tokenlist, const Settings& settings)
69566956
{
6957-
if (settings.checkUnknownFunctionReturn.empty())
6957+
if (!tokenlist.front())
69586958
return;
6959-
for (Token* tok = tokenlist.front(); tok; tok = tok->next()) {
6959+
for (Token* tok = tokenlist.front()->next(); tok; tok = tok->next()) {
69606960
if (!tok->astParent() || tok->str() != "(" || !tok->previous()->isName())
69616961
continue;
6962+
6963+
if (settings.library.getAllocFuncInfo(tok->astOperand1()) && settings.library.returnValueType(tok->astOperand1()).find('*') != std::string::npos) {
6964+
// Allocation function that returns a pointer
6965+
ValueFlow::Value value(0);
6966+
value.setPossible();
6967+
value.errorPath.emplace_back(tok, "Assuming allocation function fails");
6968+
const auto* f = settings.library.getAllocFuncInfo(tok->astOperand1());
6969+
if (Library::ismemory(f->groupId))
6970+
value.unknownFunctionReturn = ValueFlow::Value::UnknownFunctionReturn::outOfMemory;
6971+
else
6972+
value.unknownFunctionReturn = ValueFlow::Value::UnknownFunctionReturn::outOfResources;
6973+
setTokenValue(tok, value, settings);
6974+
continue;
6975+
}
6976+
69626977
if (settings.checkUnknownFunctionReturn.find(tok->strAt(-1)) == settings.checkUnknownFunctionReturn.end())
69636978
continue;
69646979
std::vector<MathLib::bigint> unknownValues = settings.library.unknownReturnValues(tok->astOperand1());

‎lib/vfvalue.h‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,14 @@ namespace ValueFlow
284284
/** For calculated values - varId that calculated value depends on */
285285
nonneg int varId{};
286286

287+
enum class UnknownFunctionReturn : uint8_t {
288+
no, // not unknown function return
289+
outOfMemory, // out of memory
290+
outOfResources, // out of resource
291+
other // other
292+
};
293+
UnknownFunctionReturn unknownFunctionReturn{UnknownFunctionReturn::no};
294+
287295
/** value relies on safe checking */
288296
bool safe{};
289297

@@ -296,7 +304,7 @@ namespace ValueFlow
296304
/** Is this value passed as default parameter to the function? */
297305
bool defaultArg{};
298306

299-
int indirect{};
307+
int8_t indirect{};
300308

301309
/** kind of moved */
302310
enum class MoveKind : std::uint8_t { NonMovedVariable, MovedVariable, ForwardedVariable } moveKind = MoveKind::NonMovedVariable;

‎samples/memleak/bad.c‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
#include <stdlib.h>
22
int main()
33
{
4-
int result;
4+
int result = 0;
55
char *a = malloc(10);
6-
a[0] = 0;
7-
result = a[0];
6+
if (a) {
7+
a[0] = 0;
8+
result = a[0];
9+
}
810
return result;
911
}

‎samples/memleak/good.c‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
#include <stdlib.h>
22
int main()
33
{
4-
int result;
4+
int result = 0;
55
char *a = malloc(10);
6-
a[0] = 0;
7-
result = a[0];
8-
free(a);
6+
if (a) {
7+
a[0] = 0;
8+
result = a[0];
9+
free(a);
10+
}
911
return result;
1012
}

‎samples/memleak/out.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
samples\memleak\bad.c:8:5: error: Memory leak: a [memleak]
1+
samples\memleak\bad.c:10:5: error: Memory leak: a [memleak]
22
return result;
33
^

‎test/cfg/bsd.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,8 +155,10 @@ void uninitvar(void)
155155
void arrayIndexOutOfBounds(void)
156156
{
157157
char * pAlloc = calloc(2, 3);
158+
// cppcheck-suppress nullPointerOutOfMemory
158159
pAlloc[5] = 'a';
159160
// cppcheck-suppress arrayIndexOutOfBounds
161+
// cppcheck-suppress nullPointerOutOfMemory
160162
pAlloc[6] = 1;
161163
// cppcheck-suppress memleakOnRealloc
162164
pAlloc = reallocarray(pAlloc, 3, 3);

‎test/cfg/emscripten.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ void em_asm_ptr_memleak_test()
6363
const char *str = static_cast<char*>(EM_ASM_PTR({
6464
return stringToNewUTF8("Hello");
6565
}));
66+
// cppcheck-suppress nullPointerOutOfMemory
6667
printf("%s", str);
6768

6869
// cppcheck-suppress memleak
@@ -88,4 +89,4 @@ void em_js_test()
8889
{
8990
two_alerts();
9091
take_args(100, 35.5);
91-
}
92+
}

‎test/cfg/gnu.c‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ int deallocuse_backtrace(int size) {
5353
void **buffer = (void **)malloc(sizeof(void *) * size);
5454
free(buffer);
5555
// cppcheck-suppress deallocuse
56+
// cppcheck-suppress nullPointerOutOfMemory
5657
int numEntries = backtrace(buffer, size);
5758
return numEntries;
5859
}
@@ -331,6 +332,7 @@ void valid_code(int argInt1, va_list valist_arg, const int * parg)
331332
// cppcheck-suppress valueFlowBailoutIncompleteVar
332333
const void * p_mmap = mmap(NULL, 1, PROT_NONE, MAP_ANONYMOUS | MAP_SHARED, -1, 0);
333334
printf("%p", p_mmap);
335+
// cppcheck-suppress nullPointerOutOfMemory
334336
munmap(p_mmap, 1);
335337

336338
uint16_t i16_1 = 0, i16_2;
@@ -427,6 +429,7 @@ void memleak_asprintf8(const char *fmt, const int arg) // #12204
427429
void memleak_xmalloc()
428430
{
429431
char *p = (char*)xmalloc(10);
432+
// cppcheck-suppress nullPointerOutOfMemory
430433
p[9] = 0;
431434
// cppcheck-suppress memleak
432435
}
@@ -464,14 +467,18 @@ void bufferAccessOutOfBounds()
464467
sethostname(buf, 4);
465468

466469
char * pAlloc1 = xcalloc(2, 4);
470+
// cppcheck-suppress nullPointerOutOfMemory
467471
memset(pAlloc1, 0, 8);
468472
// cppcheck-suppress bufferAccessOutOfBounds
473+
// cppcheck-suppress nullPointerOutOfMemory
469474
memset(pAlloc1, 0, 9);
470475
free(pAlloc1);
471476

472477
char * pAlloc2 = xmalloc(4);
478+
// cppcheck-suppress nullPointerOutOfMemory
473479
memset(pAlloc2, 0, 4);
474480
// cppcheck-suppress bufferAccessOutOfBounds
481+
// cppcheck-suppress nullPointerOutOfMemory
475482
memset(pAlloc2, 0, 5);
476483

477484
pAlloc2 = xrealloc(pAlloc2, 10);

‎test/cfg/gtk.c‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,12 @@ void validCode(int argInt, GHashTableIter * hash_table_iter, GHashTable * hash_t
4646
g_printerr("err");
4747

4848
GString * pGStr1 = g_string_new("test");
49+
// cppcheck-suppress nullPointerOutOfMemory
4950
g_string_append(pGStr1, "a");
5051
g_string_free(pGStr1, TRUE);
5152

5253
gchar * pGchar1 = g_strconcat("a", "b", NULL);
54+
// cppcheck-suppress nullPointerOutOfMemory
5355
printf("%s", pGchar1);
5456
g_free(pGchar1);
5557

0 commit comments

Comments
 (0)