Use returndatacopy for retrieving dynamically sized outputs. #3308

chriseth · 2017-12-12T08:28:49Z

Fixes #3270
Fixes #2964
Fixes #3273
Fixes #3516

add out of bounds checks for the old decoder, but only use it for decoding dynamic return data
use evm target selection
even if we use the current EVM, do not use the new abi decoder by default
add a test case for Static memory load of more than 32 bytes requested #3516

Depends on #3569

chriseth · 2017-12-13T16:32:15Z

This is ready for review.

Decisions to be taken:

do we want to enable this by default - some VMs might not have returndatacopy?
this might lead to problems when the returned data invalid data pointers in the ABI encoding (it still uses the old decoder unless the new is active). It is worse than with calldata because everything will reside in memory and we do not have zero-extension there because we need to copy from memory to memory.

chriseth · 2017-12-13T16:46:47Z

libsolidity/codegen/ExpressionCompiler.cpp

 		else
-			m_context << Instruction::POP;
+		{
+			utils().fetchFreeMemoryPointer();


We should add a size check here, too.

AnthonyAkentiev · 2017-12-19T19:08:19Z

Hi guys. Is it still not merged?

chriseth · 2017-12-20T09:25:01Z

@AnthonyAkentiev someone is impatient there :)

There are still security issues to be discussed, see above.

chriseth · 2018-01-25T16:11:08Z

The first comment above is solved (pragma target byzantium), but for the second comment, we probably do not want this to run with the old ABI decoder.

axic · 2018-01-25T16:18:53Z

Perhaps we could remove the InaccessibleDynamicType in a separate pull request because it should already be covered with an assertion. (Need to verify that.)

chriseth · 2018-01-25T16:19:04Z

Use the old decoder only on statically-sized data or on the statically-sized head of dynamically-sized data. This means returndatacopy is not used with the old decoder.

axic · 2018-01-25T16:27:31Z

libsolidity/ast/Types.cpp

-		// Alter dynamic types to be non-accessible.
-		for (auto& param: returnParameterTypes)
-			if (param->isDynamicallySized())
-				param = make_shared<InaccessibleDynamicType>();


We could move this check into the TypeChecker around line 1500.

... only if the pragma specifies that we are using the old encoder.

axic · 2018-02-14T13:20:45Z

Should use the new evmTarget from #1117.

chriseth · 2018-02-23T18:44:26Z

This is finished, but I still have to untangle the various commits.

chriseth · 2018-02-26T18:07:00Z

Commits untangled.

chriseth · 2018-03-05T11:27:17Z

Rebased.

chriseth · 2018-03-05T14:45:30Z

Ready for review.

axic · 2018-03-07T15:16:31Z

Ah, the problem is Github lists commits in the wrong order (by timestamp), but merge wise Move the old ABI decoder code comes before Simple size check for old ABI decoder..

chriseth · 2018-03-07T15:47:37Z

Oh wow that is interesting. I last rebased two days ago.

axic · 2018-03-07T16:07:52Z

libsolidity/codegen/CompilerUtils.cpp

+						// Check that the data pointer is valid and that length times
+						// item size is still inside the range.
+						Whiskers templ(R"({
+							if gt(ptr, 0x100000000) { revert(0, 0) }


Is this a random constant (we assume it is not possible to transfer that much due to block gas limits)?

Do you want something to be changed here?

axic · 2018-03-20T15:24:30Z

libsolidity/ast/Types.cpp

+
+	if (m_kind == Kind::External || m_kind == Kind::CallCode || m_kind == Kind::DelegateCall)
+		for (auto& param: returnParameterTypes)
+			if (param->isDynamicallySized() && !param->dataStoredIn(DataLocation::Storage))


Can an external call return a storage pointer?

Yes, but only via delegatecall.

I.e. only a call to a library function.

axic · 2018-03-20T15:29:24Z

libsolidity/codegen/ExpressionCompiler.cpp

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


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

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

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

Let's postpone it. Created issue #3769.

axic · 2018-03-20T15:34:28Z

libsolidity/codegen/ExpressionCompiler.cpp

 		else
-			m_context << Instruction::POP;
+			solAssert(retSize > 0, "");
+		m_context << (haveReturndatacopy ? eth::AssemblyItem(Instruction::RETURNDATASIZE) : u256(retSize));


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

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

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.

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

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.

axic · 2018-03-21T14:53:55Z

libsolidity/codegen/CompilerUtils.h

+	/// From memory if @a _fromMemory is true, otherwise from call data.
+	/// Calls revert if @a _revertOnOutOfBounds is true and the supplied size is shorter
+	/// than the static data requirements or if dynamic data pointers reach outside of the
+	/// area. Also has a hard cap of 0x100000000 for any given length/offset field.


@chriseth added this comment also

LianaHus added the in progress label Dec 12, 2017

chriseth force-pushed the usereturndatacopy branch from a023ade to aab2ac1 Compare December 13, 2017 16:27

chriseth changed the title ~~[WIP] Use returndatacopy for retrieving dynamically sized outputs.~~ Use returndatacopy for retrieving dynamically sized outputs. Dec 13, 2017

chriseth requested a review from axic December 13, 2017 16:30

chriseth commented Dec 13, 2017

View reviewed changes

chriseth force-pushed the usereturndatacopy branch from 431379d to 384115e Compare December 14, 2017 15:39

axic reviewed Jan 25, 2018

View reviewed changes

axic added the nextrelease label Feb 14, 2018

chriseth force-pushed the usereturndatacopy branch 3 times, most recently from 352cf60 to 5ecc33c Compare February 21, 2018 15:10

chriseth force-pushed the usereturndatacopy branch 2 times, most recently from d938d19 to 693858d Compare February 23, 2018 18:39

chriseth force-pushed the usereturndatacopy branch from 693858d to 6f26336 Compare February 26, 2018 18:05

chriseth force-pushed the usereturndatacopy branch from 6f26336 to 645098d Compare March 5, 2018 11:27

chriseth force-pushed the usereturndatacopy branch from 645098d to 22dc19d Compare March 5, 2018 11:32

axic reviewed Mar 7, 2018

View reviewed changes

chriseth force-pushed the usereturndatacopy branch from 8a98324 to 7adadfa Compare March 9, 2018 13:20

chriseth mentioned this pull request Mar 12, 2018

Static memory load of more than 32 bytes requested #3516

Closed

Move the old ABI decoder code.

2cdf44f

axic reviewed Mar 20, 2018

View reviewed changes

This was referenced Mar 20, 2018

Always use returndatacopy if supported #3767

Closed

Consider adding calldataEncodedSize/isDynamicallyEncoded to TupleType #3769

Closed

axic force-pushed the usereturndatacopy branch 2 times, most recently from 4683f91 to 7c3de57 Compare March 21, 2018 14:51

chriseth added 5 commits March 21, 2018 14:53

Simple size check for old ABI decoder.

32c94f5

Move dynamic type removal out of the type system.

cc2f71e

Decode dynamic data.

c2709a2

Tests for returning dynamic data.

cc0f702

Changelog entry.

c7860a0

axic force-pushed the usereturndatacopy branch from 7c3de57 to c7860a0 Compare March 21, 2018 14:53

axic reviewed Mar 21, 2018

View reviewed changes

axic approved these changes Mar 21, 2018

View reviewed changes

chriseth merged commit 85b0cfe into develop Mar 21, 2018

axic deleted the usereturndatacopy branch March 21, 2018 17:13

abdulowork mentioned this pull request Apr 4, 2018

Missing contract call response interpretation zeriontech/Web3Swift#97

Closed

spalladino mentioned this pull request Apr 12, 2018

Store project name in ProjectController as a string instead of its hash zeppelinos/zos-lib#39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use returndatacopy for retrieving dynamically sized outputs. #3308

Use returndatacopy for retrieving dynamically sized outputs. #3308

chriseth commented Dec 12, 2017 •

edited

Loading

chriseth commented Dec 13, 2017 •

edited

Loading

chriseth Dec 13, 2017

AnthonyAkentiev commented Dec 19, 2017

chriseth commented Dec 20, 2017

chriseth commented Jan 25, 2018

axic commented Jan 25, 2018

chriseth commented Jan 25, 2018

axic Jan 25, 2018

chriseth Jan 25, 2018

axic commented Feb 14, 2018

chriseth commented Feb 23, 2018

chriseth commented Feb 26, 2018

chriseth commented Mar 5, 2018

chriseth commented Mar 5, 2018

axic commented Mar 7, 2018 •

edited

Loading

chriseth commented Mar 7, 2018

axic Mar 7, 2018

chriseth Mar 7, 2018

chriseth Mar 9, 2018

axic Mar 20, 2018

chriseth Mar 20, 2018

chriseth Mar 20, 2018

axic Mar 20, 2018

axic Mar 20, 2018 •

edited

Loading

chriseth Mar 20, 2018

axic Mar 21, 2018

axic Mar 20, 2018

axic Mar 20, 2018

axic Mar 20, 2018

chriseth Mar 20, 2018

axic Mar 20, 2018 •

edited

Loading

axic Mar 21, 2018

Use returndatacopy for retrieving dynamically sized outputs. #3308

Use returndatacopy for retrieving dynamically sized outputs. #3308

Conversation

chriseth commented Dec 12, 2017 • edited Loading

chriseth commented Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

AnthonyAkentiev commented Dec 19, 2017

chriseth commented Dec 20, 2017

chriseth commented Jan 25, 2018

axic commented Jan 25, 2018

chriseth commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Feb 14, 2018

chriseth commented Feb 23, 2018

chriseth commented Feb 26, 2018

chriseth commented Mar 5, 2018

chriseth commented Mar 5, 2018

axic commented Mar 7, 2018 • edited Loading

chriseth commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Dec 12, 2017 •

edited

Loading

chriseth commented Dec 13, 2017 •

edited

Loading

axic commented Mar 7, 2018 •

edited

Loading

axic Mar 20, 2018 •

edited

Loading

axic Mar 20, 2018 •

edited

Loading