Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix SA xcode 7 pointed out #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix SA xcode 7 pointed out #118

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Reachability.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ +(instancetype)reachabilityWithHostname:(NSString*)hostname
{
id reachability = [[self alloc] initWithReachabilityRef:ref];

CFRelease(ref);

return reachability;
}

Expand All @@ -112,7 +114,9 @@ +(instancetype)reachabilityWithAddress:(void *)hostAddress
if (ref)
{
id reachability = [[self alloc] initWithReachabilityRef:ref];


CFRelease(ref);

return reachability;
}

Expand Down Expand Up @@ -152,6 +156,8 @@ -(instancetype)initWithReachabilityRef:(SCNetworkReachabilityRef)ref
self.reachableOnWWAN = YES;
self.reachabilityRef = ref;

CFRetain(self.reachabilityRef);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matches CFRelease(self.reachabilityRef); in dealloc

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except there's still a potential leak according to the analyzer, since SCNetworkReachabilityCreateWithName has already increased the retain count.

While I agree that this line is necessary to match the CFRelease call in dealloc, the ref should also be released in the factory methods to match the create calls.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just re-ran the SA on my branch. No SA errors visible. Running xCode 7.2

Line 98 & 111 each create a ref locally, therefore I would say and hopefully everyone agrees it should be released locally. (My fix)

Ignoring all other retain / release we have sufficiently balanced the life cycle, we have 1 retain count added and one retain count removed. We have a well balanced life cycle in local scope.

Now looking at the actual instance of Reachability, it also has a stake in the lifecycle of the ref. Because it has a release in dealloc on the ref, it should keep that balance and have a retain on the ref in the init. Which is what I did.

In summary, functionality wise nothing changes. Under current impl, theres not a leak, just the SA being grumpy. So all I did was remove a retain count in the class methods, so the SA would be happy, however since I did that I created a zombie so I had to rebalance and shove a retain down the common method (the init).

Again, this is purely a commit to make the SA go away. No memory leaks introduced, and no memory leaks fixed because there are none :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analyzer is correct. Consider the case where the SCNetworkReachabilityRef is created, and the initWithReachabilityRef: call fails. The ref will leak.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so my implementation fixes that. Unless I'm missing something.

Line 98, SCNetworkReachabilityRef is created, retain count 1.
Line 101, init does something with it. Sucess or fail, doesn't matter. As far as we know, the retain count is still 1. So ...
Line 103, we release it. If other objects are claiming ownership its one them

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake. I missed the last CFRetain call on line 159 because of the way the inline comments split up the diff.


// We need to create a serial queue.
// We allocate this once for the lifetime of the notifier.

Expand Down