False positive: Potential leak of memory [unix.malloc]

[josephcsible]

I'm running version 14.0.0 obtained from https://github.com/llvm/llvm-project/releases/download/llvmorg-14.0.0/clang+llvm-14.0.0-x86_64-linux-gnu-ubuntu-18.04.tar.xz

Consider this C program:

#include <stdlib.h>
#include <string.h>

struct mystruct {
    void *ptr;
    char arr[4];
};

int main(void) {
    struct mystruct x;
    x.ptr = malloc(1);
    strcpy(x.arr, "hi\n");
    free(x.ptr);
}

When I run clang --analyze /tmp/bug.c, I get this:

/tmp/bug.c:13:5: warning: Potential leak of memory pointed to by 'x.ptr' [unix.Malloc]
    free(x.ptr);
    ^~~~~~~~~~~
1 warning generated.

But it's clear that there's not actually a potential leak of memory here.

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

[Snape3058]

Confirmed to be a bug.

CStringChecker.cpp:1010 (CStringChecker::InvalidateBuffer), as the Size expr is not available, we will invalidate the entire structure, which makes the Store binding of x.ptr to the return value of malloc removed.
IMO, we can fix this problem by further confirming whether the sizes of both the source and destination are concrete or comparable via constraints, even though unbounded versions. If so, we should only invalidate the region instead of the entire structure. And other similar places also need to be checked for this checker.

Besides, should we add a mechanism to notify the MallocChecker that a heap object is disconnected from a region via region invalidation? Or has there already been such a mechanism?

And, any other suggestions for the fix I proposed?

[haoNoQ]

I confirm this understanding! It's likely that the model for strcpy() is misbehaving.

Besides, should we add a mechanism to notify the MallocChecker that a heap object is disconnected from a region via region invalidation? Or has there already been such a mechanism?

Yes, right, without such mechanism nothing would work. This mechanism is called pointer escape. A pointer is said to escape if exclusive ownership of the pointer by the code under analysis can no longer be proven. Checkers are notified that their pointers are escaping via checkPointerEscape() and checkConstPointerEscape(). When the checker updates the Store it's supposed to notify other checkers that any pointers in those regions potentially escape. For example,

ProgramState::bindLoc(Loc location, SVal V, const LocationContext *LCtx, bool notifyChanges=true)

would notify other checkers if notifyChanges is true. Similarly, invalidateRegions() accepts parameter CausesPointerEscape (I guess we could use some consistency here).

[Snape3058]

Sorry for the delay. A fix to this bug is not being discussed with differential https://reviews.llvm.org/D152435

[steakhal]

I felt weird to have FPs of this kind for to aggressive invalidation, and I still share this feeling.
IMO if invalidation happens, the Malloc checker should evict information about the escaped regions/symbols, hence later in checkDead it shouldn't find any of the escaped symbols to report in the first place.

I saw, that the Malloc checker only evicts symbols in case of a direct invalidation, but keeps them in case of an indirect one. The reasoning there according to the git blame looks solid.

During the investigation I noticed a strange event sequence:

1. The MallocChecker sees the malloc(1) and binds &HeapSymRegion{conj_$2{void *, LC1, S1114, #1}} as the returned value, and maps the conj_$2 -> Allocated.
2. Just before the PostStmt<free(x.ptr)> gets conservatively evaluated, the PreStmtPurgeDeadSymbols event kicks in which triggers the MallocChecker::checkDead marking conj_$2 dead, as that's the last place it's read.
3. Inside MallocChecker::checkDead, we see that conj_$2 is in Allocated state, so it reports itas a leak!
4. After PreStmtPurgeDeadSymbols, the conservative eval call of free invalidates the content of x.
5. When we get to the PostStmt<free(x.ptr)>, finally the MallocChecker::checkPostCall kicks in and marks the region pointed by the freshly invalidated x.ptr (aka. derived_$7{conj_$4{int, LC1, S1176, #1},x.ptr}) as Released.

IMO the intent of the MallocChecker::checkPostCall would be to cleanse the pointer, so that we don't get this leak report. So, this leaves me wondering if there is a logic bug here.

By looking at the order of these events, I'm not sure how the CStringChecker interleaves there, but I can also see that if I replace the strcpy with some random opaque call, the issue disappears.
Maybe the kind of the invalidation that the CStringChecker models is the culprit, as the malloc checker only respects direct invalidations?
Could you @Snape3058 investigate if the MallocChecker does the right thing for the example in the presence of the strcpy before potentially hiding the primary reason by patching the CStringChecker?

[Snape3058]

but I can also see that if I replace the strcpy with some random opaque call, the issue disappears.

When calling some other opaque functions (e.g. foo(x.arr), if I get you correctly), ExprEngine::conservativeEvalCall -> CallEvent::invalidateRegions -> ProgramState::invalidateRegions will invalidate the entire x object through x.arr with CausedByPointerEscape set to true. This makes the state of the heap conjured symbol transit to Escaped during invalidation. However, in CStringChecker::InvalidateBuffer, the CausedByPointerEscape parameter is set to false when invalidating object x through x.arr under Non-RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion cases, which will not mark the heap conjured symbol escaped, and hence lead to the false positive report.

In addition to updating the size value, I also set CausedByPointerEscape to true for the cases that we do not want to report a leak.

The event sequence you listed above is correct. The misleading part may be the free call after strcpy. You can replace it with another opaque call unrelated to x (e.g. bar();) to see if it makes sence.

1. The MallocChecker sees the malloc(1) and binds &HeapSymRegion{conj_$2{void *, LC1, S1114, #1}} as the returned value, and maps the conj_$2 -> Allocated.
2. After strcpy call, x is invalidated. And hence there is no region binding to conj_$2. (This is the key step of generating the FP. By setting the CausedByPointerEscape parameter to true, conj_$2 will be escaped during invalidation; whereas under TK_DoNotInvalidateSuperRegion case, x.ptr will not be invalidated and bound to conj_$2. Unfortunately, in the trunk version, the CausedByPointerEscape parameter is set to false together with TK_DoNotInvalidateSuperRegion seldom triggers unless a concrete copy length is given via arguments of the function call, such as the size argument of memcpy)
3. Then, before bar(), the PreStmtPurgeDeadSymbols event kicks in which triggers the MallocChecker::checkDead marking conj_$2 dead, and MallocChecker::checkDead reports conj_$2 leaks.
4. After PreStmtPurgeDeadSymbols, the conservative eval call of bar() does nothing to x (aka the invalidation-assigned values).
5. Finally, call to free(x.ptr) is evaluated, and marks the derived_$7{conj_$4{int, LC1, S1176, #1},x.ptr} symbol as released.

I am not very familiar with the MallocChecker, but the event sequence makes sense to me.

[steakhal]

Fixed by 1bd2d33