PA: Add `IntendedLeakSize` to `MallocMemoryDump` [chromium/src : main]
Takashi Sakamoto (Gerrit)
Takashi Sakamoto added 1 comment![]( "Open in Gerrit")
Related details
- Daniel Cheng
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
2 comments
NormalTestClass1* normal_obj1 = new NormalTestClass1;
ASSERT_NE(normal_obj1, nullptr);
EXPECT_NE(leaky_root,
partition_alloc::PartitionRoot::GetRootFromAddress(normal_obj1));
delete normal_obj1;nit: we can use unique_ptr
```
{
auto normal_obj1 = std::make_unique<NormalTestClass1>
ASSERT_NE(normal_obj1, nullptr);
EXPECT_NE(leaky_root,
partition_alloc::PartitionRoot::GetRootFromAddress(normal_obj1));
}
```
Same below.
"malloc/partitions/leaky",maybe `leaked` or `quarantined` or `kept_alive` rather than `leaky` which I feel doesn't fully tell you that things never get freed in here.
Related details
- Daniel Cheng
- Takashi Sakamoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Takashi Sakamoto voted and added 3 comments![]( "Open in Gerrit")
Votes added by Takashi Sakamoto
| Commit-Queue | +0 |
3 comments
NormalTestClass1* normal_obj1 = new NormalTestClass1;
ASSERT_NE(normal_obj1, nullptr);
EXPECT_NE(leaky_root,
partition_alloc::PartitionRoot::GetRootFromAddress(normal_obj1));
delete normal_obj1;nit: we can use unique_ptr
```
{
auto normal_obj1 = std::make_unique<NormalTestClass1>
ASSERT_NE(normal_obj1, nullptr);
EXPECT_NE(leaky_root,
partition_alloc::PartitionRoot::GetRootFromAddress(normal_obj1));
}
```Same below.
Done
maybe `leaked` or `quarantined` or `kept_alive` rather than `leaky` which I feel doesn't fully tell you that things never get freed in here.
I see. Done.
I also replaced all `Leaky` with `Leaked` (or `LeakedSecurityObject`).
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
5 comments
LGTM with nits addressed, and the CL description updated to explain more about the purpose of this change–I don't really know why we need this CL from reading the CL description, but I assume there's a good reason, and that reason should be in the description :)
auto* leaked_secirty_object_allocator =Is this a typo?
std::unique_ptr<NormalTestClass1> normal_obj1(new NormalTestClass1);
ASSERT_NE(normal_obj1, nullptr);Please fix this WARNING reported by ClangTidy: check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.o...
check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-unique.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-make-unique` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
std::unique_ptr<LeakedTestClass1> leaked_obj1(new LeakedTestClass1);
ASSERT_NE(leaked_obj1, nullptr);Please fix this WARNING reported by ClangTidy: check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.o...
check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-unique.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-make-unique` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
std::unique_ptr<LeakedTestClass2> leaked_obj2(new LeakedTestClass2);
ASSERT_NE(leaked_obj2, nullptr);Please fix this WARNING reported by ClangTidy: check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.o...
check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-unique.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-make-unique` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Related details
- Takashi Sakamoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Takashi Sakamoto added 6 comments![]( "Open in Gerrit")
LGTM with nits addressed, and the CL description updated to explain more about the purpose of this change–I don't really know why we need this CL from reading the CL description, but I assume there's a good reason, and that reason should be in the description :)
Done
Thank you for the review.
auto* leaked_secirty_object_allocator =Takashi SakamotoIs this a typo?
Done
std::unique_ptr<NormalTestClass1> normal_obj1(new NormalTestClass1);
ASSERT_NE(normal_obj1, nullptr);Please fix this WARNING reported by ClangTidy: check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.o...
check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-unique.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-make-unique` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
std::unique_ptr<LeakedTestClass1> leaked_obj1(new LeakedTestClass1);
ASSERT_NE(leaked_obj1, nullptr);Please fix this WARNING reported by ClangTidy: check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.o...
check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-unique.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-make-unique` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Done
std::unique_ptr<LeakedTestClass2> leaked_obj2(new LeakedTestClass2);
ASSERT_NE(leaked_obj2, nullptr);Please fix this WARNING reported by ClangTidy: check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.o...
check: modernize-make-unique
use std::make_unique instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-unique.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-make-unique` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Takashi Sakamoto voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: base/trace_event/malloc_dump_provider_unittest.cc
Insertions: 6, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/trace_event/malloc_dump_provider.cc
Insertions: 4, Deletions: 4.
The diff is too large to show. Please review the diff.
```
Change information
PA: Add `IntendedLeakSize` to `MallocMemoryDump`
Since leaked security objects are leaked, not really freed, to avoid
heap-use-after-free, they may cause significant memory regressions, and
finally lots of OOM. So we have to monitor how much memory are leaked.
Firstly update memory-infra to gather total size of the leaked memory as
"malloc/partitions/leaked". The size will be reported to UMA in the
later CL.
- M base/allocator/partition_allocator/src/partition_alloc/partition_root.cc
- M base/allocator/partition_allocator/src/partition_alloc/partition_root.h
- M base/memory/advanced_memory_safety_checks.cc
- M base/memory/advanced_memory_safety_checks.h
- M base/memory/safety_checks_unittest.cc
- M base/trace_event/malloc_dump_provider.cc
- M base/trace_event/malloc_dump_provider.h
- M base/trace_event/malloc_dump_provider_unittest.cc
- M base/trace_event/memory_infra_background_allowlist.cc
Code-Review: +1 by Daniel Cheng, +1 by Stephen Nusko
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |