diff --git a/c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.md b/c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.md index 8124cd49cd..58e887ff87 100644 --- a/c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.md +++ b/c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.md @@ -183,7 +183,7 @@ DCL30-C = Union( CWE-562, list) where list = ## Implementation notes -The rule checks specifically for pointers to objects with automatic storage duration that are returned by functions or assigned to function output parameters. +The rule checks specifically for pointers to objects with automatic storage duration that are returned by functions. ## References diff --git a/c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.ql b/c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.ql index 2e1064ee9d..e0d499888f 100644 --- a/c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.ql +++ b/c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.ql @@ -3,7 +3,7 @@ * @name DCL30-C: Declare objects with appropriate storage durations * @description When pointers to local variables are returned by a function it can lead to referring * to objects outside of their lifetime, which is undefined behaviour. - * @kind problem + * @kind path-problem * @precision high * @problem.severity error * @tags external/cert/id/dcl30-c @@ -18,41 +18,65 @@ import cpp import codingstandards.c.cert -import codingstandards.c.Objects -import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.ir.IR +import semmle.code.cpp.ir.dataflow.MustFlow +import PathGraph -class Source extends Expr { - ObjectIdentity rootObject; +/** Holds if `f` appears to intentionally return a stack pointer. */ +predicate intentionallyReturnsStackPointer(Function f) { + f.getName().toLowerCase().matches(["%stack%", "%sp%"]) +} + +/** Configuration for detecting stack-allocated memory returned by a function. */ +class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration { + ReturnStackAllocatedMemoryConfig() { this = "DCL30CReturnStackAllocatedMemoryConfig" } - Source() { - rootObject.getStorageDuration().isAutomatic() and - this = rootObject.getASubobjectAddressExpr() + override predicate isSource(Instruction source) { + exists(Function func | + not func.hasErrors() and + not intentionallyReturnsStackPointer(func) and + func = source.getEnclosingFunction() + | + exists(VariableAddressInstruction var | + var = source and + var.getAstVariable() instanceof StackVariable and + not var.getResultType() instanceof PointerToMemberType + ) + or + exists(Call call | + call.getTarget().hasGlobalName(["alloca", "strdupa", "strndupa", "_alloca", "_malloca"]) and + source.getUnconvertedResultExpression() = call + ) + ) } -} -class Sink extends DataFlow::Node { - Sink() { - //output parameter - exists(Parameter f | - f.getAnAccess() = this.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() and - f.getUnderlyingType() instanceof PointerType + override predicate isSink(Operand sink) { + exists(StoreInstruction store | + store.getDestinationAddress().(VariableAddressInstruction).getIRVariable() instanceof + IRReturnVariable and + sink = store.getSourceValueOperand() ) + } + + override predicate allowInterproceduralFlow() { none() } + + override predicate isAdditionalFlowStep(Operand node1, Instruction node2) { + node2.(FieldAddressInstruction).getObjectAddressOperand() = node1 or - //function returns pointer - exists(Function f, ReturnStmt r | - f.getType() instanceof PointerType and - r.getEnclosingFunction() = f and - r.getExpr() = this.asExpr() - ) + node2.(PointerOffsetInstruction).getLeftOperand() = node1 } + + override predicate isBarrier(Instruction n) { n.getResultType() instanceof ErroneousType } } -from DataFlow::Node src, DataFlow::Node sink +from + MustFlowPathNode source, MustFlowPathNode sink, Instruction instr, + ReturnStackAllocatedMemoryConfig conf where - not isExcluded(sink.asExpr(), - Declarations8Package::appropriateStorageDurationsFunctionReturnQuery()) and - exists(Source s | src.asExpr() = s) and - sink instanceof Sink and - DataFlow::localFlow(src, sink) -select sink, "$@ with automatic storage may be accessible outside of its lifetime.", src, - src.toString() + conf.hasFlowPath(pragma[only_bind_into](source), pragma[only_bind_into](sink)) and + source.getInstruction() = instr and + not isExcluded(sink.getInstruction().getAst(), + Declarations8Package::appropriateStorageDurationsFunctionReturnQuery()) +select sink.getInstruction(), source, sink, + "$@ with automatic storage may be accessible outside of its lifetime.", instr.getAst(), + instr.getAst().toString() diff --git a/c/cert/test/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.expected b/c/cert/test/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.expected index a4359d7000..aebc02b254 100644 --- a/c/cert/test/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.expected +++ b/c/cert/test/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.expected @@ -1,7 +1,19 @@ -WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:33,20-28) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:37,31-39) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:50,6-14) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:50,26-34) -WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:56,3-11) -| test.c:3:10:3:10 | a | $@ with automatic storage may be accessible outside of its lifetime. | test.c:3:10:3:10 | a | a | -| test.c:15:4:15:8 | param [inner post update] | $@ with automatic storage may be accessible outside of its lifetime. | test.c:15:12:15:13 | a2 | a2 | +edges +| test.c:3:10:3:10 | a | test.c:3:10:3:10 | array to pointer conversion | +| test.c:38:10:38:11 | a6 | test.c:38:10:38:11 | array to pointer conversion | +| test.c:38:10:38:11 | array to pointer conversion | test.c:38:10:38:15 | ... + ... | +| test.c:47:11:47:11 | s | test.c:47:13:47:13 | x | +| test.c:47:13:47:13 | x | test.c:47:10:47:13 | & ... | +nodes +| test.c:3:10:3:10 | a | semmle.label | a | +| test.c:3:10:3:10 | array to pointer conversion | semmle.label | array to pointer conversion | +| test.c:38:10:38:11 | a6 | semmle.label | a6 | +| test.c:38:10:38:11 | array to pointer conversion | semmle.label | array to pointer conversion | +| test.c:38:10:38:15 | ... + ... | semmle.label | ... + ... | +| test.c:47:10:47:13 | & ... | semmle.label | & ... | +| test.c:47:11:47:11 | s | semmle.label | s | +| test.c:47:13:47:13 | x | semmle.label | x | +#select +| test.c:3:10:3:10 | Convert: array to pointer conversion | test.c:3:10:3:10 | a | test.c:3:10:3:10 | array to pointer conversion | $@ with automatic storage may be accessible outside of its lifetime. | test.c:3:10:3:10 | a | a | +| test.c:38:10:38:15 | PointerAdd: ... + ... | test.c:38:10:38:11 | a6 | test.c:38:10:38:15 | ... + ... | $@ with automatic storage may be accessible outside of its lifetime. | test.c:38:10:38:11 | a6 | a6 | +| test.c:47:10:47:13 | CopyValue: & ... | test.c:47:11:47:11 | s | test.c:47:10:47:13 | & ... | $@ with automatic storage may be accessible outside of its lifetime. | test.c:47:11:47:11 | s | s | diff --git a/c/cert/test/rules/DCL30-C/test.c b/c/cert/test/rules/DCL30-C/test.c index f703f158c0..43ff36c4b9 100644 --- a/c/cert/test/rules/DCL30-C/test.c +++ b/c/cert/test/rules/DCL30-C/test.c @@ -31,4 +31,27 @@ void f5(void) { const char a5[] = "test"; g = a5; // COMPLIANT[FALSE_POSITIVE] g = NULL; -} \ No newline at end of file +} + +char *f6(void) { + char a6[10]; + return a6 + 3; // NON_COMPLIANT +} + +struct S { + char x; +}; + +char *f7(void) { + struct S s; + return &s.x; // NON_COMPLIANT +} + +char *f8(char *p) { + return p; // COMPLIANT +} + +void f9(void) { + char x; + char *p = f8(&x); // COMPLIANT +} diff --git a/change_notes/2026-08-28-fix-dcl30-c-return-stack-memory.md b/change_notes/2026-08-28-fix-dcl30-c-return-stack-memory.md new file mode 100644 index 0000000000..07a05845ed --- /dev/null +++ b/change_notes/2026-08-28-fix-dcl30-c-return-stack-memory.md @@ -0,0 +1,3 @@ +- `DCL30-C` - `AppropriateStorageDurationsFunctionReturn.ql`: + - Replaced the legacy local data-flow implementation with the improved stack-allocated-memory return analysis. + - Improved detection of stack-derived pointer returns, including pointer offsets. diff --git a/rule_packages/c/Declarations8.json b/rule_packages/c/Declarations8.json index 6275e32595..9bc71515e1 100644 --- a/rule_packages/c/Declarations8.json +++ b/rule_packages/c/Declarations8.json @@ -27,7 +27,7 @@ }, { "description": "When pointers to local variables are returned by a function it can lead to referring to objects outside of their lifetime, which is undefined behaviour.", - "kind": "problem", + "kind": "path-problem", "name": "Declare objects with appropriate storage durations", "precision": "high", "severity": "error", @@ -41,11 +41,11 @@ "external/cert/level/l2" ], "implementation_scope": { - "description": "The rule checks specifically for pointers to objects with automatic storage duration that are returned by functions or assigned to function output parameters." + "description": "The rule checks specifically for pointers to objects with automatic storage duration that are returned by functions." } } ], "title": "Declare objects with appropriate storage durations" } } -} \ No newline at end of file +}