Commit 9aa76aed authored by Benoit Lize's avatar Benoit Lize Committed by Commit Bot

blink/bindings: Fix false-positive ASAN check.

A ParkableString underlying String may be atomic. In this case, as long
as it it alive, there is a raw pointer reference to it in a per-thread
table. This can lead to a use-after-poison as the string gets poisoned
whereas it is still in the table.

This is due to not freeing string_ in ParkableStringImpl. To fix that,
don't poison AtomicStrings (which are not the majority of
ParkableString).

This is a false positive as when real parking happens the underlying
string would be freed, hence removed from the AtomicStringTable.

Bug: 883344,877044
Change-Id: I685260eafe31da4cafed150b74870a08aa61ed40
Reviewed-on: https://chromium-review.googlesource.com/c/1228057Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601522}
parent c56690c3
...@@ -80,7 +80,14 @@ void ParkableStringImpl::Unlock() { ...@@ -80,7 +80,14 @@ void ParkableStringImpl::Unlock() {
// Requires DCHECK_IS_ON() for the |owning_thread_| check. // Requires DCHECK_IS_ON() for the |owning_thread_| check.
if (lock_depth_ == 0 && is_parkable_ && string_.Impl()->HasOneRef() && if (lock_depth_ == 0 && is_parkable_ && string_.Impl()->HasOneRef() &&
owning_thread_ == CurrentThread()) { owning_thread_ == CurrentThread()) {
AsanPoisonString(string_); // Since |string_| is not deallocated, it remains in the per-thread
// AtomicStringTable, where its content can be accessed for equality
// comparison for instance, triggering a poisoned memory access.
// See crbug.com/883344 for an example.
// TODO(lizeb): park the string in |Unlock()| when ASAN is enabled, removing
// this check.
if (!string_.Impl()->IsAtomic())
AsanPoisonString(string_);
} }
#endif // defined(ADDRESS_SANITIZER) && DCHECK_IS_ON() #endif // defined(ADDRESS_SANITIZER) && DCHECK_IS_ON()
} }
...@@ -122,7 +129,9 @@ bool ParkableStringImpl::Park() { ...@@ -122,7 +129,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) #if defined(ADDRESS_SANITIZER)
AsanPoisonString(string_); // See comment in |Unlock()| for this restriction.
if (!string_.Impl()->IsAtomic())
AsanPoisonString(string_);
#endif #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