[webkit-changes] [WebKit/WebKit] cdb96b: WebContent may get killed due to invalid RemoteLay...

Chris Dumez noreply at github.com
Thu Nov 2 12:10:20 PDT 2023 

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: cdb96bc02a635dd676c28b4b8a7f4388a0edc9e0
      https://github.com/WebKit/WebKit/commit/cdb96bc02a635dd676c28b4b8a7f4388a0edc9e0
  Author: Chris Dumez <cdumez at apple.com>
  Date:   2023-11-02 (Thu, 02 Nov 2023)

  Changed paths:
    M Source/WebCore/platform/graphics/Region.cpp

  Log Message:
  -----------
  WebContent may get killed due to invalid RemoteLayerTreeDrawingAreaProxy_CommitLayerTree IPC message
https://bugs.webkit.org/show_bug.cgi?id=260757
rdar://113860744

Reviewed by Aditya Keerthi.

The fuzzer found a case where the RemoteLayerTreeDrawingAreaProxy_CommitLayerTree
IPC message may fail decoding because its contains an invalid IntRect. After some
investigation, I found that we didn't handle overflows in the arithmetics in
Region::Shape::bounds(), which means that we could end up with an IntRect that
had a negative width or height.

In the fuzzer case, we ended up with the following values:
minX=-2147483648, minY=3, maxX=62, maxY=2306

We would compute the width doing `62 - (-2147483648)` which would overflow and end
up with a negative width. We now use checkedDifference<int32_t>() to detect
overflows and clamp to std::numeric_limits<int32_t>::max() when it happens.

* Source/WebCore/platform/graphics/Region.cpp:
(WebCore::Region::Shape::bounds const):

Originally-landed-as: 265870.452 at safari-7616-branch (ca4f7c9b9939). rdar://117809786
Canonical link: https://commits.webkit.org/270125@main


  Commit: c180d85669a6aa81e5d3b746284999982d28baaa
      https://github.com/WebKit/WebKit/commit/c180d85669a6aa81e5d3b746284999982d28baaa
  Author: Jer Noble <jer.noble at apple.com>
  Date:   2023-11-02 (Thu, 02 Nov 2023)

  Changed paths:
    M Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp
    M Source/WebCore/Modules/mediarecorder/MediaRecorder.h

  Log Message:
  -----------
  CRASH in MediaRecorderPrivate::startRecording()
https://bugs.webkit.org/show_bug.cgi?id=260736
rdar://113544631

Reviewed by Brent Fulgham and Eric Carlson.

MediaRecorder can be destroyed before the completion handler passed to
MediaRecorderPrivate::startRecording() is called. Detect this state by
passing a WeakPtr into the completion handler lambda. Because MediaRecoder
has multiple base classes which are CanMakeWeakPtr subclasses, disambiguate
which subclass's WeakPtr implementation to use in the MediaRecoder class
declaration.

* Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:
(WebCore::MediaRecorder::startRecording):
* Source/WebCore/Modules/mediarecorder/MediaRecorder.h:

Originally-landed-as: 265870.463 at safari-7616-branch (f255dd40b82e). rdar://117809782
Canonical link: https://commits.webkit.org/270126@main


  Commit: c89614c803991659691622c0dc621ba10c9094ab
      https://github.com/WebKit/WebKit/commit/c89614c803991659691622c0dc621ba10c9094ab
  Author: Mark Lam <mark.lam at apple.com>
  Date:   2023-11-02 (Thu, 02 Nov 2023)

  Changed paths:
    M Source/WebCore/bindings/js/SerializedScriptValue.cpp

  Log Message:
  -----------
  CloneDeserializer::readBigInt() should check for overflow when reifying JSBigInt length on 32-bit platforms.
https://bugs.webkit.org/show_bug.cgi?id=260822
rdar://114547822

Reviewed by Chris Dumez.

The serialized length is a number of Uint64 elements. On 32-bit platforms, this length gets multiplied by 2 in
order to compute the actual length of the backing store to re-construct the JSBigInt.  Both the transmitted length
and the JSBigInt length is stored as uint32_t.  Hence, the 2x multiplication can theoretically result in an
overflow.  This patch adds an overflow check to handle this edge case.

Also renamed lengthInUint64 to numberOfUint64Elements.  lengthInUint64 can be misread to mean a length stored as a
uint64_t value, which is not what it actually means.  The length value is store in a uint32_t, and is a count of
the number of uint64_t sized elements.

* Source/WebCore/bindings/js/SerializedScriptValue.cpp:
(WebCore::CloneSerializer::dumpHeapBigIntData):
(WebCore::CloneDeserializer::readBigInt):

Originally-landed-as: 265870.467 at safari-7616-branch (1f9e21240663). rdar://117809900
Canonical link: https://commits.webkit.org/270127@main


  Commit: 13885ac4490643e1e00c0a4958aa48e0bad7124b
      https://github.com/WebKit/WebKit/commit/13885ac4490643e1e00c0a4958aa48e0bad7124b
  Author: Antti Koivisto <antti at apple.com>
  Date:   2023-11-02 (Thu, 02 Nov 2023)

  Changed paths:
    A LayoutTests/fast/css/style-scope-destruction-crash-expected.txt
    A LayoutTests/fast/css/style-scope-destruction-crash.html
    M Source/WebCore/rendering/PathOperation.cpp
    M Source/WebCore/rendering/PathOperation.h
    M Source/WebCore/style/StyleScope.cpp

  Log Message:
  -----------
  heap-use-after-free | Style::Scope::removeStyleSheetCandidateNode; WebCore::SVGStyleElement::~SVGStyleElement; WebCore::ContainerNode::~ContainerNode
https://bugs.webkit.org/show_bug.cgi?id=260896
rdar://114231775

Reviewed by Alan Baradlay.

* LayoutTests/fast/css/style-scope-destruction-crash-expected.txt: Added.
* LayoutTests/fast/css/style-scope-destruction-crash.html: Added.
* Source/WebCore/rendering/PathOperation.cpp:
(WebCore::ReferencePathOperation::ReferencePathOperation):
(WebCore::ReferencePathOperation::element const): Deleted.

Get rid of the unused element field. This creates a RenderStyle -> DOM ownership cycle which
allows this crash to happen.

* Source/WebCore/rendering/PathOperation.h:
* Source/WebCore/style/StyleScope.cpp:
(WebCore::Style::Scope::~Scope):

Ensure we revoke weak pointers at the start of the destructor to avoid this class of problems.

Originally-landed-as: 265870.481 at safari-7616-branch (382b02a5fc10). rdar://117809896
Canonical link: https://commits.webkit.org/270128@main


  Commit: a043d05b6d0b277940c34b8fe55c0085e797d176
      https://github.com/WebKit/WebKit/commit/a043d05b6d0b277940c34b8fe55c0085e797d176
  Author: David Kilzer <ddkilzer at apple.com>
  Date:   2023-11-02 (Thu, 02 Nov 2023)

  Changed paths:
    M Source/WebCore/WebCore.xcodeproj/project.pbxproj
    M Source/WebCore/platform/audio/SincResampler.h

  Log Message:
  -----------
  Make WebCore::SincResampler testable
https://bugs.webkit.org/show_bug.cgi?id=260933
<rdar://114732348>

Reviewed by Chris Dumez.

* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
- Make SincResampler.h a private header.
* Source/WebCore/platform/audio/SincResampler.h:
(WebCore::SincResampler::processBuffer):
- Export static class method.

Originally-landed-as: 265870.482 at safari-7616-branch (aae3b70b0eff). rdar://117809957
Canonical link: https://commits.webkit.org/270129@main


  Commit: 81e5846d415723b37a4e466ed0a80b5335e877c9
      https://github.com/WebKit/WebKit/commit/81e5846d415723b37a4e466ed0a80b5335e877c9
  Author: Chris Dumez <cdumez at apple.com>
  Date:   2023-11-02 (Thu, 02 Nov 2023)

  Changed paths:
    M LayoutTests/TestExpectations
    A LayoutTests/fast/frames/frame-append-body-child-crash-expected.txt
    A LayoutTests/fast/frames/frame-append-body-child-crash.html
    M Source/WebCore/html/HTMLBodyElement.cpp

  Log Message:
  -----------
  Crash under HTMLBodyElement::didFinishInsertingNode()
https://bugs.webkit.org/show_bug.cgi?id=260907
rdar://114177696

Reviewed by Ryosuke Niwa.

When a <body> is inserted into the document, `HTMLBodyElement::insertedIntoAncestor()`
gets called. This function would only return `InsertedIntoAncestorResult::NeedsPostInsertionCallback`
if `is<HTMLFrameElementBase>(document().ownerElement())`, causing `HTMLBodyElement::didFinishInsertingNode()`
to get called later on. We would then assume in didFinishInsertingNode() that the document's owner element
is a non-null HTMLFrameElementBase.

However, as proven by the test, DOM manipulations can happen in between the 2 calls
causing the assertion to no longer hold. To address the issue we now early return
if `is<HTMLFrameElementBase>(document().ownerElement())` is no longer true in
`HTMLBodyElement::didFinishInsertingNode()`.

In the case of the test, `document().frame()` becomes null because the frame gets
detached, causing `document().ownerElement()` to return null as well.

* LayoutTests/fast/frames/frame-append-body-child-crash-expected.txt: Added.
* LayoutTests/fast/frames/frame-append-body-child-crash.html: Added.
* Source/WebCore/html/HTMLBodyElement.cpp:
(WebCore::HTMLBodyElement::didFinishInsertingNode):

Originally-landed-as: 265870.486 at safari-7616-branch (3fc6931dcc2c). rdar://117810024
Canonical link: https://commits.webkit.org/270130@main


Compare: https://github.com/WebKit/WebKit/compare/0dadce54c06a...81e5846d4157

More information about the webkit-changes mailing list