Bug 173803 – Assert that callback ID is not 0 or -1 during encoding and decoding

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

To diagnose a hang inside the m_callbacks in WebPageProxy, assert that callbackID is not 0 or -1 in IPC message encode/decode code

[Ryosuke Niwa]

Created attachment 313771 [details]
Adds assertion

[Ryosuke Niwa]

Created attachment 313773 [details]
Fixed iOS build

[Ryosuke Niwa]

Created attachment 313774 [details]
WPE build fix attempt

[Ryosuke Niwa]

Created attachment 313775 [details]
Another WPE build fix attempt

[Ryosuke Niwa]

Created attachment 313776 [details]
Yet another WPE build fix attempt

[Ryosuke Niwa]

Created attachment 313777 [details]
More GTK+ and WPE build fixes

[Ryosuke Niwa]

Created attachment 313779 [details]
Another GTK+ and WPE build fixes

[Ryosuke Niwa]

Created attachment 313780 [details]
Yet another GTK+ and WPE build fixes

[Ryosuke Niwa]

Created attachment 313781 [details]
Even more GTK+ and WPE build fixes

[Ryosuke Niwa]

Created attachment 313782 [details]
Another GTK+ build fix

[Radar WebKit Bug Importer]

<rdar://problem/32964323>

Comment on attachment 313782 [details]
Another GTK+ build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=313782&action=review

> Source/WebKit2/ChangeLog:47
> +        (WebKit::SpecificCallbackMap): Added. Used in WebCookieManagerProxy.

In earlier work, we replaced specific maps with generic ones. Why keep the ones in WebCookieManagerProxy separate?

> Source/WebKit2/ChangeLog:646
> +>>>>>>> .r218782

Stray conflict marker.

[Ryosuke Niwa]

(In reply to mitz from comment #12)
> Comment on attachment 313782 [details]
> Another GTK+ build fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313782&action=review
> 
> > Source/WebKit2/ChangeLog:47
> > +        (WebKit::SpecificCallbackMap): Added. Used in WebCookieManagerProxy.
> 
> In earlier work, we replaced specific maps with generic ones. Why keep the
> ones in WebCookieManagerProxy separate?

Okay, I wasn't aware of that refactoring effort. Fixed.

> > Source/WebKit2/ChangeLog:646
> > +>>>>>>> .r218782
> 
> Stray conflict marker.

Removed.

[Ryosuke Niwa]

Created attachment 313820 [details]
Updated per Dan's comments

[Chris Dumez]

We don’t really need this anymore, do we? I think adding MESSAGE_CHECKs on UI process side for hardening would likely suffice to protect against this in the future.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #15)
> We don’t really need this anymore, do we? I think adding MESSAGE_CHECKs on
> UI process side for hardening would likely suffice to protect against this
> in the future.

I think it's still useful to wrap CallbackID in a class so that we can easily add diagnostics in the future. For example, we should really be making the decode() function fail when a callbackID is not optional and it's 0 or -1.

[Geoffrey Garen]

Comment on attachment 313820 [details]
Updated per Dan's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=313820&action=review

> Source/WebKit2/Shared/OptionalCallbackID.h:62
> +    static ALWAYS_INLINE bool isValidCallbackID(uint64_t rawId)
> +    {
> +        return !HashTraits<uint64_t>::isDeletedValue(rawId);
> +    }

Why do we consider emptyValue() to be valid? Isn't that the bug we're trying to catch?

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #17)
> Comment on attachment 313820 [details]
> Updated per Dan's comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313820&action=review
> 
> > Source/WebKit2/Shared/OptionalCallbackID.h:62
> > +    static ALWAYS_INLINE bool isValidCallbackID(uint64_t rawId)
> > +    {
> > +        return !HashTraits<uint64_t>::isDeletedValue(rawId);
> > +    }
> 
> Why do we consider emptyValue() to be valid? Isn't that the bug we're trying
> to catch?

For OptionalCallbackID, we use 0 to mean it's not set. I have a separate release assertion when the value is used like WTF::Optional.

[Brady Eidson]

Comment on attachment 313820 [details]
Updated per Dan's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=313820&action=review

> Source/WebKit2/Shared/CallbackID.h:35
> +namespace WTF {
> +
> +struct CallbackIDHash;
> +
> +}

I don't usually see empty lines between namespace and forward decls like this.

> Source/WebKit2/UIProcess/GenericCallback.h:209
> +    // FIXME: Every caller should pass in activityToken
> +    template<typename... T>
> +    CallbackID put(Function<void(T...)>&& function)
> +    {
> +        auto callback = GenericCallbackType<sizeof...(T), T...>::type::create(WTFMove(function));
> +        return put(WTFMove(callback));
> +    }

Is it okay to punt on this FIXME right now?

[Ryosuke Niwa]

(In reply to Brady Eidson from comment #19)
> Comment on attachment 313820 [details]
> Updated per Dan's comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313820&action=review
> 
> > Source/WebKit2/Shared/CallbackID.h:35
> > +namespace WTF {
> > +
> > +struct CallbackIDHash;
> > +
> > +}
> 
> I don't usually see empty lines between namespace and forward decls like
> this.
> 
> > Source/WebKit2/UIProcess/GenericCallback.h:209
> > +    // FIXME: Every caller should pass in activityToken
> > +    template<typename... T>
> > +    CallbackID put(Function<void(T...)>&& function)
> > +    {
> > +        auto callback = GenericCallbackType<sizeof...(T), T...>::type::create(WTFMove(function));
> > +        return put(WTFMove(callback));
> > +    }
> 
> Is it okay to punt on this FIXME right now?

I wanted to fix it in a separate patch at least to reduce the chance of this patch introducing a new regression.

[Brady Eidson]

(In reply to Ryosuke Niwa from comment #20)
> (In reply to Brady Eidson from comment #19)
> > Comment on attachment 313820 [details]
> > Updated per Dan's comments
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=313820&action=review
> > 
> > > Source/WebKit2/Shared/CallbackID.h:35
> > > +namespace WTF {
> > > +
> > > +struct CallbackIDHash;
> > > +
> > > +}
> > 
> > I don't usually see empty lines between namespace and forward decls like
> > this.
> > 
> > > Source/WebKit2/UIProcess/GenericCallback.h:209
> > > +    // FIXME: Every caller should pass in activityToken
> > > +    template<typename... T>
> > > +    CallbackID put(Function<void(T...)>&& function)
> > > +    {
> > > +        auto callback = GenericCallbackType<sizeof...(T), T...>::type::create(WTFMove(function));
> > > +        return put(WTFMove(callback));
> > > +    }
> > 
> > Is it okay to punt on this FIXME right now?
> 
> I wanted to fix it in a separate patch at least to reduce the chance of this
> patch introducing a new regression.

Could we file a bug and include it in the comment?

[Ryosuke Niwa]

(In reply to Brady Eidson from comment #21)
> (In reply to Ryosuke Niwa from comment #20)
> > (In reply to Brady Eidson from comment #19)
> > > Comment on attachment 313820 [details]
> > > Updated per Dan's comments
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=313820&action=review
> > > 
> > > > Source/WebKit2/Shared/CallbackID.h:35
> > > > +namespace WTF {
> > > > +
> > > > +struct CallbackIDHash;
> > > > +
> > > > +}
> > > 
> > > I don't usually see empty lines between namespace and forward decls like
> > > this.
> > > 
> > > > Source/WebKit2/UIProcess/GenericCallback.h:209
> > > > +    // FIXME: Every caller should pass in activityToken
> > > > +    template<typename... T>
> > > > +    CallbackID put(Function<void(T...)>&& function)
> > > > +    {
> > > > +        auto callback = GenericCallbackType<sizeof...(T), T...>::type::create(WTFMove(function));
> > > > +        return put(WTFMove(callback));
> > > > +    }
> > > 
> > > Is it okay to punt on this FIXME right now?
> > 
> > I wanted to fix it in a separate patch at least to reduce the chance of this
> > patch introducing a new regression.
> 
> Could we file a bug and include it in the comment?

Filed https://bugs.webkit.org/show_bug.cgi?id=174007 to track this.

Thanks for the review.

[Ryosuke Niwa]

Committed r218985: <http://trac.webkit.org/changeset/218985>