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

[Quuxplusone]

Bugzilla Link	PR2587
Status	CONFIRMED
Importance	P enhancement
Reported by	Nikita Zhuk (nikita@zhuk.fi)
Reported on	2008-07-23 01:45:52 -0700
Last modified on	2018-12-23 15:29:24 -0800
Version	unspecified
Hardware	Macintosh All
CC	kremenek@apple.com, llvm-bugs@lists.llvm.org, noqnoqneo@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

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

[Quuxplusone]

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.

[Quuxplusone]

(In reply to comment #1)
> 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

[Quuxplusone]

(In reply to comment #2)

> @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.

[Quuxplusone]

(In reply to comment #3)
> (In reply to comment #2)
>
> > @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.

[Quuxplusone]

(In reply to comment #4)
> 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.

[Quuxplusone]

(In reply to comment #5)
> (In reply to comment #4)
> > 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.

[Quuxplusone]

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.