Page MenuHomePhabricator

[TSan][Darwin] Avoid potential crashes after unobserved operations

Authored by yln on Aug 30 2021, 2:33 PM.



Certain frameworks (e.g., IOSurface, IOKit) map and unmap VM ranges
through direct interaction with the kernel. So unfortunately, there is
no API that we can intercept to observe these operations and clear the
associated shadow memory. When such a previously-used VM region is
reused for a new thread, the shadow memory for the pthread_t struct may
have leftover, non-zero shadow bytes. Previously, we erroneously
interpreted these non-zero bytes as a pointer to the ThreadState
metadata and crashed.

In SignalSafeGetOrAllocate() we assumed all non-zero values to be
valid pointers, which led to crashes when a pthread_t struct later is
placed the the corresponding app memory and we interpret the shadow as
our pointer to the ThreadState metadata.

Now we always allocate this ThreadState metadata once and only once at
thread creation and remove the "get or allocate" semantics. We
therefore don't depend on the previous value anymore.

This requires that we don't call any interceptors before THREAD_CREATE
runs, which is a condition that currently holds. I added additional
asserts and tests to ensure we notice if this changes.

This change also fixes the following bug: previously we deallocated the
ThreadState metadata in THREAD_TERMINATE, however, some interceptors
still get called after that, which re-creates a new ThreadState
instance for the shutting down thread. Usually this worked "by
accident". The created metadata was either leaked or reused when
another thread was created in its place.


Diff Detail

Event Timeline

yln requested review of this revision.Aug 30 2021, 2:33 PM
yln created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2021, 2:33 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
kubamracek added inline comments.Aug 31 2021, 7:13 AM

Let's please add a comment containing as much detailed explanation as possible of what exactly the problem is and how it's being addressed here.

yln updated this revision to Diff 370399.Sep 2 2021, 2:01 PM

Update patch.

yln edited the summary of this revision. (Show Details)Sep 2 2021, 2:01 PM
yln updated this revision to Diff 370402.Sep 2 2021, 2:08 PM


yln updated this revision to Diff 370956.Sep 6 2021, 12:20 PM

Improve interaction between THREAD_TERMINATE/DESTROY.

yln edited the summary of this revision. (Show Details)Sep 6 2021, 12:21 PM
delcypher requested changes to this revision.Sep 7 2021, 7:19 PM
delcypher added inline comments.

The The current thread is a newly created GCD worker thread. comment got dropped. Is that intentional?


This used to be an if (condition), now it's a runtime check that needs to hold. Do we know why it was an if () condition before? Probably worth a comment if there are some subtleties here.


Why do we call ThreadFinish(cur_thread()) here rather than effectively letting the DestroyThreadState() do everything in the PTHREAD_INTROSPECTION_THREAD_DESTROY branch?


If we need to go with the approach of skipping the ThreadFinish() call, to avoid duplicating code (that may become out of date) how about we change DestroyThreadState() to take a bool parameter that is used to decided if ThreadFinish() should be called?

This revision now requires changes to proceed.Sep 7 2021, 7:19 PM
yln abandoned this revision.Thu, Sep 30, 8:55 AM

Abandoning in favor of D110236