Translate char as core::ffi::c_char instead of u8#194
Conversation
|
@nunoplopes this is ready for review |
|
What's the advantage of using c_char vs u8? The code seems a lot more verbose and less idiomatic. Refcount should not use c_char. |
u8 is incompatible with We can use It's ok for refcount to use c_char. core::ffi::c_char does not depend on libc and c_char is the correct and portable solution, consider the following code: #include <cassert>
int main() {
char a = -2;
char b = 1;
assert(a + b == -1);
return 0;
}With the u8 translation this fails. With the c_char translation, this is ok. |
|
I disagree. I don't think I don't know why the translation of that example fails, but |
Then for unsafe I will continue with c_char so that I can get rid of IsCharPointerFieldFromLibc and IsCharArrayFieldFromLibc. For refcount we keep u8 instead of c_char because we don't expect to interact with libc.
On a platform where char is signed, in fn main_0() -> i32 {
let a: Value<u8> = Rc::new(RefCell::new((-2_i32 as u8)));
let b: Value<u8> = Rc::new(RefCell::new(1_u8));
assert!(((((*a.borrow()) as i32) + ((*b.borrow()) as i32)) == -1_i32));
return 0;
} |
Translates char as c_char instead of u8 in both unsafe and refcount. It uses core:ffi::c_char instead of libc::c_char so that refcount is not dependent on libc.
This contains:
c""syntax to define c_char compatible string literals0 as c_char(correct) instead of0_c_char(incorrect)I wanted to split each of them in a separate PR, however in isolation they don't work. So I had to pack them together.
9 is a bug that I encountered during the u8 -> c_char change. brunsli does
reinterpret_cast<uint8_t>(string.data()).string.data()returns a char (c_char) and uint8_t is translated as u8. This means that the following fast-path inPtr::reinterprete_castis skipped and a real reinterpret takes place:https://github.com/Cpp2Rust/cpp2rust/blob/master/libcc2rs/src/rc.rs#L389-L392
Later, brunsli passes the reinterpreted pointer to the unsafe brotli ffi. In the previous version of StrongPtr::deref/PtrKind::with_mut/PtrKind::with we only materialized one u8 from c_char, meaning that the brotli ffi received a single u8 and a size > 1, resulting in OOB. The current code materializes the entire remaining bytes of the source pointer.
This problem only appears because we interact with the unsafe ffi. In safe code, it's fine to materialize only one element because all our Ptr APIs work on element boundary, e.g. write/read. I expect 9 to be a temporary solution until we replace the unsafe brotli ffi with a safe brotli crate.