Exclude handlers that terminate the execution on SIG_ERR

Author: mbaluda

c/cert/src/rules/ERR32-C/DoNotRelyOnIndeterminateValuesOfErrno.md

@@ -200,7 +200,7 @@ Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+D
 
 ## Implementation notes
 
-None
+The rule is enforced in the context of a single function.
 
 ## References
 

c/cert/src/rules/ERR32-C/DoNotRelyOnIndeterminateValuesOfErrno.ql

@@ -13,22 +13,54 @@
 import cpp
 import codingstandards.c.cert
 import codingstandards.c.Errno
+import semmle.code.cpp.controlflow.Guards
 
 class SignalCall extends FunctionCall {
   SignalCall() { this.getTarget().hasGlobalName("signal") }
 }
 
 /**
- * Models signal handlers that call signal()
+ * A check on `signal` call return value
+ * `if (signal(SIGINT, handler) == SIG_ERR)`
+ */
+class SignalCheckOperation extends EqualityOperation, GuardCondition {
+  ControlFlowNode errorSuccessor;
+
+  SignalCheckOperation() {
+    this.getAnOperand() = any(MacroInvocation m | m.getMacroName() = "SIG_ERR").getExpr() and
+    (
+      this.getOperator() = "==" and
+      this.controls(errorSuccessor, true)
+      or
+      this.getOperator() = "!=" and
+      this.controls(errorSuccessor, false)
+    )
+  }
+
+  ControlFlowNode getErrorSuccessor() { result = errorSuccessor }
+}
+
+/**
+ * Models signal handlers that call signal() and return
  */
 class SignalCallingHandler extends Function {
   SignalCall sh;
 
   SignalCallingHandler() {
     // is a signal handler
     this = sh.getArgument(1).(FunctionAccess).getTarget() and
-    // calls signal()
-    this.calls*(any(SignalCall c).getTarget())
+    // calls signal() on the handled signal
+    exists(SignalCall sCall |
+      sCall.getEnclosingFunction() = this and
+      DataFlow::localFlow(DataFlow::parameterNode(this.getParameter(0)),
+        DataFlow::exprNode(sCall.getArgument(0))) and
+      // does not abort on error
+      not exists(SignalCheckOperation sCheck, FunctionCall abort |
+        DataFlow::localExprFlow(sCall, sCheck.getAnOperand()) and
+        abort.getTarget().hasGlobalName(["abort", "_Exit"]) and
+        abort.getEnclosingElement*() = sCheck.getErrorSuccessor()
+      )
+    )
   }
 
   SignalCall getHandler() { result = sh }
@@ -37,14 +69,12 @@ class SignalCallingHandler extends Function {
 from ErrnoRead errno, SignalCall h
 where
   not isExcluded(errno, Contracts5Package::doNotRelyOnIndeterminateValuesOfErrnoQuery()) and
-  // errno read in the handler
-  exists(SignalCallingHandler sc |
+  exists(SignalCallingHandler sc | sc.getHandler() = h |
+    // errno read in the handler
+    sc.calls*(errno.getEnclosingFunction())
+    or
+    // errno is read after the handle returns
     sc.getHandler() = h and
-    (
-      sc.calls*(errno.getEnclosingFunction())
-      or
-      // errno is read after the handle
-      errno.(ControlFlowNode).getAPredecessor+() = sc.getHandler()
-    )
+    errno.getAPredecessor+() = h
   )
 select errno, "`errno` has indeterminate value after this $@.", h, h.toString()

c/cert/test/rules/ERR32-C/DoNotRelyOnIndeterminateValuesOfErrno.expected

@@ -1,3 +1,5 @@
-| test.c:12:5:12:10 | call to perror | `errno` has indeterminate value after this $@. | test.c:32:21:32:26 | call to signal | call to signal |
-| test.c:26:3:26:8 | call to perror | `errno` has indeterminate value after this $@. | test.c:41:17:41:22 | call to signal | call to signal |
-| test.c:34:5:34:10 | call to perror | `errno` has indeterminate value after this $@. | test.c:32:21:32:26 | call to signal | call to signal |
+| test.c:12:5:12:10 | call to perror | `errno` has indeterminate value after this $@. | test.c:35:21:35:26 | call to signal | call to signal |
+| test.c:37:5:37:10 | call to perror | `errno` has indeterminate value after this $@. | test.c:35:21:35:26 | call to signal | call to signal |
+| test.c:42:5:42:10 | call to perror | `errno` has indeterminate value after this $@. | test.c:35:21:35:26 | call to signal | call to signal |
+| test.c:51:5:51:10 | call to perror | `errno` has indeterminate value after this $@. | test.c:35:21:35:26 | call to signal | call to signal |
+| test.c:51:5:51:10 | call to perror | `errno` has indeterminate value after this $@. | test.c:45:17:45:22 | call to signal | call to signal |

c/cert/test/rules/ERR32-C/test.c

@@ -15,18 +15,21 @@ void handler1(int signum) {
 
 void handler2(int signum) {
   pfv old_handler = signal(signum, SIG_DFL);
-  if (old_handler == SIG_ERR) {
-    abort(); // COMPLIANT
+  if (old_handler != SIG_ERR) {
+    perror(""); // COMPLIANT
+  } else {
+    abort();
   }
 }
 
 void handler3(int signum) { pfv old_handler = signal(signum, SIG_DFL); }
 
-pfv helper4(int signum) {
-  perror(""); // NON_COMPLIANT
-  return signal(signum, SIG_DFL);
+void handler4(int signum) {
+  pfv old_handler = signal(signum, SIG_DFL);
+  if (old_handler == SIG_ERR) {
+    _Exit(0);
+  }
 }
-void handler4(int signum) { pfv old_handler = helper4(signum); }
 
 int main(void) {
   pfv old_handler = signal(SIGINT, handler1);
@@ -35,8 +38,16 @@ int main(void) {
   }
 
   old_handler = signal(SIGINT, handler2);
+  if (old_handler == SIG_ERR) {
+    perror(""); // NON_COMPLIANT
+  }
 
   old_handler = signal(SIGINT, handler3);
 
   old_handler = signal(SIGINT, handler4);
+
+  FILE *fp = fopen("something", "r");
+  if (fp == NULL) {
+    perror("Error: "); // NON_COMPLIANT
+  }
 }