diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll index 904701144ca5..b085440f6bcd 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll @@ -31,6 +31,11 @@ abstract class MustFlowConfiguration extends string { */ abstract predicate isSink(Operand sink); + /** + * Holds if data flow through `instr` is prohibited. + */ + predicate isBarrier(Instruction instr) { none() } + /** * Holds if the additional flow step from `node1` to `node2` must be taken * into account in the analysis. @@ -48,18 +53,21 @@ abstract class MustFlowConfiguration extends string { */ final predicate hasFlowPath(MustFlowPathNode source, MustFlowPathSink sink) { this.isSource(source.getInstruction()) and - source.getASuccessor+() = sink + source.getASuccessor*() = sink } } /** Holds if `node` flows from a source. */ pragma[nomagic] private predicate flowsFromSource(Instruction node, MustFlowConfiguration config) { - config.isSource(node) - or - exists(Instruction mid | - step(mid, node, config) and - flowsFromSource(mid, pragma[only_bind_into](config)) + not config.isBarrier(node) and + ( + config.isSource(node) + or + exists(Instruction mid | + step(mid, node, config) and + flowsFromSource(mid, pragma[only_bind_into](config)) + ) ) } diff --git a/cpp/ql/src/Likely Bugs/Memory Management/UninitializedLocal.ql b/cpp/ql/src/Likely Bugs/Memory Management/UninitializedLocal.ql index 0fbd1707db04..85bfb963fb9a 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/UninitializedLocal.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/UninitializedLocal.ql @@ -13,7 +13,8 @@ */ import cpp -import semmle.code.cpp.controlflow.StackVariableReachability +import semmle.code.cpp.ir.IR +import semmle.code.cpp.ir.dataflow.MustFlow /** * Auxiliary predicate: Types that don't require initialization @@ -33,31 +34,6 @@ predicate allocatedType(Type t) { allocatedType(t.getUnspecifiedType()) } -/** - * A declaration of a local variable that leaves the - * variable uninitialized. - */ -DeclStmt declWithNoInit(LocalVariable v) { - result.getADeclaration() = v and - not exists(v.getInitializer()) and - /* The type of the variable is not stack-allocated. */ - exists(Type t | t = v.getType() | not allocatedType(t)) -} - -class UninitialisedLocalReachability extends StackVariableReachability { - UninitialisedLocalReachability() { this = "UninitialisedLocal" } - - override predicate isSource(ControlFlowNode node, StackVariable v) { node = declWithNoInit(v) } - - override predicate isSink(ControlFlowNode node, StackVariable v) { useOfVarActual(v, node) } - - override predicate isBarrier(ControlFlowNode node, StackVariable v) { - // only report the _first_ possibly uninitialized use - useOfVarActual(v, node) or - definitionBarrier(v, node) - } -} - pragma[noinline] predicate containsInlineAssembly(Function f) { exists(AsmStmt s | s.getEnclosingFunction() = f) } @@ -82,8 +58,33 @@ VariableAccess commonException() { containsInlineAssembly(result.getEnclosingFunction()) } -from UninitialisedLocalReachability r, LocalVariable v, VariableAccess va +predicate isSinkImpl(Instruction sink, VariableAccess va) { + exists(LoadInstruction load | + va = load.getUnconvertedResultExpression() and + not va = commonException() and + sink = load.getSourceValue() + ) +} + +class MustFlow extends MustFlowConfiguration { + MustFlow() { this = "MustFlow" } + + override predicate isSource(Instruction source) { + source instanceof UninitializedInstruction and + exists(Type t | t = source.getResultType() | not allocatedType(t)) + } + + override predicate isSink(Operand sink) { isSinkImpl(sink.getDef(), _) } + + override predicate allowInterproceduralFlow() { none() } + + override predicate isBarrier(Instruction instr) { instr instanceof ChiInstruction } +} + +from + VariableAccess va, LocalVariable v, MustFlow conf, MustFlowPathNode source, MustFlowPathNode sink where - r.reaches(_, v, va) and - not va = commonException() + conf.hasFlowPath(source, sink) and + isSinkImpl(sink.getInstruction(), va) and + v = va.getTarget() select va, "The variable $@ may not be initialized at this access.", v, v.getName() diff --git a/cpp/ql/src/change-notes/2023-11-07-uninitialized-local.md b/cpp/ql/src/change-notes/2023-11-07-uninitialized-local.md new file mode 100644 index 000000000000..bdb676925344 --- /dev/null +++ b/cpp/ql/src/change-notes/2023-11-07-uninitialized-local.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `cpp/uninitialized-local` query has been improved to produce fewer false positives. diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/UninitializedLocal.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/UninitializedLocal.expected index 2b5e6549a30c..7b5233f45b49 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/UninitializedLocal.expected +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/UninitializedLocal.expected @@ -1,14 +1,7 @@ | test.cpp:12:6:12:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:11:6:11:8 | foo | foo | -| test.cpp:30:6:30:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:26:6:26:8 | foo | foo | -| test.cpp:46:6:46:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:42:6:42:8 | foo | foo | -| test.cpp:55:7:55:9 | foo | The variable $@ may not be initialized at this access. | test.cpp:50:6:50:8 | foo | foo | -| test.cpp:67:7:67:9 | foo | The variable $@ may not be initialized at this access. | test.cpp:61:6:61:8 | foo | foo | -| test.cpp:92:6:92:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:82:6:82:8 | foo | foo | | test.cpp:113:6:113:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:111:6:111:8 | foo | foo | -| test.cpp:132:9:132:9 | j | The variable $@ may not be initialized at this access. | test.cpp:126:6:126:6 | j | j | | test.cpp:219:3:219:3 | x | The variable $@ may not be initialized at this access. | test.cpp:218:7:218:7 | x | x | | test.cpp:243:13:243:13 | i | The variable $@ may not be initialized at this access. | test.cpp:241:6:241:6 | i | i | -| test.cpp:329:9:329:11 | val | The variable $@ may not be initialized at this access. | test.cpp:321:6:321:8 | val | val | | test.cpp:336:10:336:10 | a | The variable $@ may not be initialized at this access. | test.cpp:333:7:333:7 | a | a | | test.cpp:369:10:369:10 | a | The variable $@ may not be initialized at this access. | test.cpp:358:7:358:7 | a | a | | test.cpp:378:9:378:11 | val | The variable $@ may not be initialized at this access. | test.cpp:359:6:359:8 | val | val | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/test.cpp index 7660f564d6d5..dc1ad9a03760 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/test.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/test.cpp @@ -27,7 +27,7 @@ void test4(bool b) { if (b) { foo = 1; } - use(foo); // BAD + use(foo); // BAD [NOT DETECTED] } void test5() { @@ -43,7 +43,7 @@ void test5(int count) { for (int i = 0; i < count; i++) { foo = i; } - use(foo); // BAD + use(foo); // BAD [NOT DETECTED] } void test6(bool b) { @@ -52,7 +52,7 @@ void test6(bool b) { foo = 42; } if (b) { - use(foo); // GOOD (REPORTED, FP) + use(foo); // GOOD } } @@ -64,7 +64,7 @@ void test7(bool b) { set = true; } if (set) { - use(foo); // GOOD (REPORTED, FP) + use(foo); // GOOD } } @@ -89,7 +89,7 @@ void test9(int count) { if (!set) { foo = 42; } - use(foo); // GOOD (REPORTED, FP) + use(foo); // GOOD } void test10() { @@ -129,7 +129,7 @@ int absWrong(int i) { } else if (i < 0) { j = -i; } - return j; // wrong: j may not be initialized before use + return j; // wrong: j may not be initialized before use [NOT DETECTED] } // Example from qhelp @@ -326,7 +326,7 @@ int test28() { a = false; c = false; } - return val; // GOOD [FALSE POSITIVE] + return val; // GOOD } int test29() { @@ -472,4 +472,64 @@ void test44() { int y = 1; void(x + y); // BAD +} + +enum class State { StateA, StateB, StateC }; + +int exhaustive_switch(State s) { + int y; + switch(s) { + case State::StateA: + y = 1; + break; + case State::StateB: + y = 2; + break; + case State::StateC: + y = 3; + break; + } + return y; // GOOD (y is always initialized) +} + +int exhaustive_switch_2(State s) { + int y; + switch(s) { + case State::StateA: + y = 1; + break; + default: + y = 2; + break; + } + return y; // GOOD (y is always initialized) +} + +int non_exhaustive_switch(State s) { + int y; + switch(s) { + case State::StateA: + y = 1; + break; + case State::StateB: + y = 2; + break; + } + return y; // BAD [NOT DETECTED] (y is not initialized when s = StateC) +} + +int non_exhaustive_switch_2(State s) { + int y; + switch(s) { + case State::StateA: + y = 1; + break; + case State::StateB: + y = 2; + break; + } + if(s != State::StateC) { + return y; // GOOD (y is not initialized when s = StateC, but if s = StateC we won't reach this point) + } + return 0; } \ No newline at end of file