Fix out-of-bounds read in createAsciiSubMat (segfault in rescorediagonal ungapped scoring) - #1132
Open
matsen wants to merge 1 commit into
Open
Fix out-of-bounds read in createAsciiSubMat (segfault in rescorediagonal ungapped scoring)#1132matsen wants to merge 1 commit into
matsen wants to merge 1 commit into
Conversation
DistanceCalculator.h's ungapped scoring functions (computeSubstitutionDistance and siblings) index this table directly with raw, untranslated byte values up to 255 and never bounds-check against its size. Any byte >=123 — e.g. a literal '|' observed in the crash that reproduces soedinglab#866 — reads a garbage row pointer 1+ slots past the end of the 123-element `matrix` array and segfaults on dereference. aa2num already maps every byte 0..UCHAR_MAX-1 to a valid alphabet index (SubstitutionMatrix::setupLetterMapping's `default:` case sends anything unrecognized to 'X'), so widening this table to the same domain is sufficient and doesn't change behavior for valid sequence data: reran the repro's easy-cluster with the default (v2/align2clust) pipeline before and after this change and got byte-identical cluster.tsv output. Reproducer (needs --linclust-version 1; the new default pipeline no longer calls rescorediagonal and so doesn't reach this code): mmseqs easy-cluster in.fasta out tmp --min-seq-id 0.7 --threads 1 \ -c 0.8 --cov-mode 0 --linclust-version 1 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
|
Thank you for the PR. Do you try to clusters special characters? |
Author
|
Hi Martin! Thanks for looking at this. No, this came up as a malformed file from a corrupted download from EBI. You are right that this is user error, still, it'd be nice to avoid a segfault. Here's a repro: Then |
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
SubstitutionMatrix::createAsciiSubMatbuilds its ASCII-indexed lookup table for bytes 0–122 ('z'+1), butDistanceCalculator.h's ungapped scoring functions (computeSubstitutionDistance,computeSubstitutionStartEndDistance,computeGlobalSubstitutionStartEndDistance,computeWindowQualitySubstitutionStartEndDistance) index it with raw, untranslated byte values up to 255 and never bounds-check. Any byte ≥123 reads a garbage row pointer past the end of the table and segfaults on dereference. This is the crash reported in #866 ("Ungapped alignment step died" / "Search died") and looks like the same call site as #323.This fixes it by widening the table to the same domain
aa2numalready safely maps (setupLetterMapping'sdefault:case sends every unrecognized byte to'X'), so no new out-of-bounds access is introduced and behavior for valid sequence data is unchanged.Root cause
Backtrace on a debug build (
-DCMAKE_BUILD_TYPE=Debug), replaying the crashingrescorediagonalinvocation under gdb:seq1/seq2(self-comparison,length=591) contain a literal|(0x7C= 124) at two positions in the 591-byte comparison window — one past the old table's last valid index (122).subMat[124]reads 2 slots past the end of the 123-elementmatrixarray; the resulting fault address matched neither the heap, the substitution table, nor the mmap'd sequence DB — consistent with dereferencing a garbage pointer read from past the array.Reproducer
The current default pipeline (
--linclust-version 2,align2clust) no longer callsrescorediagonal, so reproducing on currentmasterneeds the legacy path:Before this fix:
Segmentation fault (core dumped)/Error: Ungapped alignment step died/Error: linclust died/Error: Search died, 100% reproducible on Ubuntu 24.04.4 / glibc 2.39.Validation
--linclust-version 1command completes (149,739 sequences -> 32,234 clusters, exit 0) instead of crashing.easy-clusterwith the default pipeline (--linclust-version 2, unaffected by the bug) before and after this change on the same input:cluster.tsvis byte-identical, so this doesn't change results for the code path most users hit today.Test plan
--linclust-version 1no longer segfaults on an input containing non-alphabet bytesrescorediagonalspecifically)🤖 Generated with Claude Code