17.09.2020, 09:35, "Ryosuke Niwa" <rn...@webkit.org>: > Hi all, > > I’ve been working with Geoff (ggaren) and Jan Korous from Apple's compiler > team to create a static analyzer which detects dangerous use of ref counted > objects, and we’re looking for folks who are interested in trying out this > tool and helping us refine the tool. Please let me know if you’re interested > in using this tool & try applying code changes to WebKit. Our goal is to > eventually integrate this tool into buildbot and EWS so that we can catch > dangerous code before they get committed. > > What is Dangerous? > > So what is a dangerous use of ref counted objects you may ask? It’s any use > of which we can’t trivially conclude that it doesn’t lead to a use-after-free. > > For now, we don’t detect dangerous use of non-ref counted objects including > ones that can vend WeakPtr. It’s on us, humans, to decide which objects need > to be ref counted or need to be CanMakeWeakPtr. > > Consider the following example. This code most certainly will lead to a > use-after-free of “parent” in the third line because the code doesn’t keep > the parent alive. Because isContentEditable updates style in some cases, it > could lead to arbitrary script execution which could remove the parent from > the document. > > Node* parent = element->parentElement(); > if (parent->isContentEditable()) > parent->scrollIntoView(); > > In general, relying on a complex data structure such as DOM tree to keep > RefCounted objects alive while we call a non-trivial function is not safe. > All it takes for the code to have a use-after-free is for someone to start > updating style, layout, etc… inside the function either directly or > indirectly. And we don’t want to make WebKit development really hard by > forcing anyone who modifies a function to check every caller of the function > and their callers, etc… to make sure it’s safe to do so. > > For this reason, it’s dangerous to store a raw pointer or a reference to a > ref counted object as a local variable and use it after calling a non-trivial > function. We did a similar analysis of a number of other patterns and usage > of ref counted objects in WebKit and came up with the following basic rules > for using ref counted objects in a safe manner. We’re hoping that these rules > would be eventually incorporated into our coding style guideline: > https://webkit.org/code-style-guidelines/ > > Rules for Using Ref Counted Objects > > * Every data member to a ref counted object must use either Ref, RefPtr, or > WeakPtr. webkit.NoUncountedMemberChecker > * Every ref counted base class, if it has a derived class, must define a > virtual destructor. webkit.RefCntblBaseVirtualDtor > * Every ref counted object passed to a non-trivial function as an argument > (including "this" pointer) should be stored as a Ref or RefPtr in the > caller’s local scope unless it's an argument to the caller itself by > transitive property [1]. alpha.webkit.UncountedCallArgsChecker > * Every ref counted object must be captured using Ref or RefPtr for lambda. > webkit.UncountedLambdaCapturesChecker > * Local variables - we’re still working on this > (https://reviews.llvm.org/D83259) > > Below, I've dissected each one of these rules with the real warning emitted > by the analysis tool in development. Please let me know if you have any > comments / concerns. > > ---------------------------------------- > (1) is pretty trivial. Every ref counted data member should be stored using > Ref, RefPtr, or WeakPtr since it would be not trivially obvious that life > cycles of two or more objects are correctly tied or managed together. > > ---------------------------------------- > (2) is also pretty easy to understand. In the following example, if someone > destroys an instance of B using Ref<A>, then it would result in an undefined > behavior. > > struct A : public RefCounted<A> { > Vector<int> someData; > }; > > struct B : public A { > Vector<int> otherData; > }; > > ---------------------------------------- > For (3), passing a ref counted object as a raw pointer or reference to a > function as an argument, the tool may generate a warning like this: > > Source/WebCore/html/FormAssociatedElement.cpp:181:13: warning: [WebKit] call > argument is a raw pointers to a ref-countable type > [-Wfunction-arg-ptr-to-refcntbl] > setForm(findAssociatedForm(&asHTMLElement(), originalForm.get())); > ^ > > This warns that void setForm(HTMLFormElement*) is called with the result of > findAssociatedForm, which returns HTMLFormElement* without storing it > anywhere. If setForm can somehow cause HTMLFormElement to be deleted, then > this can result in the use-after-free in setForm. > > void FormAssociatedElement::resetFormOwner() > { > RefPtr<HTMLFormElement> originalForm = m_form.get(); > setForm(findAssociatedForm(&asHTMLElement(), originalForm.get())); > HTMLElement& element = asHTMLElement(); > auto* newForm = m_form.get(); > if (newForm && newForm != originalForm && newForm->isConnected()) > element.document().didAssociateFormControl(element); > } > > Why, you may ask, we don't put HTMLFormElement* in a Ref or RefPtr in setForm > instead? This is because a lot of code has calls to a function with a local > variable. Because the callee doesn’t know whether the caller has already > stored each argument in Ref / RefPtr or not, we need to be safe and store > them again in Ref / RefPtr, resulting in an unnecessary ref-churn. > Additionally, if we took the approach of the callee being responsible for > keeping every argument alive, then every non-trivial member function of a ref > counted object must have "protectedThis" at the beginning of the function, > which would be particularly wasteful if those member functions are sometimes > called by other member functions. > > Once this rule is applied everywhere, for example, we can get rid of > “protectedThis” from our codebase because it would be redundant. Furthermore, > this rule transitively allows a raw pointer or a reference passed in as an > argument to be used as arguments to call another function without storing it > in Ref or RefPtr. > > For example, the following function satisfies this rule because both > oldDocument and newDocument are raw references passed to this function as > arguments, and therefore the caller of this function must have already stored > as Ref / RefPtr as local variables. > > void HTMLMediaElement::didMoveToNewDocument(Document& oldDocument, Document& > newDocument) > { > ASSERT_WITH_SECURITY_IMPLICATION(&document() == &newDocument); > if (m_shouldDelayLoadEvent) { > oldDocument.decrementLoadEventDelayCount(); > newDocument.incrementLoadEventDelayCount(); > } > > unregisterWithDocument(oldDocument); > registerWithDocument(newDocument); > > HTMLElement::didMoveToNewDocument(oldDocument, newDocument); > updateShouldAutoplay(); > } > > ---------------------------------------- > > For (4), capturing a ref counted object as a raw pointer or a reference, the > tool may generate a warning like this: > > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:115:74: warning: [WebKit] > captured a raw pointer to a ref-countable type > [-Wlambda-capture-ptr-to-refcntbl] > cookieManager->deleteCookie(m_owningDataStore->sessionID(), cookie, [pool > = WTFMove(pool), completionHandler = WTFMove(completionHandler)]() mutable { > ^ > This is warning about webPage object getting captured using a raw reference > in the following function: > > void HTTPCookieStore::deleteCookie(const WebCore::Cookie& cookie, > CompletionHandler<void()>&& completionHandler) > { > auto* pool = m_owningDataStore->processPoolForCookieStorageOperations(); > if (!pool) { > if (m_owningDataStore->sessionID() == > PAL::SessionID::defaultSessionID() && !cookie.session) > deleteCookieFromDefaultUIProcessCookieStore(cookie); > else > m_owningDataStore->removePendingCookie(cookie); > > RunLoop::main().dispatch([completionHandler = > WTFMove(completionHandler)] () mutable { > completionHandler(); > }); > return; > } > > auto* cookieManager = pool->supplement<WebKit::WebCookieManagerProxy>(); > cookieManager->deleteCookie(m_owningDataStore->sessionID(), cookie, [pool > = WTFMove(pool), completionHandler = WTFMove(completionHandler)]() mutable { > completionHandler(); > }); > } > > Here, we should have stored WebProcessPool in Ref instead.
Sounds great! A few questions: * Do I understand correctly that analyzer is a part of upstream clang and can work on any platform? * Does it require WebKit trunk or can work with older branches? * Any plans to detect other kinds of misuses like circular references? -- Regards, Konstantin _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
