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

ABI decoder #2863

Merged
merged 4 commits into from Nov 29, 2017

Conversation

Projects
None yet
2 participants
@chriseth
Contributor

chriseth commented Sep 1, 2017

  • always check that offset + min size <= size, if possible do it efficiently (i.e. group statically sized types) - reconfirm this, currently we only check offset < end and not offset + size < end
  • implement struct decoding
  • manual coverage testing
  • clarify if we need to call "interfaceType()" for the decoder types.

@chriseth chriseth added the in progress label Sep 1, 2017

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Sep 12, 2017

Contributor

Latest version should work for all value types - but does not yet have any tests.

Contributor

chriseth commented Sep 12, 2017

Latest version should work for all value types - but does not yet have any tests.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Sep 19, 2017

Member

@chriseth can you rebase on top of #2924 ?

Member

axic commented Sep 19, 2017

@chriseth can you rebase on top of #2924 ?

Show outdated Hide outdated Changelog.md Outdated
Show outdated Hide outdated libsolidity/ast/Types.h Outdated
case Type::Category::Struct:
solAssert(false, "Struct cleanup requested.");
solAssert(_type.dataStoredIn(DataLocation::Storage), "Cleanup requested for non-storage reference type.");
templ("body", "cleaned := value");

This comment has been minimized.

@axic

axic Sep 21, 2017

Member

Where is the actual cleanup?

@axic

axic Sep 21, 2017

Member

Where is the actual cleanup?

This comment has been minimized.

@chriseth

chriseth Sep 22, 2017

Contributor

There is no cleanup to be performed for a storage pointer. All 2**256 values are valid.

@chriseth

chriseth Sep 22, 2017

Contributor

There is no cleanup to be performed for a storage pointer. All 2**256 values are valid.

This comment has been minimized.

@axic

axic Sep 22, 2017

Member

So this part of the change is not restricted to this PR?

@axic

axic Sep 22, 2017

Member

So this part of the change is not restricted to this PR?

This comment has been minimized.

@chriseth

chriseth Sep 25, 2017

Contributor

the encoder does not call cleanup on structs - the control flow branches off earlier.

@chriseth

chriseth Sep 25, 2017

Contributor

the encoder does not call cleanup on structs - the control flow branches off earlier.

This comment has been minimized.

@chriseth

chriseth Sep 25, 2017

Contributor

Or actually, better explanation: The encoder has a "source type" and a "target type", while the decoder only has a single type. For structs, the target type is already an integer and not a struct, and thus the cleanup will be called on the integer.

@chriseth

chriseth Sep 25, 2017

Contributor

Or actually, better explanation: The encoder has a "source type" and a "target type", while the decoder only has a single type. For structs, the target type is already an integer and not a struct, and thus the cleanup will be called on the integer.

for (auto const& t: _types)
functionName += t->identifier();
if (_fromMemory)
functionName += "_fromMemory";

This comment has been minimized.

@axic

axic Sep 21, 2017

Member

Hm, wondering if we should commit to camelcase or underscores, this one mixes the two.

@axic

axic Sep 21, 2017

Member

Hm, wondering if we should commit to camelcase or underscores, this one mixes the two.

This comment has been minimized.

@chriseth

chriseth Sep 22, 2017

Contributor

underscores are used to separate components.

@chriseth

chriseth Sep 22, 2017

Contributor

underscores are used to separate components.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Sep 25, 2017

Contributor

Rebased, fixed some stuff, did manual coverage tests and ran against full EndToEnd tests.

The only thing left here is the decoder for structs.

Contributor

chriseth commented Sep 25, 2017

Rebased, fixed some stuff, did manual coverage tests and ran against full EndToEnd tests.

The only thing left here is the decoder for structs.

@chriseth chriseth changed the title from [WIP] ABI decoder to ABI decoder Oct 4, 2017

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Oct 4, 2017

Contributor

@axic this is ready for review.

Contributor

chriseth commented Oct 4, 2017

@axic this is ready for review.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Oct 5, 2017

Member

Needs rebasing since the other ABI PRs were merged.

Member

axic commented Oct 5, 2017

Needs rebasing since the other ABI PRs were merged.

/// into memory. If @a _fromMemory is true, decodes from memory instead of
/// from calldata.
/// Can allocate memory.
/// Inputs: <source_offset> <source_end> (layout reversed on stack)

This comment has been minimized.

@axic

axic Oct 5, 2017

Member

These are the inputs if _fromMemory is true, otherwise no inputs or is it an offset within the calldata?

@axic

axic Oct 5, 2017

Member

These are the inputs if _fromMemory is true, otherwise no inputs or is it an offset within the calldata?

This comment has been minimized.

@chriseth

chriseth Oct 11, 2017

Contributor

these are either pointers to calldata or to memory.

@chriseth

chriseth Oct 11, 2017

Contributor

these are either pointers to calldata or to memory.

This comment has been minimized.

@axic

axic Oct 13, 2017

Member

Also, are those both absolute pointers or is source_end actually source_len?

@axic

axic Oct 13, 2017

Member

Also, are those both absolute pointers or is source_end actually source_len?

This comment has been minimized.

@chriseth

chriseth Oct 13, 2017

Contributor

Both are absolute pointers.

@chriseth

chriseth Oct 13, 2017

Contributor

Both are absolute pointers.

/// Outputs: <value0> <value1> ... <valuen>
/// The values represent stack slots. If a type occupies more or less than one
/// stack slot, it takes exactly that number of values.
std::string tupleDecoder(TypePointers const& _types, bool _fromMemory = false);

This comment has been minimized.

@axic

axic Oct 5, 2017

Member

So it returns the function name which has a signature of <name>(source_offset, source_end) -> <v0>, <v1>, ..., <vn>. is that correct?

@axic

axic Oct 5, 2017

Member

So it returns the function name which has a signature of <name>(source_offset, source_end) -> <v0>, <v1>, ..., <vn>. is that correct?

This comment has been minimized.

@chriseth

chriseth Oct 11, 2017

Contributor

yes

@chriseth

chriseth Oct 11, 2017

Contributor

yes

This comment has been minimized.

@axic

axic Oct 12, 2017

Member

Cool, I was going to try it in the IR pull request and that API should work well.

@axic

axic Oct 12, 2017

Member

Cool, I was going to try it in the IR pull request and that API should work well.

compileAndRun(sourceCode);
ABI_CHECK(
callContractFunction("f(uint16,int16,address,bytes3,bool)", 1, 2, 3, "a", true),
encodeArgs(u256(1), u256(2), u256(3), string("a"), true)

This comment has been minimized.

@axic

axic Oct 13, 2017

Member

The function prototype above defines uint for both y and z which are here string and true?

@axic

axic Oct 13, 2017

Member

The function prototype above defines uint for both y and z which are here string and true?

This comment has been minimized.

@axic

axic Oct 13, 2017

Member

I'd assume bytes3 makes the stack item to be 0x410000....

@axic

axic Oct 13, 2017

Member

I'd assume bytes3 makes the stack item to be 0x410000....

This comment has been minimized.

@chriseth

chriseth Oct 13, 2017

Contributor

I used uint because we know that it does not modify the data in any way.

@chriseth

chriseth Oct 13, 2017

Contributor

I used uint because we know that it does not modify the data in any way.

Whiskers templ(R"(
function <functionName>(headStart, dataEnd) -> <valueReturnParams> {
switch slt(sub(dataEnd, headStart), <minimumSize>) case 1 { revert(0, 0) }

This comment has been minimized.

@axic

axic Oct 13, 2017

Member

Do we have any kind of assertions that headStart < dataEnd? Otherwise this could overflow.

@axic

axic Oct 13, 2017

Member

Do we have any kind of assertions that headStart < dataEnd? Otherwise this could overflow.

This comment has been minimized.

@chriseth

chriseth Oct 13, 2017

Contributor

Note that this is slt, i.e. signed comparison, so this case is caught here.

@chriseth

chriseth Oct 13, 2017

Contributor

Note that this is slt, i.e. signed comparison, so this case is caught here.

if (_forUseOnStack)
{
return Whiskers(R"(
function <functionName>(offset, end) -> addr, function_selector {

This comment has been minimized.

@axic

axic Oct 13, 2017

Member

Which component validates that end >= offset + 32?

@axic

axic Oct 13, 2017

Member

Which component validates that end >= offset + 32?

This comment has been minimized.

@chriseth

chriseth Oct 13, 2017

Contributor

For static data, this check is done at the caller place. This is an efficiency measure.

@chriseth

chriseth Oct 13, 2017

Contributor

For static data, this check is done at the caller place. This is an efficiency measure.

return createFunction(functionName, [&]() {
Whiskers templ(
R"(
function <functionName>(offset, end) -> array {

This comment has been minimized.

@axic

axic Oct 13, 2017

Member

Again, validation that offset + 32 <= end is done outside of this?

@axic

axic Oct 13, 2017

Member

Again, validation that offset + 32 <= end is done outside of this?

This comment has been minimized.

@chriseth

chriseth Oct 13, 2017

Contributor

I think we should re-check that at the end, for some I think it only validates that offset <= end, although it should check that offset + 32 <= end.

@chriseth

chriseth Oct 13, 2017

Contributor

I think we should re-check that at the end, for some I think it only validates that offset <= end, although it should check that offset + 32 <= end.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Nov 16, 2017

Contributor

Closes #3049

Contributor

chriseth commented Nov 16, 2017

Closes #3049

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Nov 16, 2017

Contributor

@axic this is ready to be merged from my side.

Contributor

chriseth commented Nov 16, 2017

@axic this is ready to be merged from my side.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 22, 2017

Member

Can some of the commits be squashed? Many just say "decoder", "decoder fixes", "decoder tests" :)

Member

axic commented Nov 22, 2017

Can some of the commits be squashed? Many just say "decoder", "decoder fixes", "decoder tests" :)

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Nov 22, 2017

Member

Also need to be rebased and changelog updated.

Member

axic commented Nov 22, 2017

Also need to be rebased and changelog updated.

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Nov 22, 2017

Contributor

Rebased and changed changelog.

Contributor

chriseth commented Nov 22, 2017

Rebased and changed changelog.

@@ -369,7 +357,7 @@ BOOST_AUTO_TEST_CASE(external_function_cleanup)
)";
BOTH_ENCODERS(
compileAndRun(sourceCode);
callContractFunction("f(uint256)");
callContractFunction("f(uint256)", u256(0));

This comment has been minimized.

@axic

axic Nov 22, 2017

Member

Just remind me: does the newer decoder changes semantics and requires the minimum length of the encoding present as calldata?

Do we have a specific test for short data with the new decoder?

@axic

axic Nov 22, 2017

Member

Just remind me: does the newer decoder changes semantics and requires the minimum length of the encoding present as calldata?

Do we have a specific test for short data with the new decoder?

This comment has been minimized.

@chriseth

chriseth Nov 22, 2017

Contributor

Yes, the new decoder does not tolerate short data and there are multiple tests.

@chriseth

chriseth Nov 22, 2017

Contributor

Yes, the new decoder does not tolerate short data and there are multiple tests.

@axic axic added the nextrelease label Nov 22, 2017

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Nov 23, 2017

Contributor

Added short input tests for all types the old decoder can handle.

Contributor

chriseth commented Nov 23, 2017

Added short input tests for all types the old decoder can handle.

memPtr := mload(<freeMemoryPointer>)
let newFreePtr := add(memPtr, size)
// protect against overflow
switch or(gt(newFreePtr, 0xffffffffffffffff), lt(newFreePtr, memPtr)) case 1 { revert(0, 0) }

This comment has been minimized.

@axic

axic Nov 27, 2017

Member

Could be changed to if.

@axic

axic Nov 27, 2017

Member

Could be changed to if.

This comment has been minimized.

@chriseth

chriseth Nov 27, 2017

Contributor

I will check all switches later, I think we should merge this pr without that change.

@chriseth

chriseth Nov 27, 2017

Contributor

I will check all switches later, I think we should merge this pr without that change.

This comment has been minimized.

@axic

axic Nov 27, 2017

Member

Never mind, the only really safe ones without review are the those checking for case 0.

@axic

axic Nov 27, 2017

Member

Never mind, the only really safe ones without review are the those checking for case 0.

@axic

axic approved these changes Nov 27, 2017

@chriseth

This comment has been minimized.

Show comment
Hide comment
@chriseth

chriseth Nov 28, 2017

Contributor

Can this be merged?

Contributor

chriseth commented Nov 28, 2017

Can this be merged?

@axic axic merged commit 0759147 into develop Nov 29, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@axic axic deleted the structDecoder branch Nov 29, 2017

@axic axic removed in progress labels Nov 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment