Careful how you define your properties

Spot the vulnerability in this Objective-C class interface:

@interface SomeParser : NSObject {
  @private
	NSString *content;
}
@property (nonatomic, retain) NSString *content;
- (void)beginParsing;
//...
@end

Any idea? Let’s have a look at a use of this class in action:

SomeParser *parser = [[SomeParser alloc] init];
NSMutableString *myMutableString = [self prepareContent];
parser.content = myMutableString;
[parser beginParsing];
[self modifyContent];

The SomeParser class retains an object that might be mutable. This can be a problem if the parser only functions correctly when its input is invariant. While it’s possible to stop the class’s API from mutating the data – perhaps using the State pattern to change the behaviour of the setters – if the ivar objects are mutable then the class cannot stop other code from making changes. Perhaps the string gets truncated while it’s being parsed, or valid data is replaced with invalid data while the parser is reading it.

If a class needs an instance variable to remain unmodified during the object’s lifetime (or during some lengthy operation), it should take a copy of that object. It’s easy to forget that in cases like strings and collections where the type of the ivar is immutable, but mutable subclasses exist. So to fix this parser:

@property (nonatomic, copy) NSString *content;

You could also make the property readonly and provide an -initWithContent: constructor, which takes a copy that will be worked on.

But with collection class properties these fixes may not be sufficient. Sure, you definitely get an immutable collection, but is it holding references to mutable elements? You need to check whether the collection class you’re using support shallow or deep copying—that is, whether copying the collection retains all of the elements or copies them. If you don’t have deep copying but need it, then you’ll end up having to implement a -deepCopy method yourself.

Note that the above discussion applies not only to collection classes, but to any object that has other objects as ivars and which is either itself mutable or might have mutable ivars. The general expression of the problem is fairly easy to express: if you don’t want your properties to change, then take copies of them. The specifics can vary from case to case and, as ever, the devil’s in the detail.

About Graham

I make it faster and easier for you to create high-quality code.
This entry was posted in iPad, iPhone, Mac, Vulnerability. Bookmark the permalink.

2 Responses to Careful how you define your properties

  1. I implemented a “Defaults Table” in Accessorizer where you can define your property specifiers based on TYPE. In other words, you can define NSString to use “copy”. When Accessorizer generates your property statements (or explicit accessors), you will get a “copy” if the type passed in is NSString. Of course, you can set up your defaults for any TYPE including custom objects. IE: Accessorizer handles this automagically for you. You won’t “forget” :-) It also generates all your accessors for collection classes like NSMutableArray or NSSet and a lot more.

    – Kevin Callahan
    http://www.kevincallahan.org/software/accessorizer.html

  2. It’s worth pointing out that for “regular” immutable objects, copy and retain are equivalent, so there’s no penalty for doing a copy. Here’s why I always s prefer copy over retain when declaring a property.

Comments are closed.