Reading file line by line with fgets in C, invalid read of size

Question

I keep getting a valgrind error in my code, and after three hours I remain clueless so I need your help people.

So I basically just read files contained in a directory and parse them, So I copied the shortest example of my code still producing the error:

int main(int argc, char** argv) {

 parse_files_dir("/Users/link_to_dir_example/");
 return (EXIT_SUCCESS);
}

void parse_files_dir(char *dirLink){

int dLink_l =strlen(dirLink);
int max_len = dLink_l*2;

char* full_path=malloc(sizeof(char)*(max_len+1));
//check if null pointer...

strncpy(full_path, dirLink, dLink_l);

DIR *dir;
struct dirent *dir_con;
dir=opendir(dirLink);

if (dir == NULL){
    fprintf(stderr, "Problem opening directory: \"%s\". Aborting...\n", dirLink);
    exit(EXIT_FAILURE);
}

while((dir_con = readdir(dir)) != NULL){

    if (dir_con->d_name[0] == '.') continue;

    if (dLink_l+strlen(dir_con->d_name)>max_len) //realloc full path..

    strncpy(&full_path[dLink_l], dir_con->d_name, strlen(dir_con->d_name));
    full_path[dLink_l+strlen(dir_con->d_name)] = '\0';        

    parse_one_file(full_path);  // (*) <=== valgrind complain 
    full_path[dLink_l] = '\0';
} 
free(full_path);
closedir(dir);
}

So now the actual problem method:

void parse_one_file(char* link) {

FILE *file = fopen(link, "r");   
if (file == NULL) //error message

int line_len=0;
int line_max=1000;
char* line= malloc(sizeof(char)*line_max);
line[0] = '\0';

char* line_full= malloc(sizeof(char)*line_max);
line_full[0] = '\0';
int line_full_len = 0;

//check all allocations for null pointers

while(fgets(line, line_max, file) != NULL){   // <=== Here is where valgrind complains !!!!

    line_len = strlen(line);
    if (line[line_len-1] == '\n'){

            strncpy(&line_full[line_full_len], line, line_len);
            line_full_len+=line_len;
    }
    else{

        //Check if line_full has enough memory left   
        strncpy(&line_full[line_full_len], line, line_len);
        line_full_len+=line_len;
    }
    line[0] = '\0';
}
free(line);
free(line_full);
fclose(file);
}

I keep getting the error:

 ==4929== Invalid read of size 32
 ==4929==    at 0x1003DDC1D: _platform_memchr$VARIANT$Haswell (in       /usr/lib/system/libsystem_platform.dylib)
 ==4929==    by 0x1001CF66A: fgets (in /usr/lib/system/libsystem_c.dylib)
 ==4929==    by 0x100000CD8: parse_one_file (main.c:93)
 ==4929==    by 0x100000B74: parse_files_dir (main.c:67)
 ==4929==    by 0x100000944: main (main.c:28)
 ==4929==  Address 0x100804dc0 is 32 bytes before a block of size 4,096 in arena "client"

So i really dont see where my mistake is, I keep emptying the buffer line, I never read more bytes than allocated there.

The interesting thing I noticed is, if the directory "dirLink" has only one file, the error does not occur, however if I have two or more, the error occurs, so I thought the mistake is how I generate the path "full_path", but then I replaced line "*" with (just for testing reasons)

   parse_one_file("another/example/path/");

and the error remained..

How are you doing `//Check if line_full has enough memory left` ? — David C. Rankin, Sep 04 '15 at 16:16
the calculation of `dLink_l` should be: `int dLink_l =strlen(dirLink)+1;` where the '+1' is to include the NUL byte at the end of the string. — user3629249, Sep 05 '15 at 11:23
in the call to malloc(), the expression: `sizeof(char)` is defined as 1. That expression has absolutely not effect on the parameter passed to malloc() and clutters the code. suggest removing that expression. Always check (!=NULL) the returned value from malloc() to assure the operation is successful — user3629249, Sep 05 '15 at 11:26
when a call to a system function fails, then immediately (not several lines of code later) call `perror()` so your error message AND the system error message, as selected by `errno` are output to stderr — user3629249, Sep 05 '15 at 11:29
regarding this kind of line: `if (file == NULL) //error message` beside outputing the error message, also call return or exit() as the file is not open so cannot be read from — user3629249, Sep 05 '15 at 11:30
please, for readability and ease of understanding by us humans, consistently indent the code. Suggest 4 space indents as that is wide enough to be seen, even when using variable width fonts. Suggest indenting after every opening brace '{' and un-indenting before every closing brace '}'. Never use tabs as each word processor/editor has the tab stops/tab widths set differently. — user3629249, Sep 05 '15 at 11:33
regarding: `FILE *file` It is very poor programming practice and often leads to misunderstandings and makes debugging and program maintenance much more difficult to have variable names that are nothing but a capitalization change from a C keyword or type name — user3629249, Sep 05 '15 at 11:36
suggest setting a variable to the returned value from `fgets()` and calling realloc() to expand the length of 'line-full' with each new line read from the file — user3629249, Sep 05 '15 at 11:40
when posting a question about a runtime problem, please post code that (by itself) cleanly compiles. That means the #include statements have to be posted, so we do not have to guess as to which ones are needed and when compiling with all warnings enabled, the compiler does not raise any warning messages. — user3629249, Sep 05 '15 at 11:44

score 2 · Answer 1 · answered Sep 04 '15 at 15:53

2

Unless your file is less than 1000 bytes in total you are writing over the end of the line_full buffer which is only 1000 bytes total in size. This will invariably clobber your memory and lead to spurious errors like the one you experience in fgets.

answered Sep 04 '15 at 15:53

Markus Dheus

576
2
8

score 1 · Answer 2 · answered Sep 04 '15 at 15:38

1

if(line[line_len-1] == '\n'){
  strncpy(&line_full[line_full_len], line, line_len);
  line_full_len+=line_len;
}

This is not quite correct, you can only strncpy() (line_max - line_full_len) bytes, there is no guarantee that you can copy line_len bytes. Or in other words. starting from position line_full[500], you can't write another 1000 bytes.

The same error is in the else branch.

answered Sep 04 '15 at 15:38

John Hammond

467
1
4
14

I know that, I just left out that part by writing "//Check if line_full has enough memory left " – malajedala Sep 07 '15 at 12:23

Reading file line by line with fgets in C, invalid read of size

2 Answers2