[Lint] Check that each object variable is set to nil after it has been released

[llvmbot]

Bugzilla Link	2587
Version	unspecified
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@tkremenek,@haoNoQ

Extended Description

It's a common and recommended idiom in ObjC to set ivar object pointers to nil after they have been released. Usually this happens in dealloc method (in non-GC mode), but sometimes ivars may be released in other instance methods as well. Setting object pointers to nil after release makes code safer, since subsequent methods which are sent to released objects don't cause undefined behaviour.

As this is only a defensive programming practise and missing nil assignment after release is not a bug, this analyzer check could be optional and not enabled by default in the scan-build script.

More strict version of this check could require that all object pointers (including local variables) should be set to nil after the release message.

Some examples:

-check-nillify-object-ivars-after-release

@​interface MyClass : NSObject { NSString _myIvar; } @​end @​implementation MyClass -(void)method1 { NSString str1 = [[NSString alloc] initWithString:@"Hello world"]; NSLog(@"%@", str1); [str1 release]; // no-warning (local object pointer released) }

-(void)dealloc { [_myIvar release]; // expected-warning: 'missing nil assignment' (ivar object pointer released) } @​end

-check-nillify-object-vars-after-release

@​interface MyClass : NSObject { NSString _myIvar; } @​end @​implementation MyClass -(void)method1 { NSString str1 = [[NSString alloc] initWithString:@"Hello world"]; NSLog(@"%@", str1); [str1 release]; // expected-warning: 'missing nil assignment' (local object pointer released) }

-(void)dealloc { [_myIvar release]; // expected-warning: 'missing nil assignment' (ivar object pointer released) } @​end

[haoNoQ]

Re-screening old bugs :)

This one's easy to implement given all the progress that we've made since 2008, but being an idea for a lint-like check, this is unlikely to be prioritized, because we don't have any user model for lint checks, and they are perceived to have less value than checks that are designed to find only real bugs. Though we could totally go for it eventually.

[tkremenek]

Can you clarify this last point? I still think this might be a useful check, even if we just restrict ourselves to a small set of cases where we know we won't get many false positives.

Well, we should at least check that basic setters don't cause any false positive reports. For example:

-(void) setMyIvar:(id)newValue { if(_myIvar != newValue) { [_myIvar release]; _myIvar = [newValue retain]; } }

Here _myIvar is not set to nil after release. But this case is probably quite easy to catch, since it's set to something else in the same context.

I think cases like this could easily be handled using a live variables analysis (which we already use for other things, like the dead store checker). This falls in with my point about only needing to set variables to nil whose values "escape."

In this case, after [_myIvar release] the current value os _myIvar is no longer live, so there is no reason to set it to nil.

[llvmbot]

Can you clarify this last point? I still think this might be a useful check, even if we just restrict ourselves to a small set of cases where we know we won't get many false positives.

Well, we should at least check that basic setters don't cause any false positive reports. For example:

-(void) setMyIvar:(id)newValue { if(_myIvar != newValue) { [_myIvar release]; _myIvar = [newValue retain]; } }

Here _myIvar is not set to nil after release. But this case is probably quite easy to catch, since it's set to something else in the same context.

[tkremenek]

@​interface MyClass : NSObject { NSString *_myIvar; } @​end @​implementation MyClass -(void)method1 { [_myIvar release]; // expected-warning: 'missing nil assignment' (ivar object pointer released) } @​end

Hmm. Actually, no. This will cause a lot of false positives, e.g. in setters. Ok, maybe this suggestion should be ignored completely.

Can you clarify this last point? I still think this might be a useful check, even if we just restrict ourselves to a small set of cases where we know we won't get many false positives.

[llvmbot]

@​interface MyClass : NSObject { NSString *_myIvar; } @​end @​implementation MyClass -(void)method1 { [_myIvar release]; // expected-warning: 'missing nil assignment' (ivar object pointer released) } @​end

Hmm. Actually, no. This will cause a lot of false positives, e.g. in setters. Ok, maybe this suggestion should be ignored completely.

[llvmbot]

For example, in the case of -dealloc, assigning nil to ivars seems unnecessary since after -dealloc returns those ivars are no longer available.

Yes, that's true. There's certainly some disagreement about setting ivars to nil in dealloc. Personally, I think of it as a clean-up, so if I'm in gdb and I see my objects with nil ivars being referenced, I know I'm referencing deallocated objects. Of course, there's NSZombie and friends for those purposes, too. But I agree that this is probably not something that should be implemented in the analyzer.

I think it makes since to assign nil to a variable/ivar when its "escapes" the current context, and thus has the danger of being used again.

Exactly, for example:

@​interface MyClass : NSObject { NSString *_myIvar; } @​end @​implementation MyClass -(void)method1 { [_myIvar release]; // expected-warning: 'missing nil assignment' (ivar object pointer released) } @​end

[tkremenek]

This seems like a nice defensive programming style guide, although it probably is only useful a variable (such as an ivar) could later be referenced later. There are many obvious cases where a variable can never be referenced later or we can easily tell that it isn't referenced later. For such cases this check would likely produce warnings too spurious for many people.

For example, in the case of -dealloc, assigning nil to ivars seems unnecessary since after -dealloc returns those ivars are no longer available.

In the case of local variables within a method, the retain/release check in the static analyzer can detect many use-after-release errors, and certainly if a variable is never even referenced after it is released then assigning nil to it just becomes a dead store.

I think it makes since to assign nil to a variable/ivar when its "escapes" the current context, and thus has the danger of being used again.

[llvmbot]

assigned to @tkremenek