Disallow passing a empty array type to abi functions.#16416
Disallow passing a empty array type to abi functions.#16416
abi functions.#16416Conversation
|
There was an error when running Please check that your changes are working as intended. |
libsolidity/analysis/TypeChecker.cpp
Outdated
|
|
||
| #include <libsolidity/analysis/TypeChecker.h> | ||
|
|
||
| #include "DeclarationTypeChecker.h" |
aa31d42 to
12c9408
Compare
|
There was an error when running Please check that your changes are working as intended. |
libsolidity/analysis/TypeChecker.cpp
Outdated
|
|
||
| #include <libsolidity/analysis/TypeChecker.h> | ||
|
|
||
| #include "DeclarationTypeChecker.h" |
12c9408 to
bc948b5
Compare
| library C { | ||
| function f() view public { | ||
| C[0]; | ||
| C[1]; |
There was a problem hiding this comment.
Can you also add new cases testing specifically for the case of zero length in a situation like this? Or do we have them already?
There was a problem hiding this comment.
Also, I'd add variants for interfaces for completeness.
And rename C here to L (we usually use C for a contract so it makes it easy to confuse things).
There was a problem hiding this comment.
This test is specifically about decoding so I'd rename it to abi_decode_zero_length_array_type.sol. You can have int[0] in other situations and this test is not checking them.
There was a problem hiding this comment.
BTW, I'm assuming we already have coverage for those other cases, but maybe check whether that's the case. These are ones I can think of:
- Variable declaration
- Contract variable (
uint[0] x;,mapping(uint => uint[0]) m;,function() returns (uint[0] memory) f;) - Local variable (
uint[0] storage x;) - Function arguments and returns
- Try/catch statement (
try this.g() returns (uint[0] memory) {} catch (bytes memory b) {})
- Contract variable (
- Tuple declaration (
(uint[0] memory a, uint b) = ([], 1);) - Struct field (
struct S { uint[0] x; }) - Error and event parameters
- Array allocation (
new uint[0][](3); }) - No-op statement consisting of just the type (
uint[0];)
Technically, these are disallowed for various other reasons in analysis, but are still valid syntax, so would not hurt to have them covered:
- UDVT declaration (
type U is uint[0];) type()builtin (type(uint[0])).- Ternary operator (
true ? uint[0] : uint[0]) - Inline Array indexing (
[uint[0]][0])
There was a problem hiding this comment.
Oh, I see f6() is actually not decoding. I'd still split it into separate tests though.
There was a problem hiding this comment.
Reworked and more tests added.
| @@ -0,0 +1,38 @@ | |||
| contract C { | |||
| function f0() public { | |||
| abi.decode("", (int[0])); | |||
There was a problem hiding this comment.
| abi.decode("", (int[0])); | |
| abi.decode("", (int[0])); |
There was a problem hiding this comment.
Please also add the case from #13652 (comment). That one was not a compilation error so it's important that this no longer allowed:
contract C {
function f() public returns (bytes memory) {
return abi.encode(abi.decode("", (int[0])));
}
}There was a problem hiding this comment.
Added as a separated test case.
There was a problem hiding this comment.
Please also add a dedicated case for super (which is actually a type):
contract C {
function f() public {
super[0];
}
}contract C {
function f() public {
super[0] s;
}
}There was a problem hiding this comment.
Interestingly, an array of super is a valid expression on its own (super[3];) even though you cannot use it in a variable declaration (super[3] s;). I think it should be disallowed in either case.
I suspect that we have more things like this. Would be good to take a look at DeclarationTypeChecker and compare what other checks it has for ArrayTypeName that we do not here. Perhaps we should extract them into a shared function to avoid such discrepancies in the future.
There was a problem hiding this comment.
Also, pinging @matheusaaguiar to pay attention to this in test cases related to comptime. Looks like we have two places in the code where we evaluate array sizes. These should always behave the same way.
Also, WTF, why does every random issue recently turn out to be related to comptime one way or another :D
There was a problem hiding this comment.
Do not open your refrigerator.
There was a problem hiding this comment.
I reworked this part and put common logic in separated function, where it was possible.
bc948b5 to
0bc0d0b
Compare
| #include <libsolidity/analysis/DeclarationTypeChecker.h> | ||
|
|
||
| #include <libsolidity/analysis/ConstantEvaluator.h> | ||
| #include "TypeChecker.h" |
libsolidity/analysis/TypeChecker.cpp
Outdated
|
|
||
| #include <libsolidity/analysis/TypeChecker.h> | ||
|
|
||
| #include "ConstantEvaluator.h" |
libsolidity/analysis/TypeChecker.cpp
Outdated
| { | ||
| TypeType const& typeType = dynamic_cast<TypeType const&>(*baseType); | ||
| if (auto const* contractType = dynamic_cast<ContractType const*>(typeType.actualType())) | ||
| const auto actualType = typeType.actualType(); |
libsolidity/analysis/TypeChecker.cpp
Outdated
| TypeType const& typeType = dynamic_cast<TypeType const&>(*baseType); | ||
| if (auto const* contractType = dynamic_cast<ContractType const*>(typeType.actualType())) | ||
| const auto actualType = typeType.actualType(); | ||
| const auto actualTypeCategory = actualType->category(); |
|
Run |
0bc0d0b to
e7e72c7
Compare
libsolidity/analysis/TypeChecker.cpp
Outdated
| { | ||
| TypeType const& typeType = dynamic_cast<TypeType const&>(*baseType); | ||
| if (auto const* contractType = dynamic_cast<ContractType const*>(typeType.actualType())) | ||
| const auto actualType = typeType.actualType(); |
libsolidity/analysis/TypeChecker.cpp
Outdated
| TypeType const& typeType = dynamic_cast<TypeType const&>(*baseType); | ||
| if (auto const* contractType = dynamic_cast<ContractType const*>(typeType.actualType())) | ||
| const auto actualType = typeType.actualType(); | ||
| const auto actualTypeCategory = actualType->category(); |
There was a problem hiding this comment.
This error is not issued when running the script locally.
There was a problem hiding this comment.
AI says so:
With git grep -E, the regex engine is POSIX extended regex. In that dialect, \s is not a whitespace escape (that’s a PCRE/Perl thing). So:
^\s* is effectively interpreted as ^s* (i.e., “zero or more s characters at BOL”)
which means it won’t match indentation made of spaces/tabs
There was a problem hiding this comment.
As usual, AI is full of shit. You're getting the error because you're using const auto ... instead of auto const .... What OS are you running locally?
There was a problem hiding this comment.
I know what was the problem, but this script doesn't find the pattern. And this is good explanation. I'm using MacOS with git in the newest version 2.52.0.
There was a problem hiding this comment.
AI proposed to change \sto [[:space:]], because (POSIX ERE) does not understand \s :)
Looks like it's right. https://en.wikibooks.org/wiki/Regular_Expressions/POSIX-Extended_Regular_Expressions
There was a problem hiding this comment.
On the other hand it looks like they should be supported too https://www.boost.org/doc/libs/1_71_0/libs/regex/doc/html/boost_regex/syntax/basic_extended.html#boost_regex.syntax.basic_extended.single_character_character_class
Probable for some reason MacOS does not support it.
It's for boost.regexp only
7c5c3f2 to
56065c7
Compare
56065c7 to
8537278
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #13652 by adding validation to prevent zero-length arrays from being used in expression contexts like abi.decode, which previously caused internal compiler errors during code generation. The fix adds a new shared validation function checkArrayLengthExpression that ensures array lengths are non-zero, positive integers within valid bounds.
Changes:
- Added
TypeChecker::checkArrayLengthExpressionfunction to validate array length expressions - Refactored array length validation logic to use the new shared function in both
TypeChecker(for expression contexts) andDeclarationTypeChecker(for declaration contexts) - Added comprehensive test coverage for zero-length arrays in various contexts (abi.decode, inline arrays, conditionals, type expressions, etc.)
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| libsolidity/analysis/TypeChecker.h | Added declaration for new static checkArrayLengthExpression function |
| libsolidity/analysis/TypeChecker.cpp | Implemented checkArrayLengthExpression function and refactored IndexAccess visitor to validate array lengths and handle super/library types more explicitly |
| libsolidity/analysis/DeclarationTypeChecker.cpp | Refactored to use the new shared checkArrayLengthExpression function instead of duplicate validation logic |
| test/libsolidity/syntaxTests/array/invalid/abi_decode_zero_length_array_type.sol | New test covering the exact issue case with various multi-dimensional array patterns |
| test/libsolidity/syntaxTests/array/invalid/abi_encode_decode_zero_length_array_type.sol | New test for abi.encode with zero-length arrays |
| test/libsolidity/syntaxTests/array/invalid/contract_zero_index_access.sol | New test for contract type with zero-length array |
| test/libsolidity/syntaxTests/array/invalid/interface_zero_index_access.sol | New test for interface type with zero-length array |
| test/libsolidity/syntaxTests/array/invalid/library_zero_index_access.sol | New test for library type with zero-length array (shows both errors) |
| test/libsolidity/syntaxTests/array/invalid/super_index_access.sol | New test for super keyword with index access |
| test/libsolidity/syntaxTests/array/length/*.sol | Multiple new tests for zero-length arrays in different contexts (inline arrays, type expressions, conditionals) |
| test/libsolidity/syntaxTests/array/length/fixed_size_zero_length.sol | Expanded test with more zero-length array scenarios |
| test/libsolidity/syntaxTests/array/type_type_index_expression.sol | New test showing valid index expressions with various types |
| test/libsolidity/syntaxTests/array/contract_index_access.sol | Updated test to use non-zero index and pure function |
| test/libsolidity/syntaxTests/array/interface_index_access.sol | New test for valid interface array type |
| test/libsolidity/syntaxTests/array/invalid/library_index_access.sol | Updated test for clarity (renamed library) |
| test/libsolidity/syntaxTests/nameAndTypeResolution/366_invalid_array_as_statement.sol | Updated expected error message |
| test/libsolidity/syntaxTests/indexing/struct_array_noninteger_index.sol | Updated expected error message |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8537278 to
a01870d
Compare
Hmm... looks like |
| baseType, | ||
| lengthValue ? u256(lengthValue->numerator()) : u256(0) | ||
| ); | ||
| auto const lengthValue = TypeChecker::checkArrayLengthExpression(*length, m_errorReporter); |
There was a problem hiding this comment.
| auto const lengthValue = TypeChecker::checkArrayLengthExpression(*length, m_errorReporter); | |
| u256 const lengthValue = TypeChecker::checkArrayLengthExpression(*length, m_errorReporter); |
There was a problem hiding this comment.
It would also be clearer to rename length to lengthExpression and for this one to be length.
libsolidity/analysis/TypeChecker.cpp
Outdated
| if (_arrayLengthExpression.annotation().type && _arrayLengthExpression.annotation().type->category() == Type::Category::RationalNumber) | ||
| lengthValue = dynamic_cast<RationalNumberType const&>(*_arrayLengthExpression.annotation().type).value(); |
There was a problem hiding this comment.
Can you try removing this special case? I'm pretty sure ConstantEvaluator can already handle rational literals. I'm not sure why this is singled out here.
solidity/libsolidity/analysis/ConstantEvaluator.cpp
Lines 392 to 396 in 80d5c53
libsolidity/analysis/TypeChecker.cpp
Outdated
| return u256(lengthValue->numerator()); | ||
|
|
||
| solAssert(lengthValue || _errorReporter.hasErrors(), "Must have reported errors."); | ||
| return 0; |
There was a problem hiding this comment.
It the function does not always return a valid value, it should be returning an optional or throw. It's too easy for someone to forget to check the error reporter and use the 0 as the actual result.
libsolidity/analysis/TypeChecker.h
Outdated
|
|
||
| /// Checks that `_arrayLengthExpression` is allowed for array length value. | ||
| /// @returns a value of length or report an error. | ||
| static u256 checkArrayLengthExpression(Expression const& _arrayLengthExpression, langutil::ErrorReporter& _errorReporter); |
There was a problem hiding this comment.
The fact that constant evaluation happens here is important enough that I'd include it in the name.
| static u256 checkArrayLengthExpression(Expression const& _arrayLengthExpression, langutil::ErrorReporter& _errorReporter); | |
| /// Evaluates `_arrayLengthExpression` and checks if the value is allowed as an array length. | |
| /// @returns length if it can be evaluated without errors. Otherwise returns 0 and adds an error to the error reporter. | |
| static u256 evaluateAndCheckArrayLengthExpression(Expression const& _arrayLengthExpression, langutil::ErrorReporter& _errorReporter); |
Alternatively, if my suggestion to remove the special case in evaluation works, you could keep the name if you do evaluation outside and have the function accept TypedRational.
There was a problem hiding this comment.
Also, I wonder if it would not make more sense to just put this function in ConstantEvaluator. It's used by two analysis components and the concept of array length is general enough that I think it would not be out of place. And ConstantEvaluator already uses an error reporter to report other errors.
libsolidity/analysis/TypeChecker.cpp
Outdated
| auto const actualType = typeType.actualType(); | ||
| auto const actualTypeCategory = actualType->category(); |
There was a problem hiding this comment.
You dropped the pointer. Also, this is more readable when the types are not hidden. They are very short.
| auto const actualType = typeType.actualType(); | |
| auto const actualTypeCategory = actualType->category(); | |
| Type const* actualType = typeType.actualType(); | |
| Category const actualTypeCategory = actualType->category(); |
There was a problem hiding this comment.
TBH I would not even introduce variables for these. typeType->actualType() and typeType->actualType()->category() are perfectly clear and not that long. On the other hand extra local variables always increase cognitive load because you have to keep track of that context in your head now.
libsolidity/analysis/TypeChecker.cpp
Outdated
| else if (contractType->isSuper()) | ||
| { | ||
| if (auto indexValue = dynamic_cast<RationalNumberType const*>(type(*index))) | ||
| length = indexValue->literalValue(nullptr); | ||
| else | ||
| m_errorReporter.fatalTypeError(3940_error, index->location(), "Integer constant expected."); | ||
| m_errorReporter.typeError( | ||
| 5530_error, | ||
| _access.location(), | ||
| "Index notation is not allowed for type."); | ||
| } |
There was a problem hiding this comment.
I suggested that we should disallow things like super[3], but I would not do it right in this PR. This is unrelated to the fix and needs a separate changelog entry and tests. And we should first consider whether the change is breaking.
Can you file an issue first? I already suggested splitting off the refactor into a separate PR below and I don't want us to get multiple levels deep :) This bit can be left for later.
libsolidity/analysis/TypeChecker.cpp
Outdated
| case Type::Category::Magic: | ||
| case Type::Category::Module: | ||
| case Type::Category::InaccessibleDynamic: | ||
| solAssert(false, "Unexpected actual type category of TypeType."); |
There was a problem hiding this comment.
Your message is more precise, but I think this one would give more context as to what went wrong:
| solAssert(false, "Unexpected actual type category of TypeType."); | |
| solAssert(false, "Unexpected type of array size expression."); |
Still, it's just an assert so a message is optional.
| { | ||
| index->accept(*this); | ||
| auto const lengthValue = checkArrayLengthExpression(*index, m_errorReporter); | ||
| resultType = TypeProvider::typeType(TypeProvider::array(DataLocation::Memory, actualType, lengthValue)); | ||
| } |
There was a problem hiding this comment.
Looks like we weren't using constant evaluator here previously, so this change actually affects functionality. Now we support a much wider range of expressions here. This needs to be included in a changelog entry and also needs test coverage.
I think that we might be better off splitting off this refactor into a separate PR and putting the fix on top of it, to make it clear which changes belong to which.
There was a problem hiding this comment.
Alternatively, if you want to avoid a refactor PR, you could also just not use constant evaluator here so that the current behavior is preserved. We should be doing that eventually, but for the purpose of this fix I only wanted the checks to be unified. You can achieve that by keeping the evaluation out of checkArrayLengthExpression() like I suggested in #16416 (comment).
In that case please file an issue for this though (i.e. for the fact that the types checked here do not support the same range of expressions as the ones in declarations; e.g. no constants or arithmetic on literals).
There was a problem hiding this comment.
Do you mean to move the common code from DeclarationTypeChecker to ConstantEvaluator and don't change the TypeChecker logic, and later in the fix PR just use evaluateAndCheckArrayLengthExpression in type checker as well?
EDIT: I haven't seen @cameel last message when writing this one.
There was a problem hiding this comment.
I will make the rector PR. I want to assure that the index expression in this two cases have the same semantic.
There was a problem hiding this comment.
Do you mean to move the common code from
DeclarationTypeCheckertoConstantEvaluatorand don't change theTypeCheckerlogic, and later in the fix PR just useevaluateAndCheckArrayLengthExpressionin type checker as well?
I meant move the common code to evaluateAndCheckArrayLengthExpression(), but without the zero-length check. Leave that last bit for the fix PR.
Though looking at it again now, maybe it would make more sense to do it the other way around. I.e. for the fix just duplicate the zero length check, without moving any code to a shared function. Then put the refactor on top of the fix and do the function there. The upside of that is that the fix is almost done so we'll probably be able to merge it much quicker than the refactor.
And don't forget that the "refactor" is not really just a refactor after all because it does affect the language. So it needs tests for the behavior that gets changed and a changelog entry.
There was a problem hiding this comment.
PR is ready. Zero length check is only where it was initially, so in DeclarationTypeChecker. I haven't changed the logic of the TypeChecker. It's also only refactoring, so I haven't used evaluateAndCheckArrayLengthExpression there yet. On the top of it would make the fix PR which will only be using common function in the TypeChecker and all the additional required steps you mentioned. Tests + changelog.
a01870d to
bdbd71c
Compare
|
This pull request is stale because it has been open for 14 days with no activity. |
|
This pull request is stale because it has been open for 14 days with no activity. |
This PR adds additional check in
TypeCheckerforIndexAccesswhich ensures that the array size is not0.As a result of this change, type which is an empty array of contracts is also forbidden.
Fixes #13652.
Depends on: #16490