Undefined behavior when growing a HashTable of non-trivial type (possibly requiring std::launder)

[hugoam]

Hello,

We have encountered a hard-to-catch relatively evasive issue which caused a segfault in our codebase, only on MSVC, and which was seemingly due to undefined behavior when growing a HashTable of non-trivial types

We managed to fix it, by adding a call to std::launder() in TValue* TItem::value() noexcept https://github.com/SergeyMakeev/ExcaliburHash/blob/main/ExcaliburHash/ExcaliburHash.h#L153

We are on an older version of the code, but it seems the problematic usage of this unlaundered value() pointer is the moving of the values when resizing/growing, in the new version of the code that would be about here https://github.com/SergeyMakeev/ExcaliburHash/blob/main/ExcaliburHash/ExcaliburHash.h#L689 (as our error seemed to be colocated to a call to grow() in the old version of the code)

I'm not an expert in those things as this is the first issue of this kind I ever encounter, but I suppose without the std::launder() call, the compiler is free to reorganize operations involving the original object so that some bad things have already happened to the storage by the time the move takes place?

Apparently this seems to be required by the standard, since we are "obtaining a pointer to an object created by placement new from a pointer to an object providing storage for that object"

Anyway, just thought this deserved a heads-up, feel free to address this how you see fit and thanks for the great library :)

[SergeyMakeev]

Thanks for catching this. I'll submit the fix ASAP.

[hugoam]

Hmm apologies, the investigation on our issue continued after opening this, as I was mistaken on thinking this fixed it, and the issue kept happening randomly, and we in fact discovered the source was an incorrect use-after-free behavior in our code causing dangling hashmap (string view) keys

We still kept the launder() calls in our version of this library as it seems to be warranted by the C++ standard anyway, but I can't tell for sure that the current code actually causes any issues in real life situations, actually it likely does not, so you can probably just close this