867530 - Move the nsPresArena poisoning code to MFBT

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Mats Palmgren (inactive)]

This is good stuff that should be generally available to all code.
MFBT seems like a good home.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Move the code

Comment 2

[Mats Palmgren (inactive)]

Attachment: Initialize it from xpcom

Comment 3

[Mats Palmgren (inactive)]

Attachment: Make the current poison consumers use the new API

https://tbpl.mozilla.org/?tree=Try&rev=58e64c34178e

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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)

Comment 5

[Mats Palmgren (inactive)]

(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?

Comment 6

[Mats Palmgren (inactive)]

Attachment: Make nsPresArena::Free use mozWritePoison

(I'll fold this with the last patch above before landing.)

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[Mats Palmgren (inactive)]

(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?

Comment 9

[Benjamin Smedberg]

It should require a lock only in the slow case, but you'd need to statically allocate the lock (jemalloc does this).

Comment 10

[Benjamin Smedberg]

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.

Comment 11

[Jeff Walden [:Waldo]]

(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.

Comment 12

[Mike Hommey [:glandium]]

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

Comment 13

[Mats Palmgren (inactive)]

(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.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Move the code, v2

(In reply to Mike Hommey [:glandium] from comment #12)
> MOZ_BEGIN_EXTERN_C
> MOZ_END_EXTERN_C

Fixed.

Comment 15

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35deab019135
https://hg.mozilla.org/integration/mozilla-inbound/rev/69c5ebd945de
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e1d4abf560

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35deab019135
https://hg.mozilla.org/mozilla-central/rev/69c5ebd945de
https://hg.mozilla.org/mozilla-central/rev/b8e1d4abf560