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

panics in the host via mistaken comparison and conversion dispatch #579

Closed
4 tasks done
graydon opened this issue Nov 19, 2022 · 2 comments
Closed
4 tasks done

panics in the host via mistaken comparison and conversion dispatch #579

graydon opened this issue Nov 19, 2022 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@graydon
Copy link
Contributor

graydon commented Nov 19, 2022

There's a fairly serious (and old!) bug in the way EnvVal<WeakHost,RawVal>::cmp calls WeakHost::obj_cmp which calls through to impl Env for CheckedEnv and treats it as infallibe. This is causing panics because it's not actually infallibe (specifically: comparing values can exceed budget and thereby return a HostError), and the latter impl escalates HostError to panic (which is appropriate for local-testing mode but inapprorpiate for production hosts).

This is happening because we (really just I) overlooked the fact that all the very-early plumbing (in April) that put in place originally to make im work (EnvVal and WeakHost and such) got made-useless by the slightly-later transition (in May) to infallible host functions and CheckedEnv. It all compiles and runs, but it's essentially incoherent: any error that happens inside an im call to Ord::cmp winds up escalating to a panic.

This is really quite bad. My best guess at a solution here is .. quite invasive and horrible:

  • DONE (Remove EnvVal #608) Get rid of EnvVal and WeakHost entirely (eliminating the Ord impl along the way)
  • DONE (Delete im #585) Get rid of im, and actually we can't really use anyone's containers because they all have this characteristic -- we have to write our own maps and vecs that have fallible lookup routines
  • Carefully audit and/or possibly feature-gate code paths in the production host that might accidentally call through to the impl Env for CheckedEnv -- it should really only be seen/called by contracts running tests. Possibly move it to the SDK to avoid the possibility of calling it in the host's own codebase.
  • DONE (Reform TryFromVal to reduce number of impls. #628) Refactor all the TryFromVal paths to enable systematic reform of errors
  • PENDING (Env error cleanup #633) Remove impl Env for CheckedEnv entirely and make all Env methods fallible with a configurable Error provided by the Env type.
@graydon graydon added the bug Something isn't working label Nov 19, 2022
@graydon graydon self-assigned this Nov 19, 2022
@graydon
Copy link
Contributor Author

graydon commented Nov 23, 2022

Update: we .. actually have another copy of the same bug re-occurring in the conversion paths triggered server-side from the builtin contract. For example in impl<E:Env> TryFrom<E, RawVal> for u64 we make infallible calls to e.obj_to_u64() assuming we're being called in a native-testing contract (non-production, local-to-the-user's-workstation) context, where it's fine to panic; but these calls also happen in the inject/extract paths of the builtin contract (production, in-the-server) context, where it's very much not fine to panic. But these calls compile because theyare served by the impl Env for CheckedEnv, that escalates errors to panic.

We need to get that impl out of soroban-env-common or host or guest, move it explicitly to the user-code SDK (and possibly also have a compile-time check (feature? colliding panic handler? some other hack?) that differentiates "code that is in a context where it's allowed to panic" from "not", and ensure the latter. This is way too dangerous to have kicking around in the host.

cc @leighmcculloch

@graydon graydon changed the title EnvVal<WeakHost,RawVal>::cmp should not be infallible panics in the host via mistaken comparison and conversion dispatch Jan 19, 2023
@graydon
Copy link
Contributor Author

graydon commented Jan 23, 2023

This is now complete. Long live the new infallible-errors boilerplate!

@graydon graydon closed this as completed Jan 23, 2023
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

Successfully merging a pull request may close this issue.

1 participant