Avoid clearing SetLastError when allocating string params
#1665
Conversation
| @@ -18,3 +18,12 @@ pub unsafe fn heap_free(ptr: RawPtr) { | |||
| HeapFree(heap, HEAP_NONE, ptr); | |||
| } | |||
| } | |||
|
|
|||
| pub fn heap_string<T: Copy + Default + Sized>(value: &[T]) -> *const T { | |||
| let buffer = heap_alloc((value.len() + 1) * std::mem::size_of::<T>()).expect("Could not allocate string") as *mut T; | |||
There was a problem hiding this comment.
Nit: Since the T here is uninitialized this should probably be *mut MaybeUninit<T>. It's probably fine to leave for now though 🤷
There was a problem hiding this comment.
I really only want to use this function for u8 and u16. Be nice if Rust had traits for stuff like that. C++ for example has the is_unsigned trait.
There was a problem hiding this comment.
Because the pointer points to uninit memory, the next line (where you make the slice) is UB.
There was a problem hiding this comment.
Feel free to suggest a better implementation or shoot me a PR.
There was a problem hiding this comment.
Pull request opened here: #1667
There was a problem hiding this comment.
Kenny that's an unfortunate way to respond to UB reports. I encourage you to consider them a serious bug even if the person who points it out doesn't provide a solution at the same time. There's several ways to deal with uninit data in Rust, and rylev had already mentioned one of them for you just above.
There was a problem hiding this comment.
I wasn't trying to minimize the issue. I just wasn't sure how to address it. Sorry for the confusion.
Switched to using the low-level
HeapAllocallocator rather thanSysAllocXxxas the former doesn't clear the thread's last error.Also added a test to ensure the behavior is persevered.
Fixes #1664