Merge branch 'master' into init-dynamic-alloc-newexpr

Author: MathiasVP

.gitignore

@@ -14,6 +14,9 @@
 .vs/*
 !.vs/VSWorkspaceSettings.json
 
+# Byte-compiled python files
+*.pyc
+
 # It's useful (though not required) to be able to unpack codeql in the ql checkout itself
 /codeql/
 .vscode/settings.json

change-notes/1.24/analysis-cpp.md

@@ -20,6 +20,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | Fewer false positive results | Fixed false positive results in template code. |
 | Missing return statement (`cpp/missing-return`) | Fewer false positive results | Functions containing `asm` statements are no longer highlighted by this query. |
+| Missing return statement (`cpp/missing-return`) | More accurate locations | Locations reported by this query are now more accurate in some cases. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. |
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) |  | This query is no longer run on LGTM. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |

config/sync-files.py

@@ -0,0 +1,140 @@
+#!/usr/bin/env python3
+
+# Due to various technical limitations, we sometimes have files that need to be
+# kept identical in the repository. This script loads a database of such
+# files and can perform two functions: check whether they are still identical,
+# and overwrite the others with a master copy if needed.
+
+import hashlib
+import shutil
+import os
+import sys
+import json
+import re
+path = os.path
+
+file_groups = {}
+
+def add_prefix(prefix, relative):
+    result = path.join(prefix, relative)
+    if path.commonprefix((path.realpath(result), path.realpath(prefix))) != \
+            path.realpath(prefix):
+        raise Exception("Path {} is not below {}".format(
+            result, prefix))
+    return result
+
+def load_if_exists(prefix, json_file_relative):
+    json_file_name = path.join(prefix, json_file_relative)
+    if path.isfile(json_file_name):
+        print("Loading file groups from", json_file_name)
+        with open(json_file_name, 'r', encoding='utf-8') as fp:
+            raw_groups = json.load(fp)
+        prefixed_groups = {
+                name: [
+                    add_prefix(prefix, relative)
+                    for relative in relatives
+                ]
+                for name, relatives in raw_groups.items()
+            }
+        file_groups.update(prefixed_groups)
+
+# Generates a list of C# test files that should be in sync
+def csharp_test_files():
+    test_file_re = re.compile('.*(Bad|Good)[0-9]*\\.cs$')
+    csharp_doc_files = {
+        file:os.path.join(root, file)
+            for root, dirs, files in os.walk("csharp/ql/src")
+            for file in files
+            if test_file_re.match(file)
+    }
+    return {
+        "C# test '" + file + "'" : [os.path.join(root, file), csharp_doc_files[file]]
+            for root, dirs, files in os.walk("csharp/ql/test")
+            for file in files
+            if file in csharp_doc_files
+    }
+
+def file_checksum(filename):
+    with open(filename, 'rb') as file_handle:
+        return hashlib.sha1(file_handle.read()).hexdigest()
+
+def check_group(group_name, files, master_file_picker, emit_error):
+    checksums = {file_checksum(f) for f in files}
+
+    if len(checksums) == 1:
+        return
+
+    master_file = master_file_picker(files)
+    if master_file is None:
+        emit_error(__file__, 0,
+                "Files from group '"+ group_name +"' not in sync.")
+        emit_error(__file__, 0,
+                "Run this script with a file-name argument among the "
+                "following to overwrite the remaining files with the contents "
+                "of that file or run with the --latest switch to update each "
+                "group of files from the most recently modified file in the group.")
+        for filename in files:
+            emit_error(__file__, 0, "    " + filename)
+    else:
+        print("  Syncing others from", master_file)
+        for filename in files:
+            if filename == master_file:
+                continue
+            print("    " + filename)
+            os.replace(filename, filename + '~')
+            shutil.copy(master_file, filename)
+        print("  Backups written with '~' appended to file names")
+
+def chdir_repo_root():
+    root_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), '..')
+    os.chdir(root_path)
+
+def choose_master_file(master_file, files):
+    if master_file in files:
+        return master_file
+    else:
+        return None
+
+def choose_latest_file(files):
+    latest_time = None
+    latest_file = None
+    for filename in files:
+        file_time = os.path.getmtime(filename)
+        if (latest_time is None) or (latest_time < file_time):
+            latest_time = file_time
+            latest_file = filename
+    return latest_file
+
+local_error_count = 0
+def emit_local_error(path, line, error):
+    print('ERROR: ' + path + ':' + line + " - " + error)
+    global local_error_count
+    local_error_count += 1
+
+# This function is invoked directly by a CI script, which passes a different error-handling
+# callback.
+def sync_identical_files(emit_error):
+    if len(sys.argv) == 1:
+        master_file_picker = lambda files: None
+    elif len(sys.argv) == 2:
+        if sys.argv[1] == "--latest":
+            master_file_picker = choose_latest_file
+        elif os.path.isfile(sys.argv[1]):
+            master_file_picker = lambda files: choose_master_file(sys.argv[1], files)
+        else:
+            raise Exception("File not found")
+    else:
+        raise Exception("Bad command line or file not found")
+    chdir_repo_root()
+    load_if_exists('.', 'config/identical-files.json')
+    file_groups.update(csharp_test_files())
+    for group_name, files in file_groups.items():
+        check_group(group_name, files, master_file_picker, emit_error)
+
+def main():
+    sync_identical_files(emit_local_error)
+    if local_error_count > 0:
+        exit(1)
+
+if __name__ == "__main__":
+    main()

cpp/ql/src/jsf/4.13 Functions/AV Rule 114.ql

@@ -30,7 +30,13 @@ predicate functionsMissingReturnStmt(Function f, ControlFlowNode blame) {
   ) and
   exists(ReturnStmt s |
     f.getAPredecessor() = s and
-    blame = s.getAPredecessor()
+    (
+      blame = s.getAPredecessor() and
+      count(blame.getASuccessor()) = 1
+      or
+      blame = s and
+      exists(ControlFlowNode pred | pred = s.getAPredecessor() | count(pred.getASuccessor()) != 1)
+    )
   )
 }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl.qll

@@ -2089,6 +2089,8 @@ private class SummaryCtxSome extends SummaryCtx, TSummaryCtxSome {
 
   SummaryCtxSome() { this = TSummaryCtxSome(p, ap) }
 
+  int getParameterPos() { p.isParameterOf(_, result) }
+
   override string toString() { result = p + ": " + ap }
 
   predicate hasLocationInfo(
@@ -2482,13 +2484,15 @@ pragma[nomagic]
 private predicate paramFlowsThrough(
   ReturnKindExt kind, CallContextCall cc, SummaryCtxSome sc, AccessPath ap, Configuration config
 ) {
-  exists(PathNodeMid mid, ReturnNodeExt ret |
+  exists(PathNodeMid mid, ReturnNodeExt ret, int pos |
     mid.getNode() = ret and
     kind = ret.getKind() and
     cc = mid.getCallContext() and
     sc = mid.getSummaryCtx() and
     config = mid.getConfiguration() and
-    ap = mid.getAp()
+    ap = mid.getAp() and
+    pos = sc.getParameterPos() and
+    not kind.(ParamUpdateReturnKind).getPosition() = pos
   )
 }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl2.qll

@@ -2089,6 +2089,8 @@ private class SummaryCtxSome extends SummaryCtx, TSummaryCtxSome {
 
   SummaryCtxSome() { this = TSummaryCtxSome(p, ap) }
 
+  int getParameterPos() { p.isParameterOf(_, result) }
+
   override string toString() { result = p + ": " + ap }
 
   predicate hasLocationInfo(
@@ -2482,13 +2484,15 @@ pragma[nomagic]
 private predicate paramFlowsThrough(
   ReturnKindExt kind, CallContextCall cc, SummaryCtxSome sc, AccessPath ap, Configuration config
 ) {
-  exists(PathNodeMid mid, ReturnNodeExt ret |
+  exists(PathNodeMid mid, ReturnNodeExt ret, int pos |
     mid.getNode() = ret and
     kind = ret.getKind() and
     cc = mid.getCallContext() and
     sc = mid.getSummaryCtx() and
     config = mid.getConfiguration() and
-    ap = mid.getAp()
+    ap = mid.getAp() and
+    pos = sc.getParameterPos() and
+    not kind.(ParamUpdateReturnKind).getPosition() = pos
   )
 }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl3.qll

@@ -2089,6 +2089,8 @@ private class SummaryCtxSome extends SummaryCtx, TSummaryCtxSome {
 
   SummaryCtxSome() { this = TSummaryCtxSome(p, ap) }
 
+  int getParameterPos() { p.isParameterOf(_, result) }
+
   override string toString() { result = p + ": " + ap }
 
   predicate hasLocationInfo(
@@ -2482,13 +2484,15 @@ pragma[nomagic]
 private predicate paramFlowsThrough(
   ReturnKindExt kind, CallContextCall cc, SummaryCtxSome sc, AccessPath ap, Configuration config
 ) {
-  exists(PathNodeMid mid, ReturnNodeExt ret |
+  exists(PathNodeMid mid, ReturnNodeExt ret, int pos |
     mid.getNode() = ret and
     kind = ret.getKind() and
     cc = mid.getCallContext() and
     sc = mid.getSummaryCtx() and
     config = mid.getConfiguration() and
-    ap = mid.getAp()
+    ap = mid.getAp() and
+    pos = sc.getParameterPos() and
+    not kind.(ParamUpdateReturnKind).getPosition() = pos
   )
 }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImpl4.qll

@@ -2089,6 +2089,8 @@ private class SummaryCtxSome extends SummaryCtx, TSummaryCtxSome {
 
   SummaryCtxSome() { this = TSummaryCtxSome(p, ap) }
 
+  int getParameterPos() { p.isParameterOf(_, result) }
+
   override string toString() { result = p + ": " + ap }
 
   predicate hasLocationInfo(
@@ -2482,13 +2484,15 @@ pragma[nomagic]
 private predicate paramFlowsThrough(
   ReturnKindExt kind, CallContextCall cc, SummaryCtxSome sc, AccessPath ap, Configuration config
 ) {
-  exists(PathNodeMid mid, ReturnNodeExt ret |
+  exists(PathNodeMid mid, ReturnNodeExt ret, int pos |
     mid.getNode() = ret and
     kind = ret.getKind() and
     cc = mid.getCallContext() and
     sc = mid.getSummaryCtx() and
     config = mid.getConfiguration() and
-    ap = mid.getAp()
+    ap = mid.getAp() and
+    pos = sc.getParameterPos() and
+    not kind.(ParamUpdateReturnKind).getPosition() = pos
   )
 }
 

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImplLocal.qll

@@ -2089,6 +2089,8 @@ private class SummaryCtxSome extends SummaryCtx, TSummaryCtxSome {
 
   SummaryCtxSome() { this = TSummaryCtxSome(p, ap) }
 
+  int getParameterPos() { p.isParameterOf(_, result) }
+
   override string toString() { result = p + ": " + ap }
 
   predicate hasLocationInfo(
@@ -2482,13 +2484,15 @@ pragma[nomagic]
 private predicate paramFlowsThrough(
   ReturnKindExt kind, CallContextCall cc, SummaryCtxSome sc, AccessPath ap, Configuration config
 ) {
-  exists(PathNodeMid mid, ReturnNodeExt ret |
+  exists(PathNodeMid mid, ReturnNodeExt ret, int pos |
     mid.getNode() = ret and
     kind = ret.getKind() and
     cc = mid.getCallContext() and
     sc = mid.getSummaryCtx() and
     config = mid.getConfiguration() and
-    ap = mid.getAp()
+    ap = mid.getAp() and
+    pos = sc.getParameterPos() and
+    not kind.(ParamUpdateReturnKind).getPosition() = pos
   )
 }
 

cpp/ql/src/semmle/code/cpp/exprs/Expr.qll

@@ -2,6 +2,7 @@ import semmle.code.cpp.Element
 private import semmle.code.cpp.Enclosing
 private import semmle.code.cpp.internal.ResolveClass
 private import semmle.code.cpp.internal.AddressConstantExpression
+private import semmle.code.cpp.models.implementations.Allocation
 
 /**
  * A C/C++ expression.
@@ -804,8 +805,10 @@ class NewOrNewArrayExpr extends Expr, @any_new_expr {
    * call the constructor of `T` but will not allocate memory.
    */
   Expr getPlacementPointer() {
-    isStandardPlacementNewAllocator(this.getAllocator()) and
-    result = this.getAllocatorCall().getArgument(1)
+    result =
+      this
+          .getAllocatorCall()
+          .getArgument(this.getAllocator().(OperatorNewAllocationFunction).getPlacementArgument())
   }
 }
 
@@ -1194,12 +1197,6 @@ private predicate convparents(Expr child, int idx, Element parent) {
   )
 }
 
-private predicate isStandardPlacementNewAllocator(Function operatorNew) {
-  operatorNew.getName().matches("operator new%") and
-  operatorNew.getNumberOfParameters() = 2 and
-  operatorNew.getParameter(1).getType() instanceof VoidPointerType
-}
-
 // Pulled out for performance. See QL-796.
 private predicate hasNoConversions(Expr e) { not e.hasConversion() }