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

diff --git a/gm/fontcache.cpp b/gm/fontcache.cpp
index d5ead8c1215d789b69f134389a9ace637c732196..0f5080e15d7a6ec4d2b3a357618b2581e4801d85 100644 (file)
--- 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 48b9815a03c9744d45c2dbe11e6559b491a682cd..abca215d0039510da7218fcf031770901967e186 100644 (file)
--- 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 3fe8f0ae9e57c69faae0cd2c9c08a58fbca78f0c..224a70a37f3f971c514775d18369a4e571dea8bd 100755 (executable)
--- 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 <kHashCount; ++i) {
+            fCharToGlyphHash[i].fID = SkGlyph::kImpossibleID;
+            fCharToGlyphHash[i].fGlyphIndex = 0;
+        }
     }
-    
+
     return &fCharToGlyphHash[ID2HashIndex(id)];
 }
 
@@ -222,7 +225,7 @@ SkGlyph* SkGlyphCache::lookupByChar(SkUnichar charCode, MetricsType type, SkFixe
     CharGlyphRec* rec = this->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 200655bb417890731dc0c01ae8aeb290faa15d5c..d0b792f273500afea83fe53ef4c314a676831bc7 100644 (file)
--- 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<SkGlyph>   fGlyphArray;
     SkChunkAlloc         fGlyphAlloc;