Closed Skepfyr closed 5 months ago
hi Jack,
thank you for your contribution. Couple of notes here:
assume_init()
andSend/Sync
stuff, etc. simply result from fighting rust compiler and borrow checker. I just did what I had to do to compile and make it run :) I have never been really super guru in rust plus don't really have any c/c++ experience so doing this library which is basically wrapper around native unsafe code was a big adventure for me :)so let's start with rebasing this so that it is mergeable.
thanks
Adam
Thanks for taking a look, I've rebased so it's now mergeable. Given you've said that you want this library to conservative and safe, why are you using MaybeUninit at all? It almost certainly doesn't provide a meaningful perf improvement and introduces the possibility of some nasty unsoundness. Why not just initialize all these handles to the null pointer (as it looks like you did before 6312562)?
ok, so this will be really difficult discussion for me since my knowledge of rust is simply not that good (and I haven't been actively using it for some time already) :)
typical use of MaybeUninit::uninit().assume_init();
in my code is as per below:
impl AudioConfig {
fn from_handle(handle: SPXAUDIOCONFIGHANDLE) -> Result<AudioConfig> {
unsafe {
let mut prop_bag_handle: SPXPROPERTYBAGHANDLE = MaybeUninit::uninit().assume_init();
let ret = audio_config_get_property_bag(handle, &mut prop_bag_handle);
convert_err(ret, "AudioConfig::from_handle error")?;
let property_bag = PropertyCollection::from_handle(prop_bag_handle);
let result = AudioConfig {
handle: SmartHandle::create("AudioConfig", handle, audio_config_release),
properties: property_bag,
};
Ok(result)
}
}
The way I understand it is that prop_bag_handle
is null pointer that is actually populated with some address by calling functionlet ret = audio_config_get_property_bag(handle, &mut prop_bag_handle);
So assume_init()
is just the way to silence the compiler from complaining and making it believe audio_config_get_property_bag will properly populate the pointer.
I am still fighting to understand what kind of nasty error could this code actually cause. Can you give me some example of some nasty situation that can occur in my code and cannot in yours?
Another thing: why do you think this is breaking change? I tried to recompile my project that is using this library using your branch and it still works (of course I am using just small subset of features).
sorry for stupid questions but it has been really long time since I actively developed in rust and worked with this code base :)
in general I think this change is OK, this is my clippy call in CI/CD:
- name: Run clippy
run: cargo clippy -- -D warnings -A clippy::uninit_assumed_init -A clippy::bool_comparison -A clippy::not_unsafe_ptr_arg_deref
so I was already silencing this lint (-A clippy::uninit_assumed_init), I just don not remember why https://github.com/jabber-tools/cognitive-services-speech-sdk-rs/commit/6312562b55dd92dad640b679c57646f9c5ab954d this was done.
I just need to better understand the benefit and ramifications before I merge. thx!
So MaybeUninit::uninit()
creates uninitialized bytes (not a null pointer which is generally the value 0), this means the compiler is allowed to assume they take any value (it's actually worse than that) but suffice to say that in Rust, the compiler assumes that bytes can never be initialised unless they're contained in a MaybeUninit
wrapper. If you break this then technically anything can happen, and you can get weird mis-compilations that make no sense.
Given that this is quite an esoteric bit of Rust, maybe it would be more sensible if I replace all the maybeuninit uses with initializing the handles to the null pointer? This won't realistically have any impact (theoretically it's slightly slower but it's at most a single instruction), and will make it easier to understand.
The reason I say it's breaking is because I've changed some public functions to be unsafe, so users of this library will need to change code that looks like:
fn foo(handle: Handle) {
Blah::from_handle(handle)
}
to
fn foo(handle: Handle) {
unsafe {
Blah::from_handle(handle)
}
}
ok, so if using null pointer everywhere instead of MaybeUninit
means we don't need to make additional functions unsafe it will be definitely preferred, especially if it will make code more concise and easier to read.
Unfortunately using null will still require making those additional methods unsafe, it just reduces the general risk of the repo.
ok, in that case going with MaybeUninit makes more sense. But asking again: we do all those functions need to be now marked as unsafe? Because it did compile even before this: https://github.com/jabber-tools/cognitive-services-speech-sdk-rs/pull/15/commits/f747d7bfc6b3a30c89fc00fac950e1e9fbfa0b8a
whole function was anyway in unsafe block, so I do not understand why do we need to have both unsafe function with unsafe block
so basically there is no need for breaking change.
In Rust, unsafe
has two different meanings depending on where it's used.
The first way is if it's in an unsafe block like this:
fn foo() {
unsafe {
do_unsafe_thing();
}
}
In this case, you are promising to the compiler that you know what you're doing and that you are still maintaining all of it's assumptions about Rust (things like bools can only be 0 or 1, references are live, etc).
The second way is if you use it in a function definition:
unsafe fn foo(ptr: *mut Bar) {
}
In this situation it has the "opposite" meaning, it means that anyone calling this function must promise some additional things are true. For example, std::ptr::read is unsafe because it requires that:
- src must be valid for reads.
- src must be properly aligned. Use read_unaligned if this is not the case.
- src must point to a properly initialized value of type T.
In this case specifically, the from_handle
functions need to be unsafe because otherwise someone could reasonably call them with invalid handles like:
fn main() {
let collection = PropertyCollection::from_handle(std::ptr::null_mut());
collection.set_property(PropertyId::SpeechServiceConnectionKey, "foo");
}
That code would (probably) cause a segfault when the C code tries to dereference the collection which points to null. That kind of thing should be impossible in safe Rust. Now I've made the from handle functions unsafe
(to call) any user now has to promise that the thing they pass in *isn't null (and it's a valid pointer etc) so it's easier to spot this mistake.
shit, you are right, just reading it here https://doc.rust-lang.org/reference/unsafe-keyword.html#unsafe-blocks-unsafe- :)
I always thought this is only about scoping (whole function vs subset of it).
one more question: since we have now whole function unsafe, removing respective unsafe block within it will not cause compilation error, any more. Do you think it is good idea to remove it? So having something like this:
impl AudioConfig {
/// # Safety
/// `handle` must be a valid handle to a live audio config.
unsafe fn from_handle(handle: SPXAUDIOCONFIGHANDLE) -> Result<AudioConfig> {
// unsafe block not needed any more!!!!!!!
// unsafe {
let mut prop_bag_handle: MaybeUninit<SPXPROPERTYBAGHANDLE> = MaybeUninit::uninit();
let ret = audio_config_get_property_bag(handle, prop_bag_handle.as_mut_ptr());
convert_err(ret, "AudioConfig::from_handle error")?;
let property_bag = PropertyCollection::from_handle(prop_bag_handle.assume_init());
let result = AudioConfig {
handle: SmartHandle::create("AudioConfig", handle, audio_config_release),
properties: property_bag,
};
Ok(result)
// }
}
...
No we should keep those unsafe blocks because of https://doc.rust-lang.org/nightly/edition-guide/rust-2024/unsafe-op-in-unsafe-fn.html
ok, unsafe approach makes sense.
We have recently added windows build support, see PR https://github.com/jabber-tools/cognitive-services-speech-sdk-rs/pull/14
i32 is used instead of u32 in some places.
I just tried to compile on windows and getting lot of errors, new changes must compile on all supported platforms, windows, linux, macos.
Compiling cognitive-services-speech-sdk-rs v0.3.1 (C:\Users\adamb\dev\cognitive-services-speech-sdk-rs-PRs\cognitive-services-speech-sdk-rs)
error[E0308]: mismatched types
--> src\speech\audio_data_stream.rs:71:73
|
71 | let ret = audio_data_stream_get_status(self.handle.inner(), &mut status);
| ---------------------------- ^^^^^^^^^^^ expected `*mut i32`, found `&mut u32`
| |
| arguments to this function are incorrect
|
= note: expected raw pointer `*mut i32`
found mutable reference `&mut u32`
note: function defined here
--> src\ffi/bindings.rs:1498:12
|
1498 | pub fn audio_data_stream_get_status(
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error[E0308]: mismatched types
--> src\speech\audio_data_stream.rs:75:46
|
75 | return Ok(StreamStatus::from_i32(status));
| ---------------------- ^^^^^^ expected `i32`, found `u32`
| |
| arguments to this function are incorrect
|
note: associated function defined here
--> src\common\stream_status.rs:31:12
|
31 | pub fn from_i32(status: i32) -> Self {
| ^^^^^^^^ -----------
help: you can convert a `u32` to an `i32` and panic if the converted value doesn't fit
|
75 | return Ok(StreamStatus::from_i32(status.try_into().unwrap()));
| ++++++++++++++++++++
error[E0308]: mismatched types
--> src\speech\cancellation_details.rs:24:17
|
22 | let mut ret = synth_result_get_reason_canceled(
| -------------------------------- arguments to this function are incorrect
23 | speech_synthesis_result.handle.inner(),
24 | &mut reason,
| ^^^^^^^^^^^ expected `*mut i32`, found `&mut u32`
|
= note: expected raw pointer `*mut i32`
found mutable reference `&mut u32`
note: function defined here
--> src\ffi/bindings.rs:1258:12
|
1258 | pub fn synth_result_get_reason_canceled(
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error[E0308]: mismatched types
--> src\speech\cancellation_details.rs:34:17
|
32 | ret = synth_result_get_canceled_error_code(
| ------------------------------------ arguments to this function are incorrect
33 | speech_synthesis_result.handle.inner(),
34 | &mut error_code,
| ^^^^^^^^^^^^^^^ expected `*mut i32`, found `&mut u32`
|
= note: expected raw pointer `*mut i32`
found mutable reference `&mut u32`
note: function defined here
--> src\ffi/bindings.rs:1264:12
|
1264 | pub fn synth_result_get_canceled_error_code(
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error[E0308]: mismatched types
--> src\speech\cancellation_details.rs:48:58
|
48 | reason: CancellationReason::from_i32(reason),
| ---------------------------- ^^^^^^ expected `i32`, found `u32`
| |
| arguments to this function are incorrect
|
note: associated function defined here
--> src\common\cancellation_reason.rs:21:12
|
21 | pub fn from_i32(code: i32) -> Self {
| ^^^^^^^^ ---------
help: you can convert a `u32` to an `i32` and panic if the converted value doesn't fit
|
48 | reason: CancellationReason::from_i32(reason.try_into().unwrap()),
| ++++++++++++++++++++
error[E0308]: mismatched types
--> src\speech\cancellation_details.rs:49:65
|
49 | error_code: CancellationErrorCode::from_i32(error_code),
| ------------------------------- ^^^^^^^^^^ expected `i32`, found `u32`
| |
| arguments to this function are incorrect
|
note: associated function defined here
--> src\common\cancellation_error_code.rs:46:12
|
46 | pub fn from_i32(code: i32) -> Self {
| ^^^^^^^^ ---------
help: you can convert a `u32` to an `i32` and panic if the converted value doesn't fit
|
49 | error_code: CancellationErrorCode::from_i32(error_code.try_into().unwrap()),
| ++++++++++++++++++++
error[E0308]: mismatched types
--> src\speech\speech_recognition_result.rs:50:45
|
50 | ret = result_get_reason(handle, &mut reason);
| ----------------- ^^^^^^^^^^^ expected `*mut i32`, found `&mut u32`
| |
| arguments to this function are incorrect
|
= note: expected raw pointer `*mut i32`
found mutable reference `&mut u32`
note: function defined here
--> src\ffi/bindings.rs:1204:12
|
1204 | pub fn result_get_reason(hresult: SPXRESULTHANDLE, reason: *mut Result_Reason) -> AZACHR;
| ^^^^^^^^^^^^^^^^^
error[E0308]: mismatched types
--> src\speech\speech_recognition_result.rs:87:49
|
87 | let reason = ResultReason::from_i32(reason);
| ---------------------- ^^^^^^ expected `i32`, found `u32`
| |
| arguments to this function are incorrect
|
note: associated function defined here
--> src\common\result_reason.rs:70:12
|
70 | pub fn from_i32(reason: i32) -> Self {
| ^^^^^^^^ -----------
help: you can convert a `u32` to an `i32` and panic if the converted value doesn't fit
|
87 | let reason = ResultReason::from_i32(reason.try_into().unwrap());
| ++++++++++++++++++++
error[E0308]: mismatched types
--> src\speech\speech_synthesis_result.rs:76:51
|
76 | ret = synth_result_get_reason(handle, &mut reason);
| ----------------------- ^^^^^^^^^^^ expected `*mut i32`, found `&mut u32`
| |
| arguments to this function are incorrect
|
= note: expected raw pointer `*mut i32`
found mutable reference `&mut u32`
note: function defined here
--> src\ffi/bindings.rs:1255:12
|
1255 | pub fn synth_result_get_reason(hresult: SPXRESULTHANDLE, reason: *mut Result_Reason) -> AZACHR;
| ^^^^^^^^^^^^^^^^^^^^^^^
error[E0308]: mismatched types
--> src\speech\speech_synthesis_result.rs:103:49
|
103 | let reason = ResultReason::from_i32(reason);
| ---------------------- ^^^^^^ expected `i32`, found `u32`
| |
| arguments to this function are incorrect
|
note: associated function defined here
--> src\common\result_reason.rs:70:12
|
70 | pub fn from_i32(reason: i32) -> Self {
| ^^^^^^^^ -----------
help: you can convert a `u32` to an `i32` and panic if the converted value doesn't fit
|
103 | let reason = ResultReason::from_i32(reason.try_into().unwrap());
| ++++++++++++++++++++
error[E0308]: mismatched types
--> src\speech\synthesis_voices_result.rs:38:62
|
38 | ret = synthesis_voices_result_get_reason(handle, &mut reason);
| ---------------------------------- ^^^^^^^^^^^ expected `*mut i32`, found `&mut u32`
| |
| arguments to this function are incorrect
|
= note: expected raw pointer `*mut i32`
found mutable reference `&mut u32`
note: function defined here
--> src\ffi/bindings.rs:1304:12
|
1304 | pub fn synthesis_voices_result_get_reason(
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error[E0308]: mismatched types
--> src\speech\synthesis_voices_result.rs:80:49
|
80 | let reason = ResultReason::from_i32(reason);
| ---------------------- ^^^^^^ expected `i32`, found `u32`
| |
| arguments to this function are incorrect
|
note: associated function defined here
--> src\common\result_reason.rs:70:12
|
70 | pub fn from_i32(reason: i32) -> Self {
| ^^^^^^^^ -----------
help: you can convert a `u32` to an `i32` and panic if the converted value doesn't fit
|
80 | let reason = ResultReason::from_i32(reason.try_into().unwrap());
| ++++++++++++++++++++
error[E0308]: mismatched types
--> src\speech\voice_info.rs:53:61
|
53 | let mut ret = voice_info_get_voice_type(handle, &mut voice_type);
| ------------------------- ^^^^^^^^^^^^^^^ expected `*mut i32`, found `&mut u32`
| |
| arguments to this function are incorrect
|
= note: expected raw pointer `*mut i32`
found mutable reference `&mut u32`
note: function defined here
--> src\ffi/bindings.rs:1350:12
|
1350 | pub fn voice_info_get_voice_type(
| ^^^^^^^^^^^^^^^^^^^^^^^^^
error[E0308]: mismatched types
--> src\speech\voice_info.rs:67:59
|
67 | let voice_type = SynthesisVoiceType::from_i32(voice_type);
| ---------------------------- ^^^^^^^^^^ expected `i32`, found `u32`
| |
| arguments to this function are incorrect
|
note: associated function defined here
--> src\common\synthesis_voice_type.rs:27:12
|
27 | pub fn from_i32(reason: i32) -> Self {
| ^^^^^^^^ -----------
help: you can convert a `u32` to an `i32` and panic if the converted value doesn't fit
|
67 | let voice_type = SynthesisVoiceType::from_i32(voice_type.try_into().unwrap());
| ++++++++++++++++++++
For more information about this error, try `rustc --explain E0308`.
error: could not compile `cognitive-services-speech-sdk-rs` (lib) due to 14 previous errors
PS C:\Users\adamb\dev\cognitive-services-speech-sdk-rs-PRs\cognitive-services-speech-sdk-rs>
Woops, sorry, that was the result of a slightly dodgy rebase. I've fixed that and tested it on Linux & Windows. I haven't got access to a Mac to test it there though.
on Mac seems to be OK
on windows compiles OK now. I tried some examples. This is how current main code (mic example) runs
[2024-04-30T18:28:11Z INFO recognizer::from_microphone] running from_microphone example...
[2024-04-30T18:28:11Z INFO recognizer::helpers] >set_session_started_cb SessionEvent { session_id: "b781cbae089c40f1849ac663b248b03d" }
[2024-04-30T18:28:14Z INFO recognizer::helpers] >set_speech_start_detected_cb RecognitionEvent { base: SessionEvent { session_id: "b781cbae089c40f1849ac663b248b03d" }, offset: 17500000 }
[2024-04-30T18:28:15Z INFO recognizer::helpers] >set_recognizing_cb "hello"
[2024-04-30T18:28:16Z INFO recognizer::helpers] >set_recognizing_cb "hello this"
[2024-04-30T18:28:16Z INFO recognizer::helpers] >set_recognizing_cb "hello this is"
[2024-04-30T18:28:17Z INFO recognizer::helpers] >set_recognizing_cb "hello this is test on"
[2024-04-30T18:28:17Z INFO recognizer::helpers] >set_recognizing_cb "hello this is test on windows"
[2024-04-30T18:28:18Z INFO recognizer::helpers] >set_recognized_cb SpeechRecognitionEvent { base: RecognitionEvent { base: SessionEvent { session_id: "b781cbae089c40f1849ac663b248b03d" }, offset: 17500000 }, result: SpeechRecognitionResult { result_id: "bd5e4773e760466a90f1f796ac7b74bb", reason: RecognizedSpeech, text: "Hello this is test on windows.", duration: "35600000", offset: "17500000" } }
your branch ends up like this
[2024-04-30T18:30:09Z INFO recognizer::from_microphone] running from_microphone example...
thread 'main' panicked at examples\recognizer\helpers.rs:61:88:
called `Result::unwrap()` on an `Err` value: Error { message: "SpeechRecognizer.from_config error: Exception with an error code: 0x38 (SPXERR_AUDIO_SYS_LIBRARY_NOT_FOUND)", caused_by: ApiError(56) }
stack backtrace:
0: std::panicking::begin_panic_handler
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library\std\src\panicking.rs:647
1: core::panicking::panic_fmt
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library\core\src\panicking.rs:72
2: core::result::unwrap_failed
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library\core\src\result.rs:1649
3: enum2$<core::result::Result<cognitive_services_speech_sdk_rs::speech::speech_recognizer::SpeechRecognizer,cognitive_services_speech_sdk_rs::error::Error> >::unwrap
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04\library\core\src\result.rs:1073
4: recognizer::helpers::speech_recognizer_from_audio_cfg
at .\examples\recognizer\helpers.rs:61
5: recognizer::helpers::speech_recognizer_default_mic
at .\examples\recognizer\helpers.rs:92
6: recognizer::from_microphone::run_example::async_fn$0
at .\examples\recognizer\from_microphone.rs:10
7: recognizer::main::async_block$0
at .\examples\recognizer\main.rs:23
8: tokio::runtime::park::impl$4::block_on::closure$0<enum2$<recognizer::main::async_block_env$0> >
at C:\Users\adamb\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.37.0\src\runtime\park.rs:281
9: tokio::runtime::coop::with_budget
at C:\Users\adamb\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.37.0\src\runtime\coop.rs:107
10: tokio::runtime::coop::budget
at C:\Users\adamb\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.37.0\src\runtime\coop.rs:73
11: tokio::runtime::park::CachedParkThread::block_on<enum2$<recognizer::main::async_block_env$0> >
at C:\Users\adamb\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.37.0\src\runtime\park.rs:281
12: tokio::runtime::context::blocking::BlockingRegionGuard::block_on<enum2$<recognizer::main::async_block_env$0> >
at C:\Users\adamb\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.37.0\src\runtime\context\blocking.rs:66
13: tokio::runtime::scheduler::multi_thread::impl$0::block_on::closure$0<enum2$<recognizer::main::async_block_env$0> >
at C:\Users\adamb\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.37.0\src\runtime\scheduler\multi_thread\mod.rs:87
14: tokio::runtime::context::runtime::enter_runtime<tokio::runtime::scheduler::multi_thread::impl$0::block_on::closure_env$0<enum2$<recognizer::main::async_block_env$0> >,tuple$<> >
at C:\Users\adamb\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.37.0\src\runtime\context\runtime.rs:65
15: tokio::runtime::scheduler::multi_thread::MultiThread::block_on<enum2$<recognizer::main::async_block_env$0> >
at C:\Users\adamb\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.37.0\src\runtime\scheduler\multi_thread\mod.rs:86
16: tokio::runtime::runtime::Runtime::block_on<enum2$<recognizer::main::async_block_env$0> >
at C:\Users\adamb\.cargo\registry\src\index.crates.io-6f17d22bba15001f\tokio-1.37.0\src\runtime\runtime.rs:351
17: recognizer::main
at .\examples\recognizer\main.rs:23
18: core::ops::function::FnOnce::call_once<void (*)(),tuple$<> >
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04\library\core\src\ops\function.rs:250
19: core::hint::black_box
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04\library\core\src\hint.rs:334
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
error: process didn't exit successfully: `target\debug\examples\recognizer.exe` (exit code: 101)
C:\Users\adamb\dev\cognitive-services-speech-sdk-rs-PRs\cognitive-services-speech-sdk-rs>
I think I need to implement some real unit tests soon:) Can you have a look what is wrong with the new code?
thanks
Adam
Are you sure? I don't think any changes I've made could have affected that (I'd expect my other PR to maybe cause that kind of problem). I can't reproduce that issue because cargo run
doesn't work at all for me (it can't find the core dll), it looks like you're hitting the issue that it can't find Microsoft.CognitiveServices.Speech.extension.audio.sys.dll
. I've got no idea why it could find one at not the other.
let me check again tommorow.
ok, my fault. somehow when I added to path speech sdk directly from target folder it could not find it. running from some simple folder like c:/tmp works well. sorry for confusion. I will prepare new version soon. thanks!
This PR fixes two sources of unsoundness that clippy spotted:
MaybeUninit::uninit().assume_init()
is unsound for almost all types as the only Rust type that's allowed to contain uninitialized bytes isMaybeUninit<T>
(and padding). I've fixed this by moving theassume_init
to after the call that should initialize the type.I've made all
from_handle
functions unsafe as they depend on the handle being passed to them being valid (and ownership of the type being passed into thefrom_handle
function although I haven't explicitly documented that bit). I believe this is a breaking change as I think they're part of the public API.I don't think this removes all the unsoundness in this crate, there's more in here that I'm suspicious of such as the Send & Sync impls and the fact that the unsafe blocks in general are quite large.