Skip to content

Commit 0317d56

Browse files
JeewoongKimJeewoongKim
committed
Improve CERT DCL30-C stack return analysis
1 parent aaac22d commit 0317d56

6 files changed

Lines changed: 103 additions & 41 deletions

File tree

‎c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ DCL30-C = Union( CWE-562, list) where list =
183183
184184
## Implementation notes
185185
186-
The rule checks specifically for pointers to objects with automatic storage duration that are returned by functions or assigned to function output parameters.
186+
The rule checks specifically for pointers to objects with automatic storage duration that are returned by functions.
187187
188188
## References
189189

‎c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.ql‎

Lines changed: 53 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* @name DCL30-C: Declare objects with appropriate storage durations
44
* @description When pointers to local variables are returned by a function it can lead to referring
55
* to objects outside of their lifetime, which is undefined behaviour.
6-
* @kind problem
6+
* @kind path-problem
77
* @precision high
88
* @problem.severity error
99
* @tags external/cert/id/dcl30-c
@@ -18,41 +18,65 @@
1818

1919
import cpp
2020
import codingstandards.c.cert
21-
import codingstandards.c.Objects
22-
import semmle.code.cpp.dataflow.DataFlow
21+
import semmle.code.cpp.ir.IR
22+
import semmle.code.cpp.ir.dataflow.MustFlow
23+
import PathGraph
2324

24-
class Source extends Expr {
25-
ObjectIdentity rootObject;
25+
/** Holds if `f` appears to intentionally return a stack pointer. */
26+
predicate intentionallyReturnsStackPointer(Function f) {
27+
f.getName().toLowerCase().matches(["%stack%", "%sp%"])
28+
}
29+
30+
/** Configuration for detecting stack-allocated memory returned by a function. */
31+
class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
32+
ReturnStackAllocatedMemoryConfig() { this = "DCL30CReturnStackAllocatedMemoryConfig" }
2633

27-
Source() {
28-
rootObject.getStorageDuration().isAutomatic() and
29-
this = rootObject.getASubobjectAddressExpr()
34+
override predicate isSource(Instruction source) {
35+
exists(Function func |
36+
not func.hasErrors() and
37+
not intentionallyReturnsStackPointer(func) and
38+
func = source.getEnclosingFunction()
39+
|
40+
exists(VariableAddressInstruction var |
41+
var = source and
42+
var.getAstVariable() instanceof StackVariable and
43+
not var.getResultType() instanceof PointerToMemberType
44+
)
45+
or
46+
exists(Call call |
47+
call.getTarget().hasGlobalName(["alloca", "strdupa", "strndupa", "_alloca", "_malloca"]) and
48+
source.getUnconvertedResultExpression() = call
49+
)
50+
)
3051
}
31-
}
3252

33-
class Sink extends DataFlow::Node {
34-
Sink() {
35-
//output parameter
36-
exists(Parameter f |
37-
f.getAnAccess() = this.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() and
38-
f.getUnderlyingType() instanceof PointerType
53+
override predicate isSink(Operand sink) {
54+
exists(StoreInstruction store |
55+
store.getDestinationAddress().(VariableAddressInstruction).getIRVariable() instanceof
56+
IRReturnVariable and
57+
sink = store.getSourceValueOperand()
3958
)
59+
}
60+
61+
override predicate allowInterproceduralFlow() { none() }
62+
63+
override predicate isAdditionalFlowStep(Operand node1, Instruction node2) {
64+
node2.(FieldAddressInstruction).getObjectAddressOperand() = node1
4065
or
41-
//function returns pointer
42-
exists(Function f, ReturnStmt r |
43-
f.getType() instanceof PointerType and
44-
r.getEnclosingFunction() = f and
45-
r.getExpr() = this.asExpr()
46-
)
66+
node2.(PointerOffsetInstruction).getLeftOperand() = node1
4767
}
68+
69+
override predicate isBarrier(Instruction n) { n.getResultType() instanceof ErroneousType }
4870
}
4971

50-
from DataFlow::Node src, DataFlow::Node sink
72+
from
73+
MustFlowPathNode source, MustFlowPathNode sink, Instruction instr,
74+
ReturnStackAllocatedMemoryConfig conf
5175
where
52-
not isExcluded(sink.asExpr(),
53-
Declarations8Package::appropriateStorageDurationsFunctionReturnQuery()) and
54-
exists(Source s | src.asExpr() = s) and
55-
sink instanceof Sink and
56-
DataFlow::localFlow(src, sink)
57-
select sink, "$@ with automatic storage may be accessible outside of its lifetime.", src,
58-
src.toString()
76+
conf.hasFlowPath(pragma[only_bind_into](source), pragma[only_bind_into](sink)) and
77+
source.getInstruction() = instr and
78+
not isExcluded(sink.getInstruction().getAst(),
79+
Declarations8Package::appropriateStorageDurationsFunctionReturnQuery())
80+
select sink.getInstruction(), source, sink,
81+
"$@ with automatic storage may be accessible outside of its lifetime.", instr.getAst(),
82+
instr.getAst().toString()

‎c/cert/test/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.expected‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,19 @@
1-
WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:33,20-28)
2-
WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:37,31-39)
3-
WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:50,6-14)
4-
WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:50,26-34)
5-
WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:56,3-11)
6-
| 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 |
7-
| 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 |
1+
edges
2+
| test.c:3:10:3:10 | a | test.c:3:10:3:10 | array to pointer conversion |
3+
| test.c:38:10:38:11 | a6 | test.c:38:10:38:11 | array to pointer conversion |
4+
| test.c:38:10:38:11 | array to pointer conversion | test.c:38:10:38:15 | ... + ... |
5+
| test.c:47:11:47:11 | s | test.c:47:13:47:13 | x |
6+
| test.c:47:13:47:13 | x | test.c:47:10:47:13 | & ... |
7+
nodes
8+
| test.c:3:10:3:10 | a | semmle.label | a |
9+
| test.c:3:10:3:10 | array to pointer conversion | semmle.label | array to pointer conversion |
10+
| test.c:38:10:38:11 | a6 | semmle.label | a6 |
11+
| test.c:38:10:38:11 | array to pointer conversion | semmle.label | array to pointer conversion |
12+
| test.c:38:10:38:15 | ... + ... | semmle.label | ... + ... |
13+
| test.c:47:10:47:13 | & ... | semmle.label | & ... |
14+
| test.c:47:11:47:11 | s | semmle.label | s |
15+
| test.c:47:13:47:13 | x | semmle.label | x |
16+
#select
17+
| 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 |
18+
| 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 |
19+
| 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 |

‎c/cert/test/rules/DCL30-C/test.c‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,4 +31,27 @@ void f5(void) {
3131
const char a5[] = "test";
3232
g = a5; // COMPLIANT[FALSE_POSITIVE]
3333
g = NULL;
34-
}
34+
}
35+
36+
char *f6(void) {
37+
char a6[10];
38+
return a6 + 3; // NON_COMPLIANT
39+
}
40+
41+
struct S {
42+
char x;
43+
};
44+
45+
char *f7(void) {
46+
struct S s;
47+
return &s.x; // NON_COMPLIANT
48+
}
49+
50+
char *f8(char *p) {
51+
return p; // COMPLIANT
52+
}
53+
54+
void f9(void) {
55+
char x;
56+
char *p = f8(&x); // COMPLIANT
57+
}

‎change_notes/2026-08-28-fix-dcl30-c-return-stack-memory.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
- `DCL30-C` - `AppropriateStorageDurationsFunctionReturn.ql`:
2+
- Replaced the legacy local data-flow implementation with the improved stack-allocated-memory return analysis.
3+
- Improved detection of stack-derived pointer returns, including pointer offsets.

‎rule_packages/c/Declarations8.json‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
},
2828
{
2929
"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.",
30-
"kind": "problem",
30+
"kind": "path-problem",
3131
"name": "Declare objects with appropriate storage durations",
3232
"precision": "high",
3333
"severity": "error",
@@ -41,11 +41,11 @@
4141
"external/cert/level/l2"
4242
],
4343
"implementation_scope": {
44-
"description": "The rule checks specifically for pointers to objects with automatic storage duration that are returned by functions or assigned to function output parameters."
44+
"description": "The rule checks specifically for pointers to objects with automatic storage duration that are returned by functions."
4545
}
4646
}
4747
],
4848
"title": "Declare objects with appropriate storage durations"
4949
}
5050
}
51-
}
51+
}

0 commit comments

Comments
 (0)