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

Refactor authorization manager to only maintain mutable borrow on minimal amount of fields #938

Merged
merged 3 commits into from
Jul 11, 2023
Merged

Refactor authorization manager to only maintain mutable borrow on minimal amount of fields #938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6041fe6
Refactor authorization manager to only maintain mutable borrow on min…
dmkozh Jul 6, 2023
3e082e2
Add a test for invoker contract auth that is not used.
dmkozh Jul 10, 2023
a3ae270
!fixup typo
dmkozh Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
644 changes: 457 additions & 187 deletions soroban-env-host/src/auth.rs
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 7 additions & 7 deletions soroban-env-host/src/host.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,8 +573,8 @@ impl Host {
) -> Result<AddressObject, HostError> {
let has_deployer = deployer.is_some();
if has_deployer {
self.try_borrow_authorization_manager_mut()?
.push_create_contract_host_fn_frame(args.metered_clone(self.budget_ref())?);
self.try_borrow_authorization_manager()?
.push_create_contract_host_fn_frame(self, args.metered_clone(self.budget_ref())?)?;
}
// Make sure that even in case of operation failure we still pop the
// stack frame.
Expand All @@ -584,7 +584,7 @@ impl Host {
// for them just to make auth work in a single case).
let res = self.create_contract_with_optional_auth(deployer, args);
if has_deployer {
self.try_borrow_authorization_manager_mut()?.pop_frame();
self.try_borrow_authorization_manager()?.pop_frame(self)?;
}
res
}
Expand All @@ -595,7 +595,7 @@ impl Host {
args: CreateContractArgs,
) -> Result<AddressObject, HostError> {
if let Some(deployer_address) = deployer {
self.try_borrow_authorization_manager_mut()?.require_auth(
self.try_borrow_authorization_manager()?.require_auth(
self,
deployer_address,
Default::default(),
Expand Down Expand Up @@ -2881,7 +2881,7 @@ impl VmCallerEnv for Host {
) -> Result<Void, Self::Error> {
let args = self.visit_obj(args, |a: &HostVec| a.to_vec(self.budget_ref()))?;
Ok(self
.try_borrow_authorization_manager_mut()?
.try_borrow_authorization_manager()?
.require_auth(self, address, args)?
.into())
}
Expand Down Expand Up @@ -2910,7 +2910,7 @@ impl VmCallerEnv for Host {
})?;

Ok(self
.try_borrow_authorization_manager_mut()?
.try_borrow_authorization_manager()?
.require_auth(self, address, args)?
.into())
}
Expand All @@ -2921,7 +2921,7 @@ impl VmCallerEnv for Host {
auth_entries: VecObject,
) -> Result<Void, HostError> {
Ok(self
.try_borrow_authorization_manager_mut()?
.try_borrow_authorization_manager()?
.add_invoker_contract_auth(self, auth_entries)?
.into())
}
Expand Down
57 changes: 22 additions & 35 deletions soroban-env-host/src/host/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,11 @@ const RESERVED_CONTRACT_FN_PREFIX: &str = "__";
/// Saves host state (storage and objects) for rolling back a (sub-)transaction
/// on error. A helper type used by [`FrameGuard`].
// Notes on metering: `RollbackPoint` are metered under Frame operations
#[derive(Clone)]
// #[derive(Clone)]
pub(super) struct RollbackPoint {
storage: StorageMap,
events: usize,
auth: Option<AuthorizationManagerSnapshot>,
auth: AuthorizationManagerSnapshot,
}

#[cfg(any(test, feature = "testutils"))]
Expand Down Expand Up @@ -117,15 +117,10 @@ impl Host {
/// operation fails, it can be used to roll the [`Host`] back to the state
/// it had before its associated [`Frame`] was pushed.
pub(super) fn push_frame(&self, frame: Frame) -> Result<RollbackPoint, HostError> {
// This is a bit hacky, as it relies on re-borrow to occur only during
// the account contract invocations. Instead we should probably call it
// in more explicitly different fashion and check if we're calling it
// instead of a borrow check.
let mut auth_snapshot = None;
if let Ok(mut auth_manager) = self.0.authorization_manager.try_borrow_mut() {
auth_manager.push_frame(self, &frame)?;
auth_snapshot = Some(auth_manager.snapshot());
}
let auth_manager = self.try_borrow_authorization_manager()?;
let auth_snapshot = auth_manager.snapshot(self)?;
auth_manager.push_frame(self, &frame)?;

let ctx = Context {
frame,
prng: None,
Expand All @@ -144,47 +139,39 @@ impl Host {
/// and storage map to the state in the provided [`RollbackPoint`].
pub(super) fn pop_frame(&self, orp: Option<RollbackPoint>) -> Result<(), HostError> {
// Instance storage is tied to the frame and only exists in-memory. So
// instead of snapshotting it and rolling it back, we jsut flush the
// instead of snapshotting it and rolling it back, we just flush the
// changes only when rollback is not needed.
if orp.is_none() {
self.flush_instance_storage()?;
}
self.try_borrow_context_mut()?
.pop()
.expect("unmatched host frame push/pop");
// This is a bit hacky, as it relies on re-borrow to occur only doing
// the account contract invocations. Instead we should probably call it
// in more explicitly different fashion and check if we're calling it
// instead of a borrow check.
if let Ok(mut auth_manager) = self.0.authorization_manager.try_borrow_mut() {
auth_manager.pop_frame();
}
self.try_borrow_authorization_manager()?.pop_frame(self)?;

if self.try_borrow_context()?.is_empty() {
// When there are no frames left, emulate authentication for the
// recording auth mode. This is a no-op for the enforcing mode.
self.try_borrow_authorization_manager_mut()?
self.try_borrow_authorization_manager()?
.maybe_emulate_authentication(self)?;
// Empty call stack in tests means that some contract function call
// has been finished and hence the authorization manager can be reset.
// In non-test scenarios, there should be no need to ever reset
// the authorization manager as the host instance shouldn't be
// shared between the contract invocations.
#[cfg(any(test, feature = "testutils"))]
{
*self.try_borrow_previous_authorization_manager_mut()? =
Some(self.try_borrow_authorization_manager()?.clone());
self.try_borrow_authorization_manager_mut()?.reset();
}
}

if let Some(rp) = orp {
self.try_borrow_storage_mut()?.map = rp.storage;
self.try_borrow_events_mut()?.rollback(rp.events)?;
if let Some(auth_rp) = rp.auth {
self.try_borrow_authorization_manager_mut()?
.rollback(auth_rp);
}
self.try_borrow_authorization_manager()?
.rollback(self, rp.auth)?;
}
// Empty call stack in tests means that some contract function call
// has been finished and hence the authorization manager can be reset.
// In non-test scenarios, there should be no need to ever reset
// the authorization manager as the host instance shouldn't be
// shared between the contract invocations.
#[cfg(any(test, feature = "testutils"))]
if self.try_borrow_context()?.is_empty() {
*self.try_borrow_previous_authorization_manager_mut()? =
Some(self.try_borrow_authorization_manager()?.clone());
self.try_borrow_authorization_manager_mut()?.reset();
}
Ok(())
}
Expand Down
Loading