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

Enforce DepthLimiter in the Host to avoid stack overflow #904

Merged
merged 7 commits into from
Jul 14, 2023

Conversation

jayz22
Copy link
Contributor

@jayz22 jayz22 commented Jul 3, 2023

What

The env side changes to resolve #861
Based on rs-stellar-xdr changes in stellar/rs-stellar-xdr#281
Xdrgen side changes: stellar/xdrgen#158

This is ready for review. I will rebase and update the rs-stellar-xdr hash once the above PR is merged.

  • Adds the DepthLimiter for EnvBase, which applies to all Envs, host, guest, etc. It's added to the EnvBase to avoid unnecessary noises (avoid E: Env -> E: Env + DepthLimiter everywhere).
  • A DepthGuard type is added as the RAII guard for the depth counter.
  • We check all the recursive paths in the env : Val convertion to/from ScVal, comparison, deep clone. (ser/deser will be covered by rs-stellar-xdr's ReadXdr, WriteXdr, which are already depth-limited).
  • Introduces a new pub const DEFAULT_HOST_DEPTH_LIMIT: u32 = 100 in the host and we can later choose to make this a network config parameter.
  • Also added some tests to demonstrate depth limit working for various code paths.

Why

[TODO: Why this change is being made. Include any context required to understand the why.]

Known limitations

[TODO or N/A]

@jayz22 jayz22 requested review from graydon, sisuresh and a team as code owners July 3, 2023 21:00
@jayz22 jayz22 marked this pull request as draft July 3, 2023 21:00
@graydon
Copy link
Contributor

graydon commented Jul 4, 2023

Generally speaking this looks great, I'm glad the rough idea looks like it holds together!

A couple suggestions:

  • Maybe try calling the associated error-type definition in the trait something else -- say type DepthError; -- so that it doesn't collide with the Error type defined in implementations of EnvBase and so forth. Might cut down on the amount of noise arising from ambiguity on the name Self::Error.
  • I talked to @tomerweller and he seems to think we're going to have an opportunity to do one more cycle of XDR additions (so long as they don't actually affect anyone downstream) before we call it finished, so ... maybe try to define this as a config setting, just in case we ever wanted to make it deeper than 100? (There's a corresponding limit in stellar-core which is not a config setting, which is hard-wired to 1000, but that also covers outer structures such as overlay messages containing transactions and so forth). I guess this isn't critical since 100 is very deep and we could always add it as a new config setting in a future protocol, but it might not hurt to include up front. @MonsieurNicolas do you have a strong feeling?

@MonsieurNicolas
Copy link
Contributor

re: make this a config setting. Right now we have a few of these limits hard coded in the XDR (like number of signers), and it's probably fine for now as it allows clients everywhere to also implement it without connecting to the network and it's just less work.

@jayz22 jayz22 changed the title [WIP] Introduce depth limit in env and apply to recursive functions [WIP] Enforce DepthLimiter on Env to avoid stack overflow Jul 11, 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 looks good, a few questions and nits.

soroban-env-host/src/host.rs Outdated Show resolved Hide resolved
soroban-env-common/src/compare.rs Outdated Show resolved Hide resolved
soroban-env-common/src/depth_guard.rs Outdated Show resolved Hide resolved
soroban-env-common/src/object.rs Outdated Show resolved Hide resolved
soroban-env-common/src/object.rs Outdated Show resolved Hide resolved
soroban-env-host/src/budget.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/num.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/num.rs Outdated Show resolved Hide resolved
soroban-env-host/src/test/depth_limit.rs Show resolved Hide resolved
@jayz22 jayz22 changed the title [WIP] Enforce DepthLimiter on Env to avoid stack overflow Enforce DepthLimiter on Env to avoid stack overflow Jul 14, 2023
@jayz22 jayz22 changed the title Enforce DepthLimiter on Env to avoid stack overflow Enforce DepthLimiter in the Host to avoid stack overflow Jul 14, 2023
@jayz22 jayz22 marked this pull request as ready for review July 14, 2023 18:32
@graydon graydon merged commit 63e8430 into stellar:main Jul 14, 2023
8 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.

Stack overflow instantiating deeply nested ScVecs, etc.
3 participants