Rule SIG31-C

Author: mbaluda

c/cert/src/rules/SIG30-C/CallOnlyAsyncSafeFunctionsWithinSignalHandlers.ql

@@ -6,6 +6,8 @@
  * @precision very-high
  * @problem.severity error
  * @tags external/cert/id/sig30-c
+ *       correctness
+ *       security
  *       external/cert/obligation/rule
  */
 
@@ -22,7 +24,7 @@ class AsyncSafeVariableAccess extends VariableAccess {
   AsyncSafeVariableAccess() {
     this.getTarget() instanceof StackVariable
     or
-    this.getTarget().getType().hasName("volatile sig_atomic_t") and // TODO search without "volatile"
+    this.getType().hasName("volatile sig_atomic_t") and // TODO search without "volatile"
     this.isModified() and
     this.getTarget().isVolatile()
   }

c/cert/src/rules/SIG31-C/DoNotAccessSharedObjectsInSignalHandlers.md

@@ -176,7 +176,7 @@ CWE-828 = SIG31-C + non-async-safe things besides shared objects.
 
 ## Implementation notes
 
-None
+The implementation does not verify the correct usage of `atomic_is_lock_free`.
 
 ## References
 

c/cert/src/rules/SIG31-C/DoNotAccessSharedObjectsInSignalHandlers.ql

@@ -6,13 +6,42 @@
  * @precision very-high
  * @problem.severity error
  * @tags external/cert/id/sig31-c
+ *       correctness
+ *       security
  *       external/cert/obligation/rule
  */
 
 import cpp
 import codingstandards.c.cert
+import codingstandards.c.Signal
 
-from
+/**
+ * Does not an access an external variable except
+ * to assign a value to a volatile static variable of sig_atomic_t type
+ */
+class UnsafeSharedVariableAccess extends VariableAccess {
+  UnsafeSharedVariableAccess() {
+    // static or thread local storage duration
+    (
+      this.getTarget() instanceof StaticStorageDurationVariable or
+      this.getTarget().isThreadLocal()
+    ) and
+    // excluding `volatile sig_atomic_t` type
+    not (
+      this.getType().hasName("volatile sig_atomic_t") and // TODO search without "volatile"
+      this.getTarget().isVolatile()
+    ) and //excluding lock-free atomic objects
+    not exists(MacroInvocation mi, VariableAccess va |
+      mi.getMacroName() = "atomic_is_lock_free" and
+      mi.getExpr().getChild(0) = va.getEnclosingElement*() and
+      va.getTarget() = this.getTarget()
+    )
+  }
+}
+
+from UnsafeSharedVariableAccess va, SignalHandler handler
 where
-  not isExcluded(x, SignalHandlersPackage::doNotAccessSharedObjectsInSignalHandlersQuery()) and
-select
+  not isExcluded(va, SignalHandlersPackage::doNotAccessSharedObjectsInSignalHandlersQuery()) and
+  handler = va.getEnclosingFunction()
+select va, "Shared object access within a $@ can lead to undefined behavior.",
+  handler.getRegistration(), "signal handler"

c/cert/test/rules/SIG30-C/CallOnlyAsyncSafeFunctionsWithinSignalHandlers.expected.qcc

@@ -0,0 +1,4 @@
+| test.c:11:3:11:18 | call to log_local_unsafe | Asyncronous-unsafe function calls within a $@ can lead to undefined behavior. | test.c:17:7:17:12 | call to signal | signal handler |
+| test.c:12:3:12:6 | call to free | Asyncronous-unsafe function calls within a $@ can lead to undefined behavior. | test.c:17:7:17:12 | call to signal | signal handler |
+| test.c:48:3:48:17 | call to siglongjmp | Asyncronous-unsafe function calls within a $@ can lead to undefined behavior. | test.c:52:7:52:12 | call to signal | signal handler |
+| test.c:78:7:78:11 | call to raise | Asyncronous-unsafe function calls within a $@ can lead to undefined behavior. | test.c:93:7:93:12 | call to signal | signal handler |

c/cert/test/rules/SIG31-C/DoNotAccessSharedObjectsInSignalHandlers.expected

@@ -1 +1 @@
-No expected results have yet been specified
+| test.c:7:10:7:16 | err_msg | Shared object access within a $@ can lead to undefined behavior. | test.c:11:3:11:8 | call to signal | signal handler |

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

@@ -0,0 +1,40 @@
+#include <signal.h>
+#include <stdlib.h>
+#include <string.h>
+
+char *err_msg;
+void handler(int signum) {
+  strcpy(err_msg, "SIGINT encountered."); // NON_COMPLIANT
+}
+
+void f1(void) {
+  signal(SIGINT, handler);
+  // ...
+}
+
+volatile sig_atomic_t e_flag1 = 0;
+void handler2(int signum) {
+  e_flag1 = 1; // COMPLIANT
+}
+
+void f2(void) {
+  signal(SIGINT, handler2);
+  // ...
+}
+
+#include <stdatomic.h>
+
+atomic_int e_flag3 = ATOMIC_VAR_INIT(0);
+void handler3(int signum) {
+  e_flag3 = 1; // COMPLIANT
+}
+
+void f3(void) {
+  if (!atomic_is_lock_free(&e_flag3)) {
+    // ...
+  }
+
+  if (signal(SIGINT, handler3) == SIG_ERR) {
+    // ...
+  }
+}

c/common/test/includes/standard-library/stdatomic.h

@@ -3,5 +3,7 @@
 #define atomic_load(a) 0
 #define atomic_load_explicit(a, b)
 #define atomic_store(a, b) 0
-#define atomic_store_explicit(a,b,c) 0
-#define ATOMIC_VAR_INIT(value) (value)
+#define atomic_store_explicit(a, b, c) 0
+#define ATOMIC_VAR_INIT(value) (value)
+#define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj)))
+typedef _Atomic(int) atomic_int;

rule_packages/c/SignalHandlers.json

@@ -12,7 +12,10 @@
           "precision": "very-high",
           "severity": "error",
           "short_name": "CallOnlyAsyncSafeFunctionsWithinSignalHandlers",
-          "tags": []
+          "tags": [
+            "correctness",
+            "security"
+          ]
         }
       ],
       "title": "Call only asynchronous-safe functions within signal handlers"
@@ -29,7 +32,13 @@
           "precision": "very-high",
           "severity": "error",
           "short_name": "DoNotAccessSharedObjectsInSignalHandlers",
-          "tags": []
+          "tags": [
+            "correctness",
+            "security"
+          ],
+          "implementation_scope": {
+            "description": "The implementation does not verify the correct usage of `atomic_is_lock_free`."
+          }
         }
       ],
       "title": "Do not access shared objects in signal handlers"