1

I have a structure which contains some elements, i free the memory of this structure in a loop, roughly like:

for (i = 0; i < teller; i++) {
   free((glycan+i)->desc);
}
free(glycan)

I assume that the pointers are still pointing to the empty memory blocks, therefore i wanted to set them to NULL as follows:

for (i = teller; i > 0; i--) {
    (glycan+i)->desc = NULL;
}
glycan = NULL;

Valgrind however tells me something which i don't really understand:

==11783== Invalid write of size 4
==11783==    at 0x8048F49: main (spectral_matcher.c:122)
==11783==  Address 0x431c070 is 72 bytes inside a block of size 28,000 free'd
==11783==    at 0x4027C02: free (vg_replace_malloc.c:366)
==11783==    by 0x8048F2C: main (spectral_matcher.c:121)

Can anyone explain to me why this warning/error occurs and what i should do differently not to solve it?

EDIT: I am aware that i am setting the pointer to NULL after freeing, freeing only marks the memory as free so the pointer is still intact (if i'm not mistaking) which i subsequently wish to set to NULL.

Bas Jansen
  • 3,273
  • 5
  • 30
  • 66

3 Answers3

7

Once you free the variable glycan you can't touch (glycan+i)->desc anymore - nor does it make sense.

About the sense part: just think of it, if you say glycan = NULL, why would you care for the individual elements' members ?

cnicutar
  • 178,505
  • 25
  • 365
  • 392
  • I always understood that free only clears the values within the memory block but that the pointer than points to an empty memory block, am i wrong on that? – Bas Jansen Jan 02 '12 at 15:09
  • 1
    @BasJansen Actually you got it wrong. Free marks the memory as "free" but on most implementations it doesn't actually change its contents. So accessing "freed" memory is always undefined behavior. – cnicutar Jan 02 '12 at 15:10
  • *nods*, but the pointer is still pointing to that element yes? Shouldn't i be 'cleaning' the pointers with NULL statements or is there a better way to do that? – Bas Jansen Jan 02 '12 at 15:12
  • @BasJansen After you free it it points to the same memory, but that memory isn't yours anymore. It's like the number on your locker. 34 used to be yours but now it might belong to someone else. You shouldn't be setting things to NULL after freeing them. It's a questionable practice at best. – cnicutar Jan 02 '12 at 15:14
  • 2
    If you set anything to `NULL`, you can set `glycan` to `NULL`, but anything pointed to by `glycan` is no longer yours and you should not touch it. (Note that setting `glycan` to `NULL` really does nothing to help you except perhaps help you catch invalid accesses later.) – Dark Falcon Jan 02 '12 at 15:17
  • @cnicutar Hmm, won't i possibly run into trouble if i intend to use the same structure and elements but allocating the elements as doubles (instead of floats) if i don't NULL the pointer? If not, then i was worrying for no reason – Bas Jansen Jan 02 '12 at 15:18
  • @Dark Falcon that is exactly why i wanted to set it to NULL yes. I also think i see the problem because of what you said, i don't own the memory space of glycans elements anymore so i can't set them to NULL but i can set glycan to NULL which should keep Valgrind happy and prevent me from getting weird bugs later on :) – Bas Jansen Jan 02 '12 at 15:19
  • I thought of something as i was in bed (sad, i know). I understand that the memory block where the pointer points to is no longer mine but the memory block that is the pointer still is mine right? Setting the pointer to something shouldn't touch the memory that i don't own anymore yes, or am i missing something? – Bas Jansen Jan 03 '12 at 09:56
  • 1
    @BasJansen It's not sad. Yes, the pointer itself is yours so you may make it point to anything, it's perfectly legal. BUT, `(glycan+i)->desc` **changes the memory formerly pointed** by `glycan`, which you freed. – cnicutar Jan 03 '12 at 12:58
  • @cnicutar I defined desc inside the glycan structure as a char*, using (glycan+i)->desc should then refer to the pointer and not the memory element, yes? – Bas Jansen Jan 03 '12 at 13:27
  • 1
    @BasJansen Yes, but where is the actual `desc` stored ? Inside the memory pointed by `glycan`. – cnicutar Jan 03 '12 at 13:28
  • @cnicutar Ahh... if i could i would post an image of Homer doing the famous 'Doh' now. I altered my code so that it frees all the elements of glycan, sets the pointers of the elements to NULL, frees glycan and lastly set glycan to NULL. This makes Valgrind and me happy ;) Thanks a ton cnicutar – Bas Jansen Jan 03 '12 at 13:37
2

The problem was you were de-referencing a free()'ed pointer to set that to NULL.

Now, try this!

for (i = 0; i < teller; i++) {
    free((glycan+i)->desc);
    (glycan+i)->desc = NULL;
}

free(glycan)
glycan = NULL;

Setting it to NULL make sense if you are going to re-use the same variable to malloc() some memory and don't want the program to crash if some module access that without checking for NULL

Sangeeth Saravanaraj
  • 16,027
  • 21
  • 69
  • 98
1

You are setting (glycan+i)->desc to NULL after glycan has been freed.

Alessandro Pezzato
  • 8,603
  • 5
  • 45
  • 63