Thread: malloc char** seg faults

> printf("after malloc %s %s\n", path,*names);
You only just allocated names, and now you're trying to dereference it.

You should fix those printf calls: yes, to you it looks like they came after the segfault, but that could be masked by the fact that that printf could be exactly where the segfault is happening, so you ended up wrongly blaming it on the malloc, and changing to calloc masked the mistake again.

Originally Posted by laserlight

You should fix those printf calls: yes, to you it looks like they came after the segfault, but that could be masked by the fact that that printf could be exactly where the segfault is happening, so you ended up wrongly blaming it on the malloc, and changing to calloc masked the mistake again.

how so? the printf are to locate when it is happening. it seg faulted somewhere, that wittled it down to the where. because it was not getting past the malloc

Code:

      path = strdup(argv[2]);        getAmount(path,0, &sum);
         
        printf("i=%d\n",i);
        printf("sum=%ld\n",sum);
        printf("before malloc %s\n", path);
         
        //segfaults here
         
        names = (char **)malloc(sum * sizeof(char *));
     
        if (names == NULL)
        {
            printf("Names is NULL\n");
        }
        printf("after malloc %s %s\n", path,*names);
        gotNames =    listdir(path,names,0);      
        printf("after malloc\n");

how can any of these printf's cause a seg fault that was already occurring prior to them being placed there? please explain your logic behind that theory.

Code:

int main(int argc, char **argv) {
    char *path;
    char **gotNames;
    char **names;
    int g; 
    unsigned long sum;
    
    if(argc < 2)
    {
        printf("no path.\n");
        return EXIT_FAILURE;
    }
    
    if (!strcmp(argv[1], "-p"))
    {
        path = strdup(argv[2]);
        getAmount(path,0, &sum);
       names = (char **)malloc(sum * sizeof(char *));
        gotNames = listdir(path,names,0);      
        
            
    }
    else
    {
        printf("could not get files missing -p.\n");
        return EXIT_FAILURE;
    }

    for(g = 0; g < i; g++) {
    ;
        //printf("%s\n",gotNames[g]);
    }
      
    printf("howmany %d\n", i);
        
    sort_file_list(gotNames, sum);
    
    return 0;
}

@poorboy, Post the code that segfaults without the printf's. And please explain your theory as to how calloc solved the problem.

BTW, you don't need the global variables. And you don't need to count the files first if you instead realloc memory as needed. Also, you can insert the files in order as you read them (which can't be that much worse than a bubble sort). I'll see if I can whip up an example for you.

EDIT: Here's a basic example. I haven't really tested it much, though.

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
 
#include <unistd.h>
#include <dirent.h>
#include <sys/stat.h>
#include <sys/types.h>
 
typedef struct Files {
    size_t capacity;
    size_t size;
    char** names;
} Files;
 
void insertFilename(Files *f, char *name) {
    // Expand names array if necessary
    if (f->size == f->capacity) {
        f->capacity += 200; // there are many possible growth strategies
        char **p = realloc(f->names, f->capacity * sizeof *f->names);
        if (!p) {
            perror("pushFilename: realloc");
            exit(EXIT_FAILURE);
        }
        f->names = p;
    }
    // Insert name in order
    size_t i = 0;
    for ( ; i < f->size; ++i)
        if (strcmp(name, f->names[i]) <= 0)
            break;
    f->size++;
    memmove(&f->names[i+1], &f->names[i], (f->size - i) * sizeof *f->names);
    f->names[i] = name;
}
 
void readFilenames_r(const char *dirname, Files *files) {
    DIR *dir = opendir(dirname);
    if (!dir) {
        perror("listDir: opendir");
        exit(EXIT_FAILURE);
    }
    char path[1024];
    struct dirent *entry;
    while ((entry = readdir(dir)) != NULL) {
        snprintf(path, sizeof(path), "%s/%s", dirname, entry->d_name);
        if (entry->d_type == DT_DIR) {
            if (!strcmp(entry->d_name, ".") || !strcmp(entry->d_name, ".."))
                continue;
            readFilenames_r(path, files);
        }
        else {
            char *name = strdup(entry->d_name);
            if (!name) {
                perror("readFilenames_r: strdup");
                exit(EXIT_FAILURE);
            }
            insertFilename(files, name);
        }
    }
    closedir(dir);
}
 
Files readFilenames(const char *dirname) {
    Files files = { .capacity = 0, .size = 0, .names = NULL };
    readFilenames_r(dirname, &files);
    return files;
}
 
int main(int argc, char **argv) {
    if (argc < 2) {
        printf("Usage: listfiles PATH\n");
        return EXIT_FAILURE;
    }
 
    const char *path = argv[1];
 
    // Read filenames
    Files files = readFilenames(path);
 
    // Print names
    for (size_t i = 0; i < files.size; ++i)
        printf("%s\n", files.names[i]);
 
    // Free memory
    for (size_t i = 0; i < files.size; ++i)
        free(files.names[i]);
    free(files.names);
 
    return 0;
}

Originally Posted by poorboy

how so? the printf are to locate when it is happening. it seg faulted somewhere, that wittled it down to the where. because it was not getting past the malloc
(...)
how can any of these printf's cause a seg fault that was already occurring prior to them being placed there? please explain your logic behind that theory.

That would be true if your printf debugging code only printed string literals. However, you're not merely doing that. Take another look at this code snippet from the main function in post #1:

Code:

printf("before malloc %s\n", path);

//segfaults here

names = (char **)malloc(sum * sizeof(char *));

if (names == NULL)
{
    printf("Names is NULL\n");
}
printf("after malloc %s %s\n", path,*names);

Suppose that malloc does not return a null pointer. Then, "Names is NULL\n" will not be printed, and indeed that was so. But what names points to is uninitialised, i.e., *names results in a (probably) non-null pointer to char that doesn't actually point to anything. Consequently this:

Code:

printf("after malloc %s %s\n", path,*names);

... results in undefined behaviour. Yes, you're just using it to check whether control has reached that point, but because you tried to print *names, you have a problem. This is why it is possible that there was no problem with the malloc call, but rather you mistook your erroneous printf that resulted in a segmentation fault for a segmentation fault due to the malloc call.

As john.c has mentioned though, you should "Post the code that segfaults without the printf's. And please explain your theory as to how calloc solved the problem." Changing malloc to calloc actually shouldn't fix the problem: what will happen is that names would to point to memory that has been zero initialised. This would mean that *names would be a null pointer, which is still not a valid argument to printf for the %s format specifier.

why does it seg fault when I use malloc, and it does not seg fault using calloc , rather, when using calloc I can read in

Code:

  find /media/data1/HPImages -type f | wc -l
109728

files without error.

Code:

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

//global to not reset count to zero
//in recursive function
int i = 0, b = 0;
int getcount = 1;

char       **
randomize_file_list(char **names, unsigned long len)
{ printf("randomize file list\n");
   int                 r;
   unsigned long       i;
   char               *tmp;

   for (i = 0; i < len - 1; i++)
     {
    r = (int)((len - i - 1) * ((float)rand()) / (RAND_MAX + 1.0)) + i + 1;
    tmp = names[i];
    names[i] = names[r];
    names[r] = tmp;
     }
     printf("leaving randomize file list\n");
   return (names);
}

char       **
sort_file_list(char **names, unsigned long len)
{ printf("sort file list 342\n%s \n%ld\n",*names,len);
   unsigned long       i;
   unsigned char       done = 0;

   while (!done)
     {
    done = 1;
    //set at 1 because zero is NULL
    for (i = 0; i < len - 1; i++)
      { printf("in for loop 353\n");
          printf("i=%ldstrcmp(names[%s], names[i + 1 %s]) > 0\n",i, names[i], names[i+1]);
         if (strcmp(names[i], names[i + 1]) > 0)
           {
          char               *tmp;

          tmp = names[i];
          names[i] = names[i + 1];
          names[i + 1] = tmp;
          done = 0;
           }
      }
     }
     printf("leaving sort_file_list\n");
   return (names);
}
//get total files to know how much to malloc 2d char array

void getAmount(const char *name, int indent, unsigned long *sum) 
{ printf("get amount\n");
    DIR *dirp;
    struct dirent *entry;
   char path[1024];
    
       if (!(dirp = opendir(name)))
       {
           printf("cannot open dir.\n");
           exit(1);
       }
 
    
    while ((entry = readdir(dirp)) != NULL) {
        if (entry->d_type == DT_DIR) {
           
            if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0)
                continue;
            snprintf(path, sizeof(path), "%s/%s", name, entry->d_name);
         
            getAmount(path, indent + 2, sum);
        } else {
        
            
            i++;
        }
    }
   closedir(dirp);
   *sum = i;
   printf("leaving get amount\n");
 }

char ** listdir(const char *name, char *names[], int indent)
{printf("listdir %s, %s, %d\n", name,*names,indent);
    
    DIR *dir;
    struct dirent *entry;
    char path[1024];
      
        if (!(dir = opendir(name)))
           return  ((char **)NULL);
 
  printf("n %s c %d \n", names[b], b);
  
    while ((entry = readdir(dir)) != NULL) {
        if (entry->d_type == DT_DIR) {
          
            if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0)
                continue;
            snprintf(path, sizeof(path), "%s/%s", name, entry->d_name);
         
            listdir(path, names, indent + 2);
        } else {
         
          snprintf(path, sizeof(path), "%s/%s", name, entry->d_name);
          printf("%s\n",path);
          
            names[b] = strdup(entry->d_name);
            b++;
        }
    }
    closedir(dir);
    printf("returning listdir\n");
    return (names);
}

int main(int argc, char **argv) {
    char *path;
    char **gotNames;
    char **names;
    int g; 
    unsigned long sum;
    
    if(argc < 2)
    {
        printf("no path.\n");
        return EXIT_FAILURE;
    }
    
    if (!strcmp(argv[1], "-p"))
    {
        path = strdup(argv[2]);
        getAmount(path,0, &sum);
        
        //using this causes seg fault
        //names = (char **)malloc(sum * sizeof(char *));
        
        //using this it does not seg fault
        names = (char**) calloc(sum, sizeof(char*));
        gotNames =    listdir(path,names,0);      
    }
    else
    {
        printf("could not get files missing -p.\n");
        return EXIT_FAILURE;
    }

    for(g = 0; g < i; g++) {
    ;
        //printf("%s\n",gotNames[g]);
    }
      
    printf("howmany %d\n", i);
        
    sort_file_list(gotNames, sum);
    
    for(g = 0; g < i; g++) {
        printf("%s\n",gotNames[g]);
    }
    randomize_file_list(gotNames, sum);
    printf("\n\n\n\n\n");
    for(g = 0; g < i; g++) {
        printf("%s\n",gotNames[g]);
    }
        
        
    return 0;
}

Originally Posted by poorboy

I completely understand all of that, but this was not a add every printf all at once procedure that I did, the overs above the malloc were added first one at a time, for each run checking to see what I had going into malloc prior to adding the ones after words. I guess I should have taken them out before posting, it is very misleading to whatever the actual problem was.
order of execution.

You wouldn't have needed to take them out (and after all they could come in useful if someone wanted to compile and run your code to see if they get the same effect!) if you wrote them to correctly give debugging info in the first place. For example, that offending printf I highlighted only needed to be:

Code:

printf("after malloc %s\n", path);

After all, if there was no segfault before the malloc call, you still wouldn't want to try and print *names before initialising it to a valid non-null pointer, so there's no useful debugging info to get by trying to print it there. Likewise, in listdir, there was no point printing *names since on the very first call it would not be valid at that point, and on subsequent calls it would not change and hence not be useful. If you really wanted to manually check that names was not a null pointer as part of debugging, you could have gone with:

Code:

printf("after malloc %s %p\n", path, (void*)names);

You're really too much of an a-hole to bother with.
It's the printfs, you braindead idiot.
Why can't you understand that?

Originally Posted by poorboy

why does it seg fault when I use malloc, and it does not seg fault using calloc , rather, when using calloc I can read in

I'll just shrug and tell you that's the nature of undefined behaviour. For what it's worth, I tried compiling and running your code on gcc 5.4.0 and Ubuntu 16.04, and you're right: somehow the zero initialisation from the calloc call didn't result in a segmentation fault when control encountered the problematic printf calls remaining in your code, but the non-initialisation from the malloc call did. That doesn't mean changing from malloc to calloc was the correct solution: when I removed the problematic printf calls from your listdir function, the segmentation faults disappeared as expected despite the use of malloc:

Code:

#include <unistd.h>
#include <sys/types.h>
#include <dirent.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/stat.h>
 
//global to not reset count to zero
//in recursive function
int i = 0, b = 0;
int getcount = 1;
 
char       **
randomize_file_list(char **names, unsigned long len)
{ printf("randomize file list\n");
   int                 r;
   unsigned long       i;
   char               *tmp;
 
   for (i = 0; i < len - 1; i++)
     {
    r = (int)((len - i - 1) * ((float)rand()) / (RAND_MAX + 1.0)) + i + 1;
    tmp = names[i];
    names[i] = names[r];
    names[r] = tmp;
     }
     printf("leaving randomize file list\n");
   return (names);
}
 
char       **
sort_file_list(char **names, unsigned long len)
{ printf("sort file list 342\n%s \n%ld\n",*names,len);
   unsigned long       i;
   unsigned char       done = 0;
 
   while (!done)
     {
    done = 1;
    //set at 1 because zero is NULL
    for (i = 0; i < len - 1; i++)
      { printf("in for loop 353\n");
          printf("i=%ldstrcmp(names[%s], names[i + 1 %s]) > 0\n",i, names[i], names[i+1]);
         if (strcmp(names[i], names[i + 1]) > 0)
           {
          char               *tmp;
 
          tmp = names[i];
          names[i] = names[i + 1];
          names[i + 1] = tmp;
          done = 0;
           }
      }
     }
     printf("leaving sort_file_list\n");
   return (names);
}
//get total files to know how much to malloc 2d char array
 
void getAmount(const char *name, int indent, unsigned long *sum) 
{ printf("get amount\n");
    DIR *dirp;
    struct dirent *entry;
   char path[1024];
     
       if (!(dirp = opendir(name)))
       {
           printf("cannot open dir.\n");
           exit(1);
       }
  
     
    while ((entry = readdir(dirp)) != NULL) {
        if (entry->d_type == DT_DIR) {
            
            if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0)
                continue;
            snprintf(path, sizeof(path), "%s/%s", name, entry->d_name);
          
            getAmount(path, indent + 2, sum);
        } else {
         
             
            i++;
        }
    }
   closedir(dirp);
   *sum = i;
   printf("leaving get amount\n");
 }
 
char ** listdir(const char *name, char *names[], int indent)
{//printf("listdir %s, %s, %d\n", name,*names,indent);
     
    DIR *dir;
    struct dirent *entry;
    char path[1024];
       
        if (!(dir = opendir(name)))
           return  ((char **)NULL);
  
  //printf("n %s c %d \n", names[b], b);
   
    while ((entry = readdir(dir)) != NULL) {
        if (entry->d_type == DT_DIR) {
           
            if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0)
                continue;
            snprintf(path, sizeof(path), "%s/%s", name, entry->d_name);
          
            listdir(path, names, indent + 2);
        } else {
          
          snprintf(path, sizeof(path), "%s/%s", name, entry->d_name);
          printf("%s\n",path);
           
            names[b] = strdup(entry->d_name);
            b++;
        }
    }
    closedir(dir);
    printf("returning listdir\n");
    return (names);
}
 
int main(int argc, char **argv) {
    char *path;
    char **gotNames;
    char **names;
    int g; 
    unsigned long sum;
     
    if(argc < 2)
    {
        printf("no path.\n");
        return EXIT_FAILURE;
    }
     
    if (!strcmp(argv[1], "-p"))
    {
        path = strdup(argv[2]);
        getAmount(path,0, &sum);
         
        //using this causes seg fault
        names = (char **)malloc(sum * sizeof(char *));
         
        //using this it does not seg fault
        //names = (char**) calloc(sum, sizeof(char*));
        gotNames =    listdir(path,names,0);      
    }
    else
    {
        printf("could not get files missing -p.\n");
        return EXIT_FAILURE;
    }
 
    for(g = 0; g < i; g++) {
    ;
        //printf("%s\n",gotNames[g]);
    }
       
    printf("howmany %d\n", i);
         
    sort_file_list(gotNames, sum);
     
    for(g = 0; g < i; g++) {
        printf("%s\n",gotNames[g]);
    }
    randomize_file_list(gotNames, sum);
    printf("\n\n\n\n\n");
    for(g = 0; g < i; g++) {
        printf("%s\n",gotNames[g]);
    }
         
         
    return 0;
}

Clearly, the printf calls with *names (and names[b]) prior to proper initialisation of names were at fault all along. As Salem's signature reads: "If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut."

EDIT:
By the way, unlike john.c's approach, your approach is susceptible to a time-of-check, time-of-use error: if a new file is added right after you perform the count of the number of files and before you start recording the file names, your code will then have a buffer overflow when trying to record the file names since there would be one more file name to record than there was space allocated.

Originally Posted by poorboy

now your resorting to name calling? no it is not your insults are being reported.

It's not name calling. You are clearly extremely stupid. I was just "reporting" that fact.