-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(lint): add UnsafeTypecast lint #11046
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
base: master
Are you sure you want to change the base?
feat(lint): add UnsafeTypecast lint #11046
Conversation
Any updates, can update the tests for you if needed @TropicalDog17 |
…foundry into feat/unsafe-typecast-lint
Hi @0xClandestine, I've updated the test, but not sure how to emit a helpful fix for the lint, can you help me on that? Thank a lot! |
maybe in this case a warning is enough, and no fix suggestion is required, but you could tell them to consider adding an explicit check like: if (a > type(uint32).max) revert TypecastOverflow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please address the feedback
solar_ast::LitKind::Number(num) => { | ||
if is_negative_number_literal(num) { | ||
Some(hir::ElementaryType::Int(solar_ast::TypeSize::ZERO)) | ||
} else { | ||
Some(hir::ElementaryType::UInt(solar_ast::TypeSize::ZERO)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the compiler performs checks when assigning literal values, i think it is safe to remove this check, which would also remove the need of the bigint
dep.
Please return None
and add a comment explaining the reason why
Assigning values which cannot fit into the type gives a compiler error. For example:
uint8 foo = 300;
The largest value an uint8 can hold is (2 8) - 1 = 255. So, the compiler says:
value 300 does not fit into type uint8
ref: https://solang.readthedocs.io/en/latest/language/types.html
…foundry into feat/unsafe-typecast-lint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, some nits 🤝
Co-authored-by: clandestine.eth <96172957+0xClandestine@users.noreply.github.com>
warning[unsafe-typecast]: typecasts that can truncate values should be checked | ||
--> ROOT/testdata/UnsafeTypecast.sol:LL:CC | ||
| | ||
74 | uint248 b = uint248(a); | ||
| ---------- | ||
| | ||
= note: Consider disabling this lint if you're certain the cast is safe: | ||
|
||
- // Cast is safe because [explain why] | ||
- // forge-lint: disable-next-line(unsafe-typecast) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is failing. Have you updated the blessed files?
If not, run cargo test -p forge --test ui -- --bless
and ensure that all tests pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi I ran the tests locally, it worked fine but but ci still fails, can you help me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me pull your fork and try myself, will push something this afternoon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi
locally it worked cause u hadn't pulled the latest changes from the remote, where u had merged from master
, which fixed an issue when printing Snippet::Block
as if they were removed diffs (hence the extra -
before the block)
Co-authored-by: 0xrusowsky <90208954+0xrusowsky@users.noreply.github.com>
…foundry into feat/unsafe-typecast-lint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TropicalDog17 while we wait for others to review, please open a PR updating the foundry book. Until docs are updated CI will fail.
you need to add a section for the new lint here:
https://github.com/foundry-rs/book/blob/master/vocs/docs/pages/forge/linting.md
Went ahead and updated docs 🤝 |
Caught this when testing the lint on a previous audit commit at EigenLayer: To reproduce
The application panicked (crashed).
Message: invalid fixed-bytes size: 46
Location: /Users/gg/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/solar-ast-0.1.5/src/ast/ty.rs:245
This is a bug. Consider reporting it at https://github.com/foundry-rs/foundry
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
⋮ 9 frames hidden ⋮
10: forge_lint::sol::med::unsafe_typecast::infer_source_type::hb9381693201f3de2
at <unknown source file>:<unknown line>
11: forge_lint::sol::med::unsafe_typecast::is_unsafe_typecast_hir::hc75a0e499a3756f1
at <unknown source file>:<unknown line>
12: forge_lint::sol::med::unsafe_typecast::<impl forge_lint::linter::late::LateLintPass for forge_lint::sol::med::UnsafeTypecast>::check_expr::h86f336a31d5eeb28
at <unknown source file>:<unknown line>
13: <forge_lint::linter::late::LateLintVisitor as solar_sema::hir::visit::Visit>::visit_expr::heed864d619e159de
at <unknown source file>:<unknown line>
14: <forge_lint::linter::late::LateLintVisitor as solar_sema::hir::visit::Visit>::visit_expr::heed864d619e159de
at <unknown source file>:<unknown line>
15: <forge_lint::linter::late::LateLintVisitor as solar_sema::hir::visit::Visit>::visit_function::h3f4620f34a7d0cc2
at <unknown source file>:<unknown line>
16: solar_sema::hir::visit::visit_nested_items::{{closure}}::hf18b76f65fc1a551
at <unknown source file>:<unknown line>
17: forge_lint::sol::SolidityLinter::process_source_hir::h090419321e5637dd
at <unknown source file>:<unknown line>
18: <&rayon::iter::par_bridge::IterParallelProducer<Iter> as rayon::iter::plumbing::UnindexedProducer>::fold_with::h9f490d2ce38eac94
at <unknown source file>:<unknown line>
19: <rayon_core::job::StackJob<L,F,R> as rayon_core::job::Job>::execute::ha44f7714e8b09998
at <unknown source file>:<unknown line>
20: rayon_core::registry::WorkerThread::wait_until_cold::h0aa2a024b0c4ca85
at <unknown source file>:<unknown line>
21: rayon_core::registry::ThreadBuilder::run::haf19535023a3f6b1
at <unknown source file>:<unknown line>
22: std::sys::backtrace::__rust_begin_short_backtrace::hed6bf63077d481ec
at <unknown source file>:<unknown line>
23: core::ops::function::FnOnce::call_once{{vtable.shim}}::h5bacb7b49023c67c
at <unknown source file>:<unknown line>
24: std::sys::pal::unix::thread::Thread::new::thread_start::hf0cf67e969add794
at <unknown source file>:<unknown line>
25: __pthread_cond_wait<unknown>
at <unknown source file>:<unknown line>
Run with COLORBT_SHOW_HIDDEN=1 environment variable to disable frame filtering.
Run with RUST_BACKTRACE=full to include source snippets.
The application panicked (crashed).
Message: invalid fixed-bytes size: 46
Location: /Users/gg/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/solar-ast-0.1.5/src/ast/ty.rs:245
This is a bug. Consider reporting it at https://github.com/foundry-rs/foundry
━━━━━━━━━━━━━━━━━━━━━━━━[1] 86533 abort forge lint --only-lint unsafe-typecast |
I also don't think it's throwing a warning for this bug, may be because it's crashing early. function _addInt128(uint64 a, int128 b) internal pure returns (uint64) {
return uint64(uint128(int128(uint128(a)) + b));
} |
thanks for flagging this, seems to be solar-related. i'll debug tomorrow and patch it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, lgtm!
} | ||
|
||
/// Infers the elementary type of a source expression. | ||
/// For cast chains, returns the ultimate source type, not intermediate cast results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we check intermediate casts for cast chains? I think that's why this isn't throwing:
function _addInt128(uint64 a, int128 b) internal pure returns (uint64) {
return uint64(uint128(int128(uint128(a)) + b));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would suggest adding a test, you can also reproduce using the following commands:
foundryup --pr 11046
git clone https://github.com/Layr-Labs/eigenlayer-contracts.git
git checkout 44ece6d
forge lint src/contracts/core/AllocationManager.sol --only-lint unsafe-typecast
Motivation
Closes #11029
Solution
PR Checklist