ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.29k stars 5.77k forks source link

zero-length array types in expression context do not raise type errors #13652

Open bshastry opened 2 years ago

bshastry commented 2 years ago
contract C {
  function f() public {
    abi.decode("",  (int[0][]));
  }
}

throws

https://github.com/ethereum/solidity/blob/84cdcec2cfb1fe9d4a9171d3ed0ffefd6107ee42/libsolidity/codegen/ABIFunctions.cpp#L1174

Repro

$ solc --ir test.sol
NunoFilipeSantos commented 2 years ago

@bshastry what is the priority of this bug (High, Medium, Low)?

bshastry commented 2 years ago

I would say Low because it is unlikely a legitimate user would try to decode an empty string into a zero length array.

cameel commented 2 years ago

Output:

/solidity/libsolidity/codegen/ABIFunctions.cpp(1174): Throw in function std::string solidity::frontend::ABIFunctions::abiDecodingFunctionArrayAvailableLength(const solidity::frontend::ArrayType&, bool)
Dynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError>
std::exception::what: Solidity assertion failed
[solidity::util::tag_comment*] = Solidity assertion failed

This started happening in 0.8.8.

The [0] part seems important here. I think it should actually be a type error since int[0] is not a valid type (i.e. you cannot have a variable of this type). So given that, impact would be indeed low - this code would not work even if it wasn't causing an ICE.

But there's another case that's a bit more worrying:

contract C {
    function f() public {
        abi.encode(abi.decode("", (int[0])));
    }
}

This compiles just fine and it shouldn't. You probably cannot assign the result of that decoding to anything (because it's of type that does not exist) but as you can see above you can e.g. encode it again. This has a higher impact but it's still pretty unlikely to happen in practice other than by mistake so still not high enough for medium.

On the other hand I'd say that the effort is low so it's worth fixing. I think it only needs an extra check at the analysis stage. Cases that do the wrong thing without reporting an error are dangerous. Technically, it could classify as a codegen error.

Krish-bhardwaj commented 2 years ago

Hi @cameel I would like to get started with this issue can you give me a brief overview of how to get started with this

cameel commented 2 years ago

@Krish-bhardwaj Take this example:

contract C {
    int[0] x;
}

It currently produces an error:

Error: Array with zero length specified.
 --> test.sol:2:9:
  |
2 |     int[0] x;
  |         ^

The example from this issue should be reporting the same error.

The error is reported by DeclarationTypeChecker::endVisit(ArrayTypeName). Probably one of the conditions in that function makes it skip checking abi.decode(). Pass the test case from the issue though solc and using a debugger find out why exactly the check is skipped. Then modify the code so that it's no longer skipped. Note that this will likely make other things break so you'll need to find a solution that keeps them working while also solving the problem.

Also, try to explore similar cases. When you fix it, try using zero-length arrays in various ways and make sure none of them is broken the same way abi.decode() is now. Try various places where it's possible to use a type (using for, type(), UDVT declarations, parameter lists, error/event declarations, function pointer arguments, type tuples, struct fields and anything else you can think of). Also try wrapping the zero-length part in various ways - e.g. like in this issue it's a part of a dynamic array, which is probably part of the reason why it's not detected. Add the cases you try as new test cases, even if they don't crash the compiler (unless we already have them). They're still useful as regression tests and overall for better coverage of features.

Krish-bhardwaj commented 2 years ago

@cameel before this i have never worked on developing compiler can you assist me how should i set up this C++ project !

i have read through your docs but i don't have much of clarity on the project set up .

cameel commented 2 years ago

@Krish-bhardwaj I'm off this week, but if you need help please come to the #solidity-dev channel, you'll always find someone from the team there. Setting up for development is mostly a matter of cloning the repo, installing the dependencies and making sure you can run tests. This is covered by the Contributing page, but if something is missing, let us know (or just open an issue :)).

redone9211 commented 1 year ago

i would like to contribute to this issue , can you let me know how i can solve it.

cameel commented 1 year ago

@redone9211 See https://github.com/ethereum/solidity/issues/13652#issuecomment-1294913283.

ananyak19 commented 1 year ago

Hi, I would like to work on this issue. Please assign.

yoharsh14 commented 1 year ago

Hey! I would really love to work on this issue.

cameel commented 1 year ago

Feel tree to try. https://github.com/ethereum/solidity/issues/13652#issuecomment-1294913283 should get you started.

shikhar2712 commented 1 year ago

Hey @cameel definitely love to make this issue my first contri. Can I contribute?

nikola-matic commented 1 year ago

Hi all. I've remove the good first issue label from here, since this isn't really a good first issue. In addition, if this is your first time contributing to the Solidity project, try to find another issue that is more appropriate. Of course, feel leave a message on the issue you'd like to work on to ask whether it's a good fit, since this will make things a lot easier for everyone involved in the long run.