Skip to content

Use returndatacopy for retrieving dynamically sized outputs. #3308

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.

Sign up for GitHub

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

Merged
chriseth merged 6 commits into from
Mar 21, 2018
Merged

Use returndatacopy for retrieving dynamically sized outputs. #3308

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2cdf44f
Move the old ABI decoder code.
chriseth Feb 20, 2018
32c94f5
Simple size check for old ABI decoder.
chriseth Dec 13, 2017
cc2f71e
Move dynamic type removal out of the type system.
chriseth Feb 20, 2018
c2709a2
Decode dynamic data.
chriseth Feb 21, 2018
cc0f702
Tests for returning dynamic data.
chriseth Feb 21, 2018
c7860a0
Changelog entry.
chriseth Mar 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Decode dynamic data.
  • Loading branch information
@chriseth @axic
chriseth authored and axic committed Mar 21, 2018
commit c2709a2d8e53155513fa8002a564e434fce68c68
2 changes: 1 addition & 1 deletion libsolidity/analysis/TypeChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1551,7 +1551,7 @@ bool TypeChecker::visit(FunctionCall const& _functionCall)
_functionCall.expression().annotation().isPure &&
functionType->isPure();

bool allowDynamicTypes = false; // @TODO
bool allowDynamicTypes = m_evmVersion.supportsReturndata();
if (!functionType)
{
m_errorReporter.typeError(_functionCall.location(), "Type is not callable");
Expand Down
3 changes: 3 additions & 0 deletions libsolidity/codegen/ABIFunctions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,9 @@ string ABIFunctions::cleanupFunction(Type const& _type, bool _revertOnFailure)
templ("body", w.render());
break;
}
case Type::Category::InaccessibleDynamic:
templ("body", "cleaned := 0");
break;
default:
solAssert(false, "Cleanup of type " + _type.identifier() + " requested.");
}
Expand Down
4 changes: 2 additions & 2 deletions libsolidity/codegen/CompilerUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ void CompilerUtils::abiDecode(TypePointers const& _typeParameters, bool _fromMem
if (type->category() == Type::Category::Array)
{
auto const& arrayType = dynamic_cast<ArrayType const&>(*type);
solUnimplementedAssert(!arrayType.baseType()->isDynamicallySized(), "Nested arrays not yet implemented.");
solUnimplementedAssert(!arrayType.baseType()->isDynamicallyEncoded(), "Nested arrays not yet implemented.");
if (_fromMemory)
{
solUnimplementedAssert(
Expand Down Expand Up @@ -308,7 +308,7 @@ void CompilerUtils::abiDecode(TypePointers const& _typeParameters, bool _fromMem
}
else
{
solAssert(!type->isDynamicallySized(), "Unknown dynamically sized type: " + type->toString());
solAssert(!type->isDynamicallyEncoded(), "Unknown dynamically sized type: " + type->toString());
loadFromMemoryDynamic(*type, !_fromMemory, true);
// stack: v1 v2 ... v(k-1) input_end base_offset v(k) mem_offset
moveToStackTop(1, type->sizeOnStack());
Expand Down
57 changes: 42 additions & 15 deletions libsolidity/codegen/ExpressionCompiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1618,22 +1618,27 @@ void ExpressionCompiler::appendExternalFunctionCall(
m_context.experimentalFeatureActive(ExperimentalFeature::V050) &&
m_context.evmVersion().hasStaticCall();

bool allowDynamicTypes = false; // @TODO
bool haveReturndatacopy = m_context.evmVersion().supportsReturndata();
unsigned retSize = 0;
TypePointers returnTypes;
if (returnSuccessCondition)
retSize = 0; // return value actually is success condition
else if (allowDynamicTypes)
else if (haveReturndatacopy)
returnTypes = _functionType.returnParameterTypes();
else
{
returnTypes = _functionType.returnParameterTypesWithoutDynamicTypes();
for (auto const& retType: returnTypes)

bool dynamicReturnSize = false;
for (auto const& retType: returnTypes)
if (retType->isDynamicallyEncoded())
Copy link
Contributor

@axic axic Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have calldataEncodedSize and isDynamicallyEncoded on the TupleType, do we need to iterate?

Copy link
Contributor

@axic axic Mar 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently no, but would it make sense to do so?

Copy link
Contributor Author

@chriseth chriseth Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuples are not fully-fledged types in Solidity, so I'm not sure which implications this would have.

Copy link
Contributor

@axic axic Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's postpone it. Created issue #3769.

{
solAssert(!retType->isDynamicallySized(), "Unable to return dynamic type from external call.");
retSize += retType->calldataEncodedSize();
solAssert(haveReturndatacopy, "");
dynamicReturnSize = true;
retSize = 0;
break;
}
}
else
retSize += retType->calldataEncodedSize();

// Evaluate arguments.
TypePointers argumentTypes;
Expand Down Expand Up @@ -1834,17 +1839,39 @@ void ExpressionCompiler::appendExternalFunctionCall(
else if (!returnTypes.empty())
{
utils().fetchFreeMemoryPointer();
bool memoryNeeded = false;
for (auto const& retType: returnTypes)
// Stack: return_data_start

// The old decoder did not allocate any memory (i.e. did not touch the free
// memory pointer), but kept references to the return data for
// (statically-sized) arrays
bool needToUpdateFreeMemoryPtr = false;
if (dynamicReturnSize || m_context.experimentalFeatureActive(ExperimentalFeature::ABIEncoderV2))
needToUpdateFreeMemoryPtr = true;
else
for (auto const& retType: returnTypes)
if (dynamic_cast<ReferenceType const*>(retType.get()))
needToUpdateFreeMemoryPtr = true;

// Stack: return_data_start
if (dynamicReturnSize)
{
utils().loadFromMemoryDynamic(*retType, false, true, true);
if (dynamic_cast<ReferenceType const*>(retType.get()))
memoryNeeded = true;
solAssert(haveReturndatacopy, "");
m_context.appendInlineAssembly("{ returndatacopy(return_data_start, 0, returndatasize()) }", {"return_data_start"});
}
if (memoryNeeded)
utils().storeFreeMemoryPointer();
else
m_context << Instruction::POP;
solAssert(retSize > 0, "");
// Always use the actual return length, and not our calculated expected length, if returndatacopy is supported.
// This ensures it can catch badly formatted input from external calls.
m_context << (haveReturndatacopy ? eth::AssemblyItem(Instruction::RETURNDATASIZE) : u256(retSize));
Copy link
Contributor

@axic axic Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just move this into the two parts of the if?

Copy link
Contributor

@axic axic Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is a separate step to always use returndatacopy and not allocate the memory on call.

Copy link
Contributor

@axic axic Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment here stating:

Always use the actual return length, and not our calculated expected length, if returndatacopy is supported. This ensures it can catch badly formatted input from external calls.

Copy link
Contributor Author

@chriseth chriseth Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was the idea, always use returndatacopy to detect badly formatted return data. Yes, please add the comment.

Copy link
Contributor

@axic axic Mar 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think this may warrant its own changelog entry as we're changing behaviour.

Compiling for byzantium (which is the default) may result in a new contract not working with an old one, if we had a case of invalid short encoding, or if the other one isn't Solidity and doesn't encode properly.

Unlikely, but still.

// Stack: return_data_start return_data_size
if (needToUpdateFreeMemoryPtr)
m_context.appendInlineAssembly(R"({
// round size to the next multiple of 32
let newMem := add(start, and(add(size, 0x1f), not(0x1f)))
mstore(0x40, newMem)
})", {"start", "size"});

utils().abiDecode(returnTypes, true, true);
}
}

Expand Down
5 changes: 5 additions & 0 deletions test/libsolidity/SolidityNameAndTypeResolution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3372,6 +3372,11 @@ BOOST_AUTO_TEST_CASE(dynamic_return_types_not_possible)
}
}
)";
m_compiler.setEVMVersion(EVMVersion{});
CHECK_WARNING(sourceCode, "Use of the \"var\" keyword is deprecated");
m_compiler.setEVMVersion(*EVMVersion::fromString("byzantium"));
CHECK_WARNING(sourceCode, "Use of the \"var\" keyword is deprecated");
m_compiler.setEVMVersion(*EVMVersion::fromString("homestead"));
CHECK_ERROR(sourceCode, TypeError, "Explicit type conversion not allowed from \"inaccessible dynamic type\" to \"bytes storage pointer\".");
}

Expand Down