cbor: don't allow infinite nesting by default#373
cbor: don't allow infinite nesting by default#373kaczmarczyck merged 1 commit intogoogle:developfrom
Conversation
daviddrysdale
force-pushed
the
infinity-is-127
branch
from
65472a3 to
a82c5b1
Compare
daviddrysdale
force-pushed
the
infinity-is-127
branch
from
a82c5b1 to
210240d
Compare
| /// Supports arbitrarily nested CBOR (so the [`DecoderError::TooMuchNesting`] error is never emitted). | ||
| pub fn read(encoded_cbor: &[u8]) -> Result<Value, DecoderError> { | ||
| read_nested(encoded_cbor, None) | ||
| read_nested(encoded_cbor, Some(i8::MAX)) |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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.
daviddrysdale
force-pushed
the
infinity-is-127
branch
from
210240d to
a2b299c
Compare
ia0
left a comment
ia0
left a comment
There was a problem hiding this comment.
I'm still wondering if it wouldn't be better to just remove the foot gun of running with infinite nesting (i.e. read_nested/write_nested take an i8 instead of an Option<i8>). But the PR as it is, is already an improvement by making the default not a foot gun. It should also fix some of the fuzzing errors we have.
Change the read()/write() methods to use a nesting limit of 127
internally, to avoid the possibility of heavily nested inputs exhausting
the stack.
Library users that still want to skip nesting checks can still get at
this functionality by using `{read,write}_nested(..., None)`.
daviddrysdale
force-pushed
the
infinity-is-127
branch
from
a2b299c to
db52294
Compare
ia0
left a comment
ia0
left a comment
There was a problem hiding this comment.
I see how removing the Option touches more lines. I can do it afterwards. This looks good to me as is.
5 participants
Change the read()/write() methods to use a nesting limit of 127
internally, to avoid the possibility of heavily nested inputs exhausting
the stack.
Library users that still want to skip nesting checks can still get at
this functionality by using
{read,write}_nested(..., None).