[runtime] Thread safety for weak references

[glessard]

It has been fairly easy to cause the runtime to crash on multithreaded accesses to weak references (e.g. SR-192). Although weak references are value types, they can get elevated to the heap in multiple ways, such as closure capture and property use in a class object. In such cases, race conditions involving weak references could cause the runtime to perform to multiple decrement operations of the unowned reference count for a single increment; this eventually caused early deallocation, leading to use-after-free, modify-after-free and double-free errors.

This commit changes the weak reference operations to use atomic operations rather than a thread-local logic. In particular, when the weak reference needs to be nulled, it is not done unconditionally, but via an atomic_compare_exchange operation, with the release operation only performed on success in order to avoid duplicated decrement operations. The assign operations assume the destination may be visible to multiple threads; the init operations assume the destination is local to the current thread. In all cases it is assumed that the source may be visible to multiple threads.

With this change, the crasher discussed in SR-192 no longer encounters modify-after-free or double-free crashes.

[gribozavr]

We have race test infrastructure (stdlib/private/StdlibUnittest/RaceTest.swift) that makes it possible to write such tests in a cross-platform way. Could you use that instead of dispatch?

[lattner]

@rjmccall ?

[glessard]

@rjmccall What are the restrictions on the uses of the Take calls? (The issues I observed seemed to bein swift_weakLoadStrong and swift_weakCopyAssign, and I expanded the logic to the rest of the swift_weak calls in HeapObject.cpp).

Why can you assume that assignment has exclusive access? Aren't those calls used to assign to a pre-existing var? One couldn't assume exclusive access of a var property in a class instance, for example.

[glessard]

The problem is due to not knowing the previous state of Value when storing to it. If two threads enter two of these calls simultaneously with a pointer to the same not-yet-nulled reference (to an object with no strong retains), they might both get past object->refCount.isDeallocating(), then both overwrite Value with null, and then they'll both call unownedRelease. One of them comes second and is making a wrong assumption. This is averted by using the exchange operation; the nulling operation is easy to catch both because of the consequence (modify-after-free), and also because more time passes between the read and the write in the original version. Other cases wouldn't lead to such obvious failures, but there could be memory leaks if Value changes between any paired read and write.

[glessard]

The only unknownWeak operations I can see are the non-ObjC stubs that directly forward to the HeapObject.cpp definitions from HeapObject.h. Is there another place to look? Thanks

[rjmccall]

In general, Swift does not guarantee correctness in the presence of read/write, write/write, or anything/destroy data races on a variable. It is one of the few undefined behavior rules that we embrace. This applies to weak references just as much as it applies to strong references.

Eventually, we want to provide a language concurrency model that makes races impossible outside of explicitly unsafe code. For now, it's the user's responsibility to ensure that these do not occur by properly synchronizing modifications of variables.

That's not to say that you aren't fixing a real bug. It's not supposed to be a race condition for two threads to simultaneously read from a weak reference, but right now it is because of the way that reads can clear the reference when they detect that the object is undergoing deallocation. That's something that needs to be fixed.

[rjmccall]

It's not easy to fix this particular race, however, because it isn't safe for other threads to access the referent at all if a thread is decrementing the unowned reference count, but other readers will naturally read the reference and try to increment the reference count of the referent. We basically need to have some way to tell other threads that there are reads in progress. One idea floated for fixing that is to use an activity count; please refer to the discussion thread with Mike Ash on swift-dev starting on 12/10. I'm not sure what the current status of his fix is, though.

[gribozavr]

You might also want to use _blackHole() from StdlibUnittest to ensure that the compiler does not eliminate the unused weak load.

[rjmccall]

The implementation of unknownWeak that I'm referring to is the non-trivial implementation in SwiftObject.mm. This is only an issue for platforms with ObjC compatibility requirements.

[glessard]

I see; I forgot to search the *.mm files. (I always do.)

[glessard]

I had thought about something like the activity count, but assumed the WeakReference struct's ABI was set in stone. Is it changeable? The CAS solution as I implemented is not perfect, but it's better, as the window of opportunity for badness is much smaller, without being too onerous.

[jckarter]

The ABI is not set in stone yet.

[glessard]

It's probably worth doing, then.

[glessard]

Here's another attempt; it's a rather simple spinlock approach. On thinking about options that expand the WeakReference struct, I can't think of a way to implement it as a lock-free algorithm. If the read-read race is to be resolved with a read-write lock (which is what Mike Ash's activity count suggestion looks like to me), I'm not convinced the extra complexity over a spinlock is warranted (please advise).

This being said, the tests that I called "direct capture" now complete without crashing, but the "class instance property" ones do not. When this occurs the crashed thread has attempted to dereference the spinlock illegal pointer, but the call stack does not contain any of the modified functions; the shared weak reference must be getting read as thread-local by error, but I don't know where at the moment.

[jckarter]

We need a lock that can yield to the scheduler, since iOS's scheduler doesn't guarantee preemption. If a background thread took the lock, a spinlock will deadlock any higher-priority threads that contend it.

[glessard]

Agreed; however, I'm unsure of the appropriate way to yield (I hope there is one.)
A lock-free method would be much better, but with this interplay of non-adjacent variables I'm not seeing what that would be.

[glessard]

If the strong and weak counts could both be CAS'd at once, refCount.isDeallocating() could be replaced by a call that tries to increment the weak count but fails if the deallocating flag is true (retainUnlessDeallocating?). This would solve the case where an unownedRetain loses a race with an unownedRelease. The other race is the one to null the weak reference and avoid a double-release; that one is easily solved with a CAS on WeakReference.Value.

I think this combination would successfully be lock-free.

[jckarter]

We might be able to pull that off on 64-bit platforms, yeah. On 32-bit, the object header refcounts are only 32-bit aligned, and I don't think either i386 or armv7 can do an unaligned 64-bit atomic cmpxchg. We could align the refcounts, at the cost of four bytes per object, maybe.

[rjmccall]

I don't understand how a CAS on WeakReference.Value can eliminate the nulling-out race. Could you spell that one out? There's a tempting solution that looks like that, but it is not actually correct because simply having loaded the reference does not guarantee its validity. You basically need transaction memory for anything like that to work.

Other than this weak-reference-nulling issue, there is no way to validly get a race between an unowned-retain and the final unowned-release that triggers deallocation. You cannot perform an unowned retain without some reason to think that the referent is valid. For example, unowned reference variables maintain an invariant of owning an unowned-retain of their referent. As long as that invariant holds, it's safe to load/copy from the variable because the unowned reference count is always at least 1. Anything that would race with the load/copy would have to be a change to the value of the variable (or destroying it), or in other words, a store; and a load/store race is undefined behavior.

[glessard]

You're right, in both cases I forgot that the pointer copied from Value becomes untrustworthy on the very next clock cycle if there's a race. So the lock-free thoughts go out the window.

As for racing between a retain and a release,: I was thinking of a situation that starts with one weak reference, visible to 2 threads; strong count is 1. Thread A gets false from isDeallocating while attempting to copy the reference, then gets preempted; then thread B sets the deallocating flag on the strong count, then thread C (working with the same weak reference as A) gets true from isDeallocating. At that point an unownedRelease could happen before the unownedRetain. Depending on the length of the preemption on A, it could end up with a use-after-free. This being said, this story involves trusting the pointer from Value for too long.

[glessard]

New version: uses 2 bits from WeakReference.Value to avoid races. One allows the unknownWeak functions to call through without having to test the reference (which led to some crashes,) while the other acts as a spin lock for weakLoadStrong and weakCopyInit. sched_yield is invoked after some small amount of spinning in the event of contention.

[jckarter]

@gparker42 Is sched_yield sufficient to prevent deadlocking on iOS?

[gparker42]

Maybe. I don't know what the kernel folks can promise, but I have been unable to make simple tests of QOS_CLASS_DEFAULT versus QOS_CLASS_BACKGROUND deadlock when there is a sched_yield in the loop (whereas similar tests do hang with a naive spinlock).

[rjmccall]

This is not guaranteed to actually reload the data.

[glessard]

The intent is to not hit the cache line too often. See line 730: an atomic_fetch occurs after line 724 returned true. If it's preferable to just repeatedly call atomic_fetch, I can change it.

[rjmccall]

You need to at least do something to prevent the compiler from folding the repeated loads into a single one.

[glessard]

My (weak) circumstantial evidence was that the benchmarked spin time scales with the counter limit at line 725, though I haven't looked at the generated assembly. Given that this depends on the current abilities of the optimizer and we agree that it isn't a high contention case, would you think it's justified to simply do atomic_fetch repeatedly? Otherwise I'm not entirely sure what would ensure repeated loads.

[rjmccall]

Repeated atomic_fetch would be fine. You're doing a relaxed load, so this really should just compile down to an ordinary load; it's just that the compiler will not be able to combine the loads.

[rjmccall]

@swift-ci Please test.

[rjmccall]

That test failure isn't this patch's fault.

[glessard]

should I squash the later changes?

[rjmccall]

If you wouldn't mind.

[rjmccall]

Please add a static_assert that WR_READING doesn't interfere with normal pointer bits, i.e. that WR_READING < alignof(void*).

[rjmccall]

Sorry about the long delay reviewing this, but with that static_assert and the squash, this should be good for 3.0.

[glessard]

Added the static_assert and squashed.
The test files are in a new directory, test/Runtime. I put them there because I couldn't find a spot; is there a better one?
I also tried to make it happen with an added field to the WeakReference struct, but wasn't successful at that. It would conceivably be less onerous in the native case, and it would support the far-fetched case of two compatibility modes for a given platform -- alas.

[rjmccall]

I do think we should bump weak references out to occupy two words for ABI stability, but we don't need to do that in this patch. Committing.

[rjmccall]

Thanks for taking care of this!

[glessard]

Thanks!

[jckarter]

Thanks for taking this on @glessard, great to see this fixed!

[glessard]

@jckarter I got lots of help from you guys!