Skip to content

Commit

Permalink
Prevent UB if a WeakPtr to an already-destroyed object is dereferenced.
Browse files Browse the repository at this point in the history
If a WeakPtr references an already-destroyed object, operator-> and
operator* end up simply dereferencing nullptr. However, dereferencing
nullptr is undefined behavior and can be optimized in surprising ways
by compilers. To prevent this from happening, add a defence of last
resort and CHECK that the WeakPtr is still valid.

(cherry picked from commit 0b308a0)

Bug: 817982
Change-Id: Ib3a025c18fbd9d5db88770fced2063135086847b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2463857
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#816701}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2524521
Reviewed-by: Adrian Taylor <adetaylor@chromium.org>
Reviewed-by: Krishna Govind <govind@chromium.org>
Commit-Queue: Krishna Govind <govind@chromium.org>
Commit-Queue: Adrian Taylor <adetaylor@chromium.org>
Cr-Commit-Position: refs/branch-heads/4240@{#1410}
Cr-Branched-From: f297677-refs/heads/master@{#800218}
  • Loading branch information
zetafunction authored and Commit Bot committed Nov 7, 2020
1 parent d2daf28 commit bbb64b5
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 2 deletions.
4 changes: 2 additions & 2 deletions base/memory/weak_ptr.h
Original file line number Diff line number Diff line change
Expand Up @@ -248,11 +248,11 @@ class WeakPtr : public internal::WeakPtrBase {
}

T& operator*() const {
DCHECK(get() != nullptr);
CHECK(ref_.IsValid());
return *get();
}
T* operator->() const {
DCHECK(get() != nullptr);
CHECK(ref_.IsValid());
return get();
}

Expand Down
22 changes: 22 additions & 0 deletions base/memory/weak_ptr_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -798,4 +798,26 @@ TEST(WeakPtrDeathTest, NonOwnerThreadReferencesObjectAfterDeletion) {
ASSERT_DCHECK_DEATH(arrow.target.get());
}

TEST(WeakPtrDeathTest, ArrowOperatorChecksOnBadDereference) {
// The default style "fast" does not support multi-threaded tests
// (introduces deadlock on Linux).
::testing::FLAGS_gtest_death_test_style = "threadsafe";

auto target = std::make_unique<Target>();
WeakPtr<Target> weak = target->AsWeakPtr();
target.reset();
EXPECT_CHECK_DEATH(weak->AsWeakPtr());
}

TEST(WeakPtrDeathTest, StarOperatorChecksOnBadDereference) {
// The default style "fast" does not support multi-threaded tests
// (introduces deadlock on Linux).
::testing::FLAGS_gtest_death_test_style = "threadsafe";

auto target = std::make_unique<Target>();
WeakPtr<Target> weak = target->AsWeakPtr();
target.reset();
EXPECT_CHECK_DEATH((*weak).AsWeakPtr());
}

} // namespace base

0 comments on commit bbb64b5

Please sign in to comment.