From 831ed083875b73faf91f9d3335e2599540366712 Mon Sep 17 00:00:00 2001 From: chriseth Date: Wed, 24 May 2017 11:47:43 +0200 Subject: [PATCH] Change invalid opcode to revert for input validation. --- Changelog.md | 1 + libsolidity/codegen/CompilerContext.cpp | 14 ++++++++++++++ libsolidity/codegen/CompilerContext.h | 10 +++++++--- libsolidity/codegen/CompilerUtils.cpp | 15 ++++++++++++--- libsolidity/codegen/CompilerUtils.h | 10 +++++++++- libsolidity/codegen/ContractCompiler.cpp | 9 ++++----- libsolidity/codegen/ExpressionCompiler.cpp | 14 ++++++-------- 7 files changed, 53 insertions(+), 20 deletions(-) diff --git a/Changelog.md b/Changelog.md index ee952a37a..6e15c9db4 100644 --- a/Changelog.md +++ b/Changelog.md @@ -14,6 +14,7 @@ Features: * Code Generator: Added the Whiskers template system. Bugfixes: + * Code generator: Use ``REVERT`` instead of ``INVALID`` for generated input validation routines. * Type Checker: Fix address literals not being treated as compile-time constants. * Type Checker: Make UTF8-validation a bit more sloppy to include more valid sequences. * Fixed crash concerning non-callable types. diff --git a/libsolidity/codegen/CompilerContext.cpp b/libsolidity/codegen/CompilerContext.cpp index 9d0d6d37a..1937b5293 100644 --- a/libsolidity/codegen/CompilerContext.cpp +++ b/libsolidity/codegen/CompilerContext.cpp @@ -244,6 +244,20 @@ CompilerContext& CompilerContext::appendConditionalInvalid() return *this; } +CompilerContext& CompilerContext::appendRevert() +{ + return *this << u256(0) << u256(0) << Instruction::REVERT; +} + +CompilerContext& CompilerContext::appendConditionalRevert() +{ + *this << Instruction::ISZERO; + eth::AssemblyItem afterTag = appendConditionalJump(); + appendRevert(); + *this << afterTag; + return *this; +} + void CompilerContext::resetVisitedNodes(ASTNode const* _node) { stack newStack; diff --git a/libsolidity/codegen/CompilerContext.h b/libsolidity/codegen/CompilerContext.h index 030b35a6a..1968c1e1c 100644 --- a/libsolidity/codegen/CompilerContext.h +++ b/libsolidity/codegen/CompilerContext.h @@ -136,11 +136,15 @@ public: /// Appends a JUMP to a new tag and @returns the tag eth::AssemblyItem appendJumpToNew() { return m_asm->appendJump().tag(); } /// Appends a JUMP to a tag already on the stack - CompilerContext& appendJump(eth::AssemblyItem::JumpType _jumpType = eth::AssemblyItem::JumpType::Ordinary); + CompilerContext& appendJump(eth::AssemblyItem::JumpType _jumpType = eth::AssemblyItem::JumpType::Ordinary); /// Appends an INVALID instruction - CompilerContext& appendInvalid(); + CompilerContext& appendInvalid(); /// Appends a conditional INVALID instruction - CompilerContext& appendConditionalInvalid(); + CompilerContext& appendConditionalInvalid(); + /// Appends a REVERT(0, 0) call + CompilerContext& appendRevert(); + /// Appends a conditional REVERT(0, 0) call + CompilerContext& appendConditionalRevert(); /// Appends a JUMP to a specific tag CompilerContext& appendJumpTo(eth::AssemblyItem const& _tag) { m_asm->appendJump(_tag); return *this; } /// Appends pushing of a new tag and @returns the new tag. diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp index bfe72961e..7fed19751 100644 --- a/libsolidity/codegen/CompilerUtils.cpp +++ b/libsolidity/codegen/CompilerUtils.cpp @@ -392,7 +392,13 @@ void CompilerUtils::pushCombinedFunctionEntryLabel(Declaration const& _function) Instruction::OR; } -void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetType, bool _cleanupNeeded, bool _chopSignBits) +void CompilerUtils::convertType( + Type const& _typeOnStack, + Type const& _targetType, + bool _cleanupNeeded, + bool _chopSignBits, + bool _asPartOfArgumentDecoding +) { // For a type extension, we need to remove all higher-order bits that we might have ignored in // previous operations. @@ -450,7 +456,10 @@ void CompilerUtils::convertType(Type const& _typeOnStack, Type const& _targetTyp EnumType const& enumType = dynamic_cast(_typeOnStack); solAssert(enumType.numberOfMembers() > 0, "empty enum should have caused a parser error."); m_context << u256(enumType.numberOfMembers() - 1) << Instruction::DUP2 << Instruction::GT; - m_context.appendConditionalInvalid(); + if (_asPartOfArgumentDecoding) + m_context.appendConditionalRevert(); + else + m_context.appendConditionalInvalid(); enumOverflowCheckPending = false; } break; @@ -985,7 +994,7 @@ unsigned CompilerUtils::loadFromMemoryHelper(Type const& _type, bool _fromCallda m_context << shiftFactor << Instruction::MUL; } if (_fromCalldata) - convertType(_type, _type, true); + convertType(_type, _type, true, false, true); return numBytes; } diff --git a/libsolidity/codegen/CompilerUtils.h b/libsolidity/codegen/CompilerUtils.h index a88951bcb..caf2cdc25 100644 --- a/libsolidity/codegen/CompilerUtils.h +++ b/libsolidity/codegen/CompilerUtils.h @@ -135,7 +135,15 @@ public: /// If @a _cleanupNeeded, high order bits cleanup is also done if no type conversion would be /// necessary. /// If @a _chopSignBits, the function resets the signed bits out of the width of the signed integer. - void convertType(Type const& _typeOnStack, Type const& _targetType, bool _cleanupNeeded = false, bool _chopSignBits = false); + /// If @a _asPartOfArgumentDecoding is true, failed conversions are flagged via REVERT, + /// otherwise they are flagged with INVALID. + void convertType( + Type const& _typeOnStack, + Type const& _targetType, + bool _cleanupNeeded = false, + bool _chopSignBits = false, + bool _asPartOfArgumentDecoding = false + ); /// Creates a zero-value for the given type and puts it onto the stack. This might allocate /// memory for memory references. diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index dc0906345..ace82ad4e 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/libsolidity/codegen/ContractCompiler.cpp @@ -111,7 +111,7 @@ void ContractCompiler::appendCallValueCheck() { // Throw if function is not payable but call contained ether. m_context << Instruction::CALLVALUE; - m_context.appendConditionalInvalid(); + m_context.appendConditionalRevert(); } void ContractCompiler::appendInitAndConstructorCode(ContractDefinition const& _contract) @@ -276,7 +276,7 @@ void ContractCompiler::appendFunctionSelector(ContractDefinition const& _contrac appendReturnValuePacker(FunctionType(*fallback).returnParameterTypes(), _contract.isLibrary()); } else - m_context.appendInvalid(); + m_context.appendRevert(); for (auto const& it: interfaceFunctions) { @@ -368,7 +368,7 @@ void ContractCompiler::appendCalldataUnpacker(TypePointers const& _typeParameter // copy to memory // move calldata type up again CompilerUtils(m_context).moveIntoStack(calldataType->sizeOnStack()); - CompilerUtils(m_context).convertType(*calldataType, arrayType); + CompilerUtils(m_context).convertType(*calldataType, arrayType, false, false, true); // fetch next pointer again CompilerUtils(m_context).moveToStackTop(arrayType.sizeOnStack()); } @@ -805,8 +805,7 @@ bool ContractCompiler::visit(Throw const& _throw) { CompilerContext::LocationSetter locationSetter(m_context, _throw); // Do not send back an error detail. - m_context << u256(0) << u256(0); - m_context << Instruction::REVERT; + m_context.appendRevert(); return false; } diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index 03bba80c6..2fc3bf6b2 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -587,7 +587,7 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) m_context << Instruction::CREATE; // Check if zero (out of stack or not enough balance). m_context << Instruction::DUP1 << Instruction::ISZERO; - m_context.appendConditionalInvalid(); + m_context.appendConditionalRevert(); if (function.valueSet()) m_context << swapInstruction(1) << Instruction::POP; break; @@ -651,7 +651,7 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) { // Check if zero (out of stack or not enough balance). m_context << Instruction::ISZERO; - m_context.appendConditionalInvalid(); + m_context.appendConditionalRevert(); } break; case FunctionType::Kind::Selfdestruct: @@ -660,9 +660,7 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) m_context << Instruction::SELFDESTRUCT; break; case FunctionType::Kind::Revert: - // memory offset returned - zero length - m_context << u256(0) << u256(0); - m_context << Instruction::REVERT; + m_context.appendRevert(); break; case FunctionType::Kind::SHA3: { @@ -890,7 +888,7 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) // condition was not met, flag an error m_context << Instruction::INVALID; else - m_context << u256(0) << u256(0) << Instruction::REVERT; + m_context.appendRevert(); // the success branch m_context << success; break; @@ -1695,7 +1693,7 @@ void ExpressionCompiler::appendExternalFunctionCall( if (funKind == FunctionType::Kind::External || funKind == FunctionType::Kind::CallCode || funKind == FunctionType::Kind::DelegateCall) { m_context << Instruction::DUP1 << Instruction::EXTCODESIZE << Instruction::ISZERO; - m_context.appendConditionalInvalid(); + m_context.appendConditionalRevert(); existenceChecked = true; } @@ -1731,7 +1729,7 @@ void ExpressionCompiler::appendExternalFunctionCall( { //Propagate error condition (if CALL pushes 0 on stack). m_context << Instruction::ISZERO; - m_context.appendConditionalInvalid(); + m_context.appendConditionalRevert(); } utils().popStackSlots(remainsSize);