fix(costs): check size arithmetic for overflow#737
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds overflow detection to arithmetic operations used in element sizing and storage cost computation. New ChangesOverflow Detection in Size Arithmetic and Storage Costs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
costs/src/storage_cost/key_value_cost.rs (1)
62-65: 💤 Low valueMinor duplication of
with_required_spacehelper.This helper duplicates the logic in
OperationCost::checked_len_with_required_space. Consider extracting to a shared utility if more call sites emerge, but acceptable as-is for locality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@costs/src/storage_cost/key_value_cost.rs` around lines 62 - 65, The helper function with_required_space duplicates logic already implemented in OperationCost::checked_len_with_required_space; replace calls to with_required_space (or remove the helper) and reuse OperationCost::checked_len_with_required_space to avoid duplication, or extract the shared checked-add logic into a new utility function used by both with_required_space and OperationCost::checked_len_with_required_space so both call the single implementation; update references to the old helper accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@grovedb/src/batch/mod.rs`:
- Around line 2957-2964: The unchecked cast "item_value.len() as u32" can
truncate; replace it with a fallible conversion using
u32::try_from(item_value.len()) and propagate/map the Err to MerkError::Overflow
where item_len is used (the variable assigned before computing
sum_item_value_cost and passed into checked_value_defined_cost). Update the
surrounding code paths (the block that computes item_len and then
sum_item_value_cost) to handle the Result and return or convert the error to
MerkError::Overflow so overflow is caught instead of silently truncating.
In `@merk/src/merk/mod.rs`:
- Around line 425-431: Replace the lossy casts where
key_updates.updated_root_key_from.as_ref().map(|k| k.len() as u32) and
tree_key.len() as u32 are used: perform safe conversions with u32::try_from(...)
for both the optional updated_root_key length and tree_key.len(), map any
TryFromIntError into Error::CorruptedData with a short contextual message
(mentioning updated_root_key or tree_key), and then pass those results into
for_updated_root_cost(...), finally mapping that call's error into
Error::CostsError as before; reference the symbols
key_updates.updated_root_key_from, tree_key.len(), for_updated_root_cost,
Error::CorruptedData and Error::CostsError when making the changes.
---
Nitpick comments:
In `@costs/src/storage_cost/key_value_cost.rs`:
- Around line 62-65: The helper function with_required_space duplicates logic
already implemented in OperationCost::checked_len_with_required_space; replace
calls to with_required_space (or remove the helper) and reuse
OperationCost::checked_len_with_required_space to avoid duplication, or extract
the shared checked-add logic into a new utility function used by both
with_required_space and OperationCost::checked_len_with_required_space so both
call the single implementation; update references to the old helper accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52811e08-dd70-4822-914c-d894862fb170
📒 Files selected for processing (10)
costs/src/error.rscosts/src/lib.rscosts/src/storage_cost/key_value_cost.rscosts/src/storage_cost/mod.rscosts/tests/coverage_regression.rsgrovedb-element/src/element/helpers.rsgrovedb-element/src/error.rsgrovedb-element/tests/element_constructors_helpers.rsgrovedb/src/batch/mod.rsmerk/src/merk/mod.rs
| let item_len = item_value.len() as u32; | ||
| let sum_item_value_cost = SUM_ITEM_COST_SIZE | ||
| + flags_len | ||
| + flags_len.required_space() as u32 | ||
| + item_len | ||
| + item_len.required_space() as u32 | ||
| + wrapper_overhead; | ||
| let sum_item_value_cost = checked_value_defined_cost(&[ | ||
| SUM_ITEM_COST_SIZE, | ||
| flags_len, | ||
| flags_len.required_space() as u32, | ||
| item_len, | ||
| item_len.required_space() as u32, | ||
| wrapper_overhead, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining potentially truncating usize->u32 casts in this file.
rg -nP '\bas\s*u32\b' --type=rust -C2Repository: dashpay/grovedb
Length of output: 50372
🏁 Script executed:
# Check the file size first
wc -l grovedb/src/batch/mod.rsRepository: dashpay/grovedb
Length of output: 89
🏁 Script executed:
# Read the specific lines in question
sed -n '2950,2970p' grovedb/src/batch/mod.rsRepository: dashpay/grovedb
Length of output: 1351
🏁 Script executed:
# Also check a broader context around the closure to understand the error handling pattern
sed -n '2910,2980p' grovedb/src/batch/mod.rsRepository: dashpay/grovedb
Length of output: 4500
Unchecked usize -> u32 cast can bypass overflow safety.
Line 2957 uses item_value.len() as u32, which silently truncates for values exceeding u32::MAX and undercounts cost before your checked sum runs. Convert with u32::try_from(item_value.len()) and map the error to MerkError::Overflow.
Suggested fix
- let item_len = item_value.len() as u32;
+ let item_len = u32::try_from(item_value.len()).map_err(|_| {
+ MerkError::Overflow("value defined cost overflow")
+ })?;
let sum_item_value_cost = checked_value_defined_cost(&[
SUM_ITEM_COST_SIZE,
flags_len,
flags_len.required_space() as u32,
item_len,
item_len.required_space() as u32,
wrapper_overhead,
])?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@grovedb/src/batch/mod.rs` around lines 2957 - 2964, The unchecked cast
"item_value.len() as u32" can truncate; replace it with a fallible conversion
using u32::try_from(item_value.len()) and propagate/map the Err to
MerkError::Overflow where item_len is used (the variable assigned before
computing sum_item_value_cost and passed into checked_value_defined_cost).
Update the surrounding code paths (the block that computes item_len and then
sum_item_value_cost) to handle the Result and return or convert the error to
MerkError::Overflow so overflow is caught instead of silently truncating.
| key_updates | ||
| .updated_root_key_from | ||
| .as_ref() | ||
| .map(|k| k.len() as u32), | ||
| tree_key.len() as u32, | ||
| ) | ||
| .map_err(Error::CostsError) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current call-site uses lossy casts and inspect surrounding code.
rg -n --type=rust -C4 'for_updated_root_cost\(|len\(\)\s+as\s+u32|u32::try_from\(' merk/src/merk/mod.rs
# Verify constructor signature for root-cost sizing in costs crate.
rg -n --type=rust -C3 'fn\s+for_updated_root_cost\s*\(' costs/src/storage_cost/key_value_cost.rsRepository: dashpay/grovedb
Length of output: 1174
Replace lossy usize → u32 casts with safe conversions and wrap errors with context.
Lines 428 and 429 use as u32 to cast usize values, which silently truncates on platforms where usize exceeds 32 bits. Additionally, line 431's .map_err(Error::CostsError) does not follow the required error-wrapping pattern. Use u32::try_from(...) for safe conversion and wrap conversion failures with Error::CorruptedData containing contextual messages before passing to for_updated_root_cost(...).
Suggested patch
- Some(cost_return_on_error_no_add!(
- inner_cost,
- KeyValueStorageCost::for_updated_root_cost(
- key_updates
- .updated_root_key_from
- .as_ref()
- .map(|k| k.len() as u32),
- tree_key.len() as u32,
- )
- .map_err(Error::CostsError)
- ))
+ let previous_root_len = cost_return_on_error_no_add!(
+ inner_cost,
+ key_updates
+ .updated_root_key_from
+ .as_ref()
+ .map(|k| {
+ u32::try_from(k.len()).map_err(|e| {
+ Error::CorruptedData(format!(
+ "updated root key length overflow: {}",
+ e
+ ))
+ })
+ })
+ .transpose()
+ );
+ let new_root_len = cost_return_on_error_no_add!(
+ inner_cost,
+ u32::try_from(tree_key.len()).map_err(|e| {
+ Error::CorruptedData(format!(
+ "new root key length overflow: {}",
+ e
+ ))
+ })
+ );
+ Some(cost_return_on_error_no_add!(
+ inner_cost,
+ KeyValueStorageCost::for_updated_root_cost(
+ previous_root_len,
+ new_root_len,
+ )
+ .map_err(Error::CostsError)
+ ))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@merk/src/merk/mod.rs` around lines 425 - 431, Replace the lossy casts where
key_updates.updated_root_key_from.as_ref().map(|k| k.len() as u32) and
tree_key.len() as u32 are used: perform safe conversions with u32::try_from(...)
for both the optional updated_root_key length and tree_key.len(), map any
TryFromIntError into Error::CorruptedData with a short contextual message
(mentioning updated_root_key or tree_key), and then pass those results into
for_updated_root_cost(...), finally mapping that call's error into
Error::CostsError as before; reference the symbols
key_updates.updated_root_key_from, tree_key.len(), for_updated_root_cost,
Error::CorruptedData and Error::CostsError when making the changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #737 +/- ##
===========================================
+ Coverage 91.42% 91.48% +0.05%
===========================================
Files 236 236
Lines 67053 67210 +157
===========================================
+ Hits 61305 61487 +182
+ Misses 5748 5723 -25
🚀 New features to boost your workflow:
|
QuantumExplorer
changed the title
1 participant
Summary
Fixes #684.
Verification
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests