Merge pull request #804 from github/michaelrfairhurst/implement-concurrency8-package

Implement Concurrency8 package, new Objects.qll and ResourceLeakAnalysis.qll

Author: nicolaswill

c/cert/src/rules/CON34-C/AppropriateThreadObjectStorageDurations.ql

@@ -14,29 +14,43 @@
 
 import cpp
 import codingstandards.c.cert
+import codingstandards.c.Objects
 import codingstandards.cpp.Concurrency
 import semmle.code.cpp.dataflow.DataFlow
 import semmle.code.cpp.commons.Alloc
 
-from C11ThreadCreateCall tcc, StackVariable sv, Expr arg, Expr acc
+from C11ThreadCreateCall tcc, Expr arg
 where
   not isExcluded(tcc, Concurrency4Package::appropriateThreadObjectStorageDurationsQuery()) and
   tcc.getArgument(2) = arg and
-  sv.getAnAccess() = acc and
-  // a stack variable that is given as an argument to a thread
-  TaintTracking::localTaint(DataFlow::exprNode(acc), DataFlow::exprNode(arg)) and
-  // or isn't one of the allowed usage patterns
-  not exists(Expr mfc |
-    isAllocationExpr(mfc) and
-    sv.getAnAssignedValue() = mfc and
-    acc.getAPredecessor*() = mfc
-  ) and
-  not exists(TSSGetFunctionCall tsg, TSSSetFunctionCall tss, DataFlow::Node src |
-    sv.getAnAssignedValue() = tsg and
-    acc.getAPredecessor*() = tsg and
-    // there should be dataflow from somewhere (the same somewhere)
-    // into each of the first arguments
-    DataFlow::localFlow(src, DataFlow::exprNode(tsg.getArgument(0))) and
-    DataFlow::localFlow(src, DataFlow::exprNode(tss.getArgument(0)))
+  (
+    exists(ObjectIdentity obj, Expr acc |
+      obj.getASubobjectAccess() = acc and
+      obj.getStorageDuration().isAutomatic() and
+      exists(DataFlow::Node addrNode |
+        (
+          addrNode = DataFlow::exprNode(any(AddressOfExpr e | e.getOperand() = acc))
+          or
+          addrNode = DataFlow::exprNode(acc) and
+          exists(ArrayToPointerConversion c | c.getExpr() = acc)
+        ) and
+        TaintTracking::localTaint(addrNode, DataFlow::exprNode(arg))
+      )
+    )
+    or
+    // TODO: This case is handling threadlocals in a useful way that's not intended to be covered
+    // by the rule. See issue #801. The actual rule should expect no tss_t objects is used, and
+    // this check that this is initialized doesn't seem to belong here. However, it is a useful
+    // check in and of itself, so we should figure out if this is part of an optional rule we
+    // haven't yet implemented and move this behavior there.
+    exists(TSSGetFunctionCall tsg |
+      TaintTracking::localTaint(DataFlow::exprNode(tsg), DataFlow::exprNode(arg)) and
+      not exists(TSSSetFunctionCall tss, DataFlow::Node src |
+        // there should be dataflow from somewhere (the same somewhere)
+        // into each of the first arguments
+        DataFlow::localFlow(src, DataFlow::exprNode(tsg.getArgument(0))) and
+        DataFlow::localFlow(src, DataFlow::exprNode(tss.getArgument(0)))
+      )
+    )
   )
 select tcc, "$@ not declared with appropriate storage duration", arg, "Shared object"

c/cert/src/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.ql

@@ -13,10 +13,16 @@
 
 import cpp
 import codingstandards.c.cert
+import codingstandards.c.Objects
 import semmle.code.cpp.dataflow.DataFlow
 
-class Source extends StackVariable {
-  Source() { not this instanceof Parameter }
+class Source extends Expr {
+  ObjectIdentity rootObject;
+
+  Source() {
+    rootObject.getStorageDuration().isAutomatic() and
+    this = rootObject.getASubobjectAddressExpr()
+  }
 }
 
 class Sink extends DataFlow::Node {
@@ -40,7 +46,7 @@ from DataFlow::Node src, DataFlow::Node sink
 where
   not isExcluded(sink.asExpr(),
     Declarations8Package::appropriateStorageDurationsFunctionReturnQuery()) and
-  exists(Source s | src.asExpr() = s.getAnAccess()) and
+  exists(Source s | src.asExpr() = s) and
   sink instanceof Sink and
   DataFlow::localFlow(src, sink)
 select sink, "$@ with automatic storage may be accessible outside of its lifetime.", src,

c/cert/src/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.ql

@@ -13,15 +13,13 @@
 
 import cpp
 import codingstandards.c.cert
-import codingstandards.cpp.lifetimes.CLifetimes
+import codingstandards.c.Objects
 
 // Note: Undefined behavior is possible regardless of whether the accessed field from the returned
 // struct is an array or a scalar (i.e. arithmetic and pointer types) member, according to the standard.
-from FieldAccess fa, FunctionCall fc
+from FieldAccess fa, TemporaryObjectIdentity tempObject
 where
   not isExcluded(fa, InvalidMemory2Package::doNotModifyObjectsWithTemporaryLifetimeQuery()) and
-  not fa.getQualifier().isLValue() and
-  fa.getQualifier().getUnconverted() = fc and
-  fa.getQualifier().getUnconverted().getUnspecifiedType() instanceof StructOrUnionTypeWithArrayField
-select fa, "Field access on $@ qualifier occurs after its temporary object lifetime.", fc,
-  "function call"
+  fa.getQualifier().getUnconverted() = tempObject
+select fa, "Field access on $@ qualifier occurs after its temporary object lifetime.", tempObject,
+  "temporary object"

c/cert/src/rules/MEM33-C/AllocStructsWithAFlexibleArrayMemberDynamically.ql

@@ -14,14 +14,15 @@
 import cpp
 import codingstandards.c.cert
 import codingstandards.c.Variable
+import codingstandards.c.Objects
 import semmle.code.cpp.models.interfaces.Allocation
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 
 abstract class FlexibleArrayAlloc extends Element {
   /**
    * Returns the `Variable` being allocated.
    */
-  abstract Variable getVariable();
+  abstract Element getReportElement();
 }
 
 /**
@@ -53,18 +54,26 @@ class FlexibleArrayStructDynamicAlloc extends FlexibleArrayAlloc, FunctionCall {
       )
   }
 
-  override Variable getVariable() { result = v }
+  override Element getReportElement() { result = v }
 }
 
 /**
  * A `Variable` of type `FlexibleArrayStructType` that is not allocated dynamically.
  */
-class FlexibleArrayNonDynamicAlloc extends FlexibleArrayAlloc, Variable {
+class FlexibleArrayNonDynamicAlloc extends FlexibleArrayAlloc {
+  ObjectIdentity object;
+
   FlexibleArrayNonDynamicAlloc() {
-    this.getUnspecifiedType().getUnspecifiedType() instanceof FlexibleArrayStructType
+    this = object and
+    not object.getStorageDuration().isAllocated() and
+    // Exclude temporaries. Though they should violate this rule, in practice these results are
+    // often spurious and redundant, such as (*x = *x) which creates an unused temporary object.
+    not object.hasTemporaryLifetime() and
+    object.getType().getUnspecifiedType() instanceof FlexibleArrayStructType and
+    not exists(Variable v | v.getInitializer().getExpr() = this)
   }
 
-  override Variable getVariable() { result = this }
+  override Element getReportElement() { result = object }
 }
 
 from FlexibleArrayAlloc alloc, string message
@@ -77,4 +86,4 @@ where
     alloc instanceof FlexibleArrayNonDynamicAlloc and
     message = "$@ contains a flexible array member but is not dynamically allocated."
   )
-select alloc, message, alloc.getVariable(), alloc.getVariable().getName()
+select alloc, message, alloc.getReportElement(), alloc.getReportElement().toString()

c/cert/test/rules/CON34-C/AppropriateThreadObjectStorageDurations.expected

@@ -1,11 +1,16 @@
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:27,29-37)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:27,54-62)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:34,62-70)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:39,5-13)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:39,30-38)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:40,5-13)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:40,30-38)
-WARNING: module 'TaintTracking' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:27,3-16)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:30,14-22)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:32,22-30)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:34,22-30)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:37,45-53)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:47,33-41)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:47,58-66)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:48,42-50)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:51,9-17)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:51,34-42)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:52,9-17)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:52,34-42)
+WARNING: module 'TaintTracking' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:37,9-22)
+WARNING: module 'TaintTracking' has been deprecated and may be removed in future (AppropriateThreadObjectStorageDurations.ql:47,7-20)
 | test.c:23:3:23:13 | call to thrd_create | $@ not declared with appropriate storage duration | test.c:23:24:23:29 | & ... | Shared object |
 | test.c:74:3:74:13 | call to thrd_create | $@ not declared with appropriate storage duration | test.c:74:24:74:24 | p | Shared object |
 | test.c:85:3:85:13 | call to thrd_create | $@ not declared with appropriate storage duration | test.c:85:24:85:24 | p | Shared object |

c/cert/test/rules/DCL30-C/AppropriateStorageDurationsFunctionReturn.expected

@@ -1,7 +1,7 @@
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:22,20-28)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:26,31-39)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:39,6-14)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:39,26-34)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:45,3-11)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:28,20-28)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:32,31-39)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:45,6-14)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:45,26-34)
+WARNING: module 'DataFlow' has been deprecated and may be removed in future (AppropriateStorageDurationsFunctionReturn.ql:51,3-11)
 | test.c:3:10:3:10 | a | $@ with automatic storage may be accessible outside of its lifetime. | test.c:3:10:3:10 | a | a |
 | test.c:15:4:15:8 | param [inner post update] | $@ with automatic storage may be accessible outside of its lifetime. | test.c:15:12:15:13 | a2 | a2 |

c/cert/test/rules/EXP35-C/DoNotModifyObjectsWithTemporaryLifetime.expected

@@ -1,4 +1,4 @@
-| test.c:65:18:65:18 | a | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:65:9:65:14 | call to get_s1 | function call |
-| test.c:67:18:67:19 | s1 | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:67:9:67:14 | call to get_s3 | function call |
-| test.c:68:18:68:19 | i1 | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:68:9:68:14 | call to get_s3 | function call |
-| test.c:69:18:69:21 | af12 | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:69:9:69:14 | call to get_s4 | function call |
+| test.c:65:18:65:18 | a | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:65:9:65:14 | call to get_s1 | temporary object |
+| test.c:67:18:67:19 | s1 | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:67:9:67:14 | call to get_s3 | temporary object |
+| test.c:68:18:68:19 | i1 | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:68:9:68:14 | call to get_s3 | temporary object |
+| test.c:69:18:69:21 | af12 | Field access on $@ qualifier occurs after its temporary object lifetime. | test.c:69:9:69:14 | call to get_s4 | temporary object |

c/common/src/codingstandards/c/IdentifierLinkage.qll

@@ -0,0 +1,47 @@
+import cpp
+
+newtype TIdentifierLinkage =
+  TIdentifierLinkageExternal() or
+  TIdentifierLinkageInternal() or
+  TIdentifierLinkageNone()
+
+/**
+ * In C, identifiers have internal linkage, or external linkage, or no linkage (6.2.2.1).
+ *
+ * The linkage of an identifier is used to, among other things, determine the storage duration
+ * and/or lifetime of that identifier. Storage durations and lifetimes are often used to define
+ * rules in the various coding standards.
+ */
+class IdentifierLinkage extends TIdentifierLinkage {
+  predicate isExternal() { this = TIdentifierLinkageExternal() }
+
+  predicate isInternal() { this = TIdentifierLinkageInternal() }
+
+  predicate isNone() { this = TIdentifierLinkageNone() }
+
+  string toString() {
+    this.isExternal() and result = "external linkage"
+    or
+    this.isInternal() and result = "internal linkage"
+    or
+    this.isNone() and result = "no linkage"
+  }
+}
+
+/**
+ * Determine the linkage of a variable: external, or static, or none.
+ *
+ * The linkage of a variable is determined by its scope and storage class. Note that other types of
+ * identifiers (e.g. functions) may also have linkage, but that behavior is not covered in this
+ * predicate.
+ */
+IdentifierLinkage linkageOfVariable(Variable v) {
+  // 6.2.2.3, file scope identifiers marked static have internal linkage.
+  v.isTopLevel() and v.isStatic() and result.isInternal()
+  or
+  // 6.2.2.4 describes generally non-static file scope identifiers, which have external linkage.
+  v.isTopLevel() and not v.isStatic() and result.isExternal()
+  or
+  // Note: Not all identifiers have linkage, see 6.2.2.6
+  not v.isTopLevel() and result.isNone()
+}