Avoid potential panic in `read_many` deserialization

[PhilippGackstatter]

Right now, the ByteReader::read_many function looks like this:

winterfell/utils/core/src/serde/byte_reader.rs

Lines 189 to 199 in 5a57503

	fn read_many<D>(&mut self, num_elements: usize) -> Result<Vec<D>, DeserializationError>
	where
	Self: Sized,
	D: Deserializable,
	{
	let mut result = Vec::with_capacity(num_elements);
	for _ in 0..num_elements {
	let element = D::read_from(self)?;
	result.push(element)
	}
	Ok(result)

If num_elements exceeds the system's available memory capacity, Vec::with_capacity panics. Deserialization input can generally not be trusted, and in theory it would be the responsibility of the caller of read_many to validate num_elements or impose limits. However, that is generally not done and is likely to be forgotten, at least in some places. So if a malicious input sets the value of num_elements that is passed to a number that exceeds the available capacity, the attacker can provoke a panic. It would probably be best if we removed the potential for a panic and errored instead if the capacity is too large, e.g.:

let mut result = Vec::new();
result
    .try_reserve(num_elements)
    .map_err(|err| DeserializationError::InvalidValue(err.to_string()))?;

(Note that Vec::try_with_capacity exists, but is not stable at this time).

Alternatively, I'd prefer adding a new error variant to DeserializationError directly for this case.

[irakliyk]

Alternatively, I'd prefer adding a new error variant to DeserializationError directly for this case.

Yes, I think this is probably the way to go. That is, read_many() tries to reserve the required capacity, and if fails, return a special error - something like DeserializationError::OutOfMemory.

Another thing we could do is to provide an additional deserialization method - something like:

fn read_many_with_limit<D>(
    &mut self,
    num_elements: usize,
    max_elements: usize,
) -> Result<Vec<D>, DeserializationError> 
where 
     Self: Sized, 
     D: Deserializable, 
{
    ...
}

This would give users a more fine-grained control over the expected size of the deserialized vector.

[PhilippGackstatter]

I think this could go nicely with something else I wanted, a ByteReader::read_many_iter function.

There aren't many places, but sometimes we use read_many, which returns a Vec<T> and then turn it into something else like a BTreeMap<KEY_TYPE, T>, e.g.: https://github.com/0xMiden/miden-base/blob/172ad4aa01b735b0b5244708e220979e38e6c757/crates/miden-objects/src/account/delta/vault.rs#L344-L349. This is a bit unfortunate because it allocates a vector and then immediately discards it.

With an iterator function, we could skip this extra allocation and do this instead (different example):

reader
    .read_many_iter::<BaseElement>(4)
    .map(|element| element.map(|felt| (felt.as_int(), felt)))
    .collect::<Result<BTreeMap<u128, BaseElement>, DeserializationError>>()?;

And the max_elements would also be supported by using Iterator::take(max_elements), although to be fair, it would be a silent error if there were more elements in the reader in that case.

This approach though would also avoid the potential panic of Vec::with_capacity, at least in the byte reader, completely. Instead, either it should always be used with take or we also pass a max_elements and error after that number of items have been yielded. That might be the least error-prone approach.

The iterator (without max elements) could look like this:

pub trait ByteReader {
    // ... other methods

    fn read_many_iter<D>(
        &mut self,
        num_elements: usize,
    ) -> impl Iterator<Item = Result<D, DeserializationError>>
    where
        Self: Sized,
        D: Deserializable,
    {
        ReadManyIter {
            reader: self,
            num_elements,
            _item: PhantomData,
        }
    }
}

pub struct ReadManyIter<'reader, R: ByteReader, D: Deserializable> {
    reader: &'reader mut R,
    num_elements: usize,
    _item: PhantomData<D>,
}

impl<'reader, R: ByteReader, D: Deserializable> Iterator for ReadManyIter<'reader, R, D> {
    type Item = Result<D, DeserializationError>;

    fn next(&mut self) -> Option<Self::Item> {
        if self.num_elements > 0 {
            self.num_elements -= 1;
            Some(D::read_from(self.reader))
        } else {
            None
        }
    }
}