search

geany / geany-plugins

The combined Geany Plugins collection
http://plugins.geany.org/
590 stars 264 forks source link

codenav/goto_file: Fix use-after-free crash #1339

Closed gkatev closed 4 months ago

gkatev commented 5 months ago

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:

``` 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 ```

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:

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.

eht16 commented 4 months ago

Tested and works. Thank you!