Add a recursion limit for decoding

[nrc]

Rust-protobuf and protobuf implementations in other languages support a recursion limit for decoding. Prost decoding is also recursive, and I believe supporting a recursion limit is necessary if messages can be defined recursively (as they can be for TiKV). Could we add this limit to Prost? I can implement this if it would be accepted, but I'm not sure where to put the option - currently Prost does not support any options for decoding/encoding (as far as I can tell). Rust-protobuf adds the recursion limit to the buffer, but because Prost uses a trait for the buffer that seems not the best solution (I think it would work, we would have a recursion-limited buffer which wrapped any implementation of buffer like a smart pointer, but it would be pretty hacky and I think might be inefficient).

The Rust-protobuf solution was added in stepancheg/rust-protobuf#248

[nrc]

Ping @danburkert do you have thoughts on how/if to implement this?

[Hoverbear]

Do you have any examples from other libraries Nick? Maybe their implementations can help inform the discussion.

[danburkert]

I'm +1 on the idea, but you're right it needs a bit of design. I'm curious if and how serde solves this issue.

It'd take a bit of work, but in theory a recursion counter could be threaded through the deserialization stack. I'd start by adding a usize param to {message, group}::merge in encoding.rs.

I don't think serialization/encoding needs a similar limit, since the recursion is naturally limited by how nested the message is.

The biggest question is if/how to make this configurable. I'd rather not create new variants of Message::{decode, decode_length_delimited, merge, merge_length_delimited} with an extra param, but I'm not sure how else to do it.

spitballing: what if we just capped it at something pretty high like 100, then added cargo features which allowed bumping it up?

(I'd also like to preempt one potential path - I wouldn't want to introduce TLS usage for this).

[dtolnay]

I'm curious if and how serde solves this issue.

For serde_json we have a limit of 128 levels on deserialization which is not configurable but can be disabled via serde_json::Deserializer::disable_recursion_limit. Once disabled, the user might want to use a Deserializer adapter like serde_stacker to grow the stack dynamically, or some other Deserializer adapter to introduce their own recursion limit.

[danburkert]

@dtolnay thanks for that background, that's really helpful. Do you have any thoughts on how common it is to remove the recursion limit? When using protobuf in other languages I've never needed to increase the limit.

Maybe it's sufficient to cap at ~128, and then have a cargo feature to remove the cap. prost doesn't have the equivalent of a Deserializer (but perhaps it should).

[dtolnay]

Not common. Here were the request for it: serde-rs/json#334.

[nrc]

Maybe it's sufficient to cap at ~128, and then have a cargo feature to remove the cap.

This sounds like a good idea to me. I couldn't find anywhere to keep a configurable value (rust-protobuf keeps it in their buffer, but Prost doesn't have its own buffer type, it uses the bytes crate).

@danburkert are you happy for me to implement this?

[danburkert]

@danburkert are you happy for me to implement this?

Yep, sounds good, thanks!