Silent downcast overflow

[crepererum]

When using VarintReader with types smaller than 64bits, the current implementation first reads a 64bit integer:

integer-encoding-rs/src/reader.rs

Lines 40 to 58 in 14cd007

	impl VarIntProcessor {
	fn push(&mut self, b: u8) -> Result<()> {
	if self.i >= 10 {
	return Err(io::Error::new(
	io::ErrorKind::InvalidData,
	"Unterminated varint",
	));
	}
	self.buf[self.i] = b;
	self.i += 1;
	Ok(())
	}
	fn finished(&self) -> bool {
	self.i > 0 && (self.buf[self.i - 1] & MSB == 0)
	}
	fn decode<VI: VarInt>(&self) -> Option<VI> {
	Some(VI::decode_var(&self.buf[0..self.i])?.0)
	}
	}

and then casts it to a smaller integer:

integer-encoding-rs/src/varint.rs

Lines 74 to 77 in 14cd007

	fn decode_var(src: &[u8]) -> Option<(Self, usize)> {
	let (n, s) = u64::decode_var(src)?;
	Some((n as Self, s))
	}

integer-encoding-rs/src/varint.rs

Lines 90 to 93 in 14cd007

	fn decode_var(src: &[u8]) -> Option<(Self, usize)> {
	let (n, s) = i64::decode_var(src)?;
	Some((n as Self, s))
	}

Now imagine a data stream w/ 9 bits 0xff followed by 0x00, which would be a pretty large 64bit integer. If you use VarIntReader::read_varint::<i32>(...) to decode that, it will sucessfully read the 64bit varint, consume all the bytes and during the conversion to 32bit will silently truncate the result. What it should do instead is to read only the number of bytes that are at max required for 32bit (5?) and then fail with "Unterminated varint".

[dermesser]

Yes this is an unsatisfactory state. However, with an unterminated varint error, there would be a fragment being left in the input stream which will give a nonsense number on next read (at least as nonsensical as a truncated int).

Now one could write documentation that the next integer after such an error must be ignored... but that also doesn't seem optimal to me.

In any case, I'm open for debate on this topic.

[crepererum]

I think if your read / deserialize data from a stream and get an error, you have to stop reading or re-synchronize the stream (the latter one is mostly only possible for low-level network protocols). I think a user cannot expect a parser to automatically recover from broken data, because there is no way to know which part was broken (in the example case above was it one byte that was broken or was a 32bit value serialized as 64bit? or did we mess up before reading the varint?). It's not only about the "next integer" btw. because likely the protocol the user is trying to deserialize consists of other data types before and after the varint in question.

With your argument, I think you should NEVER return an "unterminated varint error" because you could always argue that there could in theory be a 128bit, 256bit, ... wide varint.

[dermesser]

That's a good point, I will look into it.

[dermesser]

@crepererum would you mind reviewing the change in #22?

[dermesser]

I believe that this issue can be closed with the fix implemented.