mathias234 / binary_rw Goto Github PK

The read_write_string and read_out_of_range tests both use the temporary file out_of_range (looks like a copy-and-paste error?). Depending on when the tests are run (as they are run in parallel by default), there will be some issues with them stomping on each other.

Test output showing the failures:

test read_write_from_memorystream ... ok
test read_write_test_i8 ... ok
test read_write_test_i16 ... ok
test read_write_test_f32 ... ok
test read_write_test_i32 ... ok
test read_write_string ... FAILED
test read_out_of_range - should panic ... FAILED
test read_write_bytes ... ok
test read_write_test_u32 ... ok
test read_write_test_u16 ... ok
test read_write_test_f64 ... ok
test read_write_test_u8 ... ok
test read_write_test_i64 ... ok
test read_write_test_isize ... ok
test read_write_test_usize ... ok
test read_write_test_u64 ... ok
test seek_test ... ok

I have some fixes that address this, will be putting up a PR shortly.

Hi,

lib.rs line 60, buffer is 8 bytes instead of 4.

Thanks for the crate !

I have a situation where I am using FileStream and I want to check the file length but I don't want to create a new File just to read the meta data. We could:

1. Expose &File in FileStream
2. Expose self.file.metadata() in FileStream
3. Support a len() method on the Stream trait

My preference would be for 3) but I wonder what you think?

I can imagine a situation where there may be streams that don't have a known length so if we do support a len() function maybe it should look like:

fn len(&self) -> Option<usize>;

But we do know the length at the moment for MemoryStream and FileStream so maybe cross that bridge later.

I landed on this crate after evaluating bincode and found a bug in the version 2 beta that prevented me from using it for my use case. Whilst I don't think that bug applies to this crate I think the bincode dependency could be removed and we could use the newer from_be_bytes and to_be_bytes for primitive types.

For example for u8:

• https://doc.rust-lang.org/std/primitive.u8.html#method.from_be_bytes (reader)
• https://doc.rust-lang.org/std/primitive.u8.html#method.to_be_bytes (writer)

We should also support little endian with from_le_bytes and to_le_bytes. We could add an Endian enum and pass it to BinaryWriter::new and BinaryReader::new which would be a breaking change and require a major version bump!

If this interests you let me know and I will look into it some more 👍

I added a test that writes to a memorystream three f32s, seeks back to the beginning, and attempts to overwrite with three new f32s. The first of these overwrites will panic due to the following line in Memorystream::write:

let bytes_out_of_buffer = bytes.len() - (self.buffer.len() - self.position);

In the above case, bytes.len() is 4, self.buffer.len() is 12, and self.position is 0.
So bytes_out_of_buffer will be 4 - (12 - 0) = -8, resulting in the usize overflow.

I made a corresponding test case for Filestream, but it doesn't panic and the test succeeds.

See my memorystream test case here:

#[test]
fn write_to_memorystream_overlapping() {
    let mut stream = Memorystream::new().expect("Error");
    let mut writer = BinaryWriter::new(&mut stream);
    writer.write_f32(1.0).expect("Failed to write f32");
    writer.write_f32(2.0).expect("Failed to write f32");
    writer.write_f32(3.0).expect("Failed to write f32");

    writer.seek_to(0);
    writer.write_f32(4.0).expect("Failed to overwrite f32");
    writer.write_f32(5.0).expect("Failed to overwrite f32");
    writer.write_f32(6.0).expect("Failed to overwrite f32");

    let mut reader = BinaryReader::new(&mut stream);
    let value = reader.read_f32().expect("Failed to read f32");
    assert_eq!(4.0, value);
    let value = reader.read_f32().expect("Failed to read f32");
    assert_eq!(5.0, value);
    let value = reader.read_f32().expect("Failed to read f32");
    assert_eq!(6.0, value);
}

Currently working on a fix, will put up a PR with the fix and both test cases after.

If you change the read_write_from_memorystream test to write & read two different values instead of the same value, this bug can be reproduced. The current test case succeeds because it writes "5" twice, but instead of reading two separate "5"s it's just reading the first "5" twice.

Changing the test case to be this instead (where one value is "3", the other is "5") reproduces the bug:

#[test]
fn read_write_from_memorystream() {
    let valueA = 3.0;
    let valueB = 5.0;
    let mut stream = Memorystream::new().expect("Error");
    let mut writer = BinaryWriter::new(&mut stream);
    writer.write_f32(valueA).expect("Failed to write f32");
    writer.write_f32(valueB).expect("Failed to write f32");

    let mut reader = BinaryReader::new(&mut stream);
    reader.seek_to(0).expect("Failed to seek");
    let value = reader.read_f32().expect("Failed to read f32");
    assert_eq!(valueA, value);
    let value = reader.read_f32().expect("Failed to read f32");
    assert_eq!(valueB, value);
}

with this output:

---- read_write_from_memorystream stdout ----
thread 'read_write_from_memorystream' panicked at 'assertion failed: `(left == right)`
  left: `5.0`,
 right: `3.0`', tests\lib.rs:341:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Discovered this bug while testing my fix to #4. Will put up the PR for this fix first.