Alisim fixes

[trongnhanuit]

This pull request includes:

1. Fix the crash due to heap buffer overflow on the refactored alignment.
2. Bug fix: adding the partition rate to the Gillespie algorithm. This bug affects special cases when using partition rates (-p) with very short branch lengths or partition sequence lengths (e.g., 1 site per partition). Thanks, Mattia, for reporting this.

[StefanFlaumberg]

Hi Nhan @trongnhanuit,

On the problem with fake patterns:
I couldn't reproduce the bug, so I don't know how you are getting it, in the first palce. num_variant_sites is defaulted to 0 in alignment.h, so no overflow is possible, but maybe some compilers do not get it. But to default non-const members in a header is not a good thing anyway -- they should be initialized in a constructor instead.
Regardless, the bug is likely due to num_variant_sites and num_informative_sites being filled with large garbage values at alignment initialization, with those values potentially leading to an overflow in orderPatternByNumChars(). It is the root problem. Calling countConstSites() is a hacky workaround because:

1. it doesn't fix the root problem: Alignment members should be properly initialized instead;
2. it works with a fake pattern: as computeConst() was not called for it, it has flag = 0, so it should be treated as variant pattern, adding to num_variant_sites in countConstSites(), and thus adding patterns to ordered_pattern in orderPatternByNumChars() (nothing of these is a problem by itself, but what for?).

If you agree with the above, then the best solution would be:

1. To reset to the previous commit: git reset --hard 594e5c4
2. To modify the Alignment default constructor to initialize all simple-type variables as follows:

Alignment::Alignment() {
    name = "Noname";
    num_states = 0;
    num_variant_sites = 0;
    num_informative_sites = 0;
    num_parsimony_sites = 0;
    frac_const_sites = 0.0;
    frac_invariant_sites = 0.0;
    codon_table = nullptr;
    genetic_code = nullptr;
    non_stop_codon = nullptr;
    virtual_pop_size = 0;
    pomo_sampling_method = SAMPLING_WEIGHTED_BINOM;
    seq_type = SEQ_UNKNOWN;
    STATE_UNKNOWN = 126;
    // pars_lower_bound = nullptr; // now a local variable in orderPatternByNumChars()
    cache_ntfreq = nullptr;
}

3. To push-force the changes: git push -f

This solves the root problem. I was going to add these lines to the constructor in a later refactoring anyway, but I didn't expect that the current code could produce any bug, sorry.

Best,
Stefan

[trongnhanuit]

Hi Stefan, Many thanks for looking into this. Actually, I think we're referring to different problems. Here is how to reproduce the crash: cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-fsanitize=address -fno-omit-frame-pointer -g" \ -DCMAKE_C_FLAGS="-fsanitize=address -fno-omit-frame-pointer -g" \ -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address" <iqtree_src_code> make -j6 ./iqtree3 --alisim aln_4 -t aln.treefile -m JC -seed 1 -redo -p partition_4.nex This reports heap-buffer-overflow alignment.cpp:1529 in Alignment::orderPatternByNumChars(int). It is because in orderPatternByNumChars, pars_lower_bound is allocated with a size based on num_variant_sites. For the fake per-partition alignment we build in initializeIQTreeFromTreeFile, num_variant_sites is zero, which causes a heap-buffer-overflow later. Given that, I'd like to keep countConstSites() as the fix. Separately, I do agree that the constructor is missing several initializers, and your proposed update could help with that. Anyway, thanks a lot for looking into this. And no worries about the bug, since refactoring the alignment object is a huge task and this bug is difficult to see. Cheers,

…

On Fri, Jul 10, 2026 at 12:16 PM StefanFlaumberg ***@***.***> wrote: *StefanFlaumberg* left a comment (iqtree/iqtree3#185) <#185 (comment)> Hi Nhan @trongnhanuit <https://github.com/trongnhanuit>, On the problem with fake patterns: I couldn't reproduce the bug, so I don't know how you are getting it, in the first palce. num_variant_sites is defaulted to 0 in alignment.h, so no overflow is possible, but maybe some compilers do not get it. But to default non-const members in a header is not a good thing anyway -- they should be initialized in a constructor instead. Regardless, the bug is likely due to num_variant_sites and num_informative_sites being filled with large garbage values at alignment initialization, with those values potentially leading to an overflow in orderPatternByNumChars(). It is the root problem. Calling countConstSites() is a hacky workaround because: 1. it doesn't fix the root problem: Alignment members should be properly initialized instead; 2. it works with a fake pattern: as computeConst() was not called for it, it has flag = 0, so it should be treated as variant pattern, adding to num_variant_sites in countConstSites(), and thus adding patterns to ordered_pattern in orderPatternByNumChars() (nothing of these is a problem by itself, but what for?). If you agree with the above, then the best solution would be: 1. To reset to the previous commit: git reset --hard 594e5c4 2. To modify the Alignment default constructor to initialize all simple-type variables as follows: Alignment::Alignment() { name = "Noname"; num_states = 0; num_variant_sites = 0; num_informative_sites = 0; num_parsimony_sites = 0; frac_const_sites = 0.0; frac_invariant_sites = 0.0; codon_table = nullptr; genetic_code = nullptr; non_stop_codon = nullptr; virtual_pop_size = 0; pomo_sampling_method = SAMPLING_WEIGHTED_BINOM; seq_type = SEQ_UNKNOWN; STATE_UNKNOWN = 126; // pars_lower_bound = nullptr; // now a local variable in orderPatternByNumChars() cache_ntfreq = nullptr; } 3. To push-force the changes: git push -f This solves the root problem. I was going to add these lines to the constructor in a later refactoring anyway, but I didn't expect that the current code could produce any bug, sorry. Best, Stefan — Reply to this email directly, view it on GitHub <#185?email_source=notifications&email_token=ABZPMLF3WUIUBTEIKSNZ2HD5EBGX3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJTGEZTKNBZHA32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4931354987>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABZPMLDOXOHWHIWGMPB2TID5EBGX3AVCNFSNUABFKJSXA33TNF2G64TZHM4TIMJYGUZDAOBUHNEXG43VMU5TIOBUGQYTKMRVHEZKC5QC> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[trongnhanuit]

I've just found that directly replying from my email caused issues with the message formatting on GitHub, and the attached files were inaccessible. So, re-post it here.

Hi Stefan,
Many thanks for looking into this. Actually, I think we're referring to different problems. Here is how to reproduce the crash:

cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-fsanitize=address -fno-omit-frame-pointer -g" \ -DCMAKE_C_FLAGS="-fsanitize=address -fno-omit-frame-pointer -g" \ -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address" <iqtree_src_code>

make -j6

./iqtree3 --alisim aln_4 -t tree.txt -m JC -seed 1 -redo -p partition_4.txt

This reports heap-buffer-overflow alignment.cpp:1529 in Alignment::orderPatternByNumChars(int). It is because
in orderPatternByNumChars, pars_lower_bound is allocated with a size based on num_variant_sites. For the fake per-partition alignment we build in initializeIQTreeFromTreeFile, num_variant_sites is zero, which causes a heap-buffer-overflow later. Given that, I'd like to keep countConstSites() as the fix.

Separately, I do agree that the constructor is missing several initializers, and it's great that you're going to add these lines to the constructor in a later refactoring.

Anyway, thanks a lot for looking into this. And no worries about the bug, since refactoring the alignment object is a huge task and this bug is difficult to see.

Cheers,
Nhan

[StefanFlaumberg]

Nhan, thank you for the test data!

Now I see the problem. Initially, num_variant_sites = 0 (for now, defaulted in alignment.h), so orderPatternByNumChars() expects no parsimony sites and prepares the pars_lower_bound buffer accordingly. But the fake pattern has flag = 0 (the default value from the Pattern() constructor), so it is treated as a variant pattern and orderPatternByNumChars() treats its sites as parsimony sites, which leads to out-of-bound operations with the pars_lower_bound buffer. The failure to reproduce this behaviour on my data is explained by that I simulated too short alignments.
That some members are left uninitialized in the Alignment() constructor is an unrelated issue, irrelevant to the current bug; sorry for my misleading comment.

But I maintain that calling countConstSites() for fake alignments is too hacky. A more natural solution would be to mark the fake pattern as invariant (flag = 2); to this end in alisimulator.cpp from line 149:

            Pattern pat;
            pat.resize(current_tree->aln->getNSeq(), current_tree->aln->STATE_UNKNOWN);
            pat.frequency = expected_num_states_current_tree;
            pat.flag = PAT_INVARIANT;
            current_tree->aln->addPattern(pat);

This way, num_variant_sites is still default-zero, which is now coherent with the pattern being invariant. The pattern is not added to ordered_pattern, so ordered_pattern is kept empty, which is correct, as we cannot run parsimony analysis on the fake pattern.
In contrast, the countConstSites() solution treats the fake pattern as variant non-informative (which is less sensible) and adds it to ordered_pattern -- this works fine, but the internal logic is more surprising.

Best,
Stefan

[trongnhanuit]

Hi Stefan,
That solution works well. I've now applied it. Thanks again for digging into this!

Cheers,
Nhan

[bqminh]

looking good