malloc/free help

This is a discussion on malloc/free help within the C Programming forums, part of the General Programming Boards category; hi everyone, i'm having trouble with freeing things correctly. when i leave out most free calls, my program does run ...

  1. #1
    Registered User
    Join Date
    Apr 2009
    Posts
    1

    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;
    }

  2. #2
    ATH0 quzah's Avatar
    Join Date
    Oct 2001
    Posts
    14,826
    A simple piece of advice: If at all possible, free in the same function you allocate. Consider:
    Code:
    void foo( ... )
    {
        type * var;
    
        var = getmemoryhere( );
    
        ... do stuff ...
    
        freememoryhere( var );
    }
    It is very rare that I would consider passing something off to have some random function free it:
    Code:
    void foo( ... )
    {
        type * var;
    
        var = getmemoryhere( );
        ...
        dorandomstuff( &var ); /* oh and by the way we just  freed that... */
    
    }
    That's very poor design in my opinion. Too hard to keep track of what's getting freed and allocated where.

    Quzah.
    Hope is the first step on the road to disappointment.

  3. #3
    Super Moderator
    Join Date
    Sep 2001
    Posts
    4,913
    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.

  4. #4
    cas
    cas is offline
    Registered User
    Join Date
    Sep 2007
    Posts
    993
    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.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. New/delete vs malloc/free
    By KBriggs in forum C++ Programming
    Replies: 6
    Last Post: 07-07-2009, 04:08 PM
  2. redirect malloc/free
    By pheres in forum C Programming
    Replies: 0
    Last Post: 03-21-2009, 04:28 AM
  3. malloc/free of return value
    By Bladactania in forum C Programming
    Replies: 15
    Last Post: 02-10-2009, 01:04 PM
  4. Tracking memory in malloc/free.
    By matsp in forum C Programming
    Replies: 10
    Last Post: 01-07-2009, 05:16 AM
  5. malloc/free mark
    By yosoyvenezolano in forum C Programming
    Replies: 11
    Last Post: 12-01-2008, 10:45 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21