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

Fix auth entry charge #1297

Closed
jayz22 opened this issue Dec 14, 2023 · 2 comments
Closed

Fix auth entry charge #1297

jayz22 opened this issue Dec 14, 2023 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@jayz22
Copy link
Contributor

jayz22 commented Dec 14, 2023

Vec::<Val>::charge_bulk_init_cpy(auth_entries.len() as u64, host)?;
trackers.reserve(auth_entries.len());

- Vec::<Val>::charge_bulk_init_cpy(auth_entries.len() as u64, host)?;
+ Vec::<InvokerContractAuthorizationTracker>::charge_bulk_init_cpy(auth_entries.len() as u64, host)?;
@jayz22 jayz22 added the bug Something isn't working label Dec 14, 2023
@jayz22
Copy link
Contributor Author

jayz22 commented Jan 31, 2024

Using this to track a couple of other issue surfaced during the same review:

On line 1018 in auth.rs in function snapshot you seem to allocate a vector to store authorization snapshots but then charge for the allocation afterwards. Can you confirm that the charge should happen before the allocation?

and

On lines 694-700 in auth.rs in the function new_enforcing (for AuthorizationManager) I noticed that you allocate a vector of AccountAuthorizationTrackers of size num_entries. However, the vector you allocate is of type Vec<RefCell> so the amount of memory allocated is actually num_entries*(sizeof(RefCell)). The difference in memory allocated is n*(sizeof(RefCell) - sizeof(AccountAuthorizationTracker)). Why not try and account for the difference when charging?

github-merge-queue bot pushed a commit that referenced this issue Feb 1, 2024
### What

Fix a few small but protocol-breaking metering issues discovered by the
security review:
i. auth entry charging the wrong amount
#1297
ii. (not protocol breaking) auth `snapshot()` function charge after
allocation
iii. auth manager not charging `RefCell` wrapping. 

I also did a clean up by wrapping the `with_capacity` method with
metering into a new `MeteredContainer::with_metered_capacity` method,
which makes sure bugs like (i) and (ii) above cannot happen.

Unfortunately these breaks a lot of existing observations that depends
on auth, basically all `mem_bytes` have increased by 8 (size of the
RefCell counter) and `cpu_insns` have increased by 2, which is caused by
fixing (iii).

### Why

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

### Known limitations

[TODO or N/A]
@jayz22
Copy link
Contributor Author

jayz22 commented Feb 1, 2024

resolved by #1340

@jayz22 jayz22 closed this as completed Feb 1, 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

2 participants
@jayz22 and others