Free List UB: Undefined Behavior due to object lifetime issues
nacht-wache opened this issue · 4 comments
The Problem
The function FreeList::push performs a static_cast from void* to Node*
Node* const head = static_cast<Node*>(p); -> doesn't start Node lifetime
head->next = mHead; -> UB`
Accessing non-static members like head->next on a pointer to non-existent objects is a violation of the C++ object lifetime rules [basic.life].
Solution
The correct way to handle this is to use placement new/std::launder to explicitly construct the Node object in the provided memory. This ensures the object has a valid lifetime before its members are accessed.
NB
While this might "work" on some compilers, it's a violation of the C++ standard. The correct approach is to use something to explicitly construct the Node object, ensuring its lifetime and guaranteeing safe member access. This prevents potential, hard-to-debug crashes and ensures future code stability.
Best regards!
@nacht-wache are you able to provide a patch?
@pixelflinger Yes, I'd be happy to prepare a patch. I'll take a closer look at the FreeList implementation, address all the UB concerns, and measure the changes with benchmarks. Do you happen to have any existing benchmarks for this functionality? If not, I can certainly write my own.
I can't promise a fast turnaround as I'm currently busy, but I'll do my best to get this done within the next week.
Great! I think libs/utils/benchmark/benchmark_allocators.cpp ends up hitting that code. If not, that's where the benchmark should go :-)
I've created a pr to replace two static_cast calls with placement new. I included a code comment to explain the change.
I decided not to wait long to submit it because I found that in O1 with Clang-16, both operations generate no assembly code, and in O0 they produce identical assembly. This means the change improves code safety and correctness without impacting performance.
Release O0 static_cast
Release O0 placement new
Release O3 placement new
