A couple of days ago I finished coding up a feature in our C++ code base, hit F5 and was met with a nasty memory corruption debugger dialog. After about an hour of investigation it appeared one of my types was living past the lifetime of it’s owning heap. I decided the next step was to debug on the heap functions to see where I went wrong. I opened up the file for the heap and almost immediately new what I had done wrong.

Here’s a trimmed down version of the heap type I was looking at.

    class SpecialHeap {
    public:
        SpecialHeap() { 
            m_pBlock = ::VirtualAlloc(...);
            // Memory management magic
        }

        ~SpecialHeap() {
            VirtualFree(m_pBlock, ...);
        }
    
        void* Allocate(sizet_ bytes) { }
    private:
        void* m_pBlock;
    }

Anyone else immediately guess what I did wrong1?

What’s relevant is the code that’s not on the screen. Namely the default copy constructor generated by the C++ compiler. Types in C++ which don’t explicitly define a copy constructor are given a default copy constructor by the compiler. This is great for several classes of types in C++ as it allows for handy value copying with no extra user code.

One type it’s terrible for though are those which manage resources that are not meant to be shared. Types like our SpecialHeap for example. Any instance of SpecialHeap which is created via a copy constructor is a time bomb. A copy means there is an original and hence 2 SpecialHeap instances running around which refer to the same m_pBlock value. After the first destructor runs the other instance is left holding a garbage pointer.

After taking one look at this I knew I almost certainly left a & off of a SpecialHeap parameter which I intended to pass by reference. Instead it created a copy and when the function returned it destroyed the heap memory I expected to be alive. A quick scan of my change verified this was indeed the bug.

What’s really frustrating is that this bug is 100% preventable. Types can opt out of copy construction by simply declaring a user defined copy constructor with no implementation. 2

    private:
        // Disable value copying
        SpecialHeap(const SpecialHeap&);
        SpecialHeap& operator=(const SpecialHeap&);

This code just turned my runtime bug into a compilation error. What’s very frustrating about this issue is that it took < 1 minute to type and would have saved me an hour of debugging had the original author taken the time to add it. This type is not copy safe and allowing it to have a copy constructor is simply a bug.

This pattern is not special to this bug. It should be done for every single type you ever define in C++ that you do not explicitly intend to be copied. Getting in the habit of adding the above code **will **save you and others hours of debugging at some point down the road.

  1. The title of the blog post is a big give away 

  2. Making it private has the added bonus of being a binding error in almost all cases vs. a linker error 


Share Post

Google+

comments powered by Disqus