<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[206552] trunk/Source/WTF</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/206552">206552</a></dd>
<dt>Author</dt> <dd>mark.lam@apple.com</dd>
<dt>Date</dt> <dd>2016-09-28 14:30:27 -0700 (Wed, 28 Sep 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>Fix race condition in StringView's UnderlyingString lifecycle management.
https://bugs.webkit.org/show_bug.cgi?id=162702
Reviewed by Geoffrey Garen.
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.
This race condition was found when running layout tests fetch/fetch-worker-crash.html
and storage/indexeddb/modern/opendatabase-versions.html when CHECK_STRINGVIEW_LIFETIME
is enabled. This issue resulted in those tests crashing due to a use-after-free.
* wtf/text/StringView.cpp:
(WTF::StringView::adoptUnderlyingString):
(WTF::StringView::setUnderlyingString):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWTFChangeLog">trunk/Source/WTF/ChangeLog</a></li>
<li><a href="#trunkSourceWTFwtftextStringViewcpp">trunk/Source/WTF/wtf/text/StringView.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWTFChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/ChangeLog (206551 => 206552)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/ChangeLog 2016-09-28 21:27:29 UTC (rev 206551)
+++ trunk/Source/WTF/ChangeLog 2016-09-28 21:30:27 UTC (rev 206552)
</span><span class="lines">@@ -1,3 +1,83 @@
</span><ins>+2016-09-28 Mark Lam <mark.lam@apple.com>
+
+ Fix race condition in StringView's UnderlyingString lifecycle management.
+ https://bugs.webkit.org/show_bug.cgi?id=162702
+
+ Reviewed by Geoffrey Garen.
+
+ 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.
+
+ This race condition was found when running layout tests fetch/fetch-worker-crash.html
+ and storage/indexeddb/modern/opendatabase-versions.html when CHECK_STRINGVIEW_LIFETIME
+ is enabled. This issue resulted in those tests crashing due to a use-after-free.
+
+ * wtf/text/StringView.cpp:
+ (WTF::StringView::adoptUnderlyingString):
+ (WTF::StringView::setUnderlyingString):
+
</ins><span class="cx"> 2016-09-28 Brent Fulgham <bfulgham@apple.com>
</span><span class="cx">
</span><span class="cx"> Correct 'safeCast' implementation
</span></span></pre></div>
<a id="trunkSourceWTFwtftextStringViewcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/text/StringView.cpp (206551 => 206552)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WTF/wtf/text/StringView.cpp 2016-09-28 21:27:29 UTC (rev 206551)
+++ trunk/Source/WTF/wtf/text/StringView.cpp 2016-09-28 21:30:27 UTC (rev 206552)
</span><span class="lines">@@ -1,6 +1,6 @@
</span><span class="cx"> /*
</span><span class="cx">
</span><del>-Copyright (C) 2014 Apple Inc. All rights reserved.
</del><ins>+Copyright (C) 2014, 2016 Apple Inc. All rights reserved.
</ins><span class="cx">
</span><span class="cx"> Redistribution and use in source and binary forms, with or without
</span><span class="cx"> modification, are permitted provided that the following conditions
</span><span class="lines">@@ -236,9 +236,9 @@
</span><span class="cx"> void StringView::adoptUnderlyingString(UnderlyingString* underlyingString)
</span><span class="cx"> {
</span><span class="cx"> if (m_underlyingString) {
</span><ins>+ std::lock_guard<StaticLock> lock(underlyingStringsMutex);
</ins><span class="cx"> if (!--m_underlyingString->refCount) {
</span><span class="cx"> if (m_underlyingString->isValid) {
</span><del>- std::lock_guard<StaticLock> lock(underlyingStringsMutex);
</del><span class="cx"> underlyingStrings().remove(&m_underlyingString->string);
</span><span class="cx"> }
</span><span class="cx"> delete m_underlyingString;
</span><span class="lines">@@ -264,9 +264,9 @@
</span><span class="cx"> adoptUnderlyingString(underlyingString);
</span><span class="cx"> }
</span><span class="cx">
</span><del>-void StringView::setUnderlyingString(const StringView& string)
</del><ins>+void StringView::setUnderlyingString(const StringView& otherString)
</ins><span class="cx"> {
</span><del>- UnderlyingString* underlyingString = string.m_underlyingString;
</del><ins>+ UnderlyingString* underlyingString = otherString.m_underlyingString;
</ins><span class="cx"> if (underlyingString)
</span><span class="cx"> ++underlyingString->refCount;
</span><span class="cx"> adoptUnderlyingString(underlyingString);
</span></span></pre>
</div>
</div>
</body>
</html>