[webkit-changes] [WebKit/WebKit] a330ea: Potential use-after-free of the VM under ~FetchEve...

Yusuke Suzuki noreply at github.com
Thu Nov 2 14:13:16 PDT 2023 

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

  Changed paths:
    M Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp
    M Source/WebCore/workers/service/ServiceWorkerGlobalScope.h

  Log Message:
  -----------
  Potential use-after-free of the VM under ~FetchEvent()
https://bugs.webkit.org/show_bug.cgi?id=259896
rdar://113148936

Reviewed by Brent Fulgham.

The VM gets destroyed in between the call for WorkerGlobalScope::prepareForDestruction()
and the call for the WorkerGlobalScope destructor. The crash trace indicates that
the ServiceWorkerGlobalScope destructor destroys FetchEvent objects which end up needing
the VM in their destructor.

This is a speculative fix as I cannot reproduce the issue. Brady already imported the
test case at 266608 at main.

* Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:
(WebCore::ServiceWorkerGlobalScope::prepareForDestruction):
* Source/WebCore/workers/service/ServiceWorkerGlobalScope.h:

Originally-landed-as: 265870.237 at safari-7616-branch (76715edd316d). rdar://117809542
Canonical link: https://commits.webkit.org/270139@main


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

  Changed paths:
    M Source/WebKit/UIProcess/WebPageProxy.cpp
    M Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

  Log Message:
  -----------
  Main frame URL is wrong after server-side redirect to a page serving the COOP header
https://bugs.webkit.org/show_bug.cgi?id=260046
rdar://111855179

Reviewed by Brent Fulgham and Alex Christensen.

In the poc, the page is opening a popup (without opener) to the same origin URL1.
This URL1 does a server-side redirect to URL2 which serves the `COOP: same-origin`
HTTP header. After the navigation, Safari was displaying URL1 instead of URL2 in
the URL bar.

It is important to note that that 2 process-swap occur here. The first occurs when
we do the navigation to URL1 in a popup that doesn't have an opener (in the
decidePolicyForNavigationAction). The second one occurs when we receive the
COOP header from URL2 (on navigation response).

In ProvisionalPageProxy::didCreateMainFrame(), we have code which does the following:
```
if (previousMainFrame && !previousMainFrame->provisionalURL().isEmpty()) {
        // In case of a process swap after response policy, the didStartProvisionalLoad already happened but the new main frame doesn't know about it
        // so we need to tell it so it can update its provisional URL.
        m_mainFrame->didStartProvisionalLoad(previousMainFrame->provisionalURL());
    }
```

During the second process-swap, we forward the provisional URL from the committed
frame to the provisional one. This is because the didStartProvisionalLoad IPC was
handled by the committed main frame, before we decided to process-swap on resource
response later on. As a result, the provisional main frame doesn't know yet about
the provisional load and we have to let it know about it so it sets its provisional
URL.

This worked fine in the usual case where the COOP process-swap doesn't follow
another process swap. However, in this case, the provisional URL got updated by
an earlier server side redirect which got handled by a provisional frame, not the
committed one. As a result, the committed frame didn't know about the latest
provisional URL, only the original one before the server side redirect.

To address the issue, whenever a provisional main frame receives a server-side
redirect, we now let the committed main frame know about it too so that the
committed frame's provisional URL always stays up-to-date. As a result, when
ProvisionalPageProxy::didCreateMainFrame() forwards the committed frame's URL to
the new provisional frame, it is now accurate.

* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didReceiveServerRedirectForProvisionalLoadForFrameShared):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Originally-landed-as: 265870.357 at safari-7616-branch (8e677b301cae). rdar://117809466
Canonical link: https://commits.webkit.org/270140@main


  Commit: 70c12516bb62c6a7f33ab2be9895c97c055e86c1
      https://github.com/WebKit/WebKit/commit/70c12516bb62c6a7f33ab2be9895c97c055e86c1
  Author: J Pascoe <j_pascoe at apple.com>
  Date:   2023-11-02 (Thu, 02 Nov 2023)

  Changed paths:
    M Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.h
    M Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm
    M Tools/TestWebKitAPI/Tests/WebKitCocoa/SOAuthorizationTests.mm

  Log Message:
  -----------
  Check CSP and X-Frame-Options for subframe AppSSO
https://bugs.webkit.org/show_bug.cgi?id=260100
rdar://108625087

Reviewed by Alex Christensen.

Before this patch, AppSSO unconditionally sets cookies whenever
a session occurs without considering the headers in the response.
This patch starts considering CSP and X-Frame-Options for AppSSO responses.

Added API tests for this behavior.

* Source/WebKit/UIProcess/API/APIFrameInfo.h:
* Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.h:
* Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:
(WebKit::SOAuthorizationSession::complete):
(WebKit::SOAuthorizationSession::shouldInterruptLoadForXFrameOptions):
(WebKit::SOAuthorizationSession::shouldInterruptLoadForCSPFrameAncestorsOrXFrameOptions):
(): Deleted.
* Tools/TestWebKitAPI/Tests/WebKitCocoa/SOAuthorizationTests.mm:
(TestWebKitAPI::TEST):

Originally-landed-as: 265870.403 at safari-7616-branch (43c01fefcaa5). rdar://117809541
Canonical link: https://commits.webkit.org/270141@main


  Commit: 506a38b00501c414cc748abd41d53f47e329e726
      https://github.com/WebKit/WebKit/commit/506a38b00501c414cc748abd41d53f47e329e726
  Author: Yusuke Suzuki <ysuzuki at apple.com>
  Date:   2023-11-02 (Thu, 02 Nov 2023)

  Changed paths:
    A JSTests/stress/date-set-time-purify-nan.js
    M Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
    M Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

  Log Message:
  -----------
  [JSC] Purify NaN for Date#setTime DFG / FTL implementations
https://bugs.webkit.org/show_bug.cgi?id=260497
rdar://114177456

Reviewed by Mark Lam.

Date#setTime should purify NaN, otherwise, it can put arbitrary NaN boxed values, and cause type-confusion.
We can just use canonical NaN when the input is NaN.

* JSTests/stress/date-set-time-purify-nan.js: Added.
(opt):
(main):
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compileDateSet):
* Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq):

Originally-landed-as: 265870.404 at safari-7616-branch (aa32244a89e7). rdar://117809448
Canonical link: https://commits.webkit.org/270142@main


  Commit: 690ddf9c00c2d19da81d1833b1e29db2780955b6
      https://github.com/WebKit/WebKit/commit/690ddf9c00c2d19da81d1833b1e29db2780955b6
  Author: Yusuke Suzuki <ysuzuki at apple.com>
  Date:   2023-11-02 (Thu, 02 Nov 2023)

  Changed paths:
    A JSTests/stress/same-offset-different-property-name-multiple-get-by-variants.js
    M Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
    M Source/JavaScriptCore/dfg/DFGGraph.cpp
    M Source/JavaScriptCore/dfg/DFGGraph.h

  Log Message:
  -----------
  [JSC] DFG AI GetById adhoc folding should insert watchpoints for structures
https://bugs.webkit.org/show_bug.cgi?id=260678
rdar://114072069

Reviewed by Keith Miller.

For DFG AI GetById's variants, they are tuples of StructureSet and offset.
So, we should not obtain constant property just with offset since we first need to
ensure that the base object is having a structure in StructureSet.
Let's say [S0, 0] [S1, 1] variants are produced. In that case, we should not load
a value from offset 1 when object is S0. But previously we were doing that since
only thing we checked is that base is S0 or S1.
This patch just extends DFG AI GetById handling to use existing tryGetConstantProperty
mechanism with StructureSet. This properly inserts replacement watchpoints too, so that
we can guarantee that the loaded value is inferred constant (if it gets different, then
watchpoint fires). And we correctly check that the current object's structure is meeting
the requirement against *variant*'s structure set.

* JSTests/stress/same-offset-different-property-name-multiple-get-by-variants.js: Added.
(main.const.object1):
(main.const.object2):
(main.const.object3):
(main.get opt):
(main):
* Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* Source/JavaScriptCore/dfg/DFGGraph.cpp:
(JSC::DFG::Graph::inferredValueForProperty):
* Source/JavaScriptCore/dfg/DFGGraph.h:

Originally-landed-as: 265870.440 at safari-7616-branch (965be685c2ff). rdar://117809719
Canonical link: https://commits.webkit.org/270143@main


Compare: https://github.com/WebKit/WebKit/compare/3666f20911e0...690ddf9c00c2

More information about the webkit-changes mailing list