Fix memory leaks and integer mismatches identified by Clang static analyzer #14
Conversation
| newE = [NSEvent otherEventWithType:NSSystemDefined location:[e locationInWindow] modifierFlags:([e modifierFlags] | ([e type] == NSKeyDown ? 0xa00 : 0xb00)) timestamp:[e timestamp] windowNumber:[e windowNumber] context:[e context] subtype:8 data1:(specialCode << 16) + (([e type] == NSKeyDown ? 0x0a : 0x0b) << 8) data2:-1]; | ||
| } | ||
| CGEventRef newEvent = [newE CGEvent]; | ||
| CFRetain(newEvent); // newEvent gets released by the event system |
There was a problem hiding this comment.
-[NSEvent CGEvent] is documented thusly:
The
CGEventRefopaque type returned is autoreleased. If noCGEventRefobject corresponding to theNSEventobject can be created, this method returnsNULL.
Since it's autoreleased, I don't think we need to retain it here.
There was a problem hiding this comment.
That is true, but events returned by event taps are otherwise released:
If the event tap is an active filter, your callback function should return one of the following:
...
A newly-constructed event. After the new event has been passed back to the event system, the new event will be released along with the original event.
(https://developer.apple.com/documentation/coregraphics/cgeventtapcallback)
I read that as saying it needs to be retained before it's returned. Do you agree, @ZevEisenberg?
There was a problem hiding this comment.
That’s effectively what an autorelease does. It’s a deferred release, so the +1 retain count from being created will be in effect until the run loop drains the main autorelease pool. Caveat: I’m a little sleep deprived at the moment, but my reading is that, since the value returned from [newE CGEvent] is autoreleased already, it will not get a further release until the pool drains.
|
Thanks @ZevEisenberg! I will take a close look and get this merged by later this month. |
|
Great! My Objective-C is a little rusty, and the HID stuff is new for me, so I’m happy to make changes or discuss anything. |
|
Any chance this can be merged and released soon? It seems like it's been in limbo for a while. I am also experiencing the memory leaks described in #9. Thanks in advance! |
|
@antipex hi, since @kevingessner has stopped working on FunctionFlip altogether, can this also be merged into your fork? |
4 participants
Also modernized some Obj-C syntax.
Might address some of the leaks causing #9, but I haven't tested it. Not sure what the best way to profile the app is, since I don't understand the relationship between the preference pane and the helper. If you can tell me how to run in Instruments, I'd be happy to give it a look.
Static analysis warnings I didn't fix:
+[DDHidDevice allDevicesMatchingCFDictionary:withClass:skipZeroLocations:]. The static analyzer complains that the match dictionary that is passed in may be leaked. However, the dictionary is passed toIOServiceGetMatchingServices, which is documented to consume (i.e. release) the dictionary that is passed in. Not sure how to suppress the static analyzer, but as I understand it, this is not actually leaking.