I don't understand why I'm definitely losing blocks of memory

Question

As a project I'm creating a student database. However according to valgrind there's memory leak in my program and I don't know why. I can't really say much more: I don't understand why memory is being definitely lost.

Student structure:

typedef struct {
    char student_number[7];
    char *first_name;
    char *last_name;
    int round_points[6];
} Student;

Disclaimer: I'm using the gcc option -std=c99, so I had to implement my own strdup().

Important pieces of code:

char *copy_string(const char *string) {
    int len = strlen(string);
    char *copy = calloc(len + 1, sizeof(char));
    if (copy == NULL)
        return NULL;
    strcpy(copy, string);
    /* copy[len] = '\0'; */
    return copy;
}

char **parse_one_line_params(const char *one_line, int param_count) {
    char *copy = copy_string(one_line);
    if (copy == NULL)
        return NULL;
    //copy_start is used to free the copy string in the end
    char *copy_start = copy;
    //It is assumed that one_line is of the form COMMAND|SPACE|ARGUMENTS
    //Move pointer to the first important character
    copy += 2;
    const char *separator = " ";
    char **content = malloc(sizeof(char *) * param_count);
    if (content == NULL)
        return NULL;
    int occurrences = 0;
    char *delimiter_start;
    while ((delimiter_start = strstr(copy, separator)) != NULL) {
        delimiter_start[0] = '\0';
        char *sub_string = copy_string(copy);
        if (sub_string == NULL)
            return NULL;
        if (sub_string[0] != '\0') {
            content[occurrences] = sub_string;
        }
        //Since separator is of the length of one
        copy = delimiter_start + 1;
        occurrences++;
    }
    //param n - 1 will be assigned from the last portion of copy
    if (occurrences != param_count - 1)
        return NULL;
    int last_len = strlen(copy);
    if (last_len > 0 && copy[last_len - 1] == '\n')
        copy[last_len - 1] = '\0';
    content[occurrences] = copy_string(copy);
    free(copy_start);
    return content;
}

char **deliver_payload(const char *one_line, int param_count) {
    if (one_line[1] != ' ') {
        printf("Command was longer than one character.\nPlease see manual for instructions\n");
        return NULL;
    }
    char **payload = parse_one_line_params(one_line, param_count);
    if (payload == NULL) {
        printf("Invalid arguments for given command\n");
        return NULL;
    }
    return payload;
}

The error in question is: (records 1 to 5 are the same as below)

==15== 32 bytes in 4 blocks are definitely lost in loss record 5 of 7
==15==    at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15==    by 0x400ECA: copy_string (projekti.c:80)
==15==    by 0x400F81: parse_one_line_params (projekti.c:106)
==15==    by 0x4010B6: deliver_payload (projekti.c:133)
==15==    by 0x4019A2: main (projekti.c:301)

Line 301 is just:

char **payload = deliver_payload(one_line, 3);

Edited version of the code:

char **parse_one_line_params(const char *one_line, int param_count) {
    char *copy = copy_string(one_line);
    if (copy == NULL)
        return NULL;
    //copy_start is used to free the copy string in the end
    char *copy_start = copy;
    //It is assumed that one_line is of the form COMMAND SPACE ARGUMENTS
    //Move pointer to the first important character
    copy += 2;
    const char *separator = " ";
    char **content = malloc(sizeof(char *) * param_count);
    if (content == NULL) {
        free(copy_start);
        return NULL;
    }
    int occurrences = 0;
    char *delimiter_start;
    while ((delimiter_start = strstr(copy, separator)) != NULL) {
        delimiter_start[0] = '\0';
        char *sub_string = copy_string(copy);
        if (sub_string == NULL) {
            for (int i = 0; i < occurrences; i++) {
                free(content[i]);
            }
            free(copy_start);
            return NULL;
        }
        if (sub_string[0] != '\0') {
            int sub_len = strlen(sub_string);
            content[occurrences] = calloc(sub_len + 1, sizeof(char));
            strncpy(content[occurrences], sub_string, sub_len);
            free(sub_string);
        }
        //Since separator is of the length of one
        copy = delimiter_start + 1;
        occurrences++;
    }
    //param n - 1 will be assigned from the last portion of copy
     if (occurrences != param_count - 1) {
        printf("Too few parameters\nAborting\n");
        if (occurrences > 0) {
            for (int i = 0; i < occurrences; i++) {
                free(content[i]);
            }
        }
        free(content);
        return NULL;
    }
    int last_len = strlen(copy);
    if (last_len > 0 && copy[last_len - 1] == '\n')
        copy[last_len - 1] = '\0';
    content[occurrences] = calloc(last_len + 1, sizeof(char));
    strncpy(content[occurrences], copy, last_len);
    /* content[occurrences] = copy_string(copy); */
    free(copy_start);
    return content;
}

Edit valgrind: (in the full output the problematic lines are 140 and 118 i.e. the callocs)

==15== 13 bytes in 2 blocks are definitely lost in loss record 3 of 7
==15==    at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15==    by 0x4011B0: parse_one_line_params (projekti.c:140)
==15==    by 0x401236: deliver_payload (projekti.c:153)
==15==    by 0x401B22: main (projekti.c:321)
==15== 
==15== 27 bytes in 6 blocks are definitely lost in loss record 4 of 7
==15==    at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==15==    by 0x401075: parse_one_line_params (projekti.c:118)
==15==    by 0x401236: deliver_payload (projekti.c:153)
==15==    by 0x401BBE: main (projekti.c:337)

Last edit: I solved this particular problem: I forgot to free the parameter list in the main function so in the end, I was barking the wrong tree.

Answer 1

You have a memory leak here

char **parse_one_line_params(const char *one_line, int param_count){
    char *copy = copy_string(one_line);
    if (copy == NULL)
        return NULL;

    /* ... */

    char **content = malloc(sizeof(char *) * param_count);
    if (content == NULL)
        return NULL;

If malloc returns NULL for content, you return from the function without freeing the memory of copy. malloc is extremely unlikely to fail, but if the rest of your code is written in the same pattern, then this is where the leaks come from.

It doesn't suffice to "balance" each malloc/strdup/calloc with a free. You must keep track of where you allocate memory and free it in case the last pointer you have access to it goes out of scope or is overwritten (thanks @Ctx).

Answer 2

In the end the fault was in main function where I forgot to free the newly created parameter list. Thanks for @GSerg and @datenwolf for pointing out other mistakes in my code.