<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>Bug ID</th>
<td><a class="bz_bug_link
bz_status_NEW "
title="NEW - Fix race condition in StringView's UnderlyingString lifecycle management."
href="https://bugs.webkit.org/show_bug.cgi?id=162702">162702</a>
</td>
</tr>
<tr>
<th>Summary</th>
<td>Fix race condition in StringView's UnderlyingString lifecycle management.
</td>
</tr>
<tr>
<th>Classification</th>
<td>Unclassified
</td>
</tr>
<tr>
<th>Product</th>
<td>WebKit
</td>
</tr>
<tr>
<th>Version</th>
<td>WebKit Local Build
</td>
</tr>
<tr>
<th>Hardware</th>
<td>Unspecified
</td>
</tr>
<tr>
<th>OS</th>
<td>Unspecified
</td>
</tr>
<tr>
<th>Status</th>
<td>NEW
</td>
</tr>
<tr>
<th>Severity</th>
<td>Normal
</td>
</tr>
<tr>
<th>Priority</th>
<td>P2
</td>
</tr>
<tr>
<th>Component</th>
<td>Web Template Framework
</td>
</tr>
<tr>
<th>Assignee</th>
<td>webkit-unassigned@lists.webkit.org
</td>
</tr>
<tr>
<th>Reporter</th>
<td>mark.lam@apple.com
</td>
</tr></table>
<p>
<div>
<pre>There 2 relevant functions at play:
void StringView::setUnderlyingString(const StringImpl* string)
{
UnderlyingString* underlyingString;
if (!string)
underlyingString = nullptr;
else {
std::lock_guard<StaticLock> lock(underlyingStringsMutex);
auto result = underlyingStrings().add(string, nullptr);
if (result.isNewEntry)
result.iterator->value = new UnderlyingString(*string);
else
++result.iterator->value->refCount;
underlyingString = result.iterator->value; // Point P2.
}
adoptUnderlyingString(underlyingString); // Point P5.
}
... and ...
void StringView::adoptUnderlyingString(UnderlyingString* underlyingString)
{
if (m_underlyingString) {
// Point P0.
if (!--m_underlyingString->refCount) {
if (m_underlyingString->isValid) { // Point P1.
std::lock_guard<StaticLock> lock(underlyingStringsMutex);
underlyingStrings().remove(&m_underlyingString->string); // Point P3.
}
delete m_underlyingString; // Point P4.
}
}
m_underlyingString = underlyingString;
}
Imagine the following scenario:
1. Thread T1 has been using an UnderlyingString U1, and is now done with it.
T1 runs up to point P1 in adoptUnderlyingString(), and is blocked waiting for
the underlyingStringsMutex (which is currently being held by Thread T2).
2. Context switch to Thread T2.
T2 wants to use UnderlyingString U1, and runs up to point P2 in setUnderlyingString()
and releases the underlyingStringsMutex.
Note: T2 thinks it has successfully refCounted U1, and therefore U1 is safe to use.
3. Context switch to Thread T1.
T1 acquires the underlyingStringsMutex, and proceeds to remove it from the
underlyingStrings() map (see Point P3). It thinks it has successfully done so
and proceeds to delete U1 (see Point P4).
4. Context switch to Thread T2.
T2 proceeds to use U1 (see Point P5 in setUnderlyingString()).
Note: U1 has already been freed. This is a use after free.
The fix is to acquire the underlyingStringsMutex at Point P0 in adoptUnderlyingString() instead of after P1. This ensures that the decrementing of the UnderlyingString refCount and its removal from the underlyingStrings() map is done as an atomic unit.
Note: If you look in StringView.cpp, you see another setUnderlyingString() which takes a StringView otherString. This version of setUnderlyingString() can only be called from within the same thread that created the other StringView. As a result, here, we are guaranteed that the UnderlyingString refCount is never zero, and there's no other threat of another thread trying to delete the UnderlyingString while we adopt it. Hence, we don't need to acquire the underlyingStringsMutex here.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>