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

Add DepthLimiter and adapt to the Read/WriteXDR #158

Merged
merged 16 commits into from Jul 14, 2023

Conversation

jayz22
Copy link
Contributor

@jayz22 jayz22 commented Jul 7, 2023

This is the xdrgen side of changed needed for stellar/rs-soroban-env#861
Generated rs-stellar-xdr: stellar/rs-stellar-xdr#281
The corresponding env PR is stellar/rs-soroban-env#904

  • Adds a new trait DepthLimiter and implements it on two new structures DepthLimitedRead and DepthLimitedWrite, which replace the normal R: Read and W: Write in [Read,Write]Xdr.
  • The depth limiter provides a guard that automatically increases and decreases a counter on enter and leave. Thus limiting the number of recursions on [read_ write_]xdr calls.
  • Defines a pub const DEFAULT_MAX_DEPTH_LIMIT: u32 which is used for scenarios where DepthLimited structure is constructed internally ( from_xdr(bytes) and from_xdr_base64(bytes)). I don't quite like the fact that this is an internal constant, I think ideally this should be externally defined, like the other scenarios where the user constructs the DepthLimited struct. This also means the same workflows may have two different limits, depending on which api to call.
    But passing an additional u32 into these apis makes the UX hideous, and I've checked there is no extern std::uint32_t (how xdrpp does it) equivalent in Rust, and Rust doesn't like static mutable variables (I experimented with it in one commit but it would require turning off the "no-unsafe" guard so I reverted it). So I just decided using a pub const here is the best compromise (I can think of, would much welcome suggestions) for UX.
  • Added a the _with_depth_limit version of from_xdr to allow user-specified depth limits, and having one version calling another. This is a fair compromise that allows the user deal the the inconsistency mentioned above.

cc @graydon @anupsdf

@jayz22 jayz22 marked this pull request as ready for review July 10, 2023 16:22
@jayz22 jayz22 changed the title [WIP] Add DepthLimiter and adapt to the Read/WriteXDR Add DepthLimiter and adapt to the Read/WriteXDR Jul 10, 2023
Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

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

Generally looking good! A few nits, a few deeper design questions.

spec/output/generator_spec_rust/union.x/MyXDR.rs Outdated Show resolved Hide resolved
spec/output/generator_spec_rust/union.x/MyXDR.rs Outdated Show resolved Hide resolved
spec/output/generator_spec_rust/union.x/MyXDR.rs Outdated Show resolved Hide resolved
spec/output/generator_spec_rust/union.x/MyXDR.rs Outdated Show resolved Hide resolved
spec/output/generator_spec_rust/union.x/MyXDR.rs Outdated Show resolved Hide resolved
spec/output/generator_spec_rust/union.x/MyXDR.rs Outdated Show resolved Hide resolved
@graydon graydon merged commit 3113f99 into stellar:master Jul 14, 2023
3 checks passed
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.

None yet

2 participants