Address feedback

Author: MichaelRFairhurst

Diff for: c/common/src/codingstandards/c/SubObjects.qll

@@ -1,3 +1,31 @@
+/**
+ * A library that expands upon the `Objects.qll` library, to support nested "Objects" such as
+ * `x.y.z` or `x[i][j]` within an object `x`.
+ *
+ * Objects in C are values in memory, that have a type and a storage duration. In the case of
+ * array objects and struct objects, the object will contain other objects. The these subobjects
+ * will share properties of the root object such as storage duration. This library can be used to,
+ * for instance, find all usages of a struct member to ensure that member is initialized before it
+ * is used.
+ *
+ * To use this library, select `SubObject` and find its usages in the AST via `getAnAccess()` (to
+ * find usages of the subobject by value) or `getAnAddressOfExpr()` (to find usages of the object
+ * by address).
+ *
+ * Note that a struct or array object may contain a pointer. In this case, the pointer itself is
+ * a subobject of the struct or array object, but the object that the pointer points to is not.
+ * This is because the pointed-to object does not necessarily have the same storage duration,
+ * lifetime, or linkage as the pointer and the object containing the pointer.
+ *
+ * Note as well that `getAnAccess()` on an array subobject will return all accesses to the array,
+ * not just accesses to a particular index. For this reason, `SubObject` exposes the predicate
+ * `isPrecise()`. If a subobject is precise, that means all results of `getAnAccess()` will
+ * definitely refer to the same object in memory. If it is not precise, the different accesses
+ * may refer to the same or different objects in memory. For instance, `x[i].y` and `x[j].y` are
+ * the same object if `i` and `j` are the same, but they are different objects if `i` and `j` are
+ * different.
+ */
+
 import codingstandards.c.Objects
 
 newtype TSubObject =
@@ -70,6 +98,8 @@ class SubObject extends TSubObject {
     exists(MemberVariable m |
       this = TObjectMember(_, m) and
       result = m.getAnAccess() and
+      // Only consider `DotFieldAccess`es, not `PointerFieldAccess`es, as the latter
+      // are not subobjects of the root object:
       result.(DotFieldAccess).getQualifier() = getParent().getAnAccess()
     )
     or
@@ -79,6 +109,11 @@ class SubObject extends TSubObject {
 
   AddressOfExpr getAnAddressOfExpr() { result.getOperand() = this.getAnAccess() }
 
+  /**
+   * Get the "root" object identity to which this subobject belongs. For instance, in the
+   * expression `x.y.z`, the root object is `x`. This subobject will share properties with the root
+   * object such as storage duration, lifetime, and linkage.
+   */
   ObjectIdentity getRootIdentity() {
     exists(ObjectIdentity i |
       this = TObjectRoot(i) and

Diff for: c/common/src/codingstandards/c/initialization/GlobalInitializationAnalysis.qll

@@ -7,7 +7,9 @@ signature module GlobalInitializationAnalysisConfigSig {
   /** A function which is not called or started as a thread */
   default predicate isRootFunction(Function f) {
     not exists(Function f2 | f2.calls(f)) and
-    not f instanceof ThreadedFunction
+    not f instanceof ThreadedFunction and
+    // Exclude functions which are used as function pointers.
+    not exists(FunctionAccess access | f = access.getTarget())
   }
 
   ObjectIdentity getAnInitializedObject(Expr e);

Diff for: c/misra/src/rules/DIR-5-1/PossibleDataRaceBetweenThreads.ql

@@ -47,7 +47,7 @@ class NonReentrantOperation extends TNonReentrantOperation {
     )
   }
 
-  Expr getARead() {
+  Expr getAReadExpr() {
     exists(SubObject object |
       this = TReadWrite(object) and
       result = object.getAnAccess()
@@ -56,7 +56,7 @@ class NonReentrantOperation extends TNonReentrantOperation {
     this = TStdFunctionCall(result)
   }
 
-  Expr getAWrite() {
+  Expr getAWriteExpr() {
     exists(SubObject object, Assignment assignment |
       this = TReadWrite(object) and
       result = assignment and
@@ -94,39 +94,43 @@ class NonReentrantOperation extends TNonReentrantOperation {
 
 class WritingThread extends ThreadedFunction {
   NonReentrantOperation aWriteObject;
-  Expr aWrite;
+  Expr aWriteExpr;
 
   WritingThread() {
-    aWrite = aWriteObject.getAWrite() and
-    this.calls*(aWrite.getEnclosingFunction()) and
-    not aWrite instanceof LockProtectedControlFlowNode and
-    not aWrite.getEnclosingFunction().getName().matches(["%init%", "%boot%", "%start%"])
+    aWriteExpr = aWriteObject.getAWriteExpr() and
+    // This function directly contains the write expression, or transitively calls the function
+    // that contains the write expression.
+    this.calls*(aWriteExpr.getEnclosingFunction()) and
+    // The write isn't synchronized with a mutex or condition object.
+    not aWriteExpr instanceof LockProtectedControlFlowNode and
+    // The write doesn't seem to be during a special initialization phase of the program.
+    not aWriteExpr.getEnclosingFunction().getName().matches(["%init%", "%boot%", "%start%"])
   }
 
-  Expr getAWrite() { result = aWrite }
+  Expr getAWriteExpr() { result = aWriteExpr }
 }
 
 class ReadingThread extends ThreadedFunction {
   Expr aReadExpr;
 
   ReadingThread() {
     exists(NonReentrantOperation op |
-      aReadExpr = op.getARead() and
+      aReadExpr = op.getAReadExpr() and
       this.calls*(aReadExpr.getEnclosingFunction()) and
       not aReadExpr instanceof LockProtectedControlFlowNode
     )
   }
 
-  Expr getARead() { result = aReadExpr }
+  Expr getAReadExpr() { result = aReadExpr }
 }
 
 predicate mayBeDataRace(Expr write, Expr read, NonReentrantOperation operation) {
   exists(WritingThread wt |
-    wt.getAWrite() = write and
-    write = operation.getAWrite() and
+    wt.getAWriteExpr() = write and
+    write = operation.getAWriteExpr() and
     exists(ReadingThread rt |
-      read = rt.getARead() and
-      read = operation.getARead() and
+      read = rt.getAReadExpr() and
+      read = operation.getAReadExpr() and
       (
         wt.isMultiplySpawned() or
         not wt = rt
@@ -141,18 +145,18 @@ from
 where
   not isExcluded(write, Concurrency9Package::possibleDataRaceBetweenThreadsQuery()) and
   mayBeDataRace(write, read, operation) and
-  wt = min(WritingThread f | f.getAWrite() = write | f order by f.getName()) and
-  rt = min(ReadingThread f | f.getARead() = read | f order by f.getName()) and
+  wt = min(WritingThread f | f.getAWriteExpr() = write | f order by f.getName()) and
+  rt = min(ReadingThread f | f.getAReadExpr() = read | f order by f.getName()) and
   writeString = operation.getWriteString() and
   readString = operation.getReadString() and
   if wt.isMultiplySpawned()
   then
     message =
       "Threaded " + writeString +
-        " $@ not synchronized, for example from thread function $@ spawned from a loop."
+        " $@ not synchronized from thread function $@ spawned from a loop."
   else
     message =
       "Threaded " + writeString +
-        " $@, for example from thread function $@, not synchronized with $@, for example from thread function $@."
+        " $@ from thread function $@ is not synchronized with $@ from thread function $@."
 select write, message, operation.getSourceElement(), operation.toString(), wt, wt.getName(), read,
   "concurrent " + readString, rt, rt.getName()

Diff for: c/misra/src/rules/RULE-22-17/InvalidOperationOnUnlockedMutex.ql

@@ -64,5 +64,5 @@ where
   not isExcluded(operation, Concurrency9Package::invalidOperationOnUnlockedMutexQuery()) and
   mutex = operation.getMutex() and
   not DominatingSet::isDominatedByBehavior(operation)
-select operation, "Invalid operation on mutex '$@' not locked by the current thread",
+select operation, "Invalid operation on mutex '$@' not locked by the current thread.",
   mutex.getRootIdentity(), mutex.toString()

Diff for: c/misra/src/rules/RULE-22-18/NonRecursiveMutexRecursivelyLockedAudit.ql

@@ -2,7 +2,7 @@
  * @id c/misra/non-recursive-mutex-recursively-locked-audit
  * @name RULE-22-18: (Audit) Non-recursive mutexes shall not be recursively locked
  * @description Mutex that may be initialized without mtx_recursive shall not be locked by a thread
- *              that has previous may havec locked it.
+ *              that may have previously locked it.
  * @kind problem
  * @precision high
  * @problem.severity error
@@ -57,4 +57,4 @@ where
     isTrackableMutex(lockCall, true) or
     isTrackableMutex(coveredByLock, true)
   )
-select n, "Mutex locked after previous $@.", coveredByLock, "already locked"
+select n, "Mutex locked after it was already $@.", coveredByLock, "previously locked"

Diff for: c/misra/src/rules/RULE-22-19/ConditionVariableUsedWithMultipleMutexes.ql

@@ -64,6 +64,6 @@ where
   useOne = firstCallForConditionMutex(cond, mutexOne) and
   useTwo = firstCallForConditionMutex(cond, mutexOne)
 select useOne,
-  "Condition variable $@ associated with multiple mutexes, operation uses mutex $@ while $@ uses other mutex $@",
+  "Condition variable $@ associated with multiple mutexes, operation uses mutex $@ while $@ uses other mutex $@.",
   cond.getRootIdentity(), cond.toString(), mutexOne.getRootIdentity(), mutexOne.toString(), useTwo,
   "another operation", mutexTwo.getRootIdentity(), mutexTwo.toString()

Diff for: c/misra/src/rules/RULE-22-20/ThreadStorageNotInitializedBeforeUse.ql

@@ -40,5 +40,5 @@ where
   not isExcluded(objUse, Concurrency9Package::threadStorageNotInitializedBeforeUseQuery()) and
   InitAnalysis::uninitializedFrom(objUse, obj, callRoot)
 select objUse,
-  "Thread specific storage pointer '$@' possibly used before initialization, from entry point function '$@'.",
+  "Thread specific storage pointer '$@' used before initialization from entry point function '$@'.",
   obj, obj.toString(), callRoot, callRoot.getName()