What caused the memory leak in this code?

Question

I am inspecting the code that may cause memory leak. I know something is wrong with std::set.erase(this) and the destructor of SomeObject. So how to fix it?

class SomeObject;
////....
std::set<SomeObject*> managedObjects;
///.....
class SomeObject{
public:
    SomeObject(){  managedObjects.insert(this); }
    SomeObject(SomeObject&& S)/*move cter*/{ managedObjects.insert(this); }
    virtual ~SomeObject() { managedObjects.erase(this); }
    ////....
};
////....
void clearAllObjects() {
    for(auto p : managedObjects){
        if(p){
            delete p;
        }
    }
    managedObjects.clear();
}
////....

Answer 1

When you delete inside clearAllObjects() it will result in managedObjects.erase(this) which is the same as managedObjects.erase(p).

This means that the internal iterator in the range based for-loop may be invalidated (I'm not sure). If it is, it'll try to do ++internal_iterator; on an invalid iterator - with undefined behavior as a result.

To be safe, you could copy the iterator and step that to the next in the set before doing erase.

Also note: There's no need to check if what you delete is a nullptr or not. It's mandated by the standard to have no effect if that's the case.

Example:

void clearAllObjects() {
    for(auto pit = managedObjects.begin(); pit != managedObjects.end();) {
        delete *pit++ // postfix ++ returns a copy of the old iterator
    }
    
    managedObjects.clear();
}

A side effect by having this managedObjects set is that you can't have automatic variables of SomeObject.

int main() {
    SomeObject foo;
    clearAllObjects(); // deletes the automatic object "foo" (not allowed)
}                      // <- the automatic object is destroyed here