Hi, this PR fixes a use-after-free in codenav's go-to-file. (which often results in a crash)
## Problem: ### How to reproduce 1. Run geany from command line, to observe error messages 2. Enable codenav plugin, open some document (to allow use of go-to tool) 3. Trigger go-to-file tool (e.g. via shortcut) 4. Write `/` 5. Cancel & close dialog 6. Trigger go-to-file again 7. Write `a` -> You will likely see an assertion fail in the output, or if you are lucky geany will crash altogether: `gtk_entry_completion_set_model: assertion 'model == NULL || GTK_IS_TREE_MODEL (model)' failed` Valgrind shows an invalid read to previously freed memory: <details> ``` Invalid read of size 8 at 0x4DD5820: UnknownInlinedFun (gtkentrycompletion.c:1224) by 0x4DD5820: gtk_entry_completion_set_model (gtkentrycompletion.c:1220) by 0xE2E89F3: directory_check (goto_file.c:166) by 0x5A6B72F: g_closure_invoke (gclosure.c:834) by 0x5A9AC1A: signal_emit_unlocked_R.isra.0 (gsignal.c:3961) by 0x5A8B7A1: signal_emit_valist_unlocked (gsignal.c:3520) by 0x5A8BCAF: g_signal_emit_by_name (gsignal.c:3624) by 0x4DBAC29: end_change.lto_priv.0 (gtkentry.c:2941) by 0x4DC6576: gtk_entry_real_insert_text.lto_priv.0 (gtkentry.c:5401) by 0x5A6B72F: g_closure_invoke (gclosure.c:834) by 0x5A9AF49: signal_emit_unlocked_R.isra.0 (gsignal.c:3928) by 0x5A8B7A1: signal_emit_valist_unlocked (gsignal.c:3520) by 0x5A8BCAF: g_signal_emit_by_name (gsignal.c:3624) Address 0xd42b8e0 is 96 bytes inside a block of size 136 free'd at 0x48468CF: free (vg_replace_malloc.c:985) by 0x5A90164: g_type_free_instance (gtype.c:2023) by 0x5A7A732: g_object_unref (gobject.c:4475) by 0x48E5AC9: run_kb (keybindings.c:1334) by 0x48E5AC9: run_kb (keybindings.c:1325) by 0x48E67E4: on_key_press_event (keybindings.c:1396) by 0x4CFA6CC: _gtk_marshal_BOOLEAN__BOXED.part.0 (gtkmarshalers.c:84) by 0x5A6B72F: g_closure_invoke (gclosure.c:834) by 0x5A9A895: signal_emit_unlocked_R.isra.0 (gsignal.c:3888) by 0x5A8B094: signal_emit_valist_unlocked (gsignal.c:3533) by 0x5A8B9D6: g_signal_emit_valist (gsignal.c:3263) by 0x5A8BA93: g_signal_emit (gsignal.c:3583) by 0x4FC2CD4: gtk_widget_event_internal.part.0.lto_priv.0 (gtkwidget.c:7812) Block was alloc'd at at 0x484A993: calloc (vg_replace_malloc.c:1595) by 0x5B2651A: g_malloc0 (gmem.c:133) by 0x5A96F1B: g_type_create_instance (gtype.c:1923) by 0x5A7CB10: g_object_new_internal.part.0 (gobject.c:2603) by 0x5A7E0C6: UnknownInlinedFun (gobject.c:2600) by 0x5A7E0C6: g_object_new_with_properties (gobject.c:2766) by 0x5A7F009: g_object_new (gobject.c:2412) by 0x4E56A25: gtk_list_store_new (gtkliststore.c:426) by 0xE2E8573: build_file_list (goto_file.c:111) by 0xE2E86A7: menu_item_activate (goto_file.c:285) by 0x48E5AC9: run_kb (keybindings.c:1334) by 0x48E5AC9: run_kb (keybindings.c:1325) by 0x48E67E4: on_key_press_event (keybindings.c:1396) by 0x4CFA6CC: _gtk_marshal_BOOLEAN__BOXED.part.0 (gtkmarshalers.c:84) Invalid read of size 8 at 0x5A92C79: g_type_check_instance_is_a (gtype.c:4133) by 0x4DD5836: UnknownInlinedFun (gtkentrycompletion.c:1224) by 0x4DD5836: gtk_entry_completion_set_model (gtkentrycompletion.c:1220) by 0xE2E89F3: directory_check (goto_file.c:166) by 0x5A6B72F: g_closure_invoke (gclosure.c:834) by 0x5A9AC1A: signal_emit_unlocked_R.isra.0 (gsignal.c:3961) by 0x5A8B7A1: signal_emit_valist_unlocked (gsignal.c:3520) by 0x5A8BCAF: g_signal_emit_by_name (gsignal.c:3624) by 0x4DBAC29: end_change.lto_priv.0 (gtkentry.c:2941) by 0x4DC6576: gtk_entry_real_insert_text.lto_priv.0 (gtkentry.c:5401) by 0x5A6B72F: g_closure_invoke (gclosure.c:834) by 0x5A9AF49: signal_emit_unlocked_R.isra.0 (gsignal.c:3928) by 0x5A8B7A1: signal_emit_valist_unlocked (gsignal.c:3520) Address 0xd42b8e0 is 96 bytes inside a block of size 136 free'd at 0x48468CF: free (vg_replace_malloc.c:985) by 0x5A90164: g_type_free_instance (gtype.c:2023) by 0x5A7A732: g_object_unref (gobject.c:4475) by 0x48E5AC9: run_kb (keybindings.c:1334) by 0x48E5AC9: run_kb (keybindings.c:1325) by 0x48E67E4: on_key_press_event (keybindings.c:1396) by 0x4CFA6CC: _gtk_marshal_BOOLEAN__BOXED.part.0 (gtkmarshalers.c:84) by 0x5A6B72F: g_closure_invoke (gclosure.c:834) by 0x5A9A895: signal_emit_unlocked_R.isra.0 (gsignal.c:3888) by 0x5A8B094: signal_emit_valist_unlocked (gsignal.c:3533) by 0x5A8B9D6: g_signal_emit_valist (gsignal.c:3263) by 0x5A8BA93: g_signal_emit (gsignal.c:3583) by 0x4FC2CD4: gtk_widget_event_internal.part.0.lto_priv.0 (gtkwidget.c:7812) Block was alloc'd at at 0x484A993: calloc (vg_replace_malloc.c:1595) by 0x5B2651A: g_malloc0 (gmem.c:133) by 0x5A96F1B: g_type_create_instance (gtype.c:1923) by 0x5A7CB10: g_object_new_internal.part.0 (gobject.c:2603) by 0x5A7E0C6: UnknownInlinedFun (gobject.c:2600) by 0x5A7E0C6: g_object_new_with_properties (gobject.c:2766) by 0x5A7F009: g_object_new (gobject.c:2412) by 0x4E56A25: gtk_list_store_new (gtkliststore.c:426) by 0xE2E8573: build_file_list (goto_file.c:111) by 0xE2E86A7: menu_item_activate (goto_file.c:285) by 0x48E5AC9: run_kb (keybindings.c:1334) by 0x48E5AC9: run_kb (keybindings.c:1325) by 0x48E67E4: on_key_press_event (keybindings.c:1396) by 0x4CFA6CC: _gtk_marshal_BOOLEAN__BOXED.part.0 (gtkmarshalers.c:84) Gtk-CRITICAL **: 22:43:16.822: gtk_entry_completion_set_model: assertion 'model == NULL || GTK_IS_TREE_MODEL (model)' failed ``` </details> ## Cause >From what I understand, the root cause is here: https://github.com/geany/geany-plugins/blob/2b897dc56c097551d2214aef9bfe2c9341585e52/codenav/src/goto_file.c#L186-L188 The model is saved in `old_model`, but the refcount is not incremented. As soon as the entry is destroyed, the model's refcount drops to 0 and it's deallocated. The (static) `old_model` now points to freed memory. In the next invocation, `gtk_entry_completion_set_model()` will be called with `old_model`, resulting in an invalid read in freed memory. ## Fix Increment ref count when caching the completion model (what this PR does). Extra evidence: With this PR applied, add a print for the refcount before gtk_entry_completion_set_model()`, like so: ```c printf("old model %p refcount %u\n", old_model, ((GObject *) old_model)->ref_count); gtk_entry_completion_set_model (completion, old_model); ``` This shows a refcount of 1 at that point, so if not for the `g_object_ref()` call added in this PR, this would have been 0, and the model would have been freed. You can view, comment on, or merge this pull request online at: https://github.com/geany/geany-plugins/pull/1339 -- Commit Summary -- * codenav: Fix use-after-free in cached completion model -- File Changes -- M codenav/src/goto_file.c (6) -- Patch Links -- https://github.com/geany/geany-plugins/pull/1339.patch https://github.com/geany/geany-plugins/pull/1339.diff -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany-plugins/pull/1339 You are receiving this because you are subscribed to this thread. Message ID: <geany/geany-plugins/pull/[email protected]>
