diff options
| -rw-r--r-- | .circleci/config.yml | 24 | ||||
| -rw-r--r-- | docs/bugs.json | 35 | ||||
| -rw-r--r-- | docs/bugs.rst | 17 | ||||
| -rwxr-xr-x | test/buglistTests.js | 131 | ||||
| -rw-r--r-- | test/buglist_test_vectors.md | 45 |
5 files changed, 199 insertions, 53 deletions
diff --git a/.circleci/config.yml b/.circleci/config.yml index aec8be18..3e3d8c0a 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -189,15 +189,21 @@ jobs: command: ./scripts/detect_trailing_whitespace.sh test_buglist: - docker: - - image: circleci/node - environment: - TERM: xterm - steps: - - checkout - - run: - name: Test buglist - command: ./test/buglistTests.js + docker: + - image: circleci/node + environment: + TERM: xterm + steps: + - checkout + - run: + name: JS deps + command: | + npm install download + npm install JSONPath + npm install mktemp + - run: + name: Test buglist + command: ./test/buglistTests.js test_x86_linux: docker: diff --git a/docs/bugs.json b/docs/bugs.json index 839ea128..cf03adfe 100644 --- a/docs/bugs.json +++ b/docs/bugs.json @@ -1,21 +1,22 @@ [ - { - "name": "EventStructWrongData", - "summary": "Using structs in events logged wrong data.", - "description": "If a struct is used in an event, the address of the struct is logged instead of the actual data.", - "introduced": "0.4.17", - "fixed": "0.5.0", - "severity": "very low" - }, - { - "name": "NestedArrayFunctionCallDecoder", - "summary": "Calling functions that return multi-dimensional fixed-size arrays can result in memory corruption.", - "description": "If Solidity code calls a function that returns a multi-dimensional fixed-size array, array elements are incorrectly interpreted as memory pointers and thus can cause memory corruption if the return values are accessed. Calling functions with multi-dimensional fixed-size arrays is unaffected as is returning fixed-size arrays from function calls. The regular expression only checks if such functions are present, not if they are called, which is required for the contract to be affected.", - "introduced": "0.1.4", - "fixed": "0.4.22", - "severity": "medium", - "check": {"regex-source": "returns[^;{]*\\[\\s*[^\\] \\t\\r\\n\\v\\f][^\\]]*\\]\\s*\\[\\s*[^\\] \\t\\r\\n\\v\\f][^\\]]*\\][^{;]*[;{]"} - }, + { + "name": "EventStructWrongData", + "summary": "Using structs in events logged wrong data.", + "description": "If a struct is used in an event, the address of the struct is logged instead of the actual data.", + "introduced": "0.4.17", + "fixed": "0.5.0", + "severity": "very low", + "check": {"ast-compact-json-path": "$..[?(@.nodeType === 'EventDefinition')]..[?(@.nodeType === 'UserDefinedTypeName' && @.typeDescriptions.typeString.startsWith('struct'))]"} + }, + { + "name": "NestedArrayFunctionCallDecoder", + "summary": "Calling functions that return multi-dimensional fixed-size arrays can result in memory corruption.", + "description": "If Solidity code calls a function that returns a multi-dimensional fixed-size array, array elements are incorrectly interpreted as memory pointers and thus can cause memory corruption if the return values are accessed. Calling functions with multi-dimensional fixed-size arrays is unaffected as is returning fixed-size arrays from function calls. The regular expression only checks if such functions are present, not if they are called, which is required for the contract to be affected.", + "introduced": "0.1.4", + "fixed": "0.4.22", + "severity": "medium", + "check": {"regex-source": "returns[^;{]*\\[\\s*[^\\] \\t\\r\\n\\v\\f][^\\]]*\\]\\s*\\[\\s*[^\\] \\t\\r\\n\\v\\f][^\\]]*\\][^{;]*[;{]"} + }, { "name": "OneOfTwoConstructorsSkipped", "summary": "If a contract has both a new-style constructor (using the constructor keyword) and an old-style constructor (a function with the same name as the contract) at the same time, one of them will be ignored.", diff --git a/docs/bugs.rst b/docs/bugs.rst index 6f315a65..f7522183 100644 --- a/docs/bugs.rst +++ b/docs/bugs.rst @@ -57,13 +57,18 @@ conditions means that the optimizer has to be switched on to enable the bug. If no conditions are given, assume that the bug is present. check - This field contains JavaScript regular expressions that are to be matched - against the source code ("source-regex") to find out if the - smart contract contains the bug or not. If there is no match, - then the bug is very likely not present. If there is a match, - the bug might be present. For improved accuracy, the regular - expression should be applied to the source code after stripping + This field contains different checks that report whether the smart contract + contains the bug or not. The first type of check are Javascript regular + expressions that are to be matched against the source code ("source-regex") + if the bug is present. If there is no match, then the bug is very likely + not present. If there is a match, the bug might be present. For improved + accuracy, the checks should be applied to the source code after stripping comments. + The second type of check are patterns to be checked on the compact AST of + the Solidity program ("ast-compact-json-path"). The specified search query + is a `JsonPath <https://github.com/json-path/JsonPath>`_ expression. + If at least one path of the Solidity AST matches the query, the bug is + likely present. .. literalinclude:: bugs.json :language: js diff --git a/test/buglistTests.js b/test/buglistTests.js index 6b7df2f2..f24f0cb6 100755 --- a/test/buglistTests.js +++ b/test/buglistTests.js @@ -2,6 +2,11 @@ "use strict"; +var util = require('util') +var exec = util.promisify(require('child_process').exec) +var mktemp = require('mktemp'); +var download = require('download') +var JSONPath = require('JSONPath') var fs = require('fs') var bugs = JSON.parse(fs.readFileSync(__dirname + '/../docs/bugs.json', 'utf8')) @@ -19,27 +24,111 @@ var tests = fs.readFileSync(__dirname + '/buglist_test_vectors.md', 'utf8') var testVectorParser = /\s*#\s+(\S+)\s+## buggy\n([^#]*)## fine\n([^#]*)/g -var result; -while ((result = testVectorParser.exec(tests)) !== null) +runTests() + +async function runTests() { - var name = result[1] - var buggy = result[2].split('\n--\n') - var fine = result[3].split('\n--\n') - console.log("Testing " + name + " with " + buggy.length + " buggy and " + fine.length + " fine instances") + var result; + while ((result = testVectorParser.exec(tests)) !== null) + { + var name = result[1] + var buggy = result[2].split('\n--\n') + var fine = result[3].split('\n--\n') + console.log("Testing " + name + " with " + buggy.length + " buggy and " + fine.length + " fine instances") - var regex = RegExp(bugsByName[name].check['regex-source']) - for (var i in buggy) - { - if (!regex.exec(buggy[i])) - { - throw "Bug " + name + ": Buggy source does not match: " + buggy[i] - } - } - for (var i in fine) - { - if (regex.exec(fine[i])) - { - throw "Bug " + name + ": Non-buggy source matches: " + fine[i] - } - } + try { + await checkRegex(name, buggy, fine) + await checkJSONPath(name, buggy, fine) + } catch (err) { + console.error("Error: " + err) + } + } +} + +function checkRegex(name, buggy, fine) +{ + return new Promise(function(resolve, reject) { + var regexStr = bugsByName[name].check['regex-source'] + if (regexStr !== undefined) + { + var regex = RegExp(regexStr) + for (var i in buggy) + { + if (!regex.exec(buggy[i])) + { + reject("Bug " + name + ": Buggy source does not match: " + buggy[i]) + } + } + for (var i in fine) + { + if (regex.exec(fine[i])) + { + reject("Bug " + name + ": Non-buggy source matches: " + fine[i]) + } + } + } + resolve() + }) +} + +async function checkJSONPath(name, buggy, fine) +{ + var jsonPath = bugsByName[name].check['ast-compact-json-path'] + if (jsonPath !== undefined) + { + var url = "http://github.com/ethereum/solidity/releases/download/v" + bugsByName[name].introduced + "/solc-static-linux" + try { + var tmpdir = await mktemp.createDir('XXXXX') + var binary = tmpdir + "/solc-static-linux" + await download(url, tmpdir) + exec("chmod +x " + binary) + for (var i in buggy) + { + var result = await checkJsonPathTest(buggy[i], tmpdir, binary, jsonPath, i) + if (!result) + throw "Bug " + name + ": Buggy source does not contain path: " + buggy[i] + } + for (var i in fine) + { + var result = await checkJsonPathTest(fine[i], tmpdir, binary, jsonPath, i + buggy.length) + if (result) + throw "Bug " + name + ": Non-buggy source contains path: " + fine[i] + } + exec("rm -r " + tmpdir) + } catch (err) { + throw err + } + } +} + +function checkJsonPathTest(code, tmpdir, binary, query, idx) { + return new Promise(function(resolve, reject) { + var solFile = tmpdir + "/jsonPath" + idx + ".sol" + var astFile = tmpdir + "/ast" + idx + ".json" + writeFilePromise(solFile, code) + .then(() => { + return exec(binary + " --ast-compact-json " + solFile + " > " + astFile) + }) + .then(() => { + var jsonRE = /(\{[\s\S]*\})/ + var ast = JSON.parse(jsonRE.exec(fs.readFileSync(astFile, 'utf8'))[0]) + var result = JSONPath({json: ast, path: query}) + if (result.length > 0) + resolve(true) + else + resolve(false) + }) + .catch((err) => { + reject(err) + }) + }) +} + +function writeFilePromise(filename, data) { + return new Promise(function(resolve, reject) { + fs.writeFile(filename, data, 'utf8', function(err) { + if (err) reject(err) + else resolve(data) + }) + }) } diff --git a/test/buglist_test_vectors.md b/test/buglist_test_vectors.md index ce95403b..f15cf151 100644 --- a/test/buglist_test_vectors.md +++ b/test/buglist_test_vectors.md @@ -67,3 +67,48 @@ function f() m(uint[2][2]) { } -- function f() returns (uint, uint) { uint[2][2] memory x; } + +# EventStructWrongData + +## buggy + +pragma experimental ABIEncoderV2; +contract C +{ + struct S { uint x; } + event E(S); + event F(S); + enum A { B, C } + event G(A); + function f(S s); +} + +-- + +pragma experimental ABIEncoderV2; +contract C +{ + struct S { uint x; } + event E(S indexed); + event F(uint, S, bool); +} + +## fine + +pragma experimental ABIEncoderV2; +contract C +{ + struct S { uint x; } + enum A { B, C } + event G(A); +} + +-- + +pragma experimental ABIEncoderV2; +contract C +{ + struct S { uint x; } + function f(S s); + S s1; +} |