Have been looking for a simple case as I have on Stack overflow and other sites but still have not found anything, error happens on line 57, then also on line 47, valgrind saying I write a pointer which leads to an array of structures pixel, I tried to see, is there a problem with allocating memory, but it seems there is a problem with assignment bmp_file->data = file_data

==1811== error calling PR_SET_PTRACER, vgdb might block
==1811== Invalid write of size 8
==1811==    at 0x10BC02: read_bmp (bmp.c:65)
==1811==    by 0x1092D7: main (main.c:12)
==1811==  Address 0x4b97268 is 0 bytes after a block of size 8 alloc'd
==1811==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1811==    by 0x10BB88: read_bmp (bmp.c:52)
==1811==    by 0x1092D7: main (main.c:12)
==1811== 
==1811== Invalid read of size 8
==1811==    at 0x10BC16: read_bmp (bmp.c:67)
==1811==    by 0x1092D7: main (main.c:12)
==1811==  Address 0x4b97268 is 0 bytes after a block of size 8 alloc'd
==1811==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1811==    by 0x10BB88: read_bmp (bmp.c:52)
==1811==    by 0x1092D7: main (main.c:12)

struct bmp_image* read_bmp(FILE* stream) {
  struct bmp_image* bmp_file = (struct bmp_image*)calloc(1, sizeof(struct bmp_image*));
  struct bmp_header* header = read_bmp_header(stream);

  if (header->type != 0x4d42) {
    printf("Error: This is not a BMP file.\n");
    free(bmp_file);
    free(header);
    return NULL;
  }

  struct pixel* file_data = read_data(stream, header);

  bmp_file->header = header;
  bmp_file->data = file_data;

  if (bmp_file->header == NULL || bmp_file->data == NULL) {
    printf("Error: Corrupted BMP file.\n");

    if (bmp_file->header == NULL) {
      free(bmp_file->header);
    }

    if (bmp_file->data == NULL) {
      free(bmp_file->data);
    }

    free(bmp_file);
    return NULL;
  }

  return bmp_file;
}

struct pixel* read_data(FILE* stream, const struct bmp_header* header) {
  struct pixel* data = (struct pixel*)calloc(header->height * header->width, sizeof(struct pixel));

  if (!data) {
    free(data);
    return NULL;
  }

  fseek(stream, header->offset, SEEK_SET);
  unsigned short padding = header->width % 4;

  for (int pixelIdx = 0; pixelIdx < header->height * header->width; pixelIdx  ) {
    data[pixelIdx].blue = (uint8_t)fgetc(stream);
    data[pixelIdx].green = (uint8_t)fgetc(stream);
    data[pixelIdx].red = (uint8_t)fgetc(stream);

    if ((pixelIdx   1) % (int)header->width != 0 || padding == 0) continue;

    for (int i = 0; i < padding; i  ) {
      fgetc(stream);
    }
  }

  return data;
}

void free_bmp_image(struct bmp_image* image) {
  if (image == NULL) return;

  free(image->header);
  free(image->data);
  free(image);
}

CodePudding user response：

At least this memory allocation

struct bmp_image* bmp_file = (struct bmp_image *)calloc(1, sizeof(struct bmp_image *));

is incorrect. It seems you mean

struct bmp_image* bmp_file = (struct bmp_image *)calloc(1, sizeof(struct bmp_image));

or

struct bmp_image* bmp_file = (struct bmp_image *)calloc(1, sizeof( *bmp_file));

CodePudding user response：

Avoid allocation size mistakes:

1. Allocate to the size of the referenced object, not the type. It makes for cleaner correct code, easier to review and maintain.

2. Cast is unnecessary.

3. Check allocation success.

Example:

 // struct bmp_image* bmp_file = (struct bmp_image*)calloc(1, sizeof(struct bmp_image*));
 // wrong size >----------------------------------------------^-----------------------^

 struct bmp_image* bmp_file = calloc(1, sizeof bmp_file[0]);
 if (bmp_file == NULL) {
   fprintf(stderr, "Out-of-memory\n");
   return NULL; 
 }

... as well as other allocations in code.