WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
209987
Use-after-move of `formState` in WebCore::PolicyChecker::checkNavigationPolicy()
https://bugs.webkit.org/show_bug.cgi?id=209987
Summary
Use-after-move of `formState` in WebCore::PolicyChecker::checkNavigationPolicy()
David Kilzer (:ddkilzer)
Reported
2020-04-03 14:23:09 PDT
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)); }
Attachments
Patch
(2.08 KB, patch)
2020-04-03 16:48 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
David Kilzer (:ddkilzer)
Comment 1
2020-04-03 14:32:00 PDT
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
Comment 2
2020-04-03 15:36:46 PDT
<
rdar://problem/61278558
>
Chris Dumez
Comment 3
2020-04-03 15:46:13 PDT
I think Alex found it a few days ago too.
Chris Dumez
Comment 4
2020-04-03 15:48:22 PDT
(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
Comment 5
2020-04-03 15:55:29 PDT
Okay, back to normal component.
Chris Dumez
Comment 6
2020-04-03 15:58:51 PDT
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
Comment 7
2020-04-03 16:46:57 PDT
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
Comment 8
2020-04-03 16:48:49 PDT
Created
attachment 395418
[details]
Patch
EWS
Comment 9
2020-04-03 17:16:09 PDT
Committed
r259522
: <
https://trac.webkit.org/changeset/259522
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 395418
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug