lhmouse/mcfgthread

Spurious loss of TLS object in case that a key is reused multiple times

lhmouse opened this issue · 2 comments

In src/env/thread_env.c:

static TlsObject *GetTlsObject(TlsThread *pThread, TlsKey *pKey){
    _MCFCRT_ASSERT(pThread);

    if(!pKey){
        return nullptr;
    }

    TlsObject *const pObject = (TlsObject *)_MCFCRT_AvlFind(&(pThread->avlObjects), (intptr_t)pKey, &ObjectComparatorNodeKey);
    if(pObject && (pObject->uCounter != pKey->uCounter)){  // [1]
        const _MCFCRT_TlsDestructor pfnDestructor = pObject->pfnDestructor;
        if(pfnDestructor){
            (*pfnDestructor)(pObject->nContext, pObject->abyStorage);
        }

        TlsObject *const pPrev = pObject->pPrevByThread;
        TlsObject *const pNext = pObject->pNextByThread;
        if(pPrev){
            pPrev->pNextByThread = pNext;
        }
        if(pNext){
            pNext->pPrevByThread = pPrev;
        }

        _MCFCRT_AvlDetach((_MCFCRT_AvlNodeHeader *)pObject);

        _MCFCRT_free(pObject);
        return nullptr;
    }
    return pObject;
}

Since a TLS key is the address of a block of memory, if a key is allocated after another one is freed, they could accidentally have the same value. I was aware of it and, as a result, TLS keys were assigned auto incremental IDs (the uCounter member) in order to check for reused keys. This turns out to be insufficient, as the if statement at [1] only does the check once. If a TLS key is reused twice or more, GetTlsObject() might not return a valid pointer even if the TLS object actually resides in the map.

Fixed.

Well I should have kept thread_env.c up-to-date with the one in MCFCRT since long ago. It seemed lost of sync somehow.