More validation for pointer casts? Use buffer protocol for importing data?

[kylebarron]

I was reading through the code for accessing the numpy data:

zarrs-python/src/lib.rs

Lines 157 to 177 in cca07fc

	fn py_untyped_array_to_array_object<'a>(
	value: &Bound<'a, PyUntypedArray>,
	) -> &'a PyArrayObject {
	let array_object_ptr: *mut PyArrayObject = value.as_array_ptr();
	unsafe {
	// SAFETY: array_object_ptr cannot be null
	&*array_object_ptr
	}
	}
	fn nparray_to_slice<'a>(value: &'a Bound<'_, PyUntypedArray>) -> &'a [u8] {
	let array_object: &PyArrayObject = Self::py_untyped_array_to_array_object(value);
	let array_data = array_object.data.cast::<u8>();
	let array_len = value.len() * Self::pyarray_itemsize(value);
	let slice = unsafe {
	// SAFETY: array_data is a valid pointer to a u8 array of length array_len
	debug_assert!(!array_data.is_null());
	std::slice::from_raw_parts(array_data, array_len)
	};
	slice
	}

I'm not sure I'm well versed enough to assert that it could be unsound, but it looks a little quick to cast the pointer. It seems to expect that the input array is C-order contiguous, but doesn't provide any validation to that effect.

Instead of doing raw unsafe casts through pointers, you could use the safe as_slice method to access the underlying &[u8] of a numpy array.

Alternatively, you could use the buffer protocol to access data. Which is unsafe for different reasons, and requires the Python user to not mutate your buffers, but more flexible for varied inputs. https://alexgaynor.net/2022/oct/23/buffers-on-the-edge/

[LDeakin]

I'm not sure I'm well versed enough to assert that it could be unsound

Neither 🙃. I rely on miri for stuff like this, but we haven't run it over this crate. I don't even know if we can with all the interop.

Instead of doing raw unsafe casts through pointers, you could use the safe as_slice method to access the underlying &[u8] of a numpy array.

Looks like PyReadonlyArray is type-dependent, so it might be a lot of fluff to get there from a PyUntypedArray.

It seems to expect that the input array is C-order contiguous, but doesn't provide any validation to that effect

Indeed! We are validating before calling the function, but validation should be inside it

[kylebarron]

Instead of doing raw unsafe casts through pointers, you could use the safe as_slice method to access the underlying &[u8] of a numpy array.

Looks like PyReadonlyArray is type-dependent, so it might be a lot of fluff to get there from a PyUntypedArray.

It is. I'm not well versed enough in your code to know what kind of array you'd expect there. However if you only ever need a &[u8] here, then one way to fix this is to call array.view("uint8") in Python, and then you'll always be able to extract the output view as a PyReadonlyArray<u8>.

[LDeakin]

@kylebarron have a look at #80. Maybe the numpy crate can give us a safe API later, but I think we are in the clear here. I'm hesitant to switch to a uint8 view because we still haven't scoped out how to handle variable-length data types, like strings.

[flying-sheep]

Could you explain how that can possibly be an issue? Numpy arrays can’t have variable-length items inline, it’s either fixed length or pointers, right?

Otherwise the invariants around shape, stride, and itemsize aren’t upheld, right?

For reference:

• Why is e.g. decode_into unsafe? Why UnsafeCellSlice? LDeakin/zarrs#133
• Safely access data in PyUntypedArray PyO3/rust-numpy#477

[LDeakin]

Yes, but it seems odd to me to go to bytes and back for object pointers

[flying-sheep]

Hmm, the numpy crate knows numpy::PyArray<PyObject, _>, with PyObject = Py<PyAny>, which are owned objects not bound to the GIL, so it should be possible, but that’s not that relevant, right?

We don’t actually try to deal with object arrays/strings (yet), right? We only wrap our &[u8] into ArrayBytes::new_flen(...) and write that to a zarr store. We’ll have to create a new code path for strings, anyway. And that probably involves casting string arrays to numpy::PyArray<PyObject, _>

[LDeakin]

We don’t actually try to deal with object arrays/strings (yet), right?

Yea we deny these data types early. Also I consider the variable length data type support in zarrs to still be experimental because nothing has been standardised in the Zarr V3 specification. zarr-python itself warns about this when you do anything with variable length data types.

We’ll have to create a new code path for strings

Indeed, in the end we need to arrive at the zarrs in-memory representation for variable-length data
https://github.com/LDeakin/zarrs/blob/2f5a66520445f482695db6b5b51c2f83333f7dd1/zarrs/src/array/array_bytes.rs#L35-L40

This currently matches the on-disk representation for a proposed new vlen codec, as the vlen-bytes / vlen-utf8 codecs that have been used in the past for Zarr V2 data (and now for Zarr V3 as well with zarr-python) are suboptimal.