add recursion limit #248
add recursion limit #248
Conversation
protobuf/src/stream.rs
Outdated
| @@ -31,6 +31,9 @@ use buf_read_iter::BufReadIter; | |||
| // `CodedOutputStream` wraps `BufWriter`, it often skips double buffering. | |||
| const OUTPUT_STREAM_BUFFER_SIZE: usize = 8 * 1024; | |||
|
|
|||
| // Default recursion level limit. | |||
| const DEFAULT_RECURSION_LIMIT: isize = 100; | |||
There was a problem hiding this comment.
100 is just copied from C++ implementation. If you think it's too small, please let me know.
There was a problem hiding this comment.
Thanks for the explanation. Could you please paste the explanation into the source comments?
protobuf/src/stream.rs
Outdated
| } | ||
|
|
||
| /// Set the recursion limit. | ||
| pub fn set_recursion_limit(&mut self, limit: isize) { |
There was a problem hiding this comment.
Parameter should be u32 probably
- to avoid misunderstanding (negative value is meaningless)
- should be platform-independent
protobuf/src/stream.rs
Outdated
| @@ -121,24 +124,54 @@ pub mod wire_format { | |||
|
|
|||
| pub struct CodedInputStream<'a> { | |||
| source: BufReadIter<'a>, | |||
| recursion_budget: isize, | |||
There was a problem hiding this comment.
I think it should be i32 to make it platform-independent.
protobuf/src/stream.rs
Outdated
| } | ||
|
|
||
| // Only for internal tracing | ||
| #[doc(hidden)] |
There was a problem hiding this comment.
AFAIU this function can be pub(crate) instead of public+hidden.
protobuf/src/stream.rs
Outdated
| #[inline] | ||
| pub fn incr_recursion(&mut self) -> ProtobufResult<()> { | ||
| self.recursion_budget -= 1; | ||
| if self.recursion_budget < 0 { |
There was a problem hiding this comment.
Looks like there's a bug here: if we hit recursion_limit, we've lost the budget. Should probably be:
if self.recursion_budget <= 0 { return error; }
self.recursion_budget -= 1;
Ok(())
| assert_eq!(test3, t); | ||
|
|
||
| is = CodedInputStream::from_bytes(&bytes); | ||
| is.set_recursion_limit(0); |
There was a problem hiding this comment.
Ideally this should be tested with recursion level = 3 or something like that, so that read with 2 is OK and read with 3 returns error to really check that code didn't mess with signs or something like that.
|
Thank you! |
Provide a way to configure recursion limit so invalid messages or deeply nested messages won't lead to stack overflow, which has no way to recover.