3

I have the following class:

class Patient {
public:
    Patient(int x);
    ~Patient();
private:
    int* RP;
};

Patient::Patient(int x) { RP = new int [x]; }
Patient::~Patient() { delete [] RP; }

I create an instance of this class on the stack as follows:

void f() { Patient p(10); }

Now, when f() returns, I get a "double free or corruption" error, which signals to me that something is attempted to be deleted more than once. But I don't understand why that would be so. The space for the array is created on the heap, and just because the function from inside which the space was allocated returns, I wouldn't expect the space to be reclaimed.

I thought that if I allocate space on the heap (using the new keyword), then the only way to reclaim that space is to use the delete keyword. Help!

As requested, here is the actual code (slightly abridged for brevity's sake)

Here's the full class definition (split across a .cpp and .h file, but shown together):

class Patient {

public:
    Patient(int numDecisionEpochs);

    ~Patient();

    void recordRP(const int& age, const bool& t);
    void recordBiopsy(const int& age, const int& result);
    void recordPSA(const int& age, const double& level);
    void recordPSA(const int& age);
private:
    int* RP;
    int* Biopsy;
    double* PSA;
};

Patient::Patient(int numDecisionEpochs) {
    RP = new int [numDecisionEpochs];
    Biopsy = new int [numDecisionEpochs];
    PSA = new double [numDecisionEpochs];
}

Patient::~Patient() {
    delete[] RP;
}

void Patient::recordRP(const int& age, const bool& t) {
    if(t)
    RP[age-1] = 1;  // RP either yes (1) or no (0)
    else
    RP[age-1] = 0;
}

void Patient::recordBiopsy(const int& age, const int& result) {
    switch(result)
    {
    case 0:
    case 1:
    case 2:
    case 3:
    case 4:
        Biopsy[age-1]=result; // only permit results 0,1,2,3,4
        break;
    default:
        cerr << "Invalid biopsy result (" << result << ") at age " << age << "!\n";
    }
}

void Patient::recordPSA(const int& age, const double& level) {
    PSA[age-1] = level; // record PSA volume
}

void Patient::recordPSA(const int& age) {
    PSA[age-1] = -1; // symbol for no screening during epoch
}

Next, the function where the above class is used. The following function is called directly from main() and passed a Policy object which is completely independent and separate from the Patient class:

void simulate1(Policy& P)
{
    // ...
    Patient patient(260);

    for(int idx=0; idx<(P.size); idx++)
    {
        while(state != 9) // while patient not dead
        {
                // ...
                patient.recordPSA(age,PSA);
                // ...
                patient.recordPSA(age);
                // ...
                patient.recordBiopsy(age,biopsyResult);
                // ...
                patient.recordRP(age,true);
                // ...
                patient.recordRP(age,false);
                // ...

        } // end patient (while loop)

    } // end sample (for loop)

} // end function
Unihedron
  • 10,902
  • 13
  • 62
  • 72
synaptik
  • 8,971
  • 16
  • 71
  • 98
  • Your code runs fine: http://ideone.com/Gw2QM. And technically `delete` is not guaranteed to reclaim any space, it's possible you could have a `new` and `delete` implementation that just keep on allocating memory pages. – ta.speot.is Apr 10 '12 at 23:34

3 Answers3

7

You're violating the rule-of-three (or for C++11, the rule-of-five). You need a copy constructor and copy-assignment operator that does a deep copy of the pointer. Of course, since you don't track the size of the array you allocate, this isn't possible without introducing a second data member.

Community
  • 1
  • 1
ildjarn
  • 62,044
  • 9
  • 127
  • 211
  • It's also possible that the most correct thing would be to disallow copying, i.e. add `private: Patient(const Patient&);` to the class definition. Especially when it comes to objects manually handling pointers, unintentional copying can be just wrong. – Steve Howard Apr 10 '12 at 23:41
  • 1
    @Steve : The most likely scenario, IMO, is that the OP should just be using `std::vector` instead of `int*`. ;-] – ildjarn Apr 10 '12 at 23:53
  • What is "OP", ordinary person? If I use std::vector and the "reserve()" member function to pre-allocate space, should I expect to observe no difference in speed over the arrays and pointers method I'm using? (Assume that the code using the array or vector is the bottleneck.) – synaptik Apr 10 '12 at 23:56
  • @synaptik : Original Poster, aka you. :-] And yes, once you implement proper copy/move semantics, if you use `reserve` properly there will be no perceptible difference between `std::vector<>` and raw C-arrays in release mode. – ildjarn Apr 10 '12 at 23:56
  • @ildjarn, I agree (and commented below) but remember that copying a vector is expensive and probably not what you want most of the time (hence my "disallow copying" suggestion). – Steve Howard Apr 11 '12 at 00:25
  • @Steve : If the class should remain non-copyable, agreed. But if the class needs to make a deep copy either way, there's no remaining reason to not use `std::vector<>`. – ildjarn Apr 11 '12 at 00:27
  • In the code I posted above, where is the "copying" taking place? – synaptik Apr 11 '12 at 02:46
2

This doesn't answer your question directly, but also consider using std::vector.

bluedog
  • 935
  • 1
  • 8
  • 24
  • 3
    It's a perfectly valid suggestion since the author clearly isn't very experienced with C++. One of the mistakes of learning C++ is to treat it like C and then learn how to do classes (so you manage the memory for a list of integers yourself). This isn't a direct answer but I think in C++ this kind of answer should be encouraged as well. – Steve Howard Apr 10 '12 at 23:39
  • 1
    Unfortunately, I don't have enough rep to just make a comment. I can only answer. – bluedog Apr 10 '12 at 23:39
  • Yes, I do use vectors. But this code is at the center of a simulation which is run millions and millions of times (embedded in a heuristic optimization), and so I only used arrays because some people have told me they're slightly faster (though others tell me otherwise, especially for newer C++ implementation). – synaptik Apr 10 '12 at 23:45
  • 1
    @synaptik : If you use `reserve` properly, anyone who suggests raw pointers are faster than `std::vector<>` in release mode are just spouting FUD. – ildjarn Apr 10 '12 at 23:53
  • @SteveHoward Your statement should read *This isn't a direct answer but I think in C++ this kind of comment should be encouraged as well.* – ta.speot.is Apr 11 '12 at 02:35
0

There is nothing in any of the record... methods that bounds checks the age. So, if age happens to be greater than 260 or less than 0 in your example, you will write past the bounds of either RP or Biopsy or PSA. That leads directly to a "double free or corruption" error.

MSN
  • 53,214
  • 7
  • 75
  • 105
  • Good point, however I know that age doesn't happen to exceed its bounds. For example, if I simply comment out the delete statement, the program runs without error. (And, yes, I'm talking about using precisely the same random number stream.) – synaptik Apr 10 '12 at 23:54
  • @synaptik, just because it doesn't throw an error does not mean it doesn't have one. It just means you didn't trigger any error catching code. Track `numDecisionEpochs` as an additional member of `Patient` and see if you ever pass in `age<0 || age>=numDecisionEpochs`. – MSN Apr 11 '12 at 00:02
  • you're quite right! I should have said that rather than being free from error, it at least doesn't throw throw the double free error. I will double-check though, thanks for the suggestion. – synaptik Apr 11 '12 at 00:09
  • Ah-hah! You are absolutely right! I forgot that age needed to be shifted before indexing into the arrays. It was, in fact, pointing out of range. Thanks for that--forgive my stubbornness :) – synaptik Apr 11 '12 at 00:58