35

I am teaching myself C. My goal is to make a C function that just walks a query string and splits on the ampersand and the equals sign. I am getting stuck on this error from Valgrind.

==5411== Invalid free() / delete / delete[] / realloc()
==5411==    at 0x402AC38: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5411==    by 0x804857C: main (leak.c:28)
==5411==  Address 0x420a02a is 2 bytes inside a block of size 8 free'd
==5411==    at 0x402AC38: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5411==    by 0x804857C: main (leak.c:28)
==5411== 
==5411== 
==5411== HEAP SUMMARY:
==5411==     in use at exit: 0 bytes in 0 blocks
==5411==   total heap usage: 1 allocs, 2 frees, 8 bytes allocated
==5411== 
==5411== All heap blocks were freed -- no leaks are possible
==5411== 
==5411== For counts of detected and suppressed errors, rerun with: -v
==5411== ERROR SUMMARY: 20 errors from 9 contexts (suppressed: 0 from 0)

and the backtrace :

*** Error in `./leak': free(): invalid pointer: 0x08c1d00a ***
======= Backtrace: =========
/lib/i386-linux-gnu/libc.so.6(+0x767c2)[0xb75f17c2]
/lib/i386-linux-gnu/libc.so.6(+0x77510)[0xb75f2510]
./leak[0x804857d]
/lib/i386-linux-gnu/libc.so.6(__libc_start_main+0xf5)[0xb7594905]
./leak[0x8048421]
======= Memory map: ========
08048000-08049000 r-xp 00000000 08:05 262764     /home/danny/dev/c-qs-parser/leak
08049000-0804a000 r--p 00000000 08:05 262764     /home/danny/dev/c-qs-parser/leak
0804a000-0804b000 rw-p 00001000 08:05 262764     /home/danny/dev/c-qs-parser/leak
08c1d000-08c3e000 rw-p 00000000 00:00 0          [heap]
b757a000-b757b000 rw-p 00000000 00:00 0 
b757b000-b7729000 r-xp 00000000 08:05 1312132    /lib/i386-linux-gnu/libc-2.17.so
b7729000-b772b000 r--p 001ae000 08:05 1312132    /lib/i386-linux-gnu/libc-2.17.so
b772b000-b772c000 rw-p 001b0000 08:05 1312132    /lib/i386-linux-gnu/libc-2.17.so
b772c000-b772f000 rw-p 00000000 00:00 0 
b772f000-b774a000 r-xp 00000000 08:05 1312589    /lib/i386-linux-gnu/libgcc_s.so.1
b774a000-b774b000 r--p 0001a000 08:05 1312589    /lib/i386-linux-gnu/libgcc_s.so.1
b774b000-b774c000 rw-p 0001b000 08:05 1312589    /lib/i386-linux-gnu/libgcc_s.so.1
b774c000-b7750000 rw-p 00000000 00:00 0 
b7750000-b7751000 r-xp 00000000 00:00 0          [vdso]
b7751000-b7771000 r-xp 00000000 08:05 1312116    /lib/i386-linux-gnu/ld-2.17.so
b7771000-b7772000 r--p 0001f000 08:05 1312116    /lib/i386-linux-gnu/ld-2.17.so
b7772000-b7773000 rw-p 00020000 08:05 1312116    /lib/i386-linux-gnu/ld-2.17.so
bfe93000-bfeb4000 rw-p 00000000 00:00 0          [stack]
Aborted (core dumped)

finally here is the code:

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

int main() {

    //char p[] = "t=quote&k=id&v=10";
    char p[] = "t=quote";

    char* token;
    char* tk; 
    char* s;
    unsigned short int found;

    s = strdup(p);

    if (s != NULL) {
        while ((token = strsep(&s, "&")) != NULL) {

            found = 0;

            printf("TOKEN: %s\n\n", token);

            while ((tk = strsep(&token, "=")) != NULL) {

                printf("TK: %s\n\n", tk);

                free(tk);
            }   

            free(token);
        }   
    }   

    free(s);

    return 0;
}

Thanks

chris
  • 60,560
  • 13
  • 143
  • 205
Dan
  • 2,209
  • 3
  • 23
  • 44
  • 1
    Thanks for your replies. When i take out the free(token) and free(tk). Valgrind reports i am leaking 8 bytes. When i put those back in. It reports that i am not leaking anymore but the program won't run. – Dan Nov 30 '13 at 07:08

3 Answers3

63

You're attempting to free something that isn't a pointer to a "freeable" memory address. Just because something is an address doesn't mean that you need to or should free it.

There are two main types of memory you seem to be confusing - stack memory and heap memory.

  • Stack memory lives in the live span of the function. It's temporary space for things that shouldn't grow too big. When you call the function main, it sets aside some memory for your variables you've declared (p,token, and so on).

  • Heap memory lives from when you malloc it to when you free it. You can use much more heap memory than you can stack memory. You also need to keep track of it - it's not easy like stack memory!

You have a few errors:

  • You're trying to free memory that's not heap memory. Don't do that.

  • You're trying to free the inside of a block of memory. When you have in fact allocated a block of memory, you can only free it from the pointer returned by malloc. That is to say, only from the beginning of the block. You can't free a portion of the block from the inside.

For your bit of code here, you probably want to find a way to copy relevant portion of memory to somewhere else...say another block of memory you've set aside. Or you can modify the original string if you want (hint: char value 0 is the null terminator and tells functions like printf to stop reading the string).

EDIT: The malloc function does allocate heap memory*.

"9.9.1 The malloc and free Functions

The C standard library provides an explicit allocator known as the malloc package. Programs allocate blocks from the heap by calling the malloc function."

~Computer Systems : A Programmer's Perspective, 2nd Edition, Bryant & O'Hallaron, 2011

EDIT 2: * The C standard does not, in fact, specify anything about the heap or the stack. However, for anyone learning on a relevant desktop/laptop machine, the distinction is probably unnecessary and confusing if anything, especially if you're learning about how your program is stored and executed. When you find yourself working on something like an AVR microcontroller as H2CO3 has, it is definitely worthwhile to note all the differences, which from my own experience with embedded systems, extend well past memory allocation.

GraphicsMuncher
  • 4,583
  • 4
  • 35
  • 50
  • 2
    "You're trying to free memory that's not heap memory." - it is unspecified whether `malloc()` (and co.) return pointers to "heap memory", "stack memory" or whatever. The important point is that it was not allocated by the standard allocator functions. –  Nov 30 '13 at 07:14
  • @H2CO3 Unnecessary copy? What's necessary? It's an exercise. Also, I've looked up malloc and it seems that it does, in fact, allocate heap memory. – GraphicsMuncher Nov 30 '13 at 07:48
  • @GrpahicsMuncher Unnecessary because the allocated memory isn't used outside the function. Just like the entire string itself isn't used outside of the function's scope. I don't know from where you got that idea about `malloc()`, but despite "the heap" being a common concept of modern memory architectures on the desktop, there are systems where `malloc()` does not use it (nor does the system have it at all), and (exactly for this reason) the standard isn't concerned with that either. The only correct phrasing of your (our) advice is "only `free()` what you allocated with the standard... –  Nov 30 '13 at 07:51
  • ...memory allocator functions" -- there's no need to mix in irrelevant concepts such as "the heap" and "the stack". –  Nov 30 '13 at 07:52
  • 2
    The unnecessary copy is, well, unnecessary and the terminology is difficult. On most systems, `malloc()` will indeed return "heap memory", but [the standard](http://web.archive.org/web/20050207010641/http://dev.unicals.com/papers/c99-draft.html#7.20.3.3) doesn't say a word about that, so there could indeed be systems which don't have a "heap". OTOH, on these systems, we could enhance the definition of "heap" to mean the memory area where `malloc()` takes its memory from, and all is fine again. – glglgl Nov 30 '13 at 08:17
  • 1
    This answer is misleading. @H2CO3's and qwrrty's are the correct ones. – Jens Gustedt Nov 30 '13 at 09:21
  • I've updated my answer. I think the distinction is an esoteric loophole in practice (especially for beginners learning on normal computers), although I cannot argue with it, even more so given H2CO3's examples. – GraphicsMuncher Nov 30 '13 at 18:16
  • I read the other day that you're only supposed to free malloc'd memory, and I'm 99% sure you can also free calloc'd memory, but I just want to be sure. Can you free calloc'd memory? – MarcusJ Mar 27 '17 at 09:54
11

From where did you get the idea that you need to free(token) and free(tk)? You don't. strsep() doesn't allocate memory, it only returns pointers inside the original string. Of course, those are not pointers allocated by malloc() (or similar), so free()ing them is undefined behavior. You only need to free(s) when you are done with the entire string.

Also note that you don't need dynamic memory allocation at all in your example. You can avoid strdup() and free() altogether by simply writing char *s = p;.

  • That worked. I took out the strdup call and Valgrind reported no more memory leaks. Thanks a lot. – Dan Nov 30 '13 at 07:12
  • @user964491 (and you're welcome, but *pretty please* read the documentation next time!) –  Nov 30 '13 at 07:13
  • Because the other one gave me more information and a more in depth explanation. In my opinion beginners like me would benefit from it. I appreciate your answer though. – Dan Nov 30 '13 at 07:18
  • @user964491 But note that the answer you accepted is **wrong** (as probably apparent from my comments on it)... –  Nov 30 '13 at 07:18
  • I don't know enough about C to discuss why it is wrong. All i know is that you all helped me out. Does it really matter who has more points if the end result is that you helped someone out? – Dan Nov 30 '13 at 07:24
  • 1
    @user964491 No, the only thing that matters is that you accepted an incorrect answer, and now every single visitor seeing it (and not experienced enough to differentiate between good and bad) will say that "hey, great, this is what I should do", but they shouldn't in fact, because they then will get used to fundamentally wrong practice and equally wrong terminology. So you will end up having done something really nasty to the community. (But that's your decision, and I can't just convince others to do their stuff this way or another, so here you go...) –  Nov 30 '13 at 07:26
  • 1
    I think they will get the idea if all 3 answers remain on this thread. – Dan Nov 30 '13 at 07:30
  • I also gave a correct answer and I'm not arguing with you, so I would be very happy if you accepted my answer. :-) In all seriousness, I wouldn't sweat it. The answer you accepted was wrong in one detail but was otherwise correct, the author edited it to correct it, and the ensuing discussion will make it clear for everyone. – Tim Pierce Nov 30 '13 at 13:32
  • @qwrrty (Just to make it clear: I am not OP.) Note that the author hasn't actually "corrected" his answer with his edit -- the quote from the book he is referring to is equally wrong in that "Programs allocate blocks from the heap by calling the malloc function", because nothing in the standard specifies that `malloc()` should return a pointer to heap memory (and in fact, I've programmed AVR microcontrollers, on platform which it is **not** the case, you just have to take a look at the standard library implementation). –  Nov 30 '13 at 13:37
3

You can't call free on the pointers returned from strsep. Those are not individually allocated strings, but just pointers into the string s that you've already allocated. When you're done with s altogether, you should free it, but you do not have to do that with the return values of strsep.

Tim Pierce
  • 5,514
  • 1
  • 15
  • 31