| Summary: | [WPE][GTK] Ensure proper casting of data in gvariants | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jacobo Aragunde Pérez <jaragunde> | ||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bburg, berto, buildbot, cgarcia, commit-queue, gustavo, jbicha, jidanni, keith_miller, mark.lam, mcatanzaro, msaboff, saam, webkit-bug-importer, zan | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Jacobo Aragunde Pérez
2017-08-17 04:24:10 PDT
Created attachment 318350 [details]
Patch
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 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.
targetIdentifier is defined at JavaScriptCore/inspector/remote/RemoteControllableTarget.h, which is common code, should we still touch that? (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. Created attachment 318369 [details]
Patch
Additionally reviewed calls to g_variant_get and g_variant_builder_add, correcting one more case of potential corruption. 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 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 on attachment 318369 [details] Patch Clearing flags on attachment: 318369 Committed r220860: <http://trac.webkit.org/changeset/220860> All reviewed patches have been landed. Closing bug. *** Bug 175681 has been marked as a duplicate of this bug. *** *** Bug 174026 has been marked as a duplicate of this bug. *** |