Skip to content

Commit

Permalink
Static Analysis: forgotten return statements (ethereum#866)
Browse files Browse the repository at this point in the history
  • Loading branch information
soad003 authored and axic committed Nov 21, 2017
1 parent a8fc820 commit 57a496a
Show file tree
Hide file tree
Showing 6 changed files with 177 additions and 19 deletions.
12 changes: 11 additions & 1 deletion src/app/staticanalysis/modules/abstractAstView.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion src/app/staticanalysis/modules/list.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ module.exports = [
require('./inlineAssembly'),
require('./blockTimestamp'),
require('./lowLevelCalls'),
require('./blockBlockhash')
require('./blockBlockhash'),
require('./noReturn')
]
73 changes: 73 additions & 0 deletions src/app/staticanalysis/modules/noReturn.js
Original file line number Diff line number Diff line change
@@ -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
}
29 changes: 24 additions & 5 deletions src/app/staticanalysis/modules/staticAnalysisCommon.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ var nodeTypes = {
USERDEFINEDTYPENAME: 'UserDefinedTypeName',
INLINEASSEMBLY: 'InlineAssembly',
BLOCK: 'Block',
NEWEXPRESSION: 'NewExpression'
NEWEXPRESSION: 'NewExpression',
RETURN: 'Return'
}

var basicTypes = {
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand All @@ -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))
}
Expand All @@ -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:
Expand Down Expand Up @@ -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))
}
Expand Down Expand Up @@ -742,6 +759,7 @@ module.exports = {
getFullQuallyfiedFuncDefinitionIdent: getFullQuallyfiedFuncDefinitionIdent,
getStateVariableDeclarationsFormContractNode: getStateVariableDeclarationsFormContractNode,
getFunctionOrModifierDefinitionParameterPart: getFunctionOrModifierDefinitionParameterPart,
getFunctionOrModifierDefinitionReturnParameterPart: getFunctionOrModifierDefinitionReturnParameterPart,

// #################### Complex Node Identification
hasFunctionBody: hasFunctionBody,
Expand Down Expand Up @@ -783,6 +801,7 @@ module.exports = {
isConstantFunction: isConstantFunction,
isInlineAssembly: isInlineAssembly,
isNewExpression: isNewExpression,
isReturn: isReturn,

// #################### Constants
nodeTypes: nodeTypes,
Expand Down
66 changes: 54 additions & 12 deletions test/staticanalysis/staticAnalysisIntegration-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ var testFiles = [
'globals.sol',
'library.sol',
'transfer.sol',
'ctor.sol'
'ctor.sol',
'forgottenReturn.sol'
]

var testFileAsts = {}
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -348,14 +359,45 @@ 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) => {
t.equal(report.length, lengthCheck[file], `${file} has right amount of blockBlockhash warnings`)
})
})

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()
Expand Down
13 changes: 13 additions & 0 deletions test/staticanalysis/test-contracts/forgottenReturn.sol
Original file line number Diff line number Diff line change
@@ -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;
}
}

0 comments on commit 57a496a

Please sign in to comment.