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

[Matthew Brush]

On 13-10-23 11:36 AM, Thomas Martitz wrote:
> 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.
>
>

Ah, my bad, I only looked for 5 minutes on way out to work, I assumed 
on_document_activate() was used for "document-activate" signal.

>> 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().
>

Yeah, I fought this before too :( We could probably add a new/better 
function that returns the handler id and a matching function to 
disconnect it by id, and just deprecate the old function. It seems like 
it'd be quite easy.

Cheers,
Matthew Brush