[Webkit-unassigned] [Bug 212441] New: [WPE][GTK] GVariant decoding must copy the serialized data

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=212441

            Bug ID: 212441
           Summary: [WPE][GTK] GVariant decoding must copy the serialized
                    data
           Product: WebKit
           Version: WebKit Nightly Build
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: WebKitGTK
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: mcatanzaro at gnome.org
                CC: bugs-noreply at webkitgtk.org

Moving from https://gitlab.gnome.org/GNOME/epiphany/-/issues/1175:

==249329== Thread 1:
==249329== Invalid read of size 1
==249329==    at 0x48425C0: __memcpy_chk (vg_replace_strmem.c:1595)
==249329==    by 0x544025E: UnknownInlinedFun (string_fortified.h:34)
==249329==    by 0x544025E: gvs_read_unaligned_le (gvariant-serialiser.c:582)
==249329==    by 0x544025E: gvs_tuple_get_child (gvariant-serialiser.c:941)
==249329==    by 0x54409C8: g_variant_serialised_get_child (gvariant-serialiser.c:1401)
==249329==    by 0x543B64B: g_variant_get_child_value (gvariant-core.c:1077)
==249329==    by 0x5438613: g_variant_valist_get (gvariant.c:5269)
==249329==    by 0x543977F: g_variant_get_va (gvariant.c:5498)
==249329==    by 0x54399DF: g_variant_get (gvariant.c:5445)
==249329==    by 0x48D80F9: password_manager_query_finished_cb (ephy-web-view.c:2404)
==249329==    by 0x5891515: retrieve_secret_cb (ephy-password-manager.c:614)
==249329==    by 0x521FE29: g_task_return_now (gtask.c:1214)
==249329==    by 0x5220A1C: g_task_return.part.0 (gtask.c:1283)
==249329==    by 0xB0DD27F: on_retrieve_load (secret-item.c:1264)
==249329==  Address 0x40b95915 is 149 bytes inside a block of size 184 free'd
==249329==    at 0x483B9F5: free (vg_replace_malloc.c:540)
==249329==    by 0x5400A8C: g_free (gmem.c:195)
==249329==    by 0x54196BF: g_slice_free1 (gslice.c:1135)
==249329==    by 0x5380778: g_type_free_instance (gtype.c:1946)
==249329==    by 0x53F6AF2: g_source_callback_unref (gmain.c:1640)
==249329==    by 0x53F6AF2: g_source_callback_unref (gmain.c:1633)
==249329==    by 0x53F858B: g_source_destroy_internal (gmain.c:1309)
==249329==    by 0x53FA877: g_main_dispatch (gmain.c:3333)
==249329==    by 0x53FA877: g_main_context_dispatch (gmain.c:3974)
==249329==    by 0x53FAB57: g_main_context_iterate.constprop.0 (gmain.c:4047)
==249329==    by 0x53FAC22: g_main_context_iteration (gmain.c:4108)
==249329==    by 0x524E70C: g_application_run (gapplication.c:2559)
==249329==    by 0x10D060: main (ephy-main.c:427)
==249329==  Block was alloc'd at
==249329==    at 0x483A809: malloc (vg_replace_malloc.c:309)
==249329==    by 0x5400998: g_malloc (gmem.c:102)
==249329==    by 0x5418F31: g_slice_alloc (gslice.c:1024)
==249329==    by 0x541959D: g_slice_alloc0 (gslice.c:1050)
==249329==    by 0x53803B8: g_type_create_instance (gtype.c:1849)
==249329==    by 0x5366204: g_object_new_internal (gobject.c:1937)
==249329==    by 0x53676AC: g_object_new_with_properties (gobject.c:2105)
==249329==    by 0x5368330: g_object_new (gobject.c:1777)
==249329==    by 0x52201E8: g_task_new (gtask.c:714)
==249329==    by 0x5277C2A: g_dbus_connection_send_message_with_reply_unlocked (gdbusconnection.c:1921)
==249329==    by 0x527BDEF: g_dbus_connection_send_message_with_reply (gdbusconnection.c:2021)
==249329==    by 0x527C1E4: g_dbus_connection_call_internal (gdbusconnection.c:5837)

The "Block was alloc'd at" and "149 bytes inside a block of size 184 free'd" are both misleading because they have nothing to do with the bug... the memory actually gets reallocated and reused, sometimes by GDBus and sometimes by cairo, before Epiphany processes the WebKitUserMessage, because it delays processing until after it does a password manager lookup.

I tracked this down to ArgumentCodersGLib.cpp. The problem is that we construct a GVariant using g_variant_new_from_data(), which does not copy or take ownership of the data, so here we accidentally create the GVariant using data we don't own. (Here, the data is owned by the Decoder itself in its internal m_buffer.) Anyway, this is fixable by manually copying and freeing it with the GDestroyNotify parameter, but it's easier to switch to g_variant_new_from_bytes() because GBytes takes ownership when constructed.

Bonus problem: the GVariant itself is leaked because we are missing adoptGRef.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200527/b8971993/attachment-0001.htm>