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

RUST-713 Fix underflow on BSON array and binary deserialization #244

Merged
merged 3 commits into from
Mar 24, 2021

Conversation

gperinazzo
Copy link
Contributor

Closes #243

@Alexander-L-G
Copy link

Tracked in https://jira.mongodb.org/browse/RUST-713

@Alexander-L-G Alexander-L-G added the tracked-in-jira Ticket filed in Mongo's Jira system label Mar 22, 2021
@patrickfreed patrickfreed changed the title Fix underflow on array deserialization RUST-713 Fix underflow on BSON array and binary deserialization Mar 22, 2021
Copy link
Contributor

@patrickfreed patrickfreed left a 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?

@gperinazzo
Copy link
Contributor Author

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.

Copy link
Contributor

@patrickfreed patrickfreed left a 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.

src/tests/modules/serializer_deserializer.rs Outdated Show resolved Hide resolved
@gperinazzo
Copy link
Contributor Author

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 expect_err.

@patrickfreed
Copy link
Contributor

Code changes LGTM! I authorized the CI run and in the meantime am tagging in the other members of the team as reviewers.

Copy link
Contributor

@isabelatkinson isabelatkinson left a 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

Copy link
Contributor

@patrickfreed patrickfreed left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracked-in-jira Ticket filed in Mongo's Jira system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OOM on huge allocation on malformed input (found through fuzzing)
5 participants