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

Audit codebase for all unwraps in transaction execution code #944

Closed
Tracked by #2531
cwgoes opened this issue Dec 21, 2022 · 3 comments
Closed
Tracked by #2531

Audit codebase for all unwraps in transaction execution code #944

cwgoes opened this issue Dec 21, 2022 · 3 comments
Assignees
Labels
bug Something isn't working last-minute pre-mainnet Must happen before mainnet.

Comments

@cwgoes
Copy link
Collaborator

cwgoes commented Dec 21, 2022

All unwraps should be replaced with pattern-match-fail, unless we have a specific reason to do otherwise in a certain case (but I'm not sure what that would be?). Pattern-match-fail is always safe and should prevent runtime panics, which in transaction execution when dependent on user input are always a bug (and a DoS vector).

@cwgoes cwgoes added the bug Something isn't working label Dec 21, 2022
@sug0
Copy link
Collaborator

sug0 commented Dec 21, 2022

unwraps/panics in replica code should be fine, except when validating user input. it's been a hot topic of debate in the interop team, whether we should straight up panic, or propagate errors through monadic binding (>>=). ultimately I'm in defense of panics when errors are unrecoverable. user input doesn't fall under this category, because we don't want byzantine clients to DoS Namada.

@tzemanovic
Copy link
Member

Besides the easy-to-stop unwraps, there are plenty other things that can panic - imho it's essential (and beneficial as the code evolves) to write tests covering the tx/vp execution, especially for the stuff that's not sandboxed in wasm

@cwgoes cwgoes added the pre-mainnet Must happen before mainnet. label Feb 5, 2024
@cwgoes cwgoes mentioned this issue Feb 6, 2024
13 tasks
@sug0 sug0 assigned sug0 and murisi Mar 4, 2024
@brentstone brentstone added this to the Phase 1: mainnet genesis milestone Apr 6, 2024
@sug0
Copy link
Collaborator

sug0 commented May 17, 2024

I did a sweep over v0.35.1, and couldn't find any major issues. Here are the notable findings:

  • The tx_ibc_execute host function is using a reference counted object with Rc, unwrapping the inner value. The value is correctly unwrapped when refs=1, but care should be taken not to increment the ref counts again.
  • There was an unused wasm memory exported to the guest, which was addressed in Remove unused WASM memory export #3258.

The only major concern right now is moving away from our forked wasmer, which hasn't been updated in a while; the associated issue is #821. The forked wasmer has two important changes:

  • ucontext_t is redefined on Apple silicon targets, to avoid performing unaligned reads when trap signals are raised.
  • Various other alignment problems were also fixed (see commit 5783b1a5).

Out of these, at least the first one has been upstreamed. I'm not sure whether the second one has, too... Either way, we should attempt to upgrade to the 4.x series of wasmer.

@sug0 sug0 closed this as completed May 17, 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 last-minute pre-mainnet Must happen before mainnet.
Projects
None yet
Development

No branches or pull requests

5 participants