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.

Sign up for GitHub

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

Jump to bottom

Forbid contract reentry #513

Merged
merged 5 commits into from
Oct 4, 2022
Merged

Forbid contract reentry #513

merged 5 commits into from
Oct 4, 2022

Conversation

jayz22
Copy link
Contributor

@jayz22 jayz22 commented Oct 3, 2022 •
edited
Loading

What

Partially resolves #336.

Add a reentry: RawVal argument to the try_call function. If set, it checks the host context for frames with the same contract id.
Disallow contract reentry by checking the host context for frames with the same contract id.
In a later follow up, we will add a reentry: RawVal flag to the try_call function and use that.

Why

  • Why there is no checking function name? We assume if reentry is disabled, the same contract cannot be entered at all, even if the entry point is a different function than what had been called.
  • (In the future) Why only add it in try_call? The call host function is the recommended (safe) path of invocation, where any failure will results in an host error. The try_call is the more loose version that allows failure to be returned as a status. It makes sense to only enable re-entry in the try_call invocation path since that is a further loosening feature (@graydon has suggested it).

Known limitations

[TODO or N/A]

@jayz22 jayz22 changed the title Reentry Add Reentry flag to contract invocation Oct 3, 2022
@jayz22 jayz22 requested a review from leighmcculloch October 3, 2022 15:51
@jayz22
Copy link
Contributor Author

jayz22 commented Oct 3, 2022

I haven't bumped the env version because I haven't figured out how to make the sdk changes (proc-macro) in order to regen the wasm tests.

@jayz22 jayz22 changed the title Add Reentry flag to contract invocation Forbid contract reentry Oct 3, 2022
@jayz22 jayz22 marked this pull request as ready for review October 3, 2022 19:37
@jayz22 jayz22 requested review from graydon, jonjove, sisuresh and a team as code owners October 3, 2022 19:37
leighmcculloch
leighmcculloch approved these changes Oct 3, 2022
View reviewed changes
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'm a little confused by the comment that has been added, since it is incorrect in the context of testing.

soroban-env-host/src/host.rs Show resolved Hide resolved
@leighmcculloch
Copy link
Member

It makes sense to only enable re-entry in the try_call invocation path since that is a further loosening feature (@graydon has suggested it).

I don't think it makes sense to couple reentry as a capability with error handling, as there are disconnected reasons to choose one and the other.

In the SDK I hope to add support for "invocation options" on functions that call both call and try_call, and it will muddy the SDK API if these options are inconsistent between the two functions.

At the moment these options will only include the reentry flag, but I imagine them including other options in the future.

cc @graydon

graydon
graydon approved these changes Oct 4, 2022
View reviewed changes
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.

LGTM! Thanks!

@graydon graydon merged commit 3f646bc into stellar:main Oct 4, 2022
@jayz22
Copy link
Contributor Author

jayz22 commented Oct 4, 2022

I don't think it makes sense to couple reentry as a capability with error handling, as there are disconnected reasons to choose one and the other.

Got it. This was not meant to be set-in-stone, rather it was just one of the options. The main reason it was proposed was to differentiate an "straightforward and safe" route (call) that are intended to be used majority of times versus a more nuanced route with various options (try_call).

@leighmcculloch
Copy link
Member

leighmcculloch commented Oct 4, 2022 •
edited
Loading

@jayz22 I think we should add new call and try_call functions that take the additional option. I think we can simply call them call_2 and try_call_2, and each time we add a new option we can add a new set of call and try_call host functions. The old ones can just call through with default values.

@jayz22
Copy link
Contributor Author

jayz22 commented Oct 5, 2022 •
edited
Loading

@jayz22 I think we should add new call and try_call functions that take the additional option. I think we can simply call them call_2 and try_call_2, and each time we add a new option we can add a new set of call and try_call host functions. The old ones can just call through with default values.

I've reopened the issue so that we can continue discuss there.

@jayz22 jayz22 mentioned this pull request Oct 5, 2022
Open
@gangov gangov mentioned this pull request Feb 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leighmcculloch leighmcculloch leighmcculloch approved these changes

@graydon graydon graydon approved these changes

@jonjove jonjove Awaiting requested review from jonjove

@sisuresh sisuresh Awaiting requested review from sisuresh sisuresh is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make invoke/call contract require explicit flag to enable reentry
3 participants
@jayz22 @leighmcculloch @graydon