Description
cc #78485
Currently, Read::read_buf
takes a &mut ReadBuf
. That means that a malicious Read
impl could swap out the buffer for another whereas the intent is that an impl should only write into the buffer. If users rely on properties of the ReadBuf
to hold for an underlying buffer, then they must check that the ReadBuf has not been changed, otherwise they risk writing code which is unsound (see ##93305 for an example. Here copy_to should make this check, but does not). Since this check is not obvious, this is a potential footgun.
This was previously discussed on Zulip: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Read.3A.3Abuf_read.20signature
Note that this is not a soundness issue, however, it makes it easier than it ought to be to write unsound code.
Note also that this requires an actively malicious impl of Read to be harmful since the lifetime of the ReadBuf means that the impl must use a static (or leaked) ReadBuf and that is extremely unlikely to happen by accident. IMO, this is important because a malicious impl could cause similar errors in many other ways, such as calling assume_init
on the ReadBuf without in fact initializing the bytes. That does require unsafe
, but IMO is no easier to audit than swapping out the ReadBuf for another (since every legitimate use of ReadBuf requires unsafe code).
Anyway, the way to fix this is to have some kind of type struct ReadBufMut<'a>(&'a mut ReadBuf);
and to use ReadBufMut
rather than &mut ReadBuf
in read_buf
's signature. The interesting question is exactly how to convert between the two types ergonomically and whether we can design things in such a way that we can expose only one type rather than two.