Fix DeprotectMallocZone to work across vm_regions [crashpad/crashpad : main]
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Apply the review comments from https://chromium-review.googlesource.com/c/7833547 to this as well.
Preferably, letâs actually wait until the other one is done before poking this one again, since the logic and code are all the same.
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 1 comment![]( "Open in Gerrit")
Apply the review comments from https://chromium-review.googlesource.com/c/7833547 to this as well.
Preferably, letâs actually wait until the other one is done before poking this one again, since the logic and code are all the same.
Updated with feed back from https://chromium-review.googlesource.com/c/7833547
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted Code-Review+1![]( "Open in Gerrit")
- Dave MacLachlan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
https://chromium-review.git.corp.google.com/c/chromium/src/+/7833547?tab=checksThis should be rewritten as a https://chromium-review.googlesource.com/c/⌠URL.
You can get rid of ?tab=checks too, since that just makes the URL long.
Related details
- Dave MacLachlan
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 3 comments![]( "Open in Gerrit")
This should be rewritten as a https://chromium-review.googlesource.com/c/⌠URL.
You can get rid of ?tab=checks too, since that just makes the URL long.
You can get rid of this line.
- Justin Cohen
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 1 comment![]( "Open in Gerrit")
Looks like I need some code-review votes here
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted Code-Review+1![]( "Open in Gerrit")
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +0 |
1 comment
Bug: N/ADave MacLachlanYou can get rid of this line.
Done
Done
It came back?
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Bug: N/ADave MacLachlanYou can get rid of this line.
Mark MentovaiDone
Done
It came back?
Oh, I meant to get rid of âBug:â, not âChange-Idâ. Gerrit requires Change-Id and it will probably just regenerate it if you get rid of it (unless getting rid of it causes something to break, Iâve seen that happen too.)
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 2 comments![]( "Open in Gerrit")
See also
https://crrev.com/c/7833547Dave MacLachlanone line?
Done
Bug: N/ADave MacLachlanYou can get rid of this line.
Mark MentovaiDone
Mark MentovaiDone
It came back?
Oh, I meant to get rid of âBug:â, not âChange-Idâ. Gerrit requires Change-Id and it will probably just regenerate it if you get rid of it (unless getting rid of it causes something to break, Iâve seen that happen too.)
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted Code-Review+1![]( "Open in Gerrit")
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 1 comment![]( "Open in Gerrit")
Looks like I need some code-review votes here
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Dave MacLachlan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Code-Review | +1 |
1 comment
+ sizeof(ChromeMallocZone);This probably needs to be sizeof(malloc_zone_t)
Related details
- Dave MacLachlan
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 1 comment![]( "Open in Gerrit")
This probably needs to be sizeof(malloc_zone_t)
- Justin Cohen
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted![]( "Open in Gerrit")
- Dave MacLachlan
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted Code-Review+1![]( "Open in Gerrit")
- Dave MacLachlan
- Justin Cohen
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan voted Commit-Queue+2![]( "Open in Gerrit")
- Justin Cohen
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted Code-Review+1![]( "Open in Gerrit")
- Dave MacLachlan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 1 comment![]( "Open in Gerrit")
Updated with AI comments from Justin.
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
This last update is not really an improvement.
// I do not know how we could have a single structure straddling non-contiguous regions.Stay within 80 columns, at least.
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
return true;}And generally format the code correctly. `git cl format` can help.
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 2 comments![]( "Open in Gerrit")
// I do not know how we could have a single structure straddling non-contiguous regions.Stay within 80 columns, at least.
Gerrit editor was defaulting to 100 columns. Fixed and did edits on my local machine. git cl format applied.
And generally format the code correctly. `git cl format` can help.
Sorry.. copy paste issue in the gerrit editor. Went back to doing edits on my local machine. Ran git cl format.
Related details
- Justin Cohen
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 3 comments![]( "Open in Gerrit")
bool first_region = true;I personally find these more sensible if theyâre framed as something thatâs false and becomes true, like a âvalidityâ bit, rather than something that tracks a structural aspect like ânumber of times through the loopâ.
But really, since this protects the `protection` variable and is very closely coupled to that, it makes even more sense to structure it as `std::optional<bool> protection`. Now you no longer need a bogus value for `protection` (be it `VM_PROT_NONE` or `-1`). Instead, either `protection.has_value()` or it doesnât!
// I do not know how we could have a single structure straddlingWho am âIâ? Who are âweâ? The meaning gets lost as soon as this gets committed. This could simply be âA single structure shouldnât straddle non-contiguous regions.â
LOG(ERROR) << "VM regions spanning the zone are not contiguous";In the same way that the relationship between `region_start` and `default_zone` is assured by `DCHECK`, this should also be a `DCHECK`.
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 1 comment![]( "Open in Gerrit")
LOG(ERROR) << "VM regions spanning the zone are not contiguous";In the same way that the relationship between `region_start` and `default_zone` is assured by `DCHECK`, this should also be a `DCHECK`.
I believe the AI argument here was that the DCHECK is going to get compiled out and if there is a gap (which I agree should never happen) we are going to be in a bad situation. I also agree that the earlier dcheck (region_start > default_zone) also puts us in a bad situation when the DCHECK is compiled out. Would you prefer they were both dchecks, or both LOG(ERROR) and return false situations?
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
I don't know anything about the AI argument because I'm not a machine, but these are both in scope for either DCHECK or CHECK.
I suggested DCHECK because the existing one was a DCHECK. When the condition you're guarding against is really logically impossible (as it is here), DCHECK is correct, and the fact that it won't produce code in many release configurations is a feature.
CHECK is fine if you prefer it, but that level of strictness isn't really necessary here. You're not dealing with untrusted input. But in particular because we don't control the other side of the interface and xnu has had bugs that CHECKs in the field have helped find, CHECK is also OK for both of these.
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
DCHECK_LE(region_start, reinterpret_cast<vm_address_t>(default_zone));Thinking about this further:
If your goal is to make this maximally defensive, this `DCHECK` (or `CHECK`) needs to be coupled with another that ensures that `default_zone` is within the region returned by `vm_region_64` (`region_start + region_length`). Otherwise, itâs possible that the loop would never terminate.
As before, these are all contractual, and Iâm happy with `DCHECK`, but would not object if you used `CHECK`. I do think that all of them should have the same applicability, though.
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 4 comments![]( "Open in Gerrit")
I personally find these more sensible if theyâre framed as something thatâs false and becomes true, like a âvalidityâ bit, rather than something that tracks a structural aspect like ânumber of times through the loopâ.
But really, since this protects the `protection` variable and is very closely coupled to that, it makes even more sense to structure it as `std::optional<bool> protection`. Now you no longer need a bogus value for `protection` (be it `VM_PROT_NONE` or `-1`). Instead, either `protection.has_value()` or it doesnât!
Done
DCHECK_LE(region_start, reinterpret_cast<vm_address_t>(default_zone));Thinking about this further:
If your goal is to make this maximally defensive, this `DCHECK` (or `CHECK`) needs to be coupled with another that ensures that `default_zone` is within the region returned by `vm_region_64` (`region_start + region_length`). Otherwise, itâs possible that the loop would never terminate.
As before, these are all contractual, and Iâm happy with `DCHECK`, but would not object if you used `CHECK`. I do think that all of them should have the same applicability, though.
Done
// I do not know how we could have a single structure straddlingWho am âIâ? Who are âweâ? The meaning gets lost as soon as this gets committed. This could simply be âA single structure shouldnât straddle non-contiguous regions.â
Done
LOG(ERROR) << "VM regions spanning the zone are not contiguous";Dave MacLachlanIn the same way that the relationship between `region_start` and `default_zone` is assured by `DCHECK`, this should also be a `DCHECK`.
I believe the AI argument here was that the DCHECK is going to get compiled out and if there is a gap (which I agree should never happen) we are going to be in a bad situation. I also agree that the earlier dcheck (region_start > default_zone) also puts us in a bad situation when the DCHECK is compiled out. Would you prefer they were both dchecks, or both LOG(ERROR) and return false situations?
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 1 comment![]( "Open in Gerrit")
I don't know anything about the AI argument because I'm not a machine, but these are both in scope for either DCHECK or CHECK.
I suggested DCHECK because the existing one was a DCHECK. When the condition you're guarding against is really logically impossible (as it is here), DCHECK is correct, and the fact that it won't produce code in many release configurations is a feature.
CHECK is fine if you prefer it, but that level of strictness isn't really necessary here. You're not dealing with untrusted input. But in particular because we don't control the other side of the interface and xnu has had bugs that CHECKs in the field have helped find, CHECK is also OK for both of these.
Moved all to CHECK
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
CHECK_LE(default_zone_address, region_start + region_length);`CHECK_LT`.
CHECK_NE(region_start, query_address);Uh-oh, shouldnât this be `CHECK_EQ`?
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 2 comments![]( "Open in Gerrit")
CHECK_LE(default_zone_address, region_start + region_length);Dave MacLachlan`CHECK_LT`.
Yes you are right. thank you for your vigilance.
Uh-oh, shouldnât this be `CHECK_EQ`?
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
I know that this is âjustâ a test but since itâs been some effort to get the logic nailed down, can you detail how you tested the non-happy-path case where the `malloc_zone_t` straddles multiple VM regions?
Related details
- Dave MacLachlan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 1 comment![]( "Open in Gerrit")
I know that this is âjustâ a test but since itâs been some effort to get the logic nailed down, can you detail how you tested the non-happy-path case where the `malloc_zone_t` straddles multiple VM regions?
Tested in a separate code branch in a test app by allocating two relatively large blocks (page_size * 16) using vm_allocate. Verified using vm_region that the allocated blocks did end up as two distinct regions. Found in certain cases that smaller blocks (i.e. page size) were allocated in the same region which meant that the loop in DeprotectMallocZone was not being executed twice which is key to verifying correctness. Passed in the address of the second block minus half the size of malloc_zone_t to the DeprotectMallocZone routine. Walked through using debugger to verify that it cycled through the loop twice and completed successfully. Deallocated both the blocks, allocated new blocks, verified that I had two distinct regions. Protected the second region making it readonly. Passed in the address of the second block minus half the size of malloc_zone_t to the DeprotectMallocZone routine. Walked through using debugger to verify that it cycled through the loop twice and failed the protection equality check.
Vowed to never again end up in a case where I am editing code in four distinct locations at the same time đ
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted![]( "Open in Gerrit")
- Dave MacLachlan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Thanks, Dave. Weâll take your vow seriously!
Related details
- Dave MacLachlan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Dave MacLachlanI know that this is âjustâ a test but since itâs been some effort to get the logic nailed down, can you detail how you tested the non-happy-path case where the `malloc_zone_t` straddles multiple VM regions?
Tested in a separate code branch in a test app by allocating two relatively large blocks (page_size * 16) using vm_allocate. Verified using vm_region that the allocated blocks did end up as two distinct regions. Found in certain cases that smaller blocks (i.e. page size) were allocated in the same region which meant that the loop in DeprotectMallocZone was not being executed twice which is key to verifying correctness. Passed in the address of the second block minus half the size of malloc_zone_t to the DeprotectMallocZone routine. Walked through using debugger to verify that it cycled through the loop twice and completed successfully. Deallocated both the blocks, allocated new blocks, verified that I had two distinct regions. Protected the second region making it readonly. Passed in the address of the second block minus half the size of malloc_zone_t to the DeprotectMallocZone routine. Walked through using debugger to verify that it cycled through the loop twice and failed the protection equality check.
Vowed to never again end up in a case where I am editing code in four distinct locations at the same time đ
Is this 'just' a test on the Chromium side of things? Should we add a test there (since iOS can't do death tests). (don't cut and paste this, it's proof of concept test)...
```
TEST(HandlerForbiddenAllocators, DeprotectMallocZoneStraddlesRegions) {
vm_size_t page_size = getpagesize();
vm_size_t block_size = page_size * 16;
vm_address_t block1 = 0;
// Allocate two contiguous blocks.
kern_return_t kr =
vm_allocate(mach_task_self(), &block1, block_size * 2, VM_FLAGS_ANYWHERE);
ASSERT_EQ(kr, KERN_SUCCESS);
// Deallocate the second half so we can reallocate it separately.
kr = vm_deallocate(mach_task_self(), block1 + block_size, block_size);
ASSERT_EQ(kr, KERN_SUCCESS);
vm_address_t block2 = block1 + block_size;
kr = vm_allocate(mach_task_self(), &block2, block_size, VM_FLAGS_FIXED);
ASSERT_EQ(kr, KERN_SUCCESS);
// Set a different inheritance attribute on the second block so the kernel
// treats them as distinct VM regions rather than coalescing them.
kr = vm_inherit(mach_task_self(), block2, block_size, VM_INHERIT_NONE);
ASSERT_EQ(kr, KERN_SUCCESS);
// Verify they are distinct regions using vm_region_64.
vm_address_t region_addr = block1;
vm_size_t region_size = 0;
struct vm_region_basic_info_64 info;
mach_msg_type_number_t info_count = VM_REGION_BASIC_INFO_COUNT_64;
mach_port_t unused;
kr = vm_region_64(mach_task_self(),
®ion_addr,
®ion_size,
VM_REGION_BASIC_INFO_64,
reinterpret_cast<vm_region_info_t>(&info),
&info_count,
&unused);
ASSERT_EQ(kr, KERN_SUCCESS);
mach_port_deallocate(mach_task_self(), unused);
// The region starting at block1 should have length equal to block_size if
// they are distinct regions.
ASSERT_EQ(region_addr, block1);
ASSERT_EQ(region_size, block_size);
// Construct a fake malloc_zone_t pointer straddling the two regions.
// Pass the address of the second block minus half the size of malloc_zone_t.
malloc_zone_t* fake_zone =
reinterpret_cast<malloc_zone_t*>(block2 - sizeof(malloc_zone_t) / 2);
vm_address_t reprotection_start = 0;
vm_size_t reprotection_length = 0;
vm_prot_t reprotection_value = VM_PROT_NONE;
bool success = internal::DeprotectMallocZone(fake_zone,
&reprotection_start,
&reprotection_length,
&reprotection_value);
EXPECT_TRUE(success);
// Clean up reprotection if needed.
if (reprotection_start) {
vm_protect(mach_task_self(),
reprotection_start,
reprotection_length,
false,
reprotection_value);
}
kr = vm_deallocate(mach_task_self(), block1, block_size);
EXPECT_EQ(kr, KERN_SUCCESS);
kr = vm_deallocate(mach_task_self(), block2, block_size);
EXPECT_EQ(kr, KERN_SUCCESS);
}
TEST(HandlerForbiddenAllocatorsDeathTest,
DeprotectMallocZoneMismatchedProtection) {
vm_size_t page_size = getpagesize();
vm_size_t block_size = page_size * 16;
vm_address_t block1 = 0;
kern_return_t kr =
vm_allocate(mach_task_self(), &block1, block_size * 2, VM_FLAGS_ANYWHERE);
ASSERT_EQ(kr, KERN_SUCCESS);
kr = vm_deallocate(mach_task_self(), block1 + block_size, block_size);
ASSERT_EQ(kr, KERN_SUCCESS);
vm_address_t block2 = block1 + block_size;
kr = vm_allocate(mach_task_self(), &block2, block_size, VM_FLAGS_FIXED);
ASSERT_EQ(kr, KERN_SUCCESS);
// Protect the second region making it readonly.
kr = vm_protect(mach_task_self(), block2, block_size, false, VM_PROT_READ);
ASSERT_EQ(kr, KERN_SUCCESS);
malloc_zone_t* fake_zone =
reinterpret_cast<malloc_zone_t*>(block2 - sizeof(malloc_zone_t) / 2);
vm_address_t reprotection_start = 0;
vm_size_t reprotection_length = 0;
vm_prot_t reprotection_value = VM_PROT_NONE;
// Expect a CHECK failure because the protection of the two regions differs.
ASSERT_DEATH_CHECK(internal::DeprotectMallocZone(fake_zone,
&reprotection_start,
&reprotection_length,
&reprotection_value),
"");
}
```
Related details
- Dave MacLachlan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
ma...@chromium.org this is more of a question for you ^^^^
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
I donât feel strongly that this _needs_ a test, because the setup is intense and the whole thing is kind of contrived.
I guess I feel better about DeprotectMallocZoneStraddlesRegions than DeprotectMallocZoneMismatchedProtection, which has to go out of its way to set up something unexpected that shouldnât happen. There are lots of other things that shouldnât happen, like a sparse map. I donât think itâs on us (or Dave) to add a death test for each of them.
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Is the Chromium version also test only?
And I'm happy to add the tests afterwards -- doesn't need to block this CL
Related details
- Dave MacLachlan
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan added 1 comment![]( "Open in Gerrit")
@justin For this version can I get the code-review approval so we can land?
Related details
- Justin Cohen
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Code-Review | +1 |
- Dave MacLachlan
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave MacLachlan (Gerrit)
Dave MacLachlan voted Commit-Queue+2![]( "Open in Gerrit")
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Dave MacLachlan
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
crashpad-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
crashpa...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
Fix DeprotectMallocZone to work across vm_regions
Memory can be allocated across memory regions. DeprotectMallocZone was
assuming that an allocation would be confined to a single memory region.
It now iterates through the regions if necessary and verifies that all
regions have the same memory protection values before changing the
memory protection.
See also https://crrev.com/c/7833547
- M test/ios/host/handler_forbidden_allocators.cc
Code-Review: +1 by Justin Cohen, +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |