FIX: free()'s issue with already free'd pointers

[Raphael]

Hi there, I noticed some bad behaviour of the free() function with pointers that were already freed before, and using the 'safe' way of "if (!ptr) free(ptr);" doesn't work either. This will at most cases crash the program.
After looking at the sources, I noticed that the pointer given to free won't be set to NULL after being freed, so that pretty much explains the problem.

So my request is to fix that up. Here's the patch

Code: Select all

$ svn diff
Index: alloc.c
===================================================================
--- alloc.c     (revision 1847)
+++ alloc.c     (working copy)
@@ -415,6 +415,7 @@
                }

                _pspsdk_alloc_unlock();
+               ptr = NULL;
                return;
        }

@@ -446,6 +447,7 @@
        cur->prev->next = cur->next;

        _pspsdk_alloc_unlock();
+       ptr = NULL;
 }
 #endif

PS: I know it's not best programming style to not know exactly which pointers were already freed and not, but sometimes after a lot of coding you don't have the overlook and just want to get sure and then will get shot because of this :P

[ryoko_no_usagi]

This is not bad behaviour of free(), it's your responsibility to keep track of your pointers. Look at how free() is declared; it can't do what you ask. You had better set all pointers to your free()'d memory to NULL yourself.

[Raphael]

Then why would "free(ptr); if (!ptr) free(ptr);" work (not crash) on any C/C++ compiler for PC? Because libc on PC sets the freed pointer to NULL after correctly doing so.
This also fits the logic, because the calling function else cannot tell if the free failed or not since there's no return value.
So my request stays.



EDIT: Sorry, i completely read over that one sentence

Look at how free() is declared; it can't do what you ask.

. You're right, the pointer won't be set to NULL the way free is declared...

[raf]

Then why would "free(ptr); if (!ptr) free(ptr);" work (not crash) on any C/C++ compiler for PC? Because libc on PC sets the freed pointer to NULL after correctly doing so.
This also fits the logic, because the calling function else cannot tell if the free failed or not since there's no return value.
So my request stays.



EDIT: Sorry, i completely read over that one sentence

Look at how free() is declared; it can't do what you ask.

. You're right, the pointer won't be set to NULL the way free is declared...

There's no way for free to set your pointer to NULL for you.
You need to do it manually, like:

Code: Select all

free(my_ptr);
my_ptr = NULL;

or you could create a macro:

Code: Select all

#define Free(x) { free(x); x = NULL; }

The reason free can't NULL the pointer for you is because free doesn't take a double pointer, so it can't change the address pointed by your pointer. It can only change the data dereferenced.
(BTW, this is the way free has always worked, not something specific to the pspsdk. It is not true that free sets the pointer to NULL on any other architecture/libc that I'm aware of. If it doesn't crash on your PC, it may just be luck).

Hope this helps;

Raf.

[TyRaNiD]

Double freeing is a common bug in C code, it all depends on how clever the heap routines are as to whether it is likely to crash your system or not, and setting the pointer to NULL and getting free to check it might work in a specific case but it then assumes your app code knows what it is doing. Alot of systems will work okay with double freeing because the heap allocator will realise that that pointer is bogus and not do anything with it.

The real issue with free as it stands is it directly correlates the real address with the instance of that allocated memory block, therefore if you malloc then free then malloc again you could wind up with an aliased pointer somewhere which is very bad indeed, better to have a handle based system where the instance is separate from the actual location of the data. Incidently this is exactly what the psp kernel does for its memory allocations, just we then alloc a big block for our heap and use a malloc routine to partition it up. Oh well :)

[groepaz]

also

Code: Select all

if (!ptr) free(ptr);

is WRONG. it must be

Code: Select all

if (ptr!=NULL) free(ptr);

even if its often the case, NULL is _not_ defined as binary zero.

[ryoko_no_usagi]

also

Code: Select all

if (!ptr) free(ptr);

is WRONG. it must be

Code: Select all

if (ptr!=NULL) free(ptr);

even if its often the case, NULL is _not_ defined as binary zero.

It is wrong but it's a semantic error, not a syntactical:

Code: Select all

if (!ptr) free(ptr);

is equivalent to

Code: Select all

if (ptr == 0) free(ptr);

That is, the test evaluates to true if ptr is the null pointer and then the null pointer is attempted to be free(), which is harmless but also pointless. The opposite test is wanted in this case so properly it should be

Code: Select all

if (ptr) free(ptr);

Of course since free() will accept the null pointer with no effects, no test is actually needed, although one must of course keep track of whether the memory has already been free'd or not.

[groepaz]

ehrm no, you didnt get what i said at all, which is: NULL is _NOT_ 0. NULL is _only_ guaranteed to be an invalid pointer, which might be binary 0x12345678 or 0xdeadbabe or whatever the implementation uses. it just happens to be 0 in many (most) implementations. this is why the NULL symbol exists in the first place.

[PeterM]

In C++, NULL is zero (because Stroustrup says so).

I think "if (!ptr)" is a bit nicer to type, and possibly has clearer semantics ("is this a valid pointer?") than explicitly comparing to NULL.

[ryoko_no_usagi]

ehrm no, you didnt get what i said at all, which is: NULL is _NOT_ 0. NULL is _only_ guaranteed to be an invalid pointer, which might be binary 0x12345678 or 0xdeadbabe or whatever the implementation uses. it just happens to be 0 in many (most) implementations. this is why the NULL symbol exists in the first place.

No, the NULL macro is guaranteed to evaluate to 0. You are confusing a couple of things: NULL is a stylistic convention to represent the null pointer constant. The null pointer constant is a "constant integral expression with the value 0." The null pointer constant is used to represent the C language concept of the null pointer. The internal representation of the null pointer may be different from 0 if it is preferable on a particular platform, but the programmer need not be aware of this representation.

Code: Select all

if (ptr)

is a perfectly valid piece of C code. It is equivalent to

Code: Select all

if (ptr != 0)

and 0 is clearly a "constant integral expression with the value 0" so the compiler will understand that the null pointer is the intended target for the comparison. The compiler may choose to represent the null pointer differently from 0 in the generated code but that does not matter in this context.

Use of NULL or 0 is a matter of choice, but NULL may make programs more legible and easier to understand.

[BlackDiamond]

Setting the pointer to NULL inside free() would have no effects anyway. Unless you decide to change is it from "void free(void*)" to "void free(void**)" and free your pointer by calling "free(&ptr)".

[groepaz]

In C++, NULL is zero (because Stroustrup says so).

ok, i dont know C++ at all, i use strictly ansi-c only :)

No, the NULL macro is guaranteed to evaluate to 0. You are confusing a couple of things: NULL is a stylistic convention to represent the null pointer constant. The null pointer constant is a "constant integral expression with the value 0." The null pointer constant is used to represent the C language concept of the null pointer. The internal representation of the null pointer may be different from 0 if it is preferable on a particular platform, but the programmer need not be aware of this representation.

mmmh, are you sure this is actually part of the C standard (NOT C++!) ? i'm almost sure its not. (i dont have a standard handy here unfortunatly) i have worked with quite some embedded systems where 0 is a perfectly valid pointer. i even worked with one where the memory pool used for malloc started at 0. something like "if(!ptr)" wouldnt work at all, or atleast not correctly, on such a system. (and i remember fixing a lot of broken code to use the NULL constant to make it work :=P)

*afaik* there is a difference between C and C++ here, *afaik* in C NULL is defined as a typeless number representing an invalid pointer, and only in C++ its actually "(void*)0".

edit:
http://www.xploiter.com/programming/c/cnull.shtml

mmh mmh mmmh. *shrug*. i have to assume that the compiler i worked with was either made in pre-standard days, or was simply broken. interisting....next time when i have to fix such code i may blame it on the compiler :=)

[PeterM]

The difference between NULL in C and C++ is quite interesting (if you're dorky like me...)

In C, NULL is defined as (void*) 0, in standard-conforming compilers, and conversions from void* to X* are implicit (which can lead to all kinds of problems) so you can do X* p = malloc(sizeof(X)) etc.

In C++, NULL is defined simply as 0, and conversions from 0 to any pointer type are valid. You would have to cast from void* to X* using a static_cast.

Pete

[Jim]

In C, any constant expression which evaluates to 0 is equivalent to NULL. It doesn't have to be (void *)0 it can be 0 or 0L or 5-5. In C++ it must be an integer.

6.3.2.3 of C99
An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant.

4.10 of C++ standard
A null pointer constant is an integral constant expression rvalue of integer type that evaluates to zero.

Jim