Revert "runtime_loader: Add missing locking around resizing the TLS DTV."

This reverts commit d0111efead.

Initially, I, too thought this was the correct solution. In
investigating the related #14342, I found that adding a lock of
the mutex in the function that crashed most often in (DestroyAll())
increased the time it took to cause a crash from 1-2 seconds
to 10-15 seconds, but it still inevitably crashed.

Further, crash addresses were often very low (e.g. 0x1, 0x18); on
inspection I determined these were coming from the fPointer fields
of TLSBlocks. But accesses to TLSBlocks were now well protected
by the TLS mutex, and they were memset'd after allocation, so that
didn't make any sense.

At that point, I went back and read over the code until I understood
it, and it became clear this solution was incorrect: TLSBlocks and
their underlying data are always associated with a specific thread,
meaning _Resize() and DestroyAll() would never be called on the same
data from different threads, despite appearances to the contrary.

Thus despite a dearth of comments in this file, it seems pdziepak
knew what he was doing when he wrote this; no locking is needed.
That left only one place to cause this kind of memory corruption...
This commit is contained in:
Augustin Cavalier 2018-12-17 19:52:49 -05:00
parent da31c58577
commit e7d37c7e08

View File

@ -12,16 +12,11 @@
#include <tls.h>
#include <locks.h>
#include <util/kernel_cpp.h>
static const char* const kLockName = "runtime loader tls";
static mutex sLock = MUTEX_INITIALIZER(kLockName);
class TLSBlock {
public:
public:
inline TLSBlock();
inline TLSBlock(void* pointer);
@ -285,13 +280,6 @@ DynamicThreadVector::_ResizeVector(unsigned minimumSize)
if (size <= oldSize)
return B_OK;
MutexLocker _(sLock);
// If the size has changed in the meantime, we're done.
oldSize = _Size();
if (size <= oldSize)
return B_OK;
void* newVector = realloc(*fVector, (size + 1) * sizeof(TLSBlock));
if (newVector == NULL)
return B_NO_MEMORY;