DOC_VALID() usage correctness

[techee]

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:

geany/src/document.c

Lines 1282 to 1292 in 11b4a00

	static gboolean show_tab_cb(gpointer data)
	{
	GeanyDocument *doc = (GeanyDocument *) data;
	show_tab_idle = 0;
	/* doc might not be valid e.g. if user closed a tab whilst Geany is opening files */
	if (DOC_VALID(doc))
	document_show_tab(doc);
	return G_SOURCE_REMOVE;
	}

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:

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?

[elextr]

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.

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. 😁

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 Gnome 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.

Footnotes

1. or test doc->id == id and doc->valid as a shortcut before calling document_find_by_id() which does a linear search. ↩

[techee]

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

[elextr]

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

[techee]

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.

[elextr]

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.