NSNetServiceBrowser and NSNetService’s delegates are not properly __weak, objc_weak_error observed
| Originator: | mark | ||
| Number: | rdar://28943305 | Date Originated: | 2016-10-25 |
| Status: | Open | Resolved: | |
| Product: | macOS | Product Version: | 10.12.1 16B2555 |
| Classification: | Serious Bug | Reproducible: | Always |
NSNetServiceBrowser and NSNetService each have a delegate. In the normal Cocoa pattern, they do not take references to their delegates, they merely hold a weak reference. They apparently use a zeroing weak reference (__weak), whose purpose is to clear the delegate pointers in NSNetServiceBrowser and NSNetService objects should their delegates be deallocated first.
It appears that when NSNetServiceBrowser or NSNetService are deallocated prior to the delegate, the fact that they have been deallocated and thus no longer maintain references to their delegates is not recorded anywhere. Consequently, during deallocation of the delegates, the Objective-C runtime attempts to clear the delegate pointers in NSNetServiceBrowser and NSNetService, which are invalid because those objects no longer exist and the memory that hosted them may have been repurposed.
When this occurs, the Objective-C runtime may detect that the delegate field of the NSNetServiceBrowser or NSNetService object does not contain a pointer to the object being deallocated, but has instead been overwritten with something else. In this case, it log a warning like this:
objc[76391]: __weak variable at 0x100107110 holds 0x5555555555555555 instead of 0x100102760. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
The attached test program exhibits this problem. When run, it allocates an NSNetServiceBrowser object and an NSNetService object, both of which are told to use the same newly-allocated delegate object. The three objects are then released, the delegate being released last. Upon release of the delegate, the attempts to clear the weak pointers in the other two objects will look to freed memory. Whether the memory has been repurposed or not is a factor of what else happens in the process, so when run as-is, the test case will only exhibit the problem (as evidenced by the objc_weak_error being logged) intermittently. To exhibit the problem with certainty, set the MallocScribble environment variable, which causes free() to overwrite freed blocks with a 0x55 pattern.
$ clang -g weaksauce.m -o weaksauce -framework Foundation
$ MallocScribble=1 ./weaksauce
weaksauce(76587,0x7fffd1ada3c0) malloc: enabling scribbling to detect mods to free blocks
objc[76587]: __weak variable at 0x7fd669d05460 holds 0x5555555555555555 instead of 0x7fd669d03710. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
objc[76587]: __weak variable at 0x7fd669d060b0 holds 0x5555555555550004 instead of 0x7fd669d03710. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
When MallocScribble is not set, the test program spins a run loop for a second before releasing the delegate to give the rest of the program something to do, which in many cases results in the memory for at least one of these objects being repurposed and having something written to it.
$ ./weaksauce
objc[76634]: __weak variable at 0x7fd07c40b770 holds 0x7fffce91b218 instead of 0x7fd07c408a50. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
Note that the test program will tolerate being compiled both with and without ARC (-fobjc-arc). I have shown it here without ARC for clarity, because it allows |browser| and |service| to retain their values after deallocation, which allows their values to be inspected in the debugger in main()’s frame. This helps correlate the pointers in the objc_weak_error message to their previous uses.
Expanding on this in the debugger:
$ lldb weaksauce
(lldb) target create "weaksauce"
Current executable set to 'weaksauce' (x86_64).
(lldb) env MallocScribble=1
(lldb) b objc_weak_error
Breakpoint 1: where = libobjc.A.dylib`objc_weak_error, address = 0x000000000002025a
(lldb) r
Process 76625 launched: '…/weaksauce' (x86_64)
weaksauce(76625,0x7fffd1ada3c0) malloc: enabling scribbling to detect mods to free blocks
objc[76625]: __weak variable at 0x1002046d0 holds 0x5555555555555555 instead of 0x1002005c0. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
Process 76625 stopped
* thread #1: tid = 0x4b2bd, 0x00007fffc84e8261 libobjc.A.dylib`objc_weak_error, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
frame #0: 0x00007fffc84e8261 libobjc.A.dylib`objc_weak_error
libobjc.A.dylib`objc_weak_error:
-> 0x7fffc84e8261 <+0>: pushq %rbp
0x7fffc84e8262 <+1>: movq %rsp, %rbp
0x7fffc84e8265 <+4>: popq %rbp
0x7fffc84e8266 <+5>: retq
(lldb) bt
* thread #1: tid = 0x4b2bd, 0x00007fffc84e8261 libobjc.A.dylib`objc_weak_error, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
* frame #0: 0x00007fffc84e8261 libobjc.A.dylib`objc_weak_error
frame #1: 0x00007fffc84d7a5b libobjc.A.dylib`weak_clear_no_lock + 141
frame #2: 0x00007fffc84d797c libobjc.A.dylib`objc_object::clearDeallocating_slow() + 104
frame #3: 0x00007fffc84d1103 libobjc.A.dylib`objc_destructInstance + 153
frame #4: 0x00007fffc84d1059 libobjc.A.dylib`object_dispose + 22
frame #5: 0x00000001000018e3 weaksauce`main(argc=1, argv=0x00007fff5fbff9a8) + 371 at weaksauce.m:37
frame #6: 0x00007fffc8dbb255 libdyld.dylib`start + 1
frame #7: 0x00007fffc8dbb255 libdyld.dylib`start + 1
(lldb) frame sel 5
frame #5: 0x00000001000018e3 weaksauce`main(argc=1, argv=0x00007fff5fbff9a8) + 371 at weaksauce.m:37
34 }
35
36 #if !__has_feature(objc_arc)
-> 37 [delegate release];
38 #endif
39 }
40
(lldb) frame variable
(int) argc = 1
(char **) argv = 0x00007fff5fbff9a8
(Delegate *) delegate = 0x00000001002005c0
(NSNetServiceBrowser *) browser = 0x00000001002046c0
(NSNetService *) service = 0x0000000100207020
(lldb) c
Process 76625 resuming
objc[76625]: __weak variable at 0x100207030 holds 0x5555555555550004 instead of 0x1002005c0. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
Process 76625 stopped
* thread #1: tid = 0x4b2bd, 0x00007fffc84e8261 libobjc.A.dylib`objc_weak_error, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
frame #0: 0x00007fffc84e8261 libobjc.A.dylib`objc_weak_error
libobjc.A.dylib`objc_weak_error:
-> 0x7fffc84e8261 <+0>: pushq %rbp
0x7fffc84e8262 <+1>: movq %rsp, %rbp
0x7fffc84e8265 <+4>: popq %rbp
0x7fffc84e8266 <+5>: retq
(lldb) bt
* thread #1: tid = 0x4b2bd, 0x00007fffc84e8261 libobjc.A.dylib`objc_weak_error, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
* frame #0: 0x00007fffc84e8261 libobjc.A.dylib`objc_weak_error
frame #1: 0x00007fffc84d7a5b libobjc.A.dylib`weak_clear_no_lock + 141
frame #2: 0x00007fffc84d797c libobjc.A.dylib`objc_object::clearDeallocating_slow() + 104
frame #3: 0x00007fffc84d1103 libobjc.A.dylib`objc_destructInstance + 153
frame #4: 0x00007fffc84d1059 libobjc.A.dylib`object_dispose + 22
frame #5: 0x00000001000018e3 weaksauce`main(argc=1, argv=0x00007fff5fbff9a8) + 371 at weaksauce.m:37
frame #6: 0x00007fffc8dbb255 libdyld.dylib`start + 1
frame #7: 0x00007fffc8dbb255 libdyld.dylib`start + 1
(lldb) c
Process 76625 resuming
Process 76625 exited with status = 0 (0x00000000)
(lldb) q
Given the known values stored in |delegate| (0x1002005c0), |browser| (0x1002046c0) and |service| (0x100207020) during this run, a more complete understanding of these messages is possible:
objc[76625]: __weak variable at 0x1002046d0 holds 0x5555555555555555 instead of 0x1002005c0. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
objc[76625]: __weak variable at 0x100207030 holds 0x5555555555550004 instead of 0x1002005c0. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
The first message refers to an instance variable within |browser| at offset 0x10 (0x1002046d0 - 0x1002046c0), which was expected to contain a pointer to |delegate|, but was instead found with the value 0x5555555555555555, the MallocScribble free() pattern, because |browser| has already been deallocated. The second message is the same, but it refers to an instance variable within |service| at offset 0x10 (0x100207030 - 0x100207020). Note that in the second message, the observed value is 0x5555555555550004, an indication that malloc() has already given this memory out to another caller after the preceding free(), and that a new value has already been written to it.
--
Steps to Reproduce
$ clang -g weaksauce.m -o weaksauce -framework Foundation
$ MallocScribble=1 ./weaksauce
Expected Results
No output other than a message from libmalloc saying that MallocScribble has been enabled:
$ MallocScribble=1 ./weaksauce
weaksauce(76756,0x7fffd1ada3c0) malloc: enabling scribbling to detect mods to free blocks
$
--
Actual Results
objc_weak_error messages are logged, indicating that the objc_weak system has attempted to access freed memory indicated by the MallocScribble free() pattern, 0x55:
$ MallocScribble=1 ./weaksauce
weaksauce(76756,0x7fffd1ada3c0) malloc: enabling scribbling to detect mods to free blocks
objc[76756]: __weak variable at 0x7f8879c08ff0 holds 0x5555555555555555 instead of 0x7f8879c08a50. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
objc[76756]: __weak variable at 0x7f8879c0b770 holds 0x5555555555550004 instead of 0x7f8879c08a50. This is probably incorrect use of objc_storeWeak() and objc_loadWeak(). Break on objc_weak_error to debug.
$
--
macOS Version/Build
10.12.1 16B2555
Xcode 8.0 8A218a
Comments
Please note: Reports posted here will not necessarily be seen by Apple. All problems should be submitted at bugreport.apple.com before they are posted here. Please only post information for Radars that you have filed yourself, and please do not include Apple confidential information in your posts. Thank you!
We added a workaround for this bug on our side: https://codereview.chromium.org/2445343005/
It should still be fixed in CFNetwork.framework, and if the same pattern exists in any other system framework, it should be fixed there as well.
When I write my own class with a _weak instance variable, clang creates a synthetic .cxxdestruct method that calls objc_destroyWeak. You can even do this with a simple property (-fobjc-arc is required for “weak”):
@interface C : NSObject @property(nonatomic, weak) id delegate; @end @implementation C @end
synthesizes
NSNetService and NSNetServiceBrowser don’t do this. Here’s what those classes have (they’re in CFNetwork.framework):
There’s no -.cxx_destruct, and -dealloc doesn’t mention delegate at all. So based on the fact that their property definitions don’t mention “weak” at all (they’re (nullable, assign)) and the fact that they’ve got “delegate” instance variables that also aren’t called weak, it’s seems like the code to manage these classes’ _delegate instance variables is hand-written as above and not synthesized by clang. Perhaps this was done because ARC (required for weak variables and synthesized weak properties) was undesirable in those implementation files. But it’s clear that NSNetService and NSNetServiceBrowser never call objc_destroyWeak, which is exactly why their delegate objects never find out that they have been deallocated.
The minimal-change option is to add objc_destroyWeak(&_delegate) to the -dealloc method in both NSNetService and NSNetServiceBrowser. This should also be done in any other class that manages its own weak instance variables manually with objc_storeWeak and objc_loadWeak that’s not already calling objc_destroyWeak.
Furthermore, the Objective-C runtime error message that implicates “incorrect use of objc_storeWeak() and objc_loadWeak()” is somewhat misleading in this case, because both of these functions are being used correctly, but the omission of objc_destroyWeak() is causing the problem. This message should be revised to state “incorrect use of objc_storeWeak(), objc_loadWeak(), and objc_destroyWeak()”.
We discovered and are tracking this on our end at https://crbug.com/657495.
weaksauce.m
http://pastebin.com/E7W1tWKk