212441 2020-05-27 15:40:01 -0700 [WPE][GTK] GVariant decoding must copy the serialized data 2020-05-28 16:45:04 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build PC Linux RESOLVED FIXED P2 Normal --- 1 mcatanzaro mcatanzaro bugs-noreply cgarcia lmoura mcatanzaro oldest_to_newest 1656638 0 mcatanzaro 2020-05-27 15:40:01 -0700 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. 1656640 1 mcatanzaro 2020-05-27 15:42:14 -0700 BTW: this was introduced in r251181 1656644 2 400395 mcatanzaro 2020-05-27 15:46:30 -0700 Created attachment 400395 Patch 1656781 3 400395 cgarcia 2020-05-28 00:51:32 -0700 Comment on attachment 400395 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. 1656843 4 mcatanzaro 2020-05-28 07:36:55 -0700 (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. 1656846 5 400456 mcatanzaro 2020-05-28 07:45:55 -0700 Created attachment 400456 Patch for landing 1656859 6 ews-feeder 2020-05-28 08:28:31 -0700 Committed r262242: <https://trac.webkit.org/changeset/262242> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400456. 1657014 7 lmoura 2020-05-28 14:20:50 -0700 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*} 1657015 8 aperez 2020-05-28 14:22:06 -0700 (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 🤔️ 1657089 9 lmoura 2020-05-28 16:45:04 -0700 Added a build fix in r262274 400395 2020-05-27 15:46:30 -0700 2020-05-28 07:45:53 -0700 Patch bug-212441-20200527174629.patch text/plain 2489 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjYyMTk5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGMxOTgwMzJmNGJhOTA2NmFj OGNhNTM4ZmRkZmNhOGM2ZTZkMzcwYmUuLjZmZmY5N2NkMjA5MGE3M2ViNGUxMmUzOWNkZTExY2M2 YmY1MDUwMDMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjIgQEAKKzIwMjAtMDUtMjcgIE1pY2hhZWwg Q2F0YW56YXJvICA8bWNhdGFuemFyb0Bnbm9tZS5vcmc+CisKKyAgICAgICAgW1dQRV1bR1RLXSBH VmFyaWFudCBkZWNvZGluZyBtdXN0IGNvcHkgdGhlIHNlcmlhbGl6ZWQgZGF0YQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjEyNDQxCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSSB0cmFja2VkIHRoaXMgZG93 biB0byBBcmd1bWVudENvZGVyc0dMaWIuY3BwLiBUaGUgcHJvYmxlbSBpcyB0aGF0IHdlIGNvbnN0 cnVjdCBhIEdWYXJpYW50CisgICAgICAgIHVzaW5nIGdfdmFyaWFudF9uZXdfZnJvbV9kYXRhKCks IHdoaWNoIGRvZXMgbm90IGNvcHkgb3IgdGFrZSBvd25lcnNoaXAgb2YgdGhlIGRhdGEsIHNvIGhl cmUKKyAgICAgICAgd2UgYWNjaWRlbnRhbGx5IGNyZWF0ZSB0aGUgR1ZhcmlhbnQgdXNpbmcgZGF0 YSB3ZSBkb24ndCBvd24uIChIZXJlLCB0aGUgZGF0YSBpcyBvd25lZCBieSB0aGUKKyAgICAgICAg RGVjb2RlciBpdHNlbGYgaW4gaXRzIGludGVybmFsIG1fYnVmZmVyLikgQW55d2F5LCB0aGlzIGlz IGZpeGFibGUgYnkgbWFudWFsbHkgY29weWluZyBhbmQKKyAgICAgICAgZnJlZWluZyBpdCB3aXRo IHRoZSBHRGVzdHJveU5vdGlmeSBwYXJhbWV0ZXIsIGJ1dCBpdCdzIGVhc2llciB0byBzd2l0Y2gg dG8KKyAgICAgICAgZ192YXJpYW50X25ld19mcm9tX2J5dGVzKCkgYmVjYXVzZSBHQnl0ZXMgdGFr ZXMgb3duZXJzaGlwIHdoZW4gY29uc3RydWN0ZWQuCisKKyAgICAgICAgQm9udXMgcHJvYmxlbTog dGhlIEdWYXJpYW50IGl0c2VsZiBpcyBsZWFrZWQgYmVjYXVzZSB3ZSBhcmUgbWlzc2luZyBhZG9w dEdSZWYuCisKKyAgICAgICAgKiBTaGFyZWQvZ2xpYi9Bcmd1bWVudENvZGVyc0dMaWIuY3BwOgor ICAgICAgICAoSVBDOjpkZWNvZGUpOgorCiAyMDIwLTA1LTI3ICBDaHJpcyBEdW1leiAgPGNkdW1l ekBhcHBsZS5jb20+CiAKICAgICAgICAgQ3Jhc2ggdW5kZXIgV2ViTm90aWZpY2F0aW9uTWFuYWdl cjo6cmVtb3ZlTm90aWZpY2F0aW9uRnJvbUNvbnRleHRNYXAoKQpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYktpdC9TaGFyZWQvZ2xpYi9Bcmd1bWVudENvZGVyc0dMaWIuY3BwIGIvU291cmNlL1dlYktp dC9TaGFyZWQvZ2xpYi9Bcmd1bWVudENvZGVyc0dMaWIuY3BwCmluZGV4IGJhZDM2ZDk3MDIxNzYz MzNhMTdkMmZmMTBhNmUxOTRjOGI0Mjc3YmEuLjI5ZTQ1MTJlNWNiOThiNmJlMmQ1MzhiNGZmYTcw Y2Y2YzgxZWFjMzcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvU2hhcmVkL2dsaWIvQXJndW1l bnRDb2RlcnNHTGliLmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L1NoYXJlZC9nbGliL0FyZ3VtZW50 Q29kZXJzR0xpYi5jcHAKQEAgLTI4LDYgKzI4LDcgQEAKIAogI2luY2x1ZGUgIkRhdGFSZWZlcmVu Y2UuaCIKICNpbmNsdWRlIDxnbGliLmg+CisjaW5jbHVkZSA8d3RmL2dsaWIvR1VuaXF1ZVB0ci5o PgogI2luY2x1ZGUgPHd0Zi90ZXh0L0NTdHJpbmcuaD4KIAogbmFtZXNwYWNlIElQQyB7CkBAIC01 OSwxMCArNjAsOSBAQCBPcHRpb25hbDxHUmVmUHRyPEdWYXJpYW50Pj4gZGVjb2RlKERlY29kZXIm IGRlY29kZXIpCiAgICAgaWYgKCFkZWNvZGVyLmRlY29kZShkYXRhKSkKICAgICAgICAgcmV0dXJu IFdURjo6bnVsbG9wdDsKIAotICAgIGF1dG8qIHZhcmlhbnRUeXBlID0gZ192YXJpYW50X3R5cGVf bmV3KHZhcmlhbnRUeXBlU3RyaW5nLmRhdGEoKSk7Ci0gICAgR1JlZlB0cjxHVmFyaWFudD4gdmFy aWFudCA9IGdfdmFyaWFudF9uZXdfZnJvbV9kYXRhKHZhcmlhbnRUeXBlLCBkYXRhLmRhdGEoKSwg ZGF0YS5zaXplKCksIEZBTFNFLCBudWxscHRyLCBudWxscHRyKTsKLSAgICBnX3ZhcmlhbnRfdHlw ZV9mcmVlKHZhcmlhbnRUeXBlKTsKLSAgICByZXR1cm4gdmFyaWFudDsKKyAgICBHVW5pcXVlUHRy PEdWYXJpYW50VHlwZT4gdmFyaWFudFR5cGUoZ192YXJpYW50X3R5cGVfbmV3KHZhcmlhbnRUeXBl U3RyaW5nLmRhdGEoKSkpOworICAgIEdSZWZQdHI8R0J5dGVzPiBieXRlcyA9IGFkb3B0R1JlZihn X2J5dGVzX25ldyhkYXRhLmRhdGEoKSwgZGF0YS5zaXplKCkpKTsKKyAgICByZXR1cm4gYWRvcHRH UmVmKGdfdmFyaWFudF9uZXdfZnJvbV9ieXRlcyh2YXJpYW50VHlwZS5nZXQoKSwgYnl0ZXMuZ2V0 KCksIEZBTFNFKSk7CiB9CiAKIH0gLy8gbmFtZXNwYWNlIElQQwo= 400456 2020-05-28 07:45:55 -0700 2020-05-28 08:28:32 -0700 Patch for landing bug-212441-20200528094555.patch text/plain 2400 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjYyMjM5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDhiNDQxYzRjMDNmMTI1MDM3 ZmJkMDljZThmMDY4NmUzOTY1ZGQ4YzcuLmFlMjZmMmNlMTM5NzM2YjFkZTRhY2E5ZWZhYzU4ZDgz YzgxZWJmMjkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjAgQEAKKzIwMjAtMDUtMjggIE1pY2hhZWwg Q2F0YW56YXJvICA8bWNhdGFuemFyb0Bnbm9tZS5vcmc+CisKKyAgICAgICAgW1dQRV1bR1RLXSBH VmFyaWFudCBkZWNvZGluZyBtdXN0IGNvcHkgdGhlIHNlcmlhbGl6ZWQgZGF0YQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjEyNDQxCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgQ2FybG9zIEdhcmNpYSBDYW1wb3MuCisKKyAgICAgICAgSSB0cmFja2VkIHRo aXMgZG93biB0byBBcmd1bWVudENvZGVyc0dMaWIuY3BwLiBUaGUgcHJvYmxlbSBpcyB0aGF0IHdl IGNvbnN0cnVjdCBhIEdWYXJpYW50CisgICAgICAgIHVzaW5nIGdfdmFyaWFudF9uZXdfZnJvbV9k YXRhKCksIHdoaWNoIGRvZXMgbm90IGNvcHkgb3IgdGFrZSBvd25lcnNoaXAgb2YgdGhlIGRhdGEs IHNvIGhlcmUKKyAgICAgICAgd2UgYWNjaWRlbnRhbGx5IGNyZWF0ZSB0aGUgR1ZhcmlhbnQgdXNp bmcgZGF0YSB3ZSBkb24ndCBvd24uIChIZXJlLCB0aGUgZGF0YSBpcyBvd25lZCBieSB0aGUKKyAg ICAgICAgRGVjb2RlciBpdHNlbGYgaW4gaXRzIGludGVybmFsIG1fYnVmZmVyLikgQW55d2F5LCB0 aGlzIGlzIGZpeGFibGUgYnkgbWFudWFsbHkgY29weWluZyBhbmQKKyAgICAgICAgZnJlZWluZyBp dCB3aXRoIHRoZSBHRGVzdHJveU5vdGlmeSBwYXJhbWV0ZXIsIGJ1dCBpdCdzIGVhc2llciB0byBz d2l0Y2ggdG8KKyAgICAgICAgZ192YXJpYW50X25ld19mcm9tX2J5dGVzKCkgYmVjYXVzZSBHQnl0 ZXMgdGFrZXMgb3duZXJzaGlwIHdoZW4gY29uc3RydWN0ZWQuCisKKyAgICAgICAgKiBTaGFyZWQv Z2xpYi9Bcmd1bWVudENvZGVyc0dMaWIuY3BwOgorICAgICAgICAoSVBDOjpkZWNvZGUpOgorCiAy MDIwLTA1LTI4ICBZb3Vlbm4gRmFibGV0ICA8eW91ZW5uQGFwcGxlLmNvbT4KIAogICAgICAgICBS ZWFsdGltZUluY29taW5nVmlkZW9Tb3VyY2VDb2NvYTo6T25GcmFtZSBzaG91bGQgdXNlIHZpZGVv IGZyYW1lIHRpbWVzdGFtcApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9TaGFyZWQvZ2xpYi9B cmd1bWVudENvZGVyc0dMaWIuY3BwIGIvU291cmNlL1dlYktpdC9TaGFyZWQvZ2xpYi9Bcmd1bWVu dENvZGVyc0dMaWIuY3BwCmluZGV4IGJhZDM2ZDk3MDIxNzYzMzNhMTdkMmZmMTBhNmUxOTRjOGI0 Mjc3YmEuLmYzZDUxOWJiYTAyMjYxYWRlNWU4Y2RmZjdhOGYxZjUyZGY5ODVhMDQgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9XZWJLaXQvU2hhcmVkL2dsaWIvQXJndW1lbnRDb2RlcnNHTGliLmNwcAorKysg Yi9Tb3VyY2UvV2ViS2l0L1NoYXJlZC9nbGliL0FyZ3VtZW50Q29kZXJzR0xpYi5jcHAKQEAgLTI4 LDYgKzI4LDcgQEAKIAogI2luY2x1ZGUgIkRhdGFSZWZlcmVuY2UuaCIKICNpbmNsdWRlIDxnbGli Lmg+CisjaW5jbHVkZSA8d3RmL2dsaWIvR1VuaXF1ZVB0ci5oPgogI2luY2x1ZGUgPHd0Zi90ZXh0 L0NTdHJpbmcuaD4KIAogbmFtZXNwYWNlIElQQyB7CkBAIC01OSwxMCArNjAsOSBAQCBPcHRpb25h bDxHUmVmUHRyPEdWYXJpYW50Pj4gZGVjb2RlKERlY29kZXImIGRlY29kZXIpCiAgICAgaWYgKCFk ZWNvZGVyLmRlY29kZShkYXRhKSkKICAgICAgICAgcmV0dXJuIFdURjo6bnVsbG9wdDsKIAotICAg IGF1dG8qIHZhcmlhbnRUeXBlID0gZ192YXJpYW50X3R5cGVfbmV3KHZhcmlhbnRUeXBlU3RyaW5n LmRhdGEoKSk7Ci0gICAgR1JlZlB0cjxHVmFyaWFudD4gdmFyaWFudCA9IGdfdmFyaWFudF9uZXdf ZnJvbV9kYXRhKHZhcmlhbnRUeXBlLCBkYXRhLmRhdGEoKSwgZGF0YS5zaXplKCksIEZBTFNFLCBu dWxscHRyLCBudWxscHRyKTsKLSAgICBnX3ZhcmlhbnRfdHlwZV9mcmVlKHZhcmlhbnRUeXBlKTsK LSAgICByZXR1cm4gdmFyaWFudDsKKyAgICBHVW5pcXVlUHRyPEdWYXJpYW50VHlwZT4gdmFyaWFu dFR5cGUoZ192YXJpYW50X3R5cGVfbmV3KHZhcmlhbnRUeXBlU3RyaW5nLmRhdGEoKSkpOworICAg IEdSZWZQdHI8R0J5dGVzPiBieXRlcyA9IGFkb3B0R1JlZihnX2J5dGVzX25ldyhkYXRhLmRhdGEo KSwgZGF0YS5zaXplKCkpKTsKKyAgICByZXR1cm4gZ192YXJpYW50X25ld19mcm9tX2J5dGVzKHZh cmlhbnRUeXBlLmdldCgpLCBieXRlcy5nZXQoKSwgRkFMU0UpOwogfQogCiB9IC8vIG5hbWVzcGFj ZSBJUEMK