From 59ec63778bc1b1f4dfeed0378e9d0f6dcdb3983b Mon Sep 17 00:00:00 2001 From: soad003 Date: Tue, 21 Nov 2017 15:14:12 +0100 Subject: [PATCH] Static Analysis: forgotten return statements (#866) --- .../staticanalysis/modules/abstractAstView.js | 12 ++- src/app/staticanalysis/modules/list.js | 3 +- src/app/staticanalysis/modules/noReturn.js | 73 +++++++++++++++++++ .../modules/staticAnalysisCommon.js | 29 ++++++-- .../staticAnalysisIntegration-test.js | 66 ++++++++++++++--- .../test-contracts/forgottenReturn.sol | 13 ++++ 6 files changed, 177 insertions(+), 19 deletions(-) create mode 100644 src/app/staticanalysis/modules/noReturn.js create mode 100644 test/staticanalysis/test-contracts/forgottenReturn.sol diff --git a/src/app/staticanalysis/modules/abstractAstView.js b/src/app/staticanalysis/modules/abstractAstView.js index be4853b6d7..ead9dfc3eb 100644 --- a/src/app/staticanalysis/modules/abstractAstView.js +++ b/src/app/staticanalysis/modules/abstractAstView.js @@ -64,7 +64,8 @@ abstractAstView.prototype.build_visit = function (relevantNodeFilter) { relevantNodes: [], modifierInvocations: [], localVariables: getLocalVariables(node), - parameters: getLocalParameters(node) + parameters: getLocalParameters(node), + returns: getReturnParameters(node) }) // push back relevant nodes to their the current fn if any getCurrentContract(that).relevantNodes.map((item) => { @@ -155,6 +156,15 @@ function getLocalParameters (funcNode) { return getLocalVariables(common.getFunctionOrModifierDefinitionParameterPart(funcNode)).map(common.getType) } +function getReturnParameters (funcNode) { + return getLocalVariables(common.getFunctionOrModifierDefinitionReturnParameterPart(funcNode)).map((n) => { + return { + type: common.getType(n), + name: common.getDeclaredVariableName(n) + } + }) +} + function getLocalVariables (funcNode) { var locals = [] new AstWalker().walk(funcNode, {'*': function (node) { diff --git a/src/app/staticanalysis/modules/list.js b/src/app/staticanalysis/modules/list.js index f525767168..417d9b363c 100644 --- a/src/app/staticanalysis/modules/list.js +++ b/src/app/staticanalysis/modules/list.js @@ -8,5 +8,6 @@ module.exports = [ require('./inlineAssembly'), require('./blockTimestamp'), require('./lowLevelCalls'), - require('./blockBlockhash') + require('./blockBlockhash'), + require('./noReturn') ] diff --git a/src/app/staticanalysis/modules/noReturn.js b/src/app/staticanalysis/modules/noReturn.js new file mode 100644 index 0000000000..dbd212ead5 --- /dev/null +++ b/src/app/staticanalysis/modules/noReturn.js @@ -0,0 +1,73 @@ +var name = 'no return: ' +var desc = 'Function with return type is not returning' +var categories = require('./categories') +var common = require('./staticAnalysisCommon') +var AbstractAst = require('./abstractAstView') + +function noReturn () { + this.abstractAst = new AbstractAst() + + this.visit = this.abstractAst.build_visit( + (node) => common.isReturn(node) || common.isAssignment(node) + ) + + this.report = this.abstractAst.build_report(report) +} + +noReturn.prototype.visit = function () { throw new Error('noReturn.js no visit function set upon construction') } + +noReturn.prototype.report = function () { throw new Error('noReturn.js no report function set upon construction') } + +function report (contracts, multipleContractsWithSameName) { + var warnings = [] + + contracts.forEach((contract) => { + contract.functions.filter((func) => common.hasFunctionBody(func.node)).forEach((func) => { + var funcName = common.getFullQuallyfiedFuncDefinitionIdent(contract.node, func.node, func.parameters) + if (hasNamedAndUnnamedReturns(func)) { + warnings.push({ + warning: `${funcName}: Mixing of named and unnamed return parameters is not advised.`, + location: func.src + }) + } else if (shouldReturn(func) && !(hasReturnStatement(func) || (hasNamedReturns(func) && hasAssignToAllNamedReturns(func)))) { + warnings.push({ + warning: `${funcName}: Defines a return type but never explicitly returns a value.`, + location: func.src + }) + } + }) + }) + + return warnings +} + +function shouldReturn (func) { + return func.returns.length > 0 +} + +function hasReturnStatement (func) { + return func.relevantNodes.filter(common.isReturn).length > 0 +} + +function hasAssignToAllNamedReturns (func) { + var namedReturns = func.returns.filter((n) => n.name.length > 0).map((n) => n.name) + var assignedVars = func.relevantNodes.filter(common.isAssignment).map(common.getEffectedVariableName) + var diff = namedReturns.filter(e => !assignedVars.includes(e)) + return diff.length === 0 +} + +function hasNamedReturns (func) { + return func.returns.filter((n) => n.name.length > 0).length > 0 +} + +function hasNamedAndUnnamedReturns (func) { + return func.returns.filter((n) => n.name.length === 0).length > 0 && + hasNamedReturns(func) +} + +module.exports = { + name: name, + description: desc, + category: categories.MISC, + Module: noReturn +} diff --git a/src/app/staticanalysis/modules/staticAnalysisCommon.js b/src/app/staticanalysis/modules/staticAnalysisCommon.js index b9ec093bb6..41238f64bb 100644 --- a/src/app/staticanalysis/modules/staticAnalysisCommon.js +++ b/src/app/staticanalysis/modules/staticAnalysisCommon.js @@ -18,7 +18,8 @@ var nodeTypes = { USERDEFINEDTYPENAME: 'UserDefinedTypeName', INLINEASSEMBLY: 'InlineAssembly', BLOCK: 'Block', - NEWEXPRESSION: 'NewExpression' + NEWEXPRESSION: 'NewExpression', + RETURN: 'Return' } var basicTypes = { @@ -212,7 +213,7 @@ function getFunctionDefinitionName (funcDef) { * @return {string} name of contract inherited from */ function getInheritsFromName (inheritsNode) { - if (!isInheritanceSpecifier(inheritsNode)) throw new Error('staticAnalysisCommon.js: not an InheritanceSpecifier node Node') + if (!isInheritanceSpecifier(inheritsNode)) throw new Error('staticAnalysisCommon.js: not an InheritanceSpecifier Node') return inheritsNode.children[0].attributes.name } @@ -224,7 +225,7 @@ function getInheritsFromName (inheritsNode) { * @return {string} variable name */ function getDeclaredVariableName (varDeclNode) { - if (!isVariableDeclaration(varDeclNode)) throw new Error('staticAnalysisCommon.js: not an variable declaration') + if (!isVariableDeclaration(varDeclNode)) throw new Error('staticAnalysisCommon.js: not a variable declaration') return varDeclNode.attributes.name } @@ -239,7 +240,7 @@ function getDeclaredVariableName (varDeclNode) { * @return {list variable declaration} state variable node list */ function getStateVariableDeclarationsFormContractNode (contractNode) { - if (!isContractDefinition(contractNode)) throw new Error('staticAnalysisCommon.js: not an contract definition declaration') + if (!isContractDefinition(contractNode)) throw new Error('staticAnalysisCommon.js: not a contract definition declaration') if (!contractNode.children) return [] return contractNode.children.filter((el) => isVariableDeclaration(el)) } @@ -252,10 +253,22 @@ function getStateVariableDeclarationsFormContractNode (contractNode) { * @return {parameterlist node} parameterlist node */ function getFunctionOrModifierDefinitionParameterPart (funcNode) { - if (!isFunctionDefinition(funcNode) && !isModifierDefinition(funcNode)) throw new Error('staticAnalysisCommon.js: not an function definition') + if (!isFunctionDefinition(funcNode) && !isModifierDefinition(funcNode)) throw new Error('staticAnalysisCommon.js: not a function definition') return funcNode.children[0] } +/** + * Returns return parameter node for a function or modifier definition, Throws on wrong node. + * Example: + * function bar(uint a, uint b) returns (bool a, bool b) => bool a, bool b + * @funcNode {ASTNode} Contract Definition node + * @return {parameterlist node} parameterlist node + */ +function getFunctionOrModifierDefinitionReturnParameterPart (funcNode) { + if (!isFunctionDefinition(funcNode) && !isModifierDefinition(funcNode)) throw new Error('staticAnalysisCommon.js: not a function definition') + return funcNode.children[1] +} + /** * Extracts the parameter types for a function type signature * Example: @@ -340,6 +353,10 @@ function isVariableDeclaration (node) { return nodeType(node, exactMatch(nodeTypes.VARIABLEDECLARATION)) } +function isReturn (node) { + return nodeType(node, exactMatch(nodeTypes.RETURN)) +} + function isInheritanceSpecifier (node) { return nodeType(node, exactMatch(nodeTypes.INHERITANCESPECIFIER)) } @@ -742,6 +759,7 @@ module.exports = { getFullQuallyfiedFuncDefinitionIdent: getFullQuallyfiedFuncDefinitionIdent, getStateVariableDeclarationsFormContractNode: getStateVariableDeclarationsFormContractNode, getFunctionOrModifierDefinitionParameterPart: getFunctionOrModifierDefinitionParameterPart, + getFunctionOrModifierDefinitionReturnParameterPart: getFunctionOrModifierDefinitionReturnParameterPart, // #################### Complex Node Identification hasFunctionBody: hasFunctionBody, @@ -783,6 +801,7 @@ module.exports = { isConstantFunction: isConstantFunction, isInlineAssembly: isInlineAssembly, isNewExpression: isNewExpression, + isReturn: isReturn, // #################### Constants nodeTypes: nodeTypes, diff --git a/test/staticanalysis/staticAnalysisIntegration-test.js b/test/staticanalysis/staticAnalysisIntegration-test.js index 7009bd4590..c1021b6b9d 100644 --- a/test/staticanalysis/staticAnalysisIntegration-test.js +++ b/test/staticanalysis/staticAnalysisIntegration-test.js @@ -27,7 +27,8 @@ var testFiles = [ 'globals.sol', 'library.sol', 'transfer.sol', - 'ctor.sol' + 'ctor.sol', + 'forgottenReturn.sol' ] var testFileAsts = {} @@ -58,7 +59,8 @@ test('Integration test thisLocal.js', function (t) { 'globals.sol': 0, 'library.sol': 0, 'transfer.sol': 0, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -87,7 +89,8 @@ test('Integration test checksEffectsInteraction.js', function (t) { 'globals.sol': 1, 'library.sol': 1, 'transfer.sol': 1, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -116,7 +119,8 @@ test('Integration test constantFunctions.js', function (t) { 'globals.sol': 0, 'library.sol': 1, 'transfer.sol': 0, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -145,7 +149,8 @@ test('Integration test inlineAssembly.js', function (t) { 'globals.sol': 0, 'library.sol': 0, 'transfer.sol': 0, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -174,7 +179,8 @@ test('Integration test txOrigin.js', function (t) { 'globals.sol': 1, 'library.sol': 0, 'transfer.sol': 0, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -203,7 +209,8 @@ test('Integration test gasCosts.js', function (t) { 'globals.sol': 1, 'library.sol': 1, 'transfer.sol': 1, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 3 } runModuleOnFiles(module, t, (file, report) => { @@ -232,7 +239,8 @@ test('Integration test similarVariableNames.js', function (t) { 'globals.sol': 0, 'library.sol': 0, 'transfer.sol': 0, - 'ctor.sol': 1 + 'ctor.sol': 1, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -261,7 +269,8 @@ test('Integration test inlineAssembly.js', function (t) { 'globals.sol': 0, 'library.sol': 0, 'transfer.sol': 0, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -290,7 +299,8 @@ test('Integration test blockTimestamp.js', function (t) { 'globals.sol': 2, 'library.sol': 0, 'transfer.sol': 0, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -319,7 +329,8 @@ test('Integration test lowLevelCalls.js', function (t) { 'globals.sol': 1, 'library.sol': 1, 'transfer.sol': 0, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -348,7 +359,8 @@ test('Integration test blockBlockhash.js', function (t) { 'globals.sol': 0, // was 1 !! @TODO 'library.sol': 0, 'transfer.sol': 0, - 'ctor.sol': 0 + 'ctor.sol': 0, + 'forgottenReturn.sol': 0 } runModuleOnFiles(module, t, (file, report) => { @@ -356,6 +368,36 @@ test('Integration test blockBlockhash.js', function (t) { }) }) +test('Integration test noReturn.js', function (t) { + t.plan(testFiles.length) + + var module = require('../../src/app/staticanalysis/modules/noReturn') + + var lengthCheck = { + 'KingOfTheEtherThrone.sol': 0, + 'assembly.sol': 1, + 'ballot.sol': 0, + 'ballot_reentrant.sol': 0, + 'ballot_withoutWarnings.sol': 0, + 'cross_contract.sol': 0, + 'inheritance.sol': 0, + 'modifier1.sol': 1, + 'modifier2.sol': 0, + 'notReentrant.sol': 0, + 'structReentrant.sol': 0, + 'thisLocal.sol': 1, + 'globals.sol': 0, + 'library.sol': 0, + 'transfer.sol': 0, + 'ctor.sol': 0, + 'forgottenReturn.sol': 1 + } + + runModuleOnFiles(module, t, (file, report) => { + t.equal(report.length, lengthCheck[file], `${file} has right amount of noReturn warnings`) + }) +}) + // #################### Helpers function runModuleOnFiles (module, t, cb) { var statRunner = new StatRunner() diff --git a/test/staticanalysis/test-contracts/forgottenReturn.sol b/test/staticanalysis/test-contracts/forgottenReturn.sol new file mode 100644 index 0000000000..eb3df75e44 --- /dev/null +++ b/test/staticanalysis/test-contracts/forgottenReturn.sol @@ -0,0 +1,13 @@ +contract Sheep { + string public name; + string public dna; + bool g = true; + function Sheep(string _name, string _dna) { + name = _name; + dna = _dna; + } + + function geneticallyEngineer(string _dna) returns (bool) { + dna = _dna; + } +} \ No newline at end of file