A recent change to beta & nightly channels broke the tokio-core build

[carllerche]

I just noticed that tokio-core builds started to fail on Travis when using the beta and nightly channels. We are going to change our code to fix it, but I wanted to report the regression:

The build: https://travis-ci.org/tokio-rs/tokio-core/jobs/211426672

The function:

    fn read_buf<B: BufMut>(&mut self, buf: &mut B) -> Poll<usize, io::Error> {
        if let Async::NotReady = <TcpStream>::poll_read(self) {
            return Err(::would_block())
        }
        let mut bufs: [_; 16] = Default::default();
        unsafe {
            let n = buf.bytes_vec_mut(&mut bufs);
            match self.io.get_ref().read_bufs(&mut bufs[..n]) {
                Ok(n) => {
                    buf.advance_mut(n);
                    Ok(Async::Ready(n))
                }
                Err(e) => {
                    if e.kind() == io::ErrorKind::WouldBlock {
                        self.io.need_write();
                    }
                    Err(e)
                }
            }
        }
    }

The error:

error[E0499]: cannot borrow `*buf` as mutable more than once at a time
   --> src/net/tcp.rs:521:21
    |
518 |             let n = buf.bytes_vec_mut(&mut bufs);
    |                     --- first mutable borrow occurs here
...
521 |                     buf.advance_mut(n);
    |                     ^^^ second mutable borrow occurs here
...
532 |     }
    |     - first borrow ends here

[alexcrichton]

My guess is that this is fallout from #40319 in order to fix the soundness hole exposed by #40288.

This was not detected in the crater run on #40319 because this code wasn't published on crates.io yet (it was just in git).

@rust-lang/compiler can you confirm though? If it's something else we may wish to track this regression.

[nikomatsakis]

@alexcrichton I agree with your diagnosis that this the most likely cause, though it's hard to say for sure (without trying the builds)

[nikomatsakis]

What is the signature of bytes_vec_mut?

[carllerche]

This is the bytes_vec_mut fn:

    unsafe fn bytes_vec_mut<'a>(&'a mut self, dst: &mut [&'a mut IoVec]) -> usize {
        if dst.is_empty() {
            return 0;
        }

        if self.has_remaining_mut() {
            dst[0] = self.bytes_mut().into();
            1
        } else {
            0
        }
    }

From here: https://github.com/carllerche/bytes/blob/master/src/buf/buf_mut.rs#L189

[arielb1]

Looks like #40319. Now let me see whether the regression is correct.

[arielb1]

Looks like an NLL problem:

    fn read_buf<B: BufMut>(&mut self, buf: &mut B) -> Poll<usize, io::Error> {
        if let Async::NotReady = <TcpStream>::poll_read(self) {
            return Err(::would_block())
        }
        let mut bufs: [_; 16] = Default::default();
        unsafe {
            let n = buf.bytes_vec_mut(&mut bufs); // `buf` is borrowed here
            match
                self.io.get_ref().read_bufs(&mut bufs[..n]) // `buf` needs to stay valid until here
            {
                Ok(n) => {
                    buf.advance_mut(n); // ... but because regions are lexical, is also borrowed here
                    Ok(Async::Ready(n))
                }
                Err(e) => {
                    if e.kind() == io::ErrorKind::WouldBlock {
                        self.io.need_write();
                    }
                    Err(e)
                }
            }
        }
    }

If you invalidated buf between the 2 lines, that would have caused trouble.

You should be able to fix it by placing the borrow within its own scope:

    fn read_buf<B: BufMut>(&mut self, buf: &mut B) -> Poll<usize, io::Error> {
        if let Async::NotReady = <TcpStream>::poll_read(self) {
            return Err(::would_block())
        }
        let mut bufs: [_; 16] = Default::default();
        unsafe {
            let result /* : io::Result<usize> - no borrowed content */ = {
                let n = buf.bytes_vec_mut(&mut bufs);
                self.io.get_ref().read_bufs(&mut bufs[..n])
                // `buf` is only borrowed within ^
            };
            match result {
                Ok(n) => {
                    buf.advance_mut(n);
                    Ok(Async::Ready(n))
                }
                Err(e) => {
                    if e.kind() == io::ErrorKind::WouldBlock {
                        self.io.need_write();
                    }
                    Err(e)
                }
            }
        }
    }

[alexcrichton]

Ah yes and to be clear this has been fixed and published in the style @arielb1 mentioned.

If this is classified as an acceptable regression (e.g. just a soundness fix) please feel free to close!

[nikomatsakis]

Looks correct to me, thanks @arielb1 for looking in depth.