-
Notifications
You must be signed in to change notification settings - Fork 134
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
RUST-713 Fix underflow on BSON array and binary deserialization #244
Conversation
Tracked in https://jira.mongodb.org/browse/RUST-713 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @gperinazzo for submitting this patch, and also thank you @5225225 for identifying reproducible crash cases!
The fix looks good to me, but could we add the repros as test cases to ensure we don't accidentally introduce a regression in the future?
Hey @patrickfreed, I added tests for #243 in the same format as the ones for #64. Let me know if you would like any more changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Could we actually drop the issue number from the test name and instead add a link to the Jira ticket (https://jira.mongodb.org/browse/RUST-713) in a docstring for the test instead? I think that way it'll be easier to follow for others seeing it for the first time (or for me in the future 🙂). I think that test name format was used from before MongoDB started officially maintaining this library.
I've replaced the issue in the function name with a link to the Jira ticket as a docstring for the test and added the |
Code changes LGTM! I authorized the CI run and in the meantime am tagging in the other members of the team as reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the lint error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and fixed the lint for you, if that's okay. Once the CI comes back green I'll merge it in, thanks again for your contribution!
Closes #243