malloc in C. segmentation fault (time to time not always)

Question

My program reads all files in directory "./srcs/" and makes an array of name of the files found.

Then I use the array in main with switch to make a test for each function.

#include <dirent.h>
#include <stdio.h>
#include <string.h>
#include <sys/types.h>
#include <stdlib.h>

void *ft_memset(void *dest, int c, size_t nofb);

char **show_dir_content(char * path)
{
DIR *dir;
struct dirent *ent;
struct dirent *entry;
char **array;
int i;
int len;

i = 0;
len = 0;

dir = opendir(path);
if(!dir)
{
    perror("diropen");
    return(0);
}

while((ent = readdir(dir)) != NULL)
{
    if ((ent-> d_name[0]) != '.')
    {
        len++;
    }
}
closedir(dir);
dir = opendir(path);
array = (char**)malloc(sizeof(*array) * len);
while ((entry = readdir(dir)) != NULL)
{
    if ((entry-> d_name[0]) != '.')
    {
        array[i] = entry-> d_name;
        i++;
    }
}
closedir(dir);
return (array);
}

Here an function which read files from directory and put them in array. Idea is by using switch and while iterate all names of function and execute test. Special test for each.

For that I'm using next menu:

int main(int argc, char **argv)
{
char c[6] = "123fg";
char d[6] = "123fg";
char **ptr;
int hack;
ptr = show_dir_content("./srcs/");

while(*ptr)
{
    if(strcmp(*ptr, "ft_memset.c") == 0)
    {
        hack = 1;
    }

    switch (hack){
        case    1:
            ft_memset(c, 'A', 3);
            memset(d, 'A', 4);
            if(strcmp(c, d) == 0)
            {
                printf("%s", "OK");
            }
            else
            {
                printf("%s", "KO");
            }

            break;
                }
    ptr++;
}
free(ptr);
return(0);
}

As test I'm using memset lib function vs ft_memset function created by myself. It has next code:

#include <string.h>

void *ft_memset(void *dest, int c, size_t nofb)
{
unsigned char* p = dest;
while (nofb--)
    *p++ = (unsigned char)c;
return (dest);
}

Did you try using a debuger to isolate the cause of the segmentation fault? — Gnqz, Oct 30 '17 at 11:58
I haven't looked at your code closely enough to know if this is your actual problem, but have you thought about what happens if something is added to the directory while you're processing it? — Andrew Henle, Oct 30 '17 at 11:59
Add as tags the OS you are using, and the compiler for some context. If using gcc on a UNIX / Linux then you should google "gdb seg fault core backtrace" Learn how to view the stack trace that will give you the best clue as to where things are going wrong. — Rob Kielty, Oct 30 '17 at 12:04
`array[i] = entry-> d_name;` ==> `array[i] = strdup(entry-> d_name);` — mch, Oct 30 '17 at 12:06
Compile with all warnings and debug info (e.g. `gcc -Wall -Wextra -g` with [GCC](http://gcc.gnu.org/)...). Improve the code to get no warnings. Then **use the debugger** `gdb` and [valgrind](http://valgrind.org/). Be very [afraid](https://stackoverflow.com/a/25636788/841108) of [**undefined behavior**](https://en.wikipedia.org/wiki/Undefined_behavior) — Basile Starynkevitch, Oct 30 '17 at 12:09
You also need to store a `NULL` pointer at the end of `array`, since that's the test you're using to detect the end of it. So you need to allocate space for an extra pointer at the end, and set it to `NULL`. — Tom Karzes, Oct 30 '17 at 12:10
Also keep in mind that there's a race condition in this code: If the contents of the directory changes between the two passes, then you could overwrite `array`. — Tom Karzes, Oct 30 '17 at 12:11
Consider also using [nftw(3)](http://man7.org/linux/man-pages/man3/nftw.3.html) — Basile Starynkevitch, Oct 30 '17 at 12:13

score 4 · Accepted Answer · answered Oct 30 '17 at 12:07

You can't rely on the contents of entry continuing to exist outside of the readdir loop. And also each element of array will point to the same string as d_name will be the same pointer in memory, just with a different string each time.

replace

array[i] = entry->d_name;

with

array[i] = strdup(entry->d_name);

and then remember to free each element of array later.

score 1 · Answer 2 · answered Oct 30 '17 at 12:09

This loop does not do what you expect:

while ((entry = readdir(dir)) != NULL)
{
    if ((entry-> d_name[0]) != '.')
    {
        array[i] = entry-> d_name;   // <- the problem lies here
        i++;
    }
}

Array is an allocated array of pointers. array[i] = entry-> d_name; simply causes array[i] point to entry->d_name, and the memory used by entry will be destroyed at closedir. You should instead duplicate the content of entry->d_name:

while ((entry = readdir(dir)) != NULL)
{
    if ((entry-> d_name[0]) != '.')
    {
        strdup(array[i], entry-> d_name;)
        i++;
    }
}

Of course all this will have to be freed when done:

But that's not all: you neither return the size of the allocated array, nor explicitely end it with a NULL pointer, so the caller has no way to know the size.

Is there an alternate prototype for `stdup(...)` that I am not aware of? i.e., this statement : `strdup(array[i], entry-> d_name;)` is not right. — ryyker, Oct 30 '17 at 13:02

score 0 · Answer 3 · answered Oct 30 '17 at 17:25

Your malloc is not creating the segmentation error. Actually you are not properly allocating the memory as you are confused with pointer or pointer concept. Your array is a char array pointer of char string. But you are storing the name of the files in each location of this character pointer without allocating the memory for holding the file names. You have just allocated the size of total number of files only. But to work properly each pointer also need allocation of each file length. Hence while traversing the file name it giving you segmentation error as there is no contiguous memory in it . Do the following modification in your code and see it will fix your issue.

 //below is your code
 array = (char**)malloc(sizeof(*array) * len);// here len is just a total number of files but you forgot one thing each file name has its own length

//correct it like
array = malloc(len); // malloc by default return a character pointer so you no need to cast it and size of char is 1 so multiply with 1 means?
// now above you have a array of pointers and its length is total number of available files. Each location in this array can hold the address of a string pointer
//now to store the name of the file (as string) you need a another character array who's size should be the length of (file name + 1)

while ((entry = readdir(dir)) != NULL)
{
    if ((entry-> d_name[0]) != '.')
    {
            array[i] = malloc(strlen(entry->d_name)+1)
            strcpy(array[i],entry->d_name)
            i++;
    }
}

malloc in C. segmentation fault (time to time not always)

3 Answers3