Minimal unsafe here.

There are 3 areas where it’s used. The first one is:

fn poll(&mut self) -> Poll<Option<Self::Item>, Self::Error> {
        if self.buf.remaining_mut() == 0 {
            self.buf.reserve(INIT_BUFFER_SIZE);
        }

        // The buffer contains uninitialised memory so getting a readable slice is unsafe.
        // We trust the `flate2` and `miniz` writer not to read from the memory given.
        //
        // To be safe, this memory could be zeroed before passing to `flate2`.
        // Otherwise we might need to deal with the case where `flate2` panics.
        let read = try_io!(self.inner.read(unsafe { self.buf.bytes_mut() }));

        if read == 0 {
            // If GzDecoder reports EOF, it doesn't necessarily mean the
            // underlying stream reached EOF (such as the `0\r\n\r\n`
            // header meaning a chunked transfer has completed). If it
            // isn't polled till EOF, the connection may not be able
            // to be re-used.
            //
            // See https://github.com/seanmonstar/reqwest/issues/508.
            let inner_read = try_io!(self.inner.get_mut().read(&mut [0]));
            if inner_read == 0 {
                Ok(Async::Ready(None))
            } else {
                Err(error::from(io::Error::new(
                    io::ErrorKind::InvalidData,
                    "unexpected data after gzip decoder signaled end-of-file",
                )))
            }
        } else {
            unsafe { self.buf.advance_mut(read) };
            let chunk = Chunk::from_chunk(self.buf.split_to(read).freeze());

            Ok(Async::Ready(Some(chunk)))
        }
    }

As you can see, uninitialised memory is being passed to the reader which is in general quite a bad idea, but should be harmless in the case that the reader type is known not to look at the memory. The second area that uses unsafe does a similar thing, except it’s slightly worse because it’s doing it with a generic reader.

The solution would be to zero out the memory before passing to the reader. However, this needs to be benchmarked because it’s quite possible that it would have a significant impact. The impact could be mitigated somewhat by using something which lowers to alloc_zeroed inside bytes, but there is no abstraction for this yet (See tokio-rs/bytes#270).

This seems like a pretty common issue in the ecosystem as well so it would be good to think of longer term solutions, e.g some kind of Read shim which accepts a kind of mutable slice with variable length so that uninitialised memory is never exposed to safe code.

The third usage of unsafe is here:

    fn poll(&mut self) -> Poll<Self::Item, Self::Error> {
        let bytes = try_ready!(self.concat.poll());
        // a block because of borrow checker
        {
            let (text, _, _) = self.encoding.decode(&bytes);
            match text {
                Cow::Owned(s) => return Ok(Async::Ready(s)),
                _ => (),
            }
        }
        unsafe {
            // decoding returned Cow::Borrowed, meaning these bytes
            // are already valid utf8
            Ok(Async::Ready(String::from_utf8_unchecked(bytes.to_vec())))
        }
    }

This is a tricky one. Cow::into_owned is suitable here but potentially regresses performance. The current unsafe version avoids a copy (of a possibly very large buffer) when the Cow is Borrowed. Don’t see an easy solution - any ideas?

Audit reqwest #5

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions