Memory Leak False Positive (possible dup)

[Quuxplusone]

Bugzilla Link	PR4683
Status	CONFIRMED
Importance	P normal
Reported by	John Engelhart (john.engelhart@gmail.com)
Reported on	2009-08-04 18:58:03 -0700
Last modified on	2010-02-22 12:47:36 -0800
Version	unspecified
Hardware	All All
CC	kremenek@apple.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments	bug1.m (1115 bytes, text/plain)
Blocks
Blocked by
See also

Created attachment 3286
distilled example

version: checker-0.214

This may be related to PR 4399, 2304.  This is only a (wild) guess.

I found this odd ball memory leak false positive.  See the attachment for the
distilled example.  The meat of the problem is this:

- (id)initWithVarArgs:(NSString *)string, ...
{
  va_list args;
  va_start(args, string);
  id obj = NULL;
  if((obj = [[MYObject alloc] init]) == NULL) { return(NULL); }
  obj = aggregatedInitFunction(obj, _cmd);
  va_end(args);

  if(obj == NULL) { NSLog(@"an error msg"); }
  return([obj autorelease]);
}

clang static checker says that 'obj' is no longer referenced after
va_end(args), which is kind of odd in and of itself.

The aggregatedInitFunction function takes 'obj' as one of its arguments and
either initializes it and returns that object, or releases 'obj' and returns
NULL.

Switching things around slightly so that aggregatedInitFunction returns void
and accepts a 'id *' to obj, and obj is passed via &obj.  Since the function
now returns void, the 'obj =' assignment statement goes away, too.  This works
fine, but accomplishes essentially the same thing.

(I could see how this might be a pragmatic heuristic that 'works well in
practice.')

[Quuxplusone]

Attached bug1.m (1115 bytes, text/plain): distilled example

[Quuxplusone]

(In reply to comment #0)
> Created an attachment (id=3286) [details]
> distilled example
>
> version: checker-0.214
>
> This may be related to PR 4399, 2304.  This is only a (wild) guess.
>
> I found this odd ball memory leak false positive.  See the attachment for the
> distilled example.  The meat of the problem is this:
>
> - (id)initWithVarArgs:(NSString *)string, ...
> {
>   va_list args;
>   va_start(args, string);
>   id obj = NULL;
>   if((obj = [[MYObject alloc] init]) == NULL) { return(NULL); }
>   obj = aggregatedInitFunction(obj, _cmd);
>   va_end(args);
>
>   if(obj == NULL) { NSLog(@"an error msg"); }
>   return([obj autorelease]);
> }
>
> clang static checker says that 'obj' is no longer referenced after
> va_end(args), which is kind of odd in and of itself.
>

The analyzer is referring to the value of 'obj' that was allocated by
[[MyObject alloc] init].  It isn't doing any inter-procedural analysis, so it
doesn't know that the 'obj' returned by aggregatedInitFunction() is the same
object.

> The aggregatedInitFunction function takes 'obj' as one of its arguments and
> either initializes it and returns that object, or releases 'obj' and returns
> NULL.

Right.  Basically 'aggregatedInitFunction' acts ike an init method, but because
of the analyzer's (current) lack of inter-procedural analysis it doesn't know
that.  It doesn't examine the implementation of 'aggregatedInitFunction'.

That said, we could provide an annotation that indicates that
'aggregatedInitFunction' basically "claims" the object passed to it (which it
does) and using the NS_RETURNS_RETAINED annotation one can indicated that
aggregatedInitFunction returns a retained object (which it does).  This would
document in the code itself that the function acts like an init method.

> Switching things around slightly so that aggregatedIntFunction returns void
> and accepts a 'id *' to obj, and obj is passed via &obj.  Since the function
> now returns void, the 'obj =' assignment statement goes away, too.  This works
> fine, but accomplishes essentially the same thing.

It think it's less natural.  The analyzer also shuts up here because 'obj' is
passed-by-reference and assumes the worst case that the value gets nuked by
some hidden logic.  It basically stops tracking the object at that point, so it
isn't flagging a false positive because it's smarter about that case, just more
conservative.  Again, inter-procedural analysis would help.

>
> (I could see how this might be a pragmatic heuristic that 'works well in
> practice.')
>

Basically the analyzer is suffering from insufficient information to understand
'aggregatedInitFunction'.  I think annotations to document the contract of that
function would be enough to solve this problem and allow you to structure your
code in a more Cocoa-like fashion.  Inter-procedural analysis will potentially
solve this problem too, but that is farther off.

What do you think?