Merge branch 'master' into moremsalloc

Author: jbj

change-notes/1.21/analysis-cpp.md

@@ -14,5 +14,6 @@
 | Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | Fewer false positive results | Fixed an issue where functions were being identified as allocation functions inappropriately.  Also affects `cpp/new-array-delete-mismatch` and `cpp/new-delete-array-mismatch`. |
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |
 | Memory is never freed (`cpp/memory-never-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |
+| Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Resource allocation and deallocation functions are now determined more accurately. |
 
 ## Changes to QL libraries

cpp/config/suites/security/cwe-119

@@ -3,8 +3,8 @@
     @name Call to memory access function may overflow buffer (CWE-119)
 + semmlecode-cpp-queries/Critical/OverflowStatic.ql: /CWE/CWE-119
     @name Static array access may cause overflow (CWE-119)
-# + semmlecode-cpp-queries/Critical/OverflowDestination.ql: /CWE/CWE-119
-#    ^ disabled due to timeout issue
++ semmlecode-cpp-queries/Critical/OverflowDestination.ql: /CWE/CWE-119
+    @name Copy function using source size (CWE-119)
 + semmlecode-cpp-queries/Likely Bugs/Memory Management/SuspiciousCallToStrncat.ql: /CWE/CWE-119
     @name Potentially unsafe call to strncat (CWE-119)
 + semmlecode-cpp-queries/Likely Bugs/Memory Management/StrncpyFlippedArgs.ql: /CWE/CWE-119

cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql

@@ -21,7 +21,7 @@ predicate acquireExpr(Expr acquire, string kind) {
   exists(FunctionCall fc, Function f, string name |
     fc = acquire and
     f = fc.getTarget() and
-    name = f.getName() and 
+    name = f.getQualifiedName() and 
     (
       (
         name = "fopen" and
@@ -47,7 +47,7 @@ predicate releaseExpr(Expr release, Expr resource, string kind) {
   exists(FunctionCall fc, Function f, string name |
     fc = release and
     f = fc.getTarget() and
-    name = f.getName() and 
+    name = f.getQualifiedName() and 
     (
       (
         name = "fclose" and

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/Variants.cpp

@@ -73,3 +73,39 @@ class MyClass6
 
 	int *a, *b, *c;
 };
+
+class MyClass7
+{
+public:
+	MyClass7()
+	{
+	}
+
+	bool open()
+	{
+		// ...
+	}
+
+	void close()
+	{
+		// ...
+	}
+};
+
+class myClass7Test
+{
+public:
+	myClass7Test()
+	{
+		success = mc7.open(); // GOOD
+	}
+
+	~myClass7Test()
+	{
+		mc7.close();
+	}
+
+private:
+	MyClass7 mc7;
+	bool success;
+};

csharp/extractor/Semmle.Extraction.CIL/Entities/Assembly.cs

@@ -37,19 +37,21 @@ public Assembly(Context cx) : base(cx)
             if (!def.PublicKey.IsNil)
                 assemblyName.SetPublicKey(cx.mdReader.GetBlobBytes(def.PublicKey));
 
-            ShortId = cx.GetId(assemblyName.FullName) + "#file:///" + cx.assemblyPath.Replace("\\", "/");
+            ShortId = cx.GetId(FullName) + "#file:///" + cx.assemblyPath.Replace("\\", "/");
 
             file = new File(cx, cx.assemblyPath);
         }
 
         static readonly Id suffix = new StringId(";assembly");
 
+        string FullName => assemblyName.GetPublicKey() is null ? assemblyName.FullName + ", PublicKeyToken=null" : assemblyName.FullName;
+
         public override IEnumerable<IExtractionProduct> Contents
         {
             get
             {
                 yield return file;
-                yield return Tuples.assemblies(this, file, assemblyName.FullName, assemblyName.Name, assemblyName.Version.ToString());
+                yield return Tuples.assemblies(this, file, FullName, assemblyName.Name, assemblyName.Version.ToString());
 
                 if (cx.pdb != null)
                 {

csharp/ql/src/semmle/code/cil/BasicBlock.qll

@@ -257,9 +257,7 @@ private module Internal {
     or
     cfn.isJoin()
     or
-    exists(ControlFlowNode pred | pred = cfn.getAPredecessor() |
-      strictcount(pred.getASuccessor()) > 1
-    )
+    cfn.getAPredecessor().isBranch()
   }
 
   /**

csharp/ql/src/semmle/code/cil/CIL.qll

@@ -17,3 +17,4 @@ import Handler
 import ControlFlow
 import DataFlow
 import Attribute
+import Stubs

csharp/ql/src/semmle/code/cil/CallableReturns.qll

@@ -0,0 +1,55 @@
+/**
+ * Provides predicates for analysing the return values of callables.
+ */
+
+private import CIL
+
+cached
+private module Cached {
+  /** Holds if method `m` always returns null. */
+  cached
+  predicate alwaysNullMethod(Method m) { forex(Expr e | m.canReturn(e) | alwaysNullExpr(e)) }
+
+  /** Holds if method `m` always returns non-null. */
+  cached
+  predicate alwaysNotNullMethod(Method m) { forex(Expr e | m.canReturn(e) | alwaysNotNullExpr(e)) }
+
+  /** Holds if method `m` always throws an exception. */
+  cached
+  predicate alwaysThrowsMethod(Method m) {
+    m.hasBody() and
+    not exists(m.getImplementation().getAnInstruction().(Return))
+  }
+
+  /** Holds if method `m` always throws an exception of type `t`. */
+  cached
+  predicate alwaysThrowsException(Method m, Type t) {
+    alwaysThrowsMethod(m) and
+    forex(Throw ex | ex = m.getImplementation().getAnInstruction() | t = ex.getExpr().getType())
+  }
+}
+import Cached
+
+/** Holds if expression `expr` always evaluates to `null`. */
+private predicate alwaysNullExpr(Expr expr) {
+  expr instanceof NullLiteral
+  or
+  alwaysNullMethod(expr.(StaticCall).getTarget())
+  or
+  forex(VariableUpdate vu | DefUse::variableUpdateUse(_, vu, expr) |
+    alwaysNullExpr(vu.getSource())
+  )
+}
+
+/** Holds if expression `expr` always evaluates to non-null. */
+private predicate alwaysNotNullExpr(Expr expr) {
+  expr instanceof Opcodes::Newobj
+  or
+  expr instanceof Literal and not expr instanceof NullLiteral
+  or
+  alwaysNotNullMethod(expr.(StaticCall).getTarget())
+  or
+  forex(VariableUpdate vu | DefUse::variableUpdateUse(_, vu, expr) |
+    alwaysNotNullExpr(vu.getSource())
+  )
+}

csharp/ql/src/semmle/code/cil/ControlFlow.qll

@@ -104,7 +104,10 @@ class ControlFlowNode extends @cil_controlflow_node {
   Type getType() { none() }
 
   /** Holds if this control flow node has more than one predecessor. */
-  predicate isJoin() { count(getAPredecessor()) > 1 }
+  predicate isJoin() { strictcount(this.getAPredecessor()) > 1 }
+
+  /** Holds if this control flow node has more than one successor. */
+  predicate isBranch() { strictcount(this.getASuccessor()) > 1 }
 }
 
 /**
@@ -137,6 +140,6 @@ class TrueFlow extends FlowType, TTrueFlow {
 }
 
 /** False control flow. */
-class FalseFlow extends FlowType, TTrueFlow {
+class FalseFlow extends FlowType, TFalseFlow {
   override string toString() { result = "false" }
 }

csharp/ql/src/semmle/code/cil/DataFlow.qll

@@ -60,7 +60,7 @@ class Tainted extends TaintType, TTaintedValue { }
 private predicate localExactStep(DataFlowNode src, DataFlowNode sink) {
   src = sink.(Opcodes::Dup).getAnOperand()
   or
-  DefUse::defUse(_, src, sink)
+  defUse(_, src, sink)
   or
   src = sink.(ParameterReadAccess).getTarget()
   or
@@ -88,7 +88,7 @@ private predicate localTaintStep(DataFlowNode src, DataFlowNode sink) {
 }
 
 cached
-private module DefUse {
+module DefUse {
   /**
    * A classification of variable references into reads and writes.
    */
@@ -185,21 +185,26 @@ private module DefUse {
     )
   }
 
-  /** Holds if the update `def` can be used at the read `use`. */
+  /** Holds if the variable update `vu` can be used at the read `use`. */
   cached
-  predicate defUse(StackVariable target, DataFlowNode def, ReadAccess use) {
-    exists(VariableUpdate vu | def = vu.getSource() |
-      defReachesReadWithinBlock(target, vu, use)
-      or
-      exists(BasicBlock bb, int i |
-        exists(refRank(bb, i, target, Read())) and
-        use = bb.getNode(i) and
-        defReachesEndOfBlock(bb.getAPredecessor(), vu, target) and
-        not defReachesReadWithinBlock(target, _, use)
-      )
+  predicate variableUpdateUse(StackVariable target, VariableUpdate vu, ReadAccess use) {
+    defReachesReadWithinBlock(target, vu, use)
+    or
+    exists(BasicBlock bb, int i |
+      exists(refRank(bb, i, target, Read())) and
+      use = bb.getNode(i) and
+      defReachesEndOfBlock(bb.getAPredecessor(), vu, target) and
+      not defReachesReadWithinBlock(target, _, use)
     )
   }
+
+  /** Holds if the update `def` can be used at the read `use`. */
+  cached
+  predicate defUse(StackVariable target, Expr def, ReadAccess use) {
+    exists(VariableUpdate vu | def = vu.getSource() | variableUpdateUse(target, vu, use))
+  }
 }
+private import DefUse
 
 abstract library class VariableUpdate extends Instruction {
   abstract Expr getSource();