If we reach the end of a thread in Nuwa process, the stack of the ending thread will be freed and filled with garbage data before the thread actually ends, and Nuwa will crash.

Comment on attachment 8421552 [details] [diff] [review] Proposed Patch Review of attachment 8421552 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/build/Nuwa.cpp @@ +542,5 @@ > +cleanup_thread_func(void *arg) { > + thread_info_t *tinfo = (thread_info_t *)arg; > + void *ret; > + > + REAL(pthread_join)(tinfo->origThreadID, &ret); the pthread's documentation says "The results of multiple simultaneous calls to pthread_join() specifying the same target thread are undefined." I realize that everything we've done in Nuwa is dependent on a bunch of undefined pthreads behavior, but are we convinced that this is ok? @@ +580,5 @@ > + pthread_t cleanup_thread; > + if (REAL(pthread_create)(&cleanup_thread, > + nullptr, > + &cleanup_thread_func, > + arg)) { This new thread is totally unknown to Nuwa, right? What happens if we try to fork while it is running?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3) > Comment on attachment 8421552 [details] [diff] [review] > Proposed Patch > > Review of attachment 8421552 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mozglue/build/Nuwa.cpp > @@ +542,5 @@ > > +cleanup_thread_func(void *arg) { > > + thread_info_t *tinfo = (thread_info_t *)arg; > > + void *ret; > > + > > + REAL(pthread_join)(tinfo->origThreadID, &ret); > > the pthread's documentation says "The results of multiple simultaneous calls > to pthread_join() specifying the same target thread are undefined." > > I realize that everything we've done in Nuwa is dependent on a bunch of > undefined pthreads behavior, but are we convinced that this is ok? Well.. I don't think so. I didn't notice this. Yeah, we should find a way to handle multiple joins on a single thread. Thanks for catching this. > > @@ +580,5 @@ > > + pthread_t cleanup_thread; > > + if (REAL(pthread_create)(&cleanup_thread, > > + nullptr, > > + &cleanup_thread_func, > > + arg)) { > > This new thread is totally unknown to Nuwa, right? What happens if we try > to fork while it is running? The old thread has unlink from Nuwa's thread list, so it won't be recreated in the new process. But the stack space will be leaked, since there won't be a thread to clean up the resource in new process. I think we can delay the fork until the resource clean-up is done to prevent from the leakage.

Comment on attachment 8421552 [details] [diff] [review] Proposed Patch Review of attachment 8421552 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/build/Nuwa.cpp @@ +542,5 @@ > +cleanup_thread_func(void *arg) { > + thread_info_t *tinfo = (thread_info_t *)arg; > + void *ret; > + > + REAL(pthread_join)(tinfo->origThreadID, &ret); We may consider letting this thread terminate the exiting thread and don't pthread_join() it. Then another thread may safely pthread_join() the exiting thread. @@ +581,5 @@ > + if (REAL(pthread_create)(&cleanup_thread, > + nullptr, > + &cleanup_thread_func, > + arg)) { > + // Unable to create a clean up thread. We should only do the safe part on the exiting thread and leave all other else to the cleanup thread. We will need to block the fork request until thread cleanup finishes.

(In reply to Cervantes Yu from comment #5) > Comment on attachment 8421552 [details] [diff] [review] > Proposed Patch > > Review of attachment 8421552 [details] [diff] [review]: > ----------------------------------------------------------------- > > We may consider letting this thread terminate the exiting thread and don't > pthread_join() it. Then another thread may safely pthread_join() the exiting > thread. It turns out that pthread_cancel() the exiting thread is still unsafe that the cancelled thread will still call pthread_exit() and then use the stack. It looks like pthread_join() is the only means we can be sure that it's safe to free the stack.

Finally, we decide to wrap pthread_join and pthread_detch to complete the resource clean up task. If a thread want to join another thread which is running, it will be blocked until the thread end and will be responsible for release the resource for the target thread. For a thread that is not joined by another thread, we will create a thread to join the thread and store its result value in sThreadResultMap until the thread is joined or is detached. The thread that joined another thread won't become freeze before it is unblocked. And if a thread is created to clean up resource for a target thread, it will update thread count after cleaning the resource. Therefore Nuwa won't become ready when a thread is joined.

Comment on attachment 8429172 [details] [diff] [review] Proposed Patch v2 There's still a problem in this patch, it could make resource deleted twice sometimes.

And there's a bug in bionic's implementation of pthread_join: pthread_join is implemented by waiting for a condition in bionic, and it is signaled by the target thread before it ends. However, being able to signal a condition means that the target thread is still alive, and it still needs stack to return from pthread_mutex_unlock when the waiting thread is woken up to clean the resource. So we will need to workaround this or it will result in random crash.

Comment on attachment 8431430 [details] [diff] [review] Proposed Patch v3 Tested locally and on try server. It is ready for review now.

Comment on attachment 8431430 [details] [diff] [review] Proposed Patch v3 Review of attachment 8431430 [details] [diff] [review]: ----------------------------------------------------------------- r- because I need to see the issues below getting addressed. Generally, the biggest issue is that the introduction of a per-thread mutex brings in a complicated design, but the concurrency gain isn't strong enough compared to the added complexity. ::: mozglue/build/Nuwa.cpp @@ +137,5 @@ > typedef LibcAllocator<U> other; > }; > private: > + template<typename U> > + friend class LibcAllocator; nit: now that we need to declare a friend here, we'd better change LibcAllocator to class so it doesn't feel strange. @@ +261,5 @@ > typedef std::map<pthread_key_t, void (*)(void *)> TLSKeySet; > static TLSKeySet sTLSKeys; > > +typedef std::map<pthread_t, void*, > + std::less<pthread_t>, pthread_t is an opaque type. It just works here because it's backed by an integral type as in bionic libc or glibc. We introduce a dependency of implementation by comparing pthread_t instances using std::less, which is not portable. Please add a comment about the portability issue here. @@ +263,5 @@ > > +typedef std::map<pthread_t, void*, > + std::less<pthread_t>, > + LibcAllocator<std::pair<const pthread_key_t, void*> > > ThreadResultMap; > +static ThreadResultMap sThreadResultMap; nit: ThreadResultMap doesn't sound clear to me. What you store in this map is the thread exit status right? Consider a rename. @@ +564,5 @@ > > return tinfo; > } > > +// Clean resource of a ended thread. nit: This comment doesn't tell us much more about this function. @@ +597,2 @@ > sThreadCount--; > + pthread_mutex_unlock(&sThreadCountLock); You released sThreadCountLock in _thread_cleanup_thread() and then reacquire it after entering this function. Why not just hold the lock? There might be a problem that we destroy members in tinfo and then we detach it from the list. Because sThreadCountLock only protects the detachment of tinfo, we might leave tinfo in a state that it is half-destroyed but still visible to other threads. We need to look deeper into this. @@ +600,4 @@ > pthread_cond_signal(&sThreadChangeCond); > +} > + > +// The thread function of the thread used to clean a thread. nit: This is a little wordy. Why not give "thread thread used to clean a thread" a name such as the cleaner thread or the reaper thread? @@ +603,5 @@ > +// The thread function of the thread used to clean a thread. > +static void* > +_thread_cleanup_thread(void* arg) { > + // sThreadCountLock and tinfo->joinThreadLock should be locked before entering > + // this function. From the man page: "Attempting to unlock the mutex if it was not locked by the calling thread results in undefined behavior". We introduce a dependency of the implementation of bionic that just works and need to document this dependency. @@ +634,5 @@ > + > + // Lock sThreadCountLock to prevent other thread from joining this thread. If we > + // lock this later after holding joinThreadLock, it would be a deadlock if other > + // thread enters wrapped pthread_join. > + REAL(pthread_mutex_lock)(&sThreadCountLock); If sThreadCountLock already protects from concurrent join of the thread, why introducing yet another lock, tinfo->joinThreadLock? I can see that sThreadCountLock is held shorter than tinfo->jointThreadLock, but is it worth the extra cost given that it is also reacquired in the cleanup thread? If sThreadCountLock needs to be reacquired later in thread cleanup then I think just using sThreadCountLock will be simpler. After all, thread creation/termination should not be a hotspot. Otherwise, we need to use sThreadCountLock only when it is really necessary. @@ +858,2 @@ > if (tinfo == nullptr) { > + pthread_mutex_unlock(&sThreadCountLock); Same as above: why using sThreadCountLock instead of tinfo->joinThreadLock when you need to perform pthread_join? @@ +864,5 @@ > + if (tinfo->joined || tinfo->detached) { > + // There's another thread joined the thread we are going to join, > + // return a -1 to indicate error. > + pthread_mutex_unlock(&tinfo->joinThreadLock); > + pthread_mutex_unlock(&sThreadCountLock); You need to set errno when returning -1. @@ +915,5 @@ > + if (tinfo->joined || tinfo->detached) { > + // There's another thread joined the thread we are going to join, > + // return a -1 to indicate error. > + pthread_mutex_unlock(&tinfo->joinThreadLock); > + pthread_mutex_unlock(&sThreadCountLock); nit: releasing the mutexes in this function is really repetitive. Can you clean it up using an RAII class?

For what it's worth, it looks as if we don't currently ever reach thread_info_cleanup (i.e., no thread created before Nuwa freezes ever exits) — I did a try run with a MOZ_CRASH at the top and it passes: https://tbpl.mozilla.org/?tree=Try&rev=41d881a80f00 So, one alternative would be to just leak the stack with a MOZ_ASSERT(false). In fact, that's probably what should happen on all the release branches that have this code, just in case someone discovers a way to cause this use-after-free to be reached. Also, the leak would be relatively small if this did happen — the rest of thread cleanup probably doesn't need more than a page or two, and the rest could be unmapped or MADV_DONTNEED'ed. And I think we could switch to letting Bionic allocate all thread stacks and using pthread_attr_setstack only when restarting threads in the child, which would mean that threads which exit before Nuwa freeze time would be cleaned up automatically — and the number of restarted threads that could possibly exit in the child and thus leak their stacks would be finite and relatively small. But it seems less than ideal to write any nontrivial code to deal with this situation without having a test case.

(In reply to Jed Davis [:jld] from comment #13) > For what it's worth, it looks as if we don't currently ever reach > thread_info_cleanup (i.e., no thread created before Nuwa freezes ever exits) > — I did a try run with a MOZ_CRASH at the top and it passes: > https://tbpl.mozilla.org/?tree=Try&rev=41d881a80f00 > > So, one alternative would be to just leak the stack with a > MOZ_ASSERT(false). In fact, that's probably what should happen on all the > release branches that have this code, just in case someone discovers a way > to cause this use-after-free to be reached. Yes. There's no thread exit in Nuwa now, but it happens if we try to preload in Nuwa. Actually, process crashes in current code if we reach thread_info_cleanup, since it frees stack but the guarding page isn't set back to writable. > Also, the leak would be relatively small if this did happen — the rest of > thread cleanup probably doesn't need more than a page or two, and the rest > could be unmapped or MADV_DONTNEED'ed. > > And I think we could switch to letting Bionic allocate all thread stacks and > using pthread_attr_setstack only when restarting threads in the child, which > would mean that threads which exit before Nuwa freeze time would be cleaned > up automatically — and the number of restarted threads that could possibly > exit in the child and thus leak their stacks would be finite and relatively > small. We need the stack pointer and size, and it is not easy for us to obtain the values with standard pthread functions. However, I think we can try since we have pthread_getattr_np in our platform, although it is not a portable api. > But it seems less than ideal to write any nontrivial code to deal with this > situation without having a test case. Since running preload in Nuwa involves thread exit, it will make this piece of code exercised in test cases.

So we set stack size and let pthread to allocate/deallocte stack. The patch now has become much simpler!

There's another reason to allocate stack from Nuwa: to control how the space allocated. Say, when we want to free space in spawned process, we need to know if the space is obtained from mmap or malloc. Back to the original simple approach, we can wait target thread through pthread_kill instead of pthread_join. The cleaner thread is created when thread is almost exit, so the waiting loop won't take too long. And since we keep sThreadCount unchanged until we actually cleaned the resource, Nuwa won't become ready before the resource is cleaned.

Comment on attachment 8434937 [details] [diff] [review] Patch: wait until thread end and clean up resouce. Review of attachment 8434937 [details] [diff] [review]: ----------------------------------------------------------------- This is much simpler than the previous version. r+ but we still need Kyle's review before landing. ::: mozglue/build/Nuwa.cpp @@ +559,2 @@ > > + delete tinfo; It's unclear why you unlock, delete tinfo and then relock sThreadCountLock. If this is necessary, please comment the rationale behind this; otherwise, I would prefer not doing this kind of non-obvious thing.

(In reply to Cervantes Yu from comment #17) > Comment on attachment 8434937 [details] [diff] [review] > Patch: wait until thread end and clean up resouce. > > Review of attachment 8434937 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is much simpler than the previous version. r+ but we still need Kyle's > review before landing. > > ::: mozglue/build/Nuwa.cpp > @@ +559,2 @@ > > > > + delete tinfo; > > It's unclear why you unlock, delete tinfo and then relock sThreadCountLock. > If this is necessary, please comment the rationale behind this; otherwise, I > would prefer not doing this kind of non-obvious thing. The delete can't be invoked with sThreadCountLock locked, since delete calls some wrapped function that try to lock the mutex. I will add comments there.

Comment on attachment 8435779 [details] [diff] [review] Patch: wait until thread end and clean up resouce. v.2 (r=cyu) Review of attachment 8435779 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/build/Nuwa.cpp @@ +559,2 @@ > > + // delete cannot be called when sThreadCountLock is held, since delete call while sThreadCountLock is held, since delete calls