False Positive: Non-Pointer resources deallocated in destructor

[Aaron Danen]

Hi all

This is my first time posting here so apologies if I mess something up. Also I am very junior (first internship) so I am sorry if I make silly mistakes!

I was using cppcheck on a codebase and found a recurring issue where if memory or a resource is stored in a class/struct, cppcheck will throw a memory leak error even if its deallocated in the objects destructor.

#include <cstdlib>
#include <unistd.h>

class TempFile {
public:
    TempFile(int fd) {
        m_fd = fd;
    }

    ~TempFile() {
      if (m_fd >= 0)
         close(m_fd); 
    }

private:
    int m_fd = -1;
};

TempFile allocateFile(char *name) {
    return TempFile(mkostemp(name, 0));
}

int main() {
    char *ptr = (char *)malloc(10*sizeof(char));
    if (!ptr)
        return 1;

    TempFile f = allocateFile(ptr);
    free(ptr);
    return 0;
}

From my understanding of c++, the destructor of f will be called when main() returns. This would close the file allocated by mkostemp()

Actual result
error: Allocation with mkostemp, TempFile doesn't release it. [leakNoVarFunctionCall]
return TempFile(mkostemp(name, 0));

Expected result
no errors

However, I don't get any issues with a more simple case like this:

#include <cstdlib>

class TempObj {
public:
    TempObj(void *p) {
        ptr = p;
    }

    ~TempObj() {
        if (ptr)
            free(ptr);
    }

private:
    void *ptr = NULL;
};

TempObj allocateMem(int sz) {
    return TempObj(malloc(sz));
}

int main() {
    TempObj t = allocateMem(42);
    return 0;
}

Verification
The issue was produced with:
./cppcheck --library=cfg/gnu.cfg test.cpp
Version: 2.22 dev

path to a solution:
in checkmemoryleak.cpp I found the following:

 void CheckMemoryLeakInClassImpl::check() {
    logChecker("CheckMemoryLeakInClass::check");

    const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
    // only check classes and structures
    for (const Scope * scope : symbolDatabase->classAndStructScopes) {
        for (const Variable &var : scope->varlist) {
              if (!var.isStatic() && (var.isPointer() || var.isPointerArray())) {
                  // allocation but no deallocation of private variables in public function..
                const Token *tok = var.typeStartToken();
                // Either it is of standard type or a non-derived type
                if (tok->isStandardType() || (var.type() && var.type()->derivedFrom.empty())) {
                    if (var.isPrivate())
                        checkPublicFunctions(scope, var.nameToken());
                    variable(scope, var.nameToken());
                }
            }
        }
    }
}

cfg/gnu.cfg defines a resource (the file descriptor) that is allocated by mkostemp() and needs to be "freed" by close(). However, since the var is not a pointer or an array of pointers (its an integer), the if condition

 if (!var.isStatic() && (var.isPointer() || var.isPointerArray()))

blocks the analysis and variable(scope, var.nameToken()) never gets called, so we never inspect the destructor to see if it gets closed.

it seems a similar issue was fixed in
https://github.com/cppcheck-opensource/cppcheck/pull/3832
but I think we need maybe a more general solution, since the library system allows "resources" to be any datatype.

If its helpful I can try to draft a PR but I have never contributed to cppcheck before. I would be happy to review the guidelines and work on it but if you already have something in mind, perhaps its best for you to just fix it.

Thanks for your time,
Aaron

[CHR]

Thanks for reporting, ticket is here: https://trac.cppcheck.net/ticket/14949
Feel free to open a PR, you seem to have the problem figured out already.

[Aaron Danen]

I made a PR: https://github.com/cppcheck-opensource/cppcheck/pull/8765

Thanks!
Aaron