Overflow in std/io/mem.rs "combine" returns incorrect error with large positions

[MichaelGG]

The combine function at https://github.com/rust-lang/rust/blob/7d8d06f86b48520814596bd5363d2b82bc619774/src/libstd/io/mem.rs , line 25, overflows by casting the parameters "cur" and "end" from uint to i64. Apart from using very large arrays on true 64-bit architectures, this bug can be triggered by MemReader's consume function, which simply increments the position.

use std::io::{MemReader,SeekStyle,Seek,Buffer};
fn main() {
    let mut mr = MemReader::new(vec![]);
    mr.consume((1u64 << 63) as uint);
    println!("{}", mr.seek(0, SeekStyle::SeekCur));
}

When run this produces: Err(invalid seek to a negative offset). This is an incorrect error as the seek does not lead to a negative offset.

I would expect that all uses of combine should check their inputs before to make sure they're within 63-bit addressing range, or combine should correctly handle seeking at large positions.

[andrewyli]

@MichaelGG I'm a little confused. Are you saying the problem is with combine and with consume?

[MichaelGG]

Combine casts uint to i64, causing an overflow. But to get the uints into that range, you need to actually be using something in that file (Mem/Buf Reader/Writer) that's gonna get that high, which is unlikely in most scenarios.

However, you can trigger the bug without using 2^63 bytes by calling MemReader.consume, which will increment it's position freely.

Additionally, with the new overflow semantics, it seems that "uint as i64" may be an undefined value when uint exceeds i64.MAX.

[steveklabnik]

https://github.com/rust-lang/rfcs/blob/94109f02eab0c3767617f24cba3664da4d5b1ffd/text/0517-io-os-reform.md replaces memreader with Cursor:

use std::io::{Cursor,Seek,SeekFrom,BufRead};
fn main() {
        let mut mr = Cursor::new(vec![]);
            mr.consume((1u64 << 63) as usize);
                println!("{:?}", mr.seek(SeekFrom::Current(0)));
}

gives

Err(Error { repr: Custom(Custom { kind: InvalidInput, desc: "invalid seek to a negative position", detail: None }) })

today.

[MichaelGG]

The combine function seems to now be macro_rules! seek: https://github.com/rust-lang/rust/blob/master/src/libstd/io/cursor.rs#L65

It still casts usize to i64 and can overflow, for example by consuming to std::i64::MAX, then seeking Current(1). But even with --cfg ndebug I don't get any errors on i64 overflow yet. I suppose after rust-lang/rfcs#560 lands then this will be slightly more serious since it'd panic?

[steveklabnik]

Triage: no change today.

[Mark-Simulacrum]

I believe this has been fixed in #39874, closing.