Code compiles without errors or warnings, but execution gives segmentation fault error in some cases

[klebermo]

The code below is a short version of the project I am implement right now, which is a executable (https://github.com/klebermo/game) and a library (https://github.com/klebermo/codec).

In the version I put here , everything was added in a single *.cpp file (it has 2 version of the main function, the first one runs without problem here, the other is the one that gives the segmentation fault). the first 2 classes belong in the library in the original project, and the other ones in the executable.

Here in my computer, when I run the version that have only the std::cout instruction, the program is executed without issues in most executions (but not in all the executions). when I try run the the other version, all the executions end with the error.

When I run the program through the debugger, the error is always pointed in th same line, in the method toArray in the class Netpbm (it is the line marked with a commentary).

To run the code, it is necessary create (or download from any place) a PBM file.

With the code presented here, anyone can give me any hint of how to improve the code to avoid this segmentation fault error?

#include <string>
#include <vector>
#include <fstream>
#include <sstream>
#include <iostream>
#include <bitset>
#include <cstring>
#include <cmath>

struct Pixel {
    float r, g, b;
};
typedef struct Pixel pixel;

class Netpbm {
protected:
  std::vector<std::vector<pixel>> pixels;
public:
  virtual void read_file(std::string file_name) = 0;
  virtual void write_ascii_file(std::string file_name) = 0;
  virtual void write_binary_file(std::string file_name) = 0;

  int getHeight();
  int getWidth();
  std::vector<float> toArray();
};

int Netpbm::getHeight() { return pixels.size(); };

int Netpbm::getWidth() { return pixels[0].size(); };

std::vector<float> Netpbm::toArray() {
  std::vector<float> result;
  size_t h = pixels.size(), w = pixels[0].size();

  for(size_t i=0; i<h; i++) {
    for(size_t j=0; j<w; j++) {
      float x = j/w, y=i/h;
      result.push_back(-1 + (2 * x));
      result.push_back(1 - (2 * y));
      result.push_back(pixels[i][j].r); //error
      result.push_back(pixels[i][j].g);
      result.push_back(pixels[i][j].b);
    }
  }

  return result;
}

class Bitmap : public Netpbm {
public:
  void read_file(std::string file_name);
  void write_ascii_file(std::string file_name);
  void write_binary_file(std::string file_name);
};

void Bitmap::read_file(std::string file_name) {
  std::ifstream file(file_name);
  std::string line_one, line_two, line_pixels;
  char magicNumber;
  std::string width, height;

  while(getline(file, line_one)) {
    if(line_one.size() > 0 && line_one.at(0) != '#') {
      magicNumber = line_one.at(1);
      break;
    }
  }

  while(getline(file, line_two)) {
    if(line_two.size() > 0 && line_two.at(0) != '#') {
      std::stringstream ss(line_two);
      getline(ss, width, ' ');
      getline(ss, height, ' ');
      break;
    }
  }

  if(magicNumber == '1') {
    std::vector<pixel> v;

    while(getline(file, line_pixels)) {
      if(line_pixels.size() > 0 && line_pixels.at(0) != '#') {
        std::stringstream ss(line_pixels);
        std::string value;
        while(getline(ss, value, ' ')) {
          pixel p;
          p.r = p.g = p.b = stoi(value);
          v.push_back(p);
        }
      }
    }

    int counter = 0;
    for(int i=0; i<stoi(height); i++) {
      std::vector<pixel> row;
      for(int j=0; j<stoi(width); j++) {
        row.push_back(v[counter++]);
      }
      pixels.push_back(row);
    }
  }

  if(magicNumber == '4') {
    std::vector<pixel> v;

    while(!file.eof()) {
      unsigned char data[1];
      file.read((char*)data, 1);
      for(int i=0; i<8; i++) {
        pixel p;
        if(data[0]&(1<<i))
          p.r = p.g = p.b = 1;
        else
          p.r = p.g = p.b = 0;
        v.push_back(p);
      }
    }

    int counter = 0;
    for(int i=0; i<stoi(height); i++) {
      std::vector<pixel> row;
      for(int j=0; j<stoi(width); j++) {
        row.push_back(v[counter++]);
      }
      pixels.push_back(row);
    }
  }
}

void Bitmap::write_ascii_file(std::string file_name) {
  //
}

void Bitmap::write_binary_file(std::string file_name) {
  //
}

class Image {
private:
    float * vertexList;
    int width;
    int height;
public:
    Image(float * v, int w, int h) {
        vertexList = v;
        width = w;
        height = h;
    };

    void draw() {
        std::cout << "height: " << height << std::endl;
        std::cout << "width: " << width << std::endl;
        std::cout << "size: " << sizeof(vertexList) / sizeof(float) << std::endl;
    };
};

class Renderer {
private:
    Image * image;
public:
    void setImage(float * vertices, int width, int height) {
        image = new Image(vertices, width, height);
    };

    void drawFrame() {
        image->draw();
    };
};

class Surface {
private:
    int width;
    int height;
    Renderer * renderer;
public:
    Surface(std::string windows_title, int width, int height) {
        this->width = width;
        this->height = height;
        this->renderer = new Renderer();
    };

    ~Surface() {
        delete renderer;
    };

    Renderer * getRenderer() {
        return renderer;
    };

    void loop() {
        renderer->drawFrame();
    };
};

/*
int main() {
    std::cout << "height: " << image.getHeight() << std::endl;
    std::cout << "width: " << image.getWidth() << std::endl;
    std::cout << "size: " << image.toArray().size() << std::endl;
    return 0;
}
*/

int main() {
    Bitmap image;
    image.read_file("teste.pbm");
    Surface * view = new Surface("image", image.getWidth(), image.getHeight());
    view->getRenderer()->setImage(image.toArray().data(), image.getWidth(), image.getHeight());
    view->loop();
    delete view;
    return 0;
}

[salem c]

> When I run the program through the debugger, the error is always pointed in th same line
So start putting breakpoints at earlier points in the code, and verify your data is really what you expect.


You're assuming each row of pixels is the same size. If your file reading is broken (see later), this might not be as true as you imagine.

  size_t h = pixels.size(), w = pixels[0].size();

  for(size_t i=0; i<h; i++) {
    size_t row_w = pixels[i].size();
    if ( row_w != w ) {
        std::cerr << "oops"; // put a breakpoint here.
    }
    for(size_t j=0; j<w; j++) {
      float x = j/w, y=i/h;
      result.push_back(-1 + (2 * x));
      result.push_back(1 - (2 * y));
      result.push_back(pixels[i][j].r); //error 

Like is it always on the 2nd row, 3rd row, last row?
These are all useful clues to where to look next.

  if(magicNumber == '1') {
    std::vector<pixel> v;

    while(getline(file, line_pixels)) {
      if(line_pixels.size() > 0 && line_pixels.at(0) != '#') {
        std::stringstream ss(line_pixels);
        std::string value;
        while(getline(ss, value, ' ')) {

The first .pbm file I created in GIMP was not a set of space separated pixels.

    while(!file.eof()) {
      unsigned char data[1];
      file.read((char*)data, 1);
      for(int i=0; i<8; i++) {
        pixel p;
        if(data[0]&(1<<i))
          p.r = p.g = p.b = 1;
        else
          p.r = p.g = p.b = 0;
        v.push_back(p);
      }
    }

Using eof() is wrong. You go round the loop one more time than you expect, resulting in the very last file.read leaving junk in your char data[1]
eof() only becomes true when a previous read() has failed.

Or put another way, 10 reads to a file with 10 bytes does not set eof() to true. You have to attempt to read byte 11 to make eof() true.


It should be

    unsigned char data[1];
    while(file.read((char*)data, 1)) {
        // do your thing
    }

[klebermo]

The first .pbm file I created in GIMP was not a set of space separated pixels.

I am following the description of the format for ascii data, as showed here: https://en.wikipedia.org/wiki/Netpbm. Do you choose to save the file as ascii or binary?

You're assuming each row of pixels is the same size. If your file reading is broken (see later), this might not be as true as you imagine.

yes, because when I am reading the file in 2 steps: first, I put all data in a 1d array, and after that I store this data in the 2d array:

  if(magicNumber == '1') {
    std::vector<pixel> v;

    while(getline(file, line_pixels)) {
      if(line_pixels.size() > 0 && line_pixels.at(0) != '#') {
        std::stringstream ss(line_pixels);
        std::string value;
        while(getline(ss, value, ' ')) {
          pixel p;
          p.r = p.g = p.b = stoi(value);
          v.push_back(p);
        }
      }
    }

    int counter = 0;
    for(int i=0; i<stoi(height); i++) {
      std::vector<pixel> row;
      for(int j=0; j<stoi(width); j++) {
        row.push_back(v[counter++]);
      }
      pixels.push_back(row);
    }
  }

I believe this approach could work even the file is really broken (lines with different sizes).

[seeplus]

Why do you persist with your 'broken' Netpbm and Bitmap classes when 'corrected' ones have been provided in previous posts?

The Renderer and Surface classes also have several issues.

std::cout << "size: " << sizeof(vertexList) / sizeof(float) << std::endl;

NO!! sizeof(vertexList) gives the size of the pointer, NOT the number of elements! Given a pointer, there's no way of knowing to how many elements it points.

view.getRenderer()->setImage(image.toArray().data(), image.getWidth(), image.getHeight());

NO!! the vaklue returned by .toarray() is a temp. It ceases to exist after this line has been executed. Hence the value returned by .data() is invalid afyter this line!

Try (NOT tried):

auto toarr { image.toArray() };

view.getRenderer()->setImage(toarr.data(), image.getWidth(), image.getHeight());

[seeplus]

Using my previous Bitmap and Netpbm classes, then as a first refactor consider:

class Image {
private:
	const float* vertexList {};
	size_t width {};
	size_t height {};

public:
	Image(const float* v, size_t w, size_t h) : vertexList(v), width(w), height(h) {}

	void draw() const {
		std::cout << "height: " << height << '\n';
		std::cout << "width: " << width << '\n';
		//std::cout << "size: " << sizeof(vertexList) / sizeof(float) << std::endl;
	};
};

class Renderer {
private:
	Image* image {};

public:
	Renderer() {}

	void setImage(const float* vertices, size_t width, size_t height) {
		image = new Image(vertices, width, height);
	};

	~Renderer() {
		delete image;
	}

	// Provide code if these needed
	Renderer(const Renderer&) = delete;
	Renderer& operator=(const Renderer&) = delete;

	void drawFrame() const {
		image->draw();
	};
};

class Surface {
private:
	size_t width {};
	size_t height {};
	Renderer* renderer {};

public:
	Surface(const std::string&, size_t w, size_t h) : width(w), height(h), renderer(new Renderer) {}

	~Surface() {
		delete renderer;
	};

	// Provide code if these needed
	Surface(const Surface&) = delete;
	Surface& operator=(const Surface&) = delete;

	Renderer* getRenderer() const {
		return renderer;
	};

	void loop() const {
		renderer->drawFrame();
	};
};

int main() {
	Bitmap image;

	if (!image.read_file("test.bm"))
		return (std::cout << "Cannot open/read file\n"), 1;

	std::cout << "width: " << image.getWidth() << '\n';
	std::cout << "height: " << image.getHeight() << '\n';

	const auto toarr { image.toArray() };

	std::cout << "size of toArray: " << toarr.size() << '\n';

	Surface view("image", image.getWidth(), image.getHeight());

	view.getRenderer()->setImage(toarr.data(), image.getWidth(), image.getHeight());
	view.loop();

	/*
	for (size_t i {}; i < toarr.size(); i += 5) {
		for (size_t k {}; k < 5; ++k)
			std::cout << toarr[i + k] << ' ';

		std::cout << '\n';
	}*/
}

which for the file contents provided previously displays:

width: 32
height: 32
size of toArray: 5120
height: 32
width: 32

https://cplusplus.com/forum/beginner/285127/

[seeplus]

For a second refactor, perhaps something like:

class Image {
private:
	const std::vector<float>& vertexList {};
	size_t width {};
	size_t height {};

public:
	Image(const std::vector<float>& v, size_t w, size_t h) : vertexList(v), width(w), height(h) {}

	void draw() const {
		std::cout << "height: " << height << '\n';
		std::cout << "width: " << width << '\n';
		std::cout << "size: " <<  vertexList.size() << '\n';
	};
};

class Renderer {
private:
	Image* image {};

public:
	Renderer() {}

	void setImage(const std::vector<float>& vertices, size_t width, size_t height) {
		image = new Image(vertices, width, height);
	};

	~Renderer() {
		delete image;
	}

	// Provide code if these needed
	Renderer(const Renderer&) = delete;
	Renderer& operator=(const Renderer&) = delete;

	void drawFrame() const {
		image->draw();
	};
};

class Surface {
private:
	size_t width {};
	size_t height {};
	Renderer* renderer {};

public:
	Surface(const std::string&, size_t w, size_t h) : width(w), height(h), renderer(new Renderer) {}

	~Surface() {
		delete renderer;
	};

	// Provide code if these needed
	Surface(const Surface&) = delete;
	Surface& operator=(const Surface&) = delete;

	Renderer* getRenderer() const {
		return renderer;
	};

	void loop() const {
		renderer->drawFrame();
	};
};

int main() {
	Bitmap image;

	if (!image.read_file("test.bm"))
		return (std::cout << "Cannot open/read file\n"), 1;

	std::cout << "width: " << image.getWidth() << '\n';
	std::cout << "height: " << image.getHeight() << '\n';

	const auto toarr { image.toArray() };

	std::cout << "size of toArray: " << toarr.size() << '\n';

	Surface view("image", image.getWidth(), image.getHeight());

	view.getRenderer()->setImage(toarr, image.getWidth(), image.getHeight());
	view.loop();

	/*
	for (size_t i {}; i < toarr.size(); i += 5) {
		for (size_t k {}; k < 5; ++k)
			std::cout << toarr[i + k] << ' ';

		std::cout << '\n';
	}*/
}

which displays:

width: 32
height: 32
size of toArray: 5120
height: 32
width: 32
size: 5120

[kbw]

In the original code, the class is missing a constructor that sets up the 2d array.

[klebermo]

Using my previous Bitmap and Netpbm classes, then as a first refactor consider:

With this suggested code, I still got the same error, but now with the instruction to resize the vector.

[coder777]

This:

int Netpbm::getWidth() { return pixels[0].size(); };

is dangerous. You must not call it until pixels has at least 1 element. Try this:

int Netpbm::getWidth() { return pixels.empty() ? 0 : pixels[0].size(); };

[mbozzi]

vector::resize is only likely to segfault if the vector object is clobbered because of a buffer overrun, outside of its lifetime, or there's a similar issue. The code posted by @seeplus looks fine, there's nothing in there to cause this. So the problem is probably elsewhere.

There is (very likely) an invalid vector object. The question is, why is it invalid? You could start troubleshooting by figuring out, with your debugger's help, where the invalid pointer originated. Alternately, you can remove stuff from your program to narrow down where the problem is. A tiny, complete, compilable example of the problem is a powerful troubleshooting tool.

[seeplus]

With this suggested code, I still got the same error, but now with the instruction to resize the vector.

With my and only my code - or have you 'amended' or added to it? My code as supplied above works fine with the provided file under Windows with VS2022. If you're tried that code exactly as provided and it still generates an error, then I'd suggest there's something wrong with your installation. If mine works as provided, but yours doesn't then it's something you're done with the code. In this case, post a full compilable minimal code that demonstrates the issue. As the issue is with segfault, misuse/mismanagement of dynamic memory is a strong candidate.