TBaseVirtualTree.InternalAddToSelection() may cause access violation

[nminkov]

Hello,

You have introduced changes in TBaseVirtualTree.InternalAddToSelection which cause access violations.

More specifically, FSelectionCount is incremented before node is actually added to FSelection array.
Because FOnAddToSelection event is called in between FSelectionCount increment and actual add into FSelection, if FOnAddToSelection does modify the tree selection, it will cause access violation.

E.g. If tree has 3 nodes, there is no selection, add one element to selection, then if in FOnAddToSelection changes the selection, like calling RemoveFromSelection, TBaseVirtualTree.FindNodeInSelection will crash with access violation because FSelectionCount is 1 but FSelection is nil.

I've modified this function to do the add in 4 steps.

1. Filter the nodes to add and add their indexes to an array
2. Add nodes to FSelection array (unmodified)
3. Increment the FSelectionCount
4. Postprocess the added nodes and for each, set proper state, call the FOnAddToSelection

mypatch.txt

[joachimmarder]

You have introduced changes in TBaseVirtualTree.InternalAddToSelection which cause access violations.

Can you name the commit or the version?

[nminkov]

I was using the original VirtualTree code and your code crashed in my usage. Took me time to trace back and its commit:

Revision: 8e7e44e
Author: Joachim Marder [email protected]
Date: 04.05.2015 22:02:01
Message:
Fixed issue #487: When OnAddToSelection is called, GetFirstSelected returns nil

[joachimmarder]

I reviewed you patch and it looks good. It also does not make #487 reoccur, which is often a problem. Thanks for you contribution.

[nminkov]

Thanks for accepting it and maintaining the project!