Use-after-move of `formState` in WebCore::PolicyChecker::checkNavigationPolicy()

[David Kilzer (:ddkilzer)]

Use-after-move of `formState` in WebCore::PolicyChecker::checkNavigationPolicy().

The last if/else statement uses formState.get(), but that's been moved into the `decisionHandler` lambda.

void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, const ResourceResponse& redirectResponse, DocumentLoader* loader, RefPtr<FormState>&& formState, NavigationPolicyDecisionFunction&& function, PolicyDecisionMode policyDecisionMode)
{
    [...]
    FramePolicyFunction decisionHandler = [this, function = WTFMove(function), request = ResourceRequest(request), formState = WTFMove(formState), suggestedFilename = WTFMove(suggestedFilename),
         blobURLLifetimeExtension = WTFMove(blobURLLifetimeExtension), requestIdentifier, isInitialEmptyDocumentLoad] (PolicyAction policyAction, PolicyCheckIdentifier responseIdentifier) mutable {
        [...]
    };

    if (isInitialEmptyDocumentLoad) {
        // We ignore the response from the client for initial empty document loads and proceed with the load synchronously.
        m_frame.loader().client().dispatchDecidePolicyForNavigationAction(action, request, redirectResponse, formState.get(), policyDecisionMode, requestIdentifier, [](PolicyAction, PolicyCheckIdentifier) { });
        decisionHandler(PolicyAction::Use, requestIdentifier);
    } else
        m_frame.loader().client().dispatchDecidePolicyForNavigationAction(action, request, redirectResponse, formState.get(), policyDecisionMode, requestIdentifier, WTFMove(decisionHandler));
}

[David Kilzer (:ddkilzer)]

Not sure the best way to fix this.  Should WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() be changed to take a RefPtr<FormState>&& instead of a FormState* and the `decisionHandler` just make a copy of `formState` instead of moving it?

[Radar WebKit Bug Importer]

<rdar://problem/61278558>

[Chris Dumez]

I think Alex found it a few days ago too.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> I think Alex found it a few days ago too.

I believe he has a pending fix for it.

Also, I don't see why this is a security bug. The behavior of the RefPtr<> move constructor is well defined. We simply pass null the second time if it was already moved (which was intentional behavior).

Anyway, AFAICT, the current code is safe and correct. I do agree that it is bad practice though and should be written some other way (which I believe Alex is doing).

[Ryosuke Niwa]

Okay, back to normal component.

[Chris Dumez]

Should be coordinated with Alex because I think he is in the process of refactoring this code (and fixing this at the same time) I believe.

That said, a trivial fix would be to replace WTFMove(formState), std::exchange(formState, nullptr). It is equivalent given our current implementation of the RefPtr<> move constructor but looks safer.

[Alex Christensen]

I fixed a similar use-after-move in WebPageProxy::receivedNavigationPolicyDecision in r259307.  We should use std::exchange here and be well defined without changing behavior.

[Alex Christensen]

Created attachment 395418 [details]
Patch

[EWS]

Committed r259522: <https://trac.webkit.org/changeset/259522>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395418 [details].