Analyzer claims to have found potential memory leak, yet it clearly is not a leak

[CodingMarkus]

According to clang analyzer, the following code is a potential leak:

guard_C (certRef, SecCertificateCreateWithData(NULL, derData)) else return nil;
__auto_type const instance = [self initWithCertificate:certRef];
CFRelease(certRef);
return instance;

The guard_C macro expands as follows:

__auto_type const v_217 = (SecCertificateCreateWithData(NULL, derData)); 
__auto_type const n_217 = (typeof(* v_217) *_Nonnull)1; 
__auto_type const certRef = (!v_217 ? n_217 : (typeof(* v_217) *_Nonnull)v_217); 
if (certRef != n_217) { } else return nil;

This is clearly not a leak. return nil is only executed if SecCertificateCreateWithData() returned NULL.

More precisely, it is only executed if certRef == n_217 and that is only the case if v_217 is NULL.

If v_217 is not NULL, certRef == v_217, so certRef != n_217 is true, the else part is not executed, the code continues below the if and the result of SecCertificateCreateWithData() is released.

[llvmbot]

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

Author: None (CodingMarkus)

According to clang analyzer, the following code is a potential leak: ``` guard_C (certRef, SecCertificateCreateWithData(NULL, derData)) else return nil; __auto_type const instance = [self initWithCertificate:certRef]; CFRelease(certRef); return instance; ``` The `guard_C` macro expands as follows: ``` __auto_type const v_217 = (SecCertificateCreateWithData(NULL, derData)); __auto_type const n_217 = (typeof(* v_217) *_Nonnull)1; __auto_type const certRef = (!v_217 ? n_217 : (typeof(* v_217) *_Nonnull)v_217); if (certRef != n_217) { } else return nil; ``` This is clearly not a leak. `return nil` is only executed if `SecCertificateCreateWithData()` returned `NULL`. More precisely, it is only executed if `certRef == n_217` and that is only the case if `v_217` is `NULL`. If `v_217` is not `NULL`, `certRef == v_217`, so `certRef != n_217` is true, the else part is not executed, the code continues below the if and the result of `SecCertificateCreateWithData()` is released.

[steakhal]

Could you please post a reproducer using godbolt.org? Or alternatively, a step by step reproduction, including the compiler invocation.

[CodingMarkus]

Could you please post a reproducer using godbolt.org?

How can I use the analyzer at godbolt.org? And how can I use Apple's security framework there?

The issue is from a huge Xcode project (the source code alone is 91 MB, the Git repo is 1.3 GB). The file where the issue is reported has lot's of dependencies and the compiler invocation is also huge. I doubt any of this would be useful to you as you won't be able to reproduce anything from that information.

If it is possible to build a minimal test case using godbolt.org, that's probably the better option, I just don't know how I can use the analyzer there. I guess I can fake the security framework call and replace it by a call of my own that just has the same signature.

[steakhal]

Could you please post a reproducer using godbolt.org?

How can I use the analyzer at godbolt.org? And how can I use Apple's security framework there?

The issue is from a huge Xcode project (the source code alone is 91 MB, the Git repo is 1.3 GB). The file where the issue is reported has lot's of dependencies and the compiler invocation is also huge. I doubt any of this would be useful to you as you won't be able to reproduce anything from that information.

If it is possible to build a minimal test case using godbolt.org, that's probably the better option, I just don't know how I can use the analyzer there. I guess I can fake the security framework call and replace it by a call of my own that just has the same signature.

Here is a link to an editor. Once you are done, hit "share" and paste it back here. A preprocessed file is also fine, whatever large it is. Just compress it and we will deal with it (assuming you also share the clang invocation along with it).

As you said, usually frameworks can be mocked by adding just a declaration. Note that attributes there might be also important, so copy those as well.

[CodingMarkus]

I guess a minimal test case is something like this:

#include <stdlib.h>
#include <stdio.h>

#define nil NULL

typedef struct SecCertificate { } * SecCertificateRef;
typedef struct CFData { } * CFDataRef;

void CFRelease( __attribute__((cf_consumed)) void * ptr ) { }

__attribute__((cf_returns_retained))
SecCertificateRef _Nullable SecCertificateCreateWithData(
    void * allocator, CFDataRef data )
{
    return malloc(sizeof(struct SecCertificate));
}

void * triggerIssue( void )
{
    __auto_type derData = (CFDataRef)nil;
    __auto_type const v_217 = (SecCertificateCreateWithData(NULL, derData)); 
    __auto_type const n_217 = (typeof(* v_217) *_Nonnull)1; 
    __auto_type const certRef = (!v_217 ? n_217 : (typeof(* v_217) *_Nonnull)v_217); 
    if (certRef != n_217) { } else return nil;

    CFRelease(certRef);
    return "test";
}

int main( void )
{
    printf("%p\n", triggerIssue());
    return 0;
}

Here's the godbolt version of it (with two tiny changes, to make C++ happy): \ https://godbolt.org/z/E3xnase3W

[steakhal]

I had a look,and it seems like the pointer that malloc returns we assume that it might alias with (typeof(* v_217) *_Nonnull)1. Consequently, we might reach the return nil within if (certRef != n_217) { } else return nil;

I tested the theory by having this code in between the allocation and the check:

if (v_217 == (SecCertificate*)1) {
  free(v_217);
  return;
}

This suppressed the leak issue, but now it complained about Argument to free() is a constant address (1), which is not memory allocated by malloc() [unix.Malloc].

Funnily, if I omit the free(v_217), then it complains about leaking there :D So, we definitely have some disharmony, but I don't think this is a big deal to be honest.

---

To resolve this FP, I'd suggest to use a simpler coding pattern: Just avoid the use of hard-coded sentinel values (such as ((void*)-1), and alike), and just check the malloced pointer for nullness.

auto p = alloc();
if (!p) return;
use(p);
free(p);

[CodingMarkus]

The original implementation of guard_C was this:

#define guard_C( var, value )   \
    __auto_type var = (value);  \
    if (var) { }

But that doesn't work anymore, as now when using var in the code below, clang will complain that var might be NULL, despite the fact that it cannot in cases where the code below is only executed when it surely is not but clang no longer understands that.

See also https://gist.github.com/robb/d55b72d62d32deaee5fa#gistcomment-4823609

The trick with n_217 is only there, so that clang will consider the final value to be _Nonnull.

[steakhal]

I don't understand what's the purpose of if (var) {} within the macro guard_C. The miscommunication might be because I've never touched any Objective-C code, and I only have very limited understanding about that language.

If this had a different behavior with earlier clang versions, it's likely that some "recent-ish" commit changed it. I can recall @tripleCC submitted some Objective-C patches about half year ago. Patches.

Other than this, I don't think I can help you.

[CodingMarkus]

The purpose of if (var) {} is to mimic a Swift guard statement:

Swift:

guard let value = optionalValue else { /* ... */ }

Compare to:

guard_C (value, optionalValue) else { /* ... */ }

In the past the following code would work as expected in clang:

#include <stdio.h>

void print( const char *_Nonnull str )
{
    printf("%s\n", str);
}

void printUnlessNull( const char *_Nullable maybeStr )
{
    if (maybeStr) print(maybeStr);
}

int main( int argc, char **argv )
{
    printUnlessNull(argv[0]);
    return 0;
}

But now this code produces the following error:

test.c:12:22: warning: implicit conversion from nullable pointer 'const char * _Nullable' to non-nullable pointer type 'const char * _Nonnull' [-Wnullable-to-nonnull-conversion]
        if (maybeStr) print(maybeStr);

Despite the fact, that print() is never called if the argument was NULL and when print() is called, the argument is definitely not NULL. That's why this code also won't work:

__auto_type value = ...;
if (!value) { /* ... */; return; }
print(value);

As then the compiler will complain, that value could be NULL, despite the fact that it cannot be NULL, as it will never reach that statement when it was NULL.

The guard_C macro eliminates the problem:

guard_C (value, ...) else { return; }
print(value); // Compiler is certain, that `value` is _Nonnull!

This works as expected and produces no warning in the compiler. It's just that analyzer that cannot cope with it but the analyzer should be able to cope with that as static code analysis should tell the analyzer, that the only way how this function could return early is when ... returned NULL and if that is the case, there is nothing to release, hence there cannot be a leak.