0

I just ran into an issue with the XCode static code analyzer of XCode 3.2. It shows me a potential leak which I find is not justified. I just want to check with other people and make sure it really is a false positive.

Here is the main code (in some function body):

NSError *error = nil;
NSData *urlData = /* ... */;
NSXMLDocument *doc = [[NSXMLDocument alloc] initWithData:urlData options:0 error:&error];
if (![self validateObject:doc withError:error]) {
    return;
}
// ...
[doc release];

Here is the validate method called above:

- (BOOL)validateObject:(id)object withError:(NSError *)error {
    if (!object) {
        // do something meaningful...
        return NO;
    } else {
        return YES;
    }
}

XCode tells me that the allocation of doc is a potential leak as the validate method could return NO and release would not be sent to doc. But as a matter of fact, if the initialization fails, nil is returned by initWithData:options: and no harm is caused. The docs even state this.

So, what do the experts say? False positive or not?

Best, Harald

Harald
  • 41
  • 1

3 Answers3

2

It's not a false positive, as it's doing the right thing. But you're right that in practice, you won't actually leak. However, you can silence the analyse by putting a [doc release] before the premature return, or autorelease the NSXMLDocument and not explicitly release it at the end of the method.

The reason it's not a false positive is that the analyser will not look into what validateObject:withError: does. The point of the analyser is to analyse each method individually and not assume anything about the methods it calls (apart from standard Cocoa naming, i.e. new*, alloc/init*, copy*, etc - the usual thing you read about with ARC). Imagine you changed subtly what validateObject:withError: did. Then you might end up with a leak. It's warning you to change your code so that it's safe.

As @JeremyP puts it (much better than I):

"The code breaks encapsulation. You have to know the internals of -validateObject:withError: to understand the top code snippet is not broken."

mattjgalloway
  • 34,792
  • 12
  • 100
  • 110
1

The static analyzer in Xcode does not recognize this because it can only detect issues within a method, not across different methods. So it doesn't look in validateObject what it does.

If you would change the implementation of validateObject, the leak could occur. You should not rely on the implementation of validateObject.

You should fix this potential leak by autoreleasing the NSXMLDocument.

NSXMLDocument *doc = [[[NSXMLDocument alloc] initWithData:urlData options:0 error:&error] autorelease];
Felix
  • 35,354
  • 13
  • 96
  • 143
1

The static analyzer is not smart enough to know that your validation method always returns YES when the object is not nil. And I wouldn't rely on that either. It's quite likely that you expand the logic of your validation method in the future, so that even objects that aren't nil wouldn't necessarily pass validation. In that case, you would introduce a leak. As sending a message to nil is a no-op, I would release (or autorelease) doc in any case.

omz
  • 53,243
  • 5
  • 129
  • 141
  • It's not really that it's not smart enough. It's by design that it's like that. Also, sending a message to nil isn't quite a no-op. It doesn't do much, but obviously it has to at least do a single comparison against `0` and a branch/return :-). – mattjgalloway Mar 12 '12 at 16:24
  • I meant no-op more in the sense of "nothing happens" (as opposed to a null pointer exception being thrown), but yes, it's technically not entirely correct. – omz Mar 12 '12 at 21:04
  • Yeh. Sorry, I was in pedantic mode I think :-). I was thinking of writing something similar in my answer and moved away from it, which I think put me into pedantic mode. – mattjgalloway Mar 12 '12 at 21:07