Segfault Copy Constructor

Question

My code is as follows:

void Scene::copy(Scene const & source)    
{
maxnum=source.maxnum;
imagelist = new Image*[maxnum];

for(int i=0; i<maxnum; i++)
{
   if(source.imagelist[i] != NULL)
   {
    imagelist[i] = new Image;
    imagelist[i]->xcoord = source.imagelist[i]->xcoord;
    imagelist[i]->ycoord = source.imagelist[i]->ycoord;
    (*imagelist[i])=(*source.imagelist[i]);
   }

   else
   {
   imagelist[i] = NULL;
   }
}
}

A little background: The Scene class has a private int called maxnum and an dynamically allocated Array of Image pointers upon construction. These pointers point to images. The copy constructor attempts to make a deep copy of all of the images in the array. Somehow I'm getting a Segfault, but I don't see how I would be accessing an array out of bounds.

Anyone see something wrong?

I'm new to C++, so its probably something obvious.

Thanks,

Answer 1

I would suggest that maxnum (and maybe imagelist) become a private data member and implement const getMaxnum() and setMaxnum() methods. But I doubt that is the cause of any segfault the way you described this.

I would try removing that const before your reference and implement const public methods to extract data. It probably compiles since it is just a reference. Also, I would try switching to a pointer instead of pass by reference.

Alternatively, you can create a separate Scene class object and pass the Image type data as an array pointer. And I don't think you can declare Image *imagelist[value];.

void Scene::copy(Image *sourceimagelist, int sourcemaxnum) {  
maxnum=sourcemaxnum;
imagelist=new Image[maxnum];
//...
    imagelist[i].xcoord = sourceimagelist[i].xcoord;
    imagelist[i].ycoord = sourceimagelist[i].ycoord;
//...
}
//...
Scene a,b;  
//...
b.Copy(a.imagelist,a.maxnum);

Answer 2

If the source Image had maxnum set higher than the actual number of items in its imagelist, then the loop would run past the end of the source.imagelist array. Maybe maxnum is getting initialized to the value one while the array starts out empty (or maxnum might not be getting initalized at all), or maybe if you have a Scene::remove_image() function, it might have removed an imagelist entry without decrementing maxnum. I'd suggest using an std::vector rather than a raw array. The vector will keep track of its own size, so your for loop would be:

for(int i=0; i<source.imagelist.size(); i++)

and it would only access as many items as the source vector held. Another possible explanation for the crash is that one of your pointers in source.imagelist belongs to an Image that was deleted, but the pointer was never set to NULL and is now a dangling pointer.

delete source.imagelist[4];
...
...  //  If source.imagelist[4] wasn't set to NULL or removed from the array,
...  //  then we'll have trouble later.
...
for(int i=0; i<maxnum; i++)
{
    if (source.imagelist[i] != NULL)  //  This evaluates to true even when i == 4
    {
        //  When i == 4, we're reading the xcoord member from an Image
        //  object that no longer exists.
        imagelist[i]->xcoord = source.imagelist[i]->xcoord;

That last line will access memory that it shouldn't. Maybe the object still happens to exist in memory because it hasn't gotten overwritten yet, or maybe it has been overwritten and you'll retrieve an invalid xcoord value. If you're lucky, though, then your program will simply crash. If you're dealing directly with new and delete, make sure that you set a pointer to NULL after you delete it so that you don't have a dangling pointer. That doesn't prevent this problem if you're holding a copy of the pointer somewhere, though, in which case the second copy isn't going to get set to NULL when you delete-and-NULL the first copy. If you later try to access the second copy of the pointer, you'll have no way of knowing that it's no longer pointing to a valid object.

It's much safer to use a smart pointer class and let that deal with memory management for you. There's a smart pointer in the standard C++ library called std::auto_ptr, but it has strange semantics and can't be used in C++ containers, such as std::vector. If you have the Boost libraries installed, though, then I'd suggest replacing your raw pointers with a boost::shared_ptr.