Move the nsPresArena poisoning code to MFBT

RESOLVED FIXED in mozilla23

Status

()

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({sec-want})

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

This is good stuff that should be generally available to all code.
MFBT seems like a good home.
Posted patch Move the code (obsolete) — Splinter Review
Attachment #744060 - Flags: review?(roc)
Attachment #744061 - Flags: review?(benjamin)
Comment on attachment 744060 [details] [diff] [review]
Move the code

Review of attachment 744060 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresArena.cpp
@@ +110,5 @@
>  
>    char* p = reinterpret_cast<char*>(aPtr);
>    char* limit = p + list->mEntrySize;
>    for (; p < limit; p += sizeof(uintptr_t)) {
> +    *reinterpret_cast<uintptr_t*>(p) = mozPoisonValue();

Can you call into mfbt here?

::: mfbt/Poison.h
@@ +15,5 @@
> +#include "mozilla/Assertions.h"
> +#include "mozilla/StandardInteger.h"
> +#include "mozilla/Types.h"
> +
> +#ifdef __cplusplus

Do we use this from C?

@@ +32,5 @@
> +
> +/**
> + * Overwrite the memory block of aSize bytes at aPtr with the poison value.
> + * aPtr MUST be aligned at a sizeof(uintptr_t) boundary.
> + * Only an even number of sizeof(uintptr_t) bytes are overwritten, the last

I don't see the "event number" behaviour in the code.

@@ +37,5 @@
> + * few bytes (if any) is not overwritten.
> + */
> +inline void mozWritePoison(void* aPtr, size_t aSize)
> +{
> +  MOZ_ASSERT((uintptr_t)aPtr % sizeof(uintptr_t) == 0, "bad alignment");

reinterpret_cast (three times)
Attachment #744060 - Flags: review+ → review?(roc)
(In reply to :Ms2ger from comment #4)
> >    for (; p < limit; p += sizeof(uintptr_t)) {
> > +    *reinterpret_cast<uintptr_t*>(p) = mozPoisonValue();
> 
> Can you call into mfbt here?

Sure, I will replace that loop with a mozWritePoison call, thanks!

> Do we use this from C?

I intentionally made the API C compatible since I think poisoning can be
useful in C code too.

> > + * Overwrite the memory block of aSize bytes at aPtr with the poison value.
> > + * aPtr MUST be aligned at a sizeof(uintptr_t) boundary.
> > + * Only an even number of sizeof(uintptr_t) bytes are overwritten, the last
> 
> I don't see the "event number" behaviour in the code.

  char* p = (char*)aPtr;
  char* limit = p + aSize;
  for (; p < limit; p += sizeof(uintptr_t)) {
    *((uintptr_t*)p) = POISON;
  }

p+=sizeof(uintptr_t) will stop the iteration *before* limit is reached
in case it's not an even number of words, i.e. the last few bytes of the
last word is not poisoned.  This is how the original code did it, I guess
for efficiency and since poisoning the last uneven bytes doesn't improve
security much.

> > +  MOZ_ASSERT((uintptr_t)aPtr % sizeof(uintptr_t) == 0, "bad alignment");
> 
> reinterpret_cast (three times)

Does C have reinterpret_cast?
(I'll fold this with the last patch above before landing.)
Attachment #744071 - Flags: review?(roc)
Can we make this self-initializing? In particular we will want to use this from the allocator (see bug 860254). I don't expect that a branch in mozPoisonValue would be noticeable.
Flags: needinfo?(matspal)
Whiteboard: [call-jenny-for-a-good-time][great-bug-numbers]
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Can we make this self-initializing?

Wouldn't that require some kind of lock to be thread-safe?
Flags: needinfo?(matspal)
It should require a lock only in the slow case, but you'd need to statically allocate the lock (jemalloc does this).
Whiteboard: [call-jenny-for-a-good-time][great-bug-numbers]
Comment on attachment 744061 [details] [diff] [review]
Initialize it from xpcom

This is ok given the current usage, but if we can lazy-init I'd really prefer that.
Attachment #744061 - Flags: review?(benjamin) → review+
(In reply to Mats Palmgren [:mats] from comment #5)
>   char* p = (char*)aPtr;
>   char* limit = p + aSize;
>   for (; p < limit; p += sizeof(uintptr_t)) {
>     *((uintptr_t*)p) = POISON;
>   }
> 
> p+=sizeof(uintptr_t) will stop the iteration *before* limit is reached
> in case it's not an even number of words, i.e. the last few bytes of the
> last word is not poisoned.  This is how the original code did it, I guess
> for efficiency and since poisoning the last uneven bytes doesn't improve
> security much.

This looks funny.  Suppose p is 0x100, aSize is 5.  Then limit is 0x105, so the first time around the condition passes, and sizeof(uintpt_t) bytes starting at |p| get overwritten.  On 64-bit doesn't that overwrite 0x105, 0x106, and 0x107, beyond the specified size?  Or am I missing something totally obvious beyond the limited context/documentation here?

mfbt generally prefers things be C++.  Here, using C++ gives various good behaviors like being able to take advantage of type-parametric methods, to eliminate sizeof multiplications that can easily be gotten wrong.  If no user requires C here, I'd rather see this as C++ from get-go.  If someone wants to use this from C, and we can't get them converted to C++, we can add a C API at that point to wrap the C++ API.
Blocks: 860254
Comment on attachment 744060 [details] [diff] [review]
Move the code

Review of attachment 744060 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Poison.h
@@ +17,5 @@
> +#include "mozilla/Types.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif

MOZ_BEGIN_EXTERN_C

@@ +59,5 @@
> +extern MFBT_DATA uintptr_t gMozillaPoisonSize;
> +
> +#ifdef __cplusplus
> +} /*extern "C"  */
> +#endif

MOZ_END_EXTERN_C
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> This looks funny.  Suppose p is 0x100, aSize is 5.  Then limit is 0x105, so
> the first time around the condition passes, and sizeof(uintpt_t) bytes
> starting at |p| get overwritten.  On 64-bit doesn't that overwrite 0x105,
> 0x106, and 0x107, beyond the specified size?

There's no point in poisoning an object that is smaller than a pointer.
There's an assertion that covers it:

MOZ_ASSERT(aSize >= sizeof(uintptr_t), "poisoning this object has no effect");

> If no user requires C here

It appears there is, bug 860254, and of course various arena allocators like
nsPresArena which has a void* and a length rather than some typed object.
(In reply to Mike Hommey [:glandium] from comment #12)
> MOZ_BEGIN_EXTERN_C
> MOZ_END_EXTERN_C

Fixed.
Attachment #744060 - Attachment is obsolete: true
Attachment #744060 - Flags: review?(roc)
Attachment #746120 - Flags: review?(roc)
Depends on: 910074
You need to log in before you can comment on or make changes to this bug.