Skip to content

Read::read_buf mutable reference to ReadBuf #94742

Closed

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    T-libs-apiRelevant to the library API team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions