CODEWITHSUNDEEP
cmemorysegmentation-faultcalloc
Andre Hofmeister
Andre Hofmeister

Reputation: 3416

Segmentation fault by writing in a 2D Array

I have a small memory access problem in my program and I do not find the error, maybe someone could help me.

I have created a new type to store rgb color values. That type looks like:

typedef struct pixel {
    unsigned char r;
    unsigned char g;
    unsigned char b;
} pixel;

In my main program I create with calloc a 2D dynamically array, to store the red color information’s.

pixel **pixelvalue = (pixel **) calloc(imginformation.width, sizeof(pixel));
for (i = 0; i < imginformation.width; i++) {
    pixelvalue[i] = (pixel *) calloc(imginformation.height, sizeof(pixel));
}

After that I call my function, which read the color values and who should safe them to the array. The function gets as parameter the array.

ReadFile(file, imginformation (Stuff like height and so one), pixelvalue (The calloc array));

In that function I try to write the values with 

pixelvalue[i][j].r = (unsigned char)fgetc(in);

Here I get the memory access error, what did I wrong?

Edit

Hi, first of all sorry about the missing language, I was a little bit tired yesterday :).

To understanding, I created an array of pixel and the elements are pointing to another array of pixel? Something like [Point to another 1D array pixel]?

With pixel **pixelvalue = calloc(imginformation.width, sizeof(pixel *)); I create imginformation.width numbers of pointers from type pixel and each pointer show to pixel, right?

It would be awesome if you could explain it a little bit more, if I’m wrong. I would really like to understand it.

@carl-norum What do you mean with:

"you shouldn't be casting the return values of calloc(). Doing so can hide bugs with #include that could come back to bite you down the road".

? I use the alloc space as parameter for a function, not as return value.

Thanks for your help!

Greetz

Upvotes: 3

Views: 830

Answers (4)

Carl Norum
Carl Norum

Reputation: 225052

You're not really making a 2D array, you're making an array of pointers that point to arrays of pixels. That means your first calloc call should allocate enough space for pointers, not for pixels:

pixel **pixelvalue = calloc(imginformation.width, sizeof(pixel *));

You didn't tag your question with a language, but assuming it's C (based on your typedef, which wouldn't be necessary in C++), you shouldn't be casting the return values of calloc(). Doing so can hide bugs with #include that could come back to bite you down the road.

Edit:

You asked a couple of follow-up questions. The first has been answered pretty well by several other answers, I think, but I'll try to summarize. The way you're doing the allocation, you are first going to allocate an array of pointers - each of those pointers is going to point to one row of your array. The rows themselves then need to be allocated - space for each pixel object goes there, and the pointers to the rows are stored in the first array of pointers.

Your second question, bout the return value from calloc() is pretty straightforward. If you cast the return value, you can hide implicit declaration bugs from yourself. Since the return type of calloc is void *, if you do something like:

my_ptr = calloc(1, 2);

Everything works nicely. Now imagine that you didn't include stdlib.h, and therefore had no prototype of calloc() in your translation unit. That would lead the compiler to assume the signature of calloc() to be int calloc(int, int), which isn't true. The same line of code above would throw you a warning about assuming a default signature for that function. Using a typecast like you have in your code will mask that warning and you might never know you were missing that #include line.

Upvotes: 3

Domingo Ignacio
Domingo Ignacio

Reputation: 1214

The other posters have correctly identified that you should be allocating your first block of memory in units of pixel* instead of in units of pixel.

But why does this problem cause segfault?

On a 32 bit machine, your pixel struct as defined above takes 3 bytes, but a pointer takes 32 bits (4 bytes).

That is,

  • sizeof(pixel) == 3
  • But sizeof(pixel*) == 4

So you're only allocating 75% of the memory you need. When accessing the bottom quarter of the image, you'll access memory you never allocated.

(On some 64 bit platforms, the problem definitely only gets worse. On some 16 bit platforms, you might be able to get away with this, although it'd still be sloppy)

Upvotes: 0

Ed Heal
Ed Heal

Reputation: 60017

Please see the diagram for an explainationenter image description here

So you first create the array of pixel * using calloc. Populate that array using calloc with pixel.

Upvotes: 1

Ed Heal
Ed Heal

Reputation: 60017

The code

pixel **pixelvalue = (pixel **) calloc(imginformation.width, sizeof(pixel));

pixelvalue is a pointer to a pointer to pixel - your typedef.

You need to write

pixel **pixelvalue = calloc(imginformation.width, sizeof(pixel *));

instead.

Upvotes: 0

Related Questions