Fix segmentation fault in callbacks

[wangfu91]

This PR fixes #33, it refactors how event callback closures are managed in both the SpeechRecognizer and SpeechSynthesizer structs.

Instead of storing callback closures directly in these structs, a new internal CallbackBag struct is introduced and boxed, ensuring that callbacks reside at a stable heap address.

This change improves memory safety and allows the main structs to be moved freely without invalidating callback pointers.

Checklist:

• Fix the callbacks in SpeechRecognizer & add tests
• Fix SpeechSynthesizer & add tests
• Fix PullAudioInputStream & add tests
• Fix PushAudioOutputStream & add tests

[adambezecny]

wow, you are productive :) let's address PR31 first, there are two pending items before I can merge. can you have a look? 32 should be easy then. le't s address this one afterwards

[wangfu91]

let's address PR31 first, there are two pending items before I can merge. can you have a look?

Hi @adambezecny ,
I had a quick look at PR #31, but I didn’t see any review comments from you yet—perhaps they weren’t submitted? Could you double-check when you get a chance?

[adambezecny]

yeap, sorry I did forget to submit review for pr 31, did it know. can you have a look? thanks

[Copilot]

Pull Request Overview

This PR fixes segmentation faults in callback handling by refactoring how event callback closures are managed across multiple speech SDK components. Instead of storing callback closures directly in the main structs, a new internal CallbackBag struct is introduced and boxed to ensure callbacks remain at stable heap addresses when the main structs are moved.

• Introduces CallbackBag structs for SpeechRecognizer, SpeechSynthesizer, PullAudioInputStream, and PushAudioOutputStream
• Updates callback registration to use stable heap-allocated callback storage
• Adds comprehensive test coverage for segmentation fault scenarios across all affected components

Reviewed Changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/speech/speech_recognizer.rs	Refactors callback storage using boxed CallbackBag to prevent segfaults
src/speech/speech_synthesizer.rs	Implements callback bag pattern for synthesizer event callbacks
src/audio/pull_audio_input_stream.rs	Updates callback handling with stable heap allocation for input streams
src/audio/push_audio_output_stream.rs	Applies callback bag pattern to output stream callbacks
tests/recognizer_segfault_test.rs	Adds test case for speech recognizer callback stability
tests/synthesizer_segfault_test.rs	Adds test case for speech synthesizer callback stability
tests/pull_input_segfault_test.rs	Adds test case for pull input stream callback stability
tests/push_output_segfault_test.rs	Adds test case for push output stream callback stability
tests/common.rs	Introduces shared test utilities for speech recognition and synthesis
tests/integration_test.rs	Refactors existing tests to use new common utilities
Cargo.toml	Removes unused rust-embed dependency and cleans up formatting

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[adambezecny]

hi @wangfu91 give me more time to review please, I am extremely busy lately, really no time for this. I will try to start reviewing in 1-2 weeks from now. sorry :(

[wangfu91]

Hi @adambezecny ,
Thanks for the update!
Totally understand. I'm in no hurry for this PR to be merged, so feel free to prioritize other items as needed. :)

[adambezecny]

hi @wangfu91

probably will have not much time to review this week but I already started to be looking into it :)

one fundamental question so far: why to create separate boxed struct for callbacks rather then simply pinning callback attributes? please bear in mind my rust is very rusty, not actively using it for long time, so pardon me if the question is stupid. But I think by pinning the inner callback attributes you could save yourself from such massive refactoring. Also if I remember correctly boxing comes with cost since it forces data to be allocated on heap rather than stack.

I am also struggling to understand how boxing can prevent dangling pointer. Boxed data can still be moved right, leaving the C pointer not valid any more. Is this somehow enforcing pinning implicitly?

[wangfu91]

Hi @adambezecny

Thank you for taking the time to look at this! These are excellent questions. Let me try to explain the reasoning behind this approach.

why to create separate boxed struct for callbacks rather then simply pinning callback attributes?

This is a great question. The main reason is that pinning in Rust doesn't work on individual fields of a movable struct. Pinning guarantees that the memory of the pinned object will not be moved. If we wanted to make the callback's address stable, we would have to pin the entire SpeechRecognizer struct, this would affect the library's public API, every user would be forced to put the SpeechRecognizer in a Box or Pin themselves. This leaks the implementation detail and makes the library much harder to use.

The CallbackBag approach is an internal implementation detail that achieves the same safety guarantee without forcing any complexity onto the user.

Also if I remember correctly boxing comes with cost since it forces data to be allocated on heap rather than stack.

Yes, Box forces a heap allocation, which has a small performance cost compared to the stack, however, in this context the cost is negligible and well worth the safety it provides.

Boxed data can still be moved right, leaving the C pointer not valid anymore. Is this somehow enforcing pinning implicitly?

You are right that a Box itself can be moved. However, it's crucial to distinguish between moving the Box<T> (which is a smart pointer) and moving the T(the data the pointer points to).

• The Box<T> is a smart pointer that lives on the stack, it's just a single pointer-sized value.
• The T is the actual data, which Box allocates on the heap.

When we move a Box<T>, we are only moving the smart pointer on the stack, the data T on the heap does not move, its memory address remains stable, so we can pass this stable heap address as the context pointer to the C library.

This way, the SpeechRecognizer struct itself can be moved by the library users freely, but the callbacks will remain at a fixed memory address on the heap.

[adambezecny]

lgtm

[adambezecny]

@wangfu91
one fundamental question so far: why to create separate boxed struct for callbacks rather then simply pinning callback attributes? please bear in mind my rust is very rusty, not actively using it for long time, so pardon me if the question is stupid. But I think by pinning the inner callback attributes you could save yourself from such massive refactoring. Also if I remember correctly boxing comes with cost since it forces data to be allocated on heap rather than stack.

I am also struggling to understand how boxing can prevent dangling pointer. Boxed data can still be moved right, leaving the C pointer not valid any more. Is this somehow enforcing pinning implicitly?

[adambezecny]

this is excellent work, I am kind of speechless. Thank you for all the effort you are putting into this. Thanks to all your contributions this library got so much better after I stopped actively contributing to it on regular basis. Really appreciated! thanks :-)

[adambezecny]

published v1.3.0 with latest code

[wangfu91]

Hi Adam,
Thank you so much for the kind words! It means a lot coming from you. I’ve really enjoyed working on this library and building on the great foundation you created. I’m glad my contributions have been helpful, and I’m looking forward to continuing to improve it. :)