Bug 223241

Summary:

Fix race condition in ConcurrentPtrHashSet.

Product:

WebKit

Reporter:

Mark Lam <mark.lam>

Component:

JavaScriptCore

Assignee:

Mark Lam <mark.lam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, cdumez, cmarcelo, ews-watchlist, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=222947

Attachments:

Description	Flags
proposed patch.	ews-feeder: commit-queue-
proposed patch.	ysuzuki: review+, ews-feeder: commit-queue-
proposed patch.	none
proposed patch.	ysuzuki: review+

Mark Lam

Reported 2021-03-16 00:36:37 PDT

There exists a race condition where ConcurrentPtrHashSet::resizeIfNecessary() may not capture an entry added by ConcurrentPtrHashSet::addSlow(). rdar://74637896

Attachments
proposed patch. (12.20 KB, patch) 2021-03-16 10:11 PDT, Mark Lam	ews-feeder: commit-queue-	Details Formatted Diff Diff
proposed patch. (12.21 KB, patch) 2021-03-16 10:16 PDT, Mark Lam	ysuzuki: review+ ews-feeder: commit-queue-	Details Formatted Diff Diff
proposed patch. (13.35 KB, patch) 2021-03-17 13:42 PDT, Mark Lam	no flags	Details Formatted Diff Diff
proposed patch. (13.35 KB, patch) 2021-03-17 13:43 PDT, Mark Lam	ysuzuki: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Mark Lam

Comment 1 2021-03-16 10:11:52 PDT

Created attachment 423349 [details] proposed patch.

Mark Lam

Comment 2 2021-03-16 10:16:51 PDT

Created attachment 423350 [details] proposed patch.

Yusuke Suzuki

Comment 3 2021-03-16 14:24:55 PDT

Comment on attachment 423350 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=423350&action=review r=me, looks good! > Source/WTF/wtf/ConcurrentPtrHashSet.cpp:157 > + // addSlow() will always start by exchangeAdd'ing 1 to the current m_table's > + // load value before checking if it exceeds its max allowed load. For the > + // real m_table, this is not an issue because at most, it will accummulate > + // up to N extra adds above max load, where N is the number of GC marker > + // threads. However, if m_table may be replaced with m_stubTable for each > + // resize operation. As a result, the cummulative error on its load value > + // may far exceed N (as specified above). To fix this, we always reset it > + // here to prevent an overflow. Note: a load of stubDefaultLoadValue means > + // that m_stubTable is full since its size is 0. > + // > + // In practice, this won't matter because we most likely won't do so many > + // resize operations such that this will get to the point of overflowing. > + // However, since resizing is not in the fast path, let's just be pedantic > + // and reset it for correctness. > + m_stubTable.load.store(Table::stubDefaultLoadValue); Should we reset it after `m_table.store(newTable.get());`?

Mark Lam

Comment 4 2021-03-16 14:33:28 PDT

Comment on attachment 423350 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=423350&action=review >> Source/WTF/wtf/ConcurrentPtrHashSet.cpp:157 >> + m_stubTable.load.store(Table::stubDefaultLoadValue); > > Should we reset it after `m_table.store(newTable.get());`? Good idea. I will do that.

Mark Lam

Comment 5 2021-03-17 13:42:24 PDT

Created attachment 423525 [details] proposed patch.

Mark Lam

Comment 6 2021-03-17 13:43:36 PDT

Created attachment 423526 [details] proposed patch.

Yusuke Suzuki

Comment 7 2021-03-17 13:56:20 PDT

Comment on attachment 423526 [details] proposed patch. r=me, make sense.

Mark Lam

Comment 8 2021-03-17 19:40:55 PDT

Thanks for the review. Landed in r274608: <http://trac.webkit.org/r274608>.

Note You need to log in before you can comment on or make changes to this bug.