Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shifting the lifetime semantics of io::Read #34

Closed
mcy opened this issue Jul 23, 2020 · 2 comments
Closed

Shifting the lifetime semantics of io::Read #34

mcy opened this issue Jul 23, 2020 · 2 comments
Assignees

Comments

@mcy
Copy link
Contributor

mcy commented Jul 23, 2020

Currently, manticore::io::Read<'a> behaves like a "grant me parts of your buffer":

trait Read<'a> {
  // Reads `len` bytes; always blocks.
  fn read_bytes(&mut self, len: usize) -> Result<&'a [u8]>;
}

This is kind of a weird interface, but it's informed by the design of FromWire:

trait FromWire<'a> {
  fn from_wire<R: Read<'a>>(r: R) -> Result<Self>;
}

This allows FromWire implementations to bind any lifetimes in their definition to the Read<'a> the data comes from.


In practice, this is a problem: it means that Read<'a> needs to allocate memory to buffer its entire message, which is undesirable. This also makes Read<'a> implementations, often the purview of an integration, needlessly complicated.

An alternative solution to the above FromWire signature is the following:

trait FromWire<'a> {
  fn from_wire<R: Read, A: Arena>(r: R, arena: &'a mut A) -> Result<Self>;
}

The new trait Arena can be imagined as a thin wrapper over split_at_mut:

trait Arena {
  fn alloc(&mut self, bytes: usize) -> Result<&mut [u8]>;
}
impl Arena for &mut [u8] { ... }

Then, we would be able to change Read to look a lot more like std's version:

trait Read {
  // Reads `out.len()` bytes; always blocks.
  fn read_bytes(&mut self, out: &mut [u8]) -> Result<()>;
}

The main upshot from this change is that an implementation of Read doesn't need to buffer entire messages, while implementations of FromWire can buffer only what they absolutely have to:

impl<'a> FromWire<'a> for FirmwareVersionResponse<'a> {
  fn from_wire<R: Read, A: Arena>(r: R, arena: &'a mut A) -> Result<Self> {
    let version = arena.alloc(32)?;
    r.read_bytes(&mut *version)?;
    Self { version }
  }
}

While this does introduce a copy into FromWire, this is a copy any non-trivial Read implementation was already doing. Read implementations can now simply be think wrappers over syscalls, like they should have been in the first place.

@mcy mcy self-assigned this Jul 23, 2020
@mcy
Copy link
Contributor Author

mcy commented Jul 23, 2020

@osenft @gkelly @lenary I want to get a quick glance on this design before I start refactoring, in case anyone has suggestions for improving this.

@mcy
Copy link
Contributor Author

mcy commented Aug 10, 2020

This has been implemented in #35

@mcy mcy closed this as completed Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant