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.

P.S.: I add the patch file as attachment to this mail, becouse for now i do not have any expiriance in commiting patches on GitHub, but tonight i will fix this big mistake :)

On 23/10/13 13:00, Lex Trotman wrote:

Looking at the line that failed, first guess would be failure to check doc is valid before using it.

Cheers Lex

On 23 October 2013 20:20, n@sk0 <arrtedone@gmail.com mailto:arrtedone@gmail.com> wrote:

There is strange behaviour when Auto-close plug-in is enabled.

Steps to reproduce :
1. open Geany
2. Enable auto-close plug-in
3. open several files for edit
4. Click Ctrl+W (close document)
--

Expected result :
Currently opened document is closed.
--

Actual result :
Segmentation fault
--

See attached gdb backtrace for more info.

Debug info :
Geany-INFO: Geany 1.24 (git >= ef33175), en_US.UTF-8
Geany-INFO: GTK 2.24.22, GLib 2.36.3
Geany-INFO: System data dir: /usr/local/share/geany
Geany-INFO: User config dir: /home/nask0/.config/geany
Geany-INFO: System plugin path: /usr/local/lib/geany
Geany-INFO: Added filetype CUDA (57).
Geany-INFO: Added filetype Rust (58).
Geany-INFO: Added filetype Scala (59).
Geany-INFO: Added filetype Graphviz (60).
Geany-INFO: Added filetype Cython (61).
Geany-INFO: Added filetype Genie (62).
Geany-INFO: Added filetype Clojure (63).
Geany-INFO: Added filetype Go (64).
Geany-INFO: Disabling terminal support
Geany-INFO: Loaded:   /usr/local/lib/geany/addons.so (Addons)
Geany-INFO: Loaded:   /usr/local/lib/geany/autoclose.so (Auto-close)
Geany-INFO: Loaded:   /usr/local/lib/geany/geanylipsum.so
(GeanyLipsum)
Geany-INFO: Loaded:   /usr/local/lib/geany/geanyvc.so (GeanyVC)
Geany-INFO: Loaded:   /usr/local/lib/geany/htmlchars.so (HTML
Characters)
Geany-INFO: Loaded:   /usr/local/lib/geany/geanyprj.so (Project)
Geany-INFO: Loaded:   /usr/local/lib/geany/splitwindow.so (Split
Window)
Geany-INFO: Loaded:   /usr/local/lib/geany/treebrowser.so
(TreeBrowser)
Geany-INFO: Loaded:   /usr/local/lib/geany/xmlsnippets.so (XML
Snippets)


_______________________________________________
Devel mailing list
Devel@lists.geany.org <mailto:Devel@lists.geany.org>
https://lists.geany.org/cgi-bin/mailman/listinfo/devel

---

Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel

On 13-10-23 05:29 AM, n@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 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).

Cheers, Matthew Brush

P.S.: I add the patch file as attachment to this mail, becouse for now i do not have any expiriance in commiting patches on GitHub, but tonight i will fix this big mistake :)

On 23/10/13 13:00, Lex Trotman wrote:

...

Looking at the line that failed, first guess would be failure to check doc is valid before using it.

Cheers Lex

On 23 October 2013 20:20, n@sk0 <arrtedone@gmail.com mailto:arrtedone@gmail.com> wrote:

There is strange behaviour when Auto-close plug-in is enabled.

Steps to reproduce :
1. open Geany
2. Enable auto-close plug-in
3. open several files for edit
4. Click Ctrl+W (close document)
--

Expected result :
Currently opened document is closed.
--

Actual result :
Segmentation fault
--

See attached gdb backtrace for more info.

Debug info :
Geany-INFO: Geany 1.24 (git >= ef33175), en_US.UTF-8
Geany-INFO: GTK 2.24.22, GLib 2.36.3
Geany-INFO: System data dir: /usr/local/share/geany
Geany-INFO: User config dir: /home/nask0/.config/geany
Geany-INFO: System plugin path: /usr/local/lib/geany
Geany-INFO: Added filetype CUDA (57).
Geany-INFO: Added filetype Rust (58).
Geany-INFO: Added filetype Scala (59).
Geany-INFO: Added filetype Graphviz (60).
Geany-INFO: Added filetype Cython (61).
Geany-INFO: Added filetype Genie (62).
Geany-INFO: Added filetype Clojure (63).
Geany-INFO: Added filetype Go (64).
Geany-INFO: Disabling terminal support
Geany-INFO: Loaded:   /usr/local/lib/geany/addons.so (Addons)
Geany-INFO: Loaded:   /usr/local/lib/geany/autoclose.so (Auto-close)
Geany-INFO: Loaded:   /usr/local/lib/geany/geanylipsum.so
(GeanyLipsum)
Geany-INFO: Loaded:   /usr/local/lib/geany/geanyvc.so (GeanyVC)
Geany-INFO: Loaded:   /usr/local/lib/geany/htmlchars.so (HTML
Characters)
Geany-INFO: Loaded:   /usr/local/lib/geany/geanyprj.so (Project)
Geany-INFO: Loaded:   /usr/local/lib/geany/splitwindow.so (Split
Window)
Geany-INFO: Loaded:   /usr/local/lib/geany/treebrowser.so
(TreeBrowser)
Geany-INFO: Loaded:   /usr/local/lib/geany/xmlsnippets.so (XML
Snippets)


_______________________________________________
Devel mailing list
Devel@lists.geany.org <mailto:Devel@lists.geany.org>
https://lists.geany.org/cgi-bin/mailman/listinfo/devel

---

Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel

---

Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel

(for reasons I never understood, Geany "recycles" documents, so it's entirely possible to have a document pointer that is neither NULL nor valid)

Didn't expect that. I'll fix my plugins for such kind of mistakes.

-- Best regards, Pavel Roschin aka RPG

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

On 24 October 2013 13:09, Matthew Brush mbrush@codebrainz.ca wrote:

On 13-10-23 11:36 AM, Thomas Martitz wrote:

...

[snip]

Regarding that pattern we discussed previously and used in this AutoClose code for attaching data to a document, I'd be interested whether you or anyone thinks this branch (last/top two commits) would be useful to plugins:

https://github.com/codebrainz/**geany/commits/document-**datalisthttps://github.com/codebrainz/geany/commits/document-datalist

IIUC several plugins already use various methods of storing data for different documents and other Geany data structures. Having a common way of doing that which cleans up when the Geany struct is destroyed is a good idea.

But it seems a pity to have to duplicate the glib interface for each structure that offers the facility, can we just return the GData and let the plugins use the normal g_datalist functions?

IMO it'd be better to make GeanyDocument an actual GObject and get the data lists for "free", but at least this is sort of a step in the same direction.

Disclaimer: I have no interest in "defending" this or hashing it out, it's just one of my many local branches and hasn't been really tested. If anyone doesn't like it, or spots some bugs or something, they can just not saying anything (ie. I'm not asking for Code Review) and it will quietly go away, if anyone really likes it, they can push it forward and we can review and discuss it at that point.

Don't try to make special conditions that say your contributions must not be discussed/reviewed, thats rude, its like saying you think you are better than the other contributors on this list.

Cheers Lex

PS On the recycling of doc structures and doc->is_valid, this does have the advantage (for a structure where miscellaneous pointers to the structure are going to exist in Geany and plugins) that doc pointers will always point to a geanydocument struct. So the is_valid test is always right. If the memory was returned and re-cycled into some other struct, the old doc pointers could point to anything, and could just as easily appear a valid document. So its safer than the alternative, but the requirement to check is_valid really does need more visibility since its an unusual idiom.

Cheers,

Matthew Brush

______________________________**_________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-**bin/mailman/listinfo/develhttps://lists.geany.org/cgi-bin/mailman/listinfo/devel

[...]

I said it specifically to avoid getting into deep design discussions or code reviews, I just want a general +1 or -1 whether anyone thinks the general concept is useful and whether it's worth looking at implementing something similar. If not one else really cares, it's not worth getting into details about.

Agree that if nobody wants it then going further is not worthwhile, but the world of software design is not black and white. A software design suggestion on a mailing list cannot be summarised by a Farcebook like/dislike count. Suggestions of different interfaces or approaches can change a "don't see the point" into an "aha!, I could use that". Stymieing discussion is not going to get to that point.

Cheers Lex

Cheers, Matthew Brush ______________________________**_________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-**bin/mailman/listinfo/develhttps://lists.geany.org/cgi-bin/mailman/listinfo/devel

Am 24.10.2013 05:59, schrieb Matthew Brush:Don't try to make special conditions that say your contributions must not

...

be discussed/reviewed, thats rude, its like saying you think you are better than the other contributors on this list.

I said it specifically to avoid getting into deep design discussions or code reviews, I just want a general +1 or -1 whether anyone thinks the general concept is useful and whether it's worth looking at implementing something similar. If not one else really cares, it's not worth getting into details about.

+1, see my other mail

FWIW, I don't took your statement as rude. I understood that you don't feel like supporting your proposal unless you see someone else supporting your general idea. Because you're tired of fighting against windmills on this list.

Best regards.

[...]

But it seems a pity to have to duplicate the glib interface for each

...

structure that offers the facility, can we just return the GData and let the plugins use the normal g_datalist functions?

[...]

To summarise the answer to the question Matthew gave on IRC, no, we don't have to duplicate the glib interface but it makes it easier to clean up after plugins if we do. And its unlikely that many other structs will need it.

Ok, if thats the case then its reasonable to duplicate the (fairly small) g_datalist interface for the few structs that need the functionality.

[...]

We could just use the usual idiom (in G*) and use objects that have destruction signals and reference counting and such stuff that is useful for this type of scenario. IMO it'd be more useful to just treat GeanyDocuments like other stuff, and maybe having an _is_open() or _is_closed() function or something to see if the document you have a reference on is still open (if you didn't want to connect to its open/close signals it could emit).

No problem to replace is_valid with an is_open function that can hide the implementation. But whichever implementation is used, it is going to have to be able to identify if a pointer is no longer to an open document. The current implementation sort of does that by keeping the document objects around so the test of the boolean is_valid is always legal. Comparing the pointer against the list of open documents is a (slower) alternative. It could also be implemented in some other way.

Cheers Lex

Cheers, Matthew Brush

______________________________**_________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-**bin/mailman/listinfo/develhttps://lists.geany.org/cgi-bin/mailman/listinfo/devel

On 13-10-24 12:23 AM, Thomas Martitz wrote:

Am 24.10.2013 04:09, schrieb Matthew Brush:

...

On 13-10-23 11:36 AM, Thomas Martitz wrote:

...

[snip]

Regarding that pattern we discussed previously and used in this AutoClose code for attaching data to a document, I'd be interested whether you or anyone thinks this branch (last/top two commits) would be useful to plugins:

https://github.com/codebrainz/geany/commits/document-datalist

IMO it'd be better to make GeanyDocument an actual GObject and get the data lists for "free", but at least this is sort of a step in the same direction.

I agree with that.

I also agree with your general idea of per-document data lists. However, I'm not seeing the point of the new code you added because each doc has already a ScintillaObject, which is a GObject. I would suggest that the document_set_data() and friends wrap around g_object_set_data(doc->editor->sci). This way things are prepared for when GeanyDocument becomes a GObject but without the need to duplicate the glib interface in Geany.

Unless I missed anything that requires the datalist in GeanyDocument directly?

There isn't (or rather shouldn't be) a 1:1 correspondence between a GeanyDocument and the view that's showing it. Consider say a split view where the same document is shown in multiple views, or in two windows or whatever. But you're right, with the way the code is currently, you could just wrap the Scintilla data list, it's just not very future-proof (at least this was my thinking).

Cheers, Matthew Brush