Merge pull request #719 from github/michaelrfairhurst/implement-funct…

…ion-types-package

Implement function types package

c/misra/src/rules/RULE-17-12/FunctionAddressesShouldAddressOperator.ql

@@ -0,0 +1,33 @@
+/**
+ * @id c/misra/function-addresses-should-address-operator
+ * @name RULE-17-12: A function identifier should only be called with a parenthesized parameter list or used with a &
+ * @description A function identifier should only be called with a parenthesized parameter list or
+ *              used with a & (address-of).
+ * @kind problem
+ * @precision very-high
+ * @problem.severity error
+ * @tags external/misra/id/rule-17-12
+ *       readability
+ *       external/misra/c/2012/amendment3
+ *       external/misra/obligation/advisory
+ */
+
+import cpp
+import codingstandards.c.misra
+
+predicate isImplicitlyAddressed(FunctionAccess access) {
+  not access.getParent() instanceof AddressOfExpr and
+  // Note: the following *seems* to only exist in c++ codebases, for instance,
+  // when calling a member. In c, this syntax should always extract as a
+  // [FunctionCall] rather than a [ExprCall] of a [FunctionAccess]. Still, this
+  // is a good pattern to be defensive against.
+  not exists(ExprCall call | call.getExpr() = access)
+}
+
+from FunctionAccess funcAccess
+where
+  not isExcluded(funcAccess, FunctionTypesPackage::functionAddressesShouldAddressOperatorQuery()) and
+  isImplicitlyAddressed(funcAccess)
+select funcAccess,
+  "The address of function " + funcAccess.getTarget().getName() +
+    " is taken without the & operator."

c/misra/test/rules/RULE-17-12/FunctionAddressesShouldAddressOperator.expected

@@ -0,0 +1,13 @@
+| test.c:14:25:14:29 | func2 | The address of function func2 is taken without the & operator. |
+| test.c:15:25:15:29 | func3 | The address of function func3 is taken without the & operator. |
+| test.c:21:12:21:16 | func1 | The address of function func1 is taken without the & operator. |
+| test.c:38:3:38:7 | func1 | The address of function func1 is taken without the & operator. |
+| test.c:39:3:39:7 | func2 | The address of function func2 is taken without the & operator. |
+| test.c:57:13:57:17 | func1 | The address of function func1 is taken without the & operator. |
+| test.c:58:21:58:25 | func2 | The address of function func2 is taken without the & operator. |
+| test.c:59:13:59:17 | func1 | The address of function func1 is taken without the & operator. |
+| test.c:59:20:59:24 | func2 | The address of function func2 is taken without the & operator. |
+| test.c:67:11:67:15 | func1 | The address of function func1 is taken without the & operator. |
+| test.c:68:12:68:16 | func1 | The address of function func1 is taken without the & operator. |
+| test.c:69:12:69:16 | func1 | The address of function func1 is taken without the & operator. |
+| test.c:71:18:71:22 | func1 | The address of function func1 is taken without the & operator. |

c/misra/test/rules/RULE-17-12/FunctionAddressesShouldAddressOperator.qlref

@@ -0,0 +1 @@
+rules/RULE-17-12/FunctionAddressesShouldAddressOperator.ql

c/misra/test/rules/RULE-17-12/test.c

@@ -0,0 +1,107 @@
+void func1() {}
+void func2(int x, char *y) {}
+
+typedef struct {
+} s;
+
+int func3() { return 0; }
+
+typedef void (*func_ptr_t1)();
+typedef void (*func_ptr_t2)(int x, char *y);
+typedef s (*func_ptr_t3)();
+
+func_ptr_t1 func_ptr1 = &func1;    // COMPLIANT
+func_ptr_t2 func_ptr2 = func2;     // NON-COMPLIANT
+func_ptr_t3 func_ptr3 = func3 + 0; // NON-COMPLIANT
+
+void take_func(func_ptr_t1 f1, func_ptr_t2 f2);
+
+func_ptr_t1 returns_func(int x) {
+  if (x == 0) {
+    return func1; // NON-COMPLIANT
+  } else if (x == 1) {
+    return &func1; // COMPLIANT
+  }
+
+  return returns_func(0); // COMPLIANT
+}
+
+#define MACRO_IDENTITY(f) (f)
+#define MACRO_INVOKE_RISKY(f) (f())
+#define MACRO_INVOKE_IMPROVED(f) ((f)())
+#define MACRO_INVOKE_AND_USE_AS_TOKEN(f) f(0, #f)
+
+void test() {
+  func1();           // COMPLIANT
+  func2(1, "hello"); // COMPLIANT
+
+  func1; // NON-COMPLIANT
+  func2; // NON-COMPLIANT
+
+  &func1; // COMPLIANT
+  &func2; // COMPLIANT
+
+  (func1)();           // COMPLIANT
+  (func2)(1, "hello"); // COMPLIANT
+
+  &(func1); // COMPLIANT
+  &(func2); // COMPLIANT
+
+  (&func1)();           // COMPLIANT
+  (&func2)(1, "hello"); // COMPLIANT
+
+  (func1());           // COMPLIANT
+  (func2(1, "hello")); // COMPLIANT
+
+  take_func(&func1, &func2); // COMPLIANT
+  take_func(func1, &func2);  // NON-COMPLIANT
+  take_func(&func1, func2);  // NON-COMPLIANT
+  take_func(func1, func2);   // NON-COMPLIANT
+
+  returns_func(0);     // COMPLIANT
+  returns_func(0)();   // COMPLIANT
+  (returns_func(0))(); // COMPLIANT
+
+  (void *)&func1;   // COMPLIANT
+  (void *)(&func1); // COMPLIANT
+  (void *)func1;    // NON-COMPLIANT
+  (void *)(func1);  // NON-COMPLIANT
+  ((void *)func1);  // NON-COMPLIANT
+
+  MACRO_IDENTITY(func1);     // NON-COMPLIANT
+  MACRO_IDENTITY(func1)();   // NON-COMPLIANT[FALSE NEGATIVE]
+  MACRO_IDENTITY(&func1);    // COMPLIANT
+  MACRO_IDENTITY (&func1)(); // COMPLIANT
+
+  MACRO_INVOKE_RISKY(func3);     // NON-COMPLIANT[FALSE NEGATIVE]
+  MACRO_INVOKE_IMPROVED(func3);  // NON-COMPLIANT[FALSE NEGATIVE]
+  MACRO_INVOKE_IMPROVED(&func3); // COMPLIANT
+
+  MACRO_INVOKE_AND_USE_AS_TOKEN(func1); // COMPLIANT
+
+  // Function pointers are exempt from this rule.
+  func_ptr1();                        // COMPLIANT
+  func_ptr2(1, "hello");              // COMPLIANT
+  func_ptr1;                          // COMPLIANT
+  func_ptr2;                          // COMPLIANT
+  &func_ptr1;                         // COMPLIANT
+  &func_ptr2;                         // COMPLIANT
+  (func_ptr1)();                      // COMPLIANT
+  (func_ptr2)(1, "hello");            // COMPLIANT
+  (*func_ptr1)();                     // COMPLIANT
+  (*func_ptr2)(1, "hello");           // COMPLIANT
+  take_func(func_ptr1, func_ptr2);    // COMPLIANT
+  (void *)func_ptr1;                  // COMPLIANT
+  (void *)&func_ptr1;                 // COMPLIANT
+  (void *)(&func_ptr1);               // COMPLIANT
+  (void *)func_ptr1;                  // COMPLIANT
+  (void *)(func_ptr1);                // COMPLIANT
+  ((void *)func_ptr1);                // COMPLIANT
+  MACRO_IDENTITY(func_ptr1);          // COMPLIANT
+  MACRO_IDENTITY(func_ptr1)();        // COMPLIANT
+  MACRO_IDENTITY(&func_ptr1);         // COMPLIANT
+  (*MACRO_IDENTITY(&func_ptr1))();    // COMPLIANT
+  MACRO_INVOKE_RISKY(func_ptr3);      // COMPLIANT
+  MACRO_INVOKE_IMPROVED(func_ptr3);   // COMPLIANT
+  MACRO_INVOKE_IMPROVED(*&func_ptr3); // COMPLIANT
+}

cpp/common/src/codingstandards/cpp/exclusions/c/FunctionTypes.qll

@@ -0,0 +1,26 @@
+//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY.  **/
+import cpp
+import RuleMetadata
+import codingstandards.cpp.exclusions.RuleMetadata
+
+newtype FunctionTypesQuery = TFunctionAddressesShouldAddressOperatorQuery()
+
+predicate isFunctionTypesQueryMetadata(Query query, string queryId, string ruleId, string category) {
+  query =
+    // `Query` instance for the `functionAddressesShouldAddressOperator` query
+    FunctionTypesPackage::functionAddressesShouldAddressOperatorQuery() and
+  queryId =
+    // `@id` for the `functionAddressesShouldAddressOperator` query
+    "c/misra/function-addresses-should-address-operator" and
+  ruleId = "RULE-17-12" and
+  category = "advisory"
+}
+
+module FunctionTypesPackage {
+  Query functionAddressesShouldAddressOperatorQuery() {
+    //autogenerate `Query` type
+    result =
+      // `Query` type for `functionAddressesShouldAddressOperator` query
+      TQueryC(TFunctionTypesPackageQuery(TFunctionAddressesShouldAddressOperatorQuery()))
+  }
+}

cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll

@@ -29,6 +29,7 @@ import Declarations8
 import EssentialTypes
 import Expressions
 import FloatingTypes
+import FunctionTypes
 import IO1
 import IO2
 import IO3
@@ -102,6 +103,7 @@ newtype TCQuery =
   TEssentialTypesPackageQuery(EssentialTypesQuery q) or
   TExpressionsPackageQuery(ExpressionsQuery q) or
   TFloatingTypesPackageQuery(FloatingTypesQuery q) or
+  TFunctionTypesPackageQuery(FunctionTypesQuery q) or
   TIO1PackageQuery(IO1Query q) or
   TIO2PackageQuery(IO2Query q) or
   TIO3PackageQuery(IO3Query q) or
@@ -175,6 +177,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat
   isEssentialTypesQueryMetadata(query, queryId, ruleId, category) or
   isExpressionsQueryMetadata(query, queryId, ruleId, category) or
   isFloatingTypesQueryMetadata(query, queryId, ruleId, category) or
+  isFunctionTypesQueryMetadata(query, queryId, ruleId, category) or
   isIO1QueryMetadata(query, queryId, ruleId, category) or
   isIO2QueryMetadata(query, queryId, ruleId, category) or
   isIO3QueryMetadata(query, queryId, ruleId, category) or

docs/user_manual.md

@@ -78,7 +78,7 @@ The datasheet _"CodeQL Coding Standards: supported rules"_, provided with each r
 
 [^1]: AUTOSAR C++ versions R22-11, R21-11, R20-11, R19-11 and R19-03 are all identical as indicated in the document change history.
 [^2]: The unimplemented supportable AUTOSAR rules are `A7-1-8` and `A8-2-1`. These rules require additional support in the CodeQL CLI to ensure the required information is available in the CodeQL database to identify violations of these rules.
-[^3]: The unimplemented supportable MISRA C 2012 rules are `Rule 9.5` and `Dir 4.14`. `Rule 9.5` requires additional support in the CodeQL CLI to ensure the required information is available in the CodeQL database to identify violations of these rules. `Dir 4.14` is covered by the default CodeQL queries, which identify potential security vulnerabilities caused by not validating external input.
+[^3]: The unimplemented supportable MISRA C 2012 rules are `Rule 9.5`, `Rule 17.13`, and `Dir 4.14`. `Rule 9.5` and `Rule 17.13` require additional support in the CodeQL CLI to ensure the required information is available in the CodeQL database to identify violations of these rules. `Dir 4.14` is covered by the default CodeQL queries, which identify potential security vulnerabilities caused by not validating external input.
 [^4]: The rules 5.13.7, 19.0.1 and 19.1.2 are not planned to be implemented by CodeQL as they are compiler checked in all supported compilers.
 
 ## Supported environment

rule_packages/c/FunctionTypes.json

@@ -0,0 +1,24 @@
+{
+  "MISRA-C-2012": {
+    "RULE-17-12": {
+      "properties": {
+        "obligation": "advisory"
+      },
+      "queries": [
+        {
+          "description": "A function identifier should only be called with a parenthesized parameter list or used with a & (address-of).",
+          "kind": "problem",
+          "name": "A function identifier should only be called with a parenthesized parameter list or used with a &",
+          "precision": "very-high",
+          "severity": "error",
+          "short_name": "FunctionAddressesShouldAddressOperator",
+          "tags": [
+            "readability",
+            "external/misra/c/2012/amendment3"
+          ]
+        }
+      ],
+      "title": "A function identifier should only be called with a parenthesized parameter list or used with a & (address-of)"
+    }
+  }
+}

rules.csv

@@ -739,7 +739,7 @@ c,MISRA-C-2012,RULE-17-9,Yes,Mandatory,,,Verify that a function declared with _N
 c,MISRA-C-2012,RULE-17-10,Yes,Required,,,A function declared with _noreturn shall have a return type of void,,NoReturn,Easy,
 c,MISRA-C-2012,RULE-17-11,Yes,Advisory,,,A function without a branch that returns shall be declared with _Noreturn,,NoReturn,Easy,
 c,MISRA-C-2012,RULE-17-12,Yes,Advisory,,,A function identifier should only be called with a parenthesized parameter list or used with a & (address-of),,FunctionTypes,Easy,
-c,MISRA-C-2012,RULE-17-13,Yes,Required,,,"A function type shall not include any type qualifiers (const, volatile, restrict, or _Atomic)",,FunctionTypes,Easy,
+c,MISRA-C-2012,RULE-17-13,No,Required,,,"A function type shall not include any type qualifiers (const, volatile, restrict, or _Atomic)",,,Easy,
 c,MISRA-C-2012,RULE-18-1,Yes,Required,,,A pointer resulting from arithmetic on a pointer operand shall address an element of the same array as that pointer operand,M5-0-16,Pointers1,Import,
 c,MISRA-C-2012,RULE-18-2,Yes,Required,,,Subtraction between pointers shall only be applied to pointers that address elements of the same array,M5-0-17,Pointers1,Import,
 c,MISRA-C-2012,RULE-18-3,Yes,Required,,,"The relational operators >, >=, < and <= shall not be applied to objects of pointer type except where they point into the same object",M5-0-18,Pointers1,Import,