[MallocChecker] An Obj-C Method named initWithBytesNoCopy does not always transfer ownership

[db3989e7-7410-4315-9bfe-828e64d86539]

Bugzilla Link	17839
Version	3.3
OS	All
CC	@AnnaZaks

Extended Description

The MallocChecker incorrectly assumes, that an Objective-C method whose name starts with "initWithBytesNoCopy" always means that ownership of memory is transferred, unless there is also a "freeWhenDone" in the method name. Just because this is how NSData works doesn't mean it the same way how all other objects on earth with a method of that name work.

In my case I wrote an object, whose method is named:

"initWithBytesNoCopy:length:releaseCallback:"

and release callback is an optional block. If set, the block is called with the byte pointer as argument and may or may not release the memory; if not set (if NULL) it will not even be called and thus nothing is ever freed for sure.

The analyzer says that this is an error, because memory because "non-owned" memory well be freed by this method, even though I set no releaseCallback.

The analyzer should really not pretend to understand the meaning of methods, unless this meaning is really known (e.g. for NSData it is known and will certainly not change for compatibility reasons).

Here's an excerpt from MallocChecker, showing the code culprit:

00685 static bool isKnownDeallocObjCMethodName(const ObjCMethodCall &Call) { 00686 // If the first selector piece is one of the names below, assume that the 00687 // object takes ownership of the memory, promising to eventually deallocate it 00688 // with free(). 00689 // Ex: [NSData dataWithBytesNoCopy:bytes length:10]; 00690 // (...unless a 'freeWhenDone' parameter is false, but that's checked later.) 00691 StringRef FirstSlot = Call.getSelector().getNameForSlot(0); 00692 if (FirstSlot == "dataWithBytesNoCopy" || 00693 FirstSlot == "initWithBytesNoCopy" || 00694 FirstSlot == "initWithCharactersNoCopy") 00695 return true; 00696 00697 return false; 00698 } 00699 00700 static Optional getFreeWhenDoneArg(const ObjCMethodCall &Call) { 00701 Selector S = Call.getSelector(); 00702 00703 // FIXME: We should not rely on fully-constrained symbols being folded. 00704 for (unsigned i = 1; i < S.getNumArgs(); ++i) 00705 if (S.getNameForSlot(i).equals("freeWhenDone")) 00706 return !Call.getArgSVal(i).isZeroConstant(); 00707 00708 return None; 00709 }

[0f73b9cf-134f-41af-a8b1-14d9f305ee95]

Cloned to radar://15437918 for internal tracking.

Markus,

The analyzer often uses heuristics to find out what's going on.

The downside here is that we will not be able to report errors on the pointers that go into a generic "initWithBytesNoCopy".

[db3989e7-7410-4315-9bfe-828e64d86539]

The exact warning depends on the context. In some places I get:

"Attempt to free non-owned memory"

in other places I get:

"Argument to -initWithBytesNoCopy:length:releaseCallback: is the address of the static variable 'nullHeader', which is not memory allocated by malloc()"

If the method was calling free on the supplied memory pointer, both warnings would be correct, yet it doesn't, as in one case there is no releaseCallback at all and in the other one there is one, but it doesn't call free on the memory pointer.

I have no problem if the analyzer checks for "initWithBytesNoCopy:" as long as it also verifies that this is an object of the class NSData, NSMutableData or or subclass of it. But assuming that initWithBytesNoCopy: always releases the passed memory seems wrong. That is like assuming that any C function starting with "free..." in its name will release whatever first argument is passed to it (I guess this would trigger analyzer warnings in pretty much every bigger C project I've ever seen).

Even if I had "initWithBytesNoCopy:..." and "initWithBytesNoCopy:...freeWhenDone:", it would be wrong to assume that the default behavior is to free, just because NSData does that. It may as well make sense for a class to not free in 99,9% of all cases and thus unless free is explicitly requested, the method will not free and the analyzer would be wrong (it would say this code is perfectly okay, even though this code may actually crash at runtime).

IMHO the analyzer must never make assumptions like the ones above, unless you can override these assumptions with attributes (e.g. it makes assumptions about methods starting with new.../copy..., but it also allows to override these and it allows to mark methods of any name to behave like if they started with new/copy). Despite the fact, that the meaning of methods starting with new/copy/init is explicitly defined in several guides, but can you show me one guide that defines the meaning of initWithBytesNoCopy:? If it is not defined, it is not defined and the analyzer has no right to pretend otherwise.

[0f73b9cf-134f-41af-a8b1-14d9f305ee95]

Markus,

What is the exact warning you are getting? Is it a leak or use-after-free? We probably assume that the ownership is not transferred (because of the function name) and expect you to release the object. Is that correct?

There are advantages of following naming conventions similar to those NSData is using. They serve as a hint to the analyzer about what the behavior of your function is. This way the analyzer would be able to find more bugs in user-written code.

(On the other hand these are not rules set in stone, so we could just not use these heuristics for user-written code and stop tracking the pointer after a more comprehensive API check.)

[db3989e7-7410-4315-9bfe-828e64d86539]

There also seems no way to work around this problem, as there is "ns_consumed" but there is no "ns_not_consumed"; I could rename the method.

[db3989e7-7410-4315-9bfe-828e64d86539]

assigned to @tkremenek