Fix #4626 (new check: potential NULL pointer dereference, when memory…

… allocation fails) (#7068)
danmar · Dec 8, 2024 · fe98775 · orbitcowboy · Dec 8, 2024 · danmar
1 parent e27a0e4
commit fe98775
Show file tree

Hide file tree

Showing 17 changed files with 152 additions and 17 deletions.
diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp
@@ -442,6 +442,8 @@ void CheckNullPointer::nullPointerError(const Token *tok, const std::string &var
         reportError(tok, Severity::error, "nullPointer", "Null pointer dereference", CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
         reportError(tok, Severity::warning, "nullPointerDefaultArg", errmsgdefarg, CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
         reportError(tok, Severity::warning, "nullPointerRedundantCheck", errmsgcond, CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
+        reportError(tok, Severity::warning, "nullPointerOutOfMemory", "Null pointer dereference", CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
+        reportError(tok, Severity::warning, "nullPointerOutOfResources", "Null pointer dereference", CWE_NULL_POINTER_DEREFERENCE, Certainty::normal);
         return;
     }
 
@@ -461,12 +463,29 @@ void CheckNullPointer::nullPointerError(const Token *tok, const std::string &var
         reportError(errorPath, Severity::warning, "nullPointerDefaultArg", errmsgdefarg, CWE_NULL_POINTER_DEREFERENCE, inconclusive || value->isInconclusive() ? Certainty::inconclusive : Certainty::normal);
     } else {
         std::string errmsg = std::string(value->isKnown() ? "Null" : "Possible null") + " pointer dereference";
+
+        std::string id = "nullPointer";
+        if (value->unknownFunctionReturn == ValueFlow::Value::UnknownFunctionReturn::outOfMemory) {
+            if (errmsg.compare(0, 9, "Possible ") == 0)
+                errmsg = "If memory allocation fail: " + errmsg.substr(9);
+            else
+                errmsg = "If memory allocation fail: " + errmsg;
+            id += "OutOfMemory";
+        }
+        else if (value->unknownFunctionReturn == ValueFlow::Value::UnknownFunctionReturn::outOfResources) {
+            if (errmsg.compare(0, 9, "Possible ") == 0)
+                errmsg = "If resource allocation fail: " + errmsg.substr(9);
+            else
+                errmsg = "If resource allocation fail: " + errmsg;
+            id += "OutOfResources";
+        }
+
         if (!varname.empty())
             errmsg = "$symbol:" + varname + '\n' + errmsg + ": $symbol";
 
         reportError(errorPath,
                     value->isKnown() ? Severity::error : Severity::warning,
-                    "nullPointer",
+                    id.c_str(),
                     errmsg,
                     CWE_NULL_POINTER_DEREFERENCE, inconclusive || value->isInconclusive() ? Certainty::inconclusive : Certainty::normal);
     }
@@ -529,10 +548,21 @@ void CheckNullPointer::pointerArithmeticError(const Token* tok, const ValueFlow:
     } else {
         errmsg = "Pointer " + arithmetic + " with NULL pointer.";
     }
+
+    std::string id = "nullPointerArithmetic";
+    if (value && value->unknownFunctionReturn == ValueFlow::Value::UnknownFunctionReturn::outOfMemory) {
+        errmsg = "If memory allocation fail: " + ((char)std::tolower(errmsg[0]) + errmsg.substr(1));
+        id += "OutOfMemory";
+    }
+    else if (value && value->unknownFunctionReturn == ValueFlow::Value::UnknownFunctionReturn::outOfResources) {
+        errmsg = "If resource allocation fail: " + ((char)std::tolower(errmsg[0]) + errmsg.substr(1));
+        id += "OutOfResources";
+    }
+
     const ErrorPath errorPath = getErrorPath(tok, value, "Null pointer " + arithmetic);
     reportError(errorPath,
                 Severity::error,
-                "nullPointerArithmetic",
+                id.c_str(),
                 errmsg,
                 CWE_INCORRECT_CALCULATION,
                 inconclusive ? Certainty::inconclusive : Certainty::normal);

diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
@@ -6954,11 +6954,26 @@ static void valueFlowSafeFunctions(const TokenList& tokenlist, const SymbolDatab
 
 static void valueFlowUnknownFunctionReturn(TokenList& tokenlist, const Settings& settings)
 {
-    if (settings.checkUnknownFunctionReturn.empty())
+    if (!tokenlist.front())
         return;
-    for (Token* tok = tokenlist.front(); tok; tok = tok->next()) {
+    for (Token* tok = tokenlist.front()->next(); tok; tok = tok->next()) {
         if (!tok->astParent() || tok->str() != "(" || !tok->previous()->isName())
             continue;
+
+        if (settings.library.getAllocFuncInfo(tok->astOperand1()) && settings.library.returnValueType(tok->astOperand1()).find('*') != std::string::npos) {
+            // Allocation function that returns a pointer
+            ValueFlow::Value value(0);
+            value.setPossible();
+            value.errorPath.emplace_back(tok, "Assuming allocation function fails");
+            const auto* f = settings.library.getAllocFuncInfo(tok->astOperand1());
+            if (Library::ismemory(f->groupId))
+                value.unknownFunctionReturn = ValueFlow::Value::UnknownFunctionReturn::outOfMemory;
+            else
+                value.unknownFunctionReturn = ValueFlow::Value::UnknownFunctionReturn::outOfResources;
+            setTokenValue(tok, value, settings);
+            continue;
+        }
+
         if (settings.checkUnknownFunctionReturn.find(tok->strAt(-1)) == settings.checkUnknownFunctionReturn.end())
             continue;
         std::vector<MathLib::bigint> unknownValues = settings.library.unknownReturnValues(tok->astOperand1());

diff --git a/lib/vfvalue.h b/lib/vfvalue.h
@@ -284,6 +284,14 @@ namespace ValueFlow
         /** For calculated values - varId that calculated value depends on */
         nonneg int varId{};
 
+        enum class UnknownFunctionReturn : uint8_t {
+            no,             // not unknown function return
+            outOfMemory,    // out of memory
+            outOfResources, // out of resource
+            other           // other
+        };
+        UnknownFunctionReturn unknownFunctionReturn{UnknownFunctionReturn::no};
+
         /** value relies on safe checking */
         bool safe{};
 
@@ -296,7 +304,7 @@ namespace ValueFlow
         /** Is this value passed as default parameter to the function? */
         bool defaultArg{};
 
-        int indirect{};
+        int8_t indirect{};
 
         /** kind of moved  */
         enum class MoveKind : std::uint8_t { NonMovedVariable, MovedVariable, ForwardedVariable } moveKind = MoveKind::NonMovedVariable;

diff --git a/samples/memleak/bad.c b/samples/memleak/bad.c
@@ -1,9 +1,11 @@
 #include <stdlib.h>
 int main()
 {
-    int result;
+    int result = 0;
     char *a = malloc(10);
-    a[0] = 0;
-    result = a[0];
+    if (a) {
+        a[0] = 0;
+        result = a[0];
+    }
     return result;
 }
diff --git a/samples/memleak/good.c b/samples/memleak/good.c
@@ -1,10 +1,12 @@
 #include <stdlib.h>
 int main()
 {
-    int result;
+    int result = 0;
     char *a = malloc(10);
-    a[0] = 0;
-    result = a[0];
-    free(a);
+    if (a) {
+        a[0] = 0;
+        result = a[0];
+        free(a);
+    }
     return result;
 }
diff --git a/samples/memleak/out.txt b/samples/memleak/out.txt
@@ -1,3 +1,3 @@
-samples\memleak\bad.c:8:5: error: Memory leak: a [memleak]
+samples\memleak\bad.c:10:5: error: Memory leak: a [memleak]
     return result;
     ^
diff --git a/test/cfg/bsd.c b/test/cfg/bsd.c
@@ -155,8 +155,10 @@ void uninitvar(void)
 void arrayIndexOutOfBounds(void)
 {
     char * pAlloc = calloc(2, 3);
+    // cppcheck-suppress nullPointerOutOfMemory
     pAlloc[5] = 'a';
     // cppcheck-suppress arrayIndexOutOfBounds
+    // cppcheck-suppress nullPointerOutOfMemory
     pAlloc[6] = 1;
     // cppcheck-suppress memleakOnRealloc
     pAlloc = reallocarray(pAlloc, 3, 3);

diff --git a/test/cfg/emscripten.cpp b/test/cfg/emscripten.cpp
@@ -63,6 +63,7 @@ void em_asm_ptr_memleak_test()
     const char *str = static_cast<char*>(EM_ASM_PTR({
         return stringToNewUTF8("Hello");
     }));
+    // cppcheck-suppress nullPointerOutOfMemory
     printf("%s", str);
 
     // cppcheck-suppress memleak
@@ -88,4 +89,4 @@ void em_js_test()
 {
     two_alerts();
     take_args(100, 35.5);
-}
+}
diff --git a/test/cfg/gnu.c b/test/cfg/gnu.c
@@ -53,6 +53,7 @@ int deallocuse_backtrace(int size) {
     void **buffer = (void **)malloc(sizeof(void *) * size);
     free(buffer);
     // cppcheck-suppress deallocuse
+    // cppcheck-suppress nullPointerOutOfMemory
     int numEntries = backtrace(buffer, size);
     return numEntries;
 }
@@ -331,6 +332,7 @@ void valid_code(int argInt1, va_list valist_arg, const int * parg)
     // cppcheck-suppress valueFlowBailoutIncompleteVar
     const void * p_mmap = mmap(NULL, 1, PROT_NONE, MAP_ANONYMOUS | MAP_SHARED, -1, 0);
     printf("%p", p_mmap);
+    // cppcheck-suppress nullPointerOutOfMemory
     munmap(p_mmap, 1);
 
     uint16_t i16_1 = 0, i16_2;
@@ -427,6 +429,7 @@ void memleak_asprintf8(const char *fmt, const int arg) // #12204
 void memleak_xmalloc()
 {
     char *p = (char*)xmalloc(10);
+    // cppcheck-suppress nullPointerOutOfMemory
     p[9] = 0;
     // cppcheck-suppress memleak
 }
@@ -464,14 +467,18 @@ void bufferAccessOutOfBounds()
     sethostname(buf, 4);
 
     char * pAlloc1 = xcalloc(2, 4);
+    // cppcheck-suppress nullPointerOutOfMemory
     memset(pAlloc1, 0, 8);
     // cppcheck-suppress bufferAccessOutOfBounds
+    // cppcheck-suppress nullPointerOutOfMemory
     memset(pAlloc1, 0, 9);
     free(pAlloc1);
 
     char * pAlloc2 = xmalloc(4);
+    // cppcheck-suppress nullPointerOutOfMemory
     memset(pAlloc2, 0, 4);
     // cppcheck-suppress bufferAccessOutOfBounds
+    // cppcheck-suppress nullPointerOutOfMemory
     memset(pAlloc2, 0, 5);
 
     pAlloc2 = xrealloc(pAlloc2, 10);

diff --git a/test/cfg/gtk.c b/test/cfg/gtk.c
@@ -46,10 +46,12 @@ void validCode(int argInt, GHashTableIter * hash_table_iter, GHashTable * hash_t
     g_printerr("err");
 
     GString * pGStr1 = g_string_new("test");
+    // cppcheck-suppress nullPointerOutOfMemory
     g_string_append(pGStr1, "a");
     g_string_free(pGStr1, TRUE);
 
     gchar * pGchar1 = g_strconcat("a", "b", NULL);
+    // cppcheck-suppress nullPointerOutOfMemory
     printf("%s", pGchar1);
     g_free(pGchar1);
 

diff --git a/test/cfg/posix.c b/test/cfg/posix.c
@@ -921,11 +921,13 @@ typedef struct {
 
 S_memalign* posix_memalign_memleak(size_t n) { // #12248
     S_memalign* s = malloc(sizeof(*s));
+    // cppcheck-suppress nullPointerOutOfMemory
     s->N = n;
     if (0 != posix_memalign((void**)&s->data, 16, n * sizeof(int))) {
         free(s);
         return NULL;
     }
+    // cppcheck-suppress nullPointerOutOfMemory
     memset(s->data, 0, n * sizeof(int));
     return s;
 }
@@ -1155,13 +1157,16 @@ int munmap_no_double_free(int tofd, // #11396
         return -1;
     }
 
+    // cppcheck-suppress nullPointerOutOfMemory
     memcpy(tptr,fptr,len);
 
+    // cppcheck-suppress nullPointerOutOfMemory
     if ((rc = munmap(fptr,len)) != 0) {
         // cppcheck-suppress memleak
         return -1;
     }
 
+    // cppcheck-suppress nullPointerOutOfMemory
     if ((rc = munmap(tptr,len)) != 0) {
         return -1;
     }
@@ -1181,6 +1186,7 @@ void resourceLeak_fdopen2(const char* fn) // #2767
     // cppcheck-suppress valueFlowBailoutIncompleteVar
     int fi = open(fn, O_RDONLY);
     FILE* fd = fdopen(fi, "r");
+    // cppcheck-suppress nullPointerOutOfResources
     fclose(fd);
 }
 
@@ -1240,8 +1246,10 @@ void resourceLeak_open2(void)
 void noleak(int x, int y, int z)
 {
     DIR *p1 = fdopendir(x);
+    // cppcheck-suppress nullPointerOutOfResources
     closedir(p1);
     DIR *p2 = opendir("abc");
+    // cppcheck-suppress nullPointerOutOfResources
     closedir(p2);
     int s = socket(AF_INET,SOCK_STREAM,0);
     close(s);

diff --git a/test/cfg/selinux.c b/test/cfg/selinux.c
@@ -80,8 +80,10 @@ void selabel_fail2(void)
     struct selabel_handle *hnd = selabel_open(SELABEL_CTX_FILE, NULL, 0);
 
     char *ctx;
+    // cppcheck-suppress nullPointerOutOfResources
     selabel_lookup(hnd, &ctx, "/", 0);
 
+    // cppcheck-suppress nullPointerOutOfResources
     selabel_close(hnd);
 
     // cppcheck-suppress memleak
@@ -92,14 +94,18 @@ void selabel_success(void)
     struct selabel_handle *hnd = selabel_open(SELABEL_CTX_FILE, NULL, 0);
 
     char *ctx;
+    // cppcheck-suppress nullPointerOutOfResources
     selabel_lookup(hnd, &ctx, "/", 0);
 
     freecon(ctx);
 
+    // cppcheck-suppress nullPointerOutOfResources
     (void)selabel_cmp(hnd, hnd);
 
+    // cppcheck-suppress nullPointerOutOfResources
     selabel_stats(hnd);
 
+    // cppcheck-suppress nullPointerOutOfResources
     selabel_close(hnd);
 }