[haiku-bugs] Re: [Haiku] #6312: slab: protection from wrong freed objects
- From: "bonefish" <trac@xxxxxxxxxxxx>
- Date: Wed, 14 Jul 2010 00:50:30 -0000
#6312: slab: protection from wrong freed objects
----------------------------+-----------------------------------------------
Reporter: lucian | Owner: axeld
Type: enhancement | Status: closed
Priority: normal | Milestone: R1
Component: System/Kernel | Version: R1/Development
Resolution: fixed | Keywords:
Blocked By: | Has a Patch: 1
Platform: All | Blocking:
----------------------------+-----------------------------------------------
Changes (by bonefish):
* status: new => closed
* resolution: => fixed
Comment:
Replying to [comment:3 lucian]:
> Are you sure the second check is not correct?
> {{{
> intptr_t objectOffset = data - source->offset - (uint8*)
source->pages;
> if (objectOffset % object_size != 0) { panic... }
> }}}
> I do remove the slab::offset before checking: {{{data -
source->offset}}}.
Yeah, sorry, I overlooked that.
> I took as guide the way objects are created in InitSlab:
> {{{
> uint8* data = ((uint8*)pages) + slab->offset;
> for (size_t i = 0; i < slab->size; i++) {
> status = constructor(cookie, data);
> data += object_size;
> }
> }}}
>
> The lower bounds check can be tighter (should be {{{data < source->pages
+ source->offset}}} I'll address that in a second version, but as far as
I understand the code there's no off-by-one error (the check uses ">=",
not ">").
You're right. The check could be tighter, but just like for the lower
bounds check, the alignment check catches those cases anyway. Never mind.
> I wasn't aware one could return **after** a {{{panic()}}}.
Yeah, the `continue` KDL command does that for you. Depending on the case
that might not make much sense or will cause the same `panic()` to occur
again (e.g. on page fault), but some situations are well continuable (as
this one). Although one might want to shut the system down orderly as soon
as possible and fix the cause of the problem.
> I'm not as sure as you are about the KDEBUG >= 1 checks. Trashing a
slab's ->free list can easily lead to data corruption or data loss. As a
user (not only as a developer) I'd prefer to reboot than have data
silently corrupted (think of a malloc() from a file system that receives a
pointer to an object that's being written by someone else in parallel
corrupting data in transit or before being sent to the disk).
> I'll put them in because you requested them, but I still disagree with
you.
There's always a trade-off when it comes to error checks. This is pretty
hot code (on UP machines called for every free()), so only very cheap
checks for programmer errors should be in the production code. I'd also
like to add a check for double-frees, but since that is even more
expensive, I would only enable it at a higher debug level or as an
optional debug feature.
> I've rewritten the patch.
Thanks. Committed in r37508 with small changes: I joined the two `if`s and
adjusted the `panic()` text.
--
Ticket URL: <http://dev.haiku-os.org/ticket/6312#comment:4>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.
Other related posts:
