Overload operator delete to not delete object - valid or undefined behavior?

user_6741772 0
Viewed 112

Preface: I'm optimizing an old codebase.

I have a class named Token for which I added caching to for a subset of the tokens (but not all). Cached tokens may not be deleted as their pointers are stored in a permanent collection in memory for the whole lifetime of the program.

Unfortunately, the codebase has delete token all over the place. So what I did was add a bool cached member that is checked from inside Token::operator delete and destructor ~Token(), which returns from these respective functions immediately if cached is true.

My question is, is this undefined behavior, or is it okay for me to do? Is it okay to execute the delete operator on something that doesn't get deleted? Or will this bite me in the future?

class Token
{
    bool cached;
    void* data;

public:
    ~Token()
    {
        if (this->cached) return;
        free(data);
    }

    void operator delete(void* p)
    {
        if (((Token*)p)->cached) return;
        ::operator delete(p);
    }

    // operator new, constructor etc.

}
c++
1 Answers

This is not OK. It is not UB to not free memory in operator delete, but it is UB to access anything inside the deleted object. delete token will first call the destructor for token, and then it will call operator delete. Even if you yourself do nothing inside the body of the destructor, once the body returns it will continue destroy all subobjects of the object, and once the whole destructor returns the whole object is considered to no longer exist. Even if your data members are scalars like bools or void*s, which implementations don't usually touch when destroying, the language considers them to be made inaccessible by the destructor. In your example, operator delete cannot access cached, and later, if you still have a "cached" pointer to the destroyed object around, using that pointer is also UB. This is why you receive a void* and not a Token* in operator delete, and also why operator delete is implicitly static (no this): it signifies that your object is already gone.

If you're in C++20 land, you can use destroying operator delete instead:

struct Token {
   // ...
   bool cached;
   void operator delete(Token *thiz, std::destroying_delete_t) {
       if(thiz->cached) return;
       thiz->~Token();
       ::operator delete(thiz);
   }
   ~Token() { /* clean up without worrying about cached */ }
};

When such an overload of operator delete exists, delete token will call the overload without calling the destructor itself, so you can safely choose to simply not destroy the object. Of course, now you're responsible for calling the destructor yourself.

If you can't do that, you're somewhat out of luck. You might try void Token::operator delete(void*) = delete; to find all uses of delete on Tokens so you can replace them. Preferably you'd replace your (I presume) Token*s with some kind of smart pointer (whether std::shared_ptr or something you write yourself), so that you no longer need to have as many deletes at all.

user_56842570
Related