Undefined behaviors in `Drop` implementation for `DartCObject`

[shinmao]

Unsoundness in Drop for DartCObject

We found that a memory safety bugs could be triggered with the drop of DartCObject if a DartArray is constructed with a null pointer and a non-zero length.

allo-isolate/src/ffi.rs

Lines 172 to 180 in 9134004

	impl Drop for DartCObject {
	fn drop(&mut self) {
	match self.ty {
	DartCObjectType::DartString => {
	let _ = unsafe { CString::from_raw(self.value.as_string) };
	},
	DartCObjectType::DartArray => {
	let _ = DartArray::from(unsafe { self.value.as_array });
	},

If the public field of values is assigned to NULL, it will be passed to the unsafe function slice::from_raw_parts_mut,

allo-isolate/src/dart_array.rs

Lines 54 to 60 in 9134004

	fn from(arr: DartNativeArray) -> Self {
	let inner = unsafe {
	let slice =
	slice::from_raw_parts_mut(arr.values, arr.length as usize);
	Box::from_raw(slice)
	};
	Self { inner }

which violates its safety invariants: The pointer passed to the unsafe function is required to be non-null!

PoC

fn main() {
    let corrupted_obj = DartCObject {
        ty: DartCObjectType::DartArray,
        value: DartCObjectValue {
            as_array: DartNativeArray {
                length: 100,
                values: std::ptr::null_mut(),
            },
        },
    };

    drop(corrupted_obj);
}

run with Miri,

error: Undefined Behavior: pointer not dereferenceable: pointer must be dereferenceable for 800 bytes, but got null pointer
  --> /home/usr/.cargo/registry/src/index.crates.io-6f17d22bba15001f/allo-isolate-0.1.27/src/dart_array.rs:57:17
   |
57 |                 slice::from_raw_parts_mut(arr.values, arr.length as usize);
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior

How could this happen in Real-World

allo-isolate/src/into_dart.rs

Lines 168 to 172 in 9134004

	let data = DartNativeTypedData {
	ty: T::dart_typed_data_type(),
	length: 0,
	values: std::ptr::null_mut(),
	};

As shown at line 171, it is intuitive for user to learn the code convention from the crate and pass std::ptr::null_mut() to the Dart types.
Besides, we also find that the path for DartTypedData has implemented guard against null pointer perfectly.

allo-isolate/src/ffi.rs

Lines 181 to 188 in 9134004

	DartCObjectType::DartTypedData => {
	struct MyVisitor<'a>(&'a DartNativeTypedData);
	impl DartTypedDataTypeVisitor for MyVisitor<'_> {
	fn visit<T: DartTypedDataTypeTrait>(&self) {
	if self.0.values.is_null() {
	return;
	}
	let _ = unsafe {

We consider that the possibility of null pointer might be ignored on the path of DartArray. As it is encapsulated as safe function, we recommend to add the same check to the path of drop implementation.