Segmentation Fault on Delete []

[vvt72277]

My problem is that I keep a gettting a segmentation fault when I run the destructor for the PriceFeed object. It occurs on the following line: "delete [] _inputRecords[i];". I can tell because it runs fine after I comment out this line. Can someone please help? THANK YOU IN ADVANCE!!!!

/* PriceFeed.h */
#ifndef PRICEFEED_H_INCLUDED
#define PRICEFEED_H_INCLUDED

#include "priceRecord.h"

class PriceFeed
{
private:
    // PriceFeed contains _nDates by _nSecs PriceRecord variables
    int _nDates;    // rows
    int _nSecs;     // columns

    PriceRecord **_inputRecords;

public:
    bool _hasMore(std::istream& is);
    bool _isEnd;

    PriceFeed() {}
    PriceFeed(std::istream& is, int numDates, int numSecs);

    ~PriceFeed();

    PriceFeed& operator=(const PriceFeed& rhs);

 }; /* PriceFeed */

#endif // PRICEFEED_H_INCLUDED 

/* priceFeed.cpp */
#include "sim.h"
#include "priceFeed.h"
#include "priceRecord.h"

using std::cout;    using std::endl;    using std::cin;

PriceFeed::PriceFeed(std::istream& is, int numDates, int numSecs)
{
    _nDates = numDates;
    _nSecs = numSecs;

    PriceRecord** localPtr;

    localPtr = new PriceRecord*[_nDates];
    for (int i = 0; i < numDates; i++) {
        localPtr[i] = new PriceRecord[_nSecs];
    }

    _inputRecords = localPtr;

    std::vector<std::string> myTickers;
    std::string tempTicker;

    for (int i = 0; i <= _nSecs; i++) {
        is >> tempTicker;
        myTickers.push_back(tempTicker);
    }

    for (std::vector<std::string>::const_iterator it = myTickers.begin();
            it != myTickers.end(); it++) {
                cout << *it << endl;
    }


    for (int i = 0; i < _nDates; i++)  {
        for (int j = 0; j < _nSecs; j++)    {

//            _inputRecords[i][j] =
        }
    }
}

PriceFeed::~PriceFeed()
{
    for (int i = 0; i < _nDates; i++) {
        delete [] _inputRecords[i];
    }
    delete [] _inputRecords;
}


bool PriceFeed::_hasMore(std::istream& is)
{
    if (is) {
        is.unget();
        return true;
    } else {
        return false;
    }
}

PriceFeed& PriceFeed::operator=(const PriceFeed& rhs)
{
    _nDates = rhs._nDates;
    _nSecs = rhs._nSecs;
    _inputRecords = rhs._inputRecords;
    return *this;
}

/* PriceRecord.h */
#ifndef PRICERECORD_H_INCLUDED
#define PRICERECORD_H_INCLUDED

class PriceRecord
{
private:
    std::string _ticker;
    double _price;

public:
    // constructors
    PriceRecord();
    PriceRecord(const std::string& ticker, const double price);
    PriceRecord(std::istream& is) { _readFromStream(is); }
    // destructor
    ~PriceRecord();

    // assignment operator
    PriceRecord& operator= (const PriceRecord& rhs);
    


    double _printPrice() const { return _price; }
    std::string _printTicker() const { return _ticker; }



    std::istream& _readFromStream(std::istream& is);

}; /*PriceRecord */


#endif // PRICERECORD_H_INCLUDED 

/* priceRecord.cpp */
#include "sim.h"
#include "priceRecord.h"

PriceRecord::PriceRecord()
{
    _ticker = "AAPL";
    _price = 20;
}

PriceRecord::PriceRecord(const std::string& ticker, const double price)
{
    _ticker = ticker;
    _price = price;
}

PriceRecord& PriceRecord::operator= (const PriceRecord& rhs)
{
    _ticker = rhs._ticker;
    _price = rhs._price;

    return *this;
}

PriceRecord::~PriceRecord()
{
    // do nothing
}


std::istream& PriceRecord::_readFromStream(std::istream& is)
{
    is >> _ticker >> _price;
    return is;
}

[bluezor]

I think it's because if you use delete [] you can only delete the whole thing and you can't delete a part of it like an array.
So only delete [] _inputRecords works and not
delete [] _inputRecords[i]

[vvt72277]

Are you sure? Because here is a toy example that works just fine ....

#include <iostream>
#include <string>

using namespace std;

int main ( int iArgc, char ** ppcArgv ) {

		int counter = 0;
		int dim1 = 5;
		int dim2 = 10;
		int **k = new int* [5];
		for( int m = 0; m < dim1; m++ ) {
			k[ m ] = new int[ dim2 ];
			for( int p = 0; p < dim2; p++ ) {
				k[ m ][ p ] = counter++;
				printf( "k[ %d ][ %d ] = %d\n", m, p, k[ m ][ p ] );
		}    }

		for( int m = 0; m < dim1; m++ ) {
			delete[] k[ m ];
                                 printf( "deleting pointers \n");
		}
		delete[] k;

	return( 0 );
}

[bluezor]

Tbh.. i'm not completely sure.. so it's time to wait for someone more experienced to come on and help you xD

[helios]

Yes, you can only delete[] entire arrays. Passing a pointer that doesn't point to the beginning of an array has undefined behavior.

EDIT: Oh, but line 23 in the last example is indeed deleting an entire array. bluezor really needs to pay more attention.

The problem is line 67 in priceFeed.cpp. Assigning pointers without transferring ownership is just asking for trouble. You need to properly allocate the array for the new object and copy the data between the arrays.

[vvt72277]

So if I have the following pointer to pointers
int **myPtr;

Using the below line deletes both dimensions of the array?
delete [] myPtr;

Please let me know. And thank you so much (both bluezor and helios).

[helios]

See my edit.

[vvt72277]

line 23 is in my toy example that works perfectly fine. I posted it as a counter-example to what bluezor said ... which is that the following pointer to pointers:

int **myPtr;

is freed with

delete [] myPtr; // i.e. the loop is unnecessary

Is this what you guys are saying?

[helios]

You're doing it fine. bluezor misread delete [] _inputRecords[i] as delete [] _inputRecords+i. That would have been invalid.

Back to your original question,

The problem is line 67 in priceFeed.cpp. Assigning pointers without transferring ownership is just asking for trouble. You need to properly allocate the array for the new object and copy the data between the arrays.

[vvt72277]

I deleted the assignment operator altogether from the class and the implementation. Still getting the same error and when I comment that line (priceFeed.cpp line 47) it goes away. Suggestions?

[PanGalactic]

Helios is correct. You need to allocate the memory for inputRecords in your assignment operator, then copy the data from rhs. With the way it is currently implemented, every assignment will eventually result in calling delete an additional time for the same pointer. That is what is causing the segfault.

Why not just use vectors and avoid the new/delete brain damage?

[vvt72277]

Well I removed the assignment operator and it is still giving the same error ... anything else that could be causing this?

[PanGalactic]

Declare the assignment operator and copy constructor as private and unimplemented and then tell us what happens.

[vvt72277]

How exactly do I do this? I am assuming that commenting out the assignment operator's declaration and implementation isn't enough ... what should I do? and btw, I don't think I have a copy constructor - should add one?

[helios]

It's because there's a default assignment operator defined by the compiler which does exactly what yours does. If you declare yours as private, the compiler can't define its own and you can't make assignments to objects of that type.

Why are you disabling the assignment operator? PanGalactic and I already told what's wrong.

[vvt72277]

Ok here is what I did:

1) I moved the following to the private area of the PriceFeed class:

    PriceFeed& operator=(const PriceFeed& rhs);
    PriceFeed(const PriceFeed& p);

2) Commented out that implementation of assignment operator.

Still got the same error. *Sorry*

[vvt72277]

Further, I removed the assignment operator and copy constructor from private and implemented them with the pasted code below. I still got the same error. PLEASE HELP!

PriceFeed& PriceFeed::operator=(const PriceFeed& rhs)
{
    _nDates = rhs._nDates;
    _nSecs = rhs._nSecs;

    for (int i = 0; i < _nDates; i++) {
        for (int j = 0; j < _nSecs; j++) {
            _inputRecords[i][j] = rhs._inputRecords[i][j];
        }
    }
    return *this;
}

PriceFeed::PriceFeed(const PriceFeed& rhs)
{
    _nDates = rhs._nDates;
    _nSecs = rhs._nSecs;

    _inputRecords = new PriceRecord*[_nDates];
    for (int i = 0; i < _nDates; i++) {
        _inputRecords[i] = new PriceRecord[_nSecs];
    }

    for (int i = 0; i < _nDates; i++) {
        for (int j = 0; j < _nSecs; j++) {
            _inputRecords[i][j] = rhs._inputRecords[i][j];
        }
    }
}

[vvt72277]

I tried making the changes and am still stuck (going on four days now). Any thoughts on what could be causing the segmentation fault issue?

If needed, I can update what the original four files (i.e. priceFeed.h, priceFeed.cpp, priceRecord.h and priceRecord.cpp) look like after revising the class declaration and implementation using the changes that helios and PanGalactic indicated.

[guestgulkan]

yes - please post the updated files

[vvt72277]

UPDATED FILES - the problem occurs in line 48 of priceFeed.cpp (in the destructor for the priceFeed object)

Original Problem

My problem is that I keep a gettting a segmentation fault when I run the destructor for the PriceFeed object. It occurs on the following line: "delete [] _inputRecords[i];". I can tell because it runs fine after I comment out this line. Can someone please help?

THANK YOU IN ADVANCE!!!!

/* priceFeed.h */
#ifndef PRICEFEED_H_INCLUDED
#define PRICEFEED_H_INCLUDED

#include "priceRecord.h"

class PriceFeed
{
private:
    // PriceFeed contains _nDates by _nSecs PriceRecord variables
    int _nDates;    // rows
    int _nSecs;     // columns

    PriceRecord **_inputRecords;

public:
    bool _hasMore(std::istream& is);
    bool _isEnd;

    PriceFeed() {}
    PriceFeed(std::istream& is, int numDates, int numSecs);

    PriceFeed& PriceFeed::operator=(const PriceFeed& rhs);
    PriceFeed::PriceFeed(const PriceFeed& p);

    ~PriceFeed();

}; /* PriceFeed */

#endif // PRICEFEED_H_INCLUDED 

/* priceFeed.cpp */
#include "sim.h"
#include "priceFeed.h"
#include "priceRecord.h"

using std::cout;    using std::endl;    using std::cin;

PriceFeed::PriceFeed(std::istream& is, int numDates, int numSecs)
{
    _nDates = numDates;
    _nSecs = numSecs;

    _inputRecords = new PriceRecord*[_nDates];
    for (int i = 0; i < _nDates; i++) {
        _inputRecords [i] = new PriceRecord[_nSecs];
    }

    std::vector<std::string> myTickers;
    std::string tempTicker;

    do {
        is >> tempTicker;
        myTickers.push_back(tempTicker);

        char ch;
        is.get(ch);
        if(ch == '\n') break;
        else is.putback(ch);
    } while (1==1);

    std::string date;
    for (int i = 0; i < _nDates; i++)  {
        cout << '(' << i + 1 << ')' << endl;
        is >> date;
        for (int j = 0; j < _nSecs; j++)    {
            _inputRecords[i][j]._setTicker(myTickers[j]);
            _inputRecords[i][j]._setPriceFromStream(is);
        cout << _inputRecords[i][j]._getTicker() << '\t' <<
            _inputRecords[i][j]._getPrice() << endl;
        }
        cout << endl << endl;
    }
}

PriceFeed::~PriceFeed()
{
    for (int i = 0; i < _nDates; i++) {
        delete [] _inputRecords[i];
    }
    delete [] _inputRecords;
}


bool PriceFeed::_hasMore(std::istream& is)
{
    if (is) {
        is.unget();
        return true;
    } else {
        return false;
    }
}


PriceFeed& PriceFeed::operator=(const PriceFeed& rhs)
{
    _nDates = rhs._nDates;
    _nSecs = rhs._nSecs;

    for (int i = 0; i < _nDates; i++) {
        for (int j = 0; j < _nSecs; j++) {
            _inputRecords[i][j] = rhs._inputRecords[i][j];
        }
    }
    return *this;
}

PriceFeed::PriceFeed(const PriceFeed& rhs)
{
    _nDates = rhs._nDates;
    _nSecs = rhs._nSecs;

    _inputRecords = new PriceRecord*[_nDates];
    for (int i = 0; i < _nDates; i++) {
        _inputRecords[i] = new PriceRecord[_nSecs];
    }

    for (int i = 0; i < _nDates; i++) {
        for (int j = 0; j < _nSecs; j++) {
            _inputRecords[i][j] = rhs._inputRecords[i][j];
        }
    }
}

/* PriceRecord.h */
#ifndef PRICERECORD_H_INCLUDED
#define PRICERECORD_H_INCLUDED

class PriceRecord
{
private:
    std::string _ticker;
    double _price;

public:
    // constructors
    PriceRecord();
    PriceRecord(const std::string& ticker, const double price);
    PriceRecord(std::istream& is) { _setPriceFromStream(is); }
    // destructor
    ~PriceRecord();

    // assignment operator
    PriceRecord& operator= (const PriceRecord& rhs);

    void _setPrice(double Price) { _price = Price; }
    void _setTicker(std::string Ticker) { _ticker = Ticker; }
    double _getPrice() const { return _price; }
    std::string _getTicker() const { return _ticker; }

    std::istream& _setPriceFromStream(std::istream& is);

}; /*PriceRecord */

#endif // PRICERECORD_H_INCLUDED 

/* priceRecord.cpp */
#include "sim.h"
#include "priceRecord.h"

PriceRecord::PriceRecord()
{
    _ticker = "AAPL";
    _price = 20;
}

PriceRecord::PriceRecord(const std::string& ticker, const double price)
{
    _ticker = ticker;
    _price = price;
}

PriceRecord& PriceRecord::operator= (const PriceRecord& rhs)
{
    _ticker = rhs._ticker;
    _price = rhs._price;
    return *this;
}

PriceRecord::~PriceRecord()
{
    // do nothing
}

std::istream& PriceRecord::_setPriceFromStream(std::istream& is)
{
    is >> _price;
    return is;
}