Make SyzyAsan support the allocation > 1GB

Make SyzyAsan support the allocation > 1GB

Currently SyzyAsan doesn't support the allocations > 1GB (although we don't enforce this, see crbug.com/656554). This is causing an issue when using it on a Large Address Aware applications because this kind of allocations are valid.

This also make the SyzyAsan tests run in 4G mode by default.

BUG=656554
R=chrisha@chromium.org

Committed: https://github.com/google/syzygy/commit/ea4e050ecd48b344e82433f69c29d149bea89ed0

[Sébastien Marchand, 2016-11-23 15:57:03]

Patchset #2 (id:20001) has been deleted

[Sébastien Marchand, 2016-11-23 16:28:53]

sebmarchand@chromium.org changed reviewers:
+ chrisha@chromium.org

[Sébastien Marchand, 2016-11-23 16:28:54]

PTAL.

[chrisha, 2016-11-23 18:27:50]

This could still fail, but now for allocations that are bigger than 2GB, no?

We also need to have code in the BlockHeapManager which returns NULL for such
allocations, or passes them through to the underlying heap without guarding
them.

[Sébastien Marchand, 2016-11-23 19:27:11]

Done, PTAnL.

[chrisha, 2016-11-23 19:37:17]

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right):

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
syzygy/agent/asan/heap_managers/block_heap_manager.cc:51: const size_t
kMaxAllocSize = 0x8000000;
This should be part of block.h, and tied to a #defined constant that is used to
set the bitsize of the "size" field. With some static_assert or something to
make sure that everything works well?

static constexpr BlockHeader kDummyHeader = {};
static_assert(kDummyHeader.size - 1 == kMaxAllocSize);

Something like that?

(And this value should (1 << 31) - 1 = 0x7FFFFFFF, no?)

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
syzygy/agent/asan/heap_managers/block_heap_manager.cc:187: return nullptr;
I'd just push this through to the underlying heap without instrumentation. It
could be that *that* heap can make the allocation.

[Sébastien Marchand, 2016-11-23 22:19:17]

Patchset #6 (id:120001) has been deleted

[Sébastien Marchand, 2016-11-23 22:19:26]

Patchset #5 (id:100001) has been deleted

[Sébastien Marchand, 2016-11-23 22:36:01]

PTAL.

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right):

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
syzygy/agent/asan/heap_managers/block_heap_manager.cc:51: const size_t
kMaxAllocSize = 0x8000000;
On 2016/11/23 19:37:17, chrisha (slow) wrote:
> This should be part of block.h, and tied to a #defined constant that is used
to
> set the bitsize of the "size" field. With some static_assert or something to
> make sure that everything works well?
> 
> static constexpr BlockHeader kDummyHeader = {};
> static_assert(kDummyHeader.size - 1 == kMaxAllocSize);
> 
> Something like that?
> 
> (And this value should (1 << 31) - 1 = 0x7FFFFFFF, no?)

I've tried this but it doesn't seem to work, doing an overflow on purpose seems
to use the whole |unsigned| length (and not just the 31 bits), e.g. in a
unittest:
error: Value of: ~block_header.body_size
Actual: 4294967295
Expected: kBlockMaxAllocSize
Which is: 2147483647

(same behavior in a static_assert with block_header.body_size - 1)

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
syzygy/agent/asan/heap_managers/block_heap_manager.cc:187: return nullptr;
On 2016/11/23 19:37:17, chrisha (slow) wrote:
> I'd just push this through to the underlying heap without instrumentation. It
> could be that *that* heap can make the allocation.

I think that this should live in block.cc::BlockPlanLayout, as this is a limit
imposed by the Block format.

[chrisha, 2016-11-24 17:23:23]

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right):

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
syzygy/agent/asan/heap_managers/block_heap_manager.cc:51: const size_t
kMaxAllocSize = 0x8000000;
On 2016/11/23 22:36:01, Sébastien Marchand wrote:
> On 2016/11/23 19:37:17, chrisha (slow) wrote:
> > This should be part of block.h, and tied to a #defined constant that is used
> to
> > set the bitsize of the "size" field. With some static_assert or something to
> > make sure that everything works well?
> > 
> > static constexpr BlockHeader kDummyHeader = {};
> > static_assert(kDummyHeader.size - 1 == kMaxAllocSize);
> > 
> > Something like that?
> > 
> > (And this value should (1 << 31) - 1 = 0x7FFFFFFF, no?)
> 
> I've tried this but it doesn't seem to work, doing an overflow on purpose
seems
> to use the whole |unsigned| length (and not just the 31 bits), e.g. in a
> unittest:
> error: Value of: ~block_header.body_size
> Actual: 4294967295
> Expected: kBlockMaxAllocSize
> Which is: 2147483647
> 
> (same behavior in a static_assert with block_header.body_size - 1)

Acknowledged.

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
syzygy/agent/asan/heap_managers/block_heap_manager.cc:238: 
If you're moving the logic to BlockPlanLayout, we should have a fallback to try
the allocation using the *uninstrumented* heap here. The block may be too big
for our BlockHeaps to allocate, but it may still be able to be allocated by the
underlying heap we're wrapping.

So try a last ditch effort to make an unprotected allocation. If that then fails
we can bail by returning nullptr.

https://codereview.chromium.org/2527533003/diff/140001/syzygy/agent/asan/block.h
File syzygy/agent/asan/block.h (right):

https://codereview.chromium.org/2527533003/diff/140001/syzygy/agent/asan/bloc...
syzygy/agent/asan/block.h:120: static const uint32_t kBlockMaxAllocSize =
static_cast<uint32_t>(1 << 31) - 1;
Use a constexpr or a #define to store the value 31, and use that both here and
in the BlockState definition? Should help keep them in lockstep.

[Sébastien Marchand, 2016-11-25 02:38:21]

Thanks, PTAnL

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right):

https://codereview.chromium.org/2527533003/diff/80001/syzygy/agent/asan/heap_...
syzygy/agent/asan/heap_managers/block_heap_manager.cc:238: 
On 2016/11/24 17:23:22, chrisha (slow) wrote:
> If you're moving the logic to BlockPlanLayout, we should have a fallback to
try
> the allocation using the *uninstrumented* heap here. The block may be too big
> for our BlockHeaps to allocate, but it may still be able to be allocated by
the
> underlying heap we're wrapping.
> 
> So try a last ditch effort to make an unprotected allocation. If that then
fails
> we can bail by returning nullptr.

Done.

https://codereview.chromium.org/2527533003/diff/140001/syzygy/agent/asan/block.h
File syzygy/agent/asan/block.h (right):

https://codereview.chromium.org/2527533003/diff/140001/syzygy/agent/asan/bloc...
syzygy/agent/asan/block.h:120: static const uint32_t kBlockMaxAllocSize =
static_cast<uint32_t>(1 << 31) - 1;
On 2016/11/24 17:23:23, chrisha (slow) wrote:
> Use a constexpr or a #define to store the value 31, and use that both here and
> in the BlockState definition? Should help keep them in lockstep.

Done.

[chrisha, 2016-11-25 16:14:59]

Awesome, lgtm!

[Sébastien Marchand, 2016-11-25 16:16:30]

The CQ bit was checked by sebmarchand@chromium.org

[commit-bot: I haz the power, 2016-11-25 16:16:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-25 16:16:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-25 16:16:44]

CQ has no permission to schedule in bucket tryserver.client.syzygy

[Sébastien Marchand, 2016-11-25 16:31:11]

Description was changed from

==========
Make SyzyAsan support the allocation > 1GB

Currently SyzyAsan doesn't support the allocations > 1GB (although we don't
enforce this, see crbug.com/656554). This is causing an issue when using it on a
Large Address Aware applications because this kind of allocations are valid.

This also make the SyzyAsan tests run in 4G mode by default.

BUG=656554
==========

to

==========
Make SyzyAsan support the allocation > 1GB

Currently SyzyAsan doesn't support the allocations > 1GB (although we don't
enforce this, see crbug.com/656554). This is causing an issue when using it on a
Large Address Aware applications because this kind of allocations are valid.

This also make the SyzyAsan tests run in 4G mode by default.

BUG=656554
R=chrisha@chromium.org

Committed:
https://github.com/google/syzygy/commit/ea4e050ecd48b344e82433f69c29d149bea89ed0
==========

[Sébastien Marchand, 2016-11-25 16:31:13]

Committed patchset #7 (id:180001) manually as
ea4e050ecd48b344e82433f69c29d149bea89ed0.

[Sigurður Ásgeirsson, 2016-11-25 18:16:11]

siggi@chromium.org changed reviewers:
+ siggi@chromium.org

[Sigurður Ásgeirsson, 2016-11-25 18:16:12]

https://codereview.chromium.org/2527533003/diff/180001/syzygy/agent/asan/heap...
File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right):

https://codereview.chromium.org/2527533003/diff/180001/syzygy/agent/asan/heap...
syzygy/agent/asan/heap_managers/block_heap_manager.cc:239: // The allocation
might fail because its size exceed the maximum size that
It would IMHO make much more sense, and be much simpler and easier to maintain
correctness, if we outright reject allocations that are too large. I guarantee
that there's no code in Chrome that requires the ability to allocate >1Gb in a
chunk, though there will be code that makes that attempt.

[Sébastien Marchand, 2016-11-25 21:14:57]

https://codereview.chromium.org/2527533003/diff/180001/syzygy/agent/asan/heap...
File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right):

https://codereview.chromium.org/2527533003/diff/180001/syzygy/agent/asan/heap...
syzygy/agent/asan/heap_managers/block_heap_manager.cc:239: // The allocation
might fail because its size exceed the maximum size that
On 2016/11/25 18:16:11, Sigurður Ásgeirsson wrote:
> It would IMHO make much more sense, and be much simpler and easier to maintain
> correctness, if we outright reject allocations that are too large. I guarantee
> that there's no code in Chrome that requires the ability to allocate >1Gb in a
> chunk, though there will be code that makes that attempt.

I'm not sure, we had some CF test cases who allocate some really large buffers,
and I don't think that this is an issue? We shouldn't crash for these, we could
return nullptr but we've always to try to mimic Chrome as much as we can in this
kind of edge cases, and in this case Chrome doesn't cap the allocation size, so
I'm not sure that we should do it either (not instrumenting this allocation is a
different thing).