[Geany-Devel] Segmentation fault when auto-close plug-in is enable [patch]
thomas.martitz at xxxxx
Wed Oct 23 18:36:06 UTC 2013
Am 23.10.2013 16:53, schrieb Matthew Brush:
> On 13-10-23 05:29 AM, n at sk0 wrote:
>> Before read : Keep in mind that i am *not* C/C++ "native" developer, and
>> all message below can be just rant.
>> After little testing and debugging, i found that :
>> In on_editor_notify() function, user_data is not a valid pointer :
>> on_editor_notify(GObject *obj, gint scn, SCNotification *nt, gpointer
>> AutocloseUserData *data = user_data;
>> After I playing for a while with this and reading demoplugin.c source, i
>> found that in the autoclose.c, PluginCallback (line:829) is missing
>> the definition for "editor-notify" event,so i added it, recompile plugin
>> and now all seems to work correctly.
> It's connected on line 812 inside a callback for when the document is
> activated. There's a few problems with this; the data allocated on
> L810 is going to leak, once for each time a document is activated (has
> tab switched to). The other thing is the plugin_signal_connect() is
> going to stack up signal handlers, so if you activated a document,
> on_editor_notify is going to get called on every single scintilla
> notification (keypress, cursor blink, etc.), for every number of times
> that document was activated.
The signal handler is connected for "document-new" and "document-open".
This once per document and doesn't leak or stack anything.
> The final problem is that, as Lex mentioned, it's not checking
> `DOC_VALID()` (or doc->is_valid) but just that data->doc != NULL, so
> if any document that was ever activated is closed, this is going to
> explode when the document pointer is dereferenced (for reasons I never
> understood, Geany "recycles" documents, so it's entirely possible to
> have a document pointer that is neither NULL nor valid).
The crash in autoclose is a use-after-free one. The root problem is that
the signal handler is not removed in the "document-close" callback, so
it will be called again but this time the user_data points to freed
memory. The fix (I'm going to make a PR) is to disconnect the handler in
the "document-close" callback. Unforunately there is no
plugin_signal_disconnect() (and plugin_signal_connect() eats the handler
id) so one needs to hack around with g_signal_handlers_disconnect_by_func().
I introduced the buggy code, I'm sorry.
More information about the Devel