C - Writing a struct into a 2D array causes Segmentation Fault

Question

I'm trying to write a program that reads a text file into a 2D array of structs, but trying to put a struct into that array causes the program to crash.

Here's the program

ppm.c

#include "ppm.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

image parse_ascii_image(FILE *fp) {
  char magic[3];
  char comm[1024];
  char size[10];
  image img;
  int height;
  int width;

  ... // some code
  
  pixel **pixelarr;
  printf("Commencing internal malloc...\n");
  if (height <= 1024 && width <= 1024 && height > 0 && width > 0){
    pixelarr = (pixel **) malloc(height * sizeof(pixel*));
  }else{
    fprintf(stderr, "Error: Invalid image size: %d * %d", width, height);
    return img;
  }
  for (int i = 0; i < height; i++){
    pixelarr[i] = malloc(width * sizeof(pixel));
    
  }

  int d = 0;
  int e;
  printf("Filling in array:\n");

  for (int row = 0; row < height; row++){
    for (int col = 0; col < width; col++){
      for (int i = 0; i < 3; i++){
        while ((e = fgetc(fp)) != '\n'){
          d = d * 10;
          e = e - 60;
          d += e;
        }
        if (i == 0){
          pixelarr[row][col].red = d;
        }
        if (i == 1){
          pixelarr[row][col].green = d;
        }
        if (i == 2){
          pixelarr[row][col].blue = d;
        }
        d = 0;
      }      
    }
  }
  printf("Finished! Copying pixels over: \n");
  for (int row = 0; row < height; row++){
    for (int col = 0; col < width; col++){
      img.pixels[row][col] = pixelarr[row][col];
    // ^^^This is where the program crashes
    }
  }
  printf("Finished! Freeing internal malloc:\n");
  
  ... // some more code

}

relevant info from ppm.h:

#ifndef PPM_H
#define PPM_H 1

#include <stdio.h>

...

typedef struct pixel pixel;
struct pixel {
  int red;
  int green;
  int blue;
};

typedef struct image image;
struct image {
  enum ppm_magic magic; // PPM format
  char comments[1024];  // All comments truncated to 1023 characters
  int width;            // image width
  int height;           // image height
  int max_color;        // maximum color value
  pixel **pixels;       // 2D array of pixel structs.
};

...

// Parses an ASCII PPM file.
image parse_ascii_image(FILE *fp);

...

#endif

If anyone can help me figure out what's causing my program to crash there, I would appreciate it. Thank you!

`img.pixels` is never initialized. Everything *seems* to indicate you should simply replace that final nested loops with `img.pixels = pixelarr;` and do NOT free `pixelarr` afterward. That's going to whomever is taking over the return image. You should also properly initialize *all* the members of `img`. — WhozCraig, Dec 08 '20 at 06:04
regarding: `enum ppm_magic magic; // PPM format` this is declaring an instance of a specific `enum` type. But the enum is never defined before being used — user3629249, Dec 08 '20 at 22:02
regarding: `struct pixel { int red; int green; int blue; };` The 'colors' in a pixel are 8 bits each, not an `int` (4 or 8 bytes) — user3629249, Dec 08 '20 at 22:10
OT: regarding; `pixelarr = (pixel **) malloc(height * sizeof(pixel*));` and `pixelarr[i] = malloc(width * sizeof(pixel));` 1) the contents of `pixel` is not correctly defined. (and your assuming that a pixel is 24 bits) 2) the returned type is `void*` which can be assigned to any pointer. Casting just clutters the code and is error prone. 3) always check (!=NULL) the returned value to assure the operation was successful. If not successful (==NULL) then inform the user via: `perror( "malloc failed" );` which will output to `stderr` both your error message and the text system error. — user3629249, Dec 08 '20 at 22:17
regarding: `pixelarr[i] = malloc(width * sizeof(pixel));` a image row has to be a multiple of 4 (regardless of the visible image width) so this statement might not be long enough to hold the whole row. because of the above, this: `for (int row = 0; row < height; row++){ for (int col = 0; col < width; col++){` has a very good chance of failing to access all the correct pixels — user3629249, Dec 08 '20 at 22:21

score 1 · Answer 1 · answered Dec 08 '20 at 06:40

A few problems:
1: img.pixels is never initialized. Anytime you create a pointer to something without assigning it a value first, try to set it to NULL so it's easier to debug. Note that this will not fix the issue. To fix it, simply initialize them with a struct instance.
2: You should free pixelarr. Most modern OS's do this, but it's still really, really good practice.
3: Wouldn't it be easier to not use the nested loops, and instead do img.pixels = pixelarr? It achieves the same (unless this is for a class, and this part is required).

C - Writing a struct into a 2D array causes Segmentation Fault

1 Answers1