While looking at some Geany code, I noticed that `DOC_VALID()` is used in some callbacks where it's supposed to check whether the document was closed in the meantime:
https://github.com/geany/geany/blob/11b4a00a3020b1c9ace3d3ae65aa5ec7d5ff84e0...
Unless I'm missing something, I don't think it's correct - if the pointer to the `doc` was freed in the meantime, `DOC_VALID()` usage would lead to memory access over invalid pointer.
In the LSP plugin plugin I have to deal with a similar situation where I cannot be sure whether the document still exists when I receive a response from the LSP server as a callback. I use this function:
```C gboolean lsp_utils_doc_is_valid(GeanyDocument *doc) { gint i;
foreach_document(i) { if (doc == documents[i]) return TRUE; }
return FALSE; } ```
Even this function isn't 100% correct - the original document could have been closed and a new document created at the same time and the new document's pointer could be identical to the closed document's pointer so `doc` would be valid but pointing to a different document. (I decided to accept this limitation as I think it's not so frequent.)
My question is, shouldn't we do something similar in Geany?
Sigh, welcome to asynchronous programming and state changing beneath you.
if the pointer to the doc was freed in the meantime, DOC_VALID() usage would lead to memory access over invalid pointer.
Document objects are never freed, so you will never get an invalid pointer, so thats one less worry. So no need to check if its in the document array, it will be.
But since closed document objects are reused for a different file you are correct that simply testing `doc->valid` is not sufficient to ensure its the same document, that is what `doc->id` is for, it is unique for each opened file and `document_find_by_id()` should be used, then valid can be checked.
See [plugin API docs](https://www.geany.org/manual/reference/structGeanyData.html#a933a5623cf6d093...).
The exact semantics of this changed at one point (vague memory, it was a while ago) and I bet lots of plugins didn't get updated, and new plugin writers didn't RTF above documentation. :grin:
Using `document_get_current()` is fine, so long as the callback never assumes its the same document as last time it was called.
Also its not guaranteed that Geany callbacks "do the right thing" either, `document_find_by_id()` is only used twice in Geany. In particular when dialogs are run state may change, but I think most of Geany assumes it doesn't. On Gnome and clones its probably safe because it honours modality which prevents users from modifying state (eg closing a document with the dialog open) but other window managers (eg KDE) do not honour modality, search the issues if interested.
Closed #3872 as completed.
@elextr Thanks for the answer.
Document objects are never freed, so you will never get an invalid pointer, so thats one less worry. So no need to check if its in the document array, it will be.
Alright, that's the piece of information I was missing - I never actually looked at the implementation until now, and yes, there are the pointers from the array that get reused so the code does the "right thing" even though checking the ID would be better.
checking the ID would be better.
No, actually checking `id` is essential. I didn't really realise how much until now. Consider for example the sequence:
1. Geany function gets `doc` pointer 2. does something that results in a signal 3. plugin callback is called by signal and it closes the document Geany's `doc` points to 4. plugin opens a new document and it happens to reuse the document object that Geany's `doc` points to 5. signal returns to Geany
So now, unless Geany checks both `valid` and `id` it will not know its `doc` points to something different to what it thinks.
After finding `id` is different it probably shouldn't do anything.
And of course the sequence above is just one way for it to happen.
But I suspect a lot of Geany doesn't check `id`, maybe IS_VALID should take id as a parameter. And even better it can be made a function not an EVIL macro!!!
Yes, but I think (3) is really rare - closing documents is typically done by users; if plugins do that, it would be very unexpected. That doesn't mean there couldn't be some races like that when a user closes a document, opens another, and only after that an asynchronous operation with a doc argument finishes.
Yes agree, but any scenario that is possible _can_ occur, and the case of the user closes document while dialog open has been reported already, and see #2599 for a non-KDE example.
Since we don't have many occurrences its not something that needs rushing into, just put it on the eternal todo list and consider it when "somebody" addresses one of the issues.
github-comments@lists.geany.org