Commit 4363f493 authored by Chris Palmer's avatar Chris Palmer Committed by Commit Bot

[PartitionAlloc] Zero on free.

This CL 'zaps' (`memset`s) the allocated region on free. This may
help detect some instances of UAF, causing them to crash (with
nullptr deref).

Rather than doing so on every free, we zap probabilistically to
reduce the performance impact.

If the perf bots show an unacceptable regression, we'll revert this.
But, it might be acceptable.

Bug: 1005070, 1013329, 1013326, 1013324
Change-Id: Ia36ee17625290646216985dd6fdffabfa84a9ba2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1873272
Commit-Queue: Chris Palmer <palmer@chromium.org>
Reviewed-by: default avatarBartek Nowierski <bartekn@chromium.org>
Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816834}
parent 907a06d2
...@@ -92,6 +92,10 @@ ...@@ -92,6 +92,10 @@
#include <stdlib.h> #include <stdlib.h>
#endif #endif
#if defined(OS_WIN)
#include <windows.h>
#endif
#if defined(ADDRESS_SANITIZER) #if defined(ADDRESS_SANITIZER)
#include <sanitizer/asan_interface.h> #include <sanitizer/asan_interface.h>
#endif // defined(ADDRESS_SANITIZER) #endif // defined(ADDRESS_SANITIZER)
...@@ -106,6 +110,26 @@ ...@@ -106,6 +110,26 @@
PA_CHECK(false); \ PA_CHECK(false); \
} }
namespace {
// Returns true if we've hit the end of a random-length period. We don't want to
// invoke `RandomValue` too often, because we call this function in a hot spot
// (`Free`), and `RandomValue` incurs the cost of atomics.
#if !DCHECK_IS_ON()
ALWAYS_INLINE bool RandomPeriod() {
static thread_local uint8_t counter = 0;
if (UNLIKELY(counter == 0)) {
// It's OK to truncate this value.
counter = static_cast<uint8_t>(base::RandomValue());
}
// If `counter` is 0, this will wrap. That is intentional and OK.
counter--;
return counter == 0;
}
#endif
} // namespace
namespace base { namespace base {
typedef void (*OomFunction)(size_t); typedef void (*OomFunction)(size_t);
...@@ -479,10 +503,7 @@ ALWAYS_INLINE void PartitionRoot<thread_safe>::FreeNoHooksImmediate( ...@@ -479,10 +503,7 @@ ALWAYS_INLINE void PartitionRoot<thread_safe>::FreeNoHooksImmediate(
PA_DCHECK(page); PA_DCHECK(page);
PA_DCHECK(IsValidPage(page)); PA_DCHECK(IsValidPage(page));
#if DCHECK_IS_ON() const size_t utilized_slot_size = page->GetUtilizedSlotSize();
size_t utilized_slot_size = page->GetUtilizedSlotSize();
#endif
if (allow_extras) { if (allow_extras) {
// |ptr| points after the tag and the cookie. // |ptr| points after the tag and the cookie.
// //
...@@ -548,6 +569,17 @@ ALWAYS_INLINE void PartitionRoot<thread_safe>::FreeNoHooksImmediate( ...@@ -548,6 +569,17 @@ ALWAYS_INLINE void PartitionRoot<thread_safe>::FreeNoHooksImmediate(
#if DCHECK_IS_ON() #if DCHECK_IS_ON()
memset(ptr, kFreedByte, utilized_slot_size); memset(ptr, kFreedByte, utilized_slot_size);
#else
// `memset` only once in a while: we're trading off safety for time
// efficiency.
if (UNLIKELY(RandomPeriod()) && !page->bucket->is_direct_mapped()) {
#if defined(OS_WIN)
SecureZeroMemory(ptr, allocated_size);
#else
// TODO(palmer): Use an equivalent of memset_s.
memset(ptr, 0, utilized_slot_size);
#endif
}
#endif #endif
// TLS access can be expensive, do a cheap local check first. // TLS access can be expensive, do a cheap local check first.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment