c++ copy constructon bugs when static declaration

Question

I'm struggling with an error related to the copie construction in C++. Here is a code illustrating my question:

//File MYString.h
#ifndef __MYSTRING_H__
#define __MYSTRING_H__

#include <cstring>
#include <iostream>
using namespace std;

class MYString
{
  private:
    char * m_ptr; 
    void allocation(int);
    int m_lg;  

  public: 

    MYString ();
    MYString (const char * );
    ~MYString ();
    MYString(const String & );
};
#endif

//File MYString.cpp
#include "MYString.h"


MYString::MYString ()
{
    m_ptr= NULL;
    m_lg=0;
    allocation (1);         
    m_ptr[0]='\0';
}

void MYString::allocation(int a)
{  
  delete m_ptr;
  m_lg=a;
  m_ptr=new char[m_lg];
}

MYString ::MYString (const char * str)
{
    m_ptr = NULL;
    allocation ( strlen(str)+1 );
    strcpy ( m_ptr, str );
}


//copy constructor
MYString::MYString(const MYString & str) 
{
      allocation(str.m_lg);
      strcpy(m_ptr,str.m_ptr);
}


MYString :: ~MYString()
{
  delete m_ptr;
}


//File main.cpp
#include <MYString.h"
int main()
{ 
    MYString chaine0("Toto");
    MYString chaine1(chaine1); //ERROR free(): invalid size
                               //Abandon (core dumped)

    //However, this works fine
     //MYString * chaine0 = new String("Toto");
     //MYString *chaine0= new String(chaine0);


    return 0;

}

I'm getting an error when I execute the gererated exec : //ERROR free(): invalid size //Abandon (core dumped)

However, when I use the dynamic writing as explained in the main.cpp, this works fine.

Any explanations ?

Thanks

Unrelated to your problems, but all symbols starting with double underscore (e.g. `__MYSTRING_H__`) are reserved in all scoped. See e.g. [What are the rules about using an underscore in a C++ identifier?](https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier) for more information. — Some programmer dude, Nov 29 '18 at 15:00
As for your problem: `new[]` needs to be matched by `delete[]`. Otherwise you have [*undefined behavior*](https://en.wikipedia.org/wiki/Undefined_behavior). — Some programmer dude, Nov 29 '18 at 15:02

score 2 · Accepted Answer · edited Nov 29 '18 at 15:45

MYString::MYString(const MYString & str)

calls

allocation(str.m_lg);

which calls

delete m_ptr;

which - at this point - is still unassigned. Its value is indeterminate, trying to delete it causes undefined behavior. (Which is bad.)

(Another problem here is that you call delete for a pointer that - if allocated - is allocated with new. Also undefined behavior. If you allocate with new[], use delete[]!)

A quick fix, that arguably makes a good habit: initialize all POD values in the class declaration:

class MYString
{
  private:
    char * m_ptr = nullptr;
    int m_lg = 0;
    ...

However, while this would have prevented your problem at this point, you need to learn to anticipate such problems.

@MaxLanghof: I actually agree that a modern book / tutorial should move this to "Appendix B: Advanced Capabilities", but I'm also trying to be helpful :) — peterchen, Nov 29 '18 at 15:25

score 0 · Answer 2 · answered Nov 29 '18 at 15:40

Rather than using a void allocation(int), you can implement all your other constructors in terms of MYString (int, const char *), and allocate in the member initialiser. This ensures m_ptr is always a valid pointer.

Note that new[] is paired with delete[], not delete

class MYString
{
  private:
    int m_lg;  
    char * m_ptr; 
    MYString (int, const char *);

  public: 
    MYString ();
    MYString (const char *);
    ~MYString ();
    MYString (const String & );
};

MYString::MYString (int lg, const char * str)
 : m_lg(lg), m_ptr(new char[lg])
{
    strcpy ( m_ptr, str );
}

MYString::MYString()
 : MYString(1, "")
{}

MYString::MYString (const char * str)
 : MYString(strlen(str) + 1, str)
{}

MYString::MYString(const String & other) 
 : MYString(other.m_lg, other.m_ptr) 
{}

MYString::~MYString()
{
    delete [] m_ptr;
}

c++ copy constructon bugs when static declaration

2 Answers2