Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency between documentation and code implementation #459

Closed
YichiZhang0613 opened this issue Feb 21, 2024 · 3 comments
Closed

Inconsistency between documentation and code implementation #459

YichiZhang0613 opened this issue Feb 21, 2024 · 3 comments
Assignees

Comments

@YichiZhang0613
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants