Racy access to ListDepth in CxPlatPoolFree in quic_platform_posix.h

[GaneshRapolu]

See

msquic/src/inc/quic_platform_posix.h

Line 537 in 1f2e91a

if (Pool->ListDepth >= CXPLAT_POOL_MAXIMUM_DEPTH) {

Pool->ListDepth is accessed with and without protection of Pool->Lock. However, ListDepth is declared as uint16_t in struct CXPLAT_POOL

msquic/src/inc/quic_platform_posix.h

Line 417 in 1f2e91a

uint16_t ListDepth;

It is neither an atomic variable, nor is the access inside CxPlatPoolFree atomic. Thus the read of ListDepth outside of Pool->Lock races with the write of ListDepth inside of Pool->Lock in CxPlatPoolFree. From my understanding, this is UB.

Think we can fix this without perf impact using relaxed atomics, as the CXPLAT_POOL_MAXIMUM_DEPTH is a best effort bound anyway.

[nibanks]

This depth really is treated more as a guideline than a hard limit. This is just used to try to impose a reasonable limit on the maximum number of items in the pool, but the limit is still arbitrary; so there's not a lot of reason to add complexity to enforce it. Unless there is actually an issue that can be reproduced related to this, I'd most likely leave it as is.

Thanks for doing the work to analyze the code and provide this feedback!

[GaneshRapolu]

Thanks for the quick response. I am aware that it is not and is not intended to be a hard limit. The change that I am proposing would also not make it a hard limit.

My only concern is that the racy access to ListDepth is undefined behavior (UB) and that (in theory) can cause arbitrary miscompilation/runtime behavior. To avoid this UB, we can use relaxed atomics to access ListDepth. Using relaxed atomics should not add any perf impact (ex. just mov on x86). Relaxed atomics will also not turn this into a hard limit.

So using functions from https://en.cppreference.com/w/c/atomic , the code would be something like

declare ListDepth as

atomic_uint_fast16_t ListDepth; 

change the portion of CxPlatPoolFree to be

    if (atomic_load_explicit(&Pool->ListDepth, memory_order_relaxed) >= CXPLAT_POOL_MAXIMUM_DEPTH) {
        CxPlatFree(Entry, Pool->Tag);
    } else {
        CxPlatLockAcquire(&Pool->Lock);
        CxPlatListPushEntry(&Pool->ListHead, (CXPLAT_SLIST_ENTRY*)Entry);
        uint16_t listDepth = atomic_load_explicit(&Pool->ListDepth, memory_order_relaxed);
        atomic_store_explicit(&Pool->ListDepth,  listDepth + 1,  memory_order_relaxed);
        CxPlatLockRelease(&Pool->Lock);
    }

Ditto for the other uses of ListDepth, ex. the write to ListDepth in CxPlatPoolAlloc

msquic/src/inc/quic_platform_posix.h

Line 508 in 1f2e91a

Pool->ListDepth--;

[nibanks]

Feel free to make a PR to try to improve the code, but we've never had a problem, and if the fix results in any significant performance regressions, I'd be hesitant to take it.