[WPE] Fix wpe_view_backend ownership in WKWPE::View

• Component: WPE WebKit
• Status: RESOLVED WONTFIX

[Zan Dobersek]

[WPE] Fix wpe_view_backend ownership in WKWPE::View

[Zan Dobersek]

Created attachment 324799 [details]
Patch

[Build Bot]

Attachment 324799 [details] did not pass style-queue:


ERROR: Source/WebKit/ChangeLog:17:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 324799 [details]
Patch

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

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:95
> -    wpe_view_backend_set_backend_client(m_backend, &s_backendClient, this);
> +    wpe_view_backend_set_backend_client(m_backend.object, &s_backendClient, this);

So, if the view doesn't own the backend, nothing guarantees that the backend is not destroyed until the web view is destroyed, it would be a user error, but it can happen and would cause crashes. I think we could add a callback to the backend client to be notified when the backend is about to be destroyed, so that we can set our backend to nullptr and avoid crashes.

> Source/WebKit/UIProcess/API/wpe/WPEView.h:91
> +        bool owned { false };

Instead of a bool parameter I would use a destroy function, that will be nullptr in case the object is not owned and set to a lambda that clenaups and destroys the backend. This way once we expose this in the glib API we can add GDestroyNotify parameter to the new_with_backend() to make it easier for the user to destroy the backend when the view is destroyed if desired.

[Zan Dobersek]

Comment on attachment 324799 [details]
Patch

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

>> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:95
>> +    wpe_view_backend_set_backend_client(m_backend.object, &s_backendClient, this);
> 
> So, if the view doesn't own the backend, nothing guarantees that the backend is not destroyed until the web view is destroyed, it would be a user error, but it can happen and would cause crashes. I think we could add a callback to the backend client to be notified when the backend is about to be destroyed, so that we can set our backend to nullptr and avoid crashes.

You can do plenty of things that can cause a crash. Destroying the view backend before destroying the web view that's referencing it is one such thing. The two objects should have a matching lifetime span, but that should be ensured by the user, not us.

>> Source/WebKit/UIProcess/API/wpe/WPEView.h:91
>> +        bool owned { false };
> 
> Instead of a bool parameter I would use a destroy function, that will be nullptr in case the object is not owned and set to a lambda that clenaups and destroys the backend. This way once we expose this in the glib API we can add GDestroyNotify parameter to the new_with_backend() to make it easier for the user to destroy the backend when the view is destroyed if desired.

This seems like a special case for the GLib API, so it's better to implement this there.

[Michael Catanzaro]

Maybe I'm missing something obvious, but the easy solution is to add a refcount to wpe_view_backend and always take a ref. Why not do this?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> Maybe I'm missing something obvious, but the easy solution is to add a
> refcount to wpe_view_backend and always take a ref. Why not do this?

Our public API should not have a rule like "The two objects should have a matching lifetime span, but that should be ensured by the user, not us." That's not how GObject APIs work: it's expected that objects take references where required.

After reading our internal discussion, I see it's not quite as simple as adding a refcount to wpe_view_backend. We should discuss this further.

[Michael Catanzaro]

Comment on attachment 324799 [details]
Patch

I'm not going to use r- here as I don't want to reject this patch yet, but I do want to understand this problem a bit better before we commit something.

[Michael Catanzaro]

Comment on attachment 324799 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:95
>>> +    wpe_view_backend_set_backend_client(m_backend.object, &s_backendClient, this);
>> 
>> So, if the view doesn't own the backend, nothing guarantees that the backend is not destroyed until the web view is destroyed, it would be a user error, but it can happen and would cause crashes. I think we could add a callback to the backend client to be notified when the backend is about to be destroyed, so that we can set our backend to nullptr and avoid crashes.
> 
> You can do plenty of things that can cause a crash. Destroying the view backend before destroying the web view that's referencing it is one such thing. The two objects should have a matching lifetime span, but that should be ensured by the user, not us.

To implement the new WebKitWebView constructor we need in bug #178655, the wpe_view_backend must become a property of the WebKitWebView. (This is because g_object_new() must be used to construct a WebKitWebView when multiple construction parameters are needed... otherwise it would not be possible to construct, e.g., a related view that requires a custom view backend.) By convention, this means the WebKitWebView must assume ownership of the wpe_view_backend. For programmers familiar with GObject APIs, an unowned property would be a highly-unexpected footgun, and we should avoid that.

The simplest solution is to add a refcount to wpe_view_backend. I might be missing something, but I don't foresee any problems with this. wpe_view_backend_destroy() would just need to be renamed to wpe_view_backend_unref().

Alternatively, the WebKitWebView property could be a refcounted GBoxed that wraps wpe_view_backend. That would allow the refcounting to be implemented entirely at the GLib API layer, so we would not need any changes to wpe_view_backend. But it would also mean that the wpe_view_backend must not be owned anywhere in the WPE backend. I guess that probably won't work.

Zan, what do you think?

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:142
> +    // Null out the view backend clients to avoid use-after-frees.

I don't think this comment is useful.

[Michael Catanzaro]

Comment on attachment 324799 [details]
Patch

This is obsoleted by Carlos's patch in bug #178655.