Merge pull request #1334 from ethereum/enum_conversion

check enum value range during conversion

Changelog.md

@@ -3,6 +3,12 @@
 Features:
  * Do-while loops: support for a C-style do{<block>}while(<expr>); control structure
  * Type checker: now more eagerly searches for a common type of an inline array with mixed types
+ * Code generator: generates a runtime error when an out-of-range value is converted into an enum type.
+
+Bugfixes:
+
+ * Parser: disallow empty enum definitions.
+ * Type checker: disallow conversion between different enum types.
 
 ### 0.4.4 (2016-10-31)
 

docs/control-structures.rst

@@ -329,9 +329,10 @@ Currently, there are situations, where exceptions happen automatically in Solidi
 3. If you call a function via a message call but it does not finish properly (i.e. it runs out of gas, has no matching function, or throws an exception itself), except when a low level operation ``call``, ``send``, ``delegatecall`` or ``callcode`` is used.  The low level operations never throw exceptions but indicate failures by returning ``false``.
 4. If you create a contract using the ``new`` keyword but the contract creation does not finish properly (see above for the definition of "not finish properly").
 5. If you divide or modulo by zero (e.g. ``5 / 0`` or ``23 % 0``).
-6. If you perform an external function call targeting a contract that contains no code.
-7. If your contract receives Ether via a public function without ``payable`` modifier (including the constructor and the fallback function).
-8. If your contract receives Ether via a public accessor function.
+6. If you convert a value too big or negative into an enum type.
+7. If you perform an external function call targeting a contract that contains no code.
+8. If your contract receives Ether via a public function without ``payable`` modifier (including the constructor and the fallback function).
+9. If your contract receives Ether via a public accessor function.
 
 Internally, Solidity performs an "invalid jump" when an exception is thrown and thus causes the EVM to revert all changes made to the state. The reason for this is that there is no safe way to continue execution, because an expected effect did not occur. Because we want to retain the atomicity of transactions, the safest thing to do is to revert all changes and make the whole transaction (or at least call) without effect.
 

docs/types.rst

@@ -237,7 +237,8 @@ Enums
 =====
 
 Enums are one way to create a user-defined type in Solidity. They are explicitly convertible
-to and from all integer types but implicit conversion is not allowed.
+to and from all integer types but implicit conversion is not allowed.  The explicit conversions
+check the value ranges at runtime and a failure causes an exception.  Enums needs at least one member.
 
 ::
 

libsolidity/ast/Types.cpp

@@ -1561,7 +1561,7 @@ bool EnumType::operator==(Type const& _other) const
 
 unsigned EnumType::storageBytes() const
 {
-	size_t elements = m_enum.members().size();
+	size_t elements = numberOfMembers();
 	if (elements <= 1)
 		return 1;
 	else
@@ -1578,9 +1578,14 @@ string EnumType::canonicalName(bool) const
 	return m_enum.annotation().canonicalName;
 }
 
+size_t EnumType::numberOfMembers() const
+{
+	return m_enum.members().size();
+};
+
 bool EnumType::isExplicitlyConvertibleTo(Type const& _convertTo) const
 {
-	return _convertTo.category() == category() || _convertTo.category() == Category::Integer;
+	return _convertTo == *this || _convertTo.category() == Category::Integer;
 }
 
 unsigned EnumType::memberValue(ASTString const& _member) const

libsolidity/ast/Types.h

@@ -738,6 +738,7 @@ public:
 	EnumDefinition const& enumDefinition() const { return m_enum; }
 	/// @returns the value that the string has in the Enum
 	unsigned int memberValue(ASTString const& _member) const;
+	size_t numberOfMembers() const;
 
 private:
 	EnumDefinition const& m_enum;

libsolidity/codegen/CompilerUtils.cpp

@@ -315,6 +315,8 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 	Type::Category stackTypeCategory = _typeOnStack.category();
 	Type::Category targetTypeCategory = _targetType.category();
 
+	bool enumOverflowCheckPending = (targetTypeCategory == Type::Category::Enum);
+
 	switch (stackTypeCategory)
 	{
 	case Type::Category::FixedBytes:
@@ -348,7 +350,15 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 	}
 		break;
 	case Type::Category::Enum:
-		solAssert(targetTypeCategory == Type::Category::Integer || targetTypeCategory == Type::Category::Enum, "");
+		solAssert(_targetType == _typeOnStack || targetTypeCategory == Type::Category::Integer, "");
+		if (enumOverflowCheckPending)
+		{
+			EnumType const& enumType = dynamic_cast<decltype(enumType)>(_targetType);
+			solAssert(enumType.numberOfMembers() > 0, "empty enum should have caused a parser error.");
+			m_context << u256(enumType.numberOfMembers() - 1) << Instruction::DUP2 << Instruction::GT;
+			m_context.appendConditionalJumpTo(m_context.errorTag());
+			enumOverflowCheckPending = false;
+		}
 		break;
 	case Type::Category::FixedPoint:
 		solAssert(false, "Not yet implemented - FixedPointType.");
@@ -372,6 +382,11 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 			solAssert(_typeOnStack.mobileType(), "");
 			// just clean
 			convertType(_typeOnStack, *_typeOnStack.mobileType(), true);
+			EnumType const& enumType = dynamic_cast<decltype(enumType)>(_targetType);
+			solAssert(enumType.numberOfMembers() > 0, "empty enum should have caused a parser error.");
+			m_context << u256(enumType.numberOfMembers() - 1) << Instruction::DUP2 << Instruction::GT;
+			m_context.appendConditionalJumpTo(m_context.errorTag());
+			enumOverflowCheckPending = false;
 		}
 		else if (targetTypeCategory == Type::Category::FixedPoint)
 		{
@@ -656,6 +671,8 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp
 		solAssert(_typeOnStack == _targetType, "Invalid type conversion requested.");
 		break;
 	}
+
+	solAssert(!enumOverflowCheckPending, "enum overflow checking missing.");
 }
 
 void CompilerUtils::pushZeroValue(Type const& _type)

libsolidity/parsing/Parser.cpp

@@ -406,6 +406,8 @@ ASTPointer<EnumDefinition> Parser::parseEnumDefinition()
 		if (m_scanner->currentToken() != Token::Identifier)
 			fatalParserError(string("Expected Identifier after ','"));
 	}
+	if (members.size() == 0)
+		parserError({"enum with no members is not allowed."});
 
 	nodeFactory.markEndPosition();
 	expectToken(Token::RBrace);

test/libsolidity/ASTJSON.cpp

@@ -94,20 +94,6 @@ BOOST_AUTO_TEST_CASE(using_for_directive)
 	BOOST_CHECK_EQUAL(usingFor["children"][1]["attributes"]["name"], "uint");    
 }
 
-BOOST_AUTO_TEST_CASE(enum_definition)
-{
-	CompilerStack c;
-	c.addSource("a", "contract C { enum E {} }");
-	c.parse();
-	map<string, unsigned> sourceIndices;
-	sourceIndices["a"] = 1;
-	Json::Value astJson = ASTJsonConverter(c.ast("a"), sourceIndices).json();
-	Json::Value enumDefinition = astJson["children"][0]["children"][0];
-	BOOST_CHECK_EQUAL(enumDefinition["name"], "EnumDefinition");
-	BOOST_CHECK_EQUAL(enumDefinition["attributes"]["name"], "E");
-	BOOST_CHECK_EQUAL(enumDefinition["src"], "13:9:1");
-}
-
 BOOST_AUTO_TEST_CASE(enum_value)
 {
 	CompilerStack c;

test/libsolidity/SolidityEndToEndTest.cpp

@@ -3342,6 +3342,42 @@ BOOST_AUTO_TEST_CASE(using_enums)
 	BOOST_CHECK(callContractFunction("getChoice()") == encodeArgs(2));
 }
 
+BOOST_AUTO_TEST_CASE(enum_explicit_overflow)
+{
+	char const* sourceCode = R"(
+			contract test {
+				enum ActionChoices { GoLeft, GoRight, GoStraight }
+				function test()
+				{
+				}
+				function getChoiceExp(uint x) returns (uint d)
+				{
+					choice = ActionChoices(x);
+					d = uint256(choice);
+				}
+				function getChoiceFromSigned(int x) returns (uint d)
+				{
+					choice = ActionChoices(x);
+					d = uint256(choice);
+				}
+				function getChoiceFromNegativeLiteral() returns (uint d)
+				{
+					choice = ActionChoices(-1);
+					d = uint256(choice);
+				}
+				ActionChoices choice;
+			}
+	)";
+	compileAndRun(sourceCode);
+	// These should throw
+	BOOST_CHECK(callContractFunction("getChoiceExp(uint256)", 3) == encodeArgs());
+	BOOST_CHECK(callContractFunction("getChoiceFromSigned(int256)", -1) == encodeArgs());
+	BOOST_CHECK(callContractFunction("getChoiceFromNegativeLiteral()") == encodeArgs());
+	// These should work
+	BOOST_CHECK(callContractFunction("getChoiceExp(uint256)", 2) == encodeArgs(2));
+	BOOST_CHECK(callContractFunction("getChoiceExp(uint256)", 0) == encodeArgs(0));
+}
+
 BOOST_AUTO_TEST_CASE(using_contract_enums_with_explicit_contract_name)
 {
 	char const* sourceCode = R"(

test/libsolidity/SolidityNameAndTypeResolution.cpp

@@ -1535,6 +1535,21 @@ BOOST_AUTO_TEST_CASE(enum_implicit_conversion_is_not_okay)
 	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
 }
 
+BOOST_AUTO_TEST_CASE(enum_to_enum_conversion_is_not_okay)
+{
+	char const* text = R"(
+		contract test {
+			enum Paper { Up, Down, Left, Right }
+			enum Ground { North, South, West, East }
+			function test()
+			{
+				Ground(Paper.Up);
+			}
+		}
+	)";
+	BOOST_CHECK(expectError(text) == Error::Type::TypeError);
+}
+
 BOOST_AUTO_TEST_CASE(enum_duplicate_values)
 {
 	char const* text = R"(

test/libsolidity/SolidityParser.cpp

@@ -824,7 +824,7 @@ BOOST_AUTO_TEST_CASE(empty_enum_declaration)
 		contract c {
 			enum foo { }
 		})";
-	BOOST_CHECK(successParse(text));
+	BOOST_CHECK(!successParse(text));
 }
 
 BOOST_AUTO_TEST_CASE(malformed_enum_declaration)