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

[Bug][VM] Access control checks not in order #14941

Open
georgemitenkov opened this issue Oct 11, 2024 · 0 comments
Open

[Bug][VM] Access control checks not in order #14941

georgemitenkov opened this issue Oct 11, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@georgemitenkov
Copy link
Contributor

🐛 Bug

Access control code is not yet enabled, but has multiple problems:

Problem 1

Access control is done after the resource is loaded from the global storage. For example, for borrow_global:

let res = Self::load_resource(loader, data_store, module_store, gas_meter, addr, ty)?.borrow_global();
gas_meter.charge_borrow_global(...)?;
self.check_access(...)?;
...

Ideally, the access check should be done before loading to avoid any overhead and unnecessary data caching. Also gas metering for load_resource accounts for success of the loading, and so on denied access, the gas charge should be the same as if the resource failed to load.

Problem 2

While the feature is not enabled, some code is unconditionally executed and has impact on performance: check_access. It looks up struct name in global struct name cache, which can lead to contention on the read lock when this cache is shared across multiple threads (i.e., V2 loader enabled).

To reproduce

N/A

Expected Behavior

A clear and concise description of what you expected to happen.

System information

N/A

Additional context

N/A

@georgemitenkov georgemitenkov added the bug Something isn't working label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant