Repository navigation
Add Rust API for streaming speech recognition - #3204
Conversation
dosubot
Bot
added
the
size:XL
Summary of ChangesHello @csukuangfj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds FFI and safe Rust support for online streaming ASR (recognizers, streams, wave I/O), new Rust examples and helper scripts for streaming Zipformer, bumps package versions to 0.1.1, and centralizes CI test steps into a new Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant RustHigh as Rust API<br/>(sherpa-onnx)
participant FFI as FFI Bindings<br/>(sherpa-onnx-sys)
participant CLib as C Library
App->>RustHigh: build OnlineRecognizerConfig
App->>RustHigh: OnlineRecognizer::create(config)
RustHigh->>FFI: to_sys(cfg) & SherpaOnnxCreateOnlineRecognizer(...)
FFI->>CLib: call C create function
CLib-->>FFI: recognizer handle
FFI-->>RustHigh: pointer returned
RustHigh-->>App: OnlineRecognizer
App->>RustHigh: create_stream() / read WAV
RustHigh->>FFI: SherpaOnnxCreateOnlineStream / SherpaOnnxReadWave
FFI->>CLib: call C functions
CLib-->>FFI: stream/wave handles
FFI-->>RustHigh: returns
loop feed chunks
App->>RustHigh: stream.accept_waveform(samples)
RustHigh->>FFI: SherpaOnnxOnlineStreamAcceptWaveform(...)
FFI->>CLib: feed audio
App->>RustHigh: recognizer.is_ready(stream) / decode()
RustHigh->>FFI: SherpaOnnxIsOnlineStreamReady / SherpaOnnxDecodeOnlineStream
FFI->>CLib: readiness/decoding
end
App->>RustHigh: get_result(stream)
RustHigh->>FFI: SherpaOnnxGetOnlineStreamResultAsJson(...)
FFI->>CLib: return JSON pointer
FFI-->>RustHigh: JSON string
RustHigh->>RustHigh: serde_json::from_str -> RecognizerResult
RustHigh-->>App: result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Rust API for streaming speech recognition, including FFI bindings, safe Rust wrappers, and a new example. The implementation looks solid overall. My review includes a few suggestions to improve robustness and code style. I've identified a critical typo in the Cargo.lock file that needs to be addressed. Additionally, there are a couple of places in the library code where using .unwrap() could lead to panics, for which I've recommended returning a Result instead. I also have a minor suggestion to use more idiomatic Rust in the new example.
| ] | ||
|
|
||
| [[package]] | ||
| name = "rust-ap-examples" |
There was a problem hiding this comment.
There appears to be a typo in the package name. It is specified as rust-ap-examples but it should be rust-api-examples as defined in Cargo.toml. This could lead to build failures. Please correct this and regenerate the Cargo.lock file.
name = "rust-api-examples"
| } | ||
|
|
||
| pub fn create_stream_with_hotwords(&self, hotwords: &str) -> OnlineStream { | ||
| let c = CString::new(hotwords).unwrap(); |
There was a problem hiding this comment.
Using .unwrap() here can cause the program to panic if the hotwords string contains a null byte. This function should return a Result<OnlineStream, ...> to allow the caller to handle the error gracefully, making the library API more robust.
|
|
||
| fn to_c_ptr(opt: &Option<String>, storage: &mut Vec<CString>) -> *const c_char { | ||
| if let Some(s) = opt { | ||
| let c = CString::new(s.as_str()).unwrap(); |
There was a problem hiding this comment.
Using .unwrap() here can cause the program to panic if an input string contains a null byte. It's better to handle the Result from CString::new gracefully, for instance by having to_c_ptr return a Result and propagating the error up. This would make the API more robust.
| impl Wave { | ||
| /// Read a WAV file using SherpaOnnx C API. | ||
| pub fn read(filename: &str) -> Option<Self> { | ||
| let c_filename = CString::new(filename).unwrap(); |
There was a problem hiding this comment.
Using .unwrap() on CString::new can cause a panic if the filename contains a null byte. While filenames on many systems cannot contain null bytes, it's safer for library code to handle this possibility. Consider changing Wave::read to return a Result to propagate the error.
| while k < samples.len() { | ||
| let end = (k + N).min(samples.len()); | ||
| let chunk = &samples[k..end]; | ||
| k = end; |
There was a problem hiding this comment.
This manual chunking logic is a bit C-style. A more idiomatic Rust approach would be to use the .chunks() method on the slice, which simplifies the code and improves readability. For example: for chunk in samples.chunks(N) { ... }.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
.github/workflows/test-rust.yaml (1)
84-99:cargo run --example versionruns twice — consider removing it from the "Test locally" step.Line 94 (
cargo run --example version) is now superseded byrun-version.shinsidetest-rust.sh. The "Test locally" step only needs to apply the local-pathsedpatch and docargo clean; the actual example runs can be fully delegated to the new "Run test" step.♻️ Suggested cleanup
- name: Test locally shell: bash run: | cd rust-api-examples sed -i.bak 's|^sherpa-onnx *=.*|sherpa-onnx = { path = "../sherpa-onnx/rust/sherpa-onnx" }|' Cargo.toml git diff . cargo clean - cargo run --example version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-rust.yaml around lines 84 - 99, The "Test locally" workflow step currently runs the example twice; remove the redundant cargo run --example version invocation from the step named "Test locally" and keep only the sed patch and cargo clean (and any necessary cd rust-api-examples) so the actual example execution is delegated to the "Run test" step that executes ./.github/scripts/test-rust.sh (which contains run-version.sh); update the "Test locally" step to no longer call cargo run --example version and ensure the "Run test" step remains unchanged so the example runs happen only once via test-rust.sh.sherpa-onnx/rust/sherpa-onnx/Cargo.toml (1)
24-25: Consider gatingserde/serde_jsonbehind a feature flag.These dependencies are only used in
online_asr.rsto deserialize recognizer result JSON into theRecognizerResultstruct. If users don't need online ASR functionality, they'd benefit from an optional feature that excludes these dependencies by default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/Cargo.toml` around lines 24 - 25, Make serde and serde_json optional behind a feature (e.g. "online-asr") so users who don't need online ASR don't pull them: in Cargo.toml mark serde and serde_json as optional (serde = { version = "1.0", features = ["derive"], optional = true } and serde_json = { version = "1.0", optional = true }) and add a feature entry like features = { "online-asr" = ["serde", "serde_json"] } (ensure default features don't enable it). In code (online_asr.rs) gate the ASR-specific code by feature: add #[cfg(feature = "online-asr")] to the module or functions that use RecognizerResult and use #[cfg_attr(feature = "online-asr", derive(serde::Deserialize))] (or conditionally use serde::Deserialize) so the RecognizerResult struct and any JSON deserialization are compiled only when the "online-asr" feature is enabled.sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs (3)
463-471:CString::new().unwrap()will panic without context on embedded null bytes.If any config string contains an interior
\0, this panics with a genericunwrapmessage. Consider.expect("string must not contain null bytes")or propagating the error. Same applies to line 372.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs` around lines 463 - 471, The helper function to_c_ptr currently uses CString::new(...).unwrap() which will panic with an opaque message on embedded null bytes; update to handle the error by replacing unwrap with a contextual expect (e.g., expect("string must not contain null bytes")) or propagate the Result to the caller so failures are reported instead of panicking; apply the same change to the other occurrence mentioned (the similar CString::new usage around line 372) and ensure storage.push only happens after CString is successfully created so you don't push partial/invalid state.
347-349: Consider whetherSendshould be implemented for cross-thread usage.Both
OnlineRecognizerandOnlineStreamcontain raw pointers, so they are neitherSendnorSyncby default. If the underlying C library is thread-safe (e.g., creating/using streams from different threads), you'll need explicitunsafe impl Send for OnlineRecognizer {}(and similarly forOnlineStream) to enable common multi-threaded patterns.Also applies to: 436-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs` around lines 347 - 349, The raw-pointer-containing structs OnlineRecognizer (ptr: *const sys::OnlineRecognizer) and OnlineStream need explicit thread-safety markers; add unsafe impl Send for OnlineRecognizer {} and unsafe impl Send for OnlineStream {} (and if the C API is safe for concurrent &T access also add unsafe impl Sync for them) to enable cross-thread usage, and include a short safety comment above each impl describing the C-library invariants that justify the unsafe impl (e.g., that the underlying sys::OnlineRecognizer/OnlineStream can be used from other threads concurrently).
15-23: Consider using#[derive(Default)]for simple config structs.For structs where all fields are
Option<String>(defaulting toNone), the manualDefaultimplementation is identical to what#[derive(Default)]would generate. This would reduce boilerplate forOnlineTransducerModelConfig,OnlineParaformerModelConfig,OnlineZipformer2CtcModelConfig,OnlineNemoCtcModelConfig, andOnlineToneCtcModelConfig.Also applies to: 41-48, 64-68, 83-87, 102-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs` around lines 15 - 23, The manual Default impls for simple config structs (OnlineTransducerModelConfig, OnlineParaformerModelConfig, OnlineZipformer2CtcModelConfig, OnlineNemoCtcModelConfig, OnlineToneCtcModelConfig) are redundant; remove those impl Default blocks and instead add #[derive(Default)] to each struct declaration (or include Default in their existing derive list) so Rust generates the identical default (all Option<String> -> None) automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-rust-package.yaml:
- Around line 73-76: The workflow step "Run test" currently invokes the script
using a relative executable path ("./.github/scripts/test-rust.sh") which fails
if the file lacks the executable bit; change the step to invoke the script with
an explicit shell interpreter (e.g., use "bash .github/scripts/test-rust.sh" or
"sh .github/scripts/test-rust.sh") so the Run test step runs reliably without
relying on the executable permission.
In `@rust-api-examples/examples/streaming_zipformer.rs`:
- Around line 46-83: The example hard-codes tail padding for 16kHz and contains
a small typo in the N comment; change the comment for the chunk-size constant N
to read "use any positive value you like" and compute tail padding from
wave.sample_rate() instead of using a fixed 4800: calculate padding_len =
(wave.sample_rate() as f32 * 0.3).ceil() as usize (or equivalent integer math)
and create tail_padding = vec![0.0f32; padding_len], then call
stream.accept_waveform(wave.sample_rate(), &tail_padding) so padding duration is
0.3s for any sample rate.
In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs`:
- Around line 187-194: The code casts buffer lengths to i32 unsafely for
tokens_buf_size (in the block building the tokens_buf pointer) and similarly for
hotwords_buf_size; replace the direct cast with a checked conversion using
i32::try_from(...) (or an equivalent checked conversion) and handle the failure
(e.g., expect with a clear message like "buffer exceeds i32::MAX" or return an
error) so oversized buffers cannot silently truncate; update both the
tokens_buf_size and hotwords_buf_size sites referenced in online_asr.rs to use
this checked conversion.
- Around line 457-461: The Drop impl for OnlineStream currently calls
sys::SherpaOnnxDestroyOnlineStream(self.ptr) unconditionally; add a null-pointer
check like in wave.rs to only call sys::SherpaOnnxDestroyOnlineStream when
self.ptr is not null (e.g., if !self.ptr.is_null()), leaving self.ptr unchanged
otherwise. Update the Drop implementation for the OnlineStream type so the
destructor is defensive against null pointers (consistent with create_stream and
the pattern used in wave.rs) and reference sys::SherpaOnnxDestroyOnlineStream
and the OnlineStream struct when making the change.
- Around line 366-375: create_stream and create_stream_with_hotwords currently
wrap raw FFI pointers into OnlineStream without checking for null; change their
signatures to return Option<OnlineStream>, call
sys::SherpaOnnxCreateOnlineStream and
sys::SherpaOnnxCreateOnlineStreamWithHotwords, check the returned ptr for null
(e.g. ptr.is_null()), and return None on null or Some(OnlineStream { ptr })
otherwise so Drop and other FFI calls never receive a null pointer; ensure
CString::new is still used for hotwords before the FFI call and keep ownership
semantics unchanged.
In `@sherpa-onnx/rust/sherpa-onnx/src/wave.rs`:
- Around line 13-16: The read function currently calls
CString::new(filename).unwrap() which will panic on interior NULs; change it to
handle the conversion failure and return None instead (e.g., use
CString::new(filename).ok() or match CString::new(filename) and return None on
Err) before calling sys::SherpaOnnxReadWave; keep the existing null-pointer
check for wave_ptr and only construct/return Some(Self) when both the CString
conversion succeeds and wave_ptr is non-null (refer to function read and symbol
sys::SherpaOnnxReadWave).
- Around line 33-39: The samples() method builds a slice from raw parts without
validating the raw pointer or num_samples; to avoid UB, check
(*self.inner).samples for null and ensure (*self.inner).num_samples >= 0 before
casting to usize and calling slice::from_raw_parts. If the pointer is null or
num_samples is negative, return an empty slice (or otherwise handle error)
instead of calling from_raw_parts; update the samples() implementation to
perform these guards using the existing self.inner, (*self.inner).samples and
(*self.inner).num_samples symbols.
---
Nitpick comments:
In @.github/workflows/test-rust.yaml:
- Around line 84-99: The "Test locally" workflow step currently runs the example
twice; remove the redundant cargo run --example version invocation from the step
named "Test locally" and keep only the sed patch and cargo clean (and any
necessary cd rust-api-examples) so the actual example execution is delegated to
the "Run test" step that executes ./.github/scripts/test-rust.sh (which contains
run-version.sh); update the "Test locally" step to no longer call cargo run
--example version and ensure the "Run test" step remains unchanged so the
example runs happen only once via test-rust.sh.
In `@sherpa-onnx/rust/sherpa-onnx/Cargo.toml`:
- Around line 24-25: Make serde and serde_json optional behind a feature (e.g.
"online-asr") so users who don't need online ASR don't pull them: in Cargo.toml
mark serde and serde_json as optional (serde = { version = "1.0", features =
["derive"], optional = true } and serde_json = { version = "1.0", optional =
true }) and add a feature entry like features = { "online-asr" = ["serde",
"serde_json"] } (ensure default features don't enable it). In code
(online_asr.rs) gate the ASR-specific code by feature: add #[cfg(feature =
"online-asr")] to the module or functions that use RecognizerResult and use
#[cfg_attr(feature = "online-asr", derive(serde::Deserialize))] (or
conditionally use serde::Deserialize) so the RecognizerResult struct and any
JSON deserialization are compiled only when the "online-asr" feature is enabled.
In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs`:
- Around line 463-471: The helper function to_c_ptr currently uses
CString::new(...).unwrap() which will panic with an opaque message on embedded
null bytes; update to handle the error by replacing unwrap with a contextual
expect (e.g., expect("string must not contain null bytes")) or propagate the
Result to the caller so failures are reported instead of panicking; apply the
same change to the other occurrence mentioned (the similar CString::new usage
around line 372) and ensure storage.push only happens after CString is
successfully created so you don't push partial/invalid state.
- Around line 347-349: The raw-pointer-containing structs OnlineRecognizer (ptr:
*const sys::OnlineRecognizer) and OnlineStream need explicit thread-safety
markers; add unsafe impl Send for OnlineRecognizer {} and unsafe impl Send for
OnlineStream {} (and if the C API is safe for concurrent &T access also add
unsafe impl Sync for them) to enable cross-thread usage, and include a short
safety comment above each impl describing the C-library invariants that justify
the unsafe impl (e.g., that the underlying sys::OnlineRecognizer/OnlineStream
can be used from other threads concurrently).
- Around line 15-23: The manual Default impls for simple config structs
(OnlineTransducerModelConfig, OnlineParaformerModelConfig,
OnlineZipformer2CtcModelConfig, OnlineNemoCtcModelConfig,
OnlineToneCtcModelConfig) are redundant; remove those impl Default blocks and
instead add #[derive(Default)] to each struct declaration (or include Default in
their existing derive list) so Rust generates the identical default (all
Option<String> -> None) automatically.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust-api-examples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/scripts/test-rust.sh.github/workflows/test-rust-package.yaml.github/workflows/test-rust.yamlc-api-examples/streaming-zipformer-c-api.crust-api-examples/Cargo.tomlrust-api-examples/README.mdrust-api-examples/examples/streaming_zipformer.rsrust-api-examples/run-streaming-zipformer.shrust-api-examples/run-version.shsherpa-onnx/rust/sherpa-onnx-sys/Cargo.tomlsherpa-onnx/rust/sherpa-onnx-sys/src/lib.rssherpa-onnx/rust/sherpa-onnx-sys/src/online_asr.rssherpa-onnx/rust/sherpa-onnx-sys/src/wave.rssherpa-onnx/rust/sherpa-onnx/Cargo.tomlsherpa-onnx/rust/sherpa-onnx/src/lib.rssherpa-onnx/rust/sherpa-onnx/src/online_asr.rssherpa-onnx/rust/sherpa-onnx/src/wave.rs
| - name: Run test | ||
| shell: bash | ||
| run: | | ||
| cd rust-api-examples | ||
| git checkout Cargo.toml | ||
| cargo clean | ||
| # cargo test --locked --all-features | ||
| cargo run --example version | ||
| ./.github/scripts/test-rust.sh |
There was a problem hiding this comment.
Avoid relying on executable bit for the script.
Using ./ will fail if the file isn’t marked executable. Consider invoking via bash to make the workflow robust.
✅ Suggested tweak
- ./.github/scripts/test-rust.sh
+ bash ./.github/scripts/test-rust.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-rust-package.yaml around lines 73 - 76, The workflow
step "Run test" currently invokes the script using a relative executable path
("./.github/scripts/test-rust.sh") which fails if the file lacks the executable
bit; change the step to invoke the script with an explicit shell interpreter
(e.g., use "bash .github/scripts/test-rust.sh" or "sh
.github/scripts/test-rust.sh") so the Run test step runs reliably without
relying on the executable permission.
| tokens_buf: self | ||
| .tokens_buf | ||
| .as_ref() | ||
| .map_or(ptr::null(), |buf| buf.as_ptr() as *const _), | ||
| tokens_buf_size: self | ||
| .tokens_buf | ||
| .as_ref() | ||
| .map_or(0, |buf| buf.len() as i32), |
There was a problem hiding this comment.
buf.len() as i32 silently truncates on large buffers.
Both tokens_buf_size (line 194) and hotwords_buf_size (line 339) cast usize to i32 without bounds checking. While unlikely to overflow in practice, a safe conversion would prevent subtle corruption:
i32::try_from(buf.len()).expect("buffer exceeds i32::MAX")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs` around lines 187 - 194, The
code casts buffer lengths to i32 unsafely for tokens_buf_size (in the block
building the tokens_buf pointer) and similarly for hotwords_buf_size; replace
the direct cast with a checked conversion using i32::try_from(...) (or an
equivalent checked conversion) and handle the failure (e.g., expect with a clear
message like "buffer exceeds i32::MAX" or return an error) so oversized buffers
cannot silently truncate; update both the tokens_buf_size and hotwords_buf_size
sites referenced in online_asr.rs to use this checked conversion.
| pub fn create_stream(&self) -> OnlineStream { | ||
| let ptr = unsafe { sys::SherpaOnnxCreateOnlineStream(self.ptr) }; | ||
| OnlineStream { ptr } | ||
| } | ||
|
|
||
| pub fn create_stream_with_hotwords(&self, hotwords: &str) -> OnlineStream { | ||
| let c = CString::new(hotwords).unwrap(); | ||
| let ptr = unsafe { sys::SherpaOnnxCreateOnlineStreamWithHotwords(self.ptr, c.as_ptr()) }; | ||
| OnlineStream { ptr } | ||
| } |
There was a problem hiding this comment.
Missing null-pointer check — potential undefined behavior.
create_stream and create_stream_with_hotwords wrap the FFI pointer without checking for null, unlike create() (line 359) which returns Option<Self>. If the C function returns null, every subsequent FFI call (including Drop) will pass a null pointer to C, which is undefined behavior.
Return Option<OnlineStream> for consistency and safety:
Proposed fix
- pub fn create_stream(&self) -> OnlineStream {
+ pub fn create_stream(&self) -> Option<OnlineStream> {
let ptr = unsafe { sys::SherpaOnnxCreateOnlineStream(self.ptr) };
- OnlineStream { ptr }
+ if ptr.is_null() { None } else { Some(OnlineStream { ptr }) }
}
- pub fn create_stream_with_hotwords(&self, hotwords: &str) -> OnlineStream {
+ pub fn create_stream_with_hotwords(&self, hotwords: &str) -> Option<OnlineStream> {
let c = CString::new(hotwords).unwrap();
let ptr = unsafe { sys::SherpaOnnxCreateOnlineStreamWithHotwords(self.ptr, c.as_ptr()) };
- OnlineStream { ptr }
+ if ptr.is_null() { None } else { Some(OnlineStream { ptr }) }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs` around lines 366 - 375,
create_stream and create_stream_with_hotwords currently wrap raw FFI pointers
into OnlineStream without checking for null; change their signatures to return
Option<OnlineStream>, call sys::SherpaOnnxCreateOnlineStream and
sys::SherpaOnnxCreateOnlineStreamWithHotwords, check the returned ptr for null
(e.g. ptr.is_null()), and return None on null or Some(OnlineStream { ptr })
otherwise so Drop and other FFI calls never receive a null pointer; ensure
CString::new is still used for hotwords before the FFI call and keep ownership
semantics unchanged.
| impl Drop for OnlineStream { | ||
| fn drop(&mut self) { | ||
| unsafe { sys::SherpaOnnxDestroyOnlineStream(self.ptr) } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null check in Drop, consistent with wave.rs pattern.
wave.rs (lines 43–52) guards the destroy call with if !self.inner.is_null(). OnlineStream::drop should do the same for defense in depth, especially given the absence of null checks in create_stream.
Proposed fix
impl Drop for OnlineStream {
fn drop(&mut self) {
- unsafe { sys::SherpaOnnxDestroyOnlineStream(self.ptr) }
+ unsafe {
+ if !self.ptr.is_null() {
+ sys::SherpaOnnxDestroyOnlineStream(self.ptr);
+ }
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Drop for OnlineStream { | |
| fn drop(&mut self) { | |
| unsafe { sys::SherpaOnnxDestroyOnlineStream(self.ptr) } | |
| } | |
| } | |
| impl Drop for OnlineStream { | |
| fn drop(&mut self) { | |
| unsafe { | |
| if !self.ptr.is_null() { | |
| sys::SherpaOnnxDestroyOnlineStream(self.ptr); | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs` around lines 457 - 461, The
Drop impl for OnlineStream currently calls
sys::SherpaOnnxDestroyOnlineStream(self.ptr) unconditionally; add a null-pointer
check like in wave.rs to only call sys::SherpaOnnxDestroyOnlineStream when
self.ptr is not null (e.g., if !self.ptr.is_null()), leaving self.ptr unchanged
otherwise. Update the Drop implementation for the OnlineStream type so the
destructor is defensive against null pointers (consistent with create_stream and
the pattern used in wave.rs) and reference sys::SherpaOnnxDestroyOnlineStream
and the OnlineStream struct when making the change.
| pub fn read(filename: &str) -> Option<Self> { | ||
| let c_filename = CString::new(filename).unwrap(); | ||
| let wave_ptr = unsafe { sys::SherpaOnnxReadWave(c_filename.as_ptr()) }; | ||
| if wave_ptr.is_null() { |
There was a problem hiding this comment.
Avoid panic on invalid filenames.
CString::new(...).unwrap() will panic on interior NULs; since this returns Option, you can return None instead.
🛠️ Suggested fix
- let c_filename = CString::new(filename).unwrap();
- let wave_ptr = unsafe { sys::SherpaOnnxReadWave(c_filename.as_ptr()) };
+ let c_filename = CString::new(filename).ok()?;
+ let wave_ptr = unsafe { sys::SherpaOnnxReadWave(c_filename.as_ptr()) };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn read(filename: &str) -> Option<Self> { | |
| let c_filename = CString::new(filename).unwrap(); | |
| let wave_ptr = unsafe { sys::SherpaOnnxReadWave(c_filename.as_ptr()) }; | |
| if wave_ptr.is_null() { | |
| pub fn read(filename: &str) -> Option<Self> { | |
| let c_filename = CString::new(filename).ok()?; | |
| let wave_ptr = unsafe { sys::SherpaOnnxReadWave(c_filename.as_ptr()) }; | |
| if wave_ptr.is_null() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/rust/sherpa-onnx/src/wave.rs` around lines 13 - 16, The read
function currently calls CString::new(filename).unwrap() which will panic on
interior NULs; change it to handle the conversion failure and return None
instead (e.g., use CString::new(filename).ok() or match CString::new(filename)
and return None on Err) before calling sys::SherpaOnnxReadWave; keep the
existing null-pointer check for wave_ptr and only construct/return Some(Self)
when both the CString conversion succeeds and wave_ptr is non-null (refer to
function read and symbol sys::SherpaOnnxReadWave).
| /// Get a slice of normalized samples | ||
| pub fn samples(&self) -> &[f32] { | ||
| unsafe { | ||
| let ptr = (*self.inner).samples; | ||
| let len = (*self.inner).num_samples as usize; | ||
| slice::from_raw_parts(ptr, len) | ||
| } |
There was a problem hiding this comment.
Guard against UB when building the samples slice.
Casting a negative num_samples to usize or using a null pointer will cause undefined behavior. Please validate before from_raw_parts.
🛡️ Suggested fix
pub fn samples(&self) -> &[f32] {
unsafe {
let ptr = (*self.inner).samples;
- let len = (*self.inner).num_samples as usize;
- slice::from_raw_parts(ptr, len)
+ let len = (*self.inner).num_samples;
+ if ptr.is_null() || len <= 0 {
+ return &[];
+ }
+ slice::from_raw_parts(ptr, len as usize)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Get a slice of normalized samples | |
| pub fn samples(&self) -> &[f32] { | |
| unsafe { | |
| let ptr = (*self.inner).samples; | |
| let len = (*self.inner).num_samples as usize; | |
| slice::from_raw_parts(ptr, len) | |
| } | |
| /// Get a slice of normalized samples | |
| pub fn samples(&self) -> &[f32] { | |
| unsafe { | |
| let ptr = (*self.inner).samples; | |
| let len = (*self.inner).num_samples; | |
| if ptr.is_null() || len <= 0 { | |
| return &[]; | |
| } | |
| slice::from_raw_parts(ptr, len as usize) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/rust/sherpa-onnx/src/wave.rs` around lines 33 - 39, The samples()
method builds a slice from raw parts without validating the raw pointer or
num_samples; to avoid UB, check (*self.inner).samples for null and ensure
(*self.inner).num_samples >= 0 before casting to usize and calling
slice::from_raw_parts. If the pointer is null or num_samples is negative, return
an empty slice (or otherwise handle error) instead of calling from_raw_parts;
update the samples() implementation to perform these guards using the existing
self.inner, (*self.inner).samples and (*self.inner).num_samples symbols.
There was a problem hiding this comment.
Pull request overview
This PR adds Rust bindings and examples for online/streaming ASR, including streaming Zipformer model support and WAV file reading, and updates CI to exercise the new Rust examples.
Changes:
- Added new Rust high-level APIs (
OnlineRecognizer,OnlineStream) and aWavehelper for reading WAV files via the C API. - Added new
sherpa-onnx-sysFFI bindings for online ASR and WAV reading; bumped Rust crate versions to0.1.1and addedserde/serde_json. - Added a streaming Zipformer Rust example plus CI/scripts to run Rust examples in workflows.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/rust/sherpa-onnx/src/wave.rs | New safe wrapper for reading WAV via C API and exposing samples. |
| sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs | New Rust API for streaming/online recognizer and stream management + JSON result parsing. |
| sherpa-onnx/rust/sherpa-onnx/src/lib.rs | Exports new online_asr and wave modules. |
| sherpa-onnx/rust/sherpa-onnx/Cargo.toml | Version bump + adds serde/serde_json dependencies. |
| sherpa-onnx/rust/sherpa-onnx-sys/src/wave.rs | New raw FFI bindings for wave read/free. |
| sherpa-onnx/rust/sherpa-onnx-sys/src/online_asr.rs | New raw FFI bindings for online recognizer/stream APIs. |
| sherpa-onnx/rust/sherpa-onnx-sys/src/lib.rs | Re-exports new FFI modules. |
| sherpa-onnx/rust/sherpa-onnx-sys/Cargo.toml | Version bump to 0.1.1. |
| rust-api-examples/run-version.sh | Convenience script to run version example. |
| rust-api-examples/run-streaming-zipformer.sh | Downloads model and runs the streaming Zipformer example. |
| rust-api-examples/examples/streaming_zipformer.rs | New streaming Zipformer example showing chunked audio feeding + endpointing. |
| rust-api-examples/README.md | Documents how to run the new examples. |
| rust-api-examples/Cargo.toml | Version bump + sherpa-onnx dependency bump. |
| rust-api-examples/Cargo.lock | Updated lockfile for new deps (currently contains an issue). |
| c-api-examples/streaming-zipformer-c-api.c | Enables endpoint detection in the C streaming zipformer example. |
| .github/workflows/test-rust.yaml | Runs consolidated Rust test script. |
| .github/workflows/test-rust-package.yaml | Runs consolidated Rust test script. |
| .github/scripts/test-rust.sh | New script to run Rust example(s) in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl Wave { | ||
| /// Read a WAV file using SherpaOnnx C API. | ||
| pub fn read(filename: &str) -> Option<Self> { | ||
| let c_filename = CString::new(filename).unwrap(); |
There was a problem hiding this comment.
Wave::read() uses CString::new(filename).unwrap(), which will panic if filename contains an interior NUL byte. This crate’s utils::file_exists() handles this case gracefully; consider returning None (or a Result) instead of panicking for invalid input.
| let c_filename = CString::new(filename).unwrap(); | |
| let c_filename = match CString::new(filename) { | |
| Ok(s) => s, | |
| Err(_) => return None, | |
| }; |
| let c = CString::new(hotwords).unwrap(); | ||
| let ptr = unsafe { sys::SherpaOnnxCreateOnlineStreamWithHotwords(self.ptr, c.as_ptr()) }; |
There was a problem hiding this comment.
create_stream_with_hotwords() uses CString::new(hotwords).unwrap(), which will panic for inputs containing interior NUL bytes. For a safe wrapper, prefer converting with error handling (e.g., returning None/Result or treating invalid input as an error) instead of panicking.
| let c = CString::new(hotwords).unwrap(); | |
| let ptr = unsafe { sys::SherpaOnnxCreateOnlineStreamWithHotwords(self.ptr, c.as_ptr()) }; | |
| let ptr = match CString::new(hotwords) { | |
| Ok(c) => unsafe { | |
| sys::SherpaOnnxCreateOnlineStreamWithHotwords(self.ptr, c.as_ptr()) | |
| }, | |
| Err(_) => unsafe { sys::SherpaOnnxCreateOnlineStream(self.ptr) }, | |
| }; |
|
|
||
| let mut segment_id = 0; | ||
|
|
||
| // use an use any postive value you like |
There was a problem hiding this comment.
Comment typos: “use an use any postive value you like” -> “use any positive value you like”.
| // use an use any postive value you like | |
| // use any positive value you like |
| let ptr = (*self.inner).samples; | ||
| let len = (*self.inner).num_samples as usize; | ||
| slice::from_raw_parts(ptr, len) |
There was a problem hiding this comment.
Wave::samples() calls slice::from_raw_parts(ptr, len) without guarding against ptr being null. If the C API ever returns a null samples pointer (even with len == 0), this becomes UB. Consider returning an empty slice when len == 0 or ptr.is_null() to keep this wrapper safe.
| let ptr = (*self.inner).samples; | |
| let len = (*self.inner).num_samples as usize; | |
| slice::from_raw_parts(ptr, len) | |
| if self.inner.is_null() { | |
| return &[]; | |
| } | |
| let ptr = (*self.inner).samples; | |
| let len = (*self.inner).num_samples as usize; | |
| if ptr.is_null() || len == 0 { | |
| &[] | |
| } else { | |
| slice::from_raw_parts(ptr, len) | |
| } |
| let c = CString::new(s.as_str()).unwrap(); | ||
| let ptr = c.as_ptr(); | ||
| storage.push(c); | ||
| ptr |
There was a problem hiding this comment.
to_c_ptr() uses CString::new(...).unwrap(), so any config string containing an interior NUL will panic during OnlineRecognizer::create(). Please handle CString::new() failure and propagate an error (or fall back to a null/empty pointer) so invalid user input can’t crash the process.
| let c = CString::new(s.as_str()).unwrap(); | |
| let ptr = c.as_ptr(); | |
| storage.push(c); | |
| ptr | |
| match CString::new(s.as_str()) { | |
| Ok(c) => { | |
| let ptr = c.as_ptr(); | |
| storage.push(c); | |
| ptr | |
| } | |
| Err(_) => { | |
| // If the string contains an interior NUL, fall back to a null pointer | |
| // to avoid panicking on invalid user input. | |
| ptr::null() | |
| } | |
| } |
| pub struct OnlineRecognizer { | ||
| ptr: *const sys::OnlineRecognizer, | ||
| } | ||
|
|
There was a problem hiding this comment.
OnlineRecognizer/OnlineStream contain only raw pointers, so they automatically become Send/Sync. The underlying C++ OnlineStream exposes non-thread-safe references (e.g., GetNumProcessedFrames() returns a reference after unlocking), so allowing these Rust types to be shared across threads can be unsound. Consider making these types !Send/!Sync (e.g., via a PhantomData<Rc<()>>) or explicitly unsafe impl Send/Sync only if thread-safety is guaranteed and documented.
| impl !Send for OnlineRecognizer {} | |
| impl !Sync for OnlineRecognizer {} |
| pub struct OnlineStream { | ||
| ptr: *const sys::OnlineStream, | ||
| } | ||
|
|
There was a problem hiding this comment.
OnlineStream is currently auto-Send/Sync because it only stores a raw pointer. Given the underlying C++ stream has mutable state and exposes non-thread-safe references, consider making this Rust type !Send/!Sync (or clearly documenting + explicitly implementing the auto traits if thread-safe is guaranteed).
| impl !Send for OnlineStream {} | |
| impl !Sync for OnlineStream {} |
| unsafe { | ||
| sys::SherpaOnnxOnlineStreamAcceptWaveform( | ||
| self.ptr, | ||
| sample_rate, | ||
| samples.as_ptr(), | ||
| samples.len() as i32, | ||
| ) |
There was a problem hiding this comment.
accept_waveform() casts samples.len() from usize to i32 without checking bounds. If a caller passes more than i32::MAX samples, this truncates/wraps and can lead to invalid reads in the C API. Consider validating the length and returning an error (or splitting internally) instead of doing an unchecked cast.
| unsafe { | |
| sys::SherpaOnnxOnlineStreamAcceptWaveform( | |
| self.ptr, | |
| sample_rate, | |
| samples.as_ptr(), | |
| samples.len() as i32, | |
| ) | |
| let mut offset: usize = 0; | |
| let total_len: usize = samples.len(); | |
| while offset < total_len { | |
| let remaining = total_len - offset; | |
| let chunk_len = remaining.min(i32::MAX as usize); | |
| let chunk = &samples[offset..offset + chunk_len]; | |
| unsafe { | |
| sys::SherpaOnnxOnlineStreamAcceptWaveform( | |
| self.ptr, | |
| sample_rate, | |
| chunk.as_ptr(), | |
| chunk_len as i32, | |
| ); | |
| } | |
| offset += chunk_len; |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sherpa-onnx/rust/sherpa-onnx/src/utils.rs (1)
5-9:⚠️ Potential issue | 🟡 MinorUpdate the stale doc comment to reflect the new panic-on-error behaviour.
The comment still describes the removed behavior:
- "If the pointer is null, an empty string is returned." — now panics via
assert!.- "If the C string is not valid UTF-8, a lossy UTF-8 conversion is used and the resulting string is leaked." — now panics via
.unwrap().📝 Proposed doc update
-/// Safely convert a C string pointer to a `'static` Rust string slice. -/// -/// If the pointer is null, an empty string is returned. -/// If the C string is not valid UTF-8, a lossy UTF-8 conversion is used -/// and the resulting string is leaked to obtain a `'static` lifetime. +/// Convert a C string pointer to a `'static` Rust string slice. +/// +/// # Panics +/// +/// Panics if `ptr` is null or if the C string contains invalid UTF-8. +/// +/// # Safety +/// +/// The caller must ensure that `ptr` points to a valid, null-terminated C +/// string whose memory has `'static` lifetime (e.g. a string literal +/// embedded in the binary).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/utils.rs` around lines 5 - 9, Update the doc comment for the C-string conversion function (the comment above c_str_to_static_str) to reflect that it no longer returns an empty string on null or does a lossy leak on invalid UTF-8: state that the function returns a &'static str but will panic if the input pointer is null (via assert!) and will panic on invalid UTF-8 (via .unwrap()), and remove any mention of returning empty strings or leaking lossy conversions.
♻️ Duplicate comments (4)
sherpa-onnx/rust/sherpa-onnx/src/wave.rs (2)
13-15:⚠️ Potential issue | 🟡 MinorAvoid panicking on invalid filenames.
CString::new(...).unwrap()can panic on interior NULs; this should returnNoneinstead since the API already usesOption.🛠️ Suggested fix
- let c_filename = CString::new(filename).unwrap(); - let wave_ptr = unsafe { sys::SherpaOnnxReadWave(c_filename.as_ptr()) }; + let c_filename = CString::new(filename).ok()?; + let wave_ptr = unsafe { sys::SherpaOnnxReadWave(c_filename.as_ptr()) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/wave.rs` around lines 13 - 15, The read method currently calls CString::new(filename).unwrap() which can panic on interior NULs; change it to handle that error and return None instead: replace the unwrap with a safe conversion (e.g., use CString::new(filename).ok()? or an if let Ok(c_filename) = CString::new(filename) { ... } else { return None }) before calling sys::SherpaOnnxReadWave so that read returns None for invalid filenames without panicking.
34-42:⚠️ Potential issue | 🟠 MajorGuard against UB when building the samples slice.
Casting a negative
num_samplestousizecan produce a huge length and trigger UB infrom_raw_parts. Guard before the cast.🛡️ Suggested fix
unsafe { let ptr = (*self.inner).samples; - let len = (*self.inner).num_samples as usize; - - if ptr.is_null() || len == 0 { + let len = (*self.inner).num_samples; + if ptr.is_null() || len <= 0 { &[] } else { - slice::from_raw_parts(ptr, len) + slice::from_raw_parts(ptr, len as usize) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/wave.rs` around lines 34 - 42, In samples(), avoid casting a potentially negative (*self.inner).num_samples to usize before calling slice::from_raw_parts: first load num_samples into a signed integer (e.g. let n = (*self.inner).num_samples) and check that n > 0 and that ptr is not null; only then convert n as usize and call slice::from_raw_parts(ptr, n as usize); otherwise return an empty slice. This ensures the guard runs before the cast and prevents UB; target the samples() method and the uses of (*self.inner).samples and (*self.inner).num_samples when applying the change.sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs (2)
364-373:⚠️ Potential issue | 🟠 MajorGuard against null stream pointers from FFI.
create_streamandcreate_stream_with_hotwordswrap raw pointers without null checks; if the C API returns null, subsequent calls (includingDrop) invoke FFI with a null pointer (UB). ReturnOption<OnlineStream>(orResult) and add a null guard inDrop. Call sites will need to handleNone.🛠️ Proposed fix
- pub fn create_stream(&self) -> OnlineStream { + pub fn create_stream(&self) -> Option<OnlineStream> { let ptr = unsafe { sys::SherpaOnnxCreateOnlineStream(self.ptr) }; - OnlineStream { ptr } + if ptr.is_null() { None } else { Some(OnlineStream { ptr }) } } - pub fn create_stream_with_hotwords(&self, hotwords: &str) -> OnlineStream { + pub fn create_stream_with_hotwords(&self, hotwords: &str) -> Option<OnlineStream> { let c = CString::new(hotwords).unwrap(); let ptr = unsafe { sys::SherpaOnnxCreateOnlineStreamWithHotwords(self.ptr, c.as_ptr()) }; - OnlineStream { ptr } + if ptr.is_null() { None } else { Some(OnlineStream { ptr }) } } @@ impl Drop for OnlineStream { fn drop(&mut self) { - unsafe { sys::SherpaOnnxDestroyOnlineStream(self.ptr) } + unsafe { + if !self.ptr.is_null() { + sys::SherpaOnnxDestroyOnlineStream(self.ptr); + } + } } }Verification (check C/C++ implementation for null-return semantics):
#!/bin/bash # Locate the implementation/ABI declarations for null-return behavior. rg -n "SherpaOnnxCreateOnlineStream|SherpaOnnxCreateOnlineStreamWithHotwords" -g '*.{c,cc,cpp,h,rs}'Also applies to: 455-458
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs` around lines 364 - 373, The FFI functions SherpaOnnxCreateOnlineStream and SherpaOnnxCreateOnlineStreamWithHotwords can return null but create_stream and create_stream_with_hotwords wrap the raw pointer without checks; change these two functions to return Option<OnlineStream> (or Result) and check the returned ptr for null before constructing OnlineStream (return None or Err on null), update call sites accordingly, and modify OnlineStream's Drop implementation to guard against a null ptr before calling the FFI destructor (i.e., only call sys::...Free/Destroy when ptr is non-null) to avoid undefined behavior.
187-194:⚠️ Potential issue | 🟡 MinorAvoid unchecked
usize -> i32truncation for buffer sizes.
buf.len() as i32can silently truncate for large in-memory tokens/hotwords buffers. Use a checked conversion (or surface an error) so oversized buffers fail deterministically.🛡️ Proposed fix
- tokens_buf_size: self - .tokens_buf - .as_ref() - .map_or(0, |buf| buf.len() as i32), + tokens_buf_size: self + .tokens_buf + .as_ref() + .map_or(0, |buf| i32::try_from(buf.len()).expect("tokens_buf exceeds i32::MAX")), @@ - hotwords_buf_size: self - .hotwords_buf - .as_ref() - .map_or(0, |buf| buf.len() as i32), + hotwords_buf_size: self + .hotwords_buf + .as_ref() + .map_or(0, |buf| i32::try_from(buf.len()).expect("hotwords_buf exceeds i32::MAX")),Verification (confirm FFI struct field types and sizes):
#!/bin/bash # Inspect the FFI struct field types for buffer sizes. rg -n "tokens_buf_size|hotwords_buf_size" -g 'online_asr.rs'Also applies to: 330-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs` around lines 187 - 194, The code unsafely casts buffer lengths with "buf.len() as i32" (in the construction of tokens_buf_size and the similar hotwords_buf_size) which can truncate on large buffers; replace the direct cast with a checked conversion (e.g., use i32::try_from(buf.len()) or buf.len().try_into()) and propagate an error (or return a Result) when the conversion fails instead of silently truncating; update the initialization sites that set tokens_buf_size and hotwords_buf_size to perform the checked conversion and handle the error path deterministically (same fix for the other occurrence around lines 330–337).
🧹 Nitpick comments (1)
sherpa-onnx/rust/sherpa-onnx/src/utils.rs (1)
14-16: Preferexpectover bareunwrapfor a meaningful panic message.A bare
.unwrap()produces a panic with no context. Using.expect(...)immediately communicates what went wrong and which invariant was violated.♻️ Proposed fix
unsafe { CStr::from_ptr(ptr) .to_str() - .unwrap() + .expect("C string from FFI is not valid UTF-8") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/rust/sherpa-onnx/src/utils.rs` around lines 14 - 16, Replace the bare .unwrap() on the CStr conversion with .expect(...) to give a clear panic message; locate the call chain CStr::from_ptr(ptr).to_str().unwrap() in utils.rs and change it to use expect with a descriptive message like "failed to convert C string to &str (invalid UTF-8) for ptr" so the panic explains the invariant violation and helps debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sherpa-onnx/rust/sherpa-onnx/src/utils.rs`:
- Around line 5-9: Update the doc comment for the C-string conversion function
(the comment above c_str_to_static_str) to reflect that it no longer returns an
empty string on null or does a lossy leak on invalid UTF-8: state that the
function returns a &'static str but will panic if the input pointer is null (via
assert!) and will panic on invalid UTF-8 (via .unwrap()), and remove any mention
of returning empty strings or leaking lossy conversions.
---
Duplicate comments:
In `@sherpa-onnx/rust/sherpa-onnx/src/online_asr.rs`:
- Around line 364-373: The FFI functions SherpaOnnxCreateOnlineStream and
SherpaOnnxCreateOnlineStreamWithHotwords can return null but create_stream and
create_stream_with_hotwords wrap the raw pointer without checks; change these
two functions to return Option<OnlineStream> (or Result) and check the returned
ptr for null before constructing OnlineStream (return None or Err on null),
update call sites accordingly, and modify OnlineStream's Drop implementation to
guard against a null ptr before calling the FFI destructor (i.e., only call
sys::...Free/Destroy when ptr is non-null) to avoid undefined behavior.
- Around line 187-194: The code unsafely casts buffer lengths with "buf.len() as
i32" (in the construction of tokens_buf_size and the similar hotwords_buf_size)
which can truncate on large buffers; replace the direct cast with a checked
conversion (e.g., use i32::try_from(buf.len()) or buf.len().try_into()) and
propagate an error (or return a Result) when the conversion fails instead of
silently truncating; update the initialization sites that set tokens_buf_size
and hotwords_buf_size to perform the checked conversion and handle the error
path deterministically (same fix for the other occurrence around lines 330–337).
In `@sherpa-onnx/rust/sherpa-onnx/src/wave.rs`:
- Around line 13-15: The read method currently calls
CString::new(filename).unwrap() which can panic on interior NULs; change it to
handle that error and return None instead: replace the unwrap with a safe
conversion (e.g., use CString::new(filename).ok()? or an if let Ok(c_filename) =
CString::new(filename) { ... } else { return None }) before calling
sys::SherpaOnnxReadWave so that read returns None for invalid filenames without
panicking.
- Around line 34-42: In samples(), avoid casting a potentially negative
(*self.inner).num_samples to usize before calling slice::from_raw_parts: first
load num_samples into a signed integer (e.g. let n = (*self.inner).num_samples)
and check that n > 0 and that ptr is not null; only then convert n as usize and
call slice::from_raw_parts(ptr, n as usize); otherwise return an empty slice.
This ensures the guard runs before the cast and prevents UB; target the
samples() method and the uses of (*self.inner).samples and
(*self.inner).num_samples when applying the change.
---
Nitpick comments:
In `@sherpa-onnx/rust/sherpa-onnx/src/utils.rs`:
- Around line 14-16: Replace the bare .unwrap() on the CStr conversion with
.expect(...) to give a clear panic message; locate the call chain
CStr::from_ptr(ptr).to_str().unwrap() in utils.rs and change it to use expect
with a descriptive message like "failed to convert C string to &str (invalid
UTF-8) for ptr" so the panic explains the invariant violation and helps
debugging.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust-api-examples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
rust-api-examples/examples/streaming_zipformer.rssherpa-onnx/rust/sherpa-onnx/src/online_asr.rssherpa-onnx/rust/sherpa-onnx/src/utils.rssherpa-onnx/rust/sherpa-onnx/src/wave.rs
Summary by CodeRabbit
New Features
Chores