Bug 212441 – [WPE][GTK] GVariant decoding must copy the serialized data

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

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.

[Michael Catanzaro]

BTW: this was introduced in r251181

[Michael Catanzaro]

Created attachment 400395 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 400395 [details]
Patch

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

> Source/WebKit/ChangeLog:15
> +        Bonus problem: the GVariant itself is leaked because we are missing adoptGRef.

This is not true, g_variant_new returns a floating reference that shouldn't be adopted.

> Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:65
> +    return adoptGRef(g_variant_new_from_bytes(variantType.get(), bytes.get(), FALSE));

Remove the adopt and fix the changelog before landing, please.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #3)
> This is not true, g_variant_new returns a floating reference that shouldn't
> be adopted.

Ugh, you're right! Floating refs plus GRefPtr is a trap. :/

Maybe it would make sense for adoptGRef() to do if (g_object_is_floating()) g_object_ref_sink(), so that it would be safe to use always and would only sink floating refs and otherwise not affect the refcount... dunno, something to think about.

[Michael Catanzaro]

Created attachment 400456 [details]
Patch for landing

[EWS]

Committed r262242: <https://trac.webkit.org/changeset/262242>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400456 [details].

[Lauro Moura]

Both GTK and WPE build bots are broken after this patch (but due to some other issue, they still report green, I'm investigating this).

Sample error from WPE (same error in other bots):

In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-50d0d8dd-13.cpp:7:
../../Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp: In function ‘WTF::Optional<WTF::GRefPtr<_GVariant> > IPC::decode(IPC::Decoder&)’:
../../Source/WebKit/Shared/glib/ArgumentCodersGLib.cpp:65:36: error: could not convert ‘g_variant_new_from_bytes(((const GVariantType*)variantType.std::unique_ptr<_GVariantType, WTF::GPtrDeleter<_GVariantType> >::get()), bytes.WTF::GRefPtr<_GBytes>::get(), 0)’ from ‘GVariant*’ {aka ‘_GVariant*’} to ‘WTF::Optional<WTF::GRefPtr<_GVariant> >’
   65 |     return g_variant_new_from_bytes(variantType.get(), bytes.get(), FALSE);
      |            ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                    |
      |                                    GVariant* {aka _GVariant*}

[Adrian Perez]

(In reply to Michael Catanzaro from comment #4)
> (In reply to Carlos Garcia Campos from comment #3)
> > This is not true, g_variant_new returns a floating reference that shouldn't
> > be adopted.
> 
> Ugh, you're right! Floating refs plus GRefPtr is a trap. :/
> 
> Maybe it would make sense for adoptGRef() to do if (g_object_is_floating())
> g_object_ref_sink(), so that it would be safe to use always and would only
> sink floating refs and otherwise not affect the refcount... dunno, something
> to think about.

If my memory serves me well, we discussed this as a possibility in one of
the Web engines hackfests and the conclusion was that it's a potentially
breaking change that would need manual inspection of all the callsites
where pointers are adopted to make sure the right thing is done… so it
seemed that leaving things as-is was better, and people should take care
of knowing when creating an instance results in a floating ref or not 🤔️

[Lauro Moura]

Added a build fix in r262274