Valgrind reports "Invalid free() / delete / delete[]"

I'm not sure what could be causing this.

==18270== Invalid free() / delete / delete[]
==18270==    at 0x400576A: operator delete(void*) (vg_replace_malloc.c:342)
==18270==    by 0x80537F7: LCD::LCDControl::~LCDControl() (LCDControl.cpp:23)
==18270==    by 0x806C055: main (Main.cpp:22)
==18270==  Address 0x59e92c4 is 388 bytes inside a block of size 712 alloc'd
==18270==    at 0x40068AD: operator new(unsigned int) (vg_replace_malloc.c:224)
==18270==    by 0x8078033: LCD::DrvCrystalfontz::Get(std::string, LCD::LCDControl*, Json::Value*, std::string) (DrvCrystalfontz.cpp:652)
==18270==    by 0x8053F50: LCD::LCDControl::ConfigSetup() (LCDControl.cpp:71)
==18270==    by 0x80539F7: LCD::LCDControl::Start(int, char**) (LCDControl.cpp:31)
==18270==    by 0x806C025: main (Main.cpp:21)

Here's LCDControl's destructor where delete is called.

LCDControl::~LCDControl() {
    Shutdown();
    for(std::vector<std::string>::iterator it = display_keys_.begin();
        it != display_keys_.end(); it++) {
        Error("Deleting %s %p", (*it).c_str(), devices_text_[*it]);
        if(devices_text_.find(*it) != devices_text_.end() && devices_text_[*it])
            delete devices_text_[*it]; // line 23
    }
    //delete app_;
}

Here's Crystalfontz::Get()

switch(m->GetProtocol()) {
    case 1:
        return new Protocol1(name, m, v, config);
        break;
    case 2:
        return new Protocol2(name, m, v, config); // line 652
        break;
    case 3:
        return new Protocol3(name, m, v, config, scab);
        break;
    default:
        Error("Internal error. Model has bad protocol: <%s>",
            m->GetName().c_str());
        break;

devices_text_:

std::map<std::string, Generic <LCDText>*> devices_text_;

LCDControl::ConfigSetup(),

void LCDControl::ConfigSetup() {
    if(!CFG_Get_Root()) return;
    Json::Value::Members keys = CFG_Get_Root()->getMemberNames();

    for(std::vector<std::string>::iterator it = keys.begin(); it != keys.end(); it++ ) {
        if(it->find("display_", 0) != std::string::npos) {
            Json::Value *display = CFG_Fetch_Raw(CFG_Get_Root(), it->c_str());
            Json::Value *driver = CFG_Fetch_Raw(display, "driver");
            if(!driver) {
                Error("CFG: Must specify driver <%s>", it->c_str());
                continue;
            }
            Json::Value *rows = CFG_Fetch_Raw(display, "rows", new Json::Value(-1));
            /*if(!rows->isNumeric() || rows->asInt() == -1) {
                Error("Display <%s> requires number of rows to initialize.", it->c_str());
                delete display;
                delete driver;
                continue;
            }*/
            Json::Value *cols = CFG_Fetch_Raw(display, "cols", new Json::Value(-1));
            /*if(!cols->isNumeric() || rows->asInt() == -1) {
                Error("Display <%s> requires number of columns to initialize.", it->c_str());
                delete display;
                delete driver;
                delete rows;
                continue;
            }*/

            Json::Value *model = CFG_Fetch_Raw(display, "model");
            if(driver->asString() == "crystalfontz开发者_开发技巧") {
                if(model) {
                    devices_text_[*it] = DrvCrystalfontz::Get(*it, this,
                        CFG_Get_Root(), model->asString()); // line 71
                } else {
                    Error("Device <%s> requires a model.", it->c_str());
                    delete display;
                    delete driver;
                    delete rows;
                    delete cols;
                    continue;
                }
            } else if(driver->asString() == "qt") {
                devices_text_[*it] = new DrvQt(*it, this, CFG_Get_Root(),
                    rows->asInt(), cols->asInt());

            } else if(driver->asString() == "pertelian") {
                //devices_text_[*it] = new DrvPertelian(this, CFG_Get_Root(), rows->asInt(), cols->asInt());

            } else
                continue;
            if(model) delete model;
            delete display;
            delete driver;
            delete rows;
            delete cols;
        }

    }

    for(std::map<std::string, Generic<LCDText> *>::iterator it =
        devices_text_.begin(); it != devices_text_.end(); it++) {
        display_keys_.push_back(it->first);
        Error("Starting <%s> %p", it->first.c_str(), it->second);
        Generic<LCDText> *device = it->second;
        device->CFGSetup(it->first);
        device->Connect();
        device->SetupDevice();
        device->BuildLayouts();
        device->StartLayout();
    }
}

---

I'm guessing that Protocol1 et al. are subclasses of some SuperProtocol?

devices_text_[*it] goes through assuming that it contains something of type SuperProtocol, so delete devices_text_[*it] calls SuperProtocol::~ SuperProtocol().

However, what you really want to call is Protocol1::~Protocol1() if you're destructing a Protocol1; this only works if you mark SuperProtocol::~ SuperProtocol() as virtual.

---

Instead of going through the keys, finding it in the map, then deleting it, why not just iterate through the map deleting as you go? I'd make a functor and use for_each (this isn't a guideline or anything, just my opinion),

typedef Generic<LCDText> GenericLCDText;
typedef std::map<std::string, GenericLCDText*> GenericLCDTextMap;
typedef GenericLCDTextMap::value_type GenericLCDTextPair;

struct device_text_deleter : std::unary_function<const GenericLCDTextPair&, void>
{
    void operator()(const GenericLCDTextPair& pPair)
    {
        Error("Deleting %s %p", pPair.first.c_str(), pPair.second);

        delete pPair.second;        
    }
}

std::for_each(devices_text_.begin(), devices_text_.end(), device_text_deleter());
_devices.text_.clear(); // optional, removes the deleted pointers. unnecessary
                        // if this is run in the destructor, since map goes away
                        // shortly after

That said, you're code would be improved by the following:

// typedef's for readability (would be in header, maybe private)
typedef std::vector<std::string> StringVector;
typedef Generic<LCDText> GenericLCDText;
typedef std::map<std::string, GenericLCDText*> GenericLCDTextMap;

for(StringVector::iterator it = display_keys_.begin();
    it != display_keys_.end(); it++)
{
    // find first! otherwise you're looking up the pair every time
    GenericLCDTextMap::iterator pair = devices_text_.find(*it);

    if (p != devices_text_.end())
    {
        // operator-> is overloaded for iterators but might as well use
        // the pair now.
        Error("Deleting %s %p", pair->first.c_str(), pair->second);

        // no need to check for null, delete null is a-ok
        delete pair->second;
    }
}

Hopefully this will make it easier to spot the errors. Make sure any base classes you use have virtual destructors.

Check you haven't added a string in the vector twice (this would be "fixed" buy just iterating through the map, though you'll want to find out why duplicates exist in the first place), etc.

I've never tried this before, but maybe add a double delete macro thing (totally untested):

#define DOUBLE_DELETE_GAURD static bool x = false; assert(x == false); x = true;

Then just add it to your destructor. If you double delete, and the static bool is still around, the assertion will fail. This is completely in undefined la-la land, though.

---

Could it be that

1. display_keys_ contains the same string more than once and
2. this string has an associated Generic <LCDText>* in devices_text_?

In this case the pointer would be given to delete twice and this isn't legal...