Friday, April 8, 2011

Adding an int member to a C struct causes a segfault

I'm still learning C, and have started using it to generate images. I can't figure out why one of my programs is segfaulting. Here's the source code, cut down to 40 lines:

#include <stdio.h>
#include <stdlib.h>
struct color {
        unsigned char r, g, b;
};
struct image {
        int w, h/*, o*/;
        struct color **data;
};
int main() {
        // Declarations
        int x, y;
        struct color *black;
        struct image *img;
        // Set up color black
        black = (struct color *) malloc(sizeof(struct color *));
        black->r = 0;
        black->g = 0;
        black->b = 0;
        // Set up image img
        img = (struct image *) malloc(sizeof(struct image *));
        img->w = 1;
        img->h = 1;
        /*img->o = 0;*/
        img->data = (struct color **) malloc(img->h * sizeof(struct color *));
        for (y = 0; y < img->h; y++) {
                img->data[y] = (struct color *) malloc(img->w * sizeof(struct color));
        }
        // Fill in img with black
        for (x = 0; x < img->w; x++) {
                for (y = 0; y < img->h; y++) {
                        img->data[y][x].r = black->r;
                        img->data[y][x].g = black->g;
                        img->data[y][x].b = black->b;
                }
        }
        // Free black
        free(black);
        // Free img
        for (y = 0; y < img->h; y++)
                free(img->data[y]);
        free(img->data); // Segfaults
        free(img); // Also segfaults
        return 0;
}

It compiles and runs fine (using gcc on Ubuntu and on Vista with Cygwin), but uncommenting the two lines dealing with img->o breaks it. I have a feeling it's related to this previous question, but I'm malloc'ing everything that needs to be malloc'ed (I think). Any help would be appreciated.

From stackoverflow
  • There is a bug in your malloc statements. You are mallocing a pointer and not a struct. This gives you only 4 bytes of memory instead of the actual size required by your struct.

    black = malloc(sizeof(*black));
    

    When allocating memory for a pointer, you need to allocate memory for the thing being pointed at, not the type of the pointer. If you simply write sizeof(*black) as shown, you will always get the right type, even if the type of black changes.

    Tiago : this way is also valid and more clear, IMHO: black = malloc(sizeof(struct black));
  • At first glance, it seems that you're using an additional level of pointer indirection, and that's causing the segfault. When you malloc memory, it's a pointer to the object, not a pointer to a pointer to an object. So you'll have:

    img = (struct image *)malloc(sizeof(struct image))
    img->o = 0
    
    aib : You need not (and should not) cast the return value of malloc() in C.
  • Oops, the code was cut off; I forgot to escape a less-than sign. Here it is:

    #include <stdio.h>
    #include <stdlib.h>
    struct color {
        unsigned char r, g, b;
    };
    struct image {
        int w, h/*, o*/;
        struct color **data;
    };
    int main() {
        // Declarations
        int x, y;
        struct color *black;
        struct image *img;
        // Set up color black
        black = (struct color *) malloc(sizeof(struct color *));
        black->r = 0;
        black->g = 0;
        black->b = 0;
        // Set up image img
        img = (struct image *) malloc(sizeof(struct image *));
        img->w = 1;
        img->h = 1;
        /*img->o = 0;*/
        img->data = (struct color **) malloc(img->h * sizeof(struct color *));
        for (y = 0; y < img->h; y++) {
         img->data[y] = (struct color *) malloc(img->w * sizeof(struct color));
        }
        // Fill in img with black
        for (x = 0; x < img->w; x++) {
         for (y = 0; y < img->h; y++) {
          img->data[y][x].r = black->r;
          img->data[y][x].g = black->g;
          img->data[y][x].b = black->b;
         }
        }
        // Free black
        free(black);
        // Free img
        for (y = 0; y < img->h; y++)
         free(img->data[y]);
        free(img->data);
        free(img);
        // Return
        return 0;
    }
  • JaredPar has the right answer, but if you get a segfault, the first thing to do is run the program under valgrind. It is a huge help for dealing with problems like this.

    BTW, I've wasted days on that exact bug. Be happy you ran into it early in your C programming career and will always watch out for it in the future.

  • Wow, that was fast. Thanks a lot, it works now. valgrind? I'll try that.

0 comments:

Post a Comment