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

Disallow unchecked arithmetic #1190

Merged
merged 20 commits into from
Aug 14, 2023
Merged

Disallow unchecked arithmetic #1190

merged 20 commits into from
Aug 14, 2023

Conversation

athei
Copy link
Contributor

@athei athei commented Jul 2, 2023

We are using Rust's overflow-checks instrumentation to make sure that no silent integer overflows happen in contracts.

However, using overflow checks in contracts seems to be problematic when also re-building std at the same time as we do. For example, this error cropped up from time to time: use-ink/ink#364

We were able to build around it but for RISC-V builds the workaround does not work. It seems like we are using those features in an unsupported way.

The solution I am proposing is to build without overflow and reject code using unchecked math during build with cargo contract. I think this is a good thing because it forces users to think about how to handle overflows: panic/saturating/wraparound.

We achieve this by doing the following:

  • Fail gracefully if a contract specifies overflow-checks in their manifest
  • Add an extra mandatory clippy pass which will run with our lint in deny mode.

@athei athei force-pushed the at/disallow-unchecked-arithmetic branch from 5ccc098 to 68c4ac4 Compare July 2, 2023 09:59
@athei athei requested a review from a team as a code owner July 2, 2023 10:23
@SkymanOne
Copy link
Contributor

The concern I have around the issue I mentioned in the review in ink! repo. I have also thought about external crates that devs might use in the contract. How are we handling these cases then?

@athei
Copy link
Contributor Author

athei commented Jul 3, 2023

Clippy does not apply to dependencies unless they are path dependencies.

Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Would be nice to add a test for a contract with unchecked arithmetic

@athei
Copy link
Contributor Author

athei commented Aug 11, 2023

I moved the linting pass before the building pass. I also removed the numbered build steps. It is more or less useless to know how many build steps there are but it added a lot of complexity.

One downside is that build and lint have overlap in the warnings they show. Added a comment regarding that. AFAIK the only way around this is to manually list all warnings to disable (or parse from rustc output).

@SkymanOne
Copy link
Contributor

I don't have a strong opinion on the build steps. But it was somewhat useful at what step the build failed

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -195,35 +198,27 @@ pub struct CheckCommand {
manifest_path: Option<PathBuf>,
#[clap(flatten)]
verbosity: VerbosityFlags,
#[clap(flatten)]
features: Features,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove the features from check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because check now only runs clippy where we just pass --all-features. Of course we could discuss whether this is useful but I argue it is: We just wan't to lint all the code and not only a subset.

@athei athei merged commit 29e5435 into master Aug 14, 2023
10 checks passed
@athei athei deleted the at/disallow-unchecked-arithmetic branch August 14, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants