Summary: | Use-after-move of `formState` in WebCore::PolicyChecker::checkNavigationPolicy() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | Page Loading | Assignee: | Alex Christensen <achristensen> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, beidson, bfulgham, cdumez, darin, ews-watchlist, japhet, product-security, rniwa, webkit-bug-importer, wenson_hsieh | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2020-04-03 14:23:09 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? I think Alex found it a few days ago too. (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). Okay, back to normal component. 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. 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. Created attachment 395418 [details]
Patch
Committed r259522: <https://trac.webkit.org/changeset/259522> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395418 [details]. |