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
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.
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'
--
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
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
In article <[email protected]> (at Mon, 1 Jan 2007 01:43:00 +0100), Ingo Oeser <[email protected]> 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
--- Ingo Oeser <[email protected]> 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
Hi,
On Monday, 1. January 2007 07:37, Amit Choudhary wrote:
> --- Ingo Oeser <[email protected]> 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
Ingo Oeser <[email protected]> writes:
> Hi,
>
> On Monday, 1. January 2007 07:37, Amit Choudhary wrote:
>> --- Ingo Oeser <[email protected]> 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, [email protected]
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."
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
On Monday, 1. January 2007 17:25, Andreas Schwab wrote:
> Ingo Oeser <[email protected]> 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
On Jan 1 2007 22:40, Ingo Oeser wrote:
>On Monday, 1. January 2007 17:25, Andreas Schwab wrote:
>> Ingo Oeser <[email protected]> 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'
--