Odd heap error on delete...

[Sephiroth]

I am writing a class that will enumerate the MIDI devices on a modern (Vista/7/8) PC and store their names and IDs in an array of structs so that the user may select where he or she wants MIDI to be directed to instead of the crappy GS stuff. The problem is, I get a heap error upon cleaning up during destruction.

//The MIDI device structure
typedef struct MIDIDevice
{
  short sID;
  wchar_t *pName;
} *pMIDIDevice;

//Private method to clean up the device array
void MIDIObject::FreeArray()
{
  //Clean up the array if it is allocated
  if(this->sDeviceCount > 0)
  {
    //Handle more than one device correctly
    if(this->sDeviceCount > 1)
    {
      //Loop through, reset the ID, and delete all of the name arrays
      for(short sLoop = 0; sLoop < this->sDeviceCount; sLoop++)
      {
        this->pDevice[sLoop].sID = 0;
        //The line below errors for some unknown reason after the first instance is deleted
        delete [] this->pDevice[sLoop].pName;
      }

      //Now delete the array of devices
      delete [] this->pDevice;
    }
    else
    {
      //Delete the name array then the device itself
      this->pDevice->sID = 0;
      delete [] this->pDevice->pName;
      delete this->pDevice;
    }
  }

  //Reset the class members
  this->sDeviceCount = 0;
  this->pDevice = NULL;
}

The array is allocated as follows:

pMIDIDevice pMIDI;

pMIDI = new MIDIDevice[this->sDeviceCount];

for(short sLoop = 0; sLoop < this->sDeviceCount; sLoop++)
{
  pMIDI[sLoop].pName = new wchar_t[sLength];
  //Now I query the registry and copy the device name into pName
  //I then query the registry again and set the ID
}

The variables "sDeviceCount" and "sLength" are calculated before this code based on the number of MIDI devices in the registry and the length of the longest device name. They do not change after the calculation and are the same for all devcies. What am I doing wrong here? The shortest length will always be "Microsoft GS Wavetable Synth", so the array is never allocated as a single wchar_t.

Oh and the error is about writing memory past the end of the heap. I put in some debug code and the code deletes the first device name but fails on the second, third, and fourth (I have two SB X-Fi synths, the default synth, and one other). I know that the second through fourth are allocated because I can print them to a messagebox and they show up correctly.

*EDIT*

Just so you know, these are the device names stored in the pName field of the four instances.
Default MIDI Out Device
Microsoft GS Wavetable Synth
SB X-Fi Synth A [0001]
SB X-Fi Synth B [0001]

The error is:
HEAP CORRUPTION DETECTED: after Normal block (#61) at <address>
CRT detected that the application wrote to memory after end of heap buffer

[Disch]

It sounds like you are stepping outside of an array somewhere. Possibly with one of your strings. Are you remembering to add +1 to sLength for the null terminator?


Also is there any particular reason why you are doing all this memory management manually instead of using containers? You really could be saving yourself a ton of trouble.

[Sephiroth]

That was it. I had forgotten to add +1 to the end of the max key length. Here is the revised code which works.

  //Attempt to loop through and get the device information
  for(DWORD dwLoop = 0; dwLoop < dwKeyCount; dwLoop++)
  {
    //Attempt to allocate the new name array
    try
    {
      this->pDevice[dwLoop].pName = new wchar_t[(dwMaxKeyLength + 1)];
    }
    catch(...)
    {
      this->FreeArray();
      RegCloseKey(hKey);
      return ERROR_MIDI_ALLOCATE_NAME;
    }

I even read it in the documentation that I needed to add space for the null terminator. Thank you for your help!

Also, I am a C++ guy that is self-taught after years of experience with C. Could you point me to information on containers? I am enjoying C++ for the object-oriented stuff, but have not spent much time studying the other advantages yet.

[Disch]

std::vector (in the <vector> header) is basically a resizable array. So you do not have to new[] or delete[] your array manually, you can just make a vector.

#include <vector>

// this...
    int sDeviceCount = 0;
    MIDIDevice* sDevice;

// becomes this:
    std::vector<MIDIDevice>  sDevice;
        // keeping a seperate count isn't necessary... just do sDevice.size() to get
        //  the size.

//================
// this...
    sDeviceCount = desired_count;
    sDevice = new MIDIDevice[desired_count];

// becomes this:
    sDevice.resize( desired_count );

//=================
// this...
    [ your entire FreeArray function ]

// becomes this:
    sDevice.clear();  // but even this isn't necessary unless you specifically want to clear it.
        //  vector's destructor will automatically clean up the array when it dies, so you do
        //  not need to manually do it 

strings are similar. But instead of being a variable length array of whatever type... they're specifically designed to be a variable length array of characters (or in your case, wide characters).

You don't have to manually allocate space, you can just assign it some string data and it will automatically resize the string buffer to ensure it's large enough to hold it.

No worries about buffer overflows, terminating nulls, or cleanup. It's all automatic.

[Sephiroth]

I will start reading up on the vectors then. I assume that this works on all platforms (Windows, Linux, Mac, etc)?

Now, you wouldn't happen to know why the carbs on a 1986 Honda Rebel 450 flood even though the float needles are seating and everything has been rebuilt, would ya'? That one is really getting to me tonight!

*EDIT*

One other quick question. If I switch to using vectors, how will I handle allocation and deletion of the name field in the MIDIDevice structure? Would that be "std::vector<wchar_t> pName;" or something, then I either loop through and call clear on each one or hope that during destruction it does it automatically?

[MikeyBoy]

One other quick question. If I switch to using vectors, how will I handle allocation and deletion of the name field in the MIDIDevice structure? Would that be "std::vector<wchar_t> pName;" or something, then I either loop through and call clear on each one or hope that during destruction it does it automatically?

Well, if you're on a voyage of discovery into the STL, I can thoroughly recommend looking at the std::string class (defined in the <string> header file). It handles the memory management for a string so that you don't have to worry about it. It's much more suitable for strings than a vector of chars.

Oh, wait, you're using wchar_t, not char - so you'll want std::wstring instead, which works the same way as std::string but uses wchar instead of char.

If you're desperate to stick with dynamically allocating your pName as an array, then the most sensible approach would be to create a destructor for your MIDIDevice. In C++, structs and classes are identical (except for the default access), so a struct can have member functions just like a class.

If you write a destructor for your struct, and have that destructor delete pName, then when the vector is destroyed, the following will hapopen:

- the vector class will destroy each element in turn
- when each element is destroyed, its destructor is called automatically
- the destructor for each element will free the memory used by pName

The end result is that you don't need to specifically free the memory - the MIDIDevice destructor will do it for you automatically when the MIDIDevice instances are destroyed, and that will happen when the vector itself is destroyed.

[Sephiroth]

Yes, I found the wstring class last night and switched to it. I am REALLY liking this. The old-school programmer in me does not like not having to free up resources like strings, but the new one trusts that the standard C++ stuff will work correctly. I also like not having to code every last thing by hand like I had to back in C. Thank you both for helping!

[MikeyBoy]

You're welcome! Yeah, I know exactly what you mean - when writing in C you train yourself to rigorously ensure you free up your memory, so it feels a bit wrong just trusting that the STL classes will clean up after themselves. It feels a bit like cheating :)