|
|
Created:
9 years, 7 months ago by peterssen Modified:
9 years, 7 months ago CC:
sawbuck-changes_googlegroups.com Base URL:
http://sawbuck.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionIntegrated the ZebraBlockHeap to the BlockHeapManager.
There are some details missing since the HeapManager is not finished yet.
I'm sorry for the delay. I faced not one, but two bugs with
non-deterministic behaviour, most times the tests passed, but a very
few times just failed on random places.
It was difficult to debug since I had no idea where or when it was
going to break.
Those bugs are fixed now, ironically were caused by use after free.
The good part is that now I have a very good idea how everything works
since I had to dig deep to discover the bugs.
BUG=
R=chrisha@chromium.org
Committed: https://code.google.com/p/sawbuck/source/detail?r=2272
Patch Set 1 : rebase #
Total comments: 10
Patch Set 2 : New approach, cleaner and simpler #
Total comments: 49
Patch Set 3 : Fixes after review + comments #
Total comments: 7
Patch Set 4 : Fixed nits #
MessagesTotal messages: 11
PTAL. Not perfect, since there are a few things missing, but is a good start.
Sign in to reply to this message.
A general comment about the design. https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:45: // the memory notifier is implemented. Indent + 4. https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:50: unguarded_allocation_heap_.reset(new heaps::WinHeap()); You're never allocating the zebra heap ! You should allocate it and add it to the heaps_ map... If you want to resize it in the unittests you'll need to write an fixture that delete the previous zebra heap, remove it from the map, instantiate a new one and add it to the map. https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:113: // ZebraBlockHeap always is useless since it will becomes full really fast Is this really true ? Here's the workflow I had in mind: Let's say that you have a quarantine of X slots and the following constraints: - The maximum quarantine size is of Y blocks. (X * the quarantine ratio of the heap) - Z = X - Y, the maximum number of allocated blocks. - y is the number of quarantined blocks at a given time. - z is the number of allocated blocks at a given time. The user do Z allocations, free Y of them and allocates Y of them. You're now in a state where the zebra heap is full (z == Z and y == Y). Then the user free one more blocks (called b here), we'll start by moving b from the z list (the allocated blocks) to the y list (the quarantined blocks). You now have z < Z, this mean that the heap isn't full anymore. However you now have y > Y (there's too much blocks in the quarantine !), so you'll have to trim it until y <= Y (this logic is already here in the free function). So after this operation you have a free slot in your heap, ready for the next allocation ! (the one that has been released from the quarantine during the trim operation). Does it all make sense ? Sorry for all the variables here :) https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:195: reinterpret_cast<BlockHeapInterface*>(heap_id)); You should be able to retrieve the quarantine by replacing heap_id by block_info.trailer->heap_id on this line (if you put the zebra heap in the map). https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:263: TrimQuarantine(&shared_quarantine_); If parameters_.quarantine_size is equal to 0 you'll get an error in BlockHeapManager::TrimQuarantine now that you've lifted out this case to FlushQuarantine. https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:342: Extra BL.
Sign in to reply to this message.
PTAL. I refactored the integration using the old approach, now the ZebraBlockHeap is treated the same way as the normal heaps reusing all the existing code with only a few changes. I tried to use the more general BlockQuarantineInterface instead of the specific ShardedQuarantineInterface where possible. On the SizeLimitedQuarantine, a size 0 shouldn't mean unlimited (as is now); it should mean empty. I think unlimited should be expressed with -1 or SIZE_MAX. It should be rather easy to integrate the remaining heaps to the BlockHeapManager. https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:45: // the memory notifier is implemented. On 2014/08/21 20:28:17, Sébastien Marchand wrote: > Indent + 4. Done. https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:50: unguarded_allocation_heap_.reset(new heaps::WinHeap()); On 2014/08/21 20:28:17, Sébastien Marchand wrote: > You're never allocating the zebra heap ! You should allocate it and add it to > the heaps_ map... > > If you want to resize it in the unittests you'll need to write an fixture that > delete the previous zebra heap, remove it from the map, instantiate a new one > and add it to the map. Adding the ZebraBlockHeap to heaps_ will be he best approach, but sadly doesn't integrate well, I mean the existing code will only need a few modifications, the main problem is with the existing tests, that's the main reason I opted for dealing with ZebraBlockHeap separately. UPDATE: The tests were completely broken because of the two non-deterministic bugs. I've refactored the code and everything integrates nicely with the existing code. The ZebraBlockHeap is inserted in heaps_ and treated in the same way as the other heaps. https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:113: // ZebraBlockHeap always is useless since it will becomes full really fast On 2014/08/21 20:28:17, Sébastien Marchand wrote: > Is this really true ? Here's the workflow I had in mind: > > Let's say that you have a quarantine of X slots and the following constraints: > - The maximum quarantine size is of Y blocks. (X * the quarantine ratio of the > heap) > - Z = X - Y, the maximum number of allocated blocks. > - y is the number of quarantined blocks at a given time. > - z is the number of allocated blocks at a given time. > > The user do Z allocations, free Y of them and allocates Y of them. You're now in > a state where the zebra heap is full (z == Z and y == Y). > > Then the user free one more blocks (called b here), we'll start by moving b from > the z list (the allocated blocks) to the y list (the quarantined blocks). You > now have z < Z, this mean that the heap isn't full anymore. However you now have > y > Y (there's too much blocks in the quarantine !), so you'll have to trim it > until y <= Y (this logic is already here in the free function). > > So after this operation you have a free slot in your heap, ready for the next > allocation ! (the one that has been released from the quarantine during the trim > operation). > > Does it all make sense ? Sorry for all the variables here :) The ZebraBlockHeap quarantine by itself sets a lower bound on the number of elements, that means that the whole heap can be quarantined at some point. Hopefully the BlockHeapManager trims the quarantine after each call to Free, which indeed sets an uppper bound for the number of elements, so the ZebraBlockHeap quarantine (managed by the BlockheapManager) will never occupy more than the specified ratio. https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc (right): https://codereview.appspot.com/125510044/diff/20001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:480: TestShadow::shadow_[0]; How do this got here?
Sign in to reply to this message.
Looking really good ! I haven't reviewed the unittests in details but the general implementation is way better than before :) the less changes to BlockHeapManager the better ! I'll do a deeper review on Monday. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:55: zebra_block_heap_ = new ZebraBlockHeap(1024 * 1024, why aren't you using the size specified in the parameters here ? Maybe you can initialize it only if the zebra heap has been enabled ? https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:126: remove this extra BL. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:346: Remove one of the extra BL.
Sign in to reply to this message.
https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:55: zebra_block_heap_ = new ZebraBlockHeap(1024 * 1024, On 2014/08/22 20:27:46, Sébastien Marchand wrote: > why aren't you using the size specified in the parameters here ? Maybe you can > initialize it only if the zebra heap has been enabled ? Yup... only allocate a ZebraBlockHeap if enable_zebra_block_heap is true, and use the size from the parameters. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:108: void* ptr = NULL; Call this 'alloc', for consistency? https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:113: // with non-important allocations. This isn't true. We want to use the zebra heap whenever it is available, and space will be freed up again as objects are freed. Remove this TODO? https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:114: if (parameters_.enable_zebra_block_heap) { I would simply directly check if zebra_block_heap_ is non-null. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:160: BlockHeapInterface* heap = reinterpret_cast<BlockHeapInterface*>(heap_id); This doesn't get used anymore? https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:275: BlockQuarantineInterface* quarantine) { Either: (1) Align this with BlockHeapInterface; (2) Put both paramters on this line; or, (3) Put each parameter on its own line, indented +4. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:319: delete heap; Shouldn't this be outside of the if body? ie: we should always delete |heap|? https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.h (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.h:205: heaps::ZebraBlockHeap* zebra_block_heap_; Comment that lifetime management of the heap is provided by the HeapQuarantineMap, and that this is simply a useful pointer for finding the zebra heap directly. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.h:211: class NullMemoryNotifier : public agent::asan::MemoryNotifierInterface { There is already a NullMemoryNotififer defined as a unittest fixture. Can move it to agent/asan/memory_notifiers/null_memory_notifier.* in a separate CL? Do this first to clean up this CL? https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:42: // Allows to disable the quarantine and refuse allocations. "Allows to" isn't good usage. Maybe? Can be runtime configured to disable the quarantine and refuse allocations. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:57: // Wrapper that allows to easly disable allocations ont the ZebraBlockHeap. ... allows easily* disabling allocations via the ZebraBlockHeap. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:61: BlockLayout* layout) OVERRIDE { Fix alignment. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:71: // quarantine. ... allows easily disabling the insertion of blocks in the quarantine. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:268: // runtime error. What runtime error? Can you explain this? https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:293: void EnableMockZebraBlockHeap() { EnableTestZebraBlockHeap?
Sign in to reply to this message.
more comments. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:67: DestroyHeapUnlocked(iter_heaps->first, iter_heaps->second); set the zebra heap pointer to null. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:112: // ZebraBlockHeap always is useless since it will becomes full really fast This comment isn't true anymore ? Unless an allocation is never freed it should cycle through the quarantine ? https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.h (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.h:230: // Memory notifier used as a termporal workaround for the ZebraBlockHeap. s/termporal/temporary/ https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:43: class TestZebraBlockHeap : public heaps::ZebraBlockHeap { Sounds like a mock zebra heap ? Why not using only mocks here ? https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:104: using BlockHeapManager::FreePotentiallyCorruptBlock; alphabetical order please (keep the variable separated from the functions, but in alpha order) https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:489: TestShadow::shadow_[0]; Remove this line ?
Sign in to reply to this message.
PTAL. Many fixes, zebra heap initialization delayed until enabled... https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:55: zebra_block_heap_ = new ZebraBlockHeap(1024 * 1024, On 2014/08/22 20:27:46, Sébastien Marchand wrote: > why aren't you using the size specified in the parameters here ? Maybe you can > initialize it only if the zebra heap has been enabled ? Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:55: zebra_block_heap_ = new ZebraBlockHeap(1024 * 1024, On 2014/08/25 19:27:47, chrisha wrote: > On 2014/08/22 20:27:46, Sébastien Marchand wrote: > > why aren't you using the size specified in the parameters here ? Maybe you can > > initialize it only if the zebra heap has been enabled ? > > Yup... only allocate a ZebraBlockHeap if enable_zebra_block_heap is true, and > use the size from the parameters. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:55: zebra_block_heap_ = new ZebraBlockHeap(1024 * 1024, On 2014/08/22 20:27:46, sebmarchand - Google Account wrote: > why aren't you using the size specified in the parameters here ? Maybe you can > initialize it only if the zebra heap has been enabled ? Moved all the initialization to PropagateParameters, so the zebra heap is instantiated when enabled for the first time. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:67: DestroyHeapUnlocked(iter_heaps->first, iter_heaps->second); On 2014/08/25 19:31:29, Sébastien Marchand wrote: > set the zebra heap pointer to null. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:108: void* ptr = NULL; On 2014/08/25 19:27:47, chrisha wrote: > Call this 'alloc', for consistency? Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:112: // ZebraBlockHeap always is useless since it will becomes full really fast On 2014/08/25 19:31:29, Sébastien Marchand wrote: > This comment isn't true anymore ? Unless an allocation is never freed it should > cycle through the quarantine ? Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:113: // with non-important allocations. On 2014/08/25 19:27:47, chrisha wrote: > This isn't true. We want to use the zebra heap whenever it is available, and > space will be freed up again as objects are freed. Remove this TODO? Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:114: if (parameters_.enable_zebra_block_heap) { On 2014/08/25 19:27:47, chrisha wrote: > I would simply directly check if zebra_block_heap_ is non-null. The zebra heap is not destroyed when disabled, as it can still contains allocations. (parameters_.enable_zebra_block_heap == true) =>(implies) zebra_block_heap_ != NULL BUT (zebra_block_heap_ != NULL) !=>(not implies) (parameters_.enable_zebra_block_heap == true) https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:160: BlockHeapInterface* heap = reinterpret_cast<BlockHeapInterface*>(heap_id); On 2014/08/25 19:27:47, chrisha wrote: > This doesn't get used anymore? Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:275: BlockQuarantineInterface* quarantine) { On 2014/08/25 19:27:47, chrisha wrote: > Either: > > (1) Align this with BlockHeapInterface; > (2) Put both paramters on this line; or, > (3) Put each parameter on its own line, indented +4. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:319: delete heap; On 2014/08/25 19:27:47, chrisha wrote: > Shouldn't this be outside of the if body? ie: we should always delete |heap|? Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.h (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.h:205: heaps::ZebraBlockHeap* zebra_block_heap_; On 2014/08/25 19:27:47, chrisha wrote: > Comment that lifetime management of the heap is provided by the > HeapQuarantineMap, and that this is simply a useful pointer for finding the > zebra heap directly. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.h:211: class NullMemoryNotifier : public agent::asan::MemoryNotifierInterface { On 2014/08/25 19:27:47, chrisha wrote: > There is already a NullMemoryNotififer defined as a unittest fixture. Can move > it to agent/asan/memory_notifiers/null_memory_notifier.* in a separate CL? Do > this first to clean up this CL? Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.h:230: // Memory notifier used as a termporal workaround for the ZebraBlockHeap. On 2014/08/25 19:31:29, Sébastien Marchand wrote: > s/termporal/temporary/ Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc (right): https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:42: // Allows to disable the quarantine and refuse allocations. On 2014/08/25 19:27:47, chrisha wrote: > "Allows to" isn't good usage. Maybe? > > Can be runtime configured to disable the quarantine and refuse allocations. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:43: class TestZebraBlockHeap : public heaps::ZebraBlockHeap { On 2014/08/25 19:31:29, sebmarchand - Google Account wrote: > Sounds like a mock zebra heap ? Why not using only mocks here ? I tried to use a mock, but was hard to emulate all the guarantees that the ZebraBlockHeap provides for allocations. But it can be enhanced a little more. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:57: // Wrapper that allows to easly disable allocations ont the ZebraBlockHeap. On 2014/08/25 19:27:47, chrisha wrote: > ... allows easily* disabling allocations via the ZebraBlockHeap. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:61: BlockLayout* layout) OVERRIDE { On 2014/08/25 19:27:47, chrisha wrote: > Fix alignment. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:71: // quarantine. On 2014/08/25 19:27:47, chrisha wrote: > ... allows easily disabling the insertion of blocks in the quarantine. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:104: using BlockHeapManager::FreePotentiallyCorruptBlock; On 2014/08/25 19:31:29, Sébastien Marchand wrote: > alphabetical order please (keep the variable separated from the functions, but > in alpha order) Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:126: On 2014/08/22 20:27:46, Sébastien Marchand wrote: > remove this extra BL. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:268: // runtime error. On 2014/08/25 19:27:47, chrisha wrote: > What runtime error? Can you explain this? TearDown is called before destructing the zebra heap. The blocks contains stack-related information somehow linked to the runtime_ instance. If the runtime_ instance is cleared before the zebra heap and there are still blocks in its quarantine, some non-deterministic errors (failed DCHECKS) may arise. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:293: void EnableMockZebraBlockHeap() { On 2014/08/25 19:27:47, chrisha wrote: > EnableTestZebraBlockHeap? Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:346: On 2014/08/22 20:27:46, sebmarchand - Google Account wrote: > Remove one of the extra BL. Done. https://codereview.appspot.com/125510044/diff/40001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:489: TestShadow::shadow_[0]; On 2014/08/25 19:31:29, Sébastien Marchand wrote: > Remove this line ? No idea how it got there.
Sign in to reply to this message.
A handful of nits, but otherwise looks good to me! https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.cc (right): https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.cc:250: if (parameters_.enable_zebra_block_heap) { if (parameters_.enable_zebra_block_heap && zebra_block_heap_ == NULL https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager.h (right): https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.h:60: // heap only disable allocations on it, deallocations will continue to work. disables* https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager.h:209: // Tha lifetime management of the zebra heap is provided by the The* lifetime... https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... File syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc (right): https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:58: // Wrapper that allows to easily disable allocations ont the ZebraBlockHeap. Wrapper that allows easily disabling allocations. https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:72: // quarantine. Wrapper that allows easily disabling the quarantine. https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:307: mock_zebra_block_heap_ = new TestZebraBlockHeap(); You say 'mock', but its a 'test' zebra heap. Fix the language here and elsewhere... https://codereview.appspot.com/125510044/diff/80001/syzygy/agent/asan/heap_ma... syzygy/agent/asan/heap_managers/block_heap_manager_unittest.cc:992: // The BlockHeapManager correctly unpoison the memory after free if the unpoisons*
Sign in to reply to this message.
So.... lgtm with nits :)
Sign in to reply to this message.
On 2014/08/26 19:43:56, chrisha wrote: > So.... lgtm with nits :) Nits fixed, commiting.
Sign in to reply to this message.
|