This code acts differently if you add a println! just after line 377.
With println => unwrap panic
Without println => crash
I'm on latest nightly toolchain, Windows x64
You'll need cmake and clang to build the crate.
Things like this usually point at UB somewhere in the code. Have you spot checked for buggy unsafe code?
I peeked around the ffi parts of your source and unfortunately nothing too fishy has stood out to me yet.
When it compiles mbedtls-sys I get some fishy-looking type warnings from wrapper.c:
Full warnings
warning: wrapper.c: In function ‘wrapper_get_iv_size’:
warning: wrapper.c:5:92: warning: passing argument 1 of ‘mbedtls_cipher_get_iv_size’ from incompatible pointer type [-Wincompatible-pointer-types]
warning: #define WRAPPER(n, t) t NAME(n)(const mbedtls_cipher_context_t *ctx) { return NAME_MBED(n)(&ctx); }
warning: ^
warning: wrapper.c:8:1: note: in expansion of macro ‘WRAPPER’
warning: WRAPPER(get_iv_size, int)
warning: ^~~~~~~
warning: In file included from wrapper.h:2:0,
warning: from wrapper.c:1:
warning: mbedtls/include/mbedtls/cipher.h:409:19: note: expected ‘const mbedtls_cipher_context_t * {aka const struct <anonymous> *}’ but argument is of type ‘const mbedtls_cipher_context_t ** {aka const struct <anonymous> **}’
warning: static inline int mbedtls_cipher_get_iv_size( const mbedtls_cipher_context_t *ctx )
warning: ^~~~~~~~~~~~~~~~~~~~~~~~~~
warning: wrapper.c: In function ‘wrapper_get_block_size’:
warning: wrapper.c:5:92: warning: passing argument 1 of ‘mbedtls_cipher_get_block_size’ from incompatible pointer type [-Wincompatible-pointer-types]
warning: #define WRAPPER(n, t) t NAME(n)(const mbedtls_cipher_context_t *ctx) { return NAME_MBED(n)(&ctx); }
warning: ^
warning: wrapper.c:9:1: note: in expansion of macro ‘WRAPPER’
warning: WRAPPER(get_block_size, unsigned int)
warning: ^~~~~~~
warning: In file included from wrapper.h:2:0,
warning: from wrapper.c:1:
warning: mbedtls/include/mbedtls/cipher.h:374:28: note: expected ‘const mbedtls_cipher_context_t * {aka const struct <anonymous> *}’ but argument is of type ‘const mbedtls_cipher_context_t ** {aka const struct <anonymous> **}’
warning: static inline unsigned int mbedtls_cipher_get_block_size( const mbedtls_cipher_context_t *ctx )
warning: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
warning: wrapper.c: In function ‘wrapper_get_key_bitlen’:
warning: wrapper.c:5:92: warning: passing argument 1 of ‘mbedtls_cipher_get_key_bitlen’ from incompatible pointer type [-Wincompatible-pointer-types]
warning: #define WRAPPER(n, t) t NAME(n)(const mbedtls_cipher_context_t *ctx) { return NAME_MBED(n)(&ctx); }
warning: ^
warning: wrapper.c:10:1: note: in expansion of macro ‘WRAPPER’
warning: WRAPPER(get_key_bitlen, int)
warning: ^~~~~~~
warning: In file included from wrapper.h:2:0,
warning: from wrapper.c:1:
warning: mbedtls/include/mbedtls/cipher.h:462:19: note: expected ‘const mbedtls_cipher_context_t * {aka const struct <anonymous> *}’ but argument is of type ‘const mbedtls_cipher_context_t ** {aka const struct <anonymous> **}’
warning: static inline int mbedtls_cipher_get_key_bitlen( const mbedtls_cipher_context_t *ctx )
warning: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Compiling r-mbedtls v0.1.0 (file:///home/lampam/asd/clone/r-mbedtls)
Finished dev [unoptimized + debuginfo] target(s) in 5.92 secs
These are "fixed" by removing the & from the macro in wrapper.c, but as I look around it does not appear these functions are even used as of yet, so these seem to be a red herring.
When I try to run the crypt test, I just get this:
$ cargo test -- crypt
Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
Running target/debug/deps/r_mbedtls-db9cc3c010a4dc01
running 1 test
fatal runtime error: allocator memory exhausted
error: An unknown error occurred
Adding a println!() after line 377 eliminates the "memory exhausted" line, but nothing else changes.
Then again, that's typical UB for you; not much to go on because too much is possible.
Unrelated to your issue but wrong:
#[test]
fn version() {
unsafe {
let ptr = CString::new("").unwrap().into_raw();
mbedtls_sys::mbedtls_version_get_string(ptr);
assert_eq!(
CString::from_raw(ptr).into_string().unwrap(),
CStr::from_bytes_with_nul(mbedtls_sys::MBEDTLS_VERSION_STRING)
.unwrap()
.to_string_lossy()
);
}
}
This is UB for a number of reasons:
- Allowing mbedtls to write beyond the extent of the original allocation (which we only know was large enough to hold a single NUL)
- Reconstructing a CString after it's length has changed due to the erasure of a NUL byte.
(CString::from_rawrecomputes the length based on NUL bytes, and then passes this value back to the allocator upon destruction)
Also not your issue, but you should update your bindgen dependency. You have an old version which produces #[repr(C)] enums which can be very dangerous for values received through FFI (since rust enums are not allowed to hold invalid values)
The latest version of bindgen will make these enums be type aliases for integers. The downside is that all the variants will now have abominable names like mbedtls_cipher_type_t_MBEDTLS_CIPHER_AES_128_CBC.
1 Like
In an attempt to detect the UB, you may want to activate debugging symbols, switch back from jemalloc to the system allocator (glibc is surprisingly good at trapping on some common memory corruption patterns), and if that does not yet reveal the flaw run the resulting binary through valgrind.
Is it possible that some struct gets dropped after dereferencing in rust?
I glanced at the code and didn’t find anything obviously wrong either. Which structs are you thinking of? There’s not much Rust code before you hit FFI.
One question I did have while looking at the code is whether you’re sure the output buffer (Vec) is of sufficient size? It’s sized to input byte length + block size: is that guaranteed to be the upper bound?
A few things I’d try in addition to what @HadrienG suggested:
- Write a C driver that does the same thing as the Rust code and see what happens. You can also run it with a sanitizer or two (asan and ubsan).
- Allocate a much larger output buffer just to see if the issue still appears (and check how many bytes were written to it by the C code).
- If all good thus far, try running this on Linux and/or different Rust compiler version. There’s always a chance of a miscompile albeit low, and you don’t have much Rust code to begin with. But, it’s possible.
The buffer should be large enough, according to documents.
Will try to compile and debug in an docker container.
You mentioned premature drops so I did another take. What is the return type of these calls mbedtls_sys::mbedtls_cipher_info_from_type(t)? And the as_ref() calls on those?
Pointer to a non-opaque struct (or null if not found)
Who allocs and frees the struct?
This might be a dumb thought (I've never worked with non-opaque types returned by pointers through FFI) but could there be alignment issues? I'm not sure how conservative repr(C) is with regards to alignment.
Might be worth asserting that the pointer is aligned before as_ref().
If it isn't, wrapper functions on the C side may help extract these values properly.
All variants of the struct should be static
As in they are static constants, or that they live in static memory that may be changed?
(note: in your case, if it's the latter then it might not be causing this issue, but it will be a hazard for thread safety)
They are defined using static const in C code.
Michael Lamparski rust_lang@discoursemail.com 于 2018年2月15日周四 20:54写道：
If use the wrapper instead (after fixing the warnings), I get a stack overflow.
PS > cargo test crypt
Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
Running target\debug\deps\r_mbedtls-7937db4e967216b7.exe
running 1 test
thread 'cipher::test::crypt' has overflowed its stack
error: test failed, to rerun pass '--lib'
There must be something wrong in my compiler... The official examples also crash.