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 some primitive castings #289

Closed
wants to merge 1 commit into from
Closed

Audit some primitive castings #289

wants to merge 1 commit into from

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Sep 5, 2021

  • Forbids native runtimes to enforce WASM execution,
  • Rust copied from the C standard that 2 bytes is the minimum supported size_t, so some u32 and usize types were converted to u16 where possible to enable Into::<usize>::into(123_u16).
  • -Dclippy::indexing_slicing was added to avoid future additions of some_vector_or_slice[SOME_INDEX_OUT_OF_BOUNDS] panics.
  • Makes more use of MaxRuntimeUsize to enhance robustness and avoid the common some_len as u32 or some_primitive as usize casts.

@c410-f3r c410-f3r added the s:review-needed The pull request requires reviews label Sep 6, 2021
@sea212
Copy link
Member

sea212 commented Sep 7, 2021

Thanks for auditing and optimizing our code Caio.

I agree with most of the changes, but why disable the native runtime? I understand that it's a solution to avoid obtaining different results within the runtime for different usize values, but there must be a more elegant and less restrictive solution to that.
For example, we could forbid native runtime execution on any machine that has a lesser word size than the WASM runtime (in this case less than 32 bit - I doubt any machine with 16 or 8 bit word size will ever execute nor be able to run our node properly) and allow native runtime execution on any other machine. In the latter case, we can handle the situations in the relevant places accordingly.
I think the only place where I was forced to use usize was when fetching the len of some kind of collection like a Vec or any other dynamically sized type within the heap, in other words when the value was related to the memory size and consequently the limit of memory the corresponding type can address. In any other scenario, usize usage should be avoided within Substrate (as stated by pepyakin/apopiak).
Assuming we only use usize for collections or dynamically sized types, the usize "type size counters" will never overflow, because they'll run into an memory allocation error before they do so. In any other case were we do not have control over this (e.g. external libraries) we should be able to adjust the values appropriately, for example by using the max usize value of the WASM target.

I would also appreciate if you can help me understand, why a new tuple-struct MaxRuntimeUsize(u32) and some essential trait implementations for it are necessary instead of defining a type alias type MaxRuntimeUsize = u32.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 7, 2021

MaxRuntimeUsize(u32) is used to unify the usage of variable length pointer instead of having lots of castings across the code-base along side enhanced robustness and security. Due to its custom-made structure, it is possible to avoid errors regarding orphan rules or coherence imposed by the language, i.e., more control and flexibility.

On 24 May 2021, the Polkadot network stopped partially because of native runtimes (https://polkadot.network/a-polkadot-postmortem-24-05-2021), Parity will eventually remove native runtime and smoldot was created to explore this idea of WASM-only runtimes.

This PR was a best effort to improve overall reliability but I will close it to prevent further changes

@c410-f3r c410-f3r closed this Sep 7, 2021
@sea212
Copy link
Member

sea212 commented Sep 7, 2021

Thanks for the explanation and the reference to the article. Was a very interesting read.

I am quite confused about the closing of this PR. I think you made a good effort to improve our code quality. Maybe we can relax the native runtime constraint for now but include all the other changes?

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 8, 2021

Really? Well, if its is OK, then... OK 😄

Thanks @sea212 !

@c410-f3r c410-f3r deleted the caio-stuff-cast branch September 9, 2021 12:42
@c410-f3r c410-f3r mentioned this pull request Sep 10, 2021
@sea212 sea212 added s:abandoned This pull request is abandoned and removed s:review-needed The pull request requires reviews labels Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:abandoned This pull request is abandoned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants