Merge pull request #326 from rdmarsh2/rdmarsh/cpp/dead-code-goto

C++: new query for dead code after goto or break

Author: semmledocs-ac

change-notes/1.19/analysis-cpp.md

@@ -6,9 +6,10 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
-| Cast between HRESULT and a Boolean type (`cpp/hresult-boolean-conversion`) | external/cwe/cwe-253 | Finds logic errors caused by mistakenly treating the Windows `HRESULT` type as a Boolean instead of testing it with the appropriate macros. Enabled by default. |
+| Cast between `HRESULT` and a Boolean type (`cpp/hresult-boolean-conversion`) | external/cwe/cwe-253 | Finds logic errors caused by mistakenly treating the Windows `HRESULT` type as a Boolean instead of testing it with the appropriate macros. Enabled by default. |
 | Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | external/cwe/cwe-732 | This query finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Enabled by default. |
-| Cast from char* to wchar_t* | security, external/cwe/cwe-704 | Detects potentially dangerous casts from char* to wchar_t*.  Enabled by default on LGTM. |
+| Cast from `char*` to `wchar_t*` | security, external/cwe/cwe-704 | Detects potentially dangerous casts from `char*` to `wchar_t*`.  Enabled by default on LGTM. |
+| Dead code due to `goto` or `break` statement (`cpp/dead-code-goto`) | maintainability, external/cwe/cwe-561 | Detects dead code following a goto or break statement. Enabled by default on LGTM. |
 
 ## Changes to existing queries
 

cpp/ql/src/Critical/DeadCodeGoto.cpp

@@ -0,0 +1,7 @@
+goto err1;
+free(pointer); // BAD: this line is unreachable
+err1: return -1;
+
+free(pointer); // GOOD: this line is reachable
+goto err2;
+err2: return -1;

cpp/ql/src/Critical/DeadCodeGoto.qhelp

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>
+Code immediately following a <code>goto</code> or <code>break</code> statement will not be executed,
+unless there is a label or switch case. When the code is necessary, this leads to logical errors or
+resource leaks. If the code is unnecessary, it may confuse readers.
+</p>
+</overview>
+<recommendation>
+<p>
+If the unreachable code is necessary, move the <code>goto</code> or <code>break</code> statement to
+after the code. Otherwise, delete the unreachable code.
+</p>
+
+</recommendation>
+<example><sample src="DeadCodeGoto.cpp" />
+</example>
+<references>
+<li>
+  The CERT C Secure Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/MSC12-C.+Detect+and+remove+code+that+has+no+effect+or+is+never+executed">MSC12-C. Detect and remove code that has no effect or is never executed</a>.
+</li>
+</references>
+</qhelp>

cpp/ql/src/Critical/DeadCodeGoto.ql

@@ -0,0 +1,33 @@
+/**
+ * @name Dead code due to goto or break statement
+ * @description A goto or break statement is followed by unreachable code. 
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id cpp/dead-code-goto
+ * @tags maintainability
+ *       external/cwe/cwe-561
+ */
+
+import cpp
+
+Stmt getNextRealStmt(Block b, int i) {
+  result = b.getStmt(i + 1) and
+  not result instanceof EmptyStmt
+  or
+  b.getStmt(i + 1) instanceof EmptyStmt and
+  result = getNextRealStmt(b, i + 1)
+}
+
+from JumpStmt js, Block b, int i, Stmt s
+where b.getStmt(i) = js
+  and s = getNextRealStmt(b, i)
+  // the next statement isn't jumped to
+  and not s instanceof LabelStmt
+  and not s instanceof SwitchCase
+  // the next statement isn't breaking out of a switch
+  and not s.(BreakStmt).getBreakable() instanceof SwitchStmt
+  // 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)
+select js, "This statement makes $@ unreachable.", s, s.toString()

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

@@ -0,0 +1,3 @@
+| test.cpp:2:2:2:12 | goto ... | This statement makes $@ unreachable. | test.cpp:3:2:3:5 | ExprStmt | ExprStmt |
+| test.cpp:9:3:9:8 | break; | This statement makes $@ unreachable. | test.cpp:10:3:10:6 | ExprStmt | ExprStmt |
+| test.cpp:37:3:37:8 | break; | This statement makes $@ unreachable. | test.cpp:38:3:38:11 | return ... | return ... |

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

@@ -0,0 +1 @@
+Critical/DeadCodeGoto.ql

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

@@ -0,0 +1,83 @@
+int test1(int x) {
+	goto label; // BAD
+	x++;
+	label: return x;
+}
+
+int test2(int x) {
+	do {
+		break; // BAD
+		x++;
+	} while(false);
+	return x;
+}
+
+int test3(int x) {
+	goto label; // GOOD
+	label: x++;
+	return x;
+}
+
+int test4(int x) {
+	goto label; // GOOD
+	do {
+		label: x++;
+	} while(false);
+	return x;
+}
+
+int test5(int x, int y) {
+	switch(y) {
+	case 0:
+		break; // GOOD
+	case 1:
+		goto label; // GOOD
+		break;
+	case 2:
+		break; // BAD
+		return x;
+	case 3:
+		return x;
+		break; // GOOD
+	case 4:
+		goto label; // GOOD
+	case 5:
+		goto label;; // GOOD
+	default:
+		x++;
+	}
+	label:
+	return x;
+}
+
+void test6(int x, int cond) {
+	if (cond) {
+		x++;
+	} else goto end; // GOOD
+	x++;
+	end:
+}
+
+void test7(int x, int cond) {
+	if (cond)
+	{
+		goto target;
+	}
+	goto somewhere_else; // GOOD
+	while (x < 10) // not dead code
+	{
+	target:
+		x++;
+	}
+	somewhere_else:
+	switch (1)
+	{
+	  goto end;
+	  while (x < 10) // not dead code
+	  {
+	case 1:
+	    x++;
+	  } break;
+	}
+	end:
+}