Search code examples
c++memorystlcontainersunsafe

Prevent or detect "this" from being deleted during use

One error that I often see is a container being cleared whilst iterating through it. I have attempted to put together a small example program demonstrating this happening. One thing to note is that this can often happen many function calls deep so is quite hard to detect.

Note: This example deliberately shows some poorly designed code. I am trying to find a solution to detect the errors caused by writing code such as this without having to meticulously examine an entire codebase (~500 C++ units)

#include <iostream>
#include <string>
#include <vector>

class Bomb;

std::vector<Bomb> bombs;

class Bomb
{
  std::string name;

public:
  Bomb(std::string name)
  {
    this->name = name;
  }

  void touch()
  {
    if(rand() % 100 > 30)
    {
      /* Simulate everything being exploded! */
      bombs.clear();

      /* An error: "this" is no longer valid */
      std::cout << "Crickey! The bomb was set off by " << name << std::endl;
    }
  }
};

int main()
{
  bombs.push_back(Bomb("Freddy"));
  bombs.push_back(Bomb("Charlie"));
  bombs.push_back(Bomb("Teddy"));
  bombs.push_back(Bomb("Trudy"));

  for(size_t i = 0; i < bombs.size(); i++)
  {
    bombs.at(i).touch();
  }

  return 0;
}

Can anyone suggest a way of guaranteeing this cannot happen? The only way I can currently detect this kind of thing is replacing the global new and delete with mmap / mprotect and detecting use after free memory accesses. This and Valgrind however sometimes fail to pick it up if the vector does not need to reallocate (i.e only some elements removed or the new size is not yet the reserve size). Ideally I don't want to have to clone much of the STL to make a version of std::vector that always reallocates every insertion/deletion during debug / testing.

One way that almost works is if the std::vector instead contains std::weak_ptr, then the usage of .lock() to create a temporary reference prevents its deletion whilst execution is within the classes method. However this cannot work with std::shared_ptr because you do not need lock() and same with plain objects. Creating a container of weak pointers just for this would be wasteful.

Can anyone else think of a way to protect ourselves from this.

Solution

  • In your particular example the misery boils down to no less than two design flaws:

    1. Your vector is a global variable. Limit the scope of all of your objects as much as possible and issues like this are less likely to occur.
    2. Having the single responsibility principle in mind, I can hardly imagine how one could come up with a class that needs to have some method that either directly or indirectly (maybe through 100 layers of call stack) deletes objects that could happen to be this.

    I am aware that your example is artificial and intentionally bad, so please don't get me wrong here: I'm sure that in your actual case it is not so obvious how sticking to some basic design rules can prevent you from doing this. But as I said, I strongly believe that good design will reduce the likelyhood of such bugs coming up. And in fact, I cannot remember that I was ever facing such an issue, but maybe I am just not experienced enough :)

    However, if this really keeps being an issue despite sticking with some design rules, then I have this idea how to detect it:

    1. Create a member int recursionDepth in your class and initialize it with 0
    2. At the beginning of each non-private method increment it.
    3. Use RAII to make sure that at the end of each method it is decremented again
    4. In the destructor check it to be 0, otherwise it means that the destructor is directly or indirectly called by some method of this.
    5. You may want to #ifdef all of this and enable it only in debug build. This would essentially make it a debug assertion, some people like them :)

    Note, that this does not work in a multi threaded environment.