Heap Corruption trying to add elements to existing std::vector

Question

I have an std::vector containing elements of class BoundingBox (std::vector<BoundingBox>, which I call BB_Array). First, i create this vector using a function that I will simplify here:

BB_Array* Detector::generateCandidateRegions(BB_Array* candidates){
    BB_Array* result = new BB_Array(); // without new here, i get segmentation fault
    BB_Array tempResult;

    // apply tests to candidates and add the good ones to tempResult ... 
    // ... using tempResult.push_back((*candidates)[i]);

    for (int i=0; i < tempResult.size(); i++){
        // apply more tests to tempResult[i] and, if necessary, add it...
        // ... using result->push_back(maxCandidate); 
    }

    return result;
}

This function works and there is no memory leak according to valgrind. Now, that function needs to be applied once (for performance) and I need a function to add elements to it. This is where I'm having problems. The way I'm doing it now is:

BB_Array* Detector::addCandidateRegions(BB_Array* candidates) {
    BB_Array* result = new BB_Array();
    BB_Array sparseDetections;

    // apply tests and add stuff to sparseDetections using... 
    // ... sparseDetections.push_back((*candidates)[i]);

    for (int i=0; i < sparseDetections.size(); i++){
        // add things to result using result->push_back()
    }

    return result;
}

This second function gave me the candidates I need to add to the vector created before:

BB_Array *newCandidates = addCandidateRegions(...);
if (newCandidates->size() > 0){
   denseCandidates->insert(denseCandidates->end(), newCandidates->begin(), newCandidates->end());
   delete newCandidates;
}

Now, this is causing heap corruption and the program crashes after something like 500 images. So what am I doing wrong? Is there a better way of doing this?

I also have a function to remove elements from the vector afterwards, but I think I can figure it out once I get the adding part done.

EDIT: forgot to put the error message:

*** Error in `./C_Arnoud_Calibrated_Detector': malloc(): smallbin double linked list corrupted: 0x0000000001f4eed0 ***
Aborted (core dumped)

doesn't happen at the same iteraction every time and, sometimes, I get a segmentation fault instead.

EDIT 2: I fixed it today. No more heap problems. I was probably tired and used the wrong index in one specific scenario, so out of thousands of iteractions sometimes unexpected things happened and it broke everything. Thanks everybody for the suggestions and, if you use an object detector, it is now safe to use =).
https://github.com/CArnoud/C_Arnoud_Calibrated_Detector

Just stop using new/delete. There is no need for it here. Pass the vectors by (const) reference to your function and return them by value. — D Drmmr
Can you try extracting and posting the minimal, compiling example that misbehaves? I don't see anything causing memory problems at this level of abstraction. (Delete inside if in the last sample is a little suspicious, but with the right code around the sample, it will work ok). — sbarzowski
@DDrmmr if I take away the new, i get a segmentation fault error — Charles Arnoud
@Charles: Don't just take away new -- completely change over the API and implementation so it's all in terms of values and references. — user1084944
Does your BB_Array destructor delete any memory? If you copy one, will it copy internal pointers? That's a common but disastrous combination. — sje397

Félix Cantournet Félix Cantournet · Accepted Answer · 2015-03-14T16:08:12

First off, do you use a compiler that is older than you ?

Yes -> Stop doing that. Why are you hating yourself so much ?

No -> Good news, everything your professors have "taught" you about C++ is wrong and false. Compilers are very good at return value optimization. Make your functions return values, they will be moved and not copied, which is essentially free, and stop this new/delete/raw pointer madness in application code.

BB_Array Detector::generateCandidateRegions(BB_Array& candidates){
    BB_Array result; // Use values.
    BB_Array tempResult;

    // Do whatever the heck you want.

    for (int i=0; i < tempResult.size(); i++){
        // apply more tests to tempResult[i] and, if necessary, add it...
        // ... using result.push_back(maxCandidate); 
    }

    return result;
}

BB_Array newCandidates = addCandidateRegions(...);
if (newCandidates.size() > 0){
   denseCandidates.insert(denseCandidates.end(), newCandidates.begin(),
                          newCandidates.end());
}

Easy memory management, easy life.

Heap Corruption trying to add elements to existing std::vector

1 Answers