Is this the right/best way to free up the memory

I got my code to work, but need some feedback if I am free up the memory correctly? Is there a way to make sure that I have clear up the memory? I just wanted to get in the good habit since I am pretty new at this. Thanks for the feed back:

Code:

---


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


char *readfile(const char *filename);


int main(int argc, char *argv[]) {
  char *pms;
  int size = 1;
  pms = readfile(argv[1]);
  printf("%s", pms++);
  return (0);
}


char *readfile(const char *filename){
  static char c;
  char *out;
  FILE *myfile;
  int size;
  out = (char *)malloc(24130);
  int x=0;
  myfile = fopen(filename, "r");
  while ((c=fgetc(myfile)) != EOF){
        out[x]=c;
        x++; }
  //resize the memory here
  size = strlen(out)+1;
  if(!realloc(out, size))
  {
    printf("Unable to allocate memory");
    return (0);
  }
  return(out);
  free(out);


}

---

You're not freeing the memory at all. The free call comes after the return statement so it's never executed.
You are using the returned memory in main so it has to be freed after use in main().
Also your logic to realloc the memory looks a little bit strange. It only works if realloc alwais reduces the size.
Kurt

Quote:

---

but need some feedback if I am free up the memory correctly?

---

No your not freeing your memory correctly. When the program encounters a return statement it immediately returns to the calling function, so your free() call is never reached.

Also if your file contains more than 24130 characters you will access your array out of bounds. Plus since you are using fgetc() and you don't nul terminate the array you can't use any of the C-string functions like strlen() or printf("%s").

Also why the increment in the printf() in main()?


Jim

Additionally,

> while ((c=fgetc(myfile)) != EOF)
This loop does not check to see if the buffer is going to overrun.

> size = strlen(out)+1;
You need to explicitly append a \0 if you want to use strlen.
It's largely a waste of effort, you have the length in x already.

Quote:

---

Originally Posted by jimblumberg

No your not freeing your memory correctly. When the program encounters a return statement it immediately returns to the calling function, so your free() call is never reached.

Also if your file contains more than 24130 characters you will access your array out of bounds. Plus since you are using fgetc() and you don't nul terminate the array you can't use any of the C-string functions like strlen() or printf("%s").

Also why the increment in the printf() in main()?


Jim

---

Jim,

Thanks. Yeah.. I read both your and Kurt's feedback. I moved my free function to main right before return. I also took off the increment in the printf. I guessed I thought walk through each char, but this is great. Thanks for reviewing. I have to come up with a better way on allocating the memory.

Thanks Salem.

Are you now properly terminating your array to make it a C-string?

You may want to post your modified code.

Jim

Quote:

---

Originally Posted by jimblumberg

Are you now properly terminating your array to make it a C-string?

You may want to post your modified code.

Jim

---

Here is the revised code. Still not sure about the way I allocated the memory. Just could not figure out without having to put in the initial fix size first.

I don't know the file size until after the loop.

Thanks,

Code:

---

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


char *readfile(const char *filename);


int main(int argc, char *argv[]) {
  char *pms;
  int size = 1;
  pms = readfile(argv[1]);
  printf("%s", pms);
  //add free memory here ..
  free(pms);
  return (0);
}


char *readfile(const char *filename){
  static char c;
  char *out;
  FILE *myfile;
  int size;
  //still need to work on better way to allocat memory
 // not sure where/how to place this without having to know the actual size.
  out = (char *)malloc(94130);
  int x=0;
  myfile = fopen(filename, "r");
  while ((c=fgetc(myfile)) != EOF){
        out[x]=c;
        x++; }
  //Add Null terminate out
  out[x] ='\0';
  //resize the memory here
  size = strlen(out);
  if(!realloc(out, size))
  {
    printf("Unable to allocate memory");
    return (0);
  }
  return(out);


}

---

You still need to insure you don't access your array out of bounds during your read by insuring x is less than the size you allocated.


In a C program you don't need to cast the return value from malloc.

Jim

> static char c;
There is no point in this being static.
Also, it should be an int, so you can compare it with EOF properly.

> if(!realloc(out, size))
1. realloc can move the memory, even if it's making the block smaller. Technically, it could also fail to make it smaller.
2. As written, you'll chop the \0 off the end of the new block.

You should do something like

Code:

---

void *temp = realloc( out, size+1 );
if ( temp != NULL ) {
    out = temp;
} else {
  // out still points to the oversized block
}

---

thanks for your time everyone.!