MEM_RESERVE regions are not accessible by ReadProcessMemory()

MEM_RESERVE regions are not accessible by ReadProcessMemory()

Sadly this code did not survive a collision with the real world. In
probing for the environment block there's a MEM_COMMIT region followed
directly by a MEM_RESERVE region (past the end of the environment
block).

Update region checker to correctly treat MEM_RESERVE as inaccessible.

R=mark@chromium.org
BUG=crashpad:20, crashpad:46, crashpad:59
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/ccd5ec6404be1e9843a17261d0c65dd6d620f50b

[Mark Mentovai, 2015-10-01 20:51:09]

LGTM, but the a test would protect against possible future regression.

https://codereview.chromium.org/1370063005/diff/1/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1370063005/diff/1/util/win/process_info.cc#ne...
util/win/process_info.cc:1: // Copyright 2015 The Crashpad Authors. All rights
reserved.
> Sadly this code did not survive a collision with the real world.

The buildbots aren’t the real world, then. Can we add a real-world test?

https://codereview.chromium.org/1370063005/diff/1/util/win/process_info.cc#ne...
util/win/process_info.cc:113: (memory_info.protect & PAGE_GUARD) == 0));
(Re your comment yesterday about PAGE_EXECUTE being readable)

Yeah, that’s an x86 thing, you can’t be executable without being readable. Other
CPUs are different, but since this code only runs on x86, it’s correct to not
mention PAGE_EXECUTE.

[scottmg, 2015-10-01 21:30:20]

https://codereview.chromium.org/1370063005/diff/1/util/win/process_info.cc
File util/win/process_info.cc (right):

https://codereview.chromium.org/1370063005/diff/1/util/win/process_info.cc#ne...
util/win/process_info.cc:1: // Copyright 2015 The Crashpad Authors. All rights
reserved.
On 2015/10/01 20:51:09, Mark Mentovai wrote:
> > Sadly this code did not survive a collision with the real world.
> 
> The buildbots aren’t the real world, then. Can we add a real-world test?

Done, PTAL.

https://codereview.chromium.org/1370063005/diff/1/util/win/process_info.cc#ne...
util/win/process_info.cc:113: (memory_info.protect & PAGE_GUARD) == 0));
On 2015/10/01 20:51:09, Mark Mentovai wrote:
> (Re your comment yesterday about PAGE_EXECUTE being readable)
> 
> Yeah, that’s an x86 thing, you can’t be executable without being readable.
Other
> CPUs are different, but since this code only runs on x86, it’s correct to not
> mention PAGE_EXECUTE.

Aha, make sense.

[Mark Mentovai, 2015-10-01 21:43:52]

LGTM

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
util/win/process_info_test.cc:401: ProcessInfo info;
Don’t declare this until you use it below.

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
util/win/process_info_test.cc:412: // | ????? |       | ????? |       |       |
????? |
Maybe page[2] should be PAGE_NOACCESS to test that too.

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
util/win/process_info_test.cc:431: info.Initialize(GetCurrentProcess());
You call GetCurrentProcess() five times in this test. Cache it in a local
HANDLE?

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
util/win/process_info_test.cc:435: ASSERT_EQ(ranges.size(), 2u);
Order as (2u, ranges.size()).

[scottmg, 2015-10-01 22:03:49]

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
util/win/process_info_test.cc:401: ProcessInfo info;
On 2015/10/01 21:43:52, Mark Mentovai wrote:
> Don’t declare this until you use it below.

Done.

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
util/win/process_info_test.cc:412: // | ????? |       | ????? |       |       |
????? |
On 2015/10/01 21:43:52, Mark Mentovai wrote:
> Maybe page[2] should be PAGE_NOACCESS to test that too.

Sadly, I screwed that three line function up when I changed the sense. :(

Good idea on the test anyway!

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
util/win/process_info_test.cc:431: info.Initialize(GetCurrentProcess());
On 2015/10/01 21:43:52, Mark Mentovai wrote:
> You call GetCurrentProcess() five times in this test. Cache it in a local
> HANDLE?

Sure, it's just "return -1", but I guess it means less parens in the calls.

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
util/win/process_info_test.cc:435: ASSERT_EQ(ranges.size(), 2u);
On 2015/10/01 21:43:52, Mark Mentovai wrote:
> Order as (2u, ranges.size()).

Done.

[Mark Mentovai, 2015-10-01 22:22:29]

Nice. LGTM.

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1370063005/diff/20001/util/win/process_info_t...
util/win/process_info_test.cc:412: // | ????? |       | ????? |       |       |
????? |
scottmg wrote:
> On 2015/10/01 21:43:52, Mark Mentovai wrote:
> > Maybe page[2] should be PAGE_NOACCESS to test that too.
> 
> Sadly, I screwed that three line function up when I changed the sense. :(
> 
> Good idea on the test anyway!

The power of tests!

https://codereview.chromium.org/1370063005/diff/80001/util/win/process_info_t...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1370063005/diff/80001/util/win/process_info_t...
util/win/process_info_test.cc:511: reinterpret_cast<void*>(reserved_as_int +
(kBlockSize * 2)),
Can this be no_access instead of this monstrosity?

[scottmg, 2015-10-01 22:28:10]

https://codereview.chromium.org/1370063005/diff/80001/util/win/process_info_t...
File util/win/process_info_test.cc (right):

https://codereview.chromium.org/1370063005/diff/80001/util/win/process_info_t...
util/win/process_info_test.cc:511: reinterpret_cast<void*>(reserved_as_int +
(kBlockSize * 2)),
On 2015/10/01 22:22:29, Mark Mentovai wrote:
> Can this be no_access instead of this monstrosity?

Darn, not quite "unqualified". Done.

[scottmg, 2015-10-01 22:28:47]

Committed patchset #6 (id:100001) manually as
ccd5ec6404be1e9843a17261d0c65dd6d620f50b (presubmit successful).