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

GODRIVER-1923 Error if BSON cstrings contain null bytes #622

Merged
merged 5 commits into from
Mar 29, 2021

Conversation

divjotarora
Copy link
Contributor

Per bsonspec.org, cstrings are only used for BSON key names and for the pattern/options fields in a BSON regex value so this PR applies validation to those types. This validation does not apply to regular BSON strings, which can have null bytes because they're prefixed with a 4-byte length. It also does not apply to extended JSON because the JSON specification allows null bytes and other unicode code points as long as they are properly escaped (e.g. in JSON, this would look like {"foo\u0000": "bar"}.

@divjotarora divjotarora force-pushed the godriver1923-bson-cstrings branch from 93e724c to 78b349d Compare March 25, 2021 00:57
@divjotarora divjotarora changed the title Error if BSON cstrings contain null bytes GODRIVER-1923 Error if BSON cstrings contain null bytes Mar 25, 2021
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code change LGTM! I just have two questions.

bson/marshal_test.go Show resolved Hide resolved
bson/bsonrw/value_writer.go Show resolved Hide resolved
Copy link
Contributor Author

@divjotarora divjotarora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added validation to panic in bsoncore if a raw document is being constructed directly (e.g. using bsoncore.NewDocumentBuilder).

bson/bsonrw/value_writer.go Show resolved Hide resolved
bson/bsonrw/value_writer.go Show resolved Hide resolved
bson/marshal_test.go Show resolved Hide resolved
@divjotarora divjotarora requested review from iwysiu and kevinAlbs March 25, 2021 19:02
Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

x/bsonx/bsoncore/bsoncore_test.go Show resolved Hide resolved
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, just a small question about tests.

bson/bsonrw/value_writer.go Show resolved Hide resolved
x/bsonx/bsoncore/bsoncore_test.go Show resolved Hide resolved
x/bsonx/bsoncore/bsoncore_test.go Outdated Show resolved Hide resolved
@divjotarora divjotarora requested a review from kevinAlbs March 29, 2021 21:25
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@divjotarora divjotarora merged commit 2aca31d into mongodb:master Mar 29, 2021
@divjotarora divjotarora deleted the godriver1923-bson-cstrings branch March 29, 2021 23:38
tolsen pushed a commit to tolsen/mongo-go-driver that referenced this pull request Jun 10, 2021
iwysiu pushed a commit to iwysiu/mongo-go-driver that referenced this pull request Jun 11, 2021
iwysiu added a commit that referenced this pull request Jun 14, 2021
Co-authored-by: Divjot Arora <divjot.arora@10gen.com>
kevinAlbs pushed a commit to kevinAlbs/mongo-go-driver that referenced this pull request Aug 11, 2021
faem pushed a commit to kubedb/mongo-go-driver that referenced this pull request Mar 17, 2022
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

Successfully merging this pull request may close these issues.

4 participants