<div dir="ltr"><div dir="ltr"><div class="gmail-" style="color:rgb(0,0,0);font-family:Helvetica;font-size:12px">Hi all,</div><div class="gmail-" style="color:rgb(0,0,0);font-family:Helvetica;font-size:12px"><div class="gmail-"><br class="gmail-"></div><div class="gmail-">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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><b class="gmail-">What is Dangerous?</b></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">So <b class="gmail-">what is</b> a <i class="gmail-">dangerous</i> 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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><font face="Menlo" class="gmail-">Node* parent = element->parentElement();<br class="gmail-">if (parent->isContentEditable())<br class="gmail-">    parent->scrollIntoView();</font></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">For this reason, it’s <i class="gmail-">dangerous</i> 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: <a href="https://webkit.org/code-style-guidelines/" class="gmail-">https://webkit.org/code-style-guidelines/</a></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><b class="gmail-">Rules for Using Ref Counted Objects</b></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><ol class="gmail-MailOutline"><li class="gmail-">Every data member to a ref counted object must use either Ref, RefPtr, or WeakPtr. <a href="https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id17" class="gmail-">webkit.NoUncountedMemberChecker</a></li><li class="gmail-">Every ref counted base class, if it has a derived class, must define a virtual destructor. <a href="https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id16" class="gmail-">webkit.RefCntblBaseVirtualDtor</a></li><li class="gmail-">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]. <a href="https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id21" class="gmail-">alpha.webkit.UncountedCallArgsChecker</a></li><li class="gmail-">Every ref counted object must be captured using Ref or RefPtr for lambda. <a href="https://github.com/llvm/llvm-project/blob/master/clang/docs/analyzer/checkers.rst#id18" class="gmail-">webkit.UncountedLambdaCapturesChecker</a></li><li class="gmail-">Local variables - we’re still working on this (<a href="https://reviews.llvm.org/D83259" class="gmail-">https://reviews.llvm.org/D83259</a>)</li></ol></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><div class="gmail-"><div class="gmail-"><hr class="gmail-"></div></div><div class="gmail-">(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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><div class="gmail-"><div class="gmail-"><hr class="gmail-"></div></div><div class="gmail-">(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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">struct A : public RefCounted<A> {</div><div class="gmail-">        Vector<int> someData;</div><div class="gmail-">};</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><div class="gmail-">struct B : public A {</div><div class="gmail-">        Vector<int> otherData;</div><div class="gmail-">};</div></div></div><div class="gmail-"><br class="gmail-"></div></div><div class="gmail-"><div class="gmail-"><hr class="gmail-"><br class="gmail-"></div></div><div class="gmail-">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:</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><div class="gmail-"><font face="Menlo" class="gmail-">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]</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    setForm(findAssociatedForm(&asHTMLElement(), originalForm.get()));</font></div><div class="gmail-"><font face="Menlo" class="gmail-">            ^</font></div></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">This warns that <font face="Menlo" class="gmail-">void setForm(HTMLFormElement*)</font> 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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><div class="gmail-"><font face="Menlo" class="gmail-">void FormAssociatedElement::resetFormOwner()</font></div><div class="gmail-"><font face="Menlo" class="gmail-">{</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    RefPtr<HTMLFormElement> originalForm = m_form.get();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    <font color="#ff2600" class="gmail-">setForm(findAssociatedForm(&asHTMLElement(), originalForm.get()));</font></font></div><div class="gmail-"><font face="Menlo" class="gmail-">    HTMLElement& element = asHTMLElement();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    auto* newForm = m_form.get();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    if (newForm && newForm != originalForm && newForm->isConnected())</font></div><div class="gmail-"><font face="Menlo" class="gmail-">        element.document().didAssociateFormControl(element);</font></div><div class="gmail-"><font face="Menlo" class="gmail-">}</font></div></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">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.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><div class="gmail-"><font face="Menlo" class="gmail-">void HTMLMediaElement::didMoveToNewDocument(Document& oldDocument, Document& newDocument)</font></div><div class="gmail-"><font face="Menlo" class="gmail-">{</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    ASSERT_WITH_SECURITY_IMPLICATION(&document() == &newDocument);</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    if (m_shouldDelayLoadEvent) {</font></div><div class="gmail-"><font face="Menlo" class="gmail-">        oldDocument.decrementLoadEventDelayCount();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">        newDocument.incrementLoadEventDelayCount();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    }</font></div><div class="gmail-"><font face="Menlo" class="gmail-"><br class="gmail-"></font></div><div class="gmail-"><font face="Menlo" class="gmail-">    unregisterWithDocument(oldDocument);</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    registerWithDocument(newDocument);</font></div><div class="gmail-"><font face="Menlo" class="gmail-"><br class="gmail-"></font></div><div class="gmail-"><font face="Menlo" class="gmail-">    HTMLElement::didMoveToNewDocument(oldDocument, newDocument);</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    updateShouldAutoplay();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">}</font></div></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><div class="gmail-"><hr class="gmail-"></div></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">For (4), capturing a ref counted object as a raw pointer or a reference, the tool may generate a warning like this:</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><div class="gmail-"><div class="gmail-"><font face="Menlo" class="gmail-">Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:115:74: warning: [WebKit] captured a raw pointer to a ref-countable type [-Wlambda-capture-ptr-to-refcntbl]</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    cookieManager->deleteCookie(m_owningDataStore->sessionID(), cookie, [pool = WTFMove(pool), completionHandler = WTFMove(completionHandler)]() mutable {</font></div><div class="gmail-"><font face="Menlo" class="gmail-">                                                                         ^</font></div></div><div class="gmail-">This is warning about webPage object getting captured using a raw reference in the following function:</div></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-"><div class="gmail-"><font face="Menlo" class="gmail-">void HTTPCookieStore::deleteCookie(const WebCore::Cookie& cookie, CompletionHandler<void()>&& completionHandler)</font></div><div class="gmail-"><font face="Menlo" class="gmail-">{</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    auto* pool = m_owningDataStore->processPoolForCookieStorageOperations();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    if (!pool) {</font></div><div class="gmail-"><font face="Menlo" class="gmail-">        if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session)</font></div><div class="gmail-"><font face="Menlo" class="gmail-">            deleteCookieFromDefaultUIProcessCookieStore(cookie);</font></div><div class="gmail-"><font face="Menlo" class="gmail-">        else</font></div><div class="gmail-"><font face="Menlo" class="gmail-">            m_owningDataStore->removePendingCookie(cookie);</font></div><div class="gmail-"><font face="Menlo" class="gmail-"><br class="gmail-"></font></div><div class="gmail-"><font face="Menlo" class="gmail-">        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] () mutable {</font></div><div class="gmail-"><font face="Menlo" class="gmail-">            completionHandler();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">        });</font></div><div class="gmail-"><font face="Menlo" class="gmail-">        return;</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    }</font></div><div class="gmail-"><font face="Menlo" class="gmail-"><br class="gmail-"></font></div><div class="gmail-"><font face="Menlo" class="gmail-">    auto* cookieManager = pool->supplement<WebKit::WebCookieManagerProxy>();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    cookieManager->deleteCookie(m_owningDataStore->sessionID(), cookie, [<font color="#ff2600" class="gmail-">pool = WTFMove(pool)</font>, completionHandler = WTFMove(completionHandler)]() mutable {</font></div><div class="gmail-"><font face="Menlo" class="gmail-">        completionHandler();</font></div><div class="gmail-"><font face="Menlo" class="gmail-">    });</font></div><div class="gmail-"><font face="Menlo" class="gmail-">}</font></div></div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">Here, we should have stored WebProcessPool in Ref instead.</div><div class="gmail-"><br class="gmail-"></div><div class="gmail-">- R. Niwa</div></div></div></div>