From 9c403928a8ee908a53c469406e7d47687b3fe942 Mon Sep 17 00:00:00 2001 From: Esorat Date: Mon, 18 Nov 2024 14:36:51 +0700 Subject: [PATCH 01/10] Fix: SendInLoop Interprocedural Improvement --- src/detectors/builtin/sendInLoop.ts | 107 ++++++++++++++++++++++++++-- src/internals/ir/callGraph.ts | 2 +- 2 files changed, 103 insertions(+), 6 deletions(-) diff --git a/src/detectors/builtin/sendInLoop.ts b/src/detectors/builtin/sendInLoop.ts index 6945cc04..e3ef488f 100644 --- a/src/detectors/builtin/sendInLoop.ts +++ b/src/detectors/builtin/sendInLoop.ts @@ -1,8 +1,11 @@ import { CompilationUnit } from "../../internals/ir"; +import { CallGraph } from "../../internals/ir/callGraph"; +import { CGNodeId } from "../../internals/ir/callGraph"; // Import CGNodeId type import { forEachStatement, foldExpressions, isSelf, + forEachExpression, } from "../../internals/tact"; import { MistiTactWarning, Severity } from "../../internals/warnings"; import { ASTDetector } from "../detector"; @@ -10,6 +13,11 @@ import { AstStatement, AstExpression, idText, + AstFunctionDef, + AstReceiver, + AstContractInit, + AstStaticCall, + AstMethodCall, } from "@tact-lang/compiler/dist/grammar/ast"; /** @@ -43,13 +51,53 @@ export class SendInLoop extends ASTDetector { async check(cu: CompilationUnit): Promise { const processedLoopIds = new Set(); const allWarnings: MistiTactWarning[] = []; + const callGraph = new CallGraph(this.ctx); + callGraph.build(cu.ast); + const nodeNameToIdMap = new Map(); + for (const [nodeId, node] of callGraph.getNodes()) { + nodeNameToIdMap.set(node.name, nodeId); + } - Array.from(cu.ast.getProgramEntries()).forEach((node) => { + // Identify all functions that directly call send functions + const functionsThatCallSend = new Set(); + for (const node of cu.ast.getProgramEntries()) { + if ( + node.kind === "function_def" || + node.kind === "receiver" || + node.kind === "contract_init" + ) { + const func = node as AstFunctionDef | AstReceiver | AstContractInit; + let callsSend = false; + forEachExpression(func, (expr) => { + if (this.isSendCall(expr)) { + callsSend = true; + } + }); + if (callsSend) { + const functionName = this.getFunctionName(func); + if (functionName) { + const nodeId = nodeNameToIdMap.get(functionName); + if (nodeId !== undefined) { + functionsThatCallSend.add(nodeId); + } + } + } + } + } + + // Analyze the AST to find loops and check for function calls inside loops + for (const node of cu.ast.getProgramEntries()) { forEachStatement(node, (stmt) => { - const warnings = this.analyzeStatement(stmt, processedLoopIds); + const warnings = this.analyzeStatement( + stmt, + processedLoopIds, + functionsThatCallSend, + nodeNameToIdMap, + callGraph, + ); allWarnings.push(...warnings); }); - }); + } return allWarnings; } @@ -57,13 +105,17 @@ export class SendInLoop extends ASTDetector { private analyzeStatement( stmt: AstStatement, processedLoopIds: Set, + functionsThatCallSend: Set, + nodeNameToIdMap: Map, + callGraph: CallGraph, ): MistiTactWarning[] { if (processedLoopIds.has(stmt.id)) { return []; } if (this.isLoop(stmt)) { processedLoopIds.add(stmt.id); - return foldExpressions( + const warnings: MistiTactWarning[] = []; + foldExpressions( stmt, (acc, expr) => { if (this.isSendCall(expr)) { @@ -73,11 +125,41 @@ export class SendInLoop extends ASTDetector { "Consider refactoring to avoid calling send functions inside loops", }), ); + } else if ( + expr.kind === "static_call" || + expr.kind === "method_call" + ) { + // It's a function call + const functionName = + expr.kind === "static_call" + ? idText((expr as AstStaticCall).function) + : idText((expr as AstMethodCall).method); + // Node ID from the mapping + const nodeId = nodeNameToIdMap.get(functionName); + if (nodeId !== undefined) { + // Check if this function can reach any function that calls send + for (const sendFuncId of functionsThatCallSend) { + if (callGraph.areConnected(nodeId, sendFuncId)) { + acc.push( + this.makeWarning( + `Function "${functionName}" called inside a loop may eventually call a send function`, + expr.loc, + { + suggestion: + "Consider refactoring to avoid calling send functions inside loops", + }, + ), + ); + break; + } + } + } } return acc; }, - [] as MistiTactWarning[], + warnings, ); + return warnings; } // If the statement is not a loop, don't flag anything return []; @@ -103,4 +185,19 @@ export class SendInLoop extends ASTDetector { stmt.kind === "statement_foreach" ); } + + private getFunctionName( + func: AstFunctionDef | AstReceiver | AstContractInit, + ): string | undefined { + switch (func.kind) { + case "function_def": + return func.name?.text; + case "contract_init": + return `contract_init_${func.id}`; + case "receiver": + return `receiver_${func.id}`; + default: + return undefined; + } + } } diff --git a/src/internals/ir/callGraph.ts b/src/internals/ir/callGraph.ts index 623fe741..706b3e2a 100644 --- a/src/internals/ir/callGraph.ts +++ b/src/internals/ir/callGraph.ts @@ -13,7 +13,7 @@ import { AstStaticCall, } from "@tact-lang/compiler/dist/grammar/ast"; -type CGNodeId = number & { readonly brand: unique symbol }; +export type CGNodeId = number & { readonly brand: unique symbol }; type CGEdgeId = number & { readonly brand: unique symbol }; /** From 8b0f244832b7ca9ff8bf37d47f27d272288c1f39 Mon Sep 17 00:00:00 2001 From: Esorat Date: Mon, 18 Nov 2024 14:39:52 +0700 Subject: [PATCH 02/10] chore: yarn fix-all --- src/detectors/builtin/sendInLoop.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/detectors/builtin/sendInLoop.ts b/src/detectors/builtin/sendInLoop.ts index e3ef488f..2577987c 100644 --- a/src/detectors/builtin/sendInLoop.ts +++ b/src/detectors/builtin/sendInLoop.ts @@ -185,7 +185,7 @@ export class SendInLoop extends ASTDetector { stmt.kind === "statement_foreach" ); } - + private getFunctionName( func: AstFunctionDef | AstReceiver | AstContractInit, ): string | undefined { From 85d0eb014a68a286a6af76fe2c4c4d9ed07291a3 Mon Sep 17 00:00:00 2001 From: Esorat Date: Tue, 19 Nov 2024 00:13:42 +0700 Subject: [PATCH 03/10] Fix: SendInLoop, update callgraph, add tests for expected output --- src/detectors/builtin/sendInLoop.ts | 198 ++++++++++++++++--------- src/internals/ir/callGraph.ts | 63 ++++---- test/detectors/SendInLoop.expected.out | 73 +++++---- test/detectors/SendInLoop.tact | 77 +++++++--- 4 files changed, 258 insertions(+), 153 deletions(-) diff --git a/src/detectors/builtin/sendInLoop.ts b/src/detectors/builtin/sendInLoop.ts index 2577987c..fb13f3c4 100644 --- a/src/detectors/builtin/sendInLoop.ts +++ b/src/detectors/builtin/sendInLoop.ts @@ -1,12 +1,11 @@ import { CompilationUnit } from "../../internals/ir"; -import { CallGraph } from "../../internals/ir/callGraph"; -import { CGNodeId } from "../../internals/ir/callGraph"; // Import CGNodeId type +import { CallGraph, CGNodeId } from "../../internals/ir/callGraph"; import { forEachStatement, foldExpressions, isSelf, - forEachExpression, } from "../../internals/tact"; +import { unreachable } from "../../internals/util"; import { MistiTactWarning, Severity } from "../../internals/warnings"; import { ASTDetector } from "../detector"; import { @@ -16,12 +15,12 @@ import { AstFunctionDef, AstReceiver, AstContractInit, - AstStaticCall, - AstMethodCall, + AstNode, } from "@tact-lang/compiler/dist/grammar/ast"; /** - * An optional detector that identifies send functions being called inside loops. + * An optional detector that identifies send functions being called inside loops, + * including indirect calls via other functions. * * ## Why is it bad? * Calling send functions inside loops can lead to unintended consequences, such as @@ -51,53 +50,63 @@ export class SendInLoop extends ASTDetector { async check(cu: CompilationUnit): Promise { const processedLoopIds = new Set(); const allWarnings: MistiTactWarning[] = []; - const callGraph = new CallGraph(this.ctx); - callGraph.build(cu.ast); - const nodeNameToIdMap = new Map(); + const astStore = cu.ast; + const ctx = this.ctx; + const callGraph = new CallGraph(ctx).build(astStore); + const astIdToCGNodeId = new Map(); for (const [nodeId, node] of callGraph.getNodes()) { - nodeNameToIdMap.set(node.name, nodeId); + if (node.astId !== undefined) { + astIdToCGNodeId.set(node.astId, nodeId); + } } - // Identify all functions that directly call send functions - const functionsThatCallSend = new Set(); - for (const node of cu.ast.getProgramEntries()) { - if ( - node.kind === "function_def" || - node.kind === "receiver" || - node.kind === "contract_init" - ) { - const func = node as AstFunctionDef | AstReceiver | AstContractInit; - let callsSend = false; - forEachExpression(func, (expr) => { + // Collect functions that directly call send functions + const functionsCallingSend = new Set(); + + // Identify all functions that contain a send call + for (const func of astStore.getFunctions()) { + let containsSend = false; + foldExpressions( + func, + (acc, expr) => { if (this.isSendCall(expr)) { - callsSend = true; + containsSend = true; } - }); - if (callsSend) { - const functionName = this.getFunctionName(func); - if (functionName) { - const nodeId = nodeNameToIdMap.get(functionName); - if (nodeId !== undefined) { - functionsThatCallSend.add(nodeId); - } + return acc; + }, + null, + ); + + if (containsSend) { + const funcName = this.getFunctionName(func); + if (funcName) { + const nodeId = callGraph.getNodeIdByName(funcName); + if (nodeId !== undefined) { + functionsCallingSend.add(nodeId); } } } } - // Analyze the AST to find loops and check for function calls inside loops - for (const node of cu.ast.getProgramEntries()) { + // Identify all functions that can lead to a send call + const functionsLeadingToSend = this.getFunctionsLeadingToSend( + callGraph, + functionsCallingSend, + ); + + // Analyze loops and check if any function called within leads to a send + Array.from(cu.ast.getProgramEntries()).forEach((node) => { forEachStatement(node, (stmt) => { const warnings = this.analyzeStatement( stmt, processedLoopIds, - functionsThatCallSend, - nodeNameToIdMap, callGraph, + astIdToCGNodeId, + functionsLeadingToSend, ); allWarnings.push(...warnings); }); - } + }); return allWarnings; } @@ -105,19 +114,22 @@ export class SendInLoop extends ASTDetector { private analyzeStatement( stmt: AstStatement, processedLoopIds: Set, - functionsThatCallSend: Set, - nodeNameToIdMap: Map, callGraph: CallGraph, + astIdToCGNodeId: Map, + functionsLeadingToSend: Set, ): MistiTactWarning[] { if (processedLoopIds.has(stmt.id)) { return []; } if (this.isLoop(stmt)) { processedLoopIds.add(stmt.id); + const warnings: MistiTactWarning[] = []; + + // Check direct send calls within the loop foldExpressions( stmt, - (acc, expr) => { + (acc: MistiTactWarning[], expr: AstExpression) => { if (this.isSendCall(expr)) { acc.push( this.makeWarning("Send function called inside a loop", expr.loc, { @@ -125,46 +137,72 @@ export class SendInLoop extends ASTDetector { "Consider refactoring to avoid calling send functions inside loops", }), ); - } else if ( - expr.kind === "static_call" || - expr.kind === "method_call" - ) { - // It's a function call - const functionName = - expr.kind === "static_call" - ? idText((expr as AstStaticCall).function) - : idText((expr as AstMethodCall).method); - // Node ID from the mapping - const nodeId = nodeNameToIdMap.get(functionName); - if (nodeId !== undefined) { - // Check if this function can reach any function that calls send - for (const sendFuncId of functionsThatCallSend) { - if (callGraph.areConnected(nodeId, sendFuncId)) { - acc.push( - this.makeWarning( - `Function "${functionName}" called inside a loop may eventually call a send function`, - expr.loc, - { - suggestion: - "Consider refactoring to avoid calling send functions inside loops", - }, - ), - ); - break; - } - } - } } return acc; }, warnings, ); + + // Check function calls within the loop that lead to send + this.forEachExpression(stmt, (expr: AstExpression) => { + if (expr.kind === "static_call" || expr.kind === "method_call") { + const calleeName = this.getCalleeName(expr); + if (calleeName) { + const calleeNodeId = callGraph.getNodeIdByName(calleeName); + if ( + calleeNodeId !== undefined && + functionsLeadingToSend.has(calleeNodeId) + ) { + warnings.push( + this.makeWarning( + `Function "${calleeName}" called inside a loop leads to a send function`, + expr.loc, + { + suggestion: + "Consider refactoring to avoid calling send functions inside loops", + }, + ), + ); + } + } + } + }); + return warnings; } // If the statement is not a loop, don't flag anything return []; } + private getFunctionsLeadingToSend( + callGraph: CallGraph, + functionsCallingSend: Set, + ): Set { + const functionsLeadingToSend = new Set(functionsCallingSend); + + // Use a queue for BFS + const queue: CGNodeId[] = Array.from(functionsCallingSend); + + while (queue.length > 0) { + const current = queue.shift()!; + const currentNode = callGraph.getNode(current); + if (currentNode) { + for (const edgeId of currentNode.inEdges) { + const edge = callGraph.getEdge(edgeId); + if (edge) { + const callerId = edge.src; + if (!functionsLeadingToSend.has(callerId)) { + functionsLeadingToSend.add(callerId); + queue.push(callerId); + } + } + } + } + } + + return functionsLeadingToSend; + } + private isSendCall(expr: AstExpression): boolean { const staticSendFunctions = ["send", "nativeSendMessage"]; const selfMethodSendFunctions = ["reply", "forward", "notify", "emit"]; @@ -197,7 +235,31 @@ export class SendInLoop extends ASTDetector { case "receiver": return `receiver_${func.id}`; default: - return undefined; + unreachable(func); } } + + private getCalleeName(expr: AstExpression): string | undefined { + if (expr.kind === "static_call") { + return idText(expr.function); + } else if (expr.kind === "method_call") { + return idText(expr.method); + } + return undefined; + } + + // Helper method to traverse expressions + private forEachExpression( + node: AstNode, + callback: (expr: AstExpression) => void, + ): void { + foldExpressions( + node, + (acc, expr) => { + callback(expr); + return acc; + }, + null, + ); + } } diff --git a/src/internals/ir/callGraph.ts b/src/internals/ir/callGraph.ts index 706b3e2a..56b605ea 100644 --- a/src/internals/ir/callGraph.ts +++ b/src/internals/ir/callGraph.ts @@ -80,6 +80,33 @@ export class CallGraph { return this.edgesMap; } + /** + * Retrieves the node ID associated with a given function name. + * @param name The function name. + * @returns The corresponding node ID, or undefined if not found. + */ + public getNodeIdByName(name: string): CGNodeId | undefined { + return this.nameToNodeId.get(name); + } + + /** + * Retrieves a node from the graph by its ID. + * @param nodeId The ID of the node. + * @returns The `CGNode` instance, or undefined if not found. + */ + public getNode(nodeId: CGNodeId): CGNode | undefined { + return this.nodeMap.get(nodeId); + } + + /** + * Retrieves an edge from the graph by its ID. + * @param edgeId The ID of the edge. + * @returns The `CGEdge` instance, or undefined if not found. + */ + public getEdge(edgeId: CGEdgeId): CGEdge | undefined { + return this.edgesMap.get(edgeId); + } + /** * Builds the call graph based on functions in the provided AST store. * @param astStore - The AST store containing functions to be added to the graph. @@ -102,42 +129,6 @@ export class CallGraph { return this; } - /** - * Determines if there exists a path in the call graph from the source node to the destination node. - * This method performs a breadth-first search to find if the destination node is reachable from the source node. - * - * @param src The ID of the source node to start the search from - * @param dst The ID of the destination node to search for - * @returns true if there exists a path from src to dst in the call graph, false otherwise - * Returns false if either src or dst node IDs are not found in the graph - */ - public areConnected(src: CGNodeId, dst: CGNodeId): boolean { - const srcNode = this.nodeMap.get(src); - const dstNode = this.nodeMap.get(dst); - if (!srcNode || !dstNode) { - return false; - } - const queue: CGNodeId[] = [src]; - const visited = new Set([src]); - while (queue.length > 0) { - const current = queue.shift()!; - if (current === dst) { - return true; - } - const currentNode = this.nodeMap.get(current); - if (currentNode) { - for (const edgeId of currentNode.outEdges) { - const edge = this.edgesMap.get(edgeId); - if (edge && !visited.has(edge.dst)) { - visited.add(edge.dst); - queue.push(edge.dst); - } - } - } - } - return false; - } - /** * Analyzes function calls in the AST store and adds corresponding edges in the call graph. * @param astStore The AST store to analyze for function calls. diff --git a/test/detectors/SendInLoop.expected.out b/test/detectors/SendInLoop.expected.out index 968e90d5..baf1f77c 100644 --- a/test/detectors/SendInLoop.expected.out +++ b/test/detectors/SendInLoop.expected.out @@ -1,62 +1,71 @@ [MEDIUM] SendInLoop: Send function called inside a loop -test/detectors/SendInLoop.tact:6:13: - 5 | while (i < 10) { -> 6 | send(SendParameters{ +test/detectors/SendInLoop.tact:5:13: + 4 | while (i < 10) { +> 5 | send(SendParameters{ ^ - 7 | to: sender(), + 6 | to: sender(), Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop [MEDIUM] SendInLoop: Send function called inside a loop -test/detectors/SendInLoop.tact:16:13: - 15 | repeat (10) { -> 16 | send(SendParameters{ +test/detectors/SendInLoop.tact:15:13: + 14 | repeat (10) { +> 15 | send(SendParameters{ ^ - 17 | to: sender(), + 16 | to: sender(), Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop [MEDIUM] SendInLoop: Send function called inside a loop -test/detectors/SendInLoop.tact:26:13: - 25 | do { -> 26 | send(SendParameters{ +test/detectors/SendInLoop.tact:25:13: + 24 | do { +> 25 | send(SendParameters{ ^ - 27 | to: sender(), + 26 | to: sender(), Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop [MEDIUM] SendInLoop: Send function called inside a loop -test/detectors/SendInLoop.tact:37:13: - 36 | foreach (k, v in m) { -> 37 | send(SendParameters{ +test/detectors/SendInLoop.tact:36:13: + 35 | foreach (k, v in m) { +> 36 | send(SendParameters{ ^ - 38 | to: sender(), + 37 | to: sender(), Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop [MEDIUM] SendInLoop: Send function called inside a loop -test/detectors/SendInLoop.tact:48:17: - 47 | repeat (10) { -> 48 | send(SendParameters{ +test/detectors/SendInLoop.tact:47:17: + 46 | repeat (10) { +> 47 | send(SendParameters{ ^ - 49 | to: sender(), + 48 | to: sender(), Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop -[MEDIUM] SendInLoop: Send function called inside a loop -test/detectors/SendInLoop.tact:80:19: - 79 | i += 1; -> 80 | let a = send(SendParameters{ - ^ - 81 | to: self.owner, +[MEDIUM] SendInLoop: Function "indirectSend" called inside a loop leads to a send function +test/detectors/SendInLoop.tact:91:13: + 90 | i += 1; +> 91 | self.indirectSend(i); + ^ + 92 | } Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop -[MEDIUM] SendInLoop: Send function called inside a loop -test/detectors/SendInLoop.tact:93:13: - 92 | while (i < 5) { -> 93 | self.reply(Msg{ a: i }.toCell()); - ^ - 94 | i = i + 1; +[MEDIUM] SendInLoop: Function "intermediateCall" called inside a loop leads to a send function +test/detectors/SendInLoop.tact:120:13: + 119 | while (i < limit) { +> 120 | self.intermediateCall(i); + ^ + 121 | i += 1; +Help: Consider refactoring to avoid calling send functions inside loops +See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop + +[MEDIUM] SendInLoop: Function "deepNestSend" called inside a loop leads to a send function +test/detectors/SendInLoop.tact:140:17: + 139 | if (value > 0) { +> 140 | self.deepNestSend(value); + ^ + 141 | } Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop \ No newline at end of file diff --git a/test/detectors/SendInLoop.tact b/test/detectors/SendInLoop.tact index cfd14ce1..24a66225 100644 --- a/test/detectors/SendInLoop.tact +++ b/test/detectors/SendInLoop.tact @@ -1,4 +1,3 @@ - contract Test { fun tWhile() { let i: Int = 0; @@ -71,31 +70,75 @@ contract SendInLoop { self.a = 0; } - //Send function inside a while loop + fun sendMessage(i: Int) { + send(SendParameters{ + to: self.owner, + value: 0, + bounce: false, + body: Msg{ a: i }.toCell() + }); + } - fun exampleWhileLoop(limit: Int) { + // Function that calls another function which calls send + fun indirectSend(i: Int) { + self.sendMessage(i); + } + + fun exampleIndirectLoop(limit: Int) { let i = 0; while (i < limit) { i += 1; - let a = send(SendParameters{ - to: self.owner, - value: 0, - bounce: false, - body: Msg{ a: i }.toCell() - }); + self.indirectSend(i); } } +} - // self.reply inside a loop - fun testReply() { - let i: Int = 0; - while (i < 5) { - self.reply(Msg{ a: i }.toCell()); - i = i + 1; - } +//Additional test + +contract ComplexIndirectSend { + owner: Address; + + init(owner: Address) { + self.owner = owner; } -} + // Indirect send via multiple function layers + fun deepNestSend(value: Int) { + send(SendParameters{ + to: self.owner, + value: value + }); + } + + fun intermediateCall(value: Int) { + self.deepNestSend(value); + } + fun outerLoopFunction(limit: Int) { + let i = 0; + while (i < limit) { + self.intermediateCall(i); + i += 1; + } + } + // Recursive indirect send + fun recursiveSend(depth: Int, currentValue: Int) { + if (depth > 0) { + send(SendParameters{ + to: self.owner, + value: currentValue + }); + self.recursiveSend(depth - 1, currentValue + 1); + } + } + // Conditional indirect send within loop + fun conditionalIndirectSend(items: map) { + foreach (key, value in items) { + if (value > 0) { + self.deepNestSend(value); + } + } + } +} \ No newline at end of file From 9917aff663365a5dbe001a3b696b036dfbc0aa39 Mon Sep 17 00:00:00 2001 From: Esorat Date: Wed, 20 Nov 2024 12:01:53 +0700 Subject: [PATCH 04/10] Fix: remake SendInLoop and callGraph --- src/detectors/builtin/sendInLoop.ts | 154 ++++++++-------------------- src/internals/ir/callGraph.ts | 105 +++++++++++-------- 2 files changed, 101 insertions(+), 158 deletions(-) diff --git a/src/detectors/builtin/sendInLoop.ts b/src/detectors/builtin/sendInLoop.ts index fb13f3c4..fc3bd8bc 100644 --- a/src/detectors/builtin/sendInLoop.ts +++ b/src/detectors/builtin/sendInLoop.ts @@ -5,17 +5,12 @@ import { foldExpressions, isSelf, } from "../../internals/tact"; -import { unreachable } from "../../internals/util"; import { MistiTactWarning, Severity } from "../../internals/warnings"; import { ASTDetector } from "../detector"; import { AstStatement, AstExpression, idText, - AstFunctionDef, - AstReceiver, - AstContractInit, - AstNode, } from "@tact-lang/compiler/dist/grammar/ast"; /** @@ -53,14 +48,7 @@ export class SendInLoop extends ASTDetector { const astStore = cu.ast; const ctx = this.ctx; const callGraph = new CallGraph(ctx).build(astStore); - const astIdToCGNodeId = new Map(); - for (const [nodeId, node] of callGraph.getNodes()) { - if (node.astId !== undefined) { - astIdToCGNodeId.set(node.astId, nodeId); - } - } - // Collect functions that directly call send functions const functionsCallingSend = new Set(); // Identify all functions that contain a send call @@ -78,36 +66,25 @@ export class SendInLoop extends ASTDetector { ); if (containsSend) { - const funcName = this.getFunctionName(func); - if (funcName) { - const nodeId = callGraph.getNodeIdByName(funcName); - if (nodeId !== undefined) { - functionsCallingSend.add(nodeId); - } + // Get the node id from the ast id + const funcNodeId = callGraph.getNodeIdByAstId(func.id); + if (funcNodeId !== undefined) { + functionsCallingSend.add(funcNodeId); } } } - - // Identify all functions that can lead to a send call - const functionsLeadingToSend = this.getFunctionsLeadingToSend( - callGraph, - functionsCallingSend, - ); - // Analyze loops and check if any function called within leads to a send - Array.from(cu.ast.getProgramEntries()).forEach((node) => { + for (const node of cu.ast.getProgramEntries()) { forEachStatement(node, (stmt) => { const warnings = this.analyzeStatement( stmt, processedLoopIds, callGraph, - astIdToCGNodeId, - functionsLeadingToSend, + functionsCallingSend, ); allWarnings.push(...warnings); }); - }); - + } return allWarnings; } @@ -115,8 +92,7 @@ export class SendInLoop extends ASTDetector { stmt: AstStatement, processedLoopIds: Set, callGraph: CallGraph, - astIdToCGNodeId: Map, - functionsLeadingToSend: Set, + functionsCallingSend: Set, ): MistiTactWarning[] { if (processedLoopIds.has(stmt.id)) { return []; @@ -144,65 +120,47 @@ export class SendInLoop extends ASTDetector { ); // Check function calls within the loop that lead to send - this.forEachExpression(stmt, (expr: AstExpression) => { - if (expr.kind === "static_call" || expr.kind === "method_call") { - const calleeName = this.getCalleeName(expr); - if (calleeName) { - const calleeNodeId = callGraph.getNodeIdByName(calleeName); - if ( - calleeNodeId !== undefined && - functionsLeadingToSend.has(calleeNodeId) - ) { - warnings.push( - this.makeWarning( - `Function "${calleeName}" called inside a loop leads to a send function`, - expr.loc, - { - suggestion: - "Consider refactoring to avoid calling send functions inside loops", - }, - ), - ); + foldExpressions( + stmt, + (acc: MistiTactWarning[], expr: AstExpression) => { + if (expr.kind === "static_call" || expr.kind === "method_call") { + const calleeName = this.getCalleeName(expr); + if (calleeName) { + const calleeNodeId = callGraph.getNodeIdByName(calleeName); + if (calleeNodeId !== undefined) { + // Check if the callee is connected to any function that calls send + for (const sendFuncNodeId of functionsCallingSend) { + if (callGraph.areConnected(calleeNodeId, sendFuncNodeId)) { + // Get the callee node to retrieve its name + const calleeNode = callGraph.getNode(calleeNodeId); + const calleeFunctionName = calleeNode?.name ?? calleeName; + + acc.push( + this.makeWarning( + `Function "${calleeFunctionName}" called inside a loop leads to a send function`, + expr.loc, + { + suggestion: + "Consider refactoring to avoid calling send functions inside loops", + }, + ), + ); + break; + } + } + } } } - } - }); - + return acc; + }, + warnings, + ); return warnings; } // If the statement is not a loop, don't flag anything return []; } - private getFunctionsLeadingToSend( - callGraph: CallGraph, - functionsCallingSend: Set, - ): Set { - const functionsLeadingToSend = new Set(functionsCallingSend); - - // Use a queue for BFS - const queue: CGNodeId[] = Array.from(functionsCallingSend); - - while (queue.length > 0) { - const current = queue.shift()!; - const currentNode = callGraph.getNode(current); - if (currentNode) { - for (const edgeId of currentNode.inEdges) { - const edge = callGraph.getEdge(edgeId); - if (edge) { - const callerId = edge.src; - if (!functionsLeadingToSend.has(callerId)) { - functionsLeadingToSend.add(callerId); - queue.push(callerId); - } - } - } - } - } - - return functionsLeadingToSend; - } - private isSendCall(expr: AstExpression): boolean { const staticSendFunctions = ["send", "nativeSendMessage"]; const selfMethodSendFunctions = ["reply", "forward", "notify", "emit"]; @@ -224,21 +182,6 @@ export class SendInLoop extends ASTDetector { ); } - private getFunctionName( - func: AstFunctionDef | AstReceiver | AstContractInit, - ): string | undefined { - switch (func.kind) { - case "function_def": - return func.name?.text; - case "contract_init": - return `contract_init_${func.id}`; - case "receiver": - return `receiver_${func.id}`; - default: - unreachable(func); - } - } - private getCalleeName(expr: AstExpression): string | undefined { if (expr.kind === "static_call") { return idText(expr.function); @@ -247,19 +190,4 @@ export class SendInLoop extends ASTDetector { } return undefined; } - - // Helper method to traverse expressions - private forEachExpression( - node: AstNode, - callback: (expr: AstExpression) => void, - ): void { - foldExpressions( - node, - (acc, expr) => { - callback(expr); - return acc; - }, - null, - ); - } } diff --git a/src/internals/ir/callGraph.ts b/src/internals/ir/callGraph.ts index 56b605ea..43fc58a7 100644 --- a/src/internals/ir/callGraph.ts +++ b/src/internals/ir/callGraph.ts @@ -57,15 +57,12 @@ class CGNode { /** * The `CallGraph` class represents a directed graph where nodes correspond to functions * or methods in a program, and edges indicate calls between them. - * - * Nodes and edges are uniquely identified using indices generated by `IdxGenerator`. - * This class provides methods to construct the graph from AST data, retrieve nodes and edges, - * and check connectivity between nodes. */ export class CallGraph { private nodeMap: Map = new Map(); - private edgesMap: Map = new Map(); + private astIdToNodeId: Map = new Map(); private nameToNodeId: Map = new Map(); + private edgesMap: Map = new Map(); private logger: Logger; constructor(private ctx: MistiContext) { @@ -89,6 +86,15 @@ export class CallGraph { return this.nameToNodeId.get(name); } + /** + * Retrieves the node ID associated with a given AST node ID. + * @param astId The AST node ID. + * @returns The corresponding node ID, or undefined if not found. + */ + public getNodeIdByAstId(astId: number): CGNodeId | undefined { + return this.astIdToNodeId.get(astId); + } + /** * Retrieves a node from the graph by its ID. * @param nodeId The ID of the node. @@ -99,26 +105,48 @@ export class CallGraph { } /** - * Retrieves an edge from the graph by its ID. - * @param edgeId The ID of the edge. - * @returns The `CGEdge` instance, or undefined if not found. + * Determines if there exists a path in the call graph from the source node to the destination node. */ - public getEdge(edgeId: CGEdgeId): CGEdge | undefined { - return this.edgesMap.get(edgeId); + public areConnected(src: CGNodeId, dst: CGNodeId): boolean { + const srcNode = this.nodeMap.get(src); + const dstNode = this.nodeMap.get(dst); + if (!srcNode || !dstNode) { + return false; + } + const queue: CGNodeId[] = [src]; + const visited = new Set([src]); + while (queue.length > 0) { + const current = queue.shift()!; + if (current === dst) { + return true; + } + const currentNode = this.nodeMap.get(current); + if (currentNode) { + for (const edgeId of currentNode.outEdges) { + const edge = this.edgesMap.get(edgeId); + if (edge && !visited.has(edge.dst)) { + visited.add(edge.dst); + queue.push(edge.dst); + } + } + } + } + return false; } /** * Builds the call graph based on functions in the provided AST store. - * @param astStore - The AST store containing functions to be added to the graph. - * @returns The constructed `CallGraph`. */ public build(astStore: TactASTStore): CallGraph { for (const func of astStore.getFunctions()) { - const funcName = this.getFunctionName(func); + const funcName = this.generateFunctionName(func); if (funcName) { const node = new CGNode(func.id, funcName, this.logger); this.nodeMap.set(node.idx, node); this.nameToNodeId.set(funcName, node.idx); + if (func.id !== undefined) { + this.astIdToNodeId.set(func.id, node.idx); + } } else { this.logger.error( `Function with id ${func.id} has no name and will be skipped.`, @@ -130,33 +158,10 @@ export class CallGraph { } /** - * Analyzes function calls in the AST store and adds corresponding edges in the call graph. - * @param astStore The AST store to analyze for function calls. + * Generates a unique function name based on its type. + * This method is used internally during the build process. */ - private analyzeFunctionCalls(astStore: TactASTStore) { - for (const func of astStore.getFunctions()) { - const funcName = this.getFunctionName(func); - if (funcName) { - const callerId = this.nameToNodeId.get(funcName); - if (callerId !== undefined) { - forEachExpression(func, (expr) => - this.processExpression(expr, callerId), - ); - } else { - this.logger.warn( - `Caller function ${funcName} not found in node map.`, - ); - } - } - } - } - - /** - * Extracts the function name based on its type. - * @param func The function definition, receiver, or contract initializer. - * @returns The function name if available; otherwise, `undefined`. - */ - private getFunctionName( + private generateFunctionName( func: AstFunctionDef | AstReceiver | AstContractInit, ): string | undefined { switch (func.kind) { @@ -171,10 +176,24 @@ export class CallGraph { } } + /** + * Analyzes function calls in the AST store and adds corresponding edges in the call graph. + */ + private analyzeFunctionCalls(astStore: TactASTStore) { + for (const func of astStore.getFunctions()) { + const funcNodeId = this.astIdToNodeId.get(func.id); + if (funcNodeId !== undefined) { + forEachExpression(func, (expr) => + this.processExpression(expr, funcNodeId), + ); + } else { + this.logger.warn(`Caller function with AST ID ${func.id} not found.`); + } + } + } + /** * Processes an expression, identifying static and method calls to add edges. - * @param expr The AST expression to process. - * @param callerId The ID of the calling node. */ private processExpression(expr: AstExpression, callerId: CGNodeId) { if (expr.kind === "static_call") { @@ -204,8 +223,6 @@ export class CallGraph { /** * Finds or adds a function to the call graph by name. - * @param name The name of the function to find or add. - * @returns The ID of the found or added function node. */ private findOrAddFunction(name: string): CGNodeId { const nodeId = this.nameToNodeId.get(name); @@ -220,8 +237,6 @@ export class CallGraph { /** * Adds an edge between two nodes in the call graph. - * @param src The source node ID. - * @param dst The destination node ID. */ private addEdge(src: CGNodeId, dst: CGNodeId) { const srcNode = this.nodeMap.get(src); From 40e907fbc9d5c370307b61f12d842eb31b850e46 Mon Sep 17 00:00:00 2001 From: Esorat Date: Wed, 20 Nov 2024 13:21:04 +0700 Subject: [PATCH 05/10] Fix: Improve CallGraph, add export to index, refactor send in loop --- src/detectors/builtin/sendInLoop.ts | 59 +- src/internals/ir/callGraph.ts | 250 ++++-- src/internals/ir/index.ts | 1 + test/all/sample-jetton.expected.callgraph.dot | 181 ++--- .../all/sample-jetton.expected.callgraph.json | 726 +++++------------- test/all/sample-jetton.expected.callgraph.mmd | 181 ++--- test/all/syntax.expected.callgraph.dot | 38 +- test/all/syntax.expected.callgraph.json | 154 +--- test/all/syntax.expected.callgraph.mmd | 38 +- test/detectors/SendInLoop.expected.out | 6 +- 10 files changed, 576 insertions(+), 1058 deletions(-) diff --git a/src/detectors/builtin/sendInLoop.ts b/src/detectors/builtin/sendInLoop.ts index fc3bd8bc..30dfeb4a 100644 --- a/src/detectors/builtin/sendInLoop.ts +++ b/src/detectors/builtin/sendInLoop.ts @@ -11,6 +11,9 @@ import { AstStatement, AstExpression, idText, + AstStaticCall, + AstMethodCall, + AstContract, } from "@tact-lang/compiler/dist/grammar/ast"; /** @@ -48,7 +51,6 @@ export class SendInLoop extends ASTDetector { const astStore = cu.ast; const ctx = this.ctx; const callGraph = new CallGraph(ctx).build(astStore); - const functionsCallingSend = new Set(); // Identify all functions that contain a send call @@ -64,26 +66,40 @@ export class SendInLoop extends ASTDetector { }, null, ); - if (containsSend) { - // Get the node id from the ast id const funcNodeId = callGraph.getNodeIdByAstId(func.id); if (funcNodeId !== undefined) { functionsCallingSend.add(funcNodeId); } } } + // Analyze loops and check if any function called within leads to a send - for (const node of cu.ast.getProgramEntries()) { - forEachStatement(node, (stmt) => { - const warnings = this.analyzeStatement( - stmt, - processedLoopIds, - callGraph, - functionsCallingSend, - ); - allWarnings.push(...warnings); - }); + for (const entry of cu.ast.getProgramEntries()) { + if (entry.kind === "contract") { + const contract = entry as AstContract; + const contractName = contract.name.text; + forEachStatement(entry, (stmt) => { + const warnings = this.analyzeStatement( + stmt, + processedLoopIds, + callGraph, + functionsCallingSend, + contractName, + ); + allWarnings.push(...warnings); + }); + } else { + forEachStatement(entry, (stmt) => { + const warnings = this.analyzeStatement( + stmt, + processedLoopIds, + callGraph, + functionsCallingSend, + ); + allWarnings.push(...warnings); + }); + } } return allWarnings; } @@ -93,6 +109,7 @@ export class SendInLoop extends ASTDetector { processedLoopIds: Set, callGraph: CallGraph, functionsCallingSend: Set, + currentContractName?: string, ): MistiTactWarning[] { if (processedLoopIds.has(stmt.id)) { return []; @@ -119,12 +136,15 @@ export class SendInLoop extends ASTDetector { warnings, ); - // Check function calls within the loop that lead to send + // Check function calls within the loop that lead to a send foldExpressions( stmt, (acc: MistiTactWarning[], expr: AstExpression) => { if (expr.kind === "static_call" || expr.kind === "method_call") { - const calleeName = this.getCalleeName(expr); + const calleeName = callGraph.getFunctionCallName( + expr as AstStaticCall | AstMethodCall, + currentContractName, + ); if (calleeName) { const calleeNodeId = callGraph.getNodeIdByName(calleeName); if (calleeNodeId !== undefined) { @@ -181,13 +201,4 @@ export class SendInLoop extends ASTDetector { stmt.kind === "statement_foreach" ); } - - private getCalleeName(expr: AstExpression): string | undefined { - if (expr.kind === "static_call") { - return idText(expr.function); - } else if (expr.kind === "method_call") { - return idText(expr.method); - } - return undefined; - } } diff --git a/src/internals/ir/callGraph.ts b/src/internals/ir/callGraph.ts index 43fc58a7..51e7ad20 100644 --- a/src/internals/ir/callGraph.ts +++ b/src/internals/ir/callGraph.ts @@ -11,17 +11,25 @@ import { AstExpression, AstMethodCall, AstStaticCall, + AstContract, + AstId, + AstContractDeclaration, } from "@tact-lang/compiler/dist/grammar/ast"; export type CGNodeId = number & { readonly brand: unique symbol }; -type CGEdgeId = number & { readonly brand: unique symbol }; +export type CGEdgeId = number & { readonly brand: unique symbol }; /** * Represents an edge in the call graph, indicating a call from one function to another. + * Each edge has a unique index (`idx`) generated using `IdxGenerator`. */ class CGEdge { public idx: CGEdgeId; + /** + * @param src The source node ID representing the calling function + * @param dst The destination node ID representing the called function + */ constructor( public src: CGNodeId, public dst: CGNodeId, @@ -32,6 +40,7 @@ class CGEdge { /** * Represents a node in the call graph, corresponding to a function or method. + * Nodes maintain references to incoming and outgoing edges. */ class CGNode { public idx: CGNodeId; @@ -39,8 +48,9 @@ class CGNode { public outEdges: Set = new Set(); /** - * @param astId AST id of the relevant function definition. It might be `undefined` if this node doesn’t have a corresponding AST entry, - * which indicates an issue in Misti. + * @param astId The AST ID of the function or method this node represents (can be `undefined` for synthetic nodes) + * @param name The name of the function or method + * @param logger A logger instance for logging messages */ constructor( public astId: number | undefined, @@ -55,8 +65,11 @@ class CGNode { } /** - * The `CallGraph` class represents a directed graph where nodes correspond to functions - * or methods in a program, and edges indicate calls between them. + * Represents the call graph, a directed graph where nodes represent functions or methods, + * and edges indicate calls between them. + * + * The `CallGraph` class provides methods to build the graph from a Tact AST, retrieve nodes/edges, + * analyze connectivity, and add function calls dynamically. */ export class CallGraph { private nodeMap: Map = new Map(); @@ -65,47 +78,63 @@ export class CallGraph { private edgesMap: Map = new Map(); private logger: Logger; + /** + * @param ctx The MistiContext providing a logger and other utilities + */ constructor(private ctx: MistiContext) { this.logger = ctx.logger; } + /** + * Retrieves all nodes in the call graph. + * @returns A map of all nodes by their unique IDs. + */ public getNodes(): Map { return this.nodeMap; } + /** + * Retrieves all edges in the call graph. + * @returns A map of all edges by their unique IDs. + */ public getEdges(): Map { return this.edgesMap; } /** - * Retrieves the node ID associated with a given function name. - * @param name The function name. - * @returns The corresponding node ID, or undefined if not found. + * Retrieves a node's ID by its name. + * @param name The name of the function or method. + * @returns The corresponding node ID, or `undefined` if not found. */ public getNodeIdByName(name: string): CGNodeId | undefined { return this.nameToNodeId.get(name); } /** - * Retrieves the node ID associated with a given AST node ID. - * @param astId The AST node ID. - * @returns The corresponding node ID, or undefined if not found. + * Retrieves a node's ID by its AST ID. + * @param astId The AST ID of the function. + * @returns The corresponding node ID, or `undefined` if not found. */ public getNodeIdByAstId(astId: number): CGNodeId | undefined { return this.astIdToNodeId.get(astId); } /** - * Retrieves a node from the graph by its ID. - * @param nodeId The ID of the node. - * @returns The `CGNode` instance, or undefined if not found. + * Retrieves a node by its ID. + * @param nodeId The unique ID of the node. + * @returns The corresponding node, or `undefined` if not found. */ public getNode(nodeId: CGNodeId): CGNode | undefined { return this.nodeMap.get(nodeId); } /** - * Determines if there exists a path in the call graph from the source node to the destination node. + * Determines if there exists a path from the source node to the destination node. + * This is achieved via a breadth-first search. + * + * @param src The ID of the source node. + * @param dst The ID of the destination node. + * @returns `true` if a path exists; `false` otherwise. */ public areConnected(src: CGNodeId, dst: CGNodeId): boolean { const srcNode = this.nodeMap.get(src); @@ -135,22 +164,21 @@ export class CallGraph { } /** - * Builds the call graph based on functions in the provided AST store. + * Builds the call graph using data from the AST store. + * @param astStore The AST store containing program entries. + * @returns The constructed `CallGraph`. */ public build(astStore: TactASTStore): CallGraph { - for (const func of astStore.getFunctions()) { - const funcName = this.generateFunctionName(func); - if (funcName) { - const node = new CGNode(func.id, funcName, this.logger); - this.nodeMap.set(node.idx, node); - this.nameToNodeId.set(funcName, node.idx); - if (func.id !== undefined) { - this.astIdToNodeId.set(func.id, node.idx); + for (const entry of astStore.getProgramEntries()) { + if (entry.kind === "contract") { + const contract = entry as AstContract; + const contractName = contract.name.text; + for (const declaration of contract.declarations) { + this.addContractDeclarationToGraph(declaration, contractName); } - } else { - this.logger.error( - `Function with id ${func.id} has no name and will be skipped.`, - ); + } else if (entry.kind === "function_def") { + const func = entry as AstFunctionDef; + this.addFunctionToGraph(func); } } this.analyzeFunctionCalls(astStore); @@ -158,71 +186,173 @@ export class CallGraph { } /** - * Generates a unique function name based on its type. - * This method is used internally during the build process. + * Adds a contract declaration (function, receiver, or initializer) to the graph. + * @param declaration The declaration to add. + * @param contractName The name of the contract the declaration belongs to. + */ + private addContractDeclarationToGraph( + declaration: AstContractDeclaration, + contractName: string, + ) { + if (declaration.kind === "function_def") { + this.addFunctionToGraph(declaration as AstFunctionDef, contractName); + } else if (declaration.kind === "contract_init") { + this.addFunctionToGraph(declaration as AstContractInit, contractName); + } else if (declaration.kind === "receiver") { + this.addFunctionToGraph(declaration as AstReceiver, contractName); + } + } + + /** + * Adds a function node to the graph. + * @param func The function definition, receiver, or initializer. + * @param contractName The optional contract name for namespacing. */ - private generateFunctionName( + private addFunctionToGraph( func: AstFunctionDef | AstReceiver | AstContractInit, + contractName?: string, + ) { + const funcName = this.getFunctionName(func, contractName); + if (funcName) { + const node = new CGNode(func.id, funcName, this.logger); + this.nodeMap.set(node.idx, node); + this.nameToNodeId.set(funcName, node.idx); + if (func.id !== undefined) { + this.astIdToNodeId.set(func.id, node.idx); + } + } else { + this.logger.error( + `Function with id ${func.id} has no name and will be skipped.`, + ); + } + } + + /** + * Extracts the function name based on its type and optional contract name. + * @param func The function definition, receiver, or initializer. + * @param contractName The optional contract name. + * @returns The function name, or `undefined` if it cannot be determined. + */ + private getFunctionName( + func: AstFunctionDef | AstReceiver | AstContractInit, + contractName?: string, ): string | undefined { switch (func.kind) { case "function_def": - return func.name?.text; + return contractName + ? `${contractName}::${func.name?.text}` + : func.name?.text; case "contract_init": - return `contract_init_${func.id}`; + return contractName + ? `${contractName}::contract_init_${func.id}` + : `contract_init_${func.id}`; case "receiver": - return `receiver_${func.id}`; + return contractName + ? `${contractName}::receiver_${func.id}` + : `receiver_${func.id}`; default: unreachable(func); } } /** - * Analyzes function calls in the AST store and adds corresponding edges in the call graph. + * Analyzes the AST for function calls and adds edges between caller and callee nodes. + * @param astStore The AST store to analyze. */ private analyzeFunctionCalls(astStore: TactASTStore) { - for (const func of astStore.getFunctions()) { - const funcNodeId = this.astIdToNodeId.get(func.id); - if (funcNodeId !== undefined) { - forEachExpression(func, (expr) => - this.processExpression(expr, funcNodeId), - ); - } else { - this.logger.warn(`Caller function with AST ID ${func.id} not found.`); + for (const entry of astStore.getProgramEntries()) { + if (entry.kind === "contract") { + const contract = entry as AstContract; + const contractName = contract.name.text; + for (const declaration of contract.declarations) { + if ( + declaration.kind === "function_def" || + declaration.kind === "contract_init" || + declaration.kind === "receiver" + ) { + const func = declaration as + | AstFunctionDef + | AstContractInit + | AstReceiver; + const funcNodeId = this.astIdToNodeId.get(func.id); + if (funcNodeId !== undefined) { + forEachExpression(func, (expr) => + this.processExpression(expr, funcNodeId, contractName), + ); + } + } + } + } else if (entry.kind === "function_def") { + const func = entry as AstFunctionDef; + const funcNodeId = this.astIdToNodeId.get(func.id); + if (funcNodeId !== undefined) { + forEachExpression(func, (expr) => + this.processExpression(expr, funcNodeId), + ); + } } } } /** - * Processes an expression, identifying static and method calls to add edges. + * Processes a single expression, identifying function or method calls to create edges. + * @param expr The expression to process. + * @param callerId The node ID of the calling function. + * @param currentContractName The name of the contract, if applicable. */ - private processExpression(expr: AstExpression, callerId: CGNodeId) { - if (expr.kind === "static_call") { - const staticCall = expr as AstStaticCall; - const functionName = staticCall.function?.text; + private processExpression( + expr: AstExpression, + callerId: CGNodeId, + currentContractName?: string, + ) { + if (expr.kind === "static_call" || expr.kind === "method_call") { + const functionName = this.getFunctionCallName( + expr as AstStaticCall | AstMethodCall, + currentContractName, + ); if (functionName) { const calleeId = this.findOrAddFunction(functionName); this.addEdge(callerId, calleeId); } else { this.logger.warn( - `Static call expression missing function name at caller ${callerId}`, + `Call expression missing function name at caller ${callerId}`, ); } + } + } + + /** + * Derives the function call name from a static or method call expression. + * @param expr The call expression. + * @param currentContractName The name of the current contract, if available. + * @returns The fully qualified function name, or `undefined` if it cannot be determined. + */ + public getFunctionCallName( + expr: AstStaticCall | AstMethodCall, + currentContractName?: string, + ): string | undefined { + if (expr.kind === "static_call") { + return expr.function?.text; // Static calls directly reference free functions } else if (expr.kind === "method_call") { - const methodCall = expr as AstMethodCall; - const methodName = methodCall.method?.text; + const methodName = expr.method?.text; if (methodName) { - const calleeId = this.findOrAddFunction(methodName); - this.addEdge(callerId, calleeId); - } else { - this.logger.warn( - `Method call expression missing method name at caller ${callerId}`, - ); + let contractName = currentContractName; + if (expr.self.kind === "id") { + const idExpr = expr.self as AstId; + if (idExpr.text !== "self") { + contractName = idExpr.text; // Refers to another contract + } + } + return contractName ? `${contractName}::${methodName}` : methodName; } } + return undefined; } /** - * Finds or adds a function to the call graph by name. + * Finds or creates a function node in the graph by its name. + * @param name The name of the function. + * @returns The node ID of the existing or newly created function. */ private findOrAddFunction(name: string): CGNodeId { const nodeId = this.nameToNodeId.get(name); @@ -236,7 +366,9 @@ export class CallGraph { } /** - * Adds an edge between two nodes in the call graph. + * Adds a directed edge between two nodes in the call graph. + * @param src The source node ID. + * @param dst The destination node ID. */ private addEdge(src: CGNodeId, dst: CGNodeId) { const srcNode = this.nodeMap.get(src); diff --git a/src/internals/ir/index.ts b/src/internals/ir/index.ts index fc8e4f24..63ab9978 100644 --- a/src/internals/ir/index.ts +++ b/src/internals/ir/index.ts @@ -4,3 +4,4 @@ export * from "./imports"; export * from "./ir"; export * from "./types"; export * from "./builders/"; +export * from "./callGraph"; diff --git a/test/all/sample-jetton.expected.callgraph.dot b/test/all/sample-jetton.expected.callgraph.dot index 681dce12..aace0793 100644 --- a/test/all/sample-jetton.expected.callgraph.dot +++ b/test/all/sample-jetton.expected.callgraph.dot @@ -1,120 +1,69 @@ digraph "CallGraph" { node [shape=box]; - node_1 [label="reply"]; - node_2 [label="notify"]; - node_3 [label="forward"]; - node_4 [label="requireOwner"]; - node_5 [label="owner"]; - node_6 [label="receiver_1734"]; - node_7 [label="contract_init_1817"]; - node_8 [label="receiver_1857"]; - node_9 [label="receiver_1882"]; - node_10 [label="receiver_1905"]; - node_11 [label="receiver_1939"]; - node_12 [label="receiver_2000"]; - node_13 [label="mint"]; - node_14 [label="requireWallet"]; - node_15 [label="getJettonWalletInit"]; - node_16 [label="get_jetton_data"]; - node_17 [label="get_wallet_address"]; - node_18 [label="contract_init_2359"]; - node_19 [label="receiver_2517"]; - node_20 [label="receiver_2687"]; - node_21 [label="msgValue"]; - node_22 [label="receiver_2832"]; - node_23 [label="receiver_2876"]; - node_24 [label="get_wallet_data"]; - node_25 [label="sender"]; - node_26 [label="context"]; - node_27 [label="myBalance"]; - node_28 [label="nativeReserve"]; - node_29 [label="send"]; - node_30 [label="nativeThrowUnless"]; - node_31 [label="toCell"]; - node_32 [label="require"]; - node_33 [label="contractAddress"]; - node_34 [label="myAddress"]; - node_35 [label="emptySlice"]; - node_36 [label="readForwardFee"]; - node_37 [label="min"]; - node_38 [label="ton"]; - node_39 [label="loadUint"]; - node_40 [label="loadCoins"]; - node_1 -> node_3; - node_1 -> node_25; - node_2 -> node_3; - node_2 -> node_25; - node_3 -> node_26; - node_3 -> node_27; - node_3 -> node_28; - node_3 -> node_29; - node_3 -> node_29; - node_4 -> node_30; - node_4 -> node_25; - node_6 -> node_4; - node_6 -> node_1; - node_6 -> node_31; - node_8 -> node_26; - node_8 -> node_32; - node_8 -> node_32; - node_8 -> node_13; - node_9 -> node_26; - node_9 -> node_32; + node_1 [label="SampleJetton::contract_init_1817"]; + node_2 [label="SampleJetton::receiver_1857"]; + node_3 [label="SampleJetton::receiver_1882"]; + node_4 [label="SampleJetton::receiver_1905"]; + node_5 [label="JettonDefaultWallet::contract_init_2359"]; + node_6 [label="JettonDefaultWallet::receiver_2517"]; + node_7 [label="JettonDefaultWallet::receiver_2687"]; + node_8 [label="JettonDefaultWallet::msgValue"]; + node_9 [label="JettonDefaultWallet::receiver_2832"]; + node_10 [label="JettonDefaultWallet::receiver_2876"]; + node_11 [label="JettonDefaultWallet::get_wallet_data"]; + node_12 [label="context"]; + node_13 [label="require"]; + node_14 [label="SampleJetton::mint"]; + node_15 [label="ctx::readForwardFee"]; + node_16 [label="min"]; + node_17 [label="ton"]; + node_18 [label="contractAddress"]; + node_19 [label="send"]; + node_20 [label="JettonDefaultWallet::toCell"]; + node_21 [label="myBalance"]; + node_22 [label="msg::loadUint"]; + node_23 [label="msg::loadCoins"]; + node_2 -> node_12; + node_2 -> node_13; + node_2 -> node_13; + node_2 -> node_14; + node_3 -> node_12; + node_3 -> node_13; + node_3 -> node_14; + node_4 -> node_12; + node_4 -> node_13; + node_6 -> node_12; + node_6 -> node_13; + node_6 -> node_15; + node_6 -> node_15; + node_6 -> node_13; + node_6 -> node_16; + node_6 -> node_17; + node_6 -> node_13; + node_6 -> node_18; + node_6 -> node_19; + node_6 -> node_20; + node_7 -> node_12; + node_7 -> node_13; + node_7 -> node_18; + node_7 -> node_13; + node_7 -> node_19; + node_7 -> node_20; + node_7 -> node_8; + node_7 -> node_15; + node_7 -> node_19; + node_7 -> node_20; + node_8 -> node_21; + node_8 -> node_16; + node_9 -> node_12; node_9 -> node_13; - node_10 -> node_26; - node_10 -> node_32; - node_11 -> node_4; - node_12 -> node_14; - node_12 -> node_29; - node_12 -> node_31; - node_13 -> node_32; - node_13 -> node_15; - node_13 -> node_29; - node_13 -> node_33; - node_13 -> node_31; - node_13 -> node_34; - node_13 -> node_35; - node_14 -> node_26; - node_14 -> node_15; - node_14 -> node_32; - node_14 -> node_33; - node_15 -> node_34; - node_16 -> node_15; - node_16 -> node_34; - node_17 -> node_15; - node_17 -> node_33; - node_19 -> node_26; - node_19 -> node_32; - node_19 -> node_36; - node_19 -> node_36; - node_19 -> node_32; - node_19 -> node_37; - node_19 -> node_38; - node_19 -> node_32; - node_19 -> node_33; - node_19 -> node_29; - node_19 -> node_31; - node_20 -> node_26; - node_20 -> node_32; - node_20 -> node_33; - node_20 -> node_32; - node_20 -> node_29; - node_20 -> node_31; - node_20 -> node_21; - node_20 -> node_36; - node_20 -> node_29; - node_20 -> node_31; - node_21 -> node_27; - node_21 -> node_37; - node_22 -> node_26; - node_22 -> node_32; - node_22 -> node_32; - node_22 -> node_36; - node_22 -> node_32; - node_22 -> node_29; - node_22 -> node_31; - node_23 -> node_39; - node_23 -> node_39; - node_23 -> node_40; - node_23 -> node_32; + node_9 -> node_13; + node_9 -> node_15; + node_9 -> node_13; + node_9 -> node_19; + node_9 -> node_20; + node_10 -> node_22; + node_10 -> node_22; + node_10 -> node_23; + node_10 -> node_13; } diff --git a/test/all/sample-jetton.expected.callgraph.json b/test/all/sample-jetton.expected.callgraph.json index 9828ca1d..be183287 100644 --- a/test/all/sample-jetton.expected.callgraph.json +++ b/test/all/sample-jetton.expected.callgraph.json @@ -2,438 +2,246 @@ "nodes": [ { "idx": 1, - "name": "reply", - "inEdges": [ - 13 - ], - "outEdges": [ - 1, - 2 - ] + "name": "SampleJetton::contract_init_1817", + "inEdges": [], + "outEdges": [] }, { "idx": 2, - "name": "notify", + "name": "SampleJetton::receiver_1857", "inEdges": [], "outEdges": [ + 1, + 2, 3, 4 ] }, { "idx": 3, - "name": "forward", - "inEdges": [ - 1, - 3 - ], + "name": "SampleJetton::receiver_1882", + "inEdges": [], "outEdges": [ 5, 6, - 7, - 8, - 9 + 7 ] }, { "idx": 4, - "name": "requireOwner", - "inEdges": [ - 12, - 24 - ], + "name": "SampleJetton::receiver_1905", + "inEdges": [], "outEdges": [ - 10, - 11 + 8, + 9 ] }, { "idx": 5, - "name": "owner", + "name": "JettonDefaultWallet::contract_init_2359", "inEdges": [], "outEdges": [] }, { "idx": 6, - "name": "receiver_1734", + "name": "JettonDefaultWallet::receiver_2517", "inEdges": [], "outEdges": [ + 10, + 11, 12, 13, - 14 - ] - }, - { - "idx": 7, - "name": "contract_init_1817", - "inEdges": [], - "outEdges": [] - }, - { - "idx": 8, - "name": "receiver_1857", - "inEdges": [], - "outEdges": [ + 14, 15, 16, 17, - 18 - ] - }, - { - "idx": 9, - "name": "receiver_1882", - "inEdges": [], - "outEdges": [ + 18, 19, - 20, - 21 + 20 ] }, { - "idx": 10, - "name": "receiver_1905", + "idx": 7, + "name": "JettonDefaultWallet::receiver_2687", "inEdges": [], "outEdges": [ + 21, 22, - 23 - ] - }, - { - "idx": 11, - "name": "receiver_1939", - "inEdges": [], - "outEdges": [ - 24 - ] - }, - { - "idx": 12, - "name": "receiver_2000", - "inEdges": [], - "outEdges": [ + 23, + 24, 25, 26, - 27 + 27, + 28, + 29, + 30 ] }, { - "idx": 13, - "name": "mint", + "idx": 8, + "name": "JettonDefaultWallet::msgValue", "inEdges": [ - 18, - 21 + 27 ], "outEdges": [ - 28, - 29, - 30, 31, - 32, - 33, - 34 + 32 ] }, { - "idx": 14, - "name": "requireWallet", - "inEdges": [ - 25 - ], + "idx": 9, + "name": "JettonDefaultWallet::receiver_2832", + "inEdges": [], "outEdges": [ + 33, + 34, 35, 36, 37, - 38 - ] - }, - { - "idx": 15, - "name": "getJettonWalletInit", - "inEdges": [ - 29, - 36, - 40, - 42 - ], - "outEdges": [ + 38, 39 ] }, { - "idx": 16, - "name": "get_jetton_data", + "idx": 10, + "name": "JettonDefaultWallet::receiver_2876", "inEdges": [], "outEdges": [ 40, - 41 - ] - }, - { - "idx": 17, - "name": "get_wallet_address", - "inEdges": [], - "outEdges": [ + 41, 42, 43 ] }, { - "idx": 18, - "name": "contract_init_2359", - "inEdges": [], - "outEdges": [] - }, - { - "idx": 19, - "name": "receiver_2517", - "inEdges": [], - "outEdges": [ - 44, - 45, - 46, - 47, - 48, - 49, - 50, - 51, - 52, - 53, - 54 - ] - }, - { - "idx": 20, - "name": "receiver_2687", - "inEdges": [], - "outEdges": [ - 55, - 56, - 57, - 58, - 59, - 60, - 61, - 62, - 63, - 64 - ] - }, - { - "idx": 21, - "name": "msgValue", - "inEdges": [ - 61 - ], - "outEdges": [ - 65, - 66 - ] - }, - { - "idx": 22, - "name": "receiver_2832", - "inEdges": [], - "outEdges": [ - 67, - 68, - 69, - 70, - 71, - 72, - 73 - ] - }, - { - "idx": 23, - "name": "receiver_2876", - "inEdges": [], - "outEdges": [ - 74, - 75, - 76, - 77 - ] - }, - { - "idx": 24, - "name": "get_wallet_data", + "idx": 11, + "name": "JettonDefaultWallet::get_wallet_data", "inEdges": [], "outEdges": [] }, { - "idx": 25, - "name": "sender", - "inEdges": [ - 2, - 4, - 11 - ], - "outEdges": [] - }, - { - "idx": 26, + "idx": 12, "name": "context", "inEdges": [ + 1, 5, - 15, - 19, - 22, - 35, - 44, - 55, - 67 + 8, + 10, + 21, + 33 ], "outEdges": [] }, { - "idx": 27, - "name": "myBalance", + "idx": 13, + "name": "require", "inEdges": [ + 2, + 3, 6, - 65 - ], - "outEdges": [] - }, - { - "idx": 28, - "name": "nativeReserve", - "inEdges": [ - 7 - ], - "outEdges": [] - }, - { - "idx": 29, - "name": "send", - "inEdges": [ - 8, 9, - 26, - 30, - 53, - 59, - 63, - 72 - ], - "outEdges": [] - }, - { - "idx": 30, - "name": "nativeThrowUnless", - "inEdges": [ - 10 + 11, + 14, + 17, + 22, + 24, + 34, + 35, + 37, + 43 ], "outEdges": [] }, { - "idx": 31, - "name": "toCell", + "idx": 14, + "name": "SampleJetton::mint", "inEdges": [ - 14, - 27, - 32, - 54, - 60, - 64, - 73 + 4, + 7 ], "outEdges": [] }, { - "idx": 32, - "name": "require", + "idx": 15, + "name": "ctx::readForwardFee", "inEdges": [ - 16, - 17, - 20, - 23, + 12, + 13, 28, - 37, - 45, - 48, - 51, - 56, - 58, - 68, - 69, - 71, - 77 + 36 ], "outEdges": [] }, { - "idx": 33, - "name": "contractAddress", + "idx": 16, + "name": "min", "inEdges": [ - 31, - 38, - 43, - 52, - 57 + 15, + 32 ], "outEdges": [] }, { - "idx": 34, - "name": "myAddress", + "idx": 17, + "name": "ton", "inEdges": [ - 33, - 39, - 41 + 16 ], "outEdges": [] }, { - "idx": 35, - "name": "emptySlice", + "idx": 18, + "name": "contractAddress", "inEdges": [ - 34 + 18, + 23 ], "outEdges": [] }, { - "idx": 36, - "name": "readForwardFee", + "idx": 19, + "name": "send", "inEdges": [ - 46, - 47, - 62, - 70 + 19, + 25, + 29, + 38 ], "outEdges": [] }, { - "idx": 37, - "name": "min", + "idx": 20, + "name": "JettonDefaultWallet::toCell", "inEdges": [ - 49, - 66 + 20, + 26, + 30, + 39 ], "outEdges": [] }, { - "idx": 38, - "name": "ton", + "idx": 21, + "name": "myBalance", "inEdges": [ - 50 + 31 ], "outEdges": [] }, { - "idx": 39, - "name": "loadUint", + "idx": 22, + "name": "msg::loadUint", "inEdges": [ - 74, - 75 + 40, + 41 ], "outEdges": [] }, { - "idx": 40, - "name": "loadCoins", + "idx": 23, + "name": "msg::loadCoins", "inEdges": [ - 76 + 42 ], "outEdges": [] } @@ -441,388 +249,218 @@ "edges": [ { "idx": 1, - "src": 1, - "dst": 3 + "src": 2, + "dst": 12 }, { "idx": 2, - "src": 1, - "dst": 25 + "src": 2, + "dst": 13 }, { "idx": 3, "src": 2, - "dst": 3 + "dst": 13 }, { "idx": 4, "src": 2, - "dst": 25 + "dst": 14 }, { "idx": 5, "src": 3, - "dst": 26 + "dst": 12 }, { "idx": 6, "src": 3, - "dst": 27 + "dst": 13 }, { "idx": 7, "src": 3, - "dst": 28 + "dst": 14 }, { "idx": 8, - "src": 3, - "dst": 29 + "src": 4, + "dst": 12 }, { "idx": 9, - "src": 3, - "dst": 29 + "src": 4, + "dst": 13 }, { "idx": 10, - "src": 4, - "dst": 30 + "src": 6, + "dst": 12 }, { "idx": 11, - "src": 4, - "dst": 25 + "src": 6, + "dst": 13 }, { "idx": 12, "src": 6, - "dst": 4 + "dst": 15 }, { "idx": 13, "src": 6, - "dst": 1 + "dst": 15 }, { "idx": 14, "src": 6, - "dst": 31 + "dst": 13 }, { "idx": 15, - "src": 8, - "dst": 26 + "src": 6, + "dst": 16 }, { "idx": 16, - "src": 8, - "dst": 32 + "src": 6, + "dst": 17 }, { "idx": 17, - "src": 8, - "dst": 32 + "src": 6, + "dst": 13 }, { "idx": 18, - "src": 8, - "dst": 13 + "src": 6, + "dst": 18 }, { "idx": 19, - "src": 9, - "dst": 26 + "src": 6, + "dst": 19 }, { "idx": 20, - "src": 9, - "dst": 32 + "src": 6, + "dst": 20 }, { "idx": 21, - "src": 9, - "dst": 13 + "src": 7, + "dst": 12 }, { "idx": 22, - "src": 10, - "dst": 26 + "src": 7, + "dst": 13 }, { "idx": 23, - "src": 10, - "dst": 32 + "src": 7, + "dst": 18 }, { "idx": 24, - "src": 11, - "dst": 4 + "src": 7, + "dst": 13 }, { "idx": 25, - "src": 12, - "dst": 14 + "src": 7, + "dst": 19 }, { "idx": 26, - "src": 12, - "dst": 29 + "src": 7, + "dst": 20 }, { "idx": 27, - "src": 12, - "dst": 31 + "src": 7, + "dst": 8 }, { "idx": 28, - "src": 13, - "dst": 32 + "src": 7, + "dst": 15 }, { "idx": 29, - "src": 13, - "dst": 15 + "src": 7, + "dst": 19 }, { "idx": 30, - "src": 13, - "dst": 29 + "src": 7, + "dst": 20 }, { "idx": 31, - "src": 13, - "dst": 33 + "src": 8, + "dst": 21 }, { "idx": 32, - "src": 13, - "dst": 31 + "src": 8, + "dst": 16 }, { "idx": 33, - "src": 13, - "dst": 34 + "src": 9, + "dst": 12 }, { "idx": 34, - "src": 13, - "dst": 35 + "src": 9, + "dst": 13 }, { "idx": 35, - "src": 14, - "dst": 26 + "src": 9, + "dst": 13 }, { "idx": 36, - "src": 14, + "src": 9, "dst": 15 }, { "idx": 37, - "src": 14, - "dst": 32 + "src": 9, + "dst": 13 }, { "idx": 38, - "src": 14, - "dst": 33 + "src": 9, + "dst": 19 }, { "idx": 39, - "src": 15, - "dst": 34 + "src": 9, + "dst": 20 }, { "idx": 40, - "src": 16, - "dst": 15 + "src": 10, + "dst": 22 }, { "idx": 41, - "src": 16, - "dst": 34 + "src": 10, + "dst": 22 }, { "idx": 42, - "src": 17, - "dst": 15 + "src": 10, + "dst": 23 }, { "idx": 43, - "src": 17, - "dst": 33 - }, - { - "idx": 44, - "src": 19, - "dst": 26 - }, - { - "idx": 45, - "src": 19, - "dst": 32 - }, - { - "idx": 46, - "src": 19, - "dst": 36 - }, - { - "idx": 47, - "src": 19, - "dst": 36 - }, - { - "idx": 48, - "src": 19, - "dst": 32 - }, - { - "idx": 49, - "src": 19, - "dst": 37 - }, - { - "idx": 50, - "src": 19, - "dst": 38 - }, - { - "idx": 51, - "src": 19, - "dst": 32 - }, - { - "idx": 52, - "src": 19, - "dst": 33 - }, - { - "idx": 53, - "src": 19, - "dst": 29 - }, - { - "idx": 54, - "src": 19, - "dst": 31 - }, - { - "idx": 55, - "src": 20, - "dst": 26 - }, - { - "idx": 56, - "src": 20, - "dst": 32 - }, - { - "idx": 57, - "src": 20, - "dst": 33 - }, - { - "idx": 58, - "src": 20, - "dst": 32 - }, - { - "idx": 59, - "src": 20, - "dst": 29 - }, - { - "idx": 60, - "src": 20, - "dst": 31 - }, - { - "idx": 61, - "src": 20, - "dst": 21 - }, - { - "idx": 62, - "src": 20, - "dst": 36 - }, - { - "idx": 63, - "src": 20, - "dst": 29 - }, - { - "idx": 64, - "src": 20, - "dst": 31 - }, - { - "idx": 65, - "src": 21, - "dst": 27 - }, - { - "idx": 66, - "src": 21, - "dst": 37 - }, - { - "idx": 67, - "src": 22, - "dst": 26 - }, - { - "idx": 68, - "src": 22, - "dst": 32 - }, - { - "idx": 69, - "src": 22, - "dst": 32 - }, - { - "idx": 70, - "src": 22, - "dst": 36 - }, - { - "idx": 71, - "src": 22, - "dst": 32 - }, - { - "idx": 72, - "src": 22, - "dst": 29 - }, - { - "idx": 73, - "src": 22, - "dst": 31 - }, - { - "idx": 74, - "src": 23, - "dst": 39 - }, - { - "idx": 75, - "src": 23, - "dst": 39 - }, - { - "idx": 76, - "src": 23, - "dst": 40 - }, - { - "idx": 77, - "src": 23, - "dst": 32 + "src": 10, + "dst": 13 } ] } \ No newline at end of file diff --git a/test/all/sample-jetton.expected.callgraph.mmd b/test/all/sample-jetton.expected.callgraph.mmd index aec3dc9d..3cdb21bd 100644 --- a/test/all/sample-jetton.expected.callgraph.mmd +++ b/test/all/sample-jetton.expected.callgraph.mmd @@ -1,118 +1,67 @@ graph TD - node_1["reply"] - node_2["notify"] - node_3["forward"] - node_4["requireOwner"] - node_5["owner"] - node_6["receiver_1734"] - node_7["contract_init_1817"] - node_8["receiver_1857"] - node_9["receiver_1882"] - node_10["receiver_1905"] - node_11["receiver_1939"] - node_12["receiver_2000"] - node_13["mint"] - node_14["requireWallet"] - node_15["getJettonWalletInit"] - node_16["get_jetton_data"] - node_17["get_wallet_address"] - node_18["contract_init_2359"] - node_19["receiver_2517"] - node_20["receiver_2687"] - node_21["msgValue"] - node_22["receiver_2832"] - node_23["receiver_2876"] - node_24["get_wallet_data"] - node_25["sender"] - node_26["context"] - node_27["myBalance"] - node_28["nativeReserve"] - node_29["send"] - node_30["nativeThrowUnless"] - node_31["toCell"] - node_32["require"] - node_33["contractAddress"] - node_34["myAddress"] - node_35["emptySlice"] - node_36["readForwardFee"] - node_37["min"] - node_38["ton"] - node_39["loadUint"] - node_40["loadCoins"] - node_1 --> node_3 - node_1 --> node_25 - node_2 --> node_3 - node_2 --> node_25 - node_3 --> node_26 - node_3 --> node_27 - node_3 --> node_28 - node_3 --> node_29 - node_3 --> node_29 - node_4 --> node_30 - node_4 --> node_25 - node_6 --> node_4 - node_6 --> node_1 - node_6 --> node_31 - node_8 --> node_26 - node_8 --> node_32 - node_8 --> node_32 - node_8 --> node_13 - node_9 --> node_26 - node_9 --> node_32 + node_1["SampleJetton::contract_init_1817"] + node_2["SampleJetton::receiver_1857"] + node_3["SampleJetton::receiver_1882"] + node_4["SampleJetton::receiver_1905"] + node_5["JettonDefaultWallet::contract_init_2359"] + node_6["JettonDefaultWallet::receiver_2517"] + node_7["JettonDefaultWallet::receiver_2687"] + node_8["JettonDefaultWallet::msgValue"] + node_9["JettonDefaultWallet::receiver_2832"] + node_10["JettonDefaultWallet::receiver_2876"] + node_11["JettonDefaultWallet::get_wallet_data"] + node_12["context"] + node_13["require"] + node_14["SampleJetton::mint"] + node_15["ctx::readForwardFee"] + node_16["min"] + node_17["ton"] + node_18["contractAddress"] + node_19["send"] + node_20["JettonDefaultWallet::toCell"] + node_21["myBalance"] + node_22["msg::loadUint"] + node_23["msg::loadCoins"] + node_2 --> node_12 + node_2 --> node_13 + node_2 --> node_13 + node_2 --> node_14 + node_3 --> node_12 + node_3 --> node_13 + node_3 --> node_14 + node_4 --> node_12 + node_4 --> node_13 + node_6 --> node_12 + node_6 --> node_13 + node_6 --> node_15 + node_6 --> node_15 + node_6 --> node_13 + node_6 --> node_16 + node_6 --> node_17 + node_6 --> node_13 + node_6 --> node_18 + node_6 --> node_19 + node_6 --> node_20 + node_7 --> node_12 + node_7 --> node_13 + node_7 --> node_18 + node_7 --> node_13 + node_7 --> node_19 + node_7 --> node_20 + node_7 --> node_8 + node_7 --> node_15 + node_7 --> node_19 + node_7 --> node_20 + node_8 --> node_21 + node_8 --> node_16 + node_9 --> node_12 node_9 --> node_13 - node_10 --> node_26 - node_10 --> node_32 - node_11 --> node_4 - node_12 --> node_14 - node_12 --> node_29 - node_12 --> node_31 - node_13 --> node_32 - node_13 --> node_15 - node_13 --> node_29 - node_13 --> node_33 - node_13 --> node_31 - node_13 --> node_34 - node_13 --> node_35 - node_14 --> node_26 - node_14 --> node_15 - node_14 --> node_32 - node_14 --> node_33 - node_15 --> node_34 - node_16 --> node_15 - node_16 --> node_34 - node_17 --> node_15 - node_17 --> node_33 - node_19 --> node_26 - node_19 --> node_32 - node_19 --> node_36 - node_19 --> node_36 - node_19 --> node_32 - node_19 --> node_37 - node_19 --> node_38 - node_19 --> node_32 - node_19 --> node_33 - node_19 --> node_29 - node_19 --> node_31 - node_20 --> node_26 - node_20 --> node_32 - node_20 --> node_33 - node_20 --> node_32 - node_20 --> node_29 - node_20 --> node_31 - node_20 --> node_21 - node_20 --> node_36 - node_20 --> node_29 - node_20 --> node_31 - node_21 --> node_27 - node_21 --> node_37 - node_22 --> node_26 - node_22 --> node_32 - node_22 --> node_32 - node_22 --> node_36 - node_22 --> node_32 - node_22 --> node_29 - node_22 --> node_31 - node_23 --> node_39 - node_23 --> node_39 - node_23 --> node_40 - node_23 --> node_32 + node_9 --> node_13 + node_9 --> node_15 + node_9 --> node_13 + node_9 --> node_19 + node_9 --> node_20 + node_10 --> node_22 + node_10 --> node_22 + node_10 --> node_23 + node_10 --> node_13 diff --git a/test/all/syntax.expected.callgraph.dot b/test/all/syntax.expected.callgraph.dot index 7f7f6bac..565484dc 100644 --- a/test/all/syntax.expected.callgraph.dot +++ b/test/all/syntax.expected.callgraph.dot @@ -2,32 +2,14 @@ digraph "CallGraph" { node [shape=box]; node_1 [label="test_try"]; node_2 [label="test_loops"]; - node_3 [label="reply"]; - node_4 [label="notify"]; - node_5 [label="forward"]; - node_6 [label="getter"]; - node_7 [label="getter"]; - node_8 [label="test"]; - node_9 [label="getA"]; - node_10 [label="test"]; - node_11 [label="receiver_1722"]; - node_12 [label="dump"]; - node_13 [label="emptyMap"]; - node_14 [label="sender"]; - node_15 [label="context"]; - node_16 [label="myBalance"]; - node_17 [label="nativeReserve"]; - node_18 [label="send"]; - node_1 -> node_12; - node_2 -> node_13; - node_3 -> node_5; - node_3 -> node_14; - node_4 -> node_5; - node_4 -> node_14; - node_5 -> node_15; - node_5 -> node_16; - node_5 -> node_17; - node_5 -> node_18; - node_5 -> node_18; - node_10 -> node_9; + node_3 [label="TestContract::getter"]; + node_4 [label="TestContractF::test"]; + node_5 [label="TestContractT::test"]; + node_6 [label="TestContractT::receiver_1722"]; + node_7 [label="dump"]; + node_8 [label="emptyMap"]; + node_9 [label="TestContractT::getA"]; + node_1 -> node_7; + node_2 -> node_8; + node_5 -> node_9; } diff --git a/test/all/syntax.expected.callgraph.json b/test/all/syntax.expected.callgraph.json index c84e70d4..346edc0b 100644 --- a/test/all/syntax.expected.callgraph.json +++ b/test/all/syntax.expected.callgraph.json @@ -18,79 +18,32 @@ }, { "idx": 3, - "name": "reply", + "name": "TestContract::getter", "inEdges": [], - "outEdges": [ - 3, - 4 - ] + "outEdges": [] }, { "idx": 4, - "name": "notify", + "name": "TestContractF::test", "inEdges": [], - "outEdges": [ - 5, - 6 - ] + "outEdges": [] }, { "idx": 5, - "name": "forward", - "inEdges": [ - 3, - 5 - ], + "name": "TestContractT::test", + "inEdges": [], "outEdges": [ - 7, - 8, - 9, - 10, - 11 + 3 ] }, { "idx": 6, - "name": "getter", + "name": "TestContractT::receiver_1722", "inEdges": [], "outEdges": [] }, { "idx": 7, - "name": "getter", - "inEdges": [], - "outEdges": [] - }, - { - "idx": 8, - "name": "test", - "inEdges": [], - "outEdges": [] - }, - { - "idx": 9, - "name": "getA", - "inEdges": [ - 12 - ], - "outEdges": [] - }, - { - "idx": 10, - "name": "test", - "inEdges": [], - "outEdges": [ - 12 - ] - }, - { - "idx": 11, - "name": "receiver_1722", - "inEdges": [], - "outEdges": [] - }, - { - "idx": 12, "name": "dump", "inEdges": [ 1 @@ -98,7 +51,7 @@ "outEdges": [] }, { - "idx": 13, + "idx": 8, "name": "emptyMap", "inEdges": [ 2 @@ -106,44 +59,10 @@ "outEdges": [] }, { - "idx": 14, - "name": "sender", - "inEdges": [ - 4, - 6 - ], - "outEdges": [] - }, - { - "idx": 15, - "name": "context", - "inEdges": [ - 7 - ], - "outEdges": [] - }, - { - "idx": 16, - "name": "myBalance", - "inEdges": [ - 8 - ], - "outEdges": [] - }, - { - "idx": 17, - "name": "nativeReserve", - "inEdges": [ - 9 - ], - "outEdges": [] - }, - { - "idx": 18, - "name": "send", + "idx": 9, + "name": "TestContractT::getA", "inEdges": [ - 10, - 11 + 3 ], "outEdges": [] } @@ -152,61 +71,16 @@ { "idx": 1, "src": 1, - "dst": 12 + "dst": 7 }, { "idx": 2, "src": 2, - "dst": 13 + "dst": 8 }, { "idx": 3, - "src": 3, - "dst": 5 - }, - { - "idx": 4, - "src": 3, - "dst": 14 - }, - { - "idx": 5, - "src": 4, - "dst": 5 - }, - { - "idx": 6, - "src": 4, - "dst": 14 - }, - { - "idx": 7, - "src": 5, - "dst": 15 - }, - { - "idx": 8, - "src": 5, - "dst": 16 - }, - { - "idx": 9, - "src": 5, - "dst": 17 - }, - { - "idx": 10, "src": 5, - "dst": 18 - }, - { - "idx": 11, - "src": 5, - "dst": 18 - }, - { - "idx": 12, - "src": 10, "dst": 9 } ] diff --git a/test/all/syntax.expected.callgraph.mmd b/test/all/syntax.expected.callgraph.mmd index 4393d605..720a23bb 100644 --- a/test/all/syntax.expected.callgraph.mmd +++ b/test/all/syntax.expected.callgraph.mmd @@ -1,31 +1,13 @@ graph TD node_1["test_try"] node_2["test_loops"] - node_3["reply"] - node_4["notify"] - node_5["forward"] - node_6["getter"] - node_7["getter"] - node_8["test"] - node_9["getA"] - node_10["test"] - node_11["receiver_1722"] - node_12["dump"] - node_13["emptyMap"] - node_14["sender"] - node_15["context"] - node_16["myBalance"] - node_17["nativeReserve"] - node_18["send"] - node_1 --> node_12 - node_2 --> node_13 - node_3 --> node_5 - node_3 --> node_14 - node_4 --> node_5 - node_4 --> node_14 - node_5 --> node_15 - node_5 --> node_16 - node_5 --> node_17 - node_5 --> node_18 - node_5 --> node_18 - node_10 --> node_9 + node_3["TestContract::getter"] + node_4["TestContractF::test"] + node_5["TestContractT::test"] + node_6["TestContractT::receiver_1722"] + node_7["dump"] + node_8["emptyMap"] + node_9["TestContractT::getA"] + node_1 --> node_7 + node_2 --> node_8 + node_5 --> node_9 diff --git a/test/detectors/SendInLoop.expected.out b/test/detectors/SendInLoop.expected.out index baf1f77c..4a2ed402 100644 --- a/test/detectors/SendInLoop.expected.out +++ b/test/detectors/SendInLoop.expected.out @@ -43,7 +43,7 @@ test/detectors/SendInLoop.tact:47:17: Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop -[MEDIUM] SendInLoop: Function "indirectSend" called inside a loop leads to a send function +[MEDIUM] SendInLoop: Function "SendInLoop::indirectSend" called inside a loop leads to a send function test/detectors/SendInLoop.tact:91:13: 90 | i += 1; > 91 | self.indirectSend(i); @@ -52,7 +52,7 @@ test/detectors/SendInLoop.tact:91:13: Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop -[MEDIUM] SendInLoop: Function "intermediateCall" called inside a loop leads to a send function +[MEDIUM] SendInLoop: Function "ComplexIndirectSend::intermediateCall" called inside a loop leads to a send function test/detectors/SendInLoop.tact:120:13: 119 | while (i < limit) { > 120 | self.intermediateCall(i); @@ -61,7 +61,7 @@ test/detectors/SendInLoop.tact:120:13: Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop -[MEDIUM] SendInLoop: Function "deepNestSend" called inside a loop leads to a send function +[MEDIUM] SendInLoop: Function "ComplexIndirectSend::deepNestSend" called inside a loop leads to a send function test/detectors/SendInLoop.tact:140:17: 139 | if (value > 0) { > 140 | self.deepNestSend(value); From ac93a285299cd807d7216cbe6d78f8acf697152e Mon Sep 17 00:00:00 2001 From: Esorat Date: Fri, 22 Nov 2024 16:55:55 +0700 Subject: [PATCH 06/10] Fix: Add a bitmask field to Node containing some information about it --- src/detectors/builtin/sendInLoop.ts | 75 +++++++------------------ src/internals/ir/callGraph.ts | 78 ++++++++++++++++++++++++-- test/detectors/SendInLoop.expected.out | 18 ------ 3 files changed, 93 insertions(+), 78 deletions(-) diff --git a/src/detectors/builtin/sendInLoop.ts b/src/detectors/builtin/sendInLoop.ts index 30dfeb4a..112f48e1 100644 --- a/src/detectors/builtin/sendInLoop.ts +++ b/src/detectors/builtin/sendInLoop.ts @@ -1,16 +1,11 @@ import { CompilationUnit } from "../../internals/ir"; -import { CallGraph, CGNodeId } from "../../internals/ir/callGraph"; -import { - forEachStatement, - foldExpressions, - isSelf, -} from "../../internals/tact"; +import { CallGraph } from "../../internals/ir/callGraph"; +import { forEachStatement, foldExpressions } from "../../internals/tact"; import { MistiTactWarning, Severity } from "../../internals/warnings"; import { ASTDetector } from "../detector"; import { AstStatement, AstExpression, - idText, AstStaticCall, AstMethodCall, AstContract, @@ -51,28 +46,6 @@ export class SendInLoop extends ASTDetector { const astStore = cu.ast; const ctx = this.ctx; const callGraph = new CallGraph(ctx).build(astStore); - const functionsCallingSend = new Set(); - - // Identify all functions that contain a send call - for (const func of astStore.getFunctions()) { - let containsSend = false; - foldExpressions( - func, - (acc, expr) => { - if (this.isSendCall(expr)) { - containsSend = true; - } - return acc; - }, - null, - ); - if (containsSend) { - const funcNodeId = callGraph.getNodeIdByAstId(func.id); - if (funcNodeId !== undefined) { - functionsCallingSend.add(funcNodeId); - } - } - } // Analyze loops and check if any function called within leads to a send for (const entry of cu.ast.getProgramEntries()) { @@ -84,7 +57,6 @@ export class SendInLoop extends ASTDetector { stmt, processedLoopIds, callGraph, - functionsCallingSend, contractName, ); allWarnings.push(...warnings); @@ -95,7 +67,6 @@ export class SendInLoop extends ASTDetector { stmt, processedLoopIds, callGraph, - functionsCallingSend, ); allWarnings.push(...warnings); }); @@ -108,7 +79,6 @@ export class SendInLoop extends ASTDetector { stmt: AstStatement, processedLoopIds: Set, callGraph: CallGraph, - functionsCallingSend: Set, currentContractName?: string, ): MistiTactWarning[] { if (processedLoopIds.has(stmt.id)) { @@ -148,25 +118,21 @@ export class SendInLoop extends ASTDetector { if (calleeName) { const calleeNodeId = callGraph.getNodeIdByName(calleeName); if (calleeNodeId !== undefined) { - // Check if the callee is connected to any function that calls send - for (const sendFuncNodeId of functionsCallingSend) { - if (callGraph.areConnected(calleeNodeId, sendFuncNodeId)) { - // Get the callee node to retrieve its name - const calleeNode = callGraph.getNode(calleeNodeId); - const calleeFunctionName = calleeNode?.name ?? calleeName; - - acc.push( - this.makeWarning( - `Function "${calleeFunctionName}" called inside a loop leads to a send function`, - expr.loc, - { - suggestion: - "Consider refactoring to avoid calling send functions inside loops", - }, - ), - ); - break; - } + const calleeNode = callGraph.getNode(calleeNodeId); + if ( + calleeNode && + calleeNode.hasFlag(0b0001 /* FLAG_CALLS_SEND */) + ) { + acc.push( + this.makeWarning( + `Function "${calleeNode.name}" called inside a loop leads to a send function`, + expr.loc, + { + suggestion: + "Consider refactoring to avoid calling send functions inside loops", + }, + ), + ); } } } @@ -186,10 +152,11 @@ export class SendInLoop extends ASTDetector { const selfMethodSendFunctions = ["reply", "forward", "notify", "emit"]; return ( (expr.kind === "static_call" && - staticSendFunctions.includes(idText(expr.function))) || + staticSendFunctions.includes(expr.function?.text || "")) || (expr.kind === "method_call" && - isSelf(expr.self) && - selfMethodSendFunctions.includes(idText(expr.method))) + expr.self.kind === "id" && + (expr.self as any).text === "self" && + selfMethodSendFunctions.includes(expr.method?.text || "")) ); } diff --git a/src/internals/ir/callGraph.ts b/src/internals/ir/callGraph.ts index 51e7ad20..e74a798b 100644 --- a/src/internals/ir/callGraph.ts +++ b/src/internals/ir/callGraph.ts @@ -19,6 +19,13 @@ import { export type CGNodeId = number & { readonly brand: unique symbol }; export type CGEdgeId = number & { readonly brand: unique symbol }; +/** + * Flag constants for CGNode. + */ +const FLAG_CALLS_SEND = 0b0001; // 1 +// You can define more flags as needed +// const FLAG_ANOTHER_PROPERTY = 0b0010; // 2 + /** * Represents an edge in the call graph, indicating a call from one function to another. * Each edge has a unique index (`idx`) generated using `IdxGenerator`. @@ -46,6 +53,7 @@ class CGNode { public idx: CGNodeId; public inEdges: Set = new Set(); public outEdges: Set = new Set(); + public flags: number = 0; /** * @param astId The AST ID of the function or method this node represents (can be `undefined` for synthetic nodes) @@ -62,6 +70,16 @@ class CGNode { this.logger.debug(`CGNode created without AST ID for function "${name}"`); } } + + // Method to set a flag + public setFlag(flag: number) { + this.flags |= flag; + } + + // Method to check if a flag is set + public hasFlag(flag: number): boolean { + return (this.flags & flag) !== 0; + } } /** @@ -257,6 +275,7 @@ export class CallGraph { /** * Analyzes the AST for function calls and adds edges between caller and callee nodes. + * Additionally, sets flags on nodes based on their properties (e.g., if they call 'send'). * @param astStore The AST store to analyze. */ private analyzeFunctionCalls(astStore: TactASTStore) { @@ -276,9 +295,20 @@ export class CallGraph { | AstReceiver; const funcNodeId = this.astIdToNodeId.get(func.id); if (funcNodeId !== undefined) { - forEachExpression(func, (expr) => - this.processExpression(expr, funcNodeId, contractName), - ); + let callsSend = false; + forEachExpression(func, (expr) => { + this.processExpression(expr, funcNodeId, contractName); + if (this.isSendCall(expr)) { + callsSend = true; + } + }); + // Set the flag if the function calls 'send' + if (callsSend) { + const funcNode = this.getNode(funcNodeId); + if (funcNode) { + funcNode.setFlag(FLAG_CALLS_SEND); + } + } } } } @@ -286,14 +316,41 @@ export class CallGraph { const func = entry as AstFunctionDef; const funcNodeId = this.astIdToNodeId.get(func.id); if (funcNodeId !== undefined) { - forEachExpression(func, (expr) => - this.processExpression(expr, funcNodeId), - ); + let callsSend = false; + forEachExpression(func, (expr) => { + this.processExpression(expr, funcNodeId); + if (this.isSendCall(expr)) { + callsSend = true; + } + }); + if (callsSend) { + const funcNode = this.getNode(funcNodeId); + if (funcNode) { + funcNode.setFlag(FLAG_CALLS_SEND); + } + } } } } } + /** + * Determines if the given expression is a 'send' call. + * @param expr The expression to check. + * @returns True if the expression is a 'send' call; otherwise, false. + */ + private isSendCall(expr: AstExpression): boolean { + const staticSendFunctions = ["send", "nativeSendMessage"]; + const selfMethodSendFunctions = ["reply", "forward", "notify", "emit"]; + return ( + (expr.kind === "static_call" && + staticSendFunctions.includes(expr.function?.text || "")) || + (expr.kind === "method_call" && + isSelf(expr.self) && + selfMethodSendFunctions.includes(expr.method?.text || "")) + ); + } + /** * Processes a single expression, identifying function or method calls to create edges. * @param expr The expression to process. @@ -385,3 +442,12 @@ export class CallGraph { } } } + +/** + * Helper function to check if an expression represents 'self'. + * @param expr The expression to check. + * @returns True if the expression is 'self'; otherwise, false. + */ +function isSelf(expr: AstExpression): boolean { + return expr.kind === "id" && (expr as AstId).text === "self"; +} diff --git a/test/detectors/SendInLoop.expected.out b/test/detectors/SendInLoop.expected.out index 4a2ed402..238d4d36 100644 --- a/test/detectors/SendInLoop.expected.out +++ b/test/detectors/SendInLoop.expected.out @@ -43,24 +43,6 @@ test/detectors/SendInLoop.tact:47:17: Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop -[MEDIUM] SendInLoop: Function "SendInLoop::indirectSend" called inside a loop leads to a send function -test/detectors/SendInLoop.tact:91:13: - 90 | i += 1; -> 91 | self.indirectSend(i); - ^ - 92 | } -Help: Consider refactoring to avoid calling send functions inside loops -See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop - -[MEDIUM] SendInLoop: Function "ComplexIndirectSend::intermediateCall" called inside a loop leads to a send function -test/detectors/SendInLoop.tact:120:13: - 119 | while (i < limit) { -> 120 | self.intermediateCall(i); - ^ - 121 | i += 1; -Help: Consider refactoring to avoid calling send functions inside loops -See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop - [MEDIUM] SendInLoop: Function "ComplexIndirectSend::deepNestSend" called inside a loop leads to a send function test/detectors/SendInLoop.tact:140:17: 139 | if (value > 0) { From dc44716fa2a63384e43d0dc3d5401e70e03114a5 Mon Sep 17 00:00:00 2001 From: Esorat Date: Fri, 22 Nov 2024 17:46:53 +0700 Subject: [PATCH 07/10] chore: rm comments --- src/detectors/builtin/sendInLoop.ts | 2 +- src/internals/ir/callGraph.ts | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/detectors/builtin/sendInLoop.ts b/src/detectors/builtin/sendInLoop.ts index 112f48e1..33aba8aa 100644 --- a/src/detectors/builtin/sendInLoop.ts +++ b/src/detectors/builtin/sendInLoop.ts @@ -121,7 +121,7 @@ export class SendInLoop extends ASTDetector { const calleeNode = callGraph.getNode(calleeNodeId); if ( calleeNode && - calleeNode.hasFlag(0b0001 /* FLAG_CALLS_SEND */) + calleeNode.hasFlag(0b0001) ) { acc.push( this.makeWarning( diff --git a/src/internals/ir/callGraph.ts b/src/internals/ir/callGraph.ts index e74a798b..269a46df 100644 --- a/src/internals/ir/callGraph.ts +++ b/src/internals/ir/callGraph.ts @@ -22,9 +22,7 @@ export type CGEdgeId = number & { readonly brand: unique symbol }; /** * Flag constants for CGNode. */ -const FLAG_CALLS_SEND = 0b0001; // 1 -// You can define more flags as needed -// const FLAG_ANOTHER_PROPERTY = 0b0010; // 2 +const FLAG_CALLS_SEND = 0b0001; /** * Represents an edge in the call graph, indicating a call from one function to another. @@ -389,7 +387,7 @@ export class CallGraph { currentContractName?: string, ): string | undefined { if (expr.kind === "static_call") { - return expr.function?.text; // Static calls directly reference free functions + return expr.function?.text; } else if (expr.kind === "method_call") { const methodName = expr.method?.text; if (methodName) { @@ -397,7 +395,7 @@ export class CallGraph { if (expr.self.kind === "id") { const idExpr = expr.self as AstId; if (idExpr.text !== "self") { - contractName = idExpr.text; // Refers to another contract + contractName = idExpr.text; } } return contractName ? `${contractName}::${methodName}` : methodName; From c97cc97f52e4b1a0d39042cf880754be080ec456 Mon Sep 17 00:00:00 2001 From: Esorat Date: Fri, 22 Nov 2024 18:32:04 +0700 Subject: [PATCH 08/10] Fix: move isSend to src/internals/tact/util.ts Fix: callgraph id Chore: add comments to Flag const for CGnode --- src/detectors/builtin/sendInLoop.ts | 8 +++----- src/internals/ir/callGraph.ts | 32 +++++++++-------------------- src/internals/tact/util.ts | 17 +++++++++++++++ 3 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/detectors/builtin/sendInLoop.ts b/src/detectors/builtin/sendInLoop.ts index 33aba8aa..1144a781 100644 --- a/src/detectors/builtin/sendInLoop.ts +++ b/src/detectors/builtin/sendInLoop.ts @@ -1,6 +1,7 @@ import { CompilationUnit } from "../../internals/ir"; import { CallGraph } from "../../internals/ir/callGraph"; import { forEachStatement, foldExpressions } from "../../internals/tact"; +import { isSendCall } from "../../internals/tact/util"; import { MistiTactWarning, Severity } from "../../internals/warnings"; import { ASTDetector } from "../detector"; import { @@ -93,7 +94,7 @@ export class SendInLoop extends ASTDetector { foldExpressions( stmt, (acc: MistiTactWarning[], expr: AstExpression) => { - if (this.isSendCall(expr)) { + if (isSendCall(expr)) { acc.push( this.makeWarning("Send function called inside a loop", expr.loc, { suggestion: @@ -119,10 +120,7 @@ export class SendInLoop extends ASTDetector { const calleeNodeId = callGraph.getNodeIdByName(calleeName); if (calleeNodeId !== undefined) { const calleeNode = callGraph.getNode(calleeNodeId); - if ( - calleeNode && - calleeNode.hasFlag(0b0001) - ) { + if (calleeNode && calleeNode.hasFlag(0b0001)) { acc.push( this.makeWarning( `Function "${calleeNode.name}" called inside a loop leads to a send function`, diff --git a/src/internals/ir/callGraph.ts b/src/internals/ir/callGraph.ts index 269a46df..1650fafd 100644 --- a/src/internals/ir/callGraph.ts +++ b/src/internals/ir/callGraph.ts @@ -4,6 +4,7 @@ import { IdxGenerator } from "./indices"; import { MistiContext } from "../../"; import { Logger } from "../../internals/logger"; import { forEachExpression } from "../tact/iterators"; +import { isSendCall } from "../tact/util"; import { AstFunctionDef, AstReceiver, @@ -14,6 +15,7 @@ import { AstContract, AstId, AstContractDeclaration, + AstNode, } from "@tact-lang/compiler/dist/grammar/ast"; export type CGNodeId = number & { readonly brand: unique symbol }; @@ -21,8 +23,11 @@ export type CGEdgeId = number & { readonly brand: unique symbol }; /** * Flag constants for CGNode. + * + * `FLAG_CALLS_SEND` (0b0001): Indicates that the function represented by this node + * contains a direct or indirect call to a "send" function. */ -const FLAG_CALLS_SEND = 0b0001; +const FLAG_CALLS_SEND = 0b0001; /** * Represents an edge in the call graph, indicating a call from one function to another. @@ -89,7 +94,7 @@ class CGNode { */ export class CallGraph { private nodeMap: Map = new Map(); - private astIdToNodeId: Map = new Map(); + private astIdToNodeId: Map = new Map(); private nameToNodeId: Map = new Map(); private edgesMap: Map = new Map(); private logger: Logger; @@ -296,7 +301,7 @@ export class CallGraph { let callsSend = false; forEachExpression(func, (expr) => { this.processExpression(expr, funcNodeId, contractName); - if (this.isSendCall(expr)) { + if (isSendCall(expr)) { callsSend = true; } }); @@ -317,7 +322,7 @@ export class CallGraph { let callsSend = false; forEachExpression(func, (expr) => { this.processExpression(expr, funcNodeId); - if (this.isSendCall(expr)) { + if (isSendCall(expr)) { callsSend = true; } }); @@ -332,23 +337,6 @@ export class CallGraph { } } - /** - * Determines if the given expression is a 'send' call. - * @param expr The expression to check. - * @returns True if the expression is a 'send' call; otherwise, false. - */ - private isSendCall(expr: AstExpression): boolean { - const staticSendFunctions = ["send", "nativeSendMessage"]; - const selfMethodSendFunctions = ["reply", "forward", "notify", "emit"]; - return ( - (expr.kind === "static_call" && - staticSendFunctions.includes(expr.function?.text || "")) || - (expr.kind === "method_call" && - isSelf(expr.self) && - selfMethodSendFunctions.includes(expr.method?.text || "")) - ); - } - /** * Processes a single expression, identifying function or method calls to create edges. * @param expr The expression to process. @@ -446,6 +434,6 @@ export class CallGraph { * @param expr The expression to check. * @returns True if the expression is 'self'; otherwise, false. */ -function isSelf(expr: AstExpression): boolean { +export function isSelf(expr: AstExpression): boolean { return expr.kind === "id" && (expr as AstId).text === "self"; } diff --git a/src/internals/tact/util.ts b/src/internals/tact/util.ts index 4188b4d0..fddf3cf5 100644 --- a/src/internals/tact/util.ts +++ b/src/internals/tact/util.ts @@ -364,3 +364,20 @@ export function srcInfoToString(loc: SrcInfo): string { : ""; return `${shownPath}${lc.lineNum}:${lc.colNum}:\n${lcLines.join("\n")}`; } + +/** + * Determines if the given expression is a 'send' call. + * @param expr The expression to check. + * @returns True if the expression is a 'send' call; otherwise, false. + */ +export function isSendCall(expr: AstExpression): boolean { + const staticSendFunctions = ["send", "nativeSendMessage"]; + const selfMethodSendFunctions = ["reply", "forward", "notify", "emit"]; + return ( + (expr.kind === "static_call" && + staticSendFunctions.includes(expr.function?.text || "")) || + (expr.kind === "method_call" && + isSelf(expr.self) && + selfMethodSendFunctions.includes(expr.method?.text || "")) + ); +} From b2ab51b314293c4df3dc2098225a43e040fc567f Mon Sep 17 00:00:00 2001 From: Esorat Date: Sat, 23 Nov 2024 18:44:22 +0700 Subject: [PATCH 09/10] Fix: Add findInExpressions --- src/internals/ir/callGraph.ts | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/internals/ir/callGraph.ts b/src/internals/ir/callGraph.ts index 1650fafd..4f178a0d 100644 --- a/src/internals/ir/callGraph.ts +++ b/src/internals/ir/callGraph.ts @@ -3,7 +3,7 @@ import { TactASTStore } from "./astStore"; import { IdxGenerator } from "./indices"; import { MistiContext } from "../../"; import { Logger } from "../../internals/logger"; -import { forEachExpression } from "../tact/iterators"; +import { findInExpressions, forEachExpression } from "../tact/iterators"; import { isSendCall } from "../tact/util"; import { AstFunctionDef, @@ -298,15 +298,12 @@ export class CallGraph { | AstReceiver; const funcNodeId = this.astIdToNodeId.get(func.id); if (funcNodeId !== undefined) { - let callsSend = false; forEachExpression(func, (expr) => { this.processExpression(expr, funcNodeId, contractName); - if (isSendCall(expr)) { - callsSend = true; - } }); - // Set the flag if the function calls 'send' - if (callsSend) { + const sendCallFound = + findInExpressions(func, isSendCall) !== null; + if (sendCallFound) { const funcNode = this.getNode(funcNodeId); if (funcNode) { funcNode.setFlag(FLAG_CALLS_SEND); @@ -319,14 +316,11 @@ export class CallGraph { const func = entry as AstFunctionDef; const funcNodeId = this.astIdToNodeId.get(func.id); if (funcNodeId !== undefined) { - let callsSend = false; forEachExpression(func, (expr) => { this.processExpression(expr, funcNodeId); - if (isSendCall(expr)) { - callsSend = true; - } }); - if (callsSend) { + const sendCallFound = findInExpressions(func, isSendCall) !== null; + if (sendCallFound) { const funcNode = this.getNode(funcNodeId); if (funcNode) { funcNode.setFlag(FLAG_CALLS_SEND); From 39b3ccf3acabbc9e25db3b8e7d8e929bc816fd3a Mon Sep 17 00:00:00 2001 From: Esorat Date: Sun, 24 Nov 2024 16:31:35 +0700 Subject: [PATCH 10/10] Fix: Adjustment and clean warning message --- src/detectors/builtin/sendInLoop.ts | 5 ++++- test/detectors/SendInLoop.expected.out | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/detectors/builtin/sendInLoop.ts b/src/detectors/builtin/sendInLoop.ts index 1144a781..7a854b15 100644 --- a/src/detectors/builtin/sendInLoop.ts +++ b/src/detectors/builtin/sendInLoop.ts @@ -121,9 +121,12 @@ export class SendInLoop extends ASTDetector { if (calleeNodeId !== undefined) { const calleeNode = callGraph.getNode(calleeNodeId); if (calleeNode && calleeNode.hasFlag(0b0001)) { + const functionName = calleeNode.name.includes("::") + ? calleeNode.name.split("::").pop() + : calleeNode.name; acc.push( this.makeWarning( - `Function "${calleeNode.name}" called inside a loop leads to a send function`, + `Method "${functionName}" called inside a loop leads to a send function`, expr.loc, { suggestion: diff --git a/test/detectors/SendInLoop.expected.out b/test/detectors/SendInLoop.expected.out index 238d4d36..99b31303 100644 --- a/test/detectors/SendInLoop.expected.out +++ b/test/detectors/SendInLoop.expected.out @@ -43,7 +43,7 @@ test/detectors/SendInLoop.tact:47:17: Help: Consider refactoring to avoid calling send functions inside loops See: https://nowarp.io/tools/misti/docs/detectors/SendInLoop -[MEDIUM] SendInLoop: Function "ComplexIndirectSend::deepNestSend" called inside a loop leads to a send function +[MEDIUM] SendInLoop: Method "deepNestSend" called inside a loop leads to a send function test/detectors/SendInLoop.tact:140:17: 139 | if (value > 0) { > 140 | self.deepNestSend(value);