From f5a46ef7bcf6ba1a831f2508e7cb8b2c48129392 Mon Sep 17 00:00:00 2001 From: Josh Asplund Date: Mon, 4 Jun 2018 18:42:34 -0500 Subject: [PATCH] Fixes issues with python 3.5 dict ordering --- .circleci/config.yml | 11 +-- Pipfile | 1 + mythril/analysis/report.py | 16 +++-- .../templates/report_as_markdown.jinja2 | 2 +- .../analysis/templates/report_as_text.jinja2 | 2 +- tests/report_test.py | 46 +++++++++++- .../outputs_expected/calls.sol.o.markdown | 70 +++++++++---------- .../outputs_expected/calls.sol.o.text | 58 +++++++-------- .../outputs_expected/overflow.sol.o.markdown | 18 ++--- .../outputs_expected/overflow.sol.o.text | 18 ++--- .../outputs_expected/underflow.sol.o.markdown | 18 ++--- .../outputs_expected/underflow.sol.o.text | 18 ++--- tox.ini | 23 +++--- 13 files changed, 177 insertions(+), 124 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6a9c809b..87320f71 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -32,6 +32,12 @@ jobs: LC_ALL: C.UTF-8 LANG: C.UTF-8 + - save_cache: + key: tox-env-{{ checksum "/home/mythril/tox.ini" }}-{{ checksum "/home/mythril/setup.py" }} + paths: + - .tox/py* + - /root/.cache/pip/wheels/ + - run: background: true name: Launch of background geth instance @@ -51,11 +57,6 @@ jobs: - store_artifacts: path: /home/mythril/.tox/output - - save_cache: - key: tox-env-{{ checksum "/home/mythril/tox.ini" }}-{{ checksum "/home/mythril/setup.py" }} - paths: - - .tox/py* - - run: name: Ensuring that setup script is functional command: python3 setup.py install diff --git a/Pipfile b/Pipfile index 459f1564..753481aa 100644 --- a/Pipfile +++ b/Pipfile @@ -11,6 +11,7 @@ pylint = "*" yapf = "*" pytest = "*" pytest-mock = "*" +pytest-cov = "*" [requires] diff --git a/mythril/analysis/report.py b/mythril/analysis/report.py index d82a2530..1e61092a 100644 --- a/mythril/analysis/report.py +++ b/mythril/analysis/report.py @@ -1,5 +1,6 @@ import hashlib import json +import operator from jinja2 import PackageLoader, Environment class Issue: @@ -45,25 +46,28 @@ class Report: self.verbose = verbose pass + def sorted_issues(self): + issue_list = [issue.as_dict() for key, issue in self.issues.items()] + return sorted(issue_list, key=operator.itemgetter('address', 'title')) + def append_issue(self, issue): m = hashlib.md5() m.update((issue.contract + str(issue.address) + issue.title).encode('utf-8')) self.issues[m.digest()] = issue def as_text(self): - filename = self._file_name() + name = self._file_name() template = Report.environment.get_template('report_as_text.jinja2') - return template.render(filename=filename, issues=self.issues, verbose=self.verbose) + return template.render(filename=name, issues=self.sorted_issues(), verbose=self.verbose) def as_json(self): - issues = [issue.as_dict() for key, issue in self.issues.items()] - result = {'success': True, 'error': None, 'issues': issues} - return json.dumps(result) + result = {'success': True, 'error': None, 'issues': self.sorted_issues()} + return json.dumps(result, sort_keys=True) def as_markdown(self): filename = self._file_name() template = Report.environment.get_template('report_as_markdown.jinja2') - return template.render(filename=filename, issues=self.issues, verbose=self.verbose) + return template.render(filename=filename, issues=self.sorted_issues(), verbose=self.verbose) def _file_name(self): if len(self.issues.values()) > 0: diff --git a/mythril/analysis/templates/report_as_markdown.jinja2 b/mythril/analysis/templates/report_as_markdown.jinja2 index 9ceb80a7..3f83245f 100644 --- a/mythril/analysis/templates/report_as_markdown.jinja2 +++ b/mythril/analysis/templates/report_as_markdown.jinja2 @@ -1,6 +1,6 @@ # Analysis results for {{ filename }} {% if issues %} -{% for key, issue in issues.items() %} +{% for issue in issues %} ## {{ issue.title }} diff --git a/mythril/analysis/templates/report_as_text.jinja2 b/mythril/analysis/templates/report_as_text.jinja2 index 1fb18e1a..7e7e1482 100644 --- a/mythril/analysis/templates/report_as_text.jinja2 +++ b/mythril/analysis/templates/report_as_text.jinja2 @@ -1,5 +1,5 @@ {% if issues %} -{% for key, issue in issues.items() %} +{% for issue in issues %} ==== {{ issue.title }} ==== Type: {{ issue.type }} Contract: {{ issue.contract | default("Unknown") }} diff --git a/tests/report_test.py b/tests/report_test.py index dae6cc4e..88d4c777 100644 --- a/tests/report_test.py +++ b/tests/report_test.py @@ -19,7 +19,7 @@ def _fix_debug_data(json_str): read_json = json.loads(json_str) for issue in read_json["issues"]: issue["debug"] = "" - return json.dumps(read_json, indent=4) + return json.dumps(read_json, sort_keys=True) def _generate_report(input_file): @@ -56,6 +56,29 @@ def _assert_empty(changed_files, postfix): assert message == "", message +def _assert_empty_json(changed_files): + """ Asserts there are no changed files and otherwise builds error message""" + postfix = ".json" + expected = [] + actual = [] + + def ordered(obj): + if isinstance(obj, dict): + return sorted((k, ordered(v)) for k, v in obj.items()) + elif isinstance(obj, list): + return sorted(ordered(x) for x in obj) + else: + return obj + + for input_file in changed_files: + output_expected = json.loads((TESTDATA_OUTPUTS_EXPECTED / (input_file.name + postfix)).read_text()) + output_current = json.loads((TESTDATA_OUTPUTS_CURRENT / (input_file.name + postfix)).read_text()) + + if not ordered(output_expected.items()) == ordered(output_current.items()): + expected.append(output_expected) + actual.append(output_current) + + assert expected == actual def _get_changed_files(postfix, report_builder, reports): """ @@ -74,8 +97,27 @@ def _get_changed_files(postfix, report_builder, reports): yield input_file +def _get_changed_files_json(report_builder, reports): + postfix = ".json" + def ordered(obj): + if isinstance(obj, dict): + return sorted((k, ordered(v)) for k, v in obj.items()) + elif isinstance(obj, list): + return sorted(ordered(x) for x in obj) + else: + return obj + + for report, input_file in reports: + output_expected = TESTDATA_OUTPUTS_EXPECTED / (input_file.name + postfix) + output_current = TESTDATA_OUTPUTS_CURRENT / (input_file.name + postfix) + output_current.write_text(report_builder(report)) + + if not ordered(json.loads(output_expected.read_text())) == ordered(json.loads(output_current.read_text())): + yield input_file + + def test_json_report(reports): - _assert_empty(_get_changed_files('.json', lambda report: _fix_path(_fix_debug_data(report.as_json())).strip(), reports), '.json') + _assert_empty_json(_get_changed_files_json(lambda report: _fix_path(_fix_debug_data(report.as_json())).strip(), reports)) def test_markdown_report(reports): diff --git a/tests/testdata/outputs_expected/calls.sol.o.markdown b/tests/testdata/outputs_expected/calls.sol.o.markdown index 7c75e616..02e69173 100644 --- a/tests/testdata/outputs_expected/calls.sol.o.markdown +++ b/tests/testdata/outputs_expected/calls.sol.o.markdown @@ -11,49 +11,27 @@ This contract executes a message call to to another contract. Make sure that the called contract is trusted and does not execute user-supplied code. -## Message call to external contract - -- Type: Warning -- Contract: Unknown -- Function name: `_function_0xd24b08cc` -- PC address: 779 - -### Description - -This contract executes a message call to an address found at storage slot 1. This storage slot can be written to by calling the function `_function_0x2776b163`. Generally, it is not recommended to call user-supplied addresses using Solidity's call() construct. Note that attackers might leverage reentrancy attacks to exploit race conditions or manipulate this contract's state. - -## Message call to external contract +## Unchecked CALL return value - Type: Informational - Contract: Unknown -- Function name: `_function_0xe11f493e` -- PC address: 858 - -### Description - -This contract executes a message call to to another contract. Make sure that the called contract is trusted and does not execute user-supplied code. - -## State change after external call - -- Type: Warning -- Contract: Unknown -- Function name: `_function_0xe11f493e` -- PC address: 869 +- Function name: `_function_0x5a6814ec` +- PC address: 661 ### Description -The contract account state is changed after an external call. Consider that the called contract could re-enter the function before this state change takes place. This can lead to business logic vulnerabilities. +The return value of an external call is not checked. Note that execution continue even if the called contract throws. ## Message call to external contract - Type: Warning - Contract: Unknown -- Function name: `_function_0xe1d10f79` -- PC address: 912 +- Function name: `_function_0xd24b08cc` +- PC address: 779 ### Description -This contract executes a message call to an address provided as a function argument. Generally, it is not recommended to call user-supplied addresses using Solidity's call() construct. Note that attackers might leverage reentrancy attacks to exploit race conditions or manipulate this contract's state. +This contract executes a message call to an address found at storage slot 1. This storage slot can be written to by calling the function `_function_0x2776b163`. Generally, it is not recommended to call user-supplied addresses using Solidity's call() construct. Note that attackers might leverage reentrancy attacks to exploit race conditions or manipulate this contract's state. ## Transaction order dependence @@ -70,23 +48,23 @@ A possible transaction order independence vulnerability exists in function _func - Type: Informational - Contract: Unknown -- Function name: `_function_0x5a6814ec` -- PC address: 661 +- Function name: `_function_0xd24b08cc` +- PC address: 779 ### Description The return value of an external call is not checked. Note that execution continue even if the called contract throws. -## Unchecked CALL return value +## Message call to external contract - Type: Informational - Contract: Unknown -- Function name: `_function_0xd24b08cc` -- PC address: 779 +- Function name: `_function_0xe11f493e` +- PC address: 858 ### Description -The return value of an external call is not checked. Note that execution continue even if the called contract throws. +This contract executes a message call to to another contract. Make sure that the called contract is trusted and does not execute user-supplied code. ## Unchecked CALL return value @@ -99,6 +77,28 @@ The return value of an external call is not checked. Note that execution continu The return value of an external call is not checked. Note that execution continue even if the called contract throws. +## State change after external call + +- Type: Warning +- Contract: Unknown +- Function name: `_function_0xe11f493e` +- PC address: 869 + +### Description + +The contract account state is changed after an external call. Consider that the called contract could re-enter the function before this state change takes place. This can lead to business logic vulnerabilities. + +## Message call to external contract + +- Type: Warning +- Contract: Unknown +- Function name: `_function_0xe1d10f79` +- PC address: 912 + +### Description + +This contract executes a message call to an address provided as a function argument. Generally, it is not recommended to call user-supplied addresses using Solidity's call() construct. Note that attackers might leverage reentrancy attacks to exploit race conditions or manipulate this contract's state. + ## Unchecked CALL return value - Type: Informational diff --git a/tests/testdata/outputs_expected/calls.sol.o.text b/tests/testdata/outputs_expected/calls.sol.o.text index c327fc14..efdfd1e4 100644 --- a/tests/testdata/outputs_expected/calls.sol.o.text +++ b/tests/testdata/outputs_expected/calls.sol.o.text @@ -6,36 +6,20 @@ PC address: 661 This contract executes a message call to to another contract. Make sure that the called contract is trusted and does not execute user-supplied code. -------------------- -==== Message call to external contract ==== -Type: Warning -Contract: Unknown -Function name: _function_0xd24b08cc -PC address: 779 -This contract executes a message call to an address found at storage slot 1. This storage slot can be written to by calling the function `_function_0x2776b163`. Generally, it is not recommended to call user-supplied addresses using Solidity's call() construct. Note that attackers might leverage reentrancy attacks to exploit race conditions or manipulate this contract's state. --------------------- - -==== Message call to external contract ==== +==== Unchecked CALL return value ==== Type: Informational Contract: Unknown -Function name: _function_0xe11f493e -PC address: 858 -This contract executes a message call to to another contract. Make sure that the called contract is trusted and does not execute user-supplied code. --------------------- - -==== State change after external call ==== -Type: Warning -Contract: Unknown -Function name: _function_0xe11f493e -PC address: 869 -The contract account state is changed after an external call. Consider that the called contract could re-enter the function before this state change takes place. This can lead to business logic vulnerabilities. +Function name: _function_0x5a6814ec +PC address: 661 +The return value of an external call is not checked. Note that execution continue even if the called contract throws. -------------------- ==== Message call to external contract ==== Type: Warning Contract: Unknown -Function name: _function_0xe1d10f79 -PC address: 912 -This contract executes a message call to an address provided as a function argument. Generally, it is not recommended to call user-supplied addresses using Solidity's call() construct. Note that attackers might leverage reentrancy attacks to exploit race conditions or manipulate this contract's state. +Function name: _function_0xd24b08cc +PC address: 779 +This contract executes a message call to an address found at storage slot 1. This storage slot can be written to by calling the function `_function_0x2776b163`. Generally, it is not recommended to call user-supplied addresses using Solidity's call() construct. Note that attackers might leverage reentrancy attacks to exploit race conditions or manipulate this contract's state. -------------------- ==== Transaction order dependence ==== @@ -49,17 +33,17 @@ A possible transaction order independence vulnerability exists in function _func ==== Unchecked CALL return value ==== Type: Informational Contract: Unknown -Function name: _function_0x5a6814ec -PC address: 661 +Function name: _function_0xd24b08cc +PC address: 779 The return value of an external call is not checked. Note that execution continue even if the called contract throws. -------------------- -==== Unchecked CALL return value ==== +==== Message call to external contract ==== Type: Informational Contract: Unknown -Function name: _function_0xd24b08cc -PC address: 779 -The return value of an external call is not checked. Note that execution continue even if the called contract throws. +Function name: _function_0xe11f493e +PC address: 858 +This contract executes a message call to to another contract. Make sure that the called contract is trusted and does not execute user-supplied code. -------------------- ==== Unchecked CALL return value ==== @@ -70,6 +54,22 @@ PC address: 858 The return value of an external call is not checked. Note that execution continue even if the called contract throws. -------------------- +==== State change after external call ==== +Type: Warning +Contract: Unknown +Function name: _function_0xe11f493e +PC address: 869 +The contract account state is changed after an external call. Consider that the called contract could re-enter the function before this state change takes place. This can lead to business logic vulnerabilities. +-------------------- + +==== Message call to external contract ==== +Type: Warning +Contract: Unknown +Function name: _function_0xe1d10f79 +PC address: 912 +This contract executes a message call to an address provided as a function argument. Generally, it is not recommended to call user-supplied addresses using Solidity's call() construct. Note that attackers might leverage reentrancy attacks to exploit race conditions or manipulate this contract's state. +-------------------- + ==== Unchecked CALL return value ==== Type: Informational Contract: Unknown diff --git a/tests/testdata/outputs_expected/overflow.sol.o.markdown b/tests/testdata/outputs_expected/overflow.sol.o.markdown index d2c8f9e6..07d375b8 100644 --- a/tests/testdata/outputs_expected/overflow.sol.o.markdown +++ b/tests/testdata/outputs_expected/overflow.sol.o.markdown @@ -5,33 +5,33 @@ - Type: Warning - Contract: Unknown - Function name: `sendeth(address,uint256)` -- PC address: 649 +- PC address: 567 ### Description A possible integer underflow exists in the function `sendeth(address,uint256)`. The subtraction may result in a value < 0. -## Integer Overflow +## Integer Underflow - Type: Warning - Contract: Unknown - Function name: `sendeth(address,uint256)` -- PC address: 725 +- PC address: 649 ### Description -A possible integer overflow exists in the function `sendeth(address,uint256)`. -The addition or multiplication may result in a value higher than the maximum representable integer. +A possible integer underflow exists in the function `sendeth(address,uint256)`. +The subtraction may result in a value < 0. -## Integer Underflow +## Integer Overflow - Type: Warning - Contract: Unknown - Function name: `sendeth(address,uint256)` -- PC address: 567 +- PC address: 725 ### Description -A possible integer underflow exists in the function `sendeth(address,uint256)`. -The subtraction may result in a value < 0. +A possible integer overflow exists in the function `sendeth(address,uint256)`. +The addition or multiplication may result in a value higher than the maximum representable integer. diff --git a/tests/testdata/outputs_expected/overflow.sol.o.text b/tests/testdata/outputs_expected/overflow.sol.o.text index a3ec68c4..f15d633f 100644 --- a/tests/testdata/outputs_expected/overflow.sol.o.text +++ b/tests/testdata/outputs_expected/overflow.sol.o.text @@ -2,26 +2,26 @@ Type: Warning Contract: Unknown Function name: sendeth(address,uint256) -PC address: 649 +PC address: 567 A possible integer underflow exists in the function `sendeth(address,uint256)`. The subtraction may result in a value < 0. -------------------- -==== Integer Overflow ==== +==== Integer Underflow ==== Type: Warning Contract: Unknown Function name: sendeth(address,uint256) -PC address: 725 -A possible integer overflow exists in the function `sendeth(address,uint256)`. -The addition or multiplication may result in a value higher than the maximum representable integer. +PC address: 649 +A possible integer underflow exists in the function `sendeth(address,uint256)`. +The subtraction may result in a value < 0. -------------------- -==== Integer Underflow ==== +==== Integer Overflow ==== Type: Warning Contract: Unknown Function name: sendeth(address,uint256) -PC address: 567 -A possible integer underflow exists in the function `sendeth(address,uint256)`. -The subtraction may result in a value < 0. +PC address: 725 +A possible integer overflow exists in the function `sendeth(address,uint256)`. +The addition or multiplication may result in a value higher than the maximum representable integer. -------------------- diff --git a/tests/testdata/outputs_expected/underflow.sol.o.markdown b/tests/testdata/outputs_expected/underflow.sol.o.markdown index d2c8f9e6..07d375b8 100644 --- a/tests/testdata/outputs_expected/underflow.sol.o.markdown +++ b/tests/testdata/outputs_expected/underflow.sol.o.markdown @@ -5,33 +5,33 @@ - Type: Warning - Contract: Unknown - Function name: `sendeth(address,uint256)` -- PC address: 649 +- PC address: 567 ### Description A possible integer underflow exists in the function `sendeth(address,uint256)`. The subtraction may result in a value < 0. -## Integer Overflow +## Integer Underflow - Type: Warning - Contract: Unknown - Function name: `sendeth(address,uint256)` -- PC address: 725 +- PC address: 649 ### Description -A possible integer overflow exists in the function `sendeth(address,uint256)`. -The addition or multiplication may result in a value higher than the maximum representable integer. +A possible integer underflow exists in the function `sendeth(address,uint256)`. +The subtraction may result in a value < 0. -## Integer Underflow +## Integer Overflow - Type: Warning - Contract: Unknown - Function name: `sendeth(address,uint256)` -- PC address: 567 +- PC address: 725 ### Description -A possible integer underflow exists in the function `sendeth(address,uint256)`. -The subtraction may result in a value < 0. +A possible integer overflow exists in the function `sendeth(address,uint256)`. +The addition or multiplication may result in a value higher than the maximum representable integer. diff --git a/tests/testdata/outputs_expected/underflow.sol.o.text b/tests/testdata/outputs_expected/underflow.sol.o.text index a3ec68c4..f15d633f 100644 --- a/tests/testdata/outputs_expected/underflow.sol.o.text +++ b/tests/testdata/outputs_expected/underflow.sol.o.text @@ -2,26 +2,26 @@ Type: Warning Contract: Unknown Function name: sendeth(address,uint256) -PC address: 649 +PC address: 567 A possible integer underflow exists in the function `sendeth(address,uint256)`. The subtraction may result in a value < 0. -------------------- -==== Integer Overflow ==== +==== Integer Underflow ==== Type: Warning Contract: Unknown Function name: sendeth(address,uint256) -PC address: 725 -A possible integer overflow exists in the function `sendeth(address,uint256)`. -The addition or multiplication may result in a value higher than the maximum representable integer. +PC address: 649 +A possible integer underflow exists in the function `sendeth(address,uint256)`. +The subtraction may result in a value < 0. -------------------- -==== Integer Underflow ==== +==== Integer Overflow ==== Type: Warning Contract: Unknown Function name: sendeth(address,uint256) -PC address: 567 -A possible integer underflow exists in the function `sendeth(address,uint256)`. -The subtraction may result in a value < 0. +PC address: 725 +A possible integer overflow exists in the function `sendeth(address,uint256)`. +The addition or multiplication may result in a value higher than the maximum representable integer. -------------------- diff --git a/tox.ini b/tox.ini index edb6f042..a00a43a4 100644 --- a/tox.ini +++ b/tox.ini @@ -1,27 +1,32 @@ [tox] -envlist = py35,py36 +envlist = py36 [testenv] -deps=pipenv +deps = + pytest + pytest-mock whitelist_externals = mkdir -commands= - pipenv install --dev --ignore-pipfile +commands = mkdir -p {toxinidir}/tests/testdata/outputs_current/ - py.test \ + py.test -v \ --junitxml={toxworkdir}/output/junit-{envname}.xml \ + --junitprefix={envname} \ {posargs} [testenv:coverage] setenv = COVERAGE_FILE = .coveragerc -deps = pytest-cov +deps = + pytest + pytest-mock + pytest-cov whitelist_externals = mkdir -commands= - pipenv install --dev --ignore-pipfile +commands = mkdir -p {toxinidir}/tests/testdata/outputs_current/ - py.test \ + py.test -v \ --cov=mythril \ --cov-report=xml:{toxworkdir}/output/cov-{envname}.xml \ --cov-report=html:{toxworkdir}/output/cov-{envname}.html \ --junitxml={toxworkdir}/output/junit-{envname}.xml \ + --junitprefix={envname} \ {posargs}