Changeset 206552 in webkit


Ignore:
Timestamp:
Sep 28, 2016, 2:30:27 PM (9 years ago)
Author:
mark.lam@apple.com
Message:

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):

Location:
trunk/Source/WTF
Files:
2 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WTF/ChangeLog

    r206523 r206552  
     12016-09-28  Mark Lam  <mark.lam@apple.com>
     2
     3        Fix race condition in StringView's UnderlyingString lifecycle management.
     4        https://bugs.webkit.org/show_bug.cgi?id=162702
     5
     6        Reviewed by Geoffrey Garen.
     7
     8        There 2 relevant functions at play:
     9
     10        void StringView::setUnderlyingString(const StringImpl* string)
     11        {
     12            UnderlyingString* underlyingString;
     13            if (!string)
     14                underlyingString = nullptr;
     15            else {
     16                std::lock_guard<StaticLock> lock(underlyingStringsMutex);
     17                auto result = underlyingStrings().add(string, nullptr);
     18                if (result.isNewEntry)
     19                    result.iterator->value = new UnderlyingString(*string);
     20                else
     21                    ++result.iterator->value->refCount;
     22                underlyingString = result.iterator->value; // Point P2.
     23            }
     24            adoptUnderlyingString(underlyingString); // Point P5.
     25        }
     26
     27        ... and ...
     28
     29        void StringView::adoptUnderlyingString(UnderlyingString* underlyingString)
     30        {
     31            if (m_underlyingString) {
     32                // Point P0.
     33                if (!--m_underlyingString->refCount) {
     34                    if (m_underlyingString->isValid) { // Point P1.
     35                        std::lock_guard<StaticLock> lock(underlyingStringsMutex);
     36                        underlyingStrings().remove(&m_underlyingString->string); // Point P3.
     37                    }
     38                    delete m_underlyingString; // Point P4.
     39                }
     40            }
     41            m_underlyingString = underlyingString;
     42        }
     43
     44        Imagine the following scenario:
     45
     46        1. Thread T1 has been using an UnderlyingString U1, and is now done with it.
     47           T1 runs up to point P1 in adoptUnderlyingString(), and is blocked waiting for
     48           the underlyingStringsMutex (which is currently being held by Thread T2).
     49        2. Context switch to Thread T2.
     50           T2 wants to use UnderlyingString U1, and runs up to point P2 in setUnderlyingString()
     51           and releases the underlyingStringsMutex.
     52           Note: T2 thinks it has successfully refCounted U1, and therefore U1 is safe to use.
     53        3. Context switch to Thread T1.
     54           T1 acquires the underlyingStringsMutex, and proceeds to remove it from the
     55           underlyingStrings() map (see Point P3).  It thinks it has successfully done so
     56           and proceeds to delete U1 (see Point P4).
     57        4. Context switch to Thread T2.
     58           T2 proceeds to use U1 (see Point P5 in setUnderlyingString()).
     59           Note: U1 has already been freed.  This is a use after free.
     60
     61        The fix is to acquire the underlyingStringsMutex at Point P0 in adoptUnderlyingString()
     62        instead of after P1.  This ensures that the decrementing of the UnderlyingString
     63        refCount and its removal from the underlyingStrings() map is done as an atomic unit.
     64
     65        Note: If you look in StringView.cpp, you see another setUnderlyingString() which
     66        takes a StringView otherString.  This version of setUnderlyingString() can only
     67        be called from within the same thread that created the other StringView.  As a
     68        result, here, we are guaranteed that the UnderlyingString refCount is never zero,
     69        and there's no other threat of another thread trying to delete the UnderlyingString
     70        while we adopt it.  Hence, we don't need to acquire the underlyingStringsMutex
     71        here.
     72
     73        This race condition was found when running layout tests fetch/fetch-worker-crash.html
     74        and storage/indexeddb/modern/opendatabase-versions.html when CHECK_STRINGVIEW_LIFETIME
     75        is enabled.  This issue resulted in those tests crashing due to a use-after-free.
     76
     77        * wtf/text/StringView.cpp:
     78        (WTF::StringView::adoptUnderlyingString):
     79        (WTF::StringView::setUnderlyingString):
     80
    1812016-09-28  Brent Fulgham  <bfulgham@apple.com>
    282

  • trunk/Source/WTF/wtf/text/StringView.cpp

    r205893 r206552  
    11/*
    22
    3 Copyright (C) 2014 Apple Inc. All rights reserved.
     3Copyright (C) 2014, 2016 Apple Inc. All rights reserved.
    44
    55Redistribution and use in source and binary forms, with or without
    … …  
    237237{
    238238    if (m_underlyingString) {
     239        std::lock_guard<StaticLock> lock(underlyingStringsMutex);
    239240        if (!--m_underlyingString->refCount) {
    240241            if (m_underlyingString->isValid) {
    241                 std::lock_guard<StaticLock> lock(underlyingStringsMutex);
    242242                underlyingStrings().remove(&m_underlyingString->string);
    243243            }
    … …  
    265265}
    266266
    267 void StringView::setUnderlyingString(const StringView& string)
    268 {
    269     UnderlyingString* underlyingString = string.m_underlyingString;
     267void StringView::setUnderlyingString(const StringView& otherString)
     268{
     269    UnderlyingString* underlyingString = otherString.m_underlyingString;
    270270    if (underlyingString)
    271271        ++underlyingString->refCount;
Note: See TracChangeset for help on using the changeset viewer.