Skip to content
This repository has been archived by the owner on Aug 15, 2021. It is now read-only.

Allow increasing recursion limit #166

Open
omegablitz opened this issue Nov 30, 2019 · 7 comments
Open

Allow increasing recursion limit #166

omegablitz opened this issue Nov 30, 2019 · 7 comments

Comments

@omegablitz
Copy link

I'm trying to deserialize very large CBOR files, and end up hitting the recursion limit of serde_cbor. Would it be possible to provide a way to increase or disable the recursion limit?

The related discussion in serde_json can be found here: serde-rs/json#334.
The approach they took was to add a Deserializer::disable_recursion_limit function.

@pyfisch pyfisch added this to the v0.11 milestone Dec 1, 2019
@pyfisch
Copy link
Owner

pyfisch commented Dec 1, 2019

The API seems reasonable.

Parse arbitrarily deep JSON structures without any consideration for overflowing the stack.

Should this be an unsafe function as it may cause stack overflows and by extension denial of service or worse?

@omegablitz
Copy link
Author

Marking it as unsafe may be a bit much as it doesn't violate any of Rust's safety guarantees. I like the approach serde_json took to gate this function, which was to require the library to be built with features = ["unbounded_depth"] to make sure the user knows what they're getting into 😃

I'm not morally opposed to marking it unsafe though, if others prefer that.

@omegablitz
Copy link
Author

Another option could be to add a Deserializer::set_max_depth function as the Serde MessagePack library does.

@pyfisch
Copy link
Owner

pyfisch commented Dec 5, 2019

I like the approach serde_json took to gate this function, which was to require the library to be built with features = ["unbounded_depth"] to make sure the user knows what they're getting into smiley

I think this crate already has to many features which cause problems because every new feature doubles the amount of build configurations. Sometimes the crate breaks if you have a specific combination of features and std/no-std enabled.

@pyfisch
Copy link
Owner

pyfisch commented Dec 5, 2019

Another option could be to add a Deserializer::set_max_depth function as the Serde MessagePack library does.

Is this better than a simple opt-out from depth checking? Or asked another way: Are there people who know that their messages are at maximum nested 323 levels deep and want to set serde-cbor to this value?

@omegablitz
Copy link
Author

I think this crate already has to many features which cause problems because every new feature doubles the amount of build configurations. Sometimes the crate breaks if you have a specific combination of features and std/no-std enabled.

Yeah, that's totally fair, I didn't think of this.

Is this better than a simple opt-out from depth checking? Or asked another way: Are there people who know that their messages are at maximum nested 323 levels deep and want to set serde-cbor to this value?

My particular use case is that I have a server deserializing cbor messages generated by untrusted clients. For my specific application, the cbor messages generates are quite large by their nature.

Setting the max recursion limit would be helpful because it would allow me to accept large cbor messages within reason, but still have a cap in the case of user abuse.

So I guess to directly answer the question, I'm not sure if there are people who know exactly what depth their messages are, but there may be people who simply want to change the default for their use case.

@pyfisch pyfisch removed this from the v0.11 milestone Jan 12, 2020
@pyfisch
Copy link
Owner

pyfisch commented Jan 12, 2020

I'd merge a PR if you send one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants