AcademySoftwareFoundation / openvdb

OpenVDB - Sparse volume data structure and tools
http://www.openvdb.org/
Mozilla Public License 2.0
2.62k stars 647 forks source link

Fixed an instance of undefined behaviour in tools::activate #1731

Closed Idclip closed 9 months ago

Idclip commented 9 months ago

This should finally completely fix the UBSAN builds

Idclip commented 9 months ago

Good catch, this is worrying. You say this wouldn't typically result in unintended behaviour - do you know why that is? Is it simply that it is unlikely that retrieving the child pointer as a value from the union would match the condition for activation?

Yes exactly, its incredibly unlikely that the child pointer would match the int or float criteria when the overlapping union memory is read. And even when it does (for example for bool types, where the ptr will always be valid and could be read as true, i.e. tools::active(booltree, /*value=*/true);), the following code is run in InternalNode:

    //@{
    /// Use a mask accessor to ensure consistency between the child and value masks;
    /// i.e., the value mask should always be off wherever the child mask is on.
    void setValueMask(Index n, bool on) { mValueMask.set(n, mChildMask.isOn(n) ? false : on); }
    //@}

Which completely ignores the call to it.setValueOn(/*on=*/true); anyway :|