PDA

View Full Version : [Resolved] down the rabbit hole with pointers




MrFusion
Apr 24, 2011, 07:21 AM
I recently realized the existence of void pointers. These are quite useful if the primitive type for a data storage class is not known. Though, I have a question as to whether I am using them correctly.
My storage class will allocate memory and keep track of the byte size of the individual pieces. The user then set and extract bytes via void * pointers.
The storage class then copies bytes to and from the storage memory as needed. Basically my own crude implementation of NSMutableArray/NSMutableData.
The user gets back a void * pointer and then casts this into whatever is needed. Yes, probably too much responsibility is given to the user.
Anyway, I am concerned about memory leaks. To return a value, I memcpy() the bytes into a newly created memory malloc(). But there is no corresponding free().
The user has to free the memory. But calling free after casting and dereferencing causes a crash. Does memory have to be freed in this case? See last for loop.
Thanks for reading, replying and hopefully a lesson in pointers.


int main (int argc, const char * argv[]) {
NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];

NSLog(@"Hello, World!");
//create memory for storage
int numberOfElements = 10;
int byteSizeOfElement = sizeof(double*); // or float, int, BOOL, ... determined by user, not storage class
void *storage = calloc(numberOfElements, byteSizeOfElement);

//set values
for (int i = 0; i < numberOfElements; i++)
{
double element = i*4.5;
memcpy(storage+i*byteSizeOfElement,&element,byteSizeOfElement);
}

//read out values
for (int i = 0; i < numberOfElements; i++)
{
void *pointerGivenByInstanceFunction = memcpy(malloc(byteSizeOfElement),storage+i*byteSizeOfElement,byteSizeOfElement);
double returnElement = *(double *)pointerGivenByInstanceFunction; // or float, int, BOOL, ...
NSLog(@"%g",returnElement);
free(pointerGivenByInstanceFunction);
}
NSLog(@"Down the rabbit hole");
//read out values bis
for (int i = 0; i < numberOfElements; i++)
{
//get element in one line as in: double returnElement = *(double *)[storageInstance elementAtIndex:i];
double returnElement = *(double *)memcpy(malloc(byteSizeOfElement),storage+i*byteSizeOfElement,byteSizeOfElement); //memory leak? Function that (will eventually) return void* pointer has no corresponding free()
NSLog(@"%g",returnElement);
free(&returnElement); //causes crash
}
NSLog(@"bye, World!");
[pool drain];
return 0;
}



robbieduncan
Apr 24, 2011, 07:28 AM
Basically you are freeing memory you did not malloc/calloc. Your double returnElement within the last loop is requesting space to store a double. This will be on the stack. You are not mallocing or callocing the space for this double (if you did it'd be in the heap). You then memcpy into this space (which is fine: you are allowed to do that). You then free the space that is on the stack. You can't do that. You can't free on the stack. That is automatically done when the current stack frame is popped.

If that doesn't make sense (if you don't know what the heap and stack are for example) it's time to learn basic C properly...

jiminaus
Apr 24, 2011, 07:51 AM
Basically you are freeing memory you did not malloc/calloc.

Actually there is a malloc, it's embedded in the memcpy.


void *pointerGivenByInstanceFunction = memcpy(malloc(byteSizeOfElement),storage+i*byteSizeOfElement,byteSizeOfElement);


EDIT: Sorry ignore that. It's the "read out values bis" loop that's the problem.

Yes it's as robbieduncan says. In your one-line version, you're losing the pointer the copy of the double on the heap. Then later you try to pass the address of the copy on the stack to free, which rightly causes a crash.

Basically you can't do this in one line. You could, though, cast directly to a double pointer and still free it, like so:

double *pointerGivenByInstanceFunction = (double *)memcpy(malloc(byteSizeOfElement),storage+i*byteSizeOfElement,byteSizeOfElement);
double returnElement = *pointerGivenByInstanceFunction;
NSLog(@"%g",returnElement);
free(pointerGivenByInstanceFunction);

MrFusion
Apr 24, 2011, 07:52 AM
Basically you are freeing memory you did not malloc/calloc. Your double returnElement within the last loop is requesting space to store a double. This will be on the stack. You are not mallocing or callocing the space for this double (if you did it'd be in the heap). You then memcpy into this space (which is fine: you are allowed to do that). You then free the space that is on the stack. You can't do that. You can't free on the stack. That is automatically done when the current stack frame is popped.


My main concern was that the malloc within the memcpy did not have its own free statement. But, if I understand you correctly, the act of copying from the heap to the stack has the same effect of free()? So free() is only required to release memory that resides on the heap?


If that doesn't make sense (if you don't know what the heap and stack are for example) it's time to learn basic C properly...
Trying to. Understanding basic CPU architecture (heap vs stack) seems useful. I just started watching the MIT opencourseware*. Hopefully it will be in there. If not, any book recommendations?

*ah, so that's what is behind big O notation... cool...

robbieduncan
Apr 24, 2011, 07:54 AM
Actually there is a malloc, it's embedded in the memcpy.

My mistake. But the underlying problem is the same: the address of returnElement is on the stack. The address of the memory the at malloced in the call to memcpy is "lost" as it is never saved into a variable. So that malloced memory is leaked.

But, if I understand you correctly, the act of copying from the heap to the stack has the same effect of free()?

No. Absolutely 100% not. You have allocated space on the heap with the malloc. The malloc returned the address for you to free later. You did not save this address so cannot call free but that is OK as memcpy also returns the destination address (so as you can put the memcpy and malloc in one line like you have). But you fail to save the address their, instead dereferencing the pointer and saving that value in a variable. So at this point the malloced space on the heap is leaked. This is a problem. The space you allocated on the stack for the double is fine: it will get cleaned up for you.

The solution is to split this into two lines. You need to have a void * pointer variable that you store the return from memcpy in. You can then dereference and cast the that into returnElement. And finally free the void * pointer returned from memcpy.

MrFusion
Apr 24, 2011, 08:16 AM
Basically you can't do this in one line.

Ah, that is unfortunate... Ah well, so be it.

Thanks for the replies, robbieduncan and jiminaus.

gnasher729
Apr 24, 2011, 10:24 AM
byteSizeOfElement is wrong. Your code will only work (if all other bugs are fixed) with 64 bit code, not with 32 bit code.
Using the result of memcpy is being too smart for your own good.

MrFusion
Apr 24, 2011, 12:28 PM
byteSizeOfElement is wrong. Your code will only work (if all other bugs are fixed) with 64 bit code, not with 32 bit code.

Care to explain why? :confused:

Having little faith in the user (mainly me) to actually release the pointer, I already decided to get the value by passing a reference.


-(void) element:(void *) element
atIndex:(NSInteger) index
{
memcpy(element,storage+index*byteSizeOfElement,byteSizeOfElement);
}

...
double result;
[aInstance element:&result
atIndex:index];
...



Using the result of memcpy is being too smart for your own good.
:)

The memcpy() function returns a pointer to dest. (http://www.kernel.org/doc/man-pages/online/pages/man3/memcpy.3.html)

lee1210
Apr 24, 2011, 01:36 PM
It's wrong because you want room for doubles, but get space for double *s. These are different types with different sizes. On platforms where sizeof(double) == sizeof(double *) your code will work. On platforms where this is not true you will not have the right amount of space. In most cases you will have too little space, not too much.

-Lee

gnasher729
Apr 24, 2011, 03:24 PM
The memcpy() function returns a pointer to dest. (http://www.kernel.org/doc/man-pages/online/pages/man3/memcpy.3.html)

That falls under "esoteric knowledge that nobody should ever bother with". Which means it immediately fails a code review, just like someone writing "a + 1 << i" and expecting people to know which operator has higher precedence.

robbieduncan
Apr 24, 2011, 03:27 PM
That falls under "esoteric knowledge that nobody should ever bother with". Which means it immediately fails a code review, just like someone writing "a + 1 << i" and expecting people to know which operator has higher precedence.

In all honesty I think that's a bit harsh. Anyone who uses memcpy a lot should know what it returns (if you don't and just ignore the returned value then you don't know what you are returning which I think is very bad practice). Anyone who does not use it a lot can be expected to lookup the documentation: that's what I did as soon as I opened this thread.

subsonix
Apr 25, 2011, 07:08 AM
In all honesty I think that's a bit harsh. Anyone who uses memcpy a lot should know what it returns (if you don't and just ignore the returned value then you don't know what you are returning which I think is very bad practice).

Except that in the case of memcpy, there is only one possible return value which it always returns.

This case is a bit different though as checking the return value would find out if malloc failed earlier, but it doesn't matter either because if malloc fails, memcpy will copy memory to NULL and the program will crash.