I have a problem with a memory leak with temp_line.

This function read a whole text from a file and assign each word to a big linked list. I cant figure out who's the owner of temp_line whenever I exit this function, and whenever I am trying to replace temp_line with non-dynamic variable (like temp_line[1000]) every time I get a new line, its overwritten the data from the old line (and then I am getting a segmentation fault because of that).

So I really don't know how to solve it.

int fill(FILE *f, LinkedList *linkedlist) {
    FILE *file = fopen((const char *)f, "r");
    char line[MAX_LINE];   //MAX_LINE = 1000
    while (fgets(line, MAX_LINE, file) != NULL) {
        char *temp_line = malloc(MAX_LINE);
        if (temp_line == NULL) {
            fclose(file);
            return false;
        }
        strcpy(temp_line, line);
        read_line(linkedlist, temp_line);
    }
    fclose(file);
    return true;
}

int read_line(LinkedList *linkedlist, char *line) {
    char *word;
    while (true) {
        word = strtok(NULL, "\n ");
        if (word == NULL) {
            break;
        }
        add_node(linkedlist, word);
    }
    return true;
}

add_node adds the node to the linked list and returns the node.

CodePudding user response：

There are multiple problems in the code:

• FILE *file = fopen((const char *)f, "r"); with f defined as FILE *f is probably incorrect. If you are given a FILE *, just read from it.

• strtok should be first called with the string, then with NULL until it returns NULL.

• you insert nodes into the list from pointers into the middle of the block allocated by read_line. There is no way to determine the beginning of such a block, nor any way to determine how many pointers point into the block. Memory allocated and used this way cannot be freed. You should instead use strdup() to allocate individual copies of the words. These pointers can be later freed with free() in the del_node() function. Alternately, add_node() could make a copy of the string argument, which would be consistent with del_node() freeing this data.

• regarding your question: Cannot find who is the owner of a malloc block... indeed it is the C programmers' responsibility to keep track of allocated memory. There is no way to test if a pointer is valid, not whether it points to an allocated block. You must design the program consistently so memory ownership can always be determined from context.

Here is a modified version:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int read_line(LinkedList *linkedlist, char *line) {
    char *word;
    while ((word = strtok(line, "\n ")) != NULL) {
        add_node(linkedlist, strdup(word));
        line = NULL;
    }
    return true;
}

int fill(FILE *file, LinkedList *linkedlist) {
    char line[MAX_LINE];
    while (fgets(line, sizeof line, file) != NULL) {
        read_line(linkedlist, line);
    }
    return true;
}

CodePudding user response：

I am trying to replace temp_line with non-dynamic variable (like temp_line[1000])

temp_line[1000] is also called "automatic memory" because it is automatically deallocated at the end of its scope. What this means is temp_line would be deallocated on each iteration of the loop.

Perhaps the important thing to realize is strtok does not allocate any memory. word points at memory inside temp_line.

Let's say you did.

while (fgets (line, MAX_LINE, file) != NULL) {
  char temp_line[MAX_LINE];
  strcpy(temp_line, line);
  read_line(linkedlist, temp_line);
}

On each loop temp_line will be allocated, passed into read_line, and then deallocated. The words you add to the linked list point at deallocated memory.

The question is who "owns" memory. Because it uses strtok, read_line assumes that it owns line and that line will not be deallocated. The safe thing to do is to change read_line so it copies each word from line.

int read_line(LinkedList *linkedlist, char *line) {
    for(
      char *word = strtok(line, "\n ");
      word;
      word = strtok(NULL, "\n ")
    ) {
        // Duplicate the word so it no longer refers to line.
        add_node(linkedlist, strdup(word));
    }
    return true;
}