[Geany-Devel] Segmentation fault when auto-close plug-in is enable [patch]

[Thomas Martitz]

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
>> user_data)
>> {
>>      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).

True.


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.

Best regards.