char** allocation and how to store values

Question

    void allocateFolderTree(char **tree)
    {
        int i;
        tree = (char **)malloc(sizeof(char*)*MAX_FOLDERS);
        for(i=0;i<MAX_FOLDERS;i++)
          tree[i] = (char *)malloc(sizeof(char)*MAX_FOLDERS*MAX_FILENAME);    
    }

    void getFolderTree (char **tree, char *path, int i)
    {
        DIR *dir = opendir(path);
        struct dirent *entry;
        while (entry = readdir(dir)) 
        {
        if( !strcmp(entry->d_name, ".") || !strcmp(entry->d_name, ".."))
            continue;   

        if (entry->d_type & DT_DIR)//check file type
        {
            //segfault for the next 3 lines
            strcpy(tree[i], path);
            strcat(tree[i], "/");
            strcat(tree[i], entry->d_name);

            i++;

            char *new_path = malloc(sizeof(char)*(strlen(path)+MAX_FILENAME));
            strcpy(new_path, path);
            strcat(new_path, "/");
            strcat(new_path, entry->d_name);
            getFolderTree(tree, new_path, i);
            free(new_path);
        }
        }
        closedir (dir);
    }

int main ()
{
    char **folderTree;
    allocateFolderTree(folderTree);
    getFolderTree(folderTree, ROOT, 0);

    free(folderTree);
    return 0;
}

Why am I getting segfault and how do I solve this?

PS: MAX_FOLDERS=1000 MAX_FILENAME=30 The folders I'm reading are less than 10 and each has a name less than 30!

You are getting segfault probably because you are not creating enough space. Note that `MAX_FOLDERS` is probably not related to the actual length of your directory names, and you aren't leaving room for a null byte even if you were. Verify the length of the strings you are copying and the length you are allocating and you should see your error. — Owen S., Mar 16 '14 at 22:18
Strcat goes to end to append, you have allocated the memory, but strcat goes to the of that memory, so you are trying to append to unallocated space... — Joe DF, Mar 16 '14 at 22:19
About Malloc see http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc — Joe DF, Mar 16 '14 at 22:20
shouldn't strcat append at the end of the string added when I called strcpy? — Mihai Bratulescu, Mar 16 '14 at 22:26
Wrap your memory allocations and path concatenation routines in functions, and check for bounds in each of them. Doing this "on the fly" is just asking for trouble (as you may have noticed). — michaelmeyer, Mar 16 '14 at 22:31

score 1 · Accepted Answer · answered Mar 16 '14 at 23:13

This code:

void allocateFolderTree(char **tree)
{
    int i;
    tree = (char **)malloc(sizeof(char*)*MAX_FOLDERS);
    for(i=0;i<MAX_FOLDERS;i++)
      tree[i] = (char *)malloc(sizeof(char)*MAX_FOLDERS*MAX_FILENAME);    
}

modifies the local copy of tree in the function, but never returns it to the calling code, so the allocated memory is all lost immediately the function returns. You have at least two choices on how to fix this:

char **allocateFolderTree(void)
{
    int i;
    char **tree = (char **)malloc(sizeof(char*)*MAX_FOLDERS);
    for(i=0;i<MAX_FOLDERS;i++)
      tree[i] = (char *)malloc(sizeof(char)*MAX_FOLDERS*MAX_FILENAME);
    return tree;  
}

or:

void allocateFolderTree(char ***tree)
{
    int i;
    *tree = (char **)malloc(sizeof(char*)*MAX_FOLDERS);
    for(i=0;i<MAX_FOLDERS;i++)
      (*tree)[i] = (char *)malloc(sizeof(char)*MAX_FOLDERS*MAX_FILENAME);    
}

On the whole, avoiding triple pointers is a good idea, so I'd normally go with the other option.

I also observe that all the loops are allocating a lot of space in the inner loop. Are you sure you want to include MAX_FOLDERS in the size? On the face of it, you should be allocating the size of MAX_FILENAME, not MAX_FOLDERS * MAX_FILENAME. (Each inner allocation is currently allocating around 30 KiB, so in total you're allocating around 30 MiB of space.)

Note that sizeof(char) == 1 by definition, so there is little need to include it in the size calculation.

what is the max path length of a file (filename included)? to replace it with MAX*MAX — Mihai Bratulescu, Mar 17 '14 at 00:10
Are you looking for `PATH_MAX`? That's a standard but hard-to-track parameter, nominally set in ``, but since it can vary by file system, you may end up needing to use `pathconf()` to determine the value to use, or fall back on `_POSIX_PATH_MAX`, which is the minimum allowed value for the maximum length of a path name. On the whole, you'd do best to allocate the space for the file names as you generate them, rather than pre-allocating as in the current code. Most likely, you'll dramatically over-allocate with the current code, whereas you can allocate just enough the other way. — Jonathan Leffler, Mar 17 '14 at 00:12

Angus Comber · Answer 2 · 2014-03-16T23:15:29.257

This fixes your memory allocation problems.

I changed a little. Your main problem was the first malloc in allocateFolderTree. In allocateFolderTree you can dereference to one level and malloc, but not up two levels which over-writes the address of folderTree (the argument) and that means when the function returns you have no way of accessing folderTree.

MAX_FOLDERS is baaaad. You need to calculate the number of folders first and then allocate based on this number.

Eg when I run the program, folderTree has an address of: 0x0021edf0.

Each folderTree array element is a null pointer before allocateFolderTree is called.

Then in allocateFolderTree each array element is allocated memory on the heap.

When you return from allocateFolderTree, the tree parameter still retains the address 0x0021edf0 but each array element now contains the address of memory on the heap.

#define MAX_FOLDERS 1000
#define MAX_FILENAME 256

#include <sys/types.h>
#include "dirent.h"

void allocateFolderTree(char** tree)
{
   int i;
   for(i=0;i<MAX_FOLDERS;i++)
      tree[i] = (char *)malloc(sizeof(char)*MAX_FOLDERS*MAX_FILENAME);    
}

void getFolderTree (char **tree, char *path, int i)
{
   DIR *dir = opendir(path);
   struct dirent *entry;
   while (entry = readdir(dir)) 
   {
      if( !strcmp(entry->d_name, ".") || !strcmp(entry->d_name, ".."))
         continue;   

      if (entry->d_type & DT_DIR)//check file type
      {
         //segfault for the next 3 lines
         strcpy(tree[i], path);
         strcat(tree[i], "/");
         strcat(tree[i], entry->d_name);

         i++;

         char *new_path = (char*)malloc(sizeof(char)*(strlen(path)+MAX_FILENAME));
         strcpy(new_path, path);
         strcat(new_path, "/");
         strcat(new_path, entry->d_name);
         getFolderTree(tree, new_path, i);
         free(new_path);
      }
   }
   closedir (dir);
}

int main ()
{
   char* ROOT = "/";
   char* folderTree[MAX_FOLDERS] = {0};
   allocateFolderTree(folderTree);
   getFolderTree(folderTree, ROOT, 0);

   //You will have to create your own free_foldertree function to free each array item
   //free(folderTree);
   return 0;
}

char** allocation and how to store values

2 Answers2