Freeing memory after calling strtok() causes an error

Question

Could you please help me? My code does tokenizing, so I created code like this:

I allocate some memory,
I strcpy(malloced_memory, argv)
I execute strtok(mallocted_memory, ".")

Try free(mallocted_memory).

filename = malloc(strlen(argv));

strcpy(filename, argv);
strk_ptr = malloc(sizeof(filename));
strk_ptr = strtok(filename,".");//
i++;
sprintf(in->file_name,"%s",strk_ptr);

while(strk_ptr = strtok(NULL,"."))//
{
    i++;
    sprintf(in->file_name,"%s.%s",in->file_name,strk_ptr);
    sprintf(in->file_ext ,"%s",strk_ptr);
}
free(strk_ptr);
free(filename);

That code has the problem that I can't free(filename). If I try free(filename), then program get SIGTRAP. But program is working.

I want fix that problem. What should I do?

John Hascall · Answer 1 · 2016-01-13T09:15:46.270

This line:

filename = malloc(sizeof(argv));

should be this:

filename = malloc(strlen(argv) + 1);     /* +1 for the '\0' at the end */
if (filename == NULL) { /* take some action */ }

And this line:

strk_ptr = malloc(sizeof(filename));

is only creating a memory leak since it is followed by:

strk_ptr = strtok(filename,".");

And you should check the return value:

strk_ptr = strtok(filename,".");
if (strk_ptr == NULL) { /* take some action */ }

BTW, the strtok() function returns a pointer to a token inside the string passed in the initial call to it (filename in your example). It does not allocate memory, so its return value should NOT be freed (which your program avoids, but it's a common mistake). While I'm grousing about strtok(), I'll mention that you can't (directly or indirectly) pass it a literal string to tokenize, since it modifies the string and literal strings are readonly. That is, doing: strtok("sample.txt", ".") is a no-go.

And finally, this kind of implicit condition is not great form:

while (strk_ptr = strtok(NULL,".")) { ... }

Better is:

while ((strk_ptr = strtok(NULL,".")) != NULL) { ... }

they aren't asking about freeing strtok memory, they are asking about freeing their memory after using strtok. — Keith Nicholas, Jan 13 '16 at 08:51

terence hill · Answer 2 · 2016-01-13T09:50:53.123

You don't need to allocate memory when using strtok()

There is no problem in freeing filename as it is correctly allocated by malloc(), however there are many other problems and memory leaks. Basically you first allocate memory for str_ptr:

strk_ptr = malloc(sizeof(filename));

Here malloc() return a pointer which is stored in strk_ptr. And then you call strtok() which also return a pointer, inside filename:

strk_ptr = strtok(filename,".");

So you lost the original pointer returned by malloc() and now strk_ptr points somewhere in filename. When you call free(str_ptr) you are freeing a memory inside filename. The subsequent call to free(filename) report the error. The solution is simply that don't need to allocate memory for strk_ptr.

I wrote a working minimal code to show you how to correctly use strtok. Please remember that, when asking a question, posting a minimal working code is alway better.

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

    char *strk_ptr;
    char *filename = malloc(strlen(argv[0]) + 1);

    strcpy(filename, argv[0]);

    printf("filename = %s, size = %zu\n", filename, sizeof(filename));

    // Do not malloc this
    //strk_ptr = malloc(strlen(filename) + 1);
    strk_ptr = strtok(filename,".");//
    printf("%s\n", strk_ptr);

    while( (strk_ptr = strtok(NULL,".")) )
    {
        printf("%s\n", strk_ptr);
    }
    free(filename);

    return 0;
}

First of all argv is a char** so if you want to copy the content of the first argument passed as input you have to use argv[0], which is always the executable file name.

then, sizeof(filename) returns the size of the pointer not the size of the content as filename is not an array. you have to use strlen(filename) + 1.

strtok return a pointer inside the object (filename) which is already allocated so you don't need to allocate memory for strk_ptr.

When using strtok in a loop consider to take the following approach:

   for (strk_ptr = strtok(filename, "."); strk_ptr; strk_ptr = strtok(NULL, "."))
    {
        printf("%s\n", strk_ptr);
    }

score 1 · Answer 3 · answered Jan 13 '16 at 08:46

 filename = malloc(strlen(argv));
 strk_ptr = malloc(sizeof(filename));

strk_ptr gets some memory, which you then go leave dangling by pointing strk_ptr to filenames memory, you then end up double freeing filename.

So don't malloc strk_ptr. Just leave it as char* then only free filename at the end

JimmyB · Answer 4 · 2016-01-13T09:08:59.063

strk_ptr = malloc(sizeof(filename));
strk_ptr = strtok(filename,".");//
...
free(strk_ptr);

Doesn't work. At first, strk_ptr points to the malloc'd memory but then the pointer is immediately overwritten with some other value, so basically you lose the pointer to the malloc'd memory and hence cannot free that memory any more.

Edit:

Seeing malloc(sizeof(filename)), I should add that you do not have to allocate memory for the pointer variable itself. The declaration char* strk_ptr; makes the compiler implicitly allocate memory for that pointer (i.e. 4 or 8 bytes). So you can just use the pointer directly like any other variable, and you won't have to free that variable's memory.

char* strk_ptr;
strk_ptr = strtok(filename,".");

Or, if that wasn't your intention then note that sizeof(filename) does not return the length of the string, but just the size of the pointer variable filename, i.e. usually 4 or 8, independent of what string filename points to. See also http://www.gnu.org/software/libc/manual/html_node/String-Length.html:

char string[32] = "hello, world";
char *ptr = string;
sizeof (string)
    ⇒ 32
sizeof (ptr)
    ⇒ 4  /* (on a machine with 4 byte pointers) */

Freeing memory after calling strtok() causes an error

4 Answers4