CODEWITHSUNDEEP
c++classcopy-constructor
Max
Max

Reputation: 23

Seg fault in copy constructor

I'm working on an assignment in which we are manipulating *.ppm images using a "Image" class. It compiles and then seg faults when the copy constructor is called. Here it is:

Image::Image(const Image & imageToCopy) {
fileType = imageToCopy.fileType;
width = imageToCopy.width;
height = imageToCopy.height;
maxColor = imageToCopy.maxColor;

image = new int *[height];

for(int i=0; i < height; i++) {
    image[i] = new int [width];
    for(int j=0; j < width; j++) {
    image[i][j] = imageToCopy.image[i][j];
    }
}
}

Called like so:

Image image2(image1);

I'm at somewhat of a loss for why this happens. I don't know what is wrong because the code is almost identical to my constructor, which works fine. the only difference is that I have

image[i][j] = imageToCopy.image[i][j];

instead of

imageInputStream >> image[i][j];

Thoughts? Thanks

EDIT: Constructor is below:

Image::Image(const char* filename) {
    ifstream imageInputStream;
    imageInputStream.open(filename);

    imageInputStream >> fileType;
    imageInputStream >> width;
    imageInputStream >> height;
    imageInputStream >> maxColor;

    image = new int *[height];

    for(int i=0; i < height; i++) {
        image[i] = new int [width];
        for(int j=0; j < width; j++) {
            imageInputStream >> image[i][j];
        }
    }

    imageInputStream.close();
}

Upvotes: 2

Views: 5658

Answers (2)

abarnert
abarnert

Reputation: 365845

I took the code you pasted at pastebin and added some logging to the two constructors.

In Image(const char *), right after imageInputStream >> maxColor;:

    cout << "Image(" << filename << "): "
         << fileType << ", "
         << width << ", "
         << height << ", "
         << maxColor << endl;

In Image(const Image&), right after maxColor = imageToCopy.maxColor;:

    cout << "Image(Image): "
         << fileType << ", "
         << width << ", "
         << height << ", "
         << maxColor << endl;

And here's the output:

Testing standard constructor, opening tux.ppm
Image(tux.ppm): P3, 0, 0, 0
Calling flip method
Calling dither method
Saving the flipped and dithered image
Creating a 2nd image object, using tux.ppm again with the standard constructor
Image(tux.ppm): P3, 32767, 1656563112, 17

In other words, you're reading garbage. The first time through, I happen to read 0x0 and return an empty image. Then I read 32767x1656563112 and create a huge garbage image, which takes so long that I don't want to wait around, but I could imagine that copying it would segfault.

And to verify that this is the case, look at what gets output as tux_transformed.ppm:

P3
0 0
0

Of course I don't have your tux.ppm file, but I threw a bunch of sample PPMs at it from different sources, and they all had the same problem.

As a wild guess, your problem is that you're not handling PPM comments, so you're trying to read one as, e.g., a width. But it really doesn't matter. If your code can create garbage objects, it can crash trying to copy them.

Upvotes: 0

abarnert
abarnert

Reputation: 365845

Without seeing the complete code, this is just a guess, but if you've created a copy constructor and a destructor but no copy assignment operator, you may get a segfault exactly like this if you try to use assignment.

And you may not think you're doing an assignment anywhere, but unless you know all the rules of C++ (which even experts don't, much less new students), it's hard to be sure. The easiest way to find out is to declare a private assignment operator and don't define it (or, if you're using C++11, declare it deleted) and see if you get a compile error.

For example:

struct Image {
  int width_, height_;
  int **image_;
  Image(int width, int height) : width_(width), height_(height),
                                 image_(0) {
    image_ = new int *[height_];
    for (int i = 0; i != height_; ++i) {
      image_[i] = new int[width_];
      for (int j = 0; j != width_; ++j) {
        image_[i][j] = 1;
      }
    }
  }

  Image(const Image& rhs) : width_(rhs.width_), height_(rhs.height_),
                            image_(0) {
    image_ = new int*[height_];
    for (int i = 0; i != height_; ++i) {
      image_[i] = new int[width_];
      for (int j = 0; j != width_; ++j) {
        image_[i][j] = rhs.image_[i][j];
      }
    }
  }

  /* uncomment to uncrash
  Image& operator=(const Image& rhs) {
    if (this == &rhs) return *this;
    for (int i = 0; i != height_; ++i) {
      delete [] image_[i];
    }
    delete [] image_;
    image_ = new int*[height_];
    for (int i = 0; i != height_; ++i) {
      image_[i] = new int[width_];
      for (int j = 0; j != width_; ++j) {
        image_[i][j] = rhs.image_[i][j];
      }
    }
    return *this;
  }
  */

  ~Image() {
    for (int i = 0; i != height_; ++i) {
      delete [] image_[i];
    }
    delete [] image_;
  }
};

int main(int, char*[]) {
  Image img(200, 300);
  Image img2(img);
  Image img3(100, 200);
  img3 = img2;
  return 0;
}

As the comment implies, if you uncomment the copy assignment operator, the whole thing works.

If this is your problem, read up on the Rule of Three (Wikipedia, Ward's Wiki).

Upvotes: 3

Related Questions