-
Notifications
You must be signed in to change notification settings - Fork 660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent integer overflow in NodeManager and LeafManager #1794
Prevent integer overflow in NodeManager and LeafManager #1794
Conversation
Prevent integer overflow on grids with many nodes or leafs. Signed-off-by: ghurstunither <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a change for the log and would be ideal if we could add a simple unit test that exceeds 32-bit integer.
Is that something I add in this PR?
I'm not sure how to do that without making a gigantic tree. |
I think there's actually more to talk about here. The Tree and RootNode also make assumptions that the number of leaf nodes cannot exceed 2^32: https://github.com/AcademySoftwareFoundation/openvdb/blob/master/openvdb/openvdb/tree/Tree.h#L116 The tree ones in particular are actually virtual functions, so if we want to bump this arbitrary limit, we should also change those methods as an ABI change. |
Thanks for pointing this out. I think the following should return
The other methods seem fine to return |
leafCount, unallocatedLeafCount, nodeCount now work with Index64 to accommodate trees with more than 2^32-1 leaf nodes. Signed-off-by: ghurstunither <[email protected]>
Correct type for variable. Signed-off-by: ghurstunither <[email protected]>
Turns off OPENVDB_BUILD_UNITTESTS. Signed-off-by: ghurstunither <[email protected]>
Use Index64 for tree's leafCount. Signed-off-by: ghurstunither <[email protected]>
Correct type for nodeCount members. Signed-off-by: ghurstunither <[email protected]>
* nonLeafCount and treeDepth return Index64 in TreeBase. * All ABI breakages safeguarded for versions < 12. * Deprecated implementations of nodeCount for vector<Index32> arguments. Signed-off-by: ghurstunither <[email protected]>
Restore 32 bit functionality for versions below 12. Signed-off-by: ghurstunither <[email protected]>
Prevent nodeCount deprecation warnings. Signed-off-by: ghurstunither <[email protected]>
Backward compatibility for unit tests. Signed-off-by: ghurstunither <[email protected]>
trailing space Signed-off-by: ghurstunither <[email protected]>
AX tests for correct leafCount return type. Signed-off-by: ghurstunither <[email protected]>
trailing space Signed-off-by: ghurstunither <[email protected]>
AX test failures Signed-off-by: ghurstunither <[email protected]>
deprecation suppression in recursive nodeCount calls. Signed-off-by: ghurstunither <[email protected]>
define deprecated nodeCount for all versions. Signed-off-by: ghurstunither <[email protected]>
Keep Index return type for treeDepth. Signed-off-by: ghurstunither <[email protected]>
Avoid ABI conditionals in Root/Internal/Leaf node files. Signed-off-by: ghurstunither <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small diffing error to resolve, otherwise this all looks good to me, thanks for making all those changes!
Remove migrated tests. Signed-off-by: ghurstunither <[email protected]>
c484949
into
AcademySoftwareFoundation:master
For large enough grids,
leafCounts
ininitLeafArray()
andnodeCounts
ininitNodeChildren()
can overflow asstd::vector<Index32>
.