macOS XCode 3.2 Static Analysis

[Soulstorm]

I have been experimenting with static analysis on my machine, and I have the impression that although it is a great piece of work, it doesn't always find the correct errors.

For example, I have this code:

- (NSString *)generateSoundID
{
	NSString *result = nil;
	NSFileManager *fm = [NSFileManager defaultManager];
	
	NSAutoreleasePool *pool = [[NSAutoreleasePool alloc]init];
	
	NSString *soundsDirectory = [[SFGlobals sharedSFGlobals]applicationSoundsFolder];
	unsigned i = 0;
	BOOL shouldContinueLooping = YES;
	do {
		NSString *nameToTest = [NSString stringWithFormat:@"%@%u.%@", SOUND_FILE_PREFIX, i, SOUND_FILE_SUFFIX];
		NSString *locationToTest = [soundsDirectory stringByAppendingPathComponent:nameToTest];
		NSLog(@"now checking: %@", locationToTest);
		if (![fm fileExistsAtPath:locationToTest]){
			result = [nameToTest retain];
			shouldContinueLooping = NO;
		}
		i++;
	} while (shouldContinueLooping == YES);
	
	[pool release];
	return result;
}

The code searches for an available name of type "soundXX.wav" (where XX is a 2 digit number) on the file system, and when it finds it, it returns it. Static analyzer rings the bell on this one, and complains that there is a possible memory leak on "nameToTest".

Maybe this is my own fault, but I fail to see the memory leak here. I allocated nameToTest, retained it, released it (since I release the Autorelease pool) and returned it from my function. Shouldn't it be autoreleased afterwards by cocoa since it is an autoreleased object?

I would really like someone to explain this to me, as this may save me a lot of trouble in the future.

 

[Catfish_Man]

So, nameToTest initially has a retain count of 0 (1 + -1 from the implicit autorelease). Then you retain it, leaving it at a net retain count of +1. Then you return it, still at +1. That seems like a memory leak to me.

 

[Ritsuka]

Yes it's autoreleased, but you retained it, so the ref count won't be 0 and nameToTest won't be dealloced. Autorelease just decrease the ref count by 1. If the ref count is more then 1 you need another release.

 

[whooleytoo]

Yup, I think "return [result autorelease];" should do it.

 

[Soulstorm]

So, nameToTest initially has a retain count of 0 (1 + -1 from the implicit autorelease). Then you retain it, leaving it at a net retain count of +1. Then you return it, still at +1. That seems like a memory leak to me.

Oh, so when an object is autoreleased, it must still get an -autorelease message in order to be released in the next autorelease pool release?

 

[Catfish_Man]

Oh, so when an object is autoreleased, it must still get an -autorelease message in order to be released in the next autorelease pool release?

No, that's not what I said. The problem is that you have +2 (allocation, and retain) and only -1 (autorelease). You need another release or autorelease to balance.

 

[gnasher729]

No, that's not what I said. The problem is that you have +2 (allocation, and retain) and only -1 (autorelease). You need another release or autorelease to balance.

I suppose that is the intent of the function, to return an NSString* with a retain count of one (the result of the retain method is stored in result and returned). The method should probably have a different name, like "createSoundID" or "copySoundID", because it behaves like a "create" or "copy" method. Would be interesting to see if changing the name of the method would make a difference.

 

[Catfish_Man]

It would; clang's static analyzer understands Cocoa naming conventions.

 

[whooleytoo]

It would; clang's static analyzer understands Cocoa naming conventions.

Interesting, sounds like it's quite powerful then.

I assume so that it's clever enough to recognise that having the line retaining nameToTest within a loop is fine (since that's only executed once)?

 

[Catfish_Man]

Yup. It can't do cross-function analysis (the naming convention awareness is sort of a workaround for this... sort of) at this point, but within a function it does do data-dependent analysis, so it can tell that that path will exit the loop.

 

[Soulstorm]

Thanks a lot, guys, I placed a -autorelease message in the end of the function, and static analysis stopped complaining.

Actually, static analysis seems like a very powerful tool. It's one of the reasons I made the switch to Leopard almost right away, because I had the intention of waiting for Leopard 10.6.2 to make the upgrade.

 

[gnasher729]

Yup. It can't do cross-function analysis (the naming convention awareness is sort of a workaround for this... sort of) at this point, but within a function it does do data-dependent analysis, so it can tell that that path will exit the loop.

I did some experiments: It assumes that functions starting with Create or Copy return an object that is retained and should be released, functions starting with Get return an object that isn't yours and shouldn't be released, and anything else is assumed to have unknown behaviour, so the analyser can't warn you. This means you should rename your functions accordingly.

I also checked that the analyser doesn't look into static inline functions, even if the compiler does. So you can't write a function that will release something. For example a replacement for CFRelease that doesn't crash when the object released is NULL must be written as a macro.

Thanks a lot, guys, I placed a -autorelease message in the end of the function, and static analysis stopped complaining.

As usual, the idea is not to fix compiler warnings, but to make the code correct. Did you want that function to return an autoreleased string or a retained string? If you wanted it to create a retained string then change the function name, not the code.

 

[kainjow]

I did some experiments: It assumes that functions starting with Create or Copy return an object that is retained and should be released, functions starting with Get return an object that isn't yours and shouldn't be released, and anything else is assumed to have unknown behaviour, so the analyser can't warn you. This means you should rename your functions accordingly.

This does work nicely for functions, but not so much for methods. If you have a method that returns a retained object, Clang will only ignore it if it starts with copy, but not create, which as a result causes a warning with the createViewController method in Apple's QCPlugIn class. You *could* use the NS_RETURNS_RETAINED macro but I wish the "create" suffix was supported in methods.

I also found that if you use underscores instead of camel case for init methods, it doesn't understand that. For example init_with_something vs initWithSomething.