Segmentation fault: 11 and malloc errors in C++ code

Question

Ok, so I know there are probably a lot of errors in this code. I'm pretty new to dynamic memory allocation, pointers, etc.

The header file, account.h, is given to us by our professor. We were told not to make any changes to the .h file.

The implementation file is written by me. The main function is included just for basic initial testing. We were given another file to actually test the implementation of the account class.

If I don't comment out the cout name line, I get a seg fault 11 error. If I do, it'll print the account number, but throw this error:

Test(29976) malloc: * error for object 0x62c1aa18c9d8374: pointer being freed was not allocated* set a breakpoint in malloc_error_break to debug
Abort trap: 6

Any help at all would be greatly appreciated!

Here's the header file:

class account
{
public:
    typedef char* string;
    static const size_t MAX_NAME_SIZE = 15;
    // CONSTRUCTOR
    account (char* i_name, size_t i_acnum, size_t i_hsize);
    account (const account& ac);
    // DESTRUCTOR
    ~account ( );
    // MODIFICATION MEMBER FUNCTIONS
    void set_name(char* new_name);
    void set_account_number(size_t new_acnum);
    void set_balance(double new_balance);
    void add_history(char* new_history);
    // CONSTANT MEMBER FUNCTIONS
    char* get_name ( ) const;
    size_t get_account_number ( ) const;
    double get_balance( ) const;
    size_t get_max_history_size( ) const;
    size_t get_current_history_size ( ) const;
    string* get_history( ) const;
    friend ostream& operator <<(ostream& outs, const account& target);
private:
    char name[MAX_NAME_SIZE+1]; //name of the account holder
    size_t ac_number; //account number
    double balance; //current account balance
    string *history; //Array to store history of transactions
    size_t history_size; //Maximum size of transaction history
    size_t history_count; //Current size of transaction history
};

Here is the implementation file:

// File: account.cxx
// Author: Mike Travis
// Last Modified: Mar 3, 2012
// Description: implementation of Account class as prescribed by the file account.h

#include <cstdlib>
#include <stdio.h>
#include <iostream>
#include "account.h"

using namespace std;

//Constructor

account::account(char* i_name, size_t i_acnum, size_t i_hsize){
    string *d_history;
    d_history = new string[i_hsize];

    for(int i = 0; i<i_hsize; i++){
        name[i] = i_name[i];
    }
    ac_number = i_acnum;
    history_size = i_hsize;
    history_count = 0; 
}


account::account(const account& ac){
    string *d_history;
    d_history = new string[ac.history_size];

    for( int i=0; i<ac.get_current_history_size(); i++){
        strcpy(d_history[i], history[i]);
    }

    strcpy(name,ac.get_name());
    ac_number = ac.get_account_number();
    history_size = ac.get_max_history_size();
    history_count = ac.get_current_history_size();
}

account::~account(){    delete [] history;  }

void account::set_name(char* new_name){                 strcpy(name, new_name); }
void account::set_account_number(size_t new_acnum){     ac_number = new_acnum;  }
void account::set_balance(double new_balance){          balance = new_balance;  }
void account::add_history(char* new_history){
    strcpy(history[history_count], new_history);
    history_count++;
}

char* account::get_name() const {
    char* name_cpy;
    strcpy(name_cpy, name);
    return name_cpy;
}

size_t account::get_account_number() const{             return ac_number;       }
double account::get_balance() const{                    return balance;         }
size_t account::get_max_history_size() const{           return history_size;    }
size_t account::get_current_history_size() const{       return history_count;   }
//string* account::get_history() const{                         return *history;        }


int main(){

    account test1("mike travis", 12345, 20);
    //cout<<"\nname: "<< test1.get_name();

    cout<<"\n\nacnum: "<<test1.get_account_number()<<"\n\n";

    return 0;
}

Answer 1

In the destructor of account, you delete the history array. However, in the constructor, you allocate (and leak) an array which is stored in the local variable d_history. You presumably wanted to assign that to the member variable history instead - since you haven't, if you get to the destructor it gives you an error saying that you're freeing history but have never allocated it.

There's a similar error in the copy constructor as well.

There are also other errors in your code as well, which I assume you'll find as you go - get_name(), for example, is not going to work. I suspect the header file is not helping here, but there's not much to be done if you're not supposed to change that.

Answer 2

I've written a little bit code for you and corrected the epic mistakes (even in the header file, sorry ;)). It is still extremely ugly and c-ish, but maybe you can learn something reading it:

#include <cstddef>
#include <ostream>

class account
{
    // The whole class makes no sense, since it has no useful
    // member function or anything like this.
    // Furthermore, the class provides almost full access to all its member variables.
    // At this point one could just make everything public.

    // This is not even exception safe when the constructor throws.
    // A good implementation would use history_entry and history classes, 
    // together with std::string and std::vector/std::deque
    // And it would provide some sort of functionality. ;)

public:
  account(const char* name, unsigned number, std::size_t history_max_size);
  account(const account& other);
  ~account();

    const char* name() const;
  unsigned number() const;
  double balance() const;
    const char* const* history() const;
    std::size_t history_size() const;
  unsigned history_max_size() const;

  void set_name(const char* new_name);
  void set_number(unsigned new_number);
  void set_balance(double new_balance);
  void add_history(const char* new_history);

private:
  char* name_;
  unsigned number_;
  double balance_;
  char** history_;
    std::size_t history_size_;
    const std::size_t history_max_size_;
};

std::ostream& operator << (std::ostream& stream, const account& a);


#include <cassert>
#include <cstring>

account::account(const char* name, unsigned number, std::size_t history_max_size)
    : name_(0)
    , number_(number)
    , balance_(0.0)
    , history_(new char*[history_max_size])
    , history_size_(0)
    , history_max_size_(history_max_size)
{
    assert(name != 0);
    assert(history_max_size != 0);
    set_name(name);
}

account::account(const account& other)
    : name_(0)
    , number_(other.number_)
    , balance_(other.balance_)
    , history_(new char*[other.history_max_size_])
    , history_size_(other.history_size_)
    , history_max_size_(other.history_max_size_)
{
    set_name(other.name_);
    for (std::size_t i = 0; i != other.history_size_; ++i)
    {
        history_[i] = new char[std::strlen(other.history_[i]) + 1];
        strcpy(history_[i], other.history_[i]);
    }
}

account::~account()
{
    delete[] name_;

    for (std::size_t i = 0; i != history_size_; ++i)
        delete[] history_[i];
    delete[] history_;
}

const char* account::name() const
{
    return name_;
}

unsigned account::number() const
{
    return number_;
}

double account::balance() const
{
    return balance_;
}

const char* const* account::history() const
{
    return history_;
}

std::size_t account::history_size() const
{
    return history_size_;
}

unsigned account::history_max_size() const
{
    return history_max_size_;
}

void account::set_name(const char* new_name)
{
    if (name_)
        delete[] name_;

    name_ = new char[std::strlen(new_name) + 1];
    std::strcpy(name_, new_name);
}

void account::set_number(unsigned new_number)
{
    number_ = new_number;
}

void account::set_balance(double new_balance)
{
    balance_ = new_balance;
}

void account::add_history(const char* new_history)
{
    if (history_size_ == history_max_size_)
    {
        delete[] history_[0]; // delete oldest entry
        for (std::size_t i = 0; i != history_size_ - 1; ++i)
            history_[i] = history_[i + 1];
        --history_size_;
    }

    history_[history_size_] = new char[strlen(new_history) + 1];
    std::strcpy(history_[history_size_], new_history);
    ++history_size_;
}

std::ostream& operator << (std::ostream& stream, const account& a)
{
    return stream << "account [name: " << a.name() << ", number: " 
        << a.number() << ", balance: " << a.balance() << ']';
}


#include <iostream>

int main()
{
    account a("Hello!", 500, 5);
    a.set_balance(12.546);
    for (int i = 50; i--; )
        a.add_history("Yaaay..");
    //account b = a;
    std::cout << a << '\n';
}