Thread: malloc/free help

hi everyone,
i'm having trouble with freeing things correctly. when i leave out most free calls, my program does run on a simple test file, but valgrind complains that i'm losing a ton of memory (obviously), and a heavier file crashes it. when i put in my free()s, though, i get a segfault and valgrind gives me a lot of invalid read/write errors. clearly i'm missing something as to where to place my free()s, but i can't find where i'm doing it wrong. any help would be great - here is my code (sorry about the length):

Code:

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

void merge(char *merge, char *shortest, char* longest);
void getMaxOverlap(char **array, int *size);
int compare(char *shortest, char *longest, int *bestOffset);
void getFragments(FILE *fp, char **array, int *arraySize);
void updateArray(char **array, int **size, int one, int two, char *mergedString);

int main (int argc, const char * argv[]) {
  FILE *fp = fopen(argv[1], "r");
  if(fp==NULL) {
    printf("Invalid file. Try again.\n");
    return -1;
  }
  
  char *array[10000];
  int arraySize = 0;
  getFragments(fp, array, &arraySize);

  while(arraySize>1) {
    printf("array size: %d\n", arraySize);
    getMaxOverlap(array, &arraySize);
  }
  printf("FINAL PARAGRAPH: \n%s\n", array[0]);
  return 0;
}

void getFragments(FILE *fp, char **array, int *arraySize) {
  char frag[1000];
  fscanf(fp, "%*[#]");  // gets rid of original # for first fragment
  while(fscanf(fp,"%999[^#]%*[#]", frag) == 1) {
    array[*arraySize] = (char*) malloc(sizeof(char)*strlen(frag));
    strcpy(array[*arraySize], frag);
    *arraySize += 1;
  }
}

void getMaxOverlap(char **array, int *size) {
  int maxOverlap = 0;
  int bestI, bestJ;
  // omitted calculation of bestI, bestJ, maxOverlap
  char *merged;
  merge(merged, array[bestI], array[bestJ]);
  updateArray(array, &size, bestI, bestJ, merged);
}

int compare(char* one, char* two, int *bestOffset) {
  int comparison = 0;
  // omitted calculation of comparison
  return comparison;
}

void merge(char *merge, char *shortest, char *longest) {
  int offset = 0;
  int overlap = compare(shortest, longest, &offset);
  int diff = strlen(longest)-strlen(shortest);
 
  char *mergedString; 
  if(offset<0) {
    mergedString = (char*)malloc(sizeof(char)*((-offset)+strlen(longest)));
  } else {
    mergedString = (char*)malloc(sizeof(char)*(strlen(longest)+(offset-diff)));
  }

  if(offset<0) {
    strcpy(mergedString,shortest);
    char end[1000];
    int count;
    for(count = 0; count < strlen(longest)-overlap; count++) {
      end[count] = longest[count+overlap];
    }
    strcat(mergedString, end);
  } else {
    strcpy(mergedString,longest);
    if(offset>diff) {
      char end[1000];
      int count;
      for(count = 0; count < strlen(shortest)-overlap; count++) {
	end[count] = shortest[count+overlap];
      }
      strcat(mergedString, end);
    }
  }
  strcpy(merge, mergedString);
}

void updateArray(char **array, int **size, int one, int two, char *mergedString) {
  if(one>two) {
    int temp = one;
    one = two;
    two = temp;
  }
  array[one] = (char*)realloc(array[one],sizeof(char)*strlen(mergedString));
  array[two] = (char*)realloc(array[two],sizeof(char)*strlen(array[**size-1]));
  strcpy(array[one], mergedString);
  strcpy(array[two], array[**size-1]);
  free(mergedString);          // <-- I think this is where the free should go
  **size-=1;
}

You're supplying mergedString as the parameter to free - and it's declared inside another function. I'm surprized you're getting a segfault and not a compiler error.

Make sure that you allocate memory, use it, then free it. Doing any of that out of order will give you problems - so make sure it's easy to keep track in your program. If you're doing different steps in different functions, you're sure to have trouble.

Problem #1:

Code:

array[*arraySize] = (char*) malloc(sizeof(char)*strlen(frag));
strcpy(array[*arraySize], frag);

To copy a string, you need strlen(string) + 1 bytes; strlen() does not count the terminating null character which is an essential part of a string. While we're on this code, sizeof(char) is 1, and malloc() doesn't need a cast, so the above can be written:

Code:

array[*arraySize] = malloc(strlen(frag) + 1);
strcpy(array[*arraySize], frag);

Next, you're doing this:

Code:

char *merged;
merge(merged, array[bestI], array[bestJ]);

and inside of your merge() function...

Code:

strcpy(merge, mergedString);

You never pointed "merged" anywhere, and so you're writing somewhere invalid.

I'm not sure if there are other problems; perhaps not allocating enough memory for mergedString, for example. The above were just examples of some problems, which might help you find others that are similar.

You should pay attention to what valgrind tells you. When it points out an error, go and look at the line it mentions. If it says invalid read, find out why! Invalid read/write errors don't (necessarily) mean that you should shuffle around your free() calls. They often indicate problems such as writing to memory that you never allocated, and freeing won't fix that.