Title: [277232] trunk/Source/WebCore
- Revision
- 277232
- Author
- mmaxfi...@apple.com
- Date
- 2021-05-08 17:13:59 -0700 (Sat, 08 May 2021)
Log Message
Add bounds checks around calls to GlyphBuffer::stringOffsetAt()
https://bugs.webkit.org/show_bug.cgi?id=225335
<rdar://problem/75663608>
Reviewed by Simon Fraser and Geoff Garen.
We're getting crash reports that look like they're from string offsets being out-of-bounds.
These string offsets round-trip through Core Text, which is allowed to modify them, which
can end up making them out-of-bounds.
No new tests because I don't have a reproducible test case; just crash reports.
* platform/graphics/FontCascade.cpp:
(WebCore::computeUnderlineType):
* platform/graphics/GlyphBuffer.h:
(WebCore::GlyphBuffer::uncheckedStringOffsetAt const):
(WebCore::GlyphBuffer::checkedStringOffsetAt const):
(WebCore::GlyphBuffer::stringOffsetAt const): Deleted.
* platform/graphics/WidthIterator.cpp:
(WebCore::WidthIterator::applyFontTransforms):
(WebCore::WidthIterator::applyExtraSpacingAfterShaping):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (277231 => 277232)
--- trunk/Source/WebCore/ChangeLog 2021-05-08 22:35:07 UTC (rev 277231)
+++ trunk/Source/WebCore/ChangeLog 2021-05-09 00:13:59 UTC (rev 277232)
@@ -1,3 +1,27 @@
+2021-05-08 Myles C. Maxfield <mmaxfi...@apple.com>
+
+ Add bounds checks around calls to GlyphBuffer::stringOffsetAt()
+ https://bugs.webkit.org/show_bug.cgi?id=225335
+ <rdar://problem/75663608>
+
+ Reviewed by Simon Fraser and Geoff Garen.
+
+ We're getting crash reports that look like they're from string offsets being out-of-bounds.
+ These string offsets round-trip through Core Text, which is allowed to modify them, which
+ can end up making them out-of-bounds.
+
+ No new tests because I don't have a reproducible test case; just crash reports.
+
+ * platform/graphics/FontCascade.cpp:
+ (WebCore::computeUnderlineType):
+ * platform/graphics/GlyphBuffer.h:
+ (WebCore::GlyphBuffer::uncheckedStringOffsetAt const):
+ (WebCore::GlyphBuffer::checkedStringOffsetAt const):
+ (WebCore::GlyphBuffer::stringOffsetAt const): Deleted.
+ * platform/graphics/WidthIterator.cpp:
+ (WebCore::WidthIterator::applyFontTransforms):
+ (WebCore::WidthIterator::applyExtraSpacingAfterShaping):
+
2021-05-08 Ricky Mondello <rmonde...@apple.com>
Fix a typo
Modified: trunk/Source/WebCore/platform/graphics/FontCascade.cpp (277231 => 277232)
--- trunk/Source/WebCore/platform/graphics/FontCascade.cpp 2021-05-08 22:35:07 UTC (rev 277231)
+++ trunk/Source/WebCore/platform/graphics/FontCascade.cpp 2021-05-09 00:13:59 UTC (rev 277232)
@@ -1154,15 +1154,14 @@
// so we want to draw through CJK characters (on a character-by-character basis).
// FIXME: The CSS spec says this should instead be done by the user-agent stylesheet using the lang= attribute.
UChar32 baseCharacter;
- auto offsetInString = glyphBuffer.stringOffsetAt(index);
-
- GlyphBufferStringOffset textRunLength = textRun.length();
- RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(offsetInString < textRunLength);
+ auto offsetInString = glyphBuffer.checkedStringOffsetAt(index, textRun.length());
+ if (!offsetInString)
+ return GlyphUnderlineType::SkipDescenders;
if (textRun.is8Bit())
- baseCharacter = textRun.characters8()[offsetInString];
+ baseCharacter = textRun.characters8()[offsetInString.value()];
else
- U16_GET(textRun.characters16(), 0, offsetInString, textRunLength, baseCharacter);
+ U16_GET(textRun.characters16(), 0, static_cast<unsigned>(offsetInString.value()), textRun.length(), baseCharacter);
// u_getIntPropertyValue with UCHAR_IDEOGRAPHIC doesn't return true for Japanese or Korean codepoints.
// Instead, we can use the "Unicode allocation block" for the character.
Modified: trunk/Source/WebCore/platform/graphics/GlyphBuffer.h (277231 => 277232)
--- trunk/Source/WebCore/platform/graphics/GlyphBuffer.h 2021-05-08 22:35:07 UTC (rev 277231)
+++ trunk/Source/WebCore/platform/graphics/GlyphBuffer.h 2021-05-09 00:13:59 UTC (rev 277232)
@@ -74,7 +74,14 @@
Glyph glyphAt(unsigned index) const { return m_glyphs[index]; }
GlyphBufferAdvance advanceAt(unsigned index) const { return m_advances[index]; }
GlyphBufferOrigin originAt(unsigned index) const { return m_origins[index]; }
- GlyphBufferStringOffset stringOffsetAt(unsigned index) const { return m_offsetsInString[index]; }
+ GlyphBufferStringOffset uncheckedStringOffsetAt(unsigned index) const { return m_offsetsInString[index]; }
+ Optional<GlyphBufferStringOffset> checkedStringOffsetAt(unsigned index, unsigned stringLength) const
+ {
+ auto result = uncheckedStringOffsetAt(index);
+ if (result < 0 || static_cast<unsigned>(result) >= stringLength)
+ return WTF::nullopt;
+ return result;
+ }
void setInitialAdvance(GlyphBufferAdvance initialAdvance) { m_initialAdvance = initialAdvance; }
const GlyphBufferAdvance& initialAdvance() const { return m_initialAdvance; }
Modified: trunk/Source/WebCore/platform/graphics/WidthIterator.cpp (277231 => 277232)
--- trunk/Source/WebCore/platform/graphics/WidthIterator.cpp 2021-05-08 22:35:07 UTC (rev 277231)
+++ trunk/Source/WebCore/platform/graphics/WidthIterator.cpp 2021-05-09 00:13:59 UTC (rev 277232)
@@ -115,7 +115,7 @@
}
for (unsigned i = lastGlyphCount; i < glyphBufferSize; ++i) {
- auto characterIndex = glyphBuffer.stringOffsetAt(i);
+ auto characterIndex = glyphBuffer.uncheckedStringOffsetAt(i);
auto iterator = std::lower_bound(charactersTreatedAsSpace.begin(), charactersTreatedAsSpace.end(), characterIndex, [](const OriginalAdvancesForCharacterTreatedAsSpace& l, GlyphBufferStringOffset r) -> bool {
return l.stringOffset < r;
});
@@ -395,9 +395,11 @@
Vector<Optional<GlyphIndexRange>> characterIndexToGlyphIndexRange(m_run.length(), WTF::nullopt);
Vector<float> advanceWidths(m_run.length(), 0);
for (unsigned i = glyphBufferStartIndex; i < glyphBuffer.size(); ++i) {
- auto stringOffset = glyphBuffer.stringOffsetAt(i);
- advanceWidths[stringOffset] += width(glyphBuffer.advanceAt(i));
- auto& glyphIndexRange = characterIndexToGlyphIndexRange[stringOffset];
+ auto stringOffset = glyphBuffer.checkedStringOffsetAt(i, m_run.length());
+ if (!stringOffset)
+ continue;
+ advanceWidths[stringOffset.value()] += width(glyphBuffer.advanceAt(i));
+ auto& glyphIndexRange = characterIndexToGlyphIndexRange[stringOffset.value()];
if (glyphIndexRange)
glyphIndexRange->trailingGlyphIndex = i;
else
@@ -409,9 +411,8 @@
for (unsigned i = glyphBufferStartIndex; i < glyphBuffer.size(); ++i) {
// All characters' advances get stretched, except apparently tab characters...
// This doesn't make much sense, because even tab characters get letter-spacing...
- auto stringOffset = glyphBuffer.stringOffsetAt(i);
- auto character = m_run[stringOffset];
- if (character == tabCharacter)
+ auto stringOffset = glyphBuffer.checkedStringOffsetAt(i, m_run.length());
+ if (stringOffset && m_run[stringOffset.value()] == tabCharacter)
continue;
auto currentAdvance = width(glyphBuffer.advanceAt(i));
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
