Summary: | WebProcess should be terminated if invalid frameIDs are passed to the UIProcess | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Sam Weinig
2010-12-22 16:57:53 PST
Created attachment 77280 [details]
Patch
Comment on attachment 77280 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77280&action=review > WebKit2/Platform/CoreIPC/Connection.cpp:54 > + , m_inDispatchMessageCount(0) I’m not sure this count is helpful. It’s just for the assertion? > WebKit2/Platform/CoreIPC/Connection.h:65 > + if (!(assertion)) { \ > + ASSERT(assertion); \ This evaluates the assertion twice in debug builds. It’s not obvious how to avoid that. We can probably live with this for a while, since it affects the macro not all the macro call sites. > WebKit2/UIProcess/WebPageProxy.cpp:76 > +// FIXME: This should be generalized and moved to a different place. Please do remove that FIXME. > WebKit2/UIProcess/WebPageProxy.cpp:891 > process()->frameCreated(frameID, m_mainFrame.get()); There is no need for the get() here. You can pass a RefPtr to a PassRefPtr without it. Unless frameCreated takes a raw pointer. In which case I wonder why owns the subframe proxy below. > WebKit2/UIProcess/WebPageProxy.cpp:902 > + RefPtr<WebFrameProxy> subFrame = WebFrameProxy::create(this, frameID); > + > + // Add the frame to the process wide map. > + process()->frameCreated(frameID, subFrame.get()); I probably would have done this in one line without the local variable. Also, if frameCreated takes ownership, this should be release() rather than get(). Since subframe is a word, I would not capitalize the “f” in this local variable or in function names. > WebKit2/UIProcess/WebProcessProxy.cpp:47 > +template<typename HashMap> > +static inline bool isGoodMapKey(const typename HashMap::KeyType& key) Could just name this isGoodKey since the map will be right there was the template argument. As we discussed, this would be great as a public HashMap static member function. > WebKit2/UIProcess/WebProcessProxy.cpp:49 > + return key != HashTraits<typename HashMap::KeyType>::emptyValue() && !HashTraits<typename HashMap::KeyType>::isDeletedValue(key); This implementation may be as good as we can achieve, but it gives the wrong answer for maps using non-default traits. We should consider making the trait typedefs public so they can be used for things like this. |