Is it safe to call a non-virtual base class member function from the base class destructor using a derived class pointer?

Question

This is my general question: Is it safe to call a non-virtual base class member function from the base class destructor using a derived class pointer that is getting destroyed?

Let me explain this by the following example.

I have a Base class and a derived Key class.

static unsigned int count = 0;                                                                                                                                
                                                                                                                                                              
class Base;                                                                                                                                                   
class Key;                                                                                                                                                    
                                                                                                                                                              
void notify(const Base *b);                                                                                                                                   
                                                                                                                                                              
class Base                                                                                                                                                    
{                                                                                                                                                             
public:                                                                                                                                                       
  Base(): id(count++) {}                                                                                                                                      
  virtual ~Base() { notify(this); }                                                                                                                           
  int getId() const { return id; }                                                                                                                            
  virtual int dummy() const = 0;                                                                                                                              
                                                                                                                                                              
private:                                                                                                                                                      
  unsigned int id;                                                                                                                                            
};                                                                                                                                                            
                                                                                                                                                              
class Key : public Base                                                                                                                                       
{                                                                                                                                                             
public:                                                                                                                                                       
  Key() : Base() {}                                                                                                                                           
  ~Key() {}                                                                           
                                                                                                                                                              
  int dummy() const override { return 0; }                                                                                                                    
};

I now create an std::map (std::set will also work) of derived Key class pointers sorted by their id as follows:

struct Comparator1                                                                                                                                            
{                                                                                                                                                             
  bool operator()(const Key *k1, const Key *k2) const                                                                                                         
  {                                                                                                                                                           
    return k1->getId() < k2->getId();                                                                                                                         
  }                                                                                                                                                           
};

std::map myMap;

Now as and when a Key gets deleted, I want to erase that key from myMap. To do this I first tried implementing the notify method triggered from ~Base() as follows, but I know this is not safe and can result in an undefined-behavior. I have verified this here: http://coliru.stacked-crooked.com/a/4e6cd86a9706afa1

void notify(const Base* b)
{
    myMap.erase(static_cast(b)); //not safe, results in UB
}

So to circumvent this issue, I defined a heterogenous Comparator and used variant (4) of std::map::find to find the key in the map and then passed that iterator to erase as follows:

struct Comparator2                                                                                                                                          
{                                                                                                                                                             
  using is_transparent = std::true_type;                                                                                                                      
                                                                                                                                                              
  bool operator()(const Key *k1, const Key *k2) const                                                                                                  
  {                                                                                                                                                           
    return k1->getId() < k2->getId();                                                                                                                         
  }                                                                                                                                                           
                                                                                                                                                              
  bool operator()(const Key *k1, const Base *b1) const                                                                                                 
  {                                                                                                                                                           
    return k1->getId() < b1->getId();                                                                                                                         
  }                                                                                                                                                           
                                                                                                                                                              
  bool operator()(const Base *b1, const Key *k1) const                                                                                                 
  {                                                                                                                                                           
    return b1->getId() < k1->getId();                                                                                                                         
  }                                                                                                                                                           
};      
                                                                                                                                                              
std::map myMap; 

void notify(const Base* b)
{
    // myMap.erase(static_cast(b)); //not safe, results in UB
    
    auto it = myMap.find(b);                                                                                                                                    
    if (it != myMap.end())                                                                                                                                      
        myMap.erase(it);
}

I have tested this second version with g++ and clang and I am not seeing any undefined behavior. You can try the code here: http://coliru.stacked-crooked.com/a/65f6e7498bdf06f7

So is my second version using Comparator2 and std::map::find safe? As inside the Comparator2, I am still using a pointer to the derived Key class whose destructor has already been called. I do not see any error using g++ or clang compiler, so could you please advise if this code is safe?

Thanks,

Varun

Edit: I just realized that Comparator2 can be further simplified by directly using the Base class pointer as follows:

struct Comparator2                                                                                                                                          
{                                                                                                                                                             
  using is_transparent = std::true_type;                                                                                                                      
                                                                                                                                                              
  bool operator()(const Base *k1, const Base *k2) const                                                                                                  
  {                                                                                                                                                           
    return k1->getId() < k2->getId();                                                                                                                         
  }                                                                                                                                                                                                                                                                                      
};

This also works: http://coliru.stacked-crooked.com/a/c7c10c115c20f5b6

user_2298137 · Answer

Unless I have mis-understood your code, then this is basically the same as an object that has a function that destroys itself (e.g. delete this;) - which is legal - provided that you do nothing after the deletion that relies on your object existing - like calling member functions or accessing member variables etc...

So looking at your code, I think you are ok - your pointer to the object is now UB if you use it again obviously, and the returning back up the stack of function calls looks safe.

But I would strongly suggest another approach - this will most Likely be a maintenance nightmare - if an unsuspecting developer later changes this code they very well may cause UB.

UnholySheep's idea of a separate class that manages all this for you sound much better :)

update

all you are really doing here is calling a normal function (notify()) which in turn calls the member (non virtual) getId() function via the comparator function via map.erase/find. This all occurs inside the destructor functions scope - which is fine. Here is a rough call trace of what happens when you call delete:

~Base()
    |
    v
  notify()
      |
      v
    Comparator() // This happens a number of times
        |
        v
      getId()    // This is called by Comparator
        |
   +----+           
   |
   v
~Base()          // base destructor returns

So you can see all the member (getId()) calls are done within the Base class d'tor function - and this is safe.

What I might suggest so that you don't have to write your "heterogenous Comparator" (Comparitor2), and to make your design/work easier, is make your map use the base class pointer: std::map myMap; then you can get rid of your Comparitor2 struct and you can use map.erase(b) directly in your notify() function and all this becomes a lot cleaner/clearer. Here is an example with some annotations (prints): https://godbolt.org/z/h5zTc9

Is it safe to call a non-virtual base class member function from the base class destructor using a derived class pointer?

1 Answers