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.

Sign up for GitHub

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

Jump to bottom

Remove unchecked arithmetic #1831

Merged
merged 14 commits into from
Aug 28, 2023
Merged

Remove unchecked arithmetic #1831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5ad9d0e
Remove unchecked arithmetic
athei Jul 1, 2023
9c4e59f
Remove from erc20
athei Jul 2, 2023
8ba8725
Remove from erc1155
athei Jul 2, 2023
9627c86
Remove from erc721
athei Jul 2, 2023
41bc133
Remove from incrementer
athei Jul 2, 2023
0b19225
Remove from payment-channel
athei Jul 2, 2023
526fdfd
Remove from trait-erc20
athei Jul 2, 2023
bea89ad
Remove from trait-incrementer
athei Jul 2, 2023
24da566
Remove from multi-contract-caller
athei Jul 2, 2023
ab9e040
Remove from multisig
athei Jul 2, 2023
b703b49
Remove from set-code-hash
athei Jul 2, 2023
c8e9875
cargo fmt
athei Jul 2, 2023
8675b2e
Merge branch 'master' into at/remove-unchecked-arithmetic
athei Aug 21, 2023
28175ea
Spellcheck
athei Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/env/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ derive_more = { workspace = true, features = ["from", "display"] }
num-traits = { workspace = true, features = ["i128"] }
cfg-if = { workspace = true }
paste = { workspace = true }
arrayref = { workspace = true }
static_assertions = { workspace = true }
const_env = { workspace = true }

Expand Down
8 changes: 6 additions & 2 deletions crates/env/src/call/execution_input.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,9 @@ where
{
#[inline]
fn size_hint(&self) -> usize {
scale::Encode::size_hint(&self.head) + scale::Encode::size_hint(&self.rest)
scale::Encode::size_hint(&self.head)
.checked_add(scale::Encode::size_hint(&self.rest))
.unwrap()
}

#[inline]
Expand All @@ -204,7 +206,9 @@ where
{
#[inline]
fn size_hint(&self) -> usize {
scale::Encode::size_hint(&self.selector) + scale::Encode::size_hint(&self.args)
scale::Encode::size_hint(&self.selector)
.checked_add(scale::Encode::size_hint(&self.args))
.unwrap()
}

#[inline]
Expand Down
21 changes: 21 additions & 0 deletions crates/env/src/engine/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,27 @@ use ink_primitives::{
LangError,
};

/// Convert a slice into an array reference.
///
/// Creates an array reference of size `$len` pointing to `$offset` within `$arr`.
///
/// # Panics
///
/// - The selected range is out of bounds given the supplied slice
/// - Integer overflow on `$offset + $len`
macro_rules! array_mut_ref {
($arr:expr, $offset:expr, $len:expr) => {{
{
fn as_array<T>(slice: &mut [T]) -> &mut [T; $len] {
slice.try_into().unwrap()
}
let offset: usize = $offset;
let slice = &mut $arr[offset..offset.checked_add($len).unwrap()];
as_array(slice)
}
}};
}

pub trait OnInstance: EnvBackend + TypedEnvBackend {
fn on_instance<F, R>(f: F) -> R
where
Expand Down
8 changes: 4 additions & 4 deletions crates/env/src/engine/off_chain/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ impl CryptoHash for Blake2x128 {
<Blake2x128 as HashOutput>::Type,
OutputType
);
let output: &mut OutputType = arrayref::array_mut_ref!(output, 0, 16);
let output: &mut OutputType = array_mut_ref!(output, 0, 16);
Engine::hash_blake2_128(input, output);
}
}
Expand All @@ -76,7 +76,7 @@ impl CryptoHash for Blake2x256 {
<Blake2x256 as HashOutput>::Type,
OutputType
);
let output: &mut OutputType = arrayref::array_mut_ref!(output, 0, 32);
let output: &mut OutputType = array_mut_ref!(output, 0, 32);
Engine::hash_blake2_256(input, output);
}
}
Expand All @@ -88,7 +88,7 @@ impl CryptoHash for Sha2x256 {
<Sha2x256 as HashOutput>::Type,
OutputType
);
let output: &mut OutputType = arrayref::array_mut_ref!(output, 0, 32);
let output: &mut OutputType = array_mut_ref!(output, 0, 32);
Engine::hash_sha2_256(input, output);
}
}
Expand All @@ -100,7 +100,7 @@ impl CryptoHash for Keccak256 {
<Keccak256 as HashOutput>::Type,
OutputType
);
let output: &mut OutputType = arrayref::array_mut_ref!(output, 0, 32);
let output: &mut OutputType = array_mut_ref!(output, 0, 32);
Engine::hash_keccak_256(input, output);
}
}
Expand Down
15 changes: 8 additions & 7 deletions crates/env/src/engine/on_chain/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,16 @@ impl<'a> EncodeScope<'a> {
impl<'a> scale::Output for EncodeScope<'a> {
fn write(&mut self, bytes: &[u8]) {
debug_assert!(
self.len() + bytes.len() <= self.capacity(),
self.len().checked_add(bytes.len()).unwrap() <= self.capacity(),
"encode scope buffer overflowed. capacity is {} but last write index is {}",
self.capacity(),
self.len() + bytes.len(),
self.len().checked_add(bytes.len()).unwrap(),
);
let start = self.len;
let len_bytes = bytes.len();
self.buffer[start..(start + len_bytes)].copy_from_slice(bytes);
self.len += len_bytes;
self.buffer[start..(start.checked_add(len_bytes)).unwrap()]
.copy_from_slice(bytes);
self.len = self.len.checked_add(len_bytes).unwrap();
}

fn push_byte(&mut self, byte: u8) {
Expand All @@ -101,7 +102,7 @@ impl<'a> scale::Output for EncodeScope<'a> {
self.capacity(),
);
self.buffer[self.len] = byte;
self.len += 1;
self.len = self.len.checked_add(1).unwrap();
}
}

Expand Down Expand Up @@ -144,7 +145,7 @@ impl<'a> ScopedBuffer<'a> {
self.buffer = rhs;
debug_assert_eq!(lhs.len(), len);
let len_after = self.buffer.len();
debug_assert_eq!(len_before - len_after, len);
debug_assert_eq!(len_before.checked_sub(len_after).unwrap(), len);
lhs
}

Expand Down Expand Up @@ -195,7 +196,7 @@ impl<'a> ScopedBuffer<'a> {
let mut encode_scope = EncodeScope::from(&mut buffer[offset..]);
scale::Encode::encode_to(&value, &mut encode_scope);
let encode_len = encode_scope.len();
self.offset += encode_len;
self.offset = self.offset.checked_add(encode_len).unwrap();
let _ = core::mem::replace(&mut self.buffer, buffer);
}

Expand Down
8 changes: 4 additions & 4 deletions crates/env/src/engine/on_chain/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl CryptoHash for Blake2x128 {
<Blake2x128 as HashOutput>::Type,
OutputType
);
let output: &mut OutputType = arrayref::array_mut_ref!(output, 0, 16);
let output: &mut OutputType = array_mut_ref!(output, 0, 16);
ext::hash_blake2_128(input, output);
}
}
Expand All @@ -69,7 +69,7 @@ impl CryptoHash for Blake2x256 {
<Blake2x256 as HashOutput>::Type,
OutputType
);
let output: &mut OutputType = arrayref::array_mut_ref!(output, 0, 32);
let output: &mut OutputType = array_mut_ref!(output, 0, 32);
ext::hash_blake2_256(input, output);
}
}
Expand All @@ -81,7 +81,7 @@ impl CryptoHash for Sha2x256 {
<Sha2x256 as HashOutput>::Type,
OutputType
);
let output: &mut OutputType = arrayref::array_mut_ref!(output, 0, 32);
let output: &mut OutputType = array_mut_ref!(output, 0, 32);
ext::hash_sha2_256(input, output);
}
}
Expand All @@ -93,7 +93,7 @@ impl CryptoHash for Keccak256 {
<Keccak256 as HashOutput>::Type,
OutputType
);
let output: &mut OutputType = arrayref::array_mut_ref!(output, 0, 32);
let output: &mut OutputType = array_mut_ref!(output, 0, 32);
ext::hash_keccak_256(input, output);
}
}
Expand Down
13 changes: 10 additions & 3 deletions integration-tests/erc1155/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,10 @@ mod erc1155 {

// Given that TokenId is a `u128` the likelihood of this overflowing is pretty
// slim.
self.token_id_nonce += 1;
#[allow(clippy::arithmetic_side_effects)]
Copy link
Contributor

@SkymanOne SkymanOne Jul 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not fun of a this. Is there a reason we can't do checked arithmetic operation here? If the operation is not implemented for the type, then we need to implement it first before introducing the change here and in cargo contract.

Otherwise we are breaking the arithmetic safety guarantees.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can. But it is not necessary here. Overflowing a u128 is impossible by incrementing by 1 per extrinsic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that we won't be able to use arithmetic operators (i.e. +, -, /, *) on numbers at all? I am just thinking of what impact it will have on the developer experience if we have to tell everyone knows that they need to use checked_ operations specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I understand correctly that we won't be able to use arithmetic operators (i.e. +, -, /, *) on numbers at all?

Correct.

I am just thinking of what impact it will have on the developer experience if we have to tell everyone knows that they need to use checked_ operations specifically.

Or saturating or wrapping. Developers should express the overflow behaviour in-code instead of leaving it open to a compile time switch. We should have done that from the beginning. People have to think about this anyways.

Copy link

Choose a reason for hiding this comment

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

Or saturating or wrapping.

Or use Wrapping and Saturating, so then the arithmetic itself wouldn't become an unreadable soup of function calls, e.g. this compiles:

#![deny(clippy::arithmetic_side_effects)]
use core::num::Wrapping;

fn foo(a: Wrapping<u32>, b: Wrapping<u32>) -> Wrapping<u32> {
    a + b
}

(Saturating is still unstable though, but we could just provide our own wrapper for the time being.)

{
self.token_id_nonce += 1;
}
self.balances.insert((caller, self.token_id_nonce), &value);

// Emit transfer event but with mint semantics
Expand Down Expand Up @@ -329,11 +332,15 @@ mod erc1155 {
.balances
.get((from, token_id))
.expect("Caller should have ensured that `from` holds `token_id`.");
sender_balance -= value;
// checks that sender_balance >= value were performed by caller
#[allow(clippy::arithmetic_side_effects)]
{
sender_balance -= value;
}
self.balances.insert((from, token_id), &sender_balance);

let mut recipient_balance = self.balances.get((to, token_id)).unwrap_or(0);
recipient_balance += value;
recipient_balance = recipient_balance.checked_add(value).unwrap();
self.balances.insert((to, token_id), &recipient_balance);

let caller = self.env().caller();
Expand Down
8 changes: 6 additions & 2 deletions integration-tests/erc20/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ mod erc20 {
return Err(Error::InsufficientAllowance)
}
self.transfer_from_to(&from, &to, value)?;
// We checked that allowance >= value
#[allow(clippy::arithmetic_side_effects)]
self.allowances
.insert((&from, &caller), &(allowance - value));
Ok(())
Expand All @@ -201,10 +203,12 @@ mod erc20 {
if from_balance < value {
return Err(Error::InsufficientBalance)
}

// We checked that from_balance >= value
#[allow(clippy::arithmetic_side_effects)]
self.balances.insert(from, &(from_balance - value));
let to_balance = self.balance_of_impl(to);
self.balances.insert(to, &(to_balance + value));
self.balances
.insert(to, &(to_balance.checked_add(value).unwrap()));
self.env().emit_event(Transfer {
from: Some(*from),
to: Some(*to),
Expand Down
9 changes: 6 additions & 3 deletions integration-tests/erc721/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ mod erc721 {

let count = owned_tokens_count
.get(caller)
.map(|c| c - 1)
.map(|c| c.checked_sub(1).unwrap())
.ok_or(Error::CannotFetchValue)?;
owned_tokens_count.insert(caller, &count);
token_owner.remove(id);
Expand Down Expand Up @@ -284,7 +284,7 @@ mod erc721 {

let count = owned_tokens_count
.get(from)
.map(|c| c - 1)
.map(|c| c.checked_sub(1).unwrap())
.ok_or(Error::CannotFetchValue)?;
owned_tokens_count.insert(from, &count);
token_owner.remove(id);
Expand All @@ -308,7 +308,10 @@ mod erc721 {
return Err(Error::NotAllowed)
};

let count = owned_tokens_count.get(to).map(|c| c + 1).unwrap_or(1);
let count = owned_tokens_count
.get(to)
.map(|c| c.checked_add(1).unwrap())
.unwrap_or(1);

owned_tokens_count.insert(to, &count);
token_owner.insert(id, to);
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/incrementer/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ mod incrementer {

#[ink(message)]
pub fn inc(&mut self, by: i32) {
self.value += by;
self.value = self.value.checked_add(by).unwrap();
}

#[ink(message)]
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/multi-contract-caller/accumulator/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub mod accumulator {
/// Mutates the internal value.
#[ink(message)]
pub fn inc(&mut self, by: i32) {
self.value += by;
self.value = self.value.checked_add(by).unwrap();
}

/// Returns the current state.
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/multi-contract-caller/subber/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ mod subber {
/// Decreases the `accumulator` value by some amount.
#[ink(message)]
pub fn dec(&mut self, by: i32) {
self.accumulator.inc(-by)
self.accumulator.inc(0i32.checked_sub(by).unwrap())
}
}
}
18 changes: 14 additions & 4 deletions integration-tests/multisig/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,10 @@ mod multisig {
pub fn add_owner(&mut self, new_owner: AccountId) {
self.ensure_from_wallet();
self.ensure_no_owner(&new_owner);
ensure_requirement_is_valid(self.owners.len() as u32 + 1, self.requirement);
ensure_requirement_is_valid(
(self.owners.len() as u32).checked_add(1).unwrap(),
self.requirement,
);
self.is_owner.insert(new_owner, &());
self.owners.push(new_owner);
self.env().emit_event(OwnerAddition { owner: new_owner });
Expand All @@ -398,6 +401,8 @@ mod multisig {
pub fn remove_owner(&mut self, owner: AccountId) {
self.ensure_from_wallet();
self.ensure_owner(&owner);
// If caller is an owner the len has to be > 0
#[allow(clippy::arithmetic_side_effects)]
let len = self.owners.len() as u32 - 1;
let requirement = u32::min(len, self.requirement);
ensure_requirement_is_valid(len, requirement);
Expand Down Expand Up @@ -522,7 +527,10 @@ mod multisig {
"There is a entry in `self.confirmations`. Hence a count must exit.",
);
// Will not underflow as there is at least one confirmation
confirmation_count -= 1;
#[allow(clippy::arithmetic_side_effects)]
{
confirmation_count -= 1;
}
self.confirmation_count
.insert(trans_id, &confirmation_count);
self.env().emit_event(Revocation {
Expand Down Expand Up @@ -618,14 +626,16 @@ mod multisig {
let key = (transaction, confirmer);
let new_confirmation = !self.confirmations.contains(key);
if new_confirmation {
count += 1;
count = count.checked_add(1).unwrap();
self.confirmations.insert(key, &());
self.confirmation_count.insert(transaction, &count);
}
let status = {
if count >= self.requirement {
ConfirmationStatus::Confirmed
} else {
// We checked that count < self.requirement
#[allow(clippy::arithmetic_side_effects)]
ConfirmationStatus::ConfirmationsNeeded(self.requirement - count)
}
};
Expand Down Expand Up @@ -677,7 +687,7 @@ mod multisig {
if self.confirmations.contains(key) {
self.confirmations.remove(key);
let mut count = self.confirmation_count.get(trans_id).unwrap_or(0);
count -= 1;
count = count.saturating_sub(1);
self.confirmation_count.insert(trans_id, &count);
}
}
Expand Down
8 changes: 6 additions & 2 deletions integration-tests/payment-channel/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ mod payment_channel {
return Err(Error::InvalidSignature)
}

// We checked that amount >= self.withdrawn
#[allow(clippy::arithmetic_side_effects)]
self.env()
.transfer(self.recipient, amount - self.withdrawn)
.map_err(|_| Error::TransferFailed)?;
Expand All @@ -157,7 +159,7 @@ mod payment_channel {
}

let now = self.env().block_timestamp();
let expiration = now + self.close_duration;
let expiration = now.checked_add(self.close_duration).unwrap();

self.env().emit_event(SenderCloseStarted {
expiration,
Expand Down Expand Up @@ -207,8 +209,10 @@ mod payment_channel {
return Err(Error::AmountIsLessThanWithdrawn)
}

// We checked that amount >= self.withdrawn
#[allow(clippy::arithmetic_side_effects)]
let amount_to_withdraw = amount - self.withdrawn;
self.withdrawn += amount_to_withdraw;
self.withdrawn.checked_add(amount_to_withdraw).unwrap();

self.env()
.transfer(self.recipient, amount_to_withdraw)
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/set-code-hash/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub mod incrementer {
/// Increments the counter value which is stored in the contract's storage.
#[ink(message)]
pub fn inc(&mut self) {
self.count += 1;
self.count = self.count.checked_add(1).unwrap();
ink::env::debug_println!(
"The new count is {}, it was modified using the original contract code.",
self.count
Expand Down
Loading