Problem: insufficient validation of signed integers in CLI arguments #346
Conversation
devashishdxt
requested review from
calvinaco,
leejw51crypto,
linfeng-crypto,
tomtau and
yihuang
as code owners
Codecov Report
@@ Coverage Diff @@
## master #346 +/- ##
==========================================
- Coverage 12.13% 12.12% -0.02%
==========================================
Files 32 32
Lines 5165 5171 +6
==========================================
Hits 627 627
- Misses 4289 4295 +6
Partials 249 249
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -82,24 +82,30 @@ contain valid denominations. Accounts may optionally be supplied with vesting pa | |||
| if errstart != nil { | |||
| return fmt.Errorf("failed to parse vesting start: %w", errstart) | |||
| } | |||
| vestingStart, errstart := strconv.ParseInt(vestingStartStr, 10, 64) | |||
| vestingStartUint, errstart := strconv.ParseUint(vestingStartStr, 10, 64) | |||
There was a problem hiding this comment.
should the bitsize be 63 instead of 64 (as it's later casted to a signed int64)?
There was a problem hiding this comment.
Changed to 32 to avoid overflow. Because we are converting it into int64 later.
Solution: Changed types from signed to unsigned integers. Fixes crypto-org-chain#332.
devashishdxt
force-pushed
the
issue-332
branch
from
f41e6ff
to
25c630a
Compare
There was a problem hiding this comment.
the integration test failed -- https://github.com/crypto-com/chain-main/pull/346/checks?check_run_id=1660486167#step:5:380 --- I'll try to rerun it with the latest master merged
Solution: Changed types from signed to unsigned integers. Fixes #332.