Poor path for leak report

[Quuxplusone]

Bugzilla Link	PR15715
Status	NEW
Importance	P normal
Reported by	Alexandre Colucci (timac@timac.org)
Reported on	2013-04-10 04:26:40 -0700
Last modified on	2013-04-12 05:14:51 -0700
Version	unspecified
Hardware	Macintosh MacOS X
CC	jrose@belkadan.com, llvm-bugs@lists.llvm.org, timac@timac.org
Fixed by commit(s)
Attachments	InterproceduralBug.zip (4806 bytes, application/zip) report.html (9594 bytes, text/html)
Blocks
Blocked by
See also

Created attachment 10331
Xcode project

The Clang Static Analyzer (checker-273 and Xcode 4.6.1) reports a memory leak
for the following source code.

Steps to reproduce:
1- On 10.8.3 with Xcode 4.6.1, create a new CoreFoundation project with ARC
disabled.
2- Download the attached Xcode project or replace the main.c with a main.cpp
containing the following:

#include <CoreFoundation/CoreFoundation.h>

//
// MyCFString is a wrapper around CFStringRef.
//
class MyCFString
{
public:
    static CFStringRef EmptyCFString()
    {
        static CFStringRef kEmptyCFString = CFSTR("Empty");
        return kEmptyCFString;
    }

    MyCFString()
    {
        mRef = CFStringRef(CFRetain(EmptyCFString()));
    }

    ~MyCFString()
    {
        if(mRef != NULL)
            CFRelease(mRef);
    }

    // Adopt doesn't retain the passed CFStringRef
    void Adopt(CFStringRef s)
    {
        if (s != mRef)
        {
            if(mRef != NULL)
                CFRelease(mRef);
            mRef = s;
        }
    }

private:
    CFStringRef mRef;
};

int main(int argc, const char * argv[])
{
    MyCFString theString;

    // Create a CFStringRef
    CFStringRef adoptedString = CFStringCreateCopy(kCFAllocatorDefault,
CFSTR("This is not a leak"));

    // adoptedString is not retained by Adopt() but the MyCFString destructor will
release adoptedString
    theString.Adopt(adoptedString);

    return 0;
}

3- Run scan-build /usr/bin/xcodebuild -project InterproceduralBug.xcodeproj -
target "InterproceduralBug" build

Result:
The Clang static analyzer reports a memory leak.

[Quuxplusone]

Attached InterproceduralBug.zip (4806 bytes, application/zip): Xcode project

[Quuxplusone]

Attached report.html (9594 bytes, text/html): Static analyzer report

[Quuxplusone]

The analyzer is pedantically correct; adoptedString could equal kEmptyCFString. Your Adopt() method should release its argument if it's the same as the current string.

[Quuxplusone]

To be clear, we could do special modeling of CFSTR and CFStringCreateCopy, but in general this is a real issue with this class, even though this particular case is safe.

[Quuxplusone]

Thanks a lot for the comment. You are indeed correct. I though the Static
Analyzer would be able to detect that both CFStringRef are different but that's
not the case.

I really don't think that you should do something special for CFSTR and
CFStringCreateCopy. I could reproduce the same issue with any CoreFoundation
objects (CFURLRef, ...).

Your suggestion of releasing the argument of the Adopt() method is not
something I completely like. I agree that this is theoretically what should be
done but I think that passing the same string should be considered as a
programming error. I am working on a project with a huge codebase. Changing the
Adopt() method to always release the parameter could led to crashes if some
code incorrectly uses the Adopt() method.

I ended up with the following solution. I only release the parameter in the
DEBUG build and also force a crash. This way the Static Analyzer doesn't report
a memory leak and I would catch the possible programming errors. The release
build would not be affected and I prefer to leak a string rather than crashing.

void Adopt(CFStringRef s)
{
    if (s != mRef)
    {
        if(mRef != NULL)
            CFRelease(mRef);
        mRef = s;
    }
#if DEBUG
    else
    {
        // Prevent the Clang Static Analyzer to report a memory leak.
        CFRelease(s);

        // Force a crash to detect programming errors.
        int *a = (int*)1;
        *a = 2;
    }
#endif
}

I think you can close the bug. There are only one improvement in the Static
Analyzer I could see to help with such cases. You could improve the workflow
displayed. Right now the Static Analyzer gives me the workflow:

- Call to function 'CFStringCreateCopy' returns a Core Foundation object with a
+1 retain count
- Object leaked: object allocated and stored into 'adoptedString' is not
referenced later in this execution path and has a retain count of +1

It doesn't tell me that it don't take the if branch in the Adopt() method.
I wouldn't have reported this issue if the workflow was:

- Call to function 'CFStringCreateCopy' returns a Core Foundation object with a
+1 retain count
- Taking false branch (in the Adopt() method)
- Object leaked: object allocated and stored into 'adoptedString' is not
referenced later in this execution path and has a retain count of +1

Thanks again,
Alexandre

[Quuxplusone]

Let's keep this bug open for the poor path, then. This is one in a common family of problems.

By the way, the analyzer understands assert() (and abort() or exit()), so you can use either of those to silence the issue, even without the extra CFRelease().

[Quuxplusone]

I decided to use the CFRelease call even in the release build. I discovered 2 cases in our source code where the Adopt() function was incorrectly called with a not CFRetained parameter. I believe that the Clang Static Analyzer might find these issues.

I also noticed that there were some real memory leak in the case of constant CFString: CFStringCreateCopy and similar functions have optimizations for constant strings. Instead of returning a new string, the string is simply retained. This caused the Adopt function to leak the constant string because the 'else' case was taken.