Fix heap-use-after-free in offline speaker diarization (likely #3253) - #3826
Open
kairoi-llc wants to merge 3 commits into
Open
kairoi-llc wants to merge 3 commits into
kairoi-llc wants to merge 3 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
dosubot
Bot
added
the
size:M
ComputeEmbeddings() over-allocates the embedding matrix (one row per
(chunk, speaker) pair) and then compacts it down to the rows that produced a
usable embedding:
ans = ans(seq, Eigen::placeholders::all);
That assignment aliases. Because the row count shrinks, Eigen resizes the
destination before evaluating the source, and DenseStorage::resize() frees the
old buffer and allocates a new one whenever the total size changes. The
right-hand side still refers to the freed buffer, so the copy reads freed
memory.
The compaction only runs when at least one embedding was discarded, which is
exactly the documented "the embedding model may output NaN" case, so the bug
stays dormant on well-behaved audio and then fires deterministically on a clip
that produces a bad embedding. Under the macOS/glibc allocators the freed
block is usually handed straight back and the read silently succeeds; on
Android's Scudo allocator the freed chunk is quarantined, so the read faults
and surfaces as a SIGSEGV inside the diarization call.
Evaluate the row selection into a temporary and move it into place.
AddressSanitizer, sherpa-onnx-offline-speaker-diarization, 39s 16 kHz mono
clip that yields at least one NaN embedding:
ERROR: AddressSanitizer: heap-use-after-free
READ of size 16 at 0x62c000000200 thread T0
#0 Eigen::internal::generic_dense_assignment_kernel<...>::assignPacket
AssignEvaluator.h:712
k2-fsa#1 Eigen::internal::call_dense_assignment_loop<...> AssignEvaluator.h:828
k2-fsa#2 OfflineSpeakerDiarizationPyannoteImpl::ComputeEmbeddings
offline-speaker-diarization-pyannote-impl.h:527
k2-fsa#3 OfflineSpeakerDiarizationPyannoteImpl::Process
offline-speaker-diarization-pyannote-impl.h:144
freed by thread T0 here:
k2-fsa#1 Eigen::internal::call_dense_assignment_loop<...> AssignEvaluator.h:821
k2-fsa#2 OfflineSpeakerDiarizationPyannoteImpl::ComputeEmbeddings
offline-speaker-diarization-pyannote-impl.h:527
Two related hardening changes in the same function, both on the path that the
crash exposed:
- Reject non-finite embeddings, not just NaN ones. An Inf passes an isnan()
test but is still fatal downstream: FastClustering normalizes every row,
which turns inf/inf into NaN, the NaN spreads through the cosine distance
matrix, and hclust_fast() then throws fastclustercpp::nan_error. Verified
with a unit probe driving FastClustering::Cluster directly: a single Inf in
one embedding row produces 7/28 NaN distances for an 8-row matrix and throws.
(Zero-norm and underflowing rows are safe -- Eigen's normalize() guards
squaredNorm() == 0 -- so isfinite is the exact boundary.)
- Return early when every embedding was rejected, instead of taking
&embeddings(0, 0) on an empty matrix.
hclust_fast() throws fastclustercpp::nan_error if the condensed distance matrix contains NaN. FastClustering::Cluster() builds that matrix from cosine similarities of L2-normalized embedding rows, so a single non-finite value anywhere in an embedding poisons every distance involving that row and takes down the whole diarization call. The caller now rejects non-finite embeddings, so this is defense in depth, but it is cheap and it keeps a numerical edge case from turning into a thrown exception in a library that is mostly consumed through a C API. Clamp any non-finite distance to 2, the maximum cosine dissimilarity, which makes the offending row maximally dissimilar to everything else and therefore its own cluster.
The three SherpaOnnxOfflineSpeakerDiarizationProcess* entry points had no try/catch, unlike most of the rest of c-api.cc. Letting a C++ exception escape a C-linkage function is undefined behaviour for non-C++ callers: the unwinder walks into a frame with no handler and no C++ unwind info, so a .NET or JVM P/Invoke caller crashes (typically SIGSEGV or a std::terminate abort) instead of seeing an error return. This is reachable in practice. hclust_fast() throws fastclustercpp::nan_error from inside Process() whenever the condensed distance matrix contains NaN. Confirmed off-device: an extern "C" wrapper around FastClustering::Cluster built the same way as this entry point dies with libc++abi: terminating due to uncaught exception of type fastclustercpp::nan_error Note that nan_error and fenv_error do not derive from std::exception, so catch (const std::exception &) alone is not enough; a catch-all is required. Also null-check the handle, matching the other entry points in this file.
kairoi-llc
force-pushed
the
fix/diarization-embedding-uaf
branch
from
July 30, 2026 00:37
7541d70 to
307cd73
Compare
📝 WalkthroughWalkthroughSpeaker diarization now rejects non-finite embeddings, handles empty clustering inputs, avoids unsafe Eigen aliasing, sanitizes invalid distances, and protects C API processing functions with null checks and exception handling. ChangesSpeaker diarization robustness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sherpa-onnx/c-api/c-api.cc`:
- Line 3263: Update the three affected guards in sherpa-onnx/c-api/c-api.cc at
lines 3263-3263, 3294-3294, and 3315-3315 to reject null samples when n is
positive, while preserving the existing handle validation and allowing null
samples when n is zero or negative.
- Around line 3317-3320: Update the callback validation in the surrounding
processing entry point to reject a null NoArg callback before constructing or
invoking the wrapper; alternatively, preserve nullness by passing a null
callback through to Process. Ensure callback is never dereferenced when absent,
while retaining the existing wrapper behavior for valid callbacks.
- Around line 3266-3267: Prevent leaks in the three speaker diarization result
construction sites at sherpa-onnx/c-api/c-api.cc lines 3266-3267, 3297-3298, and
3323-3324: use RAII ownership for each newly allocated result while calling the
corresponding Process method, then release ownership only after processing
succeeds and the raw pointer is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 956bac0a-5a03-4204-b825-3dc6b9edf73d
📒 Files selected for processing (3)
sherpa-onnx/c-api/c-api.ccsherpa-onnx/csrc/fast-clustering.ccsherpa-onnx/csrc/offline-speaker-diarization-pyannote-impl.h
| int32_t n) { | ||
| auto ans = new SherpaOnnxOfflineSpeakerDiarizationResult; | ||
| ans->impl = sd->impl->Process(samples, n); | ||
| if (!sd || !sd->impl) return nullptr; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate samples when n is positive.
The new guards only validate the handle. A null samples pointer with n > 0 reaches internal audio reads and can crash before C++ exception handling applies.
sherpa-onnx/c-api/c-api.cc#L3263-L3263: reject!samples && n > 0.sherpa-onnx/c-api/c-api.cc#L3294-L3294: reject!samples && n > 0.sherpa-onnx/c-api/c-api.cc#L3315-L3315: reject!samples && n > 0.
📍 Affects 1 file
sherpa-onnx/c-api/c-api.cc#L3263-L3263(this comment)sherpa-onnx/c-api/c-api.cc#L3294-L3294sherpa-onnx/c-api/c-api.cc#L3315-L3315
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sherpa-onnx/c-api/c-api.cc` at line 3263, Update the three affected guards in
sherpa-onnx/c-api/c-api.cc at lines 3263-3263, 3294-3294, and 3315-3315 to
reject null samples when n is positive, while preserving the existing handle
validation and allowing null samples when n is zero or negative.
Comment on lines
+3266
to
+3267
| auto ans = new SherpaOnnxOfflineSpeakerDiarizationResult; | ||
| ans->impl = sd->impl->Process(samples, n); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid leaking the result object on exceptions.
Process() can throw after new succeeds, bypassing deletion of ans. Use std::unique_ptr until successful return, or allocate after processing completes.
sherpa-onnx/c-api/c-api.cc#L3266-L3267: hold the allocated result in RAII storage untilProcesssucceeds.sherpa-onnx/c-api/c-api.cc#L3297-L3298: hold the allocated result in RAII storage untilProcesssucceeds.sherpa-onnx/c-api/c-api.cc#L3323-L3324: hold the allocated result in RAII storage untilProcesssucceeds.
📍 Affects 1 file
sherpa-onnx/c-api/c-api.cc#L3266-L3267(this comment)sherpa-onnx/c-api/c-api.cc#L3297-L3298sherpa-onnx/c-api/c-api.cc#L3323-L3324
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sherpa-onnx/c-api/c-api.cc` around lines 3266 - 3267, Prevent leaks in the
three speaker diarization result construction sites at
sherpa-onnx/c-api/c-api.cc lines 3266-3267, 3297-3298, and 3323-3324: use RAII
ownership for each newly allocated result while calling the corresponding
Process method, then release ownership only after processing succeeds and the
raw pointer is returned.
Comment on lines
3317
to
3320
| auto wrapper = [callback](int32_t num_processed_chunks, | ||
| int32_t num_total_chunks, void *) { | ||
| return callback(num_processed_chunks, num_total_chunks); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject a null NoArg callback.
This wrapper is non-null even when callback is null, so processing invokes it and dereferences a null function pointer. Include !callback in the entry validation or pass a null callback through to Process.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sherpa-onnx/c-api/c-api.cc` around lines 3317 - 3320, Update the callback
validation in the surrounding processing entry point to reject a null NoArg
callback before constructing or invoking the wrapper; alternatively, preserve
nullness by passing a null callback through to Process. Ensure callback is never
dereferenced when absent, while retaining the existing wrapper behavior for
valid callbacks.
csukuangfj
requested changes
Jul 30, 2026
Comment on lines
+47
to
+56
| // hclust_fast() throws fastclustercpp::nan_error (which is not a | ||
| // std::exception) if the condensed distance matrix contains NaN, and | ||
| // that exception would unwind out through the C API. A non-finite | ||
| // distance can only come from a degenerate embedding, so treat it as | ||
| // "maximally dissimilar" instead of letting it abort the whole | ||
| // diarization. | ||
| if (!std::isfinite(consine_dissimilarity)) { | ||
| consine_dissimilarity = 2; | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
Please don't change it unless you can give an example of how this if statement is reachable from our current code.
Comment on lines
+581
to
+590
| // NOTE: `ans = ans(seq, all)` is an ALIASED assignment and is not safe | ||
| // here. Assigning an indexed view of `ans` back into `ans` changes its | ||
| // number of rows, so the assignment resizes `ans` first, and Eigen's | ||
| // DenseStorage::resize() frees the old buffer and allocates a new one | ||
| // whenever the total size changes. The right-hand side still refers to | ||
| // the old, now-freed buffer, so the copy reads freed memory | ||
| // (heap-use-after-free). Evaluate the selection into a temporary first. | ||
| Matrix2D valid = | ||
| ans(Eigen::seqN(0, cur_row_index), Eigen::placeholders::all); | ||
| ans = std::move(valid); |
Collaborator
There was a problem hiding this comment.
please use
ans.conservativeResize(cur_row_index, ans.cols());
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
2 participants
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.
Fixes a heap-use-after-free in offline speaker diarization that has been crashing callers since at least 1.12.x, plus two pieces of hardening on the same fault path. Very likely the root cause of #3253 (
System.AccessViolationExceptionfrom .NET, SIGBUS from Python/macOS) — see the reasoning below.The bug
OfflineSpeakerDiarizationPyannoteImpl::ComputeEmbeddings()over-allocates the embedding matrix (one row per(chunk, speaker)pair), then compacts it to the rows that produced a usable embedding:This is an aliased assignment. Shrinking the row count changes the total size, so the assignment resizes the destination before evaluating the source, and
DenseStorage::resize()frees the old buffer and allocates a new one whenever the size changes (Eigen/src/Core/DenseStorage.h). The right-hand sideBlockstill refers to the freed buffer, so the copy reads freed memory.The branch runs only when at least one embedding was discarded — the case the code immediately above it already anticipates ("The embedding model may output NaN"). So the crash is data-dependent, reproduces on the same audio every time, and is invisible on well-conditioned audio.
ASan, against
sherpa-onnx-pyannote-segmentation-3-0+wespeaker_en_voxceleb_resnet34_LM:Why this is probably #3253
Both reporters there describe symptoms this mechanism predicts, and one of them independently narrowed it to the exact precondition:
That falls straight out of this bug:
Process()short-circuits single-chunk audio throughHandleOneChunkSpecialCase()and never reachesComputeEmbeddings(), so only multi-chunk audio can reach the aliased shrink.It also explains why the crash looks platform-specific. glibc/macOS malloc usually hands the same block straight back, so the read silently succeeds and the bug hides; allocators that quarantine freed chunks (Android's Scudo) fault hard. The
AccessViolationExceptionshape in #3253 is what this looks like from a .NET P/Invoke frame.What's in this PR
Three commits, smallest-blast-radius first:
Fix heap-use-after-free when compacting filtered speaker embeddings— evaluate the selection into a temporary before assigning. This is the crash fix; the other two are hardening.Do not let a non-finite distance abort clustering— the embedding filter testedstd::isnanonly, so an±Infembedding survived it.FastClusteringnormalizes every row, which mapsinf/infto NaN; the NaN spreads through the condensed cosine-distance matrix, andhclust_fast()then throwsfastclustercpp::nan_error. This switches the filter to!std::isfinite, clamps any residual non-finite distance to maximum dissimilarity, and adds an early return when every embedding was rejected (which would otherwise index&embeddings(0, 0)on an empty matrix).Do not let exceptions escape the diarization C API entries— the threeSherpaOnnxOfflineSpeakerDiarizationProcess*functions had notry/catch, sonan_errorcould unwind out of aextern "C"boundary; that is UB for non-C++ callers and crashes .NET/JVM frames rather than unwinding. Note these need a barecatch (...):fastclustercpp::nan_errorandfenv_errordo not derive fromstd::exception, so thecatch (const std::exception &)idiom used elsewhere inc-api.ccis not sufficient on its own.Validation
Built with
-fsanitize=address,undefinedon macOS arm64 and run against two batteries with the real models.7 pathological fixtures (32-bit float WAVs carrying non-finite / ~1e38 sample values, ~39 s each so they take the multi-chunk path):
Minimal audio repro: a single
+infsample anywhere in a 39-second clip. (f06faults on some runs and not others on macOS — the allocator sometimes returns the same block, which is exactly why this is easy to miss locally and reliable on Android.)12 realistic clips (two-speaker, digital silence, clipped, very quiet, DC offset, pure tone, overlapping speech, sparse bursts) — the check that matters for commit 2, since
!isfiniterejects strictly more embeddings thanisnan:No healthy embedding was non-finite, so the stricter filter costs nothing on real audio. There is also a standalone reproduction of just the aliasing bug that needs only Eigen — no models, no audio, no sherpa sources — if that's useful for a regression test.
How this was found
Reported by a .NET caller on Android arm64: one particular ~39-second clip killed the process on every run, always on the first
Process()call of a freshly-constructed engine — which ruled out reuse, disposal and concurrency and pointed at something input-shaped. Config wasnum_clusters = -1,cluster_threshold = 0.5, pyannote segmentation-3.0 + wespeaker resnet34.Two notes in the interest of full disclosure:
Summary by CodeRabbit