mongodb / bson-rust

Encoding and decoding support for BSON in Rust
MIT License
389 stars 130 forks source link

Inconsistency between documentation and code implementation #459

Closed YichiZhang0613 closed 4 months ago

YichiZhang0613 commented 4 months ago

I noticed possible panics due to inconsistency between documentation and code implementation in bson-rust/src/raw/serde/seeded_visitor.rs The code does not check whether range is out of bounds before using range.

    /// Copies a slice of bytes into the given range. This method will panic if the range is out of
    /// bounds.
    fn copy_from_slice(&mut self, range: Range<usize>, slice: &[u8]) {
        let buffer = self.get_owned_buffer();
        buffer[range].copy_from_slice(slice);
    }

    /// Removes the bytes in the given range from the buffer. This method will panic if the range is
    /// out of bounds.
    fn drain(&mut self, range: Range<usize>) {
        let buffer = self.get_owned_buffer();
        buffer.drain(range);
    }
isabelatkinson commented 4 months ago

Hi @YichiZhang0613, thanks for opening this issue! Can you clarify what the inconsistency is between the documentation and implementation? The docs indicate that an out-of-bounds range will cause a panic. These methods were written for a specific internal use case and all of the places from which they're called should be using safe ranges. Have you encountered a panic from this code?

YichiZhang0613 commented 4 months ago

Hi @YichiZhang0613, thanks for opening this issue! Can you clarify what the inconsistency is between the documentation and implementation? The docs indicate that an out-of-bounds range will cause a panic. These methods were written for a specific internal use case and all of the places from which they're called should be using safe ranges. Have you encountered a panic from this code?

I read the code and found that the document pointed out that panic will occur when the range is out of bounds. In order to avoid unnecessary panic, you should check whether the array is out of bounds(using get()) in the code before using it instead of calling buffer[range] directly.

isabelatkinson commented 4 months ago

Hey @YichiZhang0613, I discussed this with the team and we are not concerned about panicking in this situation. The bounds used are safe and we prefer the convenience of not needing to handle an error that should never occur. I'm going to close this out, but please let us know if you have any further questions or feedback.