[webkit-changes] [WebKit/WebKit] 6dc46a: MS Teams browser meeting IncomingAudioMediaStreamT...

[Kimmo Kinnunen]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 6dc46a71a3e1df409099e57aea286d5d8db49110
      https://github.com/WebKit/WebKit/commit/6dc46a71a3e1df409099e57aea286d5d8db49110
  Author: Kimmo Kinnunen <kkinnunen at apple.com>
  Date:   2023-06-13 (Tue, 13 Jun 2023)

  Changed paths:
    M Source/WTF/WTF.xcodeproj/project.pbxproj
    A Source/WTF/wtf/SequenceLocked.h
    M Source/WebCore/platform/audio/cocoa/CARingBuffer.cpp
    M Source/WebCore/platform/audio/cocoa/CARingBuffer.h
    M Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
    A Tools/TestWebKitAPI/Tests/WTF/SequenceLockedTest.cpp
    R Tools/TestWebKitAPI/Tests/WebCore/CARingBuffer.cpp
    A Tools/TestWebKitAPI/Tests/WebCore/CARingBufferTest.cpp

  Log Message:
  -----------
  MS Teams browser meeting IncomingAudioMediaStreamTrackRendererUnit crashes with failed: Bad bounds passed to std::clamp
https://bugs.webkit.org/show_bug.cgi?id=257892
rdar://110382833

Reviewed by Andy Estes.

CARingBuffer start, end frame bounds were used in non-thread safe
fashion with a ad hoc ringbuffer:
  - write buffer[i] = {start, end} in non-atomic way to index i
  - atomic write index i

  - atomic load index i
  - read bounds from buffer[i]

The buffer was 32 entry ring buffer.
Presumably this was done to avoid needing 128-bit atomic primitives.

The writer might wraparound the ring buffer between the index load
and bounds load. The loads would end up happening in relaxed manner from
memory that is updated in relaxed manner during the load.
For each load, writer might wrap around the ring buffer arbitrary number
of times.

This has two problems:
- Since the bounds value load is non-atomic, it might load a value from
the distant future writes, and then subsequent later loads might load
values from near future. This would mean that strictly monotonic
increases in bounds would appear to be non-monotonic, essentially
random.
- Since the start and end loads are distinct, one load, say start, might
come from distant future write, where as the other load, end, might come
from near future write. This would mean that start might be larger than
end. This would cause the crash.

The bug contains the simpilified C++ reproductions of the problems.

Instead, adapt SequenceLocked from abseil sequence_lock.h. Seqlock is
an algorithm designed for atomic updates of larger data sections.

* Source/WTF/WTF.xcodeproj/project.pbxproj:
* Source/WTF/wtf/SequenceLocked.h: Added.
(WTF::SequenceLocked::load const):
(WTF::SequenceLocked::store):
(WTF::SequenceLocked::relaxedCopyFromAtomic):
(WTF::SequenceLocked::relaxedCopyToAtomic):
* Source/WebCore/platform/audio/cocoa/CARingBuffer.cpp:
(WebCore::CARingBuffer::setTimeBounds):
(WebCore::CARingBuffer::getFetchTimeBounds):
* Source/WebCore/platform/audio/cocoa/CARingBuffer.h:
* Tools/TestWebKitAPI/Tests/WebCore/CARingBuffer.cpp:
(TestWebKitAPI::TEST_F):

Canonical link: https://commits.webkit.org/265117@main