AV while iterating through hash_map?

Question

_transaction is a private member variable of my class, declared as:

public:
    typedef stdext::hash_map<wchar_t*, MyClass*, ltstr> transaction_hash_map; 
private:
    transaction_hash_map _transactions;

During cleanup I am trying to iterate through this list and free up any objects still unfreed. However I am getting an AV on the for line here:

for (transaction_hash_map::const_iterator it = _transactions.begin(); it != _transactions.end(); it++)
{   
            MyClass* item = (MyClass*)it->second;

    if (item != NULL)
    {
        item->End();
        delete item;
    }       
}

Re: What is ltstr?

private:
    struct ltstr
    {
        enum
        {
            bucket_size = 8,
            min_buckets = 16
        };

        bool operator()(wchar_t* s1, wchar_t* s2) const
        {
            return wcscmp( s1, s2 ) < 0;
        }

        size_t operator()(wchar_t *s1) const
        {
            size_t  h = 0;

            wchar_t *p = const_cast<wchar_t*>(s1);
            wchar_t zero = L'\0';

            while ( *p != zero ) h = 31 * h + (*p++);

            return h;
        }
    };

The stack shows it inside the begin() method. Any ideas?

Answer 1

One possible thing I can think of is that your class has already been deleted elsewhere before you try to iterate through the hash_map, and thus begin() will be operating on garbage. Worth a check...

Also - how are your wchar_t*'s getting allocated/freed? The code you've shown doesn't appear to be dealing with those. I'm not sure how that would cause trouble in your loop, but it's worth thinking about.

One minor thing - you shouldn't need the (MyClass*) cast. The hash_map's values should be of that type anyway, so it's nicer to let the compiler enforce type checks than to possibly bypass them with an explicit cast. That shouldn't be making any difference here though.

Answer 2

As I understand you're checking your pointer against NULL for the "remaining" items that might have not been deleted yet. But for the items you delete before your cleanup stage, do you set the pointer to NULL?

Notice that when you delete an object the pointer is not automatically set to NULL. So if you're not doing that you're trying to delete the same object twice (because your if statement will always be true), what could cause an access violation.

The code below is an example that causes a double deletion. It can be fixed if you uncomment the line that sets the pointer to NULL.

#include <cstddef>

struct Item {};

int main()
{
  Item * p = new Item();
  delete p;

  //If you don't make the pointer null...
  //p = NULL;

  if (p != NULL)
      //You delete the object twice.
      delete p;
}

EDIT: I see you're getting the error exactly on the for line. So I'm wondering...

Apparently you have a MyClass that contains a _transactions member, which is a hash table with MyClass pointers as the data type. If the clean up code is performed inside a member function of MyClass, is it possible that you're deleting (for some reason) the MyClass instance that owns the _transactions you're iterating?

In this case you could get an error at it++ statement inside the for since the this object no longer exists. (Naturally, the error could be somewhere else too, like on the delete itself.)

Answer 3

Make sure to call _transactions.clear() after the for loop.