From c1e97b372e21edf9c7e45cfea0eca7f1a52fe9e5 Mon Sep 17 00:00:00 2001 From: herb Date: Thu, 5 Mar 2015 11:51:11 -0800 Subject: [PATCH] Fix uninitialized memory bug in the SkGlyphCache. The core of the problem is that the system is asked to lookup the metrics for a character with id == 0. This causes a hit in the fCharToGlyphHash matching the sentinel glyph. This happens because fCharToGlpyhHash is initialized with all zeros, therefore, the fID is zero matching the char with id == 0. The fAdvanceX field of the sentinel glyph is in fact not initialized. The bigger question is now did a zero character get passed to getUnicharMetrics? The breaking code is basically as follows: wchar_t glyph = L'S'; paint.measureText(&glyph, 2); This get mischaracterized as a utf8 string instead of a utf16(?) string. Because of the little endian ordering, this is the character string 'L' '\0'. Since the size of the original string is two bytes (but a single character) the '\0' is treated as its own character and past to getUnicharMetrics. TEST: On windows failed using DrMemory. With this change does not fail. BUG=463204 Review URL: https://codereview.chromium.org/977063002 --- gm/fontcache.cpp | 3 +++ src/core/SkGlyph.h | 11 ++++++++--- src/core/SkGlyphCache.cpp | 34 +++++++++++++++++++--------------- src/core/SkGlyphCache.h | 10 ++++++---- 4 files changed, 36 insertions(+), 22 deletions(-) diff --git a/gm/fontcache.cpp b/gm/fontcache.cpp index d5ead8c121..0f5080e15d 100644 --- a/gm/fontcache.cpp +++ b/gm/fontcache.cpp @@ -52,6 +52,9 @@ protected: paint.setTypeface(fTypefaces[0]); paint.setTextSize(192); + // Make sure the nul character does not cause problems. + paint.measureText("\0", 1); + SkScalar x = 20; SkScalar y = 128; SkString text("ABCDEFGHIJ"); diff --git a/src/core/SkGlyph.h b/src/core/SkGlyph.h index 48b9815a03..abca215d00 100644 --- a/src/core/SkGlyph.h +++ b/src/core/SkGlyph.h @@ -34,6 +34,8 @@ class SkGlyph { public: static const SkFixed kSubpixelRound = SK_FixedHalf >> SkGlyph::kSubBits; + // A value that can never be generated by MakeID. + static const uint32_t kImpossibleID = ~0; void* fImage; SkPath* fPath; SkFixed fAdvanceX, fAdvanceY; @@ -147,6 +149,7 @@ class SkGlyph { static uint32_t MakeID(unsigned code) { SkASSERT(code <= kCodeMask); + SkASSERT(code != kImpossibleID); return code; } @@ -154,9 +157,11 @@ class SkGlyph { SkASSERT(code <= kCodeMask); x = FixedToSub(x); y = FixedToSub(y); - return (x << (kSubShift + kSubShiftX)) | - (y << (kSubShift + kSubShiftY)) | - code; + uint32_t ID = (x << (kSubShift + kSubShiftX)) | + (y << (kSubShift + kSubShiftY)) | + code; + SkASSERT(ID != kImpossibleID); + return ID; } // FIXME - This is needed because the Android frame work directly diff --git a/src/core/SkGlyphCache.cpp b/src/core/SkGlyphCache.cpp index 3fe8f0ae9e..224a70a37f 100755 --- a/src/core/SkGlyphCache.cpp +++ b/src/core/SkGlyphCache.cpp @@ -66,11 +66,11 @@ SkGlyphCache::SkGlyphCache(SkTypeface* typeface, const SkDescriptor* desc, SkSca fDesc = desc->copy(); fScalerContext->getFontMetrics(&fFontMetrics); - + // Create the sentinel SkGlyph. - SkGlyph* sentinel = fGlyphArray.insert(kSentinelGlyphIndex); - sentinel->initGlyphFromCombinedID(kSentinelGlyphID); - + SkGlyph* sentinel = fGlyphArray.insert(0); + sentinel->initGlyphFromCombinedID(SkGlyph::kImpossibleID); + // Initialize all index to zero which points to the sentinel SkGlyph. memset(fGlyphHash, 0x00, sizeof(fGlyphHash)); @@ -128,11 +128,14 @@ SkGlyphCache::CharGlyphRec* SkGlyphCache::getCharGlyphRec(uint32_t id) { if (NULL == fCharToGlyphHash.get()) { // Allocate the array. fCharToGlyphHash.reset(kHashCount); - // Initialize entries of fCharToGlyphHash to index the sentinel glyph. - memset(fCharToGlyphHash.get(), 0x00, - sizeof(CharGlyphRec) * kHashCount); + // Initialize entries of fCharToGlyphHash to index the sentinel glyph and + // an fID value that will not match any id. + for (int i = 0; i getCharGlyphRec(id); SkGlyph* glyph; if (rec->fID != id) { - RecordHashCollisionIf(glyph_index != kSentinelGlyphIndex); + RecordHashCollisionIf(glyph_index != SkGlyph::kImpossibleID); // this ID is based on the UniChar rec->fID = id; // this ID is based on the glyph index @@ -243,9 +246,9 @@ SkGlyph* SkGlyphCache::lookupByCombinedID(uint32_t id, MetricsType type) { uint32_t hash_index = ID2HashIndex(id); uint16_t glyph_index = fGlyphHash[hash_index]; SkGlyph* glyph = &fGlyphArray[glyph_index]; - + if (glyph->fID != id) { - RecordHashCollisionIf(glyph_index != kSentinelGlyphIndex); + RecordHashCollisionIf(glyph_index != SkGlyph::kImpossibleID); glyph_index = this->lookupMetrics(id, type); fGlyphHash[hash_index] = glyph_index; glyph = &fGlyphArray[glyph_index]; @@ -259,6 +262,7 @@ SkGlyph* SkGlyphCache::lookupByCombinedID(uint32_t id, MetricsType type) { } uint16_t SkGlyphCache::lookupMetrics(uint32_t id, MetricsType mtype) { + SkASSERT(id != SkGlyph::kImpossibleID); // Count is always greater than 0 because of the sentinel. // The fGlyphArray cache is in descending order, so that the sentinel with a value of ~0 is // always at index 0. @@ -280,7 +284,7 @@ uint16_t SkGlyphCache::lookupMetrics(uint32_t id, MetricsType mtype) { if (kFull_MetricsType == mtype && glyph->isJustAdvance()) { fScalerContext->getMetrics(glyph); } - SkASSERT(glyph_index != kSentinelGlyphIndex); + SkASSERT(glyph->fID != SkGlyph::kImpossibleID); return glyph_index; } @@ -288,10 +292,10 @@ uint16_t SkGlyphCache::lookupMetrics(uint32_t id, MetricsType mtype) { if (glyph->fID > id) { glyph_index += 1; } - + // Not found, but hi contains the index of the insertion point of the new glyph. fMemoryUsed += sizeof(SkGlyph); - + this->adjustCaches(glyph_index); glyph = fGlyphArray.insert(glyph_index); @@ -304,7 +308,7 @@ uint16_t SkGlyphCache::lookupMetrics(uint32_t id, MetricsType mtype) { fScalerContext->getMetrics(glyph); } - SkASSERT(glyph_index != kSentinelGlyphIndex); + SkASSERT(glyph->fID != SkGlyph::kImpossibleID); return glyph_index; } diff --git a/src/core/SkGlyphCache.h b/src/core/SkGlyphCache.h index 200655bb41..d0b792f273 100644 --- a/src/core/SkGlyphCache.h +++ b/src/core/SkGlyphCache.h @@ -207,13 +207,15 @@ private: enum { kHashBits = 8, kHashCount = 1 << kHashBits, - kHashMask = kHashCount - 1, - kSentinelGlyphIndex = 0, - kSentinelGlyphID = ~0 + kHashMask = kHashCount - 1 }; - + // A quick lookup to avoid the binary search looking for glyphs in fGlyphArray. uint16_t fGlyphHash[kHashCount]; + // Contains the SkGlyphs that are used by fGlyphHash and fCharToGlyphHash. The zero element + // is reserved for a sentinel SkGlyph that reduces the logic to check for collisions in the + // hash arrays. The zero element has an fID of SkGlyph::kImpossibleID which never matches + // any combined id generated for a char or a glyph. SkTDArray fGlyphArray; SkChunkAlloc fGlyphAlloc; -- 2.34.1