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

Rust: add read_xdr_iter #102

Merged
merged 6 commits into from
Jun 22, 2022
Merged

Rust: add read_xdr_iter #102

merged 6 commits into from
Jun 22, 2022

Conversation

leighmcculloch
Copy link
Member

What

Add a read_xdr_iter to all ReadXdr impls that returns an iterator for reading a stream of values from a Read impl.

Why

In a couple places we need to read a stream of XDR values which is pretty inconvenient to impl in practice. Wrapping it into an iterator should make it much simpler.

Also

This highlights a problem with existing read_xdr impls that they use read_exact which unfortunately obscures partial fill EOFs which could be a security concern in our uses. A TODO has been added to address this later.

@leighmcculloch leighmcculloch enabled auto-merge (squash) June 22, 2022 21:35
@leighmcculloch leighmcculloch marked this pull request as draft June 22, 2022 21:35
auto-merge was automatically disabled June 22, 2022 21:35

Pull request was converted to draft

@leighmcculloch leighmcculloch marked this pull request as ready for review June 22, 2022 22:56
Co-authored-by: Graydon Hoare <graydon@pobox.com>
@leighmcculloch leighmcculloch enabled auto-merge (squash) June 22, 2022 23:12
Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't exactly understand the buffering regime but it seems plausible and if it works I'm happy to have more ways to read stuff!

@leighmcculloch
Copy link
Member Author

leighmcculloch commented Jun 22, 2022

I don't exactly understand the buffering regime but it seems plausible and if it works I'm happy to have more ways to read stuff!

Any reader should probably be buffered anyway, but ideally control over that is in the hands of the user and ideally we aren't creating a BufReader internally. The reason I'm doing it here is it's a quick way to get around the read_exact limitations on EOF errors. When reading a stream in the iterator I need to peak at the next byte to see if it is an EOF error. If it is then we have a clean termination of the stream. If read_exact sees an EOF then that's an unclean termination of the stream. read_exact on its own doesn't let us distinguish these situations for decent reasons.

An alternative to using the BufReader would be to define our own reader that read just the next byte, and then on the next call it got to its read function it'd return that byte if it had it, otherwise pass through to the underlying reader. This might be simpler? Maybe less efficient, not sure.

#[cfg(feature = "std")]
pub struct PeekRead<'r, R: Read> {
    reader: &'r mut R,
    buf: Option<u8>,
}

impl<'r, R: Read> PeekRead<'r, R> {
    pub fn peek(&mut self) -> Result<u8> {
        if let Some(b) = self.buf {
            Ok(b)
        } else {
            let mut buf = [0u8; 1];
            self.reader.read_exact(&mut buf)?;
            self.buf = Some(buf[0]);
            Ok(buf[0])
        }
    }
}

impl<'r, R: Read> Read for PeekRead<'r, R> {
    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        if buf.is_empty() {
            Ok(0)
        } else if let Some(b) = self.buf {
            buf[0] = b;
            self.buf = None;
            Ok(1)
        } else {
            self.reader.read(buf)
        }
    }
}

It's essentially the same thing as BufReader but a single byte instead. This is all internal so we can change this if needed without changing the API.

@leighmcculloch leighmcculloch merged commit 4f3cf56 into stellar:master Jun 22, 2022
@leighmcculloch leighmcculloch deleted the rust-iterator branch June 22, 2022 23:37
@leighmcculloch
Copy link
Member Author

Actually, I probably should circle back and replace it with the PeekRead, or remove use of read_exact... because otherwise people will have double nested BufReader's ugh.

@leighmcculloch
Copy link
Member Author

Will follow up in #103.

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

Successfully merging this pull request may close these issues.

None yet

2 participants