Fix uninitialized isRooted member in Builder class#10
Open
slowkow wants to merge 1 commit intoiqtree:masterfrom
Open
Fix uninitialized isRooted member in Builder class#10slowkow wants to merge 1 commit intoiqtree:masterfrom
slowkow wants to merge 1 commit intoiqtree:masterfrom
Conversation
The Builder template class constructor was not initializing the
isRooted member variable, leading to undefined behavior.
On some platforms (particularly Linux with GCC), the uninitialized
memory could contain non-zero garbage values. When isRooted was
later used in calculations like:
degree_of_root = isRooted ? 2 : 3
this caused incorrect tree construction behavior. For certain
template instantiations (notably BIONJMatrix), this resulted in
segmentation faults due to array bounds violations during tree
construction.
The bug was platform and algorithm dependent because:
- Different compilers/platforms have different memory initialization
- Different template instantiations have different memory layouts
- NJ/RapidNJ happened to get zero values, while BIONJ got garbage
This fix initializes isRooted to false in the constructor's
initializer list, matching the other boolean members.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes an uninitialized member variable bug in the
Buildertemplate class constructor instarttree.h.The
isRootedmember was not being initialized in the constructor's initializer list, leading to undefined behavior when the variable was later used.The Bug
The Fix
Impact
On some platforms (particularly Linux with GCC), the uninitialized memory could contain non-zero garbage values. When
isRootedwas later used in tree construction calculations, this caused:degree_of_root = isRooted ? 2 : 3The bug was platform and algorithm dependent because:
BIONJMatrix<NJFloat>vsRapidNJ) have different memory layoutsHow it was discovered
This bug was discovered while integrating decenttree into an R package. The BIONJ algorithm consistently crashed with segfaults on Ubuntu/Linux while working fine on macOS. NJ and RapidNJ worked correctly on both platforms.
Testing
The fix has been verified by successfully running BIONJ on both macOS and Linux after applying this change.