Skip to content

cbor: don't allow infinite nesting by default #373

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.

Sign up for GitHub

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

Merged
kaczmarczyck merged 1 commit into from
Sep 9, 2021
Merged

cbor: don't allow infinite nesting by default #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions libraries/cbor/src/reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ pub enum DecoderError {
}

/// Deserialize CBOR binary data to produce a single [`Value`], expecting that there is no additional data.
/// Supports arbitrarily nested CBOR (so the [`DecoderError::TooMuchNesting`] error is never emitted).
/// Maximum level of nesting supported is 127; more deeply nested structures will fail with
/// [`DecoderError::TooMuchNesting`].
pub fn read(encoded_cbor: &[u8]) -> Result<Value, DecoderError> {
read_nested(encoded_cbor, None)
read_nested(encoded_cbor, Some(i8::MAX))
Copy link
Copy Markdown
Member

@ia0 ia0 Aug 17, 2021

Choose a reason for hiding this comment

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

Actually, now I'm wondering why we even need to support infinite nesting. Shouldn't we just have read_nested(i8)? When is infinite nesting useful? We could keep read() for convenience but I'm not even sure it's a good decision. It's better to see explicitly at call site the nesting. Users could wrap that value in a convenience read() function used everywhere in their project.)

Copy link
Copy Markdown
Contributor Author

@daviddrysdale daviddrysdale Aug 24, 2021

Choose a reason for hiding this comment

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

Yeah, IIRC the read() / write() entrypoints were for convenience, so that library users mostly don't have to think about it (after all, 127 levels of nesting ought to be enough for anyone?).

Copy link
Copy Markdown
Collaborator

@kaczmarczyck kaczmarczyck Aug 30, 2021

Choose a reason for hiding this comment

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

Intention was to keep the simple interface from before. Means we have to decide between the current state of this PR (default 127, None possible) and have users always pass in the maximum nesting?

I start to see the appeal of the simpler interface that @ia0 is proposing. It does require most users to write a wrapper function for their preferred nesting level, but that is already true for everyone who doesn't like the 127 default. Also, if we don't think anyone should ever call with None, removing that Option makes sense too. And we don't have to provide the default 127 anymore.

So this PR good to merge, but feel free to change to the simple API too if you'd like.

}

/// Deserialize CBOR binary data to produce a single [`Value`], expecting that there is no additional data. If
Expand Down
5 changes: 3 additions & 2 deletions libraries/cbor/src/writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ pub enum EncoderError {
}

/// Convert a [`Value`] to serialized CBOR data, consuming it along the way and appending to the provided vector.
/// Supports arbitrarily nested CBOR (so the [`EncoderError::TooMuchNesting`] error is never emitted).
/// Maximum level of nesting supported is 127; more deeply nested structures will fail with
/// [`EncoderError::TooMuchNesting`].
pub fn write(value: Value, encoded_cbor: &mut Vec<u8>) -> Result<(), EncoderError> {
write_nested(value, encoded_cbor, None)
write_nested(value, encoded_cbor, Some(i8::MAX))
}

/// Convert a [`Value`] to serialized CBOR data, consuming it along the way and appending to the provided vector. If
Expand Down