Merge branch 'master' into python-objectapi-to-valueapi-wrongnumberargumentsincall

Author: BekaValentine

README.md

@@ -13,4 +13,4 @@ We welcome contributions to our standard library and standard checks. Do you hav
 
 ## License
 
-The code in this repository is licensed under [Apache License 2.0](LICENSE) by [GitHub](https://github.com).
+The code in this repository is licensed under the [MIT License](LICENSE) by [GitHub](https://github.com).

change-notes/1.24/analysis-cpp.md

@@ -25,6 +25,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | 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. |
 | Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | Cases where the tainted allocation size is range checked are now more reliably excluded. |
+| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | The query now produces fewer, more accurate results. |
 | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |
 | Unsafe array for days of the year (`cpp/leap-year/unsafe-array-for-days-of-the-year`) |  | This query is no longer run on LGTM. |
 | Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | More correct results | This query now also looks for comparisons of the form `0 <= x`. |
@@ -54,3 +55,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
   * The library now models data flow through formatting functions such as `sprintf`.
 * The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) uses a new intermediate representation. This provides a more precise analysis of pointers to stack variables and flow through parameters, improving the results of many security queries.
 * The global value numbering library (`semmle.code.cpp.valuenumbering.GlobalValueNumbering`) uses a new intermediate representation to provide a more precise analysis of heap allocated memory and pointers to stack variables.
+* `freeCall` in `semmle.code.cpp.commons.Alloc` has been deprecated. The`Allocation` and `Deallocation` models in `semmle.code.cpp.models.interfaces` should be used instead.

change-notes/1.24/analysis-javascript.md

@@ -86,7 +86,7 @@
 | Useless regular-expression character escape (`js/useless-regexp-character-escape`) | Fewer false positive results | This query now distinguishes escapes in strings and regular expression literals. |
 | Identical operands (`js/redundant-operation`) | Fewer results | This query now recognizes cases where the operands change a value using ++/-- expressions. |
 | Superfluous trailing arguments (`js/superfluous-trailing-arguments`) | Fewer results | This query now recognizes cases where a function uses the `Function.arguments` value to process a variable number of parameters. |
-| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes more variations of URL scheme checks. |
+| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional variations of URL scheme checks. |
 
 ## Changes to libraries
 

change-notes/1.25/analysis-javascript.md

@@ -0,0 +1,22 @@
+# Improvements to JavaScript analysis
+
+## General improvements
+
+
+## New queries
+
+| **Query**                                                                       | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
+|---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+
+
+## Changes to existing queries
+
+| **Query**                      | **Expected impact**          | **Change**                                                                |
+|--------------------------------|------------------------------|---------------------------------------------------------------------------|
+| Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
+| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
+| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
+
+## Changes to libraries
+
+* Added data flow for `Map` and `Set`, and added matching type-tracking steps that can accessed using the `CollectionsTypeTracking` module.

cpp/ql/src/Critical/FileClosed.qll

@@ -1,5 +1,6 @@
 import semmle.code.cpp.pointsto.PointsTo
 
+/** Holds if there exists a call to a function that might close the file specified by `e`. */
 predicate closed(Expr e) {
   fcloseCall(_, e) or
   exists(ExprCall c |
@@ -8,10 +9,19 @@ predicate closed(Expr e) {
   )
 }
 
+/** An expression for which there exists a function call that might close it. */
 class ClosedExpr extends PointsToExpr {
   ClosedExpr() { closed(this) }
 
   override predicate interesting() { closed(this) }
 }
 
+/**
+ * Holds if `fc` is a call to a function that opens a file that might be closed. For example:
+ * ```
+ * FILE* f = fopen("file.txt", "r");
+ * ...
+ * fclose(f);
+ * ```
+ */
 predicate fopenCallMayBeClosed(FunctionCall fc) { fopenCall(fc) and anythingPointsTo(fc) }

cpp/ql/src/Critical/LoopBounds.qll

@@ -2,12 +2,24 @@
 
 import cpp
 
+/**
+ * An assignment to a variable with the value `0`. For example:
+ * ```
+ * int x;
+ * x = 0;
+ * ```
+ * but not:
+ * ```
+ * int x = 0;
+ * ```
+ */
 class ZeroAssignment extends AssignExpr {
   ZeroAssignment() {
     this.getAnOperand() instanceof VariableAccess and
     this.getAnOperand() instanceof Zero
   }
 
+  /** Gets a variable that is assigned the value `0`. */
   Variable assignedVariable() { result.getAnAccess() = this.getAnOperand() }
 }
 

cpp/ql/src/Critical/MemoryFreed.qll

@@ -4,15 +4,24 @@ private predicate freed(Expr e) {
   e = any(DeallocationExpr de).getFreedExpr()
   or
   exists(ExprCall c |
-    // cautiously assume that any ExprCall could be a freeCall.
+    // cautiously assume that any `ExprCall` could be a deallocation expression.
     c.getAnArgument() = e
   )
 }
 
+/** An expression that might be deallocated. */
 class FreedExpr extends PointsToExpr {
   FreedExpr() { freed(this) }
 
   override predicate interesting() { freed(this) }
 }
 
+/**
+ * An allocation expression that might be deallocated. For example:
+ * ```
+ * int* p = new int;
+ * ...
+ * delete p;
+ * ```
+ */
 predicate allocMayBeFreed(AllocationExpr alloc) { anythingPointsTo(alloc) }

cpp/ql/src/Critical/Negativity.qll

@@ -1,10 +1,19 @@
 import cpp
 
+/**
+ * Holds if `val` is an access to the variable `v`, or if `val`
+ * is an assignment with an access to `v` on the left-hand side.
+ */
 predicate valueOfVar(Variable v, Expr val) {
   val = v.getAnAccess() or
   val.(AssignExpr).getLValue() = v.getAnAccess()
 }
 
+/**
+ * Holds if either:
+ * - `cond` is an (in)equality expression that compares the variable `v` to the value `-1`, or
+ * - `cond` is a relational expression that compares the variable `v` to a constant.
+ */
 predicate boundsCheckExpr(Variable v, Expr cond) {
   exists(EQExpr eq |
     cond = eq and
@@ -43,6 +52,18 @@ predicate boundsCheckExpr(Variable v, Expr cond) {
   )
 }
 
+/**
+ * Holds if `node` is an expression in a conditional statement and `succ` is an
+ * immediate successor of `node` that may be reached after evaluating `node`.
+ * For example, given
+ * ```
+ * if (a < 10 && b) func1();
+ * else func2();
+ * ```
+ * this predicate holds when either:
+ * - `node` is `a < 10` and `succ` is `func2()` or `b`, or
+ * - `node` is `b` and `succ` is `func1()` or `func2()`
+ */
 predicate conditionalSuccessor(ControlFlowNode node, ControlFlowNode succ) {
   if node.isCondition()
   then succ = node.getATrueSuccessor() or succ = node.getAFalseSuccessor()
@@ -52,6 +73,12 @@ predicate conditionalSuccessor(ControlFlowNode node, ControlFlowNode succ) {
     )
 }
 
+/**
+ * Holds if the current value of the variable `v` at control-flow
+ * node `n` has been used either in:
+ * - an (in)equality comparison with the value `-1`, or
+ * - a relational comparison that compares `v` to a constant.
+ */
 predicate boundsChecked(Variable v, ControlFlowNode node) {
   exists(Expr test |
     boundsCheckExpr(v, test) and
@@ -63,6 +90,14 @@ predicate boundsChecked(Variable v, ControlFlowNode node) {
   )
 }
 
+/**
+ * Holds if `cond` compares `v` to some common error values. Specifically, this
+ * predicate holds when:
+ * - `cond` checks that `v` is equal to `-1`, or
+ * - `cond` checks that `v` is less than `0`, or
+ * - `cond` checks that `v` is less than or equal to `-1`, or
+ * - `cond` checks that `v` is not some common success value (see `successCondition`).
+ */
 predicate errorCondition(Variable v, Expr cond) {
   exists(EQExpr eq |
     cond = eq and
@@ -88,6 +123,14 @@ predicate errorCondition(Variable v, Expr cond) {
   )
 }
 
+/**
+ * Holds if `cond` compares `v` to some common success values. Specifically, this
+ * predicate holds when:
+ * - `cond` checks that `v` is not equal to `-1`, or
+ * - `cond` checks that `v` is greater than or equal than `0`, or
+ * - `cond` checks that `v` is greater than `-1`, or
+ * - `cond` checks that `v` is not some common error value (see `errorCondition`).
+ */
 predicate successCondition(Variable v, Expr cond) {
   exists(NEExpr ne |
     cond = ne and
@@ -113,6 +156,11 @@ predicate successCondition(Variable v, Expr cond) {
   )
 }
 
+/**
+ * Holds if there exists a comparison operation that checks whether `v`
+ * represents some common *error* values, and `n` may be reached
+ * immediately following the comparison operation.
+ */
 predicate errorSuccessor(Variable v, ControlFlowNode n) {
   exists(Expr cond |
     errorCondition(v, cond) and n = cond.getATrueSuccessor()
@@ -121,6 +169,11 @@ predicate errorSuccessor(Variable v, ControlFlowNode n) {
   )
 }
 
+/**
+ * Holds if there exists a comparison operation that checks whether `v`
+ * represents some common *success* values, and `n` may be reached
+ * immediately following the comparison operation.
+ */
 predicate successSuccessor(Variable v, ControlFlowNode n) {
   exists(Expr cond |
     successCondition(v, cond) and n = cond.getATrueSuccessor()
@@ -129,6 +182,10 @@ predicate successSuccessor(Variable v, ControlFlowNode n) {
   )
 }
 
+/**
+ * Holds if the current value of the variable `v` at control-flow node
+ * `n` may have been checked against a common set of *error* values.
+ */
 predicate checkedError(Variable v, ControlFlowNode n) {
   errorSuccessor(v, n)
   or
@@ -139,6 +196,10 @@ predicate checkedError(Variable v, ControlFlowNode n) {
   )
 }
 
+/**
+ * Holds if the current value of the variable `v` at control-flow node
+ * `n` may have been checked against a common set of *success* values.
+ */
 predicate checkedSuccess(Variable v, ControlFlowNode n) {
   successSuccessor(v, n)
   or

cpp/ql/src/Critical/NewDelete.qll

@@ -5,17 +5,34 @@
 import cpp
 import semmle.code.cpp.controlflow.SSA
 import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.models.implementations.Allocation
+import semmle.code.cpp.models.implementations.Deallocation
 
 /**
  * Holds if `alloc` is a use of `malloc` or `new`.  `kind` is
  * a string describing the type of the allocation.
  */
 predicate allocExpr(Expr alloc, string kind) {
-  isAllocationExpr(alloc) and
-  not alloc.isFromUninstantiatedTemplate(_) and
   (
-    alloc instanceof FunctionCall and
-    kind = "malloc"
+    exists(Function target |
+      alloc.(AllocationExpr).(FunctionCall).getTarget() = target and
+      (
+        target.getName() = "operator new" and
+        kind = "new" and
+        // exclude placement new and custom overloads as they
+        // may not conform to assumptions
+        not target.getNumberOfParameters() > 1
+        or
+        target.getName() = "operator new[]" and
+        kind = "new[]" and
+        // exclude placement new and custom overloads as they
+        // may not conform to assumptions
+        not target.getNumberOfParameters() > 1
+        or
+        not target instanceof OperatorNewAllocationFunction and
+        kind = "malloc"
+      )
+    )
     or
     alloc instanceof NewExpr and
     kind = "new" and
@@ -28,7 +45,8 @@ predicate allocExpr(Expr alloc, string kind) {
     // exclude placement new and custom overloads as they
     // may not conform to assumptions
     not alloc.(NewArrayExpr).getAllocatorCall().getTarget().getNumberOfParameters() > 1
-  )
+  ) and
+  not alloc.isFromUninstantiatedTemplate(_)
 }
 
 /**
@@ -110,8 +128,20 @@ predicate allocReaches(Expr e, Expr alloc, string kind) {
  * describing the type of that free or delete.
  */
 predicate freeExpr(Expr free, Expr freed, string kind) {
-  freeCall(free, freed) and
-  kind = "free"
+  exists(Function target |
+    freed = free.(DeallocationExpr).getFreedExpr() and
+    free.(FunctionCall).getTarget() = target and
+    (
+      target.getName() = "operator delete" and
+      kind = "delete"
+      or
+      target.getName() = "operator delete[]" and
+      kind = "delete[]"
+      or
+      not target instanceof OperatorDeleteDeallocationFunction and
+      kind = "free"
+    )
+  )
   or
   free.(DeleteExpr).getExpr() = freed and
   kind = "delete"

cpp/ql/src/JPL_C/LOC-3/Rule 17/BasicIntTypes.ql

@@ -30,7 +30,7 @@ predicate allowedTypedefs(TypedefType t) {
  * Gets a type which appears literally in the declaration of `d`.
  */
 Type getAnImmediateUsedType(Declaration d) {
-  d.isDefined() and
+  d.hasDefinition() and
   (
     result = d.(Function).getType() or
     result = d.(Variable).getType()