Destructor causing segmentation fault

Question

I have implemented a class string, similar to std::string one. I have a problem when the destructor is called: the field length has the length of the characters allocated in field. This is the class:

class indexException:public std::exception
{
    public:
    virtual const char* what()
    {
        return "Index is either too long, or negative";
    }
};

class string
{
    public:
    static const unsigned int length_max=100;
    string(const char* field=NULL)
    {
        if(field!=NULL)
        {
            const unsigned int length=strlen(field);
            this->field=new char[length+1];
            this->length=length;
            for(unsigned int i=0;i<=length;i++)
                this->field[i]=field[i];
        }
        else
        {
            this->field=NULL;
            length=0;
        }
    }
    string(string& str)
    {
        string(str.field);
    }
    ~string()
    {
        if(length>0)
            delete field;
    }
    char& operator[] (int i) const throw()
    {
        try
        {
            if(i<0 || i>=(int)length)
                throw indexException();
        }
        catch(indexException& e)
        {
            std::cerr << e.what() << std::endl;
        }
        return field[i];
    }
    string& operator=(const char* field)
    {
        const unsigned int length=strlen(field);
        if(this->length>0)
            delete this->field;
        this->field=new char[length];
        this->length=length;
        for(unsigned int i=0;i<length;i++)
            this->field[i]=field[i];
        return *this;
    }
    string& operator= (const string& str)
    {
        if(this!=&str)
            *this=str.field;
        return *this;
    }
    operator char* ()
    {
        return field;
    }
    friend std::ostream& operator<< (std::ostream& out, string& str);
    friend std::istream& operator>> (std::istream& in, string& str);
    public:
    unsigned int length;
    char* field;
};

std::ostream& operator<<(std::ostream& out, string& str)
{
    out << str.field;
    return out;
}

std::istream& operator>> (std::istream& in, string& str)
{
    char temp[string::length_max];
    in >> temp;
    str=temp;
    return in;
}

If I use the assignment operator, this doesn't cause a segmentation fault. But it undirectly cause it. I explain how:

int main(int argc,char** argv)
{
    string str="hi";
    string str2=str;
    return 0;
}

Putting a breakpoint into the assignment operator overloading, I realized that the assigment operator doesn't cause segmentation fault. The problem is after, when exiting from main. If I remove the destructor I don't get this segmentation fault, but I would know why I get this problem.

Edit: I have understood where's the problem. I followed your suggestions but it still goes to segmentation fault. But now it doesn't crash anymore on the destructor method, but on the assignment operator overloading:

    string& operator=(const char* field)
    {
        unsigned int length=0;
        if(field!=NULL)
            length=strlen(field);
        else
            field="";
        if(this->length>0)
            delete[] this->field;
        this->field=new char[length+1];
        this->length=length;
        strcpy(this->field,field);
        return *this;
    }

The problem is when I delete this->field, the debugger stops there. An example of segmentation fault:

string str("hi");
string str2=str;

This causes segmentation fault.I suppone it's because str2 is not initialized, and length has an undefined value. If I instead do this:

string str("hi");
string str2;
str2=str;

There isn't any segmentation fault.Why? I thought that calling :

string str2;

Was also calling the constructor, or is that the "=" operator has the precedence? How to solve this?

PS: I also changed other things,like the copy constructor. Full code is here: http://pastebin.com/ubRgaVr8

Solved: I changed the copy constructor as suggested in the accepted reply:

    string(const string& str)
    {
        length=str.length;
        field=new char[str.length+1];
        memcpy(field,str.field,length+1);
    }

Answer 1

Your copy constructor doesn't initialise the object.

string(string& str)
{
    string(str.field); // Does nothing
}

string(str.field)creates an unnamed stringand immediately throws it away. It does not initialise this object using a different constructor.

Since your object now consists only of randomness, bad things will happen when you try to destroy it.

To make sure things are initialised, make a private member function

void initializeFromChars(const char* cString);

that does the work and use it in your constructors and assignment operator.

Answer 2

Once you allocate memory with

field = new char[length+1];

You should delete it with:

 delete [] field;

And you're not checking whether your allocation was successful.

Another thing considered good practice is setting field to NULL after delete so it won't get deleted twice (if you start delivering classes) for example:

~string(){
    delete [] field;
    // field = NULL;
}

Note: according to Dietmar Kühl setting field=NULL isn't good practice (take a look at the comments) and choose your way, here's question specifically about this: Is it worth setting pointers to NULL in a destructor? .

Note 2: KerrekSB pointed out that delete [] field will do nothing if pointer is NULL and whole condition is unnecessary.

Than in string& operator=(const char* field) you probably want to allocate length + 1 and iterate to it too (to include terminating NULL).

And I don't like your string& operator= (const string& str), you have cached info on length of string and you're using strlen() and than manual copy char by char.

Your copy constructor also looks bad... You should "copy" manual allocation and copy byte by byte to it. Or rather build protected function like fromCString(const char *) and use it in both constructors and assign operators.

If those doesn't help ask in comment for more help.

Answer 3

Your destructor uses delete, when it should use delete[].

Answer 4

EDIT: Scrapped my previous answer, as it was incorrect.

The problem appears to be the copy constructor, you are passing the field from the source instance as though it is merely another null terminated char*, but it isn't.

You don't copy the null character at the end during the char* assignment invoked by the previous statement, you use an internal length field instead, and copy only that many bytes.

so your copy constructor should be:

string(string& str)
{
   length = str.length;
   field = new char[length];
   memcpy(field, str.field, length);
}

or, if you want to preserve compatibility with null terminated functions, and you have ensured that the null is kept for all other assignments/constructors, etc:

string(string& str)
{
   length = str.length;
   field = new char[length + 1];
   memcpy(field, str.field, length + 1);
}

In fact, the mixing null terminated, and specified length strings so much throughout your class appears to be confusing you.

I would create an internal, private, single disposal method, and an array of methods to set various source types, and have the constructors, assignment operators, and destructors use those instead.

That way you only have a single places where any given operation occurs, rather than juggling many minor variations on the same functionality. For example:

private:
    void internalSet(const char *source) {
        if (source == NULL) {
            length = 0;
            field = NULL;
        }else{
            length = strlen(source);
            field = new char[length];
            memcpy(field, source, length);
        }
    }

    void internalSet(const string &source) {
        length = source.length;
        if (length > 0) {
            field = new char[length];
            memcpy(field, source.field, length);
        }else{
            field = NULL;
        }
    }

    void internalDispose() {
        delete[] field;
    }

public:
    string() : field(NULL), length(0) {}

    string(const string& source) { internalSet(source); }
    string(const char *source) { internalSet(source); }

    ~string() { internalDispose(); }

    string& operator=(const char *source) {
        internalDispose();
        internalSet(source);
        return *this;
    }

    string& operator=(const string &source) {
        internalDispose();
        internalSet(source);
        return *this;
    }

    void clear() {
        internalDispose();
        length = 0;
    }