CODEWITHSUNDEEP
cfunctionpointers
polslinux
polslinux

Reputation: 1779

how to free correctly the pointers

This is a function that i've written:

uint32_t file_list(char *path, char ***ls){
    DIR *dp;
  //uint32_t i;
  struct stat fileStat;
  struct dirent *ep = NULL;
  uint32_t len, count = 0;
  int file = 0;
  *ls = NULL;
  dp = opendir (path);
  if(dp == NULL){
    fprintf(stderr, "no dir: %s\n", path);
    exit(1);
  }

  ep = readdir(dp);
  while(NULL != ep){
    count++;
    ep = readdir(dp);
  }
  rewinddir(dp);

  *ls = calloc(count, sizeof(char *));
  count = 0;
  ep = readdir(dp);
  while(ep != NULL){
    if((file = open(ep->d_name, O_RDONLY)) < 0){
      perror("apertura file");
      exit(1);
    }
    if(fstat(file, &fileStat) != 0){
      perror("filestat");
      free(*ls);
      close(file);
      exit(EXIT_FAILURE);
    }
    close(file);
    if(S_ISDIR(fileStat.st_mode)){
      len = strlen(ep->d_name);
      (*ls)[count] = malloc(len+5); /* lunghezza stringa + "DIR \n" */
      strcpy((*ls)[count], "DIR "); /* copio DIR */
      strcat((*ls)[count++], ep->d_name); /* concateno la stringa DIR con il nome della dir */
      ep = readdir(dp);
    }
    else{
      (*ls)[count++] = strdup(ep->d_name);
      ep = readdir(dp);
    }
  }
  /*for(i=0; i<count; i++){
    free((*ls)[count]);
  }*/
  (void)closedir(dp);
  return count;
}

into the main program i have char **files and then the part where i get the count is count = file_list("./", &files);
What is my problem?
Everbody know that the dynamically allocated memory they (the pointers) might refer to, must be freed but if i free the pointers (with the for loop) then into the main program i got an unexpected behaviour during the file list (duplicate file name, no file name, etc).
In fact if i don't free the pointers all work perfectly.
So my question is: how to free these pointers?
Thanks in advance!

Upvotes: 0

Views: 181

Answers (4)

RobH
RobH

Reputation: 1639

For starters, your ls parameter is a char***, so if that's what you were going for, you need to have an inner loop to free up all of the inner pointers (**ls) before freeing up each of the other ones (*ls). If you only want double indirection on ls, then get rid of one of the *s in your parameter declaration.

(BTW, you needn't use array notation in your loop to free up the pointers (or anywhere else that you access the allocated elements in ls). free(*(ls + count)); is usually the more accepted idiom. Pointer arithmetic works regardless of what the data type is (other than void), because the compiler takes that into account.)

Upvotes: 1

netcoder
netcoder

Reputation: 67745

Everbody know that the pointers must be freed but if i free the pointers (with the for loop) then into the main program i got an unexpected behaviour during the file list (duplicate file name, no file name, etc).

Your problem is that you allocate and free in the same function, which render your function basically useless (if I understand correctly anyway). If you free in the same function, after it returns (i.e.: in the "main program"), you end up accessing memory segments that have been released to the operating system, which is undefined behavior.

You'd need two functions, one for the allocation (the one above), and one to free it once you're done, per example:

char** files;
uint32_t count = file_list("./", &files);
// do something with files here
file_list_free(&files, count);

Keep in mind that your free function will need to know the count to prevent a buffer overrun.

There are other problems with your code (e.g.: not checking the return value of calloc, etc.), but it would be too long to cover them all here (and don't necessarily relate to your actual question).

Upvotes: 2

Emilio Silva
Emilio Silva

Reputation: 2141

You have to define an extra function, so the caller can free the data when it is done using it:

void free_file_list(char ***ls, int count) {
    for(int i=0; i<count; i++){
        free((*ls)[i]);
    }
    free(*ls);
}

Your documentation then must explain that each call to file_list must be matched with a call to free_file_list.

I would store count in a struct like this, though:

struct {
    char ***ls;
    int count;
} FILE_LIST;

Upvotes: 1

ecatmur
ecatmur

Reputation: 157484

Free the pointers in the main program, after you've finished using them.

You need to free *ls as well (again, after you've finished with it).

Upvotes: 0

Related Questions