Commit 1ec5add4 authored by Benoit Lize's avatar Benoit Lize Committed by Commit Bot

Reland "blink/bindings: ASAN checks to ensure proper ParkableString accesses."

This is a reland of 88924f03

Added a missing DCHECK_IS_ON() check.

Original change's description:
> blink/bindings: ASAN checks to ensure proper ParkableString accesses.
>
> A ParkableString underlying string memory should not be cached.
> Following a suggestion by pasko@, add ASAN instrumentation to make sure
> this is actually the case.
>
> Bug: 877044
> Change-Id: I463ad229b118f2e629b460f1a2beed41fc54b6a7
> Reviewed-on: https://chromium-review.googlesource.com/1216422
> Reviewed-by: Egor Pasko <pasko@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Commit-Queue: Benoit L <lizeb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#590251}

Bug: 877044
Change-Id: I777d53528037015ff3d16cd8c5ce2b649788848b
Reviewed-on: https://chromium-review.googlesource.com/1219486Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Reviewed-by: default avatarEgor Pasko <pasko@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590276}
parent 7c5aa07b
...@@ -7,6 +7,7 @@ ...@@ -7,6 +7,7 @@
#include "base/logging.h" #include "base/logging.h"
#include "base/metrics/histogram_macros.h" #include "base/metrics/histogram_macros.h"
#include "third_party/blink/renderer/platform/bindings/parkable_string_manager.h" #include "third_party/blink/renderer/platform/bindings/parkable_string_manager.h"
#include "third_party/blink/renderer/platform/wtf/address_sanitizer.h"
#include "third_party/blink/renderer/platform/wtf/thread_specific.h" #include "third_party/blink/renderer/platform/wtf/thread_specific.h"
namespace blink { namespace blink {
...@@ -17,6 +18,22 @@ void RecordParkingAction(ParkableStringImpl::ParkingAction action) { ...@@ -17,6 +18,22 @@ void RecordParkingAction(ParkableStringImpl::ParkingAction action) {
UMA_HISTOGRAM_ENUMERATION("Memory.MovableStringParkingAction", action); UMA_HISTOGRAM_ENUMERATION("Memory.MovableStringParkingAction", action);
} }
#if defined(ADDRESS_SANITIZER)
void AsanPoisonString(const String& string) {
const void* start = string.Is8Bit()
? static_cast<const void*>(string.Characters8())
: static_cast<const void*>(string.Characters16());
ASAN_POISON_MEMORY_REGION(start, string.CharactersSizeInBytes());
}
void AsanUnpoisonString(const String& string) {
const void* start = string.Is8Bit()
? static_cast<const void*>(string.Characters8())
: static_cast<const void*>(string.Characters16());
ASAN_UNPOISON_MEMORY_REGION(start, string.CharactersSizeInBytes());
}
#endif // defined(ADDRESS_SANITIZER)
} // namespace } // namespace
ParkableStringImpl::ParkableStringImpl(scoped_refptr<StringImpl>&& impl, ParkableStringImpl::ParkableStringImpl(scoped_refptr<StringImpl>&& impl,
...@@ -36,6 +53,9 @@ ParkableStringImpl::ParkableStringImpl(scoped_refptr<StringImpl>&& impl, ...@@ -36,6 +53,9 @@ ParkableStringImpl::ParkableStringImpl(scoped_refptr<StringImpl>&& impl,
ParkableStringImpl::~ParkableStringImpl() { ParkableStringImpl::~ParkableStringImpl() {
AssertOnValidThread(); AssertOnValidThread();
#if defined(ADDRESS_SANITIZER)
AsanUnpoisonString(string_);
#endif
if (is_parkable_) if (is_parkable_)
ParkableStringManager::Instance().Remove(string_.Impl()); ParkableStringManager::Instance().Remove(string_.Impl());
} }
...@@ -49,6 +69,20 @@ void ParkableStringImpl::Unlock() { ...@@ -49,6 +69,20 @@ void ParkableStringImpl::Unlock() {
MutexLocker locker(mutex_); MutexLocker locker(mutex_);
DCHECK_GT(lock_depth_, 0); DCHECK_GT(lock_depth_, 0);
lock_depth_ -= 1; lock_depth_ -= 1;
#if defined(ADDRESS_SANITIZER) && DCHECK_IS_ON()
// There are no external references to the data, nobody should touch the data.
//
// Note: Only poison the memory if this is on the owning thread, as this is
// otherwise racy. Indeed |Unlock()| may be called on any thread, and
// the owning thread may concurrently call |ToString()|. It is then allowed
// to use the string until the end of the current owning thread task.
// Requires DCHECK_IS_ON() for the |owning_thread_| check.
if (lock_depth_ == 0 && is_parkable_ && string_.Impl()->HasOneRef() &&
owning_thread_ == CurrentThread()) {
AsanPoisonString(string_);
}
#endif // defined(ADDRESS_SANITIZER) && DCHECK_IS_ON()
} }
bool ParkableStringImpl::Is8Bit() const { bool ParkableStringImpl::Is8Bit() const {
...@@ -64,6 +98,10 @@ bool ParkableStringImpl::IsNull() const { ...@@ -64,6 +98,10 @@ bool ParkableStringImpl::IsNull() const {
const String& ParkableStringImpl::ToString() { const String& ParkableStringImpl::ToString() {
AssertOnValidThread(); AssertOnValidThread();
MutexLocker locker(mutex_); MutexLocker locker(mutex_);
#if defined(ADDRESS_SANITIZER)
AsanUnpoisonString(string_);
#endif
Unpark(); Unpark();
return string_; return string_;
} }
...@@ -83,6 +121,9 @@ bool ParkableStringImpl::Park() { ...@@ -83,6 +121,9 @@ bool ParkableStringImpl::Park() {
// Cannot park strings with several references. // Cannot park strings with several references.
if (string_.Impl()->HasOneRef()) { if (string_.Impl()->HasOneRef()) {
#if defined(ADDRESS_SANITIZER)
AsanPoisonString(string_);
#endif
RecordParkingAction(ParkingAction::kParkedInBackground); RecordParkingAction(ParkingAction::kParkedInBackground);
is_parked_ = true; is_parked_ = true;
} }
......
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