From d4fecc7b11586a108578a1834b634981bc854f91 Mon Sep 17 00:00:00 2001
From: Alex Beregszaszi <alex@rtfs.hu>
Date: Tue, 13 Jun 2017 19:10:26 +0100
Subject: Warn on JUMP/JUMPI in inline assembly
---
Changelog.md | 1 +
libsolidity/inlineasm/AsmAnalysis.cpp | 13 ++++++++++---
libsolidity/inlineasm/AsmAnalysis.h | 2 +-
test/libsolidity/InlineAssembly.cpp | 8 ++++++++
4 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/Changelog.md b/Changelog.md
index 4f224d5b..68f2272d 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -12,6 +12,7 @@ Features:
* Inline Assembly: introduce ``keccak256`` as an opcode. ``sha3`` is still a valid alias.
* Inline Assembly: ``for`` and ``switch`` statements.
* Inline Assembly: function definitions and function calls.
+ * Inline Assembly: Warn when using ``jump``s.
* Type Checker: Warn about copies in storage that might overwrite unexpectedly.
* Code Generator: Added the Whiskers template system.
* Remove obsolete Why3 output.
diff --git a/libsolidity/inlineasm/AsmAnalysis.cpp b/libsolidity/inlineasm/AsmAnalysis.cpp
index b0d044ae..7e00ffae 100644
--- a/libsolidity/inlineasm/AsmAnalysis.cpp
+++ b/libsolidity/inlineasm/AsmAnalysis.cpp
@@ -65,7 +65,7 @@ bool AsmAnalyzer::operator()(assembly::Instruction const& _instruction)
auto const& info = instructionInfo(_instruction.instruction);
m_stackHeight += info.ret - info.args;
m_info.stackHeightInfo[&_instruction] = m_stackHeight;
- warnOnFutureInstruction(_instruction.instruction, _instruction.location);
+ warnOnInstructions(_instruction.instruction, _instruction.location);
return true;
}
@@ -150,7 +150,6 @@ bool AsmAnalyzer::operator()(FunctionalInstruction const& _instr)
if (!(*this)(_instr.instruction))
success = false;
m_info.stackHeightInfo[&_instr] = m_stackHeight;
- warnOnFutureInstruction(_instr.instruction.instruction, _instr.location);
return success;
}
@@ -470,7 +469,7 @@ void AsmAnalyzer::expectValidType(string const& type, SourceLocation const& _loc
);
}
-void AsmAnalyzer::warnOnFutureInstruction(solidity::Instruction _instr, SourceLocation const& _location)
+void AsmAnalyzer::warnOnInstructions(solidity::Instruction _instr, SourceLocation const& _location)
{
static set<solidity::Instruction> futureInstructions{
solidity::Instruction::CREATE2,
@@ -486,4 +485,12 @@ void AsmAnalyzer::warnOnFutureInstruction(solidity::Instruction _instr, SourceLo
+ "\" instruction is only available after " +
"the Metropolis hard fork. Before that it acts as an invalid instruction."
);
+
+ if (_instr == solidity::Instruction::JUMP || _instr == solidity::Instruction::JUMPI)
+ m_errorReporter.warning(
+ _location,
+ "Jump instructions are low-level EVM features that can lead to "
+ "incorrect stack access. Because of that they are discouraged. "
+ "Please consider using \"switch\" or \"for\" statements instead."
+ );
}
diff --git a/libsolidity/inlineasm/AsmAnalysis.h b/libsolidity/inlineasm/AsmAnalysis.h
index 76d2eba1..9b2a8f9c 100644
--- a/libsolidity/inlineasm/AsmAnalysis.h
+++ b/libsolidity/inlineasm/AsmAnalysis.h
@@ -85,7 +85,7 @@ private:
Scope& scope(assembly::Block const* _block);
void expectValidType(std::string const& type, SourceLocation const& _location);
- void warnOnFutureInstruction(solidity::Instruction _instr, SourceLocation const& _location);
+ void warnOnInstructions(solidity::Instruction _instr, SourceLocation const& _location);
int m_stackHeight = 0;
julia::ExternalIdentifierAccess::Resolver m_resolver;
diff --git a/test/libsolidity/InlineAssembly.cpp b/test/libsolidity/InlineAssembly.cpp
index 7b760a1d..345ccfde 100644
--- a/test/libsolidity/InlineAssembly.cpp
+++ b/test/libsolidity/InlineAssembly.cpp
@@ -630,6 +630,14 @@ BOOST_AUTO_TEST_CASE(create2)
BOOST_CHECK(successAssemble("{ pop(create2(10, 0x123, 32, 64)) }"));
}
+BOOST_AUTO_TEST_CASE(jump_warning)
+{
+ CHECK_PARSE_ERROR("{ 1 jump }", Warning, "Jump instructions");
+ CHECK_PARSE_ERROR("{ 1 2 jumpi }", Warning, "Jump instructions");
+ CHECK_PARSE_ERROR("{ a: jump(a) }", Warning, "Jump instructions");
+ CHECK_PARSE_ERROR("{ a: jumpi(a, 2) }", Warning, "Jump instructions");
+}
+
BOOST_AUTO_TEST_SUITE_END()
BOOST_AUTO_TEST_SUITE_END()
--
cgit v1.2.3
From 0c92f5394456cb0818a53ceaab05199246b7a274 Mon Sep 17 00:00:00 2001
From: Alex Beregszaszi <alex@rtfs.hu>
Date: Wed, 28 Jun 2017 18:30:13 +0100
Subject: Correctly check for jump warnings
---
test/libsolidity/InlineAssembly.cpp | 56 +++++++++++++++++++------------------
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/test/libsolidity/InlineAssembly.cpp b/test/libsolidity/InlineAssembly.cpp
index 345ccfde..5197f649 100644
--- a/test/libsolidity/InlineAssembly.cpp
+++ b/test/libsolidity/InlineAssembly.cpp
@@ -67,23 +67,19 @@ boost::optional<Error> parseAndReturnFirstError(
BOOST_FAIL("Fatal error leaked.");
success = false;
}
- if (!success)
+ shared_ptr<Error const> error;
+ for (auto const& e: stack.errors())
{
- BOOST_REQUIRE_EQUAL(stack.errors().size(), 1);
- return *stack.errors().front();
- }
- else
- {
- // If success is true, there might still be an error in the assembly stage.
- if (_allowWarnings && Error::containsOnlyWarnings(stack.errors()))
- return {};
- else if (!stack.errors().empty())
- {
- if (!_allowWarnings)
- BOOST_CHECK_EQUAL(stack.errors().size(), 1);
- return *stack.errors().front();
- }
+ if (_allowWarnings && e->type() == Error::Type::Warning)
+ continue;
+ if (error)
+ BOOST_FAIL("Found more than one error.");
+ error = e;
}
+ if (!success)
+ BOOST_REQUIRE(error);
+ if (error)
+ return *error;
return {};
}
@@ -111,29 +107,35 @@ Error expectError(std::string const& _source, bool _assemble, bool _allowWarning
return *error;
}
-void parsePrintCompare(string const& _source)
+void parsePrintCompare(string const& _source, bool _canWarn = false)
{
AssemblyStack stack;
BOOST_REQUIRE(stack.parseAndAnalyze("", _source));
- BOOST_REQUIRE(stack.errors().empty());
+ if (_canWarn)
+ BOOST_REQUIRE(Error::containsOnlyWarnings(stack.errors()));
+ else
+ BOOST_REQUIRE(stack.errors().empty());
BOOST_CHECK_EQUAL(stack.print(), _source);
}
}
-#define CHECK_ERROR(text, assemble, typ, substring) \
+#define CHECK_ERROR(text, assemble, typ, substring, warnings) \
do \
{ \
- Error err = expectError((text), (assemble), false); \
+ Error err = expectError((text), (assemble), warnings); \
BOOST_CHECK(err.type() == (Error::Type::typ)); \
BOOST_CHECK(searchErrorMessage(err, (substring))); \
} while(0)
#define CHECK_PARSE_ERROR(text, type, substring) \
-CHECK_ERROR(text, false, type, substring)
+CHECK_ERROR(text, false, type, substring, false)
+
+#define CHECK_PARSE_WARNING(text, type, substring) \
+CHECK_ERROR(text, false, type, substring, false)
#define CHECK_ASSEMBLE_ERROR(text, type, substring) \
-CHECK_ERROR(text, true, type, substring)
+CHECK_ERROR(text, true, type, substring, false)
@@ -411,7 +413,7 @@ BOOST_AUTO_TEST_CASE(print_functional)
BOOST_AUTO_TEST_CASE(print_label)
{
- parsePrintCompare("{\n loop:\n jump(loop)\n}");
+ parsePrintCompare("{\n loop:\n jump(loop)\n}", true);
}
BOOST_AUTO_TEST_CASE(print_assignments)
@@ -515,7 +517,7 @@ BOOST_AUTO_TEST_CASE(imbalanced_stack)
BOOST_AUTO_TEST_CASE(error_tag)
{
- CHECK_ASSEMBLE_ERROR("{ jump(invalidJumpLabel) }", DeclarationError, "Identifier not found");
+ CHECK_ERROR("{ jump(invalidJumpLabel) }", true, DeclarationError, "Identifier not found", true);
}
BOOST_AUTO_TEST_CASE(designated_invalid_instruction)
@@ -632,10 +634,10 @@ BOOST_AUTO_TEST_CASE(create2)
BOOST_AUTO_TEST_CASE(jump_warning)
{
- CHECK_PARSE_ERROR("{ 1 jump }", Warning, "Jump instructions");
- CHECK_PARSE_ERROR("{ 1 2 jumpi }", Warning, "Jump instructions");
- CHECK_PARSE_ERROR("{ a: jump(a) }", Warning, "Jump instructions");
- CHECK_PARSE_ERROR("{ a: jumpi(a, 2) }", Warning, "Jump instructions");
+ CHECK_PARSE_WARNING("{ 1 jump }", Warning, "Jump instructions");
+ CHECK_PARSE_WARNING("{ 1 2 jumpi }", Warning, "Jump instructions");
+ CHECK_PARSE_WARNING("{ a: jump(a) }", Warning, "Jump instructions");
+ CHECK_PARSE_WARNING("{ a: jumpi(a, 2) }", Warning, "Jump instructions");
}
BOOST_AUTO_TEST_SUITE_END()
--
cgit v1.2.3