How to free this strdup?

Question

I am using strdup to duplicated the value inside command. I also free it at the end of my loop to make sure I don't have anything leaking but valgrind seems to disagree with me and says that what is allocated with this strdup is leaking. Any ideas?

Here is my code:

int main(void)
{
init_ui();
hist_init(100);

char *command;
while (true) {
    signal(SIGINT, SIG_IGN);
    command = read_command();
    if (command == NULL) {
        break;
    }
    char *copy = strdup(command);
    char *args[4096];
    int tokens = 0;
    char *next_tok = command;
    char *curr_tok;
    while((curr_tok = next_token(&next_tok, " \t\n\r")) != NULL) {
        if(strncmp(curr_tok, "#", 1) == 0){
            break;
        }
        args[tokens++] = curr_tok;
    }
    args[tokens] = NULL;

    if(args[0] == NULL) {
        continue;
    }
     
    hist_add(copy);
    int builtin_status = handle_builtins(tokens, args);
    if(builtin_status == 0) {
        continue;
    }

    pid_t child = fork();
    if(child == -1){
        perror("fork");
    }
    else if(child == 0){
        int ret = execvp(args[0], args);
        if(ret == -1) {
            perror("execvp");
        }
        close(fileno(stdin));
        close(fileno(stdout));
        close(fileno(stderr));
        exit(EXIT_FAILURE);
    }
    else {
        int status;
        waitpid(child, &status, 0);
        set_last_status(status);
    }
hist_destroy();
free(copy);
}
return 0;
}

Here is what valgrind gives me, I really am trying to understand what is wrong because it seems like what is defined with this strdup is freed:

HEAP SUMMARY:
==359074== in use at exit: 18 bytes in 2 blocks
==359074== total heap usage: 72 allocs, 70 frees, 20,000 bytes allocated
==359074==
==359074== 18 bytes in 2 blocks are definitely lost in loss record 1 of 1
==359074== at 0x483977F: malloc (vg_replace_malloc.c:307)
==359074== by 0x4A7D23E: strdup (in /usr/lib/libc-2.31.so)
==359074== by 0x10A703: main (shell.c:85)
==359074==
==359074== LEAK SUMMARY:
==359074== definitely lost: 18 bytes in 2 blocks
==359074== indirectly lost: 0 bytes in 0 blocks
==359074== possibly lost: 0 bytes in 0 blocks
==359074== still reachable: 0 bytes in 0 blocks
==359074== suppressed: 0 bytes in 0 blocks

There's a bunch of `continue;`s before you `free(copy);` - those would obviously leak the memory — UnholySheep, Oct 24 '20 at 18:48
Where do you `free()` the memory that was allocated with `command = read_command();` which was then duplicated to `*copy`? — Weather Vane, Oct 24 '20 at 18:49
Perhaps it pointed to a `static` array in `read_command();` which cannot be `free()`d. The problems of posting incomplete code... — Weather Vane, Oct 24 '20 at 18:51
If you want to do it right, *every* allocation must eventually be paired with a release. This is best done by implicitly tracking ownership, as in you must keep track of your responsibility at any given point in the code: You either release memory, or you pass ownership on to another part of your code. — tadman, Oct 24 '20 at 18:52
Okay! I freed after every continue and it now works. I totally got where the code was wrong... just silly me. Thanks guys! — Yohan, Oct 24 '20 at 18:57

score 1 · Accepted Answer · answered Oct 24 '20 at 22:10

strdup() uses malloc() to allocate memory, so in order to reuse strdup(), copy must be freed.

As UnholySheep commented above, continues causing to ignore the free() statement. One solution to fix this issue is to put an extra free() statement before each continue.

How to free this strdup?

1 Answers1