1

I have a class Foo defined like this:

class Foo
{
  public:
    Foo(int num);
    Foo(const Foo& other);
    ~Foo();

    Foo& operator=(const Foo& other);
    ...

  private:
    string *fooArray;
    void clearMemory();
    ...
}

Foo::Foo(int num)
{
    fooArray = new string[num];
}

Foo::Foo(const Foo& other)
{
    *this = other;
}

Foo::~Foo()
{
    clearMemory();
}

Foo& operator=(const Foo& other)
{
    clearMemory();
    fooArray = new string[other.size]; // size is a private variable
    memcpy(fooArray, other.fooArray, other.size * sizeof(string));
}

void Foo::someOtherFooFuncion(int newNum)
{
    clearMemory(); // crash with Visual Studio, does not crash with g++, but g++ also
                   // crashes when destructor is called
    fooArray = new string[newNum];
}

void Foo::clearMemory()
{
    if(fooArray != NULL)
    {
        delete[] fooArray; // sometimes it crashes here with g++; it always crashes when
                           // compiling in Visual Studio
        fooArray = NULL;
    }
}

As noted in the code comments, it is giving me crashes at times. I have tried following the steps in GDB, I got as far as

destructing Foo:
@0x7fff5fbff9b0
$26 = {
  fooArray = 0x1001009d8, 
  ...
}

And then delete[] fooArray is reached, and all of a sudden

Foo(49858) malloc: *** error for object 0x100100990: pointer being freed was not allocated

Have no idea where the 0x100100990 came from.

I realize the code is very incomplete, but I really don't know even where to start hunting for the bug right now, and would like some tips as to what possible conditions could cause delete[] errors.

EDIT:

Added c'tor, d'tor, and assignment operator. I am away from PC so code may not be 100% accurate. Assigning values to fooArray and accessing them works just fine though.

Also, I would greatly appreciate a general list of problems that could potentially cause delete[] to throw an error, so that I could at least have some idea of where to look in the code.

EDIT 2:

So I followed Xeo's advice to use std::uninitialized_copy, now the code works fine and compiles under g++. The destructor works fine in Visual Studio as well, but somehow deleting fooArray in someOtherFooFuncion makes it crash.

Any other ideas?

Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
wrongusername
  • 18,564
  • 40
  • 130
  • 214
  • Do you have a default constructor? If so, could you post it. – hmjd Jan 24 '12 at 17:35
  • Have you remembered the [Rule of Three](http://stackoverflow.com/questions/4172722)? And why aren't you using `std::vector`? – Mike Seymour Jan 24 '12 at 17:38
  • @MikeSeymour I do have a ctor and assignment operator and dtor, but can't use vector because it is a class assignment – wrongusername Jan 25 '12 at 00:25
  • This code is so wrong on so many levels.. really, scrap the class assignment and use std::vector, please... or atleast scrap memcpy.. :( – Xeo Jan 25 '12 at 01:08

3 Answers3

2

Make sure you define a copy constructor and an assignment operator. They will need to allocate memory for fooArray. If you don't define the copy ctor and operator=, the compiler will generate them for you. However, the compiler-generated ones will just copy the fooArray pointer, potentially resulting in double deletes.

If you are defining them already, please add the relevant code to your question.

edit: I can see that your copy constructor is not allocating memory, and your assignment operator is using memcpy(). Both can cause problems.

NPE
  • 486,780
  • 108
  • 951
  • 1,012
  • I have already defined the copy constructor and assignment operator, and they seem to work find, since I write to them in memory. I will add the code soon. – wrongusername Jan 25 '12 at 00:22
1

If you define a default constructor you must initialise fooArray to NULL, otherwise fooArray could be pointing to a random memory location which is then subject to a delete[].

hmjd
  • 120,187
  • 20
  • 207
  • 252
1

Do not use memcpy in C++

I can't stress that point enough. When memcpying a class object in C++ (non-POD to be exact), you're breaking that class' invariants defined by its constructors, since you are circumventing exactly those. Every time you memcpya a std::string class, you get a new object referring to the same memory as another object. You'll get a double delete with this, which is causing your crash.

Use std::uninitialized_copy from <algorithm> like this:

//                      src_begin             src_end           dest
std::uninitialized_copy(other.array, other.array + other.size, array);

(untested because I'm writing this from my iPod)

Or even better, just use std::vector instead of raw memory. You'll have no need for a destructor and copy ctor / assignment operator anymore.

Community
  • 1
  • 1
Xeo
  • 129,499
  • 52
  • 291
  • 397
  • Hey, thanks a lot for the answer! I'll try it out soon. Unfortunately, as much as I would like to use vectors, this is a class assignment that explicitly requires a dynamic array :( – wrongusername Jan 25 '12 at 01:38
  • It worked! In g++ at least. Visual Studio still crashes when I try to run my code though :( – wrongusername Jan 25 '12 at 03:06
  • @wrongusername: Without more code, it's going to be hard to find out what exactly goes wrong. Please create an [SSCCE](http://sscce.org), so we can investigate further. – Xeo Jan 25 '12 at 11:12