IPA: false positive due to failing to catch aliasing where receiver of message is returned to caller

[Quuxplusone]

Bugzilla Link	PR3871
Status	CONFIRMED
Importance	P normal
Reported by	Kevin (kevin562@gmail.com)
Reported on	2009-03-24 10:14:53 -0700
Last modified on	2010-03-06 13:59:14 -0800
Version	unspecified
Hardware	Macintosh MacOS X
CC	kremenek@apple.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments	ClangFactoryTest.zip (11584 bytes, application/zip)
Blocks
Blocked by	PR3891
See also

When there is a switch statement which does an alloc, the analyzer loses track
of the retain count at the break statement even though the mthod is returning
the object.  Quick Example:

+ (IncomingMessage *) allocParsedMessage:(uint8_t) input {
    IncomingMessage * result = nil;
    switch(input) {

    ***Control jumps to 'case 1:' ***
      case M_I_RESPONSESETUP:
    result = [[IncomingMsgResponseSetup alloc]initWithDataInput:input];

    ***Method returns an Objective-C object with a +1 retain count (owning
reference)***
    break;

    ***Execution continues on line 200 ***

    ***Object allocated on line 59 is no longer referenced after this point and
has a retain count of +1 (object leaked)***

      case M_I_RESPONSELOADING:
    result = [[IncomingMsgResponseLoading alloc]initWithDataInput:input];
    break;
   }
   return result;   //OBJECT RETURNED TO CALLER DOWN HERE SO ITS NOT LEAKED
}

[Quuxplusone]

Kevin: Is it possible to provide a self-contained example that exhibits this problem? I have not seen this before.

[Quuxplusone]

Turns out I forget the key element that makes this happen.  You have to call
another function on the object for it to happen.  So in my previous case the
line should be:
 case 1:
 result = [[[IncomingMsgResponseSetup alloc]initWithDataInput:input] read];
 break;

I will attach a simple IPhone project that shows it shortly

[Quuxplusone]

Attached ClangFactoryTest.zip (11584 bytes, application/zip): Iphone example of issue

[Quuxplusone]

The actual code really helped. The false positive actually has nothing to do with the 'switch' statement. The problem is the analyzer currently doesn't perform any inter-procedural analysis (IPA), so it doesn't realize that 'read' simply returns the receiver. The analyzer assumes that a new object is returned and thus the original object referenced is leaked.

False positives like these will simply disappear once we add IPA. In this case it should be so bad because (a) the example is simple and (b) the definition of 'read' occurs in the same file. To do this right over an entire codebase we would need infrastructure for whole-program analysis; this means that when we analyze a caller of a function/method the analyzer knows about the definition of the function/method even if it is in a separate source file.

A possible workaround is for us to allow source-level annotations to describe the aliasing relationship here, but that seems too burdensome for something the analyzer should do. The problem here is that the public interface for 'read' (in @interface) doesn't really imply that the message 'read' returns the receiver; in this manner it actually slightly violates the Cocoa API conventions (which the analyzer currently strictly follows), or at least it isn't obvious to other clients (e.g., developers) that 'read' behaves this way. This isn't a criticism of the code. Basically, because the analyzer doesn't do IPA yet, it assumes that code is structured a certain way. If your code isn't structured that way, it basically doesn't yield effective results.

[Quuxplusone]

I've added this to the basket of bugs for PR 3891. That bug tracks the progress of developing inter-procedural analysis in the analyzer.