Skip to content

Commit 642f8dc

Browse files
kjlubickmibrunin
authored andcommitted
[Backport] Security bug 421629753
Manual cherry-pick of patch originally reviewed on https://skia-review.googlesource.com/c/skia/+/1002497: Remove destructor calls on shutdown for static local variables After thinking about it more and chatting some experienced folks, I think I identified the wrong root cause in the linked bug and thus had the wrong fix in https://skia-review.googlesource.com/c/skia/+/1001824 Skia was violating the style guide [1] and running into the reasons that these non-trivial destructors are banned. sk_sp *has* a non-trivial destructor - it does reference counting and possible destructing of the held object. So even though SkImageFilterCache has a trivial destructor, sk_sp's freeing was causing the problem. "when a program starts threads that are not joined at exit, those threads may attempt to access objects after their lifetime has ended if their destructor has already run." Here's roughly what was going on, pieced together from the linked crash log 1) Thread 0 creates children threads 2) Thread 5 called SkImageFilterCache::Get() to create static local sk_sp<SkImageFilterCache>. Let's call it Alice. The function returns a copy of sk_sp called Bob. Bob and Alice point to an object with refcount 2. Bob goes out of scope. and there's just Alice with refcount of 1. 3) Thread 0 begins to shutdown. Via __run_exit_handlers it calls the destructor for Alice, which upon hitting a refcount of 0 free's the memory for the underlying cache. 4) Thread 5 is still plugging away and calls ::Get() which returns a new sk_sp (called Carol) with refcount 1 but pointing to a freed pointer. 5) Crash caught by sanitizer. In a non-sanitizer build, step 5 is likely "nothing happens anyway" or "segfault during shutdown". To fix this, we don't want the static local to be an sk_sp, but instead it should be a bare pointer. This ensures the object lives all the way until the OS reclaims the memory. [1] https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Change-Id: I134156898f840924f0ec7001d0eb7a11a45d6c53 Bug: b/421629753 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1002497 Reviewed-by: Michael Ludwig <michaelludwig@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com> Commit-Queue: Kaylee Lubick <kjlubick@google.com> Auto-Submit: Kaylee Lubick <kjlubick@google.com> Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/659334 Reviewed-by: Moss Heim <moss.heim@qt.io>
1 parent 16f89e0 commit 642f8dc

File tree

3 files changed

+23
-22
lines changed

3 files changed

+23
-22
lines changed

chromium/third_party/skia/src/core/SkFontMgr.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,8 @@ sk_sp<SkTypeface> SkFontMgr::legacyMakeTypeface(const char familyName[], SkFontS
152152
}
153153

154154
sk_sp<SkFontMgr> SkFontMgr::RefEmpty() {
155-
static sk_sp<SkFontMgr> singleton(new SkEmptyFontMgr);
156-
return singleton;
155+
static SkFontMgr* singleton = new SkEmptyFontMgr();
156+
return sk_ref_sp(singleton);
157157
}
158158

159159
/**

chromium/third_party/skia/src/core/SkImageFilterCache.cpp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,12 +158,11 @@ sk_sp<SkImageFilterCache> SkImageFilterCache::Create(size_t maxBytes) {
158158

159159
sk_sp<SkImageFilterCache> SkImageFilterCache::Get(CreateIfNecessary createIfNecessary) {
160160
static SkOnce once;
161-
static sk_sp<SkImageFilterCache> cache;
161+
static SkImageFilterCache* cache = nullptr;
162162

163163
if (createIfNecessary == CreateIfNecessary::kNo) {
164-
return cache;
164+
return sk_ref_sp(cache);
165165
}
166-
167-
once([]{ cache = SkImageFilterCache::Create(kDefaultCacheSize); });
168-
return cache;
166+
once([]{ cache = SkImageFilterCache::Create(kDefaultCacheSize).release(); });
167+
return sk_ref_sp(cache);
169168
}

chromium/third_party/skia/src/gpu/ganesh/GrTestUtils.cpp

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "include/core/SkRRect.h"
1818
#include "include/core/SkRect.h"
1919
#include "include/private/base/SkMacros.h"
20+
#include "include/private/base/SkOnce.h"
2021
#include "include/private/gpu/ganesh/GrTypesPriv.h"
2122
#include "src/base/SkRandom.h"
2223
#include "src/core/SkRectPriv.h"
@@ -325,33 +326,34 @@ SkPathEffect::DashType TestDashPathEffect::onAsADash(DashInfo* info) const {
325326

326327
sk_sp<SkColorSpace> TestColorSpace(SkRandom* random) {
327328
static sk_sp<SkColorSpace> gColorSpaces[3];
328-
static bool gOnce;
329-
if (!gOnce) {
330-
gOnce = true;
329+
static SkOnce once;
330+
once([] {
331331
// No color space (legacy mode)
332332
gColorSpaces[0] = nullptr;
333333
// sRGB or color-spin sRGB
334-
gColorSpaces[1] = SkColorSpace::MakeSRGB();
335-
gColorSpaces[2] = SkColorSpace::MakeSRGB()->makeColorSpin();
336-
}
337-
return gColorSpaces[random->nextULessThan(static_cast<uint32_t>(std::size(gColorSpaces)))];
334+
gColorSpaces[1] = SkColorSpace::MakeSRGB().release();
335+
gColorSpaces[2] = SkColorSpace::MakeSRGB()->makeColorSpin().release();
336+
});
337+
return sk_ref_sp(
338+
gColorSpaces[random->nextULessThan(static_cast<uint32_t>(std::size(gColorSpaces)))]);
338339
}
339340

340341
sk_sp<GrColorSpaceXform> TestColorXform(SkRandom* random) {
341342
// TODO: Add many more kinds of xforms here
342343
static sk_sp<GrColorSpaceXform> gXforms[3];
343-
static bool gOnce;
344-
if (!gOnce) {
345-
gOnce = true;
344+
static SkOnce once;
345+
once([] {
346346
sk_sp<SkColorSpace> srgb = SkColorSpace::MakeSRGB();
347347
sk_sp<SkColorSpace> spin = SkColorSpace::MakeSRGB()->makeColorSpin();
348348
// No gamut change
349349
gXforms[0] = nullptr;
350-
gXforms[1] = GrColorSpaceXform::Make(srgb.get(), kPremul_SkAlphaType,
351-
spin.get(), kPremul_SkAlphaType);
352-
gXforms[2] = GrColorSpaceXform::Make(spin.get(), kPremul_SkAlphaType,
353-
srgb.get(), kPremul_SkAlphaType);
354-
}
350+
gXforms[1] = GrColorSpaceXform::Make(
351+
srgb.get(), kPremul_SkAlphaType, spin.get(), kPremul_SkAlphaType)
352+
.release();
353+
gXforms[2] = GrColorSpaceXform::Make(
354+
spin.get(), kPremul_SkAlphaType, srgb.get(), kPremul_SkAlphaType)
355+
.release();
356+
});
355357
return gXforms[random->nextULessThan(static_cast<uint32_t>(std::size(gXforms)))];
356358
}
357359

0 commit comments

Comments
 (0)