Deserialization of array can panic

[ggwpez]

The deserialisation code of a static sized array uses unwrap instead of propagating the error:

algebra/serialize/src/impls.rs

Lines 481 to 483 in 065cd24

	let result = core::array::from_fn(|_| {
	T::deserialize_with_mode(&mut reader, compress, Validate::No).unwrap()
	});

This was possibly done since the try_from_fn function is experimental. However, it can cause a panic instead of an error return.

I would propose to use a vector with capacity. There is probably a way to make it a faster with unsafe, but thats not my speciality:

let mut result = Vec::<T>::with_capacity(N);
for i in 0 .. N {
    result.push(T::deserialize_with_mode(&mut reader, compress, validate)?);
}

let array: [T; N] = result.try_into().map_err(|_| SerializationError::InvalidData)?;
Ok(array)

[burdges]

Yeah, this is a serious bug. nice catch.

There are crates that do this, like arrayvec, but the unsafe solution looks like this:

impl<T: CanonicalDeserialize, const N: usize> CanonicalDeserialize for [T; N] {
    #[inline]
    fn deserialize_with_mode<R: Read>(
        mut reader: R,
        compress: Compress,
        validate: Validate,
    ) -> Result<Self, SerializationError> {
        let mut array = MaybeUninit::uninit_array::<N>();
        for i in 0..N {
            array[i].write(T::deserialize_with_mode(&mut reader, compress, Validate::No) ?);
        }
        if let Validate::Yes = validate {
            T::batch_check(result.iter())?
        }
        Ok(unsafe { MaybeUninit::array_assume_init(array) })
    }
}

What this unsafe is solving is the possibility that no sensible default initilization exists for T.

[Pratyush]

We want to avoid allocation when possible, but the MaybeUninit solution looks reasonable to me. Would folks be interested in opening a PR?

[burdges]

We should use arrayvec instead of my code above because arrayvec avoids possible memory leaks in my code. Also arrayvec already gets used in ff, not really adding a dependency even.

impl<T: CanonicalDeserialize, const N: usize> CanonicalDeserialize for [T; N] {
    #[inline]
    fn deserialize_with_mode<R: Read>(
        mut reader: R,
        compress: Compress,
        validate: Validate,
    ) -> Result<Self, SerializationError> {
        let mut array = ArrayVec::<T, N>::new();
        for i in 0..N {
            array[i].push(T::deserialize_with_mode(&mut reader, compress, Validate::No) ?);
        }
        if let Validate::Yes = validate {
            T::batch_check(result.iter())?
        }
        Ok(array.into_inner())
    }
}