[3] TimingFunction shared with the scrolling thread under non-atomic refcounting

MediumWebCore threaded animationsOther

Two threads, one easing curve, and a reference count that couldn't count.

c541bf0

Rated Medium — the object is polymorphic and the first use after a premature free is a virtual dispatch, which is the shape that supports control-flow hijack. What holds it here is that winning the interleaving reliably is the hard part; the realistic baseline is a non-deterministic renderer crash.

Reference counting is only memory-safe if every acquire and release of a shared object is serialized somehow, and WebKit encodes that choice in the base class an object derives from rather than in a lock. On macOS, animations that qualify as accelerated are handed to a second thread: an AcceleratedEffect — the graph holding an animation's timing plus its keyframes — can be evaluated on the scrolling thread so animation keeps advancing independently of main-thread work, while the same effect data stays reachable from the main thread for style resolution and test introspection. Every ref-counted type inside that graph is therefore expected to use thread-safe refcounting, since two threads will take and drop transient references on it concurrently.

The angle: a page running accelerated animations that share an easing curve can drive two threads into losing a refcount update on the same object, freeing a polymorphic C++ object while a live holder is about to virtual-dispatch through it.

Since accelerated effects may be accessed both via the main thread and the scrolling thread on macOS, the TimingFunction class should use ThreadSafeRefCounted like all the other ref-counted types used by AcceleratedEffect and AcceleratedEffectValues. (The commit message notes the fix was suggested by an LLM during bug analysis and validated by the author.)

Source/WebCore/platform/animation/TimingFunction.h

LayoutTests/webanimations/threaded-animations/timing-function-threading-check.html

Patch Details

One production line changes: the base class declaration goes from class TimingFunction : public RefCounted<TimingFunction> to class TimingFunction : public ThreadSafeRefCounted<TimingFunction>, with the include swapped from <wtf/RefCounted.h> to <wtf/ThreadSafeRefCounted.h>. This makes the reference count of TimingFunction and all its subclasses — LinearTimingFunction, CubicBezierTimingFunction, StepsTimingFunction, SpringTimingFunction — atomic, and removes the owning-thread constraint RefCounted imposes. No locking, no ownership restructuring, no API change. The rest is a new layout test that starts four accelerated animations (two of them composite: "add") sharing a cubic-bezier(0.1, 0.7, 1.0, 0.1) easing, waits for acceleration, then loops 50 times over UIHelper.remoteAnimationStackForElement(target) so that main-thread timing resolution runs while the scrolling thread applies the same effects.

A refcounted object shared across threads whose refcount assumes single-thread ownership, so concurrent acquire/release can lose an update and free the object under a live holder.

Background

RefCounted<T> vs. ThreadSafeRefCounted<T>. RefCounted<T> is WTF's base class for reference-counted objects; ref()/deref() increment and decrement a plain non-atomic counter and the object is destroyed at zero. Its contract is that refcount mutation happens on a single owning thread, and debug/ASan-enabled builds include a verification check for that contract. ThreadSafeRefCounted<T> is the counterpart whose counter is mutated with atomic read-modify-write operations, so ref()/deref() are safe from any thread.

Non-atomic increment. ++count compiles to load / add / store. Two threads running it concurrently on the same location can both load the same value and both store value+1.

Ref / RefPtr and the protect(...) idiom. These smart pointers call ref() on construction and deref() on destruction. A transient local RefPtr taken around a call exists purely to keep the callee's receiver alive for the duration of that call — every such site is an acquire/release pair executing on whichever thread happens to be running.

TimingFunction. The easing curve of an animation (linear, cubic-bezier(...), steps(...), spring). It is an abstract polymorphic base with virtual type(), clone(), and operator==; transformProgress(progress, duration) maps linear progress through the curve by switching on type().

Threaded animations. Under ENABLE(THREADED_ANIMATIONS), an accelerated animation is represented as an AcceleratedEffect graph — timing plus a Vector<Keyframe>, each keyframe holding a RefPtr<TimingFunction> — evaluated on the scrolling thread. AcceleratedEffect additionally stores RefPtr<TimingFunction> m_defaultKeyframeTimingFunction. The same data stays reachable from the main thread for style, getComputedStyle, and introspection hooks such as UIHelper.remoteAnimationStackForElement().

Web Animations composite: "add". An animation whose effect is added on top of the underlying value rather than replacing it — how a stack of several simultaneously running accelerated effects on one element is built.

Analysis

This is a data race on a non-atomic reference count: a lifetime/UAF-class defect, not a logic or bounds error.

  Main thread                      Scrolling thread
  ---------------                  ----------------
  ref(): load count = 2
                                   ref(): load count = 2
  add 1 -> 3
  store 3                          add 1 -> 3
                                   store 3   <- one increment lost (should be 4)
  ...                              ...
  deref(): 3 -> 2
  deref(): 2 -> 1
                                   deref(): 1 -> 0  -> delete TimingFunction
                                   protect(...)->transformProgress()
                                     ^ virtual dispatch through freed object

In the interleaving above, a lost increment drives the count below the true number of live holders; the next deref() observes zero and destroys the TimingFunction while another thread's RefPtr/Ref still points at it. Because TimingFunction is polymorphic and transformProgress() switches on type(), the first use after free is a virtual dispatch through the freed object's vtable slot. A lost decrement is the benign direction — a leak.

The sharing is structural: keyframes hold RefPtr<TimingFunction>, the timing-resolution paths take transient protecting refs, and per the commit message the same AcceleratedEffect graph is walked from both the main thread and the macOS scrolling thread. The NeverDestroyed<Ref<...>> singletons in the same header — LinearTimingFunction::identity() and CubicBezierTimingFunction::defaultTimingFunction() — are process-wide instances that both threads can churn the count on, and their permanently-held Ref does not protect against a count already driven to zero by a lost increment.

What surfaced first was tooling, not corruption: the bug title reports that an existing layout test "may crash under ASan", i.e. the debug/ASan-enabled owning-thread verification inside RefCounted flagged the invariant violation on an already-existing threaded-animations test. The new regression test then names the concurrent path explicitly in its own comments — AnimationEffectTiming::resolve() taking a transient protecting ref on the effect's TimingFunction from both threads — and widens the window deliberately by choosing a cubic-bezier easing so transformProgress() does non-trivial work while both threads hold refs.

An attacker who reliably won the race would obtain a use-after-free on a polymorphic C++ object that is subsequently virtual-dispatched, which under favourable heap conditions is the class of primitive that supports control-flow hijack; the realistic baseline outcome is a non-deterministic renderer crash. Any escalation stays inside the WebContent sandbox and would still require a separate escape.

This vulnerability weakens memory safety within the WebContent process by breaking the lifetime invariant that a reference-counted object survives as long as any holder retains a reference. The model assumption at stake is that objects published to a second thread — here, the scrolling thread that resolves accelerated effects — have thread-safe ownership; before the fix, TimingFunction violated that while being reachable from ordinary web content.

Insight: the fix is a pure ownership-model correction with no synchronization added, which is the right call only if TimingFunction instances are immutable once shared. The source mostly supports that — LinearTimingFunction::m_points is a const Vector<Point>, and CubicBezierTimingFunction's m_x1..m_y2 are const double. But CubicBezierTimingFunction::setTimingFunctionPreset() is a public non-const mutator of m_timingFunctionPreset, and operator== reads that field, so the class is not fully immutable and atomic refcounting does not cover a concurrent read of that field against a main-thread setter. That is a distinct question the patch does not address.

Audit directions

• A refcounted type transitively published to a second thread while its base class still uses single-thread refcounting. Thread-safety of ownership is a property of the whole reachable object graph, not just the root that was reviewed. Narrow: enumerate every type reachable from AcceleratedEffect and AcceleratedEffectValues in Source/WebCore/platform/animation/ and check each base class — AcceleratedEffectValues.h alone holds RefPtr<TransformOperation> for translate/scale/rotate plus TransformOperations, FilterOperations, AcceleratedEffectOffsetPath, and TransformOperationData, and AcceleratedEffect itself derives from RefCountedAndCanMakeWeakPtr, not a thread-safe base. Wider: any object graph crossed by a serialization/handoff boundary rather than a lock — IPC-decoded WebCore objects handed to a non-main thread, display-list and GraphicsLayer payloads passed to the compositing/scrolling thread, media/AudioWorklet parameter objects. Widest: ownership-model correctness is transitive through pointer members — the same split exists as base::RefCounted vs. RefCountedThreadSafe, and as Rust's Rc vs. Arc where the compiler enforces mechanically what C++ leaves to review. Code-review tell: a thread-safe (or cross-thread-handed) container holding a smart-pointer member whose pointee's base class is the non-atomic variant.
• Transient protecting refs as the acquire/release sites a second thread can interleave with. Grep Source/WebCore/platform/animation/ and Source/WebCore/animation/ for the protect(...) idiom and for locally-constructed RefPtr/Ref around virtual calls, then check each receiver type's base class. The caller context shows this at AnimationEffectTiming.cpp (protect(timingFunction)->transformProgress(...)), KeyframeInterpolation.cpp (if (RefPtr timingFunction = timingFunctionForKeyframe(...))), and ScrollAnimationSmooth.cpp (protect(m_timingFunction)->transformProgress(...) — note this one already runs on the scrolling side). Every protect(...) on a RefCounted-derived type is a candidate; the wider tell is any short-lived smart pointer taken purely to guard a call.
• Atomic refcount mistaken for a thread-safe object. ThreadSafeRefCounted protects only the count. Starting point in this file: trace all callers of CubicBezierTimingFunction::setTimingFunctionPreset() and confirm none can run after the owning AcceleratedEffect has been handed to the scrolling thread, since operator== and transformProgress()'s type() switch read state on the other thread. Wider: apply the same question to every WebCore type converted to a thread-safe base without gaining a lock. Widest: atomic refcount means safe lifetime, not safe state — Arc<T> is inert without an inner Mutex/RwLock, and shared_ptr gives an atomic control block over a completely unsynchronized payload. Code-review tell: a class deriving from ThreadSafeRefCounted that still exposes public non-const setters and has no Lock member.
• Process-wide singleton instances of newly-thread-safe types, where a lost refcount update is far more valuable than on a per-animation object. Start with LinearTimingFunction::identity() and CubicBezierTimingFunction::defaultTimingFunction(), both static NeverDestroyed<Ref<...>>, and grep WebCore for the static NeverDestroyed<Ref< shape generally: NeverDestroyed prevents static-destructor teardown but does not make the refcount unreachable, so the object can still be destroyed by a deref() that observes zero. Establishing that a count can actually be driven to zero requires reasoning about all sharing threads — treat this as a candidate list for TSan-instrumented runs rather than something a grep resolves.