Merge pull request #1194 from geoffw0/dead-goto

CPP: Fix false positive from DeadCodeGoto.ql

change-notes/1.21/analysis-cpp.md

@@ -11,6 +11,7 @@
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Dead code due to goto or break statement (`cpp/dead-code-goto`) | Fewer false positive results | Functions containing preprocessor logic are now excluded from this analysis. |
 | Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | Fewer false positive results | Fixed an issue where functions were being identified as allocation functions inappropriately.  Also affects `cpp/new-array-delete-mismatch` and `cpp/new-delete-array-mismatch`. |
 | Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | More correct results | This query has been reworked so that it can find a wider variety of results. |
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |

cpp/ql/src/Critical/DeadCodeGoto.ql

@@ -10,6 +10,7 @@
  */
 
 import cpp
+import semmle.code.cpp.commons.Exclusions
 
 Stmt getNextRealStmt(Block b, int i) {
   result = b.getStmt(i + 1) and
@@ -30,4 +31,6 @@ where b.getStmt(i) = js
   // the next statement isn't a loop that can be jumped into
   and not exists (LabelStmt ls | s.(Loop).getStmt().getAChild*() = ls)
   and not exists (SwitchCase sc | s.(Loop).getStmt().getAChild*() = sc)
+  // no preprocessor logic applies
+  and not functionContainsPreprocCode(js.getEnclosingFunction())
 select js, "This statement makes $@ unreachable.", s, s.toString()

cpp/ql/src/semmle/code/cpp/commons/Exclusions.qll

@@ -58,3 +58,16 @@ predicate functionContainsDisabledCode(Function f) {
     )
   )
 }
+
+/**
+ * Holds if the function `f` contains code that could be excluded by the preprocessor.
+ */
+predicate functionContainsPreprocCode(Function f) {
+  // `f` contains a preprocessor branch
+  exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int fBlockStartLine, int fBlockEndLine |
+    functionLocation(f, file, fBlockStartLine, fBlockEndLine) and
+    pbdLocation(pbd, file, pbdStartLine) and
+    pbdStartLine <= fBlockEndLine and
+    pbdStartLine >= fBlockStartLine
+  )
+}

cpp/ql/test/query-tests/Critical/DeadCodeGoto/test.cpp

@@ -81,3 +81,29 @@ void test7(int x, int cond) {
 	}
 	end:
 }
+
+#define CONFIG_DEFINE
+
+void test8() {
+	int x = 0;
+
+#ifdef CONFIG_DEFINE
+	goto skip; // GOOD (the `x++` is still reachable in some configurations)
+#endif
+	x++;
+
+skip:
+}
+
+void test9() {
+	int x = 0;
+
+#ifdef CONFIG_NOTDEFINED
+	goto mid;
+#endif
+	goto end; // GOOD (the `x++` is still reachable in some configurations)
+mid:
+	x++;
+
+end:
+}