Suppose I have a MyStack
class which exposes:
class MyStack {
public:
template <typename T>
T* Push() {
Reserve(sizeof(T)); // Make sure that the buffer can hold an additional sizeof(T) bytes , realloc if needed
auto prev= _top;
_top += sizeof(T);
new (prev) T();
return reinterpret_cast<T*>(prev);
}
template <typename T>
T* Pop() {
_top -= sizeof(T);
return return reinterpret_cast<T*>(_top);
}
bool Empty() const {
return _bottom == _top;
}
private:
char* _bottom;
char* _top;
};
// Assumes all stack elements have the same type
template <typename T>
void ClearStack(MyStack& stack) {
while (!stack.Empty()) {
stack.template Pop<T>()->~T();
}
}
There's a hidden bug here. Constructing T
in MyStack::Push()
could throw which would leave the stack buffer in an undefined state (the allocated space would contain garbage). Later, when ClearStack
is called, it will attempt to reinterpret the garbage as T
and invoke its destructor which could cause an access violation.
Is there a way to fix this bug by only modifying MyStack::Push()
? (the limitation is because this is an external code and we prefer to make minimal changes so it's relatively easy to update the library)
I thought about changing MyStack::Push
to:
T* Push() {
auto prev = _top;
T t();
Reserve(sizeof(T));
_top += sizeof(T);
reinterpret_cast<T*>(prev) = std::move(t);
return prev;
}
But it looks bad and I'm not even sure that it doesn't invoke any UB (and also forces T
to have a move constructor)
Is there a better solution here to protect against throwing constructors? (preferably a small change inside MyStack::Push()
)
How about this code:
template <typename T>
T* Push() {
Reserve(sizeof(T));
auto prev= _top;
_top += sizeof(T);
try {
new (prev) T();
return reinterpret_cast<T*>(prev);
}
catch (...) {
Unreserve(sizeof(T)); //release the memory, optional?
_top = prev;
throw;
}
}