[PATCH] [DISCUSS] Make the variable NULL after freeing it.

[PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Amit Choudhary]

Hi,

Was just wondering if the _var_ in kfree(_var_) could be set to NULL after its freed. It may solve
the problem of accessing some freed memory as the kernel will crash since _var_ was set to NULL.

Does this make sense? If yes, then how about renaming kfree to something else and providing a
kfree macro that would do the following:

#define kfree(x) do { \
                      new_kfree(x); \
                      x = NULL; \
                    } while(0)

There might be other better ways too.

Regards,
Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Pavel Machek]

Hi!

> Was just wondering if the _var_ in kfree(_var_) could be set to NULL after its freed. It may solve
> the problem of accessing some freed memory as the kernel will crash since _var_ was set to NULL.
> 
> Does this make sense? If yes, then how about renaming kfree to something else and providing a
> kfree macro that would do the following:
> 
> #define kfree(x) do { \
>                       new_kfree(x); \
>                       x = NULL; \
>                     } while(0)
> 
> There might be other better ways too.

No, that would be very confusing. Otoh having

KFREE() do kfree() and assignment might be acceptable.

						Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Jan Engelhardt]

On Dec 27 2006 17:10, Pavel Machek wrote:

>> Was just wondering if the _var_ in kfree(_var_) could be set to
>> NULL after its freed. It may solve the problem of accessing some
>> freed memory as the kernel will crash since _var_ was set to NULL.
>> 
>> Does this make sense? If yes, then how about renaming kfree to
>> something else and providing a kfree macro that would do the
>> following:
>> 
>> #define kfree(x) do { \
>>                       new_kfree(x); \
>>                       x = NULL; \
>>                     } while(0)
>> 
>> There might be other better ways too.
>
>No, that would be very confusing. Otoh having
>KFREE() do kfree() and assignment might be acceptable.

What about setting x to some poison value from <linux/poison.h>?


	-`J'
-- 

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Bernd Petrovitsch]

On Thu, 2006-12-28 at 09:54 +0100, Jan Engelhardt wrote:
> On Dec 27 2006 17:10, Pavel Machek wrote:
> 
> >> Was just wondering if the _var_ in kfree(_var_) could be set to
> >> NULL after its freed. It may solve the problem of accessing some
> >> freed memory as the kernel will crash since _var_ was set to NULL.
> >> 
> >> Does this make sense? If yes, then how about renaming kfree to
> >> something else and providing a kfree macro that would do the
> >> following:
> >> 
> >> #define kfree(x) do { \
> >>                       new_kfree(x); \
> >>                       x = NULL; \
> >>                     } while(0)
> >> 
> >> There might be other better ways too.

----  snip  ----
(x) = NULL; \
----  snip  ----
?

> >No, that would be very confusing. Otoh having
> >KFREE() do kfree() and assignment might be acceptable.
> 
> What about setting x to some poison value from <linux/poison.h>?

That depends on the decision/definition if (so called) "double free" is
an error or not (and "free(NULL)" must work in POSIX-compliant
environments).
Personally I think it is pointless to disallow "kfree(NULL)" by using
some poison value and force people to add a "we have to free that
variable" variable to work around it instead of keeping it NULL (which
makes the "kfree($variable)" a no-op).
Former discussions are to be found in the archives ......

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Ingo Oeser]

On Sunday, 31. December 2006 14:38, Bernd Petrovitsch wrote:
> That depends on the decision/definition if (so called) "double free" is
> an error or not (and "free(NULL)" must work in POSIX-compliant
> environments).

A double free of non-NULL is certainly an error.
So the idea of setting it to NULL is ok, since then you can
kfree the variable over and over again without any harm.

It is just complicated to do this side effect free.

Maybe one should check for builtin-constant and take the address,
if this is not an builtin-constant.

sth, like this

#define kfree_nullify(x) do { \
	if (__builtin_constant_p(x)) { \
		kfree(x); \
	} else { \
		typeof(x) *__addr_x = &x; \
		kfree(*__addr_x); \
		*__addr_x = NULL; \
	} \
} while (0)

Regards

Ingo Oeser

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[YOSHIFUJI Hideaki / 吉藤英明]

In article <200701010143.02870.ioe-lkml@rameria.de> (at Mon, 1 Jan 2007 01:43:00 +0100), Ingo Oeser <ioe-lkml@rameria.de> says:

> On Sunday, 31. December 2006 14:38, Bernd Petrovitsch wrote:
> > That depends on the decision/definition if (so called) "double free" is
> > an error or not (and "free(NULL)" must work in POSIX-compliant
> > environments).
> 
> A double free of non-NULL is certainly an error.
> So the idea of setting it to NULL is ok, since then you can
> kfree the variable over and over again without any harm.

I dislike (or, say, I hate) this idea; people should fix up
such broken code paths.

--yoshfuji

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Amit Choudhary]

--- Ingo Oeser <ioe-lkml@rameria.de> wrote:

> On Sunday, 31. December 2006 14:38, Bernd Petrovitsch wrote:
> > That depends on the decision/definition if (so called) "double free" is
> > an error or not (and "free(NULL)" must work in POSIX-compliant
> > environments).
> 
> A double free of non-NULL is certainly an error.
> So the idea of setting it to NULL is ok, since then you can
> kfree the variable over and over again without any harm.
> 
> It is just complicated to do this side effect free.
> 
> Maybe one should check for builtin-constant and take the address,
> if this is not an builtin-constant.
> 
> sth, like this
> 
> #define kfree_nullify(x) do { \
> 	if (__builtin_constant_p(x)) { \
> 		kfree(x); \
> 	} else { \
> 		typeof(x) *__addr_x = &x; \
> 		kfree(*__addr_x); \
> 		*__addr_x = NULL; \
> 	} \
> } while (0)
> 
> Regards
> 
> Ingo Oeser
> 

This is a nice approach but what if someone does kfree_nullify(x+20).

I decided to keep it simple. If someone is calling kfree_nullify() with anything other than a
simple variable, then they should call kfree().  But definitely an approach that takes care of all
situations is the best but I cannot think of a macro that can handle all situations. The simple
macro that I sent earlier will catch all the other usage at compile time. Please let me know if I
have missed something.

-Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Ingo Oeser]

Hi,

On Monday, 1. January 2007 07:37, Amit Choudhary wrote:
> --- Ingo Oeser <ioe-lkml@rameria.de> wrote:
> > #define kfree_nullify(x) do { \
> > 	if (__builtin_constant_p(x)) { \
> > 		kfree(x); \
> > 	} else { \
> > 		typeof(x) *__addr_x = &x; \

Ok, I should change that line to 
		typeof(x) *__addr_x = &(x); \

> > 		kfree(*__addr_x); \
> > 		*__addr_x = NULL; \
> > 	} \
> > } while (0)
> > 
> > Regards
> > 
> > Ingo Oeser
> > 
> 
> This is a nice approach but what if someone does kfree_nullify(x+20).

Then this works, because the side effect (+20) is evaluated only once. 
AFAIK __builtin_constant_p() and typeof() are both free of side effects.

 
> I decided to keep it simple. If someone is calling kfree_nullify() with anything other than a
> simple variable, then they should call kfree().

kfree_nullify() has to replace kfree() to be of any use one day. So this is not an option.

Anybody thinking of "Hey, this must be NULL afterwards!", will set it to NULL himself.
Anybody else doesn't know or care about it, which is the case we like to catch.

> But definitely an approach that takes care of all 
> situations is the best but I cannot think of a macro that can handle all situations. The simple
> macro that I sent earlier will catch all the other usage at compile time. 

The problems I see are:
1. parameter to kfree is a value not a pointer 
    -> solved by using a macro instead of function, 
         but generate new (the other) problems
    -> take the address of the value there.
2. possible side effects of macro parameter usage 
   -> solved by assigning once only and using typeof
3. Constants don't have an address 
   -> need to check for constant

So apart from missing braces before taking the address, I don't see
any problem with my solution :-)

Should I send a patch?

> Please let me know if I have missed something.

I reviewed it and you missed side effects (kfree(x); x = NULL).

Regards

Ingo Oeser

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Andreas Schwab]

Ingo Oeser <ioe-lkml@rameria.de> writes:

> Hi,
>
> On Monday, 1. January 2007 07:37, Amit Choudhary wrote:
>> --- Ingo Oeser <ioe-lkml@rameria.de> wrote:
>> > #define kfree_nullify(x) do { \
>> > 	if (__builtin_constant_p(x)) { \
>> > 		kfree(x); \
>> > 	} else { \
>> > 		typeof(x) *__addr_x = &x; \
>
> Ok, I should change that line to 
> 		typeof(x) *__addr_x = &(x); \
>
>> > 		kfree(*__addr_x); \
>> > 		*__addr_x = NULL; \
>> > 	} \
>> > } while (0)
>> > 
>> > Regards
>> > 
>> > Ingo Oeser
>> > 
>> 
>> This is a nice approach but what if someone does kfree_nullify(x+20).
>
> Then this works, because the side effect (+20) is evaluated only once. 

It's not a side effect, it's a non-lvalue, and you can't take the address
of a non-lvalue.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Pavel Machek]

Hi!

> > I decided to keep it simple. If someone is calling kfree_nullify() with anything other than a
> > simple variable, then they should call kfree().
> 
> kfree_nullify() has to replace kfree() to be of any use one day. So this is not an option.
> 

Doing kfree() that writes to its argument is not an option. kfree()
looks like a function, so it should behave as one. KFREE() might be
okay.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Ingo Oeser]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On Monday, 1. January 2007 17:25, Andreas Schwab wrote:
> Ingo Oeser <ioe-lkml@rameria.de> writes:
> > Then this works, because the side effect (+20) is evaluated only once. 
> 
> It's not a side effect, it's a non-lvalue, and you can't take the address
> of a non-lvalue.

Just verified this. So If we cannot make it work in all cases, it will
cause more problems then it will solve.

So we are left with a function, which will 
a) only be used by janitors to provide "kfree(x); x = NULL;" 
    with an macro KFREE(x) in all the simple cases.

b) be used by developers, who are aware of the fact that reusable
    pointer values should set to NULL after kfree().

Doing a) and b) is "running into open doors", so doesn't prevent any
error, obfuscates code more and works only sometimes.

I give up here and would vote for dropping that idea then.


Regards

Ingo Oeser

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [DISCUSS] Make the variable NULL after freeing it.

[Jan Engelhardt]

On Jan 1 2007 22:40, Ingo Oeser wrote:
>On Monday, 1. January 2007 17:25, Andreas Schwab wrote:
>> Ingo Oeser <ioe-lkml@rameria.de> writes:
>> > Then this works, because the side effect (+20) is evaluated only once. 
>> 
>> It's not a side effect, it's a non-lvalue, and you can't take the address
>> of a non-lvalue.
>
>Just verified this. So If we cannot make it work in all cases, it will
>cause more problems then it will solve.
>
>So we are left with a function, which will 
>a) only be used by janitors to provide "kfree(x); x = NULL;" 
>    with an macro KFREE(x) in all the simple cases.

Just checking, where has it been decided that we actually are going to have
kfree_nullify() or whatever the end result happens to be called?


Thanks,
	-`J'
--