Refine detection of reserved identifiers

 * Update message
 * Require appropriate include in the non-external linkage case.
 * Add extra tests

Author: lcartey

c/common/test/rules/declaredareservedidentifier/DeclaredAReservedIdentifier.expected

@@ -2,9 +2,9 @@
 | test.c:6:1:6:28 | #define _also_reserved_MACRO | Macro '_also_reserved_MACRO' declares a name beginning with _ which is reserved in the ordinary and tag namespaces. |
 | test.c:11:8:11:9 | _s | Type '_s' declares a name beginning with _ which is reserved in the tag name space. |
 | test.c:15:6:15:7 | _f | Function '_f' declares a name beginning with _ which is reserved in the ordinary name space. |
-| test.c:19:7:19:12 | malloc | Function 'malloc' declares a reserved name from the C11 standard library header 'stdlib.h'. |
+| test.c:19:7:19:12 | malloc | Function 'malloc' declares a name which is reserved for external linkage from the C11 standard library header 'stdlib.h'. |
 | test.c:25:5:25:9 | errno | Global variable 'errno' declares a name reserved for a macro from the C11 standard library header 'errno.h'. |
-| test.c:25:5:25:9 | errno | Global variable 'errno' declares a reserved name from the C11 standard library header 'errno.h'. |
+| test.c:25:5:25:9 | errno | Global variable 'errno' declares a name which is reserved for external linkage from the C11 standard library header 'errno.h'. |
 | test.c:39:16:39:18 | log | Parameter 'log' declares a name reserved for a macro from the C11 standard library header 'tgmath.h'. |
 | test.c:43:5:43:16 | _Test_global | Global variable '_Test_global' declares a reserved name beginning _ followed by an uppercase letter. |
 | test.c:44:6:44:15 | _Test_func | Function '_Test_func' declares a reserved name beginning _ followed by an uppercase letter. |
@@ -37,3 +37,7 @@
 | test.c:105:8:105:13 | NDEBUG | Type 'NDEBUG' declares a name reserved for a macro from the C11 standard library header 'assert.h'. |
 | test.c:106:7:106:12 | NDEBUG | Member variable 'NDEBUG' declares a name reserved for a macro from the C11 standard library header 'assert.h'. |
 | test.c:108:1:109:3 | #define NDEBUG x | Macro 'NDEBUG' declares a name reserved for a macro from the C11 standard library header 'assert.h'. |
+| test.c:114:5:114:8 | exit | Global variable 'exit' declares a name which is reserved for external linkage from the C11 standard library header 'stdlib.h'. |
+| test.c:116:6:116:9 | free | Function 'free' declares a name which is reserved for external linkage from the C11 standard library header 'stdlib.h'. |
+| test.c:126:1:126:16 | #define strlen 0 | Macro 'strlen' declares a reserved name from the C11 standard library header 'string.h' which is included directly or indirectly in this translation unit. |
+| test.c:136:1:136:16 | #define tm_sec 0 | Macro 'tm_sec' declares a reserved name from the C11 standard library header 'time.h' which is included directly or indirectly in this translation unit. |

c/common/test/rules/declaredareservedidentifier/test.c

@@ -107,4 +107,65 @@ struct NDEBUG { // NON_COMPLIANT - NDEBUG is reserved as a macro name
 };
 #define NDEBUG                                                                 \
   x // NON_COMPLIANT - NDEBUG is reserved as a macro name for the standard
-    // library
+    // library
+
+/* Functions and objects with external linkage */
+
+int exit; // NON_COMPLIANT - reserved for external linkage, even though header
+          // was not included
+void free() { // NON_COMPLIANT - reserved for external linkage, even though
+              // header was not included
+}
+static int rand; // COMPLIANT - only reserved for external linkage or if random
+                 // was included
+static int srand() { // COMPLIANT - only reserved for external linkage or if
+                     // random was included
+}
+
+#include <string.h>
+#define strlen 0 // NON_COMPLIANT - reserved when string.h is included
+
+// The examples below are non compliant, because those symbols are reserved in
+// file scope when string.h is included.  However, the compiler/edg rejects the
+// declaration cases, so we cannot test them.
+
+// static int strlen;
+// static void *memcpy(void *s1, const void *s2, size_t n) {}
+
+#include <time.h>
+#define tm_sec 0 // NON_COMPLIANT - reserved when time.h is included
+
+// The examples below are non compliant, because those symbols are reserved in
+// file scope when time.h is included.  However, the compiler/edg rejects the
+// declaration cases, so we cannot test them.
+
+// struct tm {
+//   int tm_sec;
+//   int tm_min;
+//   int tm_hour;
+//   int tm_mday;
+//   int tm_mon;
+//   int tm_year;
+//   int tm_wday;
+//   int tm_yday;
+//   int tm_isdst;
+//   long __tm_gmtoff;
+//   const char *__tm_zone;
+// };
+
+// Examples below are compliant because threads.h is not included
+
+#define tss_set 0   // COMPLIANT - threads.h not included
+static int tss_get; // COMPLIANT - threads.h not included, not external linkage
+static void
+tss_delete(        // COMPLIANT - threads.h not included, not external linkage
+    int tss_create // COMPLIANT - threads.h not included
+) {
+  int thrd_detach;   // COMPLIANT - threads.h not included
+  struct thrd_join { // COMPLIANT - threads.h not included
+    int thrd_equal;  // COMPLIANT - threads.h not included
+  };
+}
+struct thrd_yield { // COMPLIANT - threads.h not included
+  int thrd_exit;    // COMPLIANT - threads.h not included
+};

cpp/common/src/codingstandards/cpp/rules/declaredareservedidentifier/DeclaredAReservedIdentifier.qll

@@ -202,14 +202,16 @@ query predicate problems(Element m, string message) {
     |
       hasExternalLinkage(m) and
       reason =
-        "declares a reserved name from the " + TargetedCLibrary::getName() +
-          " standard library header '" + header + "'"
+        "declares a name which is reserved for external linkage from the " +
+          TargetedCLibrary::getName() + " standard library header '" + header + "'"
     )
     or
     // > Each identifier with file scope listed in any of the following subclauses (including the
     // > future library directions) is reserved for use as a macro name and as an identifier with
     // > file scope in the same name space if any of its associated headers is included
-    // Note: we do not consider the requirement of including the associated header
+    //
+    // Note: these cases are typically already rejected by the compiler, which prohibits redeclaration
+    //       of existing symbols. The macro cases are expected to work, though.
     exists(string header |
       TargetedCLibrary::hasObjectName(header, _, name, _, _) and
       (cNameSpace = OrdinaryNameSpace() or cNameSpace = MacroNameSpace())
@@ -220,17 +222,28 @@ query predicate problems(Element m, string message) {
       TargetedCLibrary::hasTypeName(header, _, name) and
       (cNameSpace = TagNameSpace() or cNameSpace = MacroNameSpace())
       or
-      TargetedCLibrary::hasMemberVariableName(header, _, _, name, _) and
-      (cNameSpace = OrdinaryNameSpace() or cNameSpace = MacroNameSpace())
+      exists(string declaringType, Class c |
+        TargetedCLibrary::hasMemberVariableName(header, _, declaringType, name, _)
+      |
+        // Each declaring type has its own namespace, so check that it's declared in the same
+        cNameSpace = MemberNameSpace() and
+        c.getAMember() = m and
+        c.getSimpleName() = declaringType
+        or
+        cNameSpace = MacroNameSpace()
+      )
     |
       (
         scope = FileScope()
         or
         scope = MacroScope()
       ) and
+      // The relevant header is included directly or transitively by the file
+      m.getFile().getAnIncludedFile*().getBaseName() = header and
       reason =
         "declares a reserved name from the " + TargetedCLibrary::getName() +
-          " standard library header '" + header + "'"
+          " standard library header '" + header +
+          "' which is included directly or indirectly in this translation unit"
     )
     or
     // C11 6.4.1/2