Skip to content
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.

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

Support custom recursion limits at runtime #760

Closed

Conversation

seanlinsley
Copy link

@seanlinsley seanlinsley commented Nov 21, 2022

We decode protobuf from both trusted and untrusted sources, so the no-recursion-limit feature wasn't a good fit for our workload.

As a followup to #186, this PR introduces a decode_with_context function that allows users to customize the DecodeContext. I chose a generic approach (instead of decode_with_recursion_limit) in case there are additional decode settings that might be added in the future.

Thoughts on the best place to document this, and whether any additional tests are required?

Todo:

  • update the inline docs on DecodeContext

@seanlinsley
Copy link
Author

CC @nrc @danburkert

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Im not a big fan of adding additional methods to the main trait for prost but I can't currently think of a better idea than potentially add this via the proc macro and have it set at build time.

@seanlinsley
Copy link
Author

A build time option isn't a good fit for our use case. We deal with both trusted and untrusted protobuf sources in the same application so it would be preferable if this option could be set at runtime.

That said: could we find a recursion limit that is high enough to work with both without risking a stack overflow? Probably.

Could you say more about how the procedural macro would work? I haven't written one previously.

@seanlinsley
Copy link
Author

Another option is to add a crate feature that sets the recursion limit to a higher value. For example, 1,000.

@LucioFranco
Copy link
Member

Another option is to add a crate feature that sets the recursion limit to a higher value. For example, 1,000.

Yeah, though I am not sure how to make this work in an additive fashion. Maybe adding a method to the Message trait is the only way which is unfortunate.

@seanlinsley
Copy link
Author

Here's a first pass at moving it to a macro attribute that's configured through prost-build:

master...pganalyze:prost:recursion-limit-macro

prost_build.type_attribute("YourStruct", "#[RecursionLimit(1000)]"); // right now
prost_build.recurstion_limit("YourStruct", 1000); // maybe in the future

Thoughts on that as an alternative API? Note that branch isn't complete yet.

@LucioFranco
Copy link
Member

Oh this approach is really smart!!! I didn't think about embedding a const in the impl and using that!!! I like it! I would maybe have the codegen either use the default impl if the recursion limit param is not supplied. On-top of that we could maybe add a specific builder item to prost build's config such that you don't need to use type_attribute but I think that works for now. I like this approach.

@seanlinsley
Copy link
Author

Unfortunately it looks like an associated const isn't going to work. The best we could do is implement a Message::recursion_limit()

Screenshot 2022-12-15 at 3 30 43 PM

@LucioFranco
Copy link
Member

I think we can close this one in favor of #785

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

Successfully merging this pull request may close these issues.

2 participants