dynamic allocation of a two dimensional array ends with segmentaion fault

[subclass]

I'm attempting to solve this for myself as this is homework but I can't get past the hurdle. I have an array that dynamically holds the pixels of an image (chars) in its second dimension (first being the image number).

I first initialize my array variable...

unsigned char** imgSet;

I try to allocate the room when you call the load image function to hold room for just one image, I was thinking it was something like imgSet[0][] as in now I have one index pointing to the second...

imgSet = malloc((numImages+1)*(sizeof(unsigned char*)));
loadImg(imgSet,&numImages);
printf("File has %d rows and %d columns\n",imgSet[numImages][0]+1,imgSet[numImages][1]+1);

My loadImg function calls a function from a piece of code my teacher gave us (it actually converts bmp files into text:

void loadImg(unsigned char** images, int *n) {
	char fileName[80];
    printf("Enter filename: ");
    scanf("%s",fileName);
    readImage(fileName,images);
	printf("File %s has %d rows and %d columns\n",fileName,images[*n][0]+1,images[*n][1]+1);
    (*n)++;
}

This is he's function, or part with the malloc:

void readImage(const char *fname, unsigned char** img) {
//a lot of code taken out that just makes sure that the file is right
*img = (unsigned char *)malloc(sizeof(unsigned char)*(2+nRows*nCols)); /* allocate space for image */
}

The red is the part that gives me the segmentation fault. I do the same thing in my loadImg function after I call my teachers function and it works, but once I leave the function it fails. What's wrong with this?

[subclass]

I've now started to randomally attempt to solve this by doing an combination of * and & I can think of to get it to work but sadly, in the 5 hours since I've asked for help I still cannot solve it.

I still believe my allocation of memory is wrong but I have no idea why.

[Adak]

Welcome to the forum, Subclass! ;)

/* dynamically creats a 2D array of pointers, in C */

#include <stdio.h>
#include <stdlib.h>

void allocate2D(double **dat, int nrows, int ncols);

int main() {
  int i,j, rows, cols; 
  double **dat;
  
  printf("\n\n\nEnter the number of rows and columns you want: \n");
  scanf("%d %d", &rows, &cols);
  getchar();  //remove the newline from the kboard buffer
  
  
  allocate2D(dat, rows, cols);
  
  for(i=0;i<rows;i++) {
    for(j=0;j<cols;j++) {
      dat[i][j]= i+j;
    }
  }

  for(i=0;i<rows;i++) {
    printf("\n%.3lf %.3lf %.3lf", dat[i][0],dat[i][1],dat[i][2]);
  }

  //free the memory correctly
  for(i=0;i<rows;i++)
    free(dat[i]);
  free(dat);

  return 0;
}
void allocate2D(double **dat, int nrows, int ncols) {
     int i;
     /*  allocate array of pointers  */
     dat = malloc( nrows*sizeof( double* ) );
     
     /*  allocate each row  */
     
     for(i = 0; i < nrows; i++) {
          dat[i] = malloc( ncols*sizeof( double ) );
     }
    if(dat==NULL || dat[i-1]==NULL) {
       printf("\nError allocating memory\n");
       exit(1);
    }
   
}

[Adak]

Many apologies, but that last program was NOT the right version:

Try this one: (and delete that other one):

/* dynamically creates a 2D array of pointers, in C */

#include <stdio.h>
#include <stdlib.h>

double** allocate2D(int nrows, int ncols);

int main() {
  int i,j, rows, cols; 
  double **dat;

  printf("\n\n\n How many rows do you want?\n ");
  scanf("%d", &rows);
  (void) getchar();
  printf(" How many columns do you want?\n ");
  scanf("%d", &cols);
  (void) getchar();

  dat = allocate2D(rows, cols);
  for(i=0;i<rows;i++) {
    for(j=0;j<cols;j++) {
      dat[i][j] = i+j;
    }
  }

  for(i=0;i<rows;i++) {
      for(j=0;j<cols;j++) 
        printf("\n%.3lf", dat[i][j]);
  }
   
  for(i=0;i<rows;i++)
    free(dat[i]);

  free(dat);
  printf("\n\t\t\t    press enter when ready");
  (void) getchar();
  return 0;
}
double** allocate2D(int nrows, int ncols) {
     int i;
     double **dat2;
     /*  allocate array of pointers  */
     dat2 = malloc( nrows*sizeof(double*));
     
     /*  allocate each row  */
     
     for(i = 0; i < nrows; i++) {
          dat2[i] = malloc( ncols*sizeof(double));
     }
    if(dat2==NULL || dat2[i-1]==NULL) {
       printf("\nError allocating memory\n");
       exit(1);
    }
  return dat2;
}

The problem with my earlier post, is that it didn't return the address that malloc was assigning, to dat. :(

Since I can't edit that bad post, that will just be a lesson to anyone in a hurry, who doesn't read through the posts. ;)

[subclass]

I still do understand, you're doing the same thing when allocating the rows as I am, aren't you?

You line is the same as mine:

dat2 = malloc( nrows*sizeof(double*));
imgSet = malloc((numImages+1)*(sizeof(unsigned char*)));

Yet I am still get that segmentation error.

Thanks for the welcome :)

[Adak]

Entirely welcome.

I can't tell by little snippets of code that you've posted, but no, you're not doing what I did in the second version of the program, in this thread.

You MIGHT be doing what I did in the first version, and not returning the address that is malloc'd, but i can't tell stink from these little bits of code you've posted.

Post the code up, if you want help. I can see very little, looking through a straw. ;)

[subclass]

So this is my own code, I took out everything that wasn't necessary (function declarations and most of the switch for size)

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <math.h>

void menu();
void loadImg(unsigned char** images, int *n);

int main() {
	int numImages = 0;
	int curImage = 0, loop = 1, bool = 0, imgIndex=0;
	unsigned char **imgSet;
	
	while(loop != -1) {
		char choice;
		menu();
		scanf("%s",&choice);
		switch(choice) {
			case 'L':
			case 'l':
				imgSet = (unsigned char**)malloc((numImages+1)*(sizeof(unsigned char*)));
				loadImg(imgSet,&numImages);
				printf("File has %d rows and %d columns\n",imgSet[numImages][0]+1,imgSet[numImages][1]+1);
				break;
			case 'Q':
			case 'q':
				printf("Program terminated\n");
				loop = -1;
				break;
		}
	}
	
	return 0;
}

void menu() {
	printf("*****OPTION*****MENU*****\n");
	printf("L: Load image\n");
	printf("R: Remove image\n");
	printf("P: Print image\n");
	printf("A: Average image\n");
	printf("H: Horizontal derivative\n");
	printf("V: Vertical derivative\n");
	printf("T: compute hisTogram\n");
	printf("D: search Digram\n");
	printf("M: local Maxima\n");
	printf("Q: Quit program\n");
	printf("*************************\n");
	printf("Select an option: ");
}

void loadImg(unsigned char** images, int *n) {
	char fileName[80];
    printf("Enter filename: ");
    scanf("%s",fileName);
    readImage(&fileName,&images[*n]);
	printf("File %s has %d rows and %d columns\n",fileName,images[*n][0]+1,images[*n][1]+1);
    (*n)++;
}

This is the readImage function that is inside my teachers own c file that I have to call, everything is taken out except this line (the code before this is error checking to make sure the file is correct)

void readImage(const char *fname, unsigned char** img) {
//a lot of code taken out that just makes sure that the file is right

*img = (unsigned char *)malloc(sizeof(unsigned char)*(2+nRows*nCols)); /* allocate space for image */
    *(*img) = (nRows-1);
    *(*img+1) = (nCols-1);
    pad = 4*ceil((float)nCols*3/4) - nCols*3;

    for (row = 0; row < h.numRows; row++) {/* read pixel values */

        pos = row*nCols + 2;

    	for (col = 0; col < h.numCols; col++) {

	    b = getc(inf);  g = getc(inf);  r = getc(inf);/* read RGB */

	    total = (int)b + (int)g + (int)r;

	    gray = floor((float)total/3 + 0.5);

	    *(*img+pos++) = gray;

	}

        for (p=0; p<pad; p++) x = getc(inf);

    }
}

[Adak]

His code doesn't seem to be relevant to the problem at hand.

1 Comment is inside the code

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <math.h>

void menu();
void loadImg(unsigned char** images, int *n);

int main() {
	int numImages = 0;
	int curImage = 0, loop = 1, bool = 0, imgIndex=0;
	unsigned char **imgSet;
	
	while(loop != -1) {
		char choice;
		menu();
		scanf("%s",&choice);
		switch(choice) {
			case 'L':
			case 'l':
				imgSet = (unsigned char**)malloc((numImages+1)*(sizeof(unsigned char*)));

/* 888888888888888888888888888888*/
/*you are mallocing 1 * sizeof(unsigned char*), since numImage is 0 */
/* that looks OK, if unusual. */
/* 888888888888888888888888888888*/

				loadImg(imgSet,&numImages);
				printf("File has %d rows and %d columns\n",imgSet[numImages][0]+1,imgSet[numImages][1]+1);
				break;
			case 'Q':
			case 'q':
				printf("Program terminated\n");
				loop = -1;
				break;
		}
	}
	
	return 0;
}

void menu() {
	printf("*****OPTION*****MENU*****\n");
	printf("L: Load image\n");
	printf("R: Remove image\n");
	printf("P: Print image\n");
	printf("A: Average image\n");
	printf("H: Horizontal derivative\n");
	printf("V: Vertical derivative\n");
	printf("T: compute hisTogram\n");
	printf("D: search Digram\n");
	printf("M: local Maxima\n");
	printf("Q: Quit program\n");
	printf("*************************\n");
	printf("Select an option: ");
}

void loadImg(unsigned char** images, int *n) {
	char fileName[80];
    printf("Enter filename: ");
    scanf("%s",fileName);
    readImage(&fileName,&images[*n]);
	printf("File %s has %d rows and %d columns\n",fileName,images[*n][0]+1,images[*n][1]+1);
    (*n)++;
}

Big problem is, I don't see where you malloc the row (the second dimension) pointers.

Look at my allocate2D() function again, there needs to be two malloc's - one for the valid array pointer, and one (loop) for mallocing each of the row pointers.

At the end of your program, you should also free them, and free them in the order I show in main() of my program: first free the row pointers, and then free the array pointer.

Also, it's very good to test them. malloc() returns NULL when it fails, to your pointer, so it's very easy to test - you don't want your program to continue if the malloc should fail - and they can and will fail, when you least expect it.

[subclass]

Yeah the numImages is odd but it increases each time, so at first it would be 1->2, so when I press 'l', and I'm loading my 6th image, numImages should be 5, so malloc should now make room for another one, but doesn't the professors code deal with the second dimension? Because, 1st is just the image number, the 2nd is the pixels in the 2D image.

[Adak]

Now I'm confused what your assignment is. I thought YOU were supposed to dynamically declare a 2D array - irrespective of anything your teacher did in his code.

I doubt very much if the teacher intends for you to alter his code. The way I use 2D arrays is to create them, load the data, make the computations needed, and then reload that SAME array, with some more data.

Is your array supposed to be a 2D array, but you only malloc 1D of it, and the teacher's code mallocs the second D of it?

[subclass]

I believe the later is correct.

The program is suppose to deal with text based images created from bitmap files. The professors code converts those 2D image files to a 1D text of 0-255 and places it into the second dimension of the array, so I assume he's the one malloc'ing that part :P

Sorry, but I believe I have solved the code with your help :) Thank you very much for this.

[subclass]

Hmm... Seems that I didn't do enough investigating, after each time I allocate more memory in terms of row, all previous "loads" are lost.

Am I going to have to have a temp array and each time I allocate more memory to my imgSet, move all the information back and forth to save it? Isn't there a better way?

[Adak]

Well, yeah. malloc() will always give you a "fresh" address for your pointer.

As I understand it now, your program has to create the array pointer, and the teacher's function, will create the row pointers.

How many images are you supposed to try and hold in memory, at one time? If it's not too many for your system, then I'd make ONE pointer array, and let teacher's code add the row pointers, and then I'd get ANOTHER ONE pointer array, and let the teacher's code add the row pointers, for that image, then malloc() ANOTHER ONE pointer array, and let the teacher's code add the row pointers, etc.

So the images are all in separate pointer addresses, but that's just what you need, I believe.

If that sounds wacko, then I'd say, go talk with some smart classmates, buy them a brew or taco or something, and find out what's up. Or check with the teacher.

I've never had an assignment where my code had to shoe horn in with the instructor's code, so closely.

[subclass]

Yeah, it's a little ridiculous lol.

As for the amount, its any. I figured it would be less than 25 so I just malloc()'ed 25 but I think it's off since it's still messing up when I attempt to print out any before the last.

imgSet = (unsigned char**)malloc(25*sizeof(unsigned char*));

[Adak]

You DO have a char ** pointer that you are saving now, RIGHT? Each char ** pointer IS separate, and must be saved separately.

An array of char ** would be a good way to go, I believe. Separate that way, but also organized.

[subclass]

A separate array of pointers to my imgSet[][]? I thought in C with 2D arrays, the first dimension is the pointer to the second dimension which is simply a 1D array?

I send &imgSet[*n] to my teachers code, with n being the newest image needed.

[Adak]

He's taking care of the inner array (the row). Your code is giving his code a new outer array.

You take care of the top part of this, and his code takes care of the lower part:

//Your code does this part:
  double **dat2;
  /*  allocate outer array (all pointers)  */
  dat2 = malloc( nrows*sizeof(double*));
     
  if(dat2==NULL) {
    printf("\nError allocating memory\n");
    exit(1);
  }
   // The teacher's code does this part, (all sized for data type)
  /*  allocate each row  */
  for(i = 0; i < nrows; i++) {
    dat2[i] = malloc( ncols*sizeof(double));
  }

I believe that's correct. Ignore that the data type here is a double.

[subclass]

Ah kk, I think I've figured it out now. For real this time :D