Merge pull request #9173 from ethereum/fixBoundCalldata

Fix bound functions with calldata parameters.

Changelog.md

@@ -1,5 +1,8 @@
 ### 0.6.10 (unreleased)
 
+Important Bugfixes:
+ * Fixed a bug related to internal library functions with ``calldata`` parameters called via ``using for``.
+
 Language Features:
 
 Compiler Features:

docs/bugs.json

@@ -1,4 +1,12 @@
 [
+    {
+        "name": "UsingForCalldata",
+        "summary": "Function calls to internal library functions with calldata parameters called via ``using for`` can result in invalid data being read.",
+        "description": "Function calls to internal library functions using the ``using for`` mechanism copied all calldata parameters to memory first and passed them on like that, regardless of whether it was an internal or an external call. Due to that, the called function would receive a memory pointer that is interpreted as a calldata pointer. Since dynamically sized arrays are passed using two stack slots for calldata, but only one for memory, this can lead to stack corruption. An affected library call will consider the JUMPDEST to which it is supposed to return as part of its arguments and will instead jump out to whatever was on the stack before the call.",
+        "introduced": "0.6.9",
+        "fixed": "0.6.10",
+        "severity": "very low"
+    },
     {
         "name": "MissingEscapingInFormatting",
         "summary": "String literals containing double backslash characters passed directly to external or encoding function calls can lead to a different string being used when ABIEncoderV2 is enabled.",

docs/bugs_by_version.json

@@ -1165,7 +1165,9 @@
         "released": "2020-05-14"
     },
     "0.6.9": {
-        "bugs": [],
+        "bugs": [
+            "UsingForCalldata"
+        ],
         "released": "2020-06-04"
     }
 }

libsolidity/ast/Types.cpp

@@ -371,7 +371,8 @@ MemberList::MemberMap Type::boundFunctions(Type const& _type, ContractDefinition
 				seenFunctions.insert(function);
 				if (function->parameters().empty())
 					continue;
-				FunctionTypePointer fun = FunctionType(*function, FunctionType::Kind::External).asExternallyCallableFunction(true, true);
+				FunctionTypePointer fun =
+					dynamic_cast<FunctionType const&>(*function->typeViaContractName()).asBoundFunction();
 				if (_type.isImplicitlyConvertibleTo(*fun->selfType()))
 					members.emplace_back(function->name(), fun, function);
 			}
@@ -3453,11 +3454,32 @@ TypePointer FunctionType::copyAndSetCallOptions(bool _setGas, bool _setValue, bo
 	);
 }
 
-FunctionTypePointer FunctionType::asExternallyCallableFunction(bool _inLibrary, bool _bound) const
+FunctionTypePointer FunctionType::asBoundFunction() const
 {
-	if (_bound)
-		solAssert(!m_parameterTypes.empty(), "");
+	solAssert(!m_parameterTypes.empty(), "");
+	FunctionDefinition const* fun = dynamic_cast<FunctionDefinition const*>(m_declaration);
+	solAssert(fun && fun->libraryFunction(), "");
+	solAssert(!m_gasSet, "");
+	solAssert(!m_valueSet, "");
+	solAssert(!m_saltSet, "");
+	return TypeProvider::function(
+		m_parameterTypes,
+		m_returnParameterTypes,
+		m_parameterNames,
+		m_returnParameterNames,
+		m_kind,
+		m_arbitraryParameters,
+		m_stateMutability,
+		m_declaration,
+		m_gasSet,
+		m_valueSet,
+		m_saltSet,
+		true
+	);
+}
 
+FunctionTypePointer FunctionType::asExternallyCallableFunction(bool _inLibrary) const
+{
 	TypePointers parameterTypes;
 	for (auto const& t: m_parameterTypes)
 		if (TypeProvider::isReferenceWithLocation(t, DataLocation::CallData))
@@ -3480,10 +3502,8 @@ FunctionTypePointer FunctionType::asExternallyCallableFunction(bool _inLibrary,
 	if (_inLibrary)
 	{
 		solAssert(!!m_declaration, "Declaration has to be available.");
-		if (!m_declaration->isPublic())
-			kind = Kind::Internal; // will be inlined
-		else
-			kind = Kind::DelegateCall;
+		solAssert(m_declaration->isPublic(), "");
+		kind = Kind::DelegateCall;
 	}
 
 	return TypeProvider::function(
@@ -3498,7 +3518,7 @@ FunctionTypePointer FunctionType::asExternallyCallableFunction(bool _inLibrary,
 		m_gasSet,
 		m_valueSet,
 		m_saltSet,
-		_bound
+		m_bound
 	);
 }
 

libsolidity/ast/Types.h

@@ -1304,13 +1304,16 @@ public:
 	/// of the parameters to false.
 	TypePointer copyAndSetCallOptions(bool _setGas, bool _setValue, bool _setSalt) const;
 
+	/// @returns a copy of this function type with the `bound` flag set to true.
+	/// Should only be called on library functions.
+	FunctionTypePointer asBoundFunction() const;
+
 	/// @returns a copy of this function type where the location of reference types is changed
 	/// from CallData to Memory. This is the type that would be used when the function is
 	/// called externally, as opposed to the parameter types that are available inside the function body.
 	/// Also supports variants to be used for library or bound calls.
 	/// @param _inLibrary if true, uses DelegateCall as location.
-	/// @param _bound if true, the function type is set to be bound.
-	FunctionTypePointer asExternallyCallableFunction(bool _inLibrary, bool _bound = false) const;
+	FunctionTypePointer asExternallyCallableFunction(bool _inLibrary) const;
 
 protected:
 	std::vector<std::tuple<std::string, TypePointer>> makeStackItems() const override;

test/libsolidity/semanticTests/libraries/bound_returning_calldata.sol

@@ -0,0 +1,17 @@
+library D {
+    function f(bytes calldata _x) internal pure returns (bytes calldata) {
+        return _x;
+    }
+    function g(bytes calldata _x) internal pure returns (bytes memory) {
+        return _x;
+    }
+}
+
+contract C {
+    using D for bytes;
+    function f(bytes calldata _x) public pure returns (byte, byte) {
+        return (_x.f()[0], _x.g()[0]);
+    }
+}
+// ----
+// f(bytes): 0x20, 4, "abcd" -> 0x6100000000000000000000000000000000000000000000000000000000000000, 0x6100000000000000000000000000000000000000000000000000000000000000

test/libsolidity/semanticTests/libraries/bound_returning_calldata_external.sol

@@ -0,0 +1,20 @@
+library D {
+    function f(bytes calldata _x) public pure returns (bytes calldata) {
+        return _x;
+    }
+    function g(bytes calldata _x) public pure returns (bytes memory) {
+        return _x;
+    }
+}
+
+contract C {
+    using D for bytes;
+    function f(bytes calldata _x) public pure returns (byte, byte) {
+        return (_x.f()[0], _x.g()[0]);
+    }
+}
+// ====
+// EVMVersion: >homestead
+// ----
+// library: D
+// f(bytes): 0x20, 4, "abcd" -> 0x6100000000000000000000000000000000000000000000000000000000000000, 0x6100000000000000000000000000000000000000000000000000000000000000

test/libsolidity/semanticTests/libraries/bound_to_calldata.sol

@@ -0,0 +1,17 @@
+library D {
+    function f(bytes calldata _x) internal pure returns (byte) {
+        return _x[0];
+    }
+    function g(bytes memory _x) internal pure returns (byte) {
+        return _x[0];
+    }
+}
+
+contract C {
+    using D for bytes;
+    function f(bytes calldata _x) public pure returns (byte, byte) {
+        return (_x.f(), _x.g());
+    }
+}
+// ----
+// f(bytes): 0x20, 4, "abcd" -> 0x6100000000000000000000000000000000000000000000000000000000000000, 0x6100000000000000000000000000000000000000000000000000000000000000

test/libsolidity/semanticTests/libraries/bound_to_calldata_external.sol

@@ -0,0 +1,20 @@
+library D {
+    function f(bytes calldata _x) public pure returns (byte) {
+        return _x[0];
+    }
+    function g(bytes memory _x) public pure returns (byte) {
+        return _x[0];
+    }
+}
+
+contract C {
+    using D for bytes;
+    function f(bytes calldata _x) public pure returns (byte, byte) {
+        return (_x.f(), _x.g());
+    }
+}
+// ====
+// EVMVersion: >homestead
+// ----
+// library: D
+// f(bytes): 0x20, 4, "abcd" -> 0x6100000000000000000000000000000000000000000000000000000000000000, 0x6100000000000000000000000000000000000000000000000000000000000000

test/libsolidity/syntaxTests/bound/bound_calldata.sol

@@ -0,0 +1,9 @@
+library D { function f(bytes calldata) internal pure {} }
+contract C {
+	using D for bytes;
+	function f(bytes memory _x) public pure {
+		_x.f();
+	}
+}
+// ----
+// TypeError: (136-140): Member "f" not found or not visible after argument-dependent lookup in bytes memory.