From 51130bc9a922acd95d1759c93a5f1fe1f259ed13 Mon Sep 17 00:00:00 2001 From: John Saigle <4022790+johnsaigle@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:44:02 -0400 Subject: [PATCH] solana: Configure clippy to deny possible truncation (#285) * solana: Configure clippy to deny possible truncation Adds a clippy rule to block the inclusion of code that can result in truncation (i.e. `as T` conversions) Adds linting directive to existing instances in the code and add comments about why they are safe Add a unit test on bitmap length to confirm that the maximum possible number of votes (i.e. maximum "length" of the bitmap) is within the limits of u8. * solana: deny truncation in Cargo.toml instead of GitHub yml * solana: Remove TODO referencing old PRs * node: solana CI - fix Cargo.toml path * solana: Change "as" to try_from() --- .github/workflows/solana.yml | 2 +- solana/Cargo.toml | 3 +-- .../example-native-token-transfers/src/bitmap.rs | 2 -- .../src/queue/rate_limit.rs | 15 +++++++++++---- .../src/instructions/governance.rs | 4 +++- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/.github/workflows/solana.yml b/.github/workflows/solana.yml index a7dadacf1..4b74b242b 100644 --- a/.github/workflows/solana.yml +++ b/.github/workflows/solana.yml @@ -50,7 +50,7 @@ jobs: run: cargo check --workspace --tests --manifest-path Cargo.toml - name: Run `cargo clippy` - run: cargo clippy --workspace --tests --manifest-path Cargo.toml + run: cargo clippy --workspace --tests --manifest-path Cargo.toml -- -Dclippy::cast_possible_truncation - name: Cache solana tools id: cache-solana diff --git a/solana/Cargo.toml b/solana/Cargo.toml index 419cda373..98b400a47 100644 --- a/solana/Cargo.toml +++ b/solana/Cargo.toml @@ -10,9 +10,8 @@ warnings = "deny" # Lints to improve security [workspace.lints.clippy] -# The following rules should be enabled but conflict with other open PRs. # TODO enable the commented rules below -# cast_possible_truncation = "deny" +cast_possible_truncation = "deny" # cast_lossless= "deny" # as_conversions = "deny" # arithmetic_side_effects = "deny" diff --git a/solana/programs/example-native-token-transfers/src/bitmap.rs b/solana/programs/example-native-token-transfers/src/bitmap.rs index 5c71724b3..e2466ee4f 100644 --- a/solana/programs/example-native-token-transfers/src/bitmap.rs +++ b/solana/programs/example-native-token-transfers/src/bitmap.rs @@ -43,8 +43,6 @@ impl Bitmap { pub fn count_enabled_votes(&self, enabled: Bitmap) -> u8 { let bm = BM::<128>::from_value(self.map) & BM::<128>::from_value(enabled.map); - // Conversion from usize to u8 is safe here. The Bitmap uses u128, so its maximum length - // (number of true bits) is 128. bm.len() .try_into() .expect("Bitmap length must not exceed the bounds of u8") diff --git a/solana/programs/example-native-token-transfers/src/queue/rate_limit.rs b/solana/programs/example-native-token-transfers/src/queue/rate_limit.rs index 3d2956635..338a72c7c 100644 --- a/solana/programs/example-native-token-transfers/src/queue/rate_limit.rs +++ b/solana/programs/example-native-token-transfers/src/queue/rate_limit.rs @@ -46,12 +46,15 @@ impl RateLimitState { /// Returns the capacity of the rate limiter. /// On-chain programs and unit tests should always use [`capacity`]. /// This function is useful in solana-program-test, where the clock sysvar - /// SECURITY: Integer division is OK here. We are not that concerned with precision. Removing - /// the remainder in this case is arguably more secure as it reduces the available capacity. - /// SECURITY: Sign loss is OK here. It is a conversion performed on a timestamp that must always be - /// positive. + // SECURITY: Integer division is OK here. We are not that concerned with precision. Removing + // the remainder in this case is arguably more secure as it reduces the available capacity. + // SECURITY: Sign loss is OK here. It is a conversion performed on a timestamp that must always be + // positive. + // SECURITY: Truncation is allowed here. Clippy warns about the final returned expression, but it is + // safe. #[allow(clippy::integer_division)] #[allow(clippy::cast_sign_loss)] + #[allow(clippy::cast_possible_truncation)] pub fn capacity_at(&self, now: UnixTimestamp) -> u64 { assert!(self.last_tx_timestamp <= now); @@ -76,6 +79,10 @@ impl RateLimitState { + time_passed as u128 * limit / (Self::RATE_LIMIT_DURATION as u128) }; + // The use of `min` here prevents truncation. + // The value of `limit` is u64 in reality. If both `calculated_capacity` and `limit` are at + // their maxiumum possible values (u128::MAX and u64::MAX), then u64::MAX will be chosen by + // `min`. So truncation is not possible. calculated_capacity.min(limit) as u64 } diff --git a/solana/programs/wormhole-governance/src/instructions/governance.rs b/solana/programs/wormhole-governance/src/instructions/governance.rs index 15b8babcd..04a8069d5 100644 --- a/solana/programs/wormhole-governance/src/instructions/governance.rs +++ b/solana/programs/wormhole-governance/src/instructions/governance.rs @@ -162,7 +162,9 @@ impl GovernanceMessage { acc.is_signer.write(writer)?; acc.is_writable.write(writer)?; } - (data.len() as u16).write(writer)?; + u16::try_from(data.len()) + .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "data length overflow"))? + .write(writer)?; writer.write_all(data) } }