1

Why can't I read what is in a directory, it keeps on giving me segmentation faults?

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

int count(char* loc)
{
        int num = 0;
        char c;
        FILE *file = fopen(loc, "r");
        while( (c = fgetc(file)) != EOF) {
                if(c == '\n')
                        num++;
        }
        int r = fclose(file);
        return num;
}

int main()
{
        int lines = 0;
        int files = 0;
        char* names[files];
        int i = 0;
        DIR* d = opendir("./visual");    //You can change this bit

        struct dirent *file;
        while((file = readdir(d)) != NULL){
                i++;
                names[i] = file->d_name;
                files++;
                printf("%s\n", names[i]);
        }
        closedir(d);
        printf("__________\n");
        for(int i = 0;i < files;i++){
                printf("i = %d\n", i);

                lines = lines + count(names[i]);
        }
        printf("you have written %d lines of code", lines);
}
Yunnosch
  • 26,130
  • 9
  • 42
  • 54
Anonymous
  • 11
  • 1
  • Please learn about the less tedious and more readable ways of formatting code here. https://stackoverflow.com/editing-help – Yunnosch Feb 22 '20 at 10:50
  • 1
    Where exactly do you get the segfault? Please identify the line, e.g. by `/* segfault here */`.. – Yunnosch Feb 22 '20 at 10:55
  • How big do you think this `int files = 0; char* names[files];` is when you do `names[i] = file->d_name;` ? Especially for the first time. – Yunnosch Feb 22 '20 at 10:56
  • 1
    Even fixing that, you're still in for an awakening. `names[i] = file->d_name` will store off a pointer to memory that is neither guaranteed, nor even likely, to be static for the lifetime of the enumeration. It can/will be reused as you enumerate each file entry. And even if it didn't, all that memory is guaranteed to be off-limits once `closedir` is fired. If you want to retain the file names, you need to make *copies* of them; not just save off pointers. – WhozCraig Feb 22 '20 at 10:59
  • @WhozCraig Assuming you will not make an answer, can I add that to mine? (giving credit to you of course) – Yunnosch Feb 22 '20 at 11:01
  • @Yunnosch Sure. there's plenty of things wrong in this code. Write whatever you thinks is helpful. – WhozCraig Feb 22 '20 at 11:02
  • regarding: `int r = fclose(file);` this will cause the compiler to output a warning message about the variable 'r' that is set but never used. – user3629249 Feb 24 '20 at 03:50
  • regarding: `while( (c = fgetc(file)) != EOF) {` The variable `c` is declared as a `char`, but the function: `fgetc()` actually returns a `int`. also the comparison to EOF can fail, depending on if a `char` is signed or unsigned on your compiler – user3629249 Feb 24 '20 at 03:54
  • regarding: `FILE *file = fopen(loc, "r");` the function: `fopen()` can fail (for many reasons) so the returned value should be checked (!=NULL) to assure it was successful, if not successful, call `perror()` so both your error message AND the text reason the system thinks the error occurred are output to `stderr` – user3629249 Feb 24 '20 at 03:56
  • regarding: `DIR* d = opendir("./visual");` As noted above with `fopen()`, This function can easily fail, Therefore always check (!=NULL) the returned code, etc. – user3629249 Feb 24 '20 at 04:05
  • regarding: `int files = 0; char* names[files];` This is trying to declare an array of pointers that contains 0 entries. The result is a useless array. Probably trying to write to this array via `names[i] = file->d_name;` is what is causing the seg fault event. – user3629249 Feb 24 '20 at 04:08
  • [how to read a directory](https://stackoverflow.com/questions/4204666/how-to-list-files-in-a-directory-in-a-c-program) – user3629249 Feb 24 '20 at 04:16
  • Does this answer your question? [Reading current directory, printing results based on file attributes](https://stackoverflow.com/questions/51906202/reading-current-directory-printing-results-based-on-file-attributes) – user3629249 Feb 24 '20 at 04:19
  • OT: for ease of readability and understanding; 1)please use 4 spaces to indent code rather than tabs. 2) please separate code blocks: `for` `if` `else` `while` `do...while` `switch`, `case`, `default` via a single blank line – user3629249 Feb 24 '20 at 04:21
  • The posted code will fail when a sub directory is encountered and/or when a symbolic link is encountered, etc – user3629249 Feb 24 '20 at 04:24
  • note: regarding: `while((file = readdir(d)) != NULL){` after each call to `readdir()` the prior returned pointer will be invalid. – user3629249 Feb 24 '20 at 04:27
  • when reading the MAN page for `readdir()` you will see that the function is NOT thread safe. One side effect of this is that the returned pointer will always point to the same struct in memory. so at each call, the struct is overlayed with the new information. So you, if you want to keep info between calls to `readdir()` then that information must be copied to a local struct. – user3629249 Feb 24 '20 at 04:33

1 Answers1

1

Here you define an array of size 0.

int files = 0;
char* names[files];

Here (while i and files are both 0) you access the first of those zero (note the conflict here?) elements in the array.

names[i] = file->d_name;

Then you increase files.

files++;

This does however not change the size of the array, and even if it would it would be too late.

Going on, I will quote WhozCraigs helpful comment (with permission):

Even fixing that, you're still in for an awakening. names[i] = file->d_name will store off a pointer to memory that is neither guaranteed, nor even likely, to be static for the lifetime of the enumeration.
It can/will be reused as you enumerate each file entry. And even if it didn't, all that memory is guaranteed to be off-limits once closedir is fired.
If you want to retain the file names, you need to make copies of them; not just save off pointers.

End of quote.

Yunnosch
  • 26,130
  • 9
  • 42
  • 54
  • If you have `strdup()` it makes an easy way to handle `names[i] = strdup(file->d_name);` – David C. Rankin Feb 22 '20 at 11:35
  • @DavidC.Rankin Thanks. I'd add that to the answer, if I was not too scared to try fixing that tangle of code as posted by OP. Maybe if more input arrives I get the impression that a clean and simple enough complete solution is found. For now yours is definitly a step in the right direction. – Yunnosch Feb 22 '20 at 11:39