Bug 175667 – [WPE][GTK] Ensure proper casting of data in gvariants

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jacobo Aragunde Pérez]

[WPE][GTK] Ensure proper casting of data in gvariants

[Jacobo Aragunde Pérez]

Created attachment 318350 [details]
Patch

[Jacobo Aragunde Pérez]

For the record, I searched and reviewed the arguments in calls to g_variant_new all around, but only found missing casts on these.

[Michael Catanzaro]

Comment on attachment 318350 [details]
Patch

In this case I'm going to question why the GVariant uses t (guint64) rather than u (guint32) if targetIdentifier is an unsigned int.

Probably targetIdentifier should be changed to use either uint32_t or uint64_t directly since it's destined to be passed over IPC.

[Jacobo Aragunde Pérez]

targetIdentifier is defined at JavaScriptCore/inspector/remote/RemoteControllableTarget.h, which is common code, should we still touch that?

[Carlos Garcia Campos]

(In reply to Jacobo Aragunde Pérez from comment #4)
> targetIdentifier is defined at
> JavaScriptCore/inspector/remote/RemoteControllableTarget.h, which is common
> code, should we still touch that?

Do not change that, at least not in this patch, let's fix the uses of gvariant, and if we need to change existing types, let's discuss it in a different bug.

[Jacobo Aragunde Pérez]

Created attachment 318369 [details]
Patch

[Jacobo Aragunde Pérez]

Additionally reviewed calls to g_variant_get and g_variant_builder_add, correcting one more case of potential corruption.

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 318369 [details]
Patch

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

Nice, thanks. Real shame we don't have static analysis to catch this sort of problem.

> Source/WebKit/UIProcess/API/glib/WebKitWebViewSessionState.cpp:174
> -    g_variant_builder_add(sessionBuilder, "d", frameState.pageScaleFactor);
> +    g_variant_builder_add(sessionBuilder, "d", static_cast<gdouble>(frameState.pageScaleFactor));

I'm a bit concerned because this affected serialized session state, but not much we can do about that.

[WebKit Commit Bot]

Comment on attachment 318369 [details]
Patch

Clearing flags on attachment: 318369

Committed r220860: <http://trac.webkit.org/changeset/220860>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33941797>

[Michael Catanzaro]

*** Bug 175681 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

*** Bug 174026 has been marked as a duplicate of this bug. ***