Bug 175667 - [WPE][GTK] Ensure proper casting of data in gvariants
Summary: [WPE][GTK] Ensure proper casting of data in gvariants
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 174026 175681 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-08-17 04:24 PDT by Jacobo Aragunde Pérez
Modified: 2017-08-18 01:09 PDT (History)
15 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2017-08-17 04:36 PDT, Jacobo Aragunde Pérez
no flags Details | Formatted Diff | Diff
Patch (4.81 KB, patch)
2017-08-17 08:30 PDT, Jacobo Aragunde Pérez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jacobo Aragunde Pérez 2017-08-17 04:24:10 PDT
[WPE][GTK] Ensure proper casting of data in gvariants
Comment 1 Jacobo Aragunde Pérez 2017-08-17 04:36:57 PDT
Created attachment 318350 [details]
Patch
Comment 2 Jacobo Aragunde Pérez 2017-08-17 04:37:46 PDT
For the record, I searched and reviewed the arguments in calls to g_variant_new all around, but only found missing casts on these.
Comment 3 Michael Catanzaro 2017-08-17 05:16:29 PDT
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.
Comment 4 Jacobo Aragunde Pérez 2017-08-17 08:18:46 PDT
targetIdentifier is defined at JavaScriptCore/inspector/remote/RemoteControllableTarget.h, which is common code, should we still touch that?
Comment 5 Carlos Garcia Campos 2017-08-17 08:22:07 PDT
(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.
Comment 6 Jacobo Aragunde Pérez 2017-08-17 08:30:53 PDT
Created attachment 318369 [details]
Patch
Comment 7 Jacobo Aragunde Pérez 2017-08-17 08:31:28 PDT
Additionally reviewed calls to g_variant_get and g_variant_builder_add, correcting one more case of potential corruption.
Comment 8 Build Bot 2017-08-17 08:31:46 PDT
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
Comment 9 Michael Catanzaro 2017-08-17 08:35:07 PDT
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.
Comment 10 WebKit Commit Bot 2017-08-17 09:17:22 PDT
Comment on attachment 318369 [details]
Patch

Clearing flags on attachment: 318369

Committed r220860: <http://trac.webkit.org/changeset/220860>
Comment 11 WebKit Commit Bot 2017-08-17 09:17:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-08-17 09:17:53 PDT
<rdar://problem/33941797>
Comment 13 Michael Catanzaro 2017-08-17 13:48:52 PDT
*** Bug 175681 has been marked as a duplicate of this bug. ***
Comment 14 Carlos Garcia Campos 2017-08-18 01:09:58 PDT
*** Bug 174026 has been marked as a duplicate of this bug. ***