Merge master into next.

Author: adityasharad

change-notes/1.19/analysis-cpp.md

@@ -9,21 +9,33 @@
 | Cast between `HRESULT` and a Boolean type (`cpp/hresult-boolean-conversion`) | external/cwe/cwe-253 | Finds logic errors caused by mistakenly treating the Windows `HRESULT` type as a Boolean instead of testing it with the appropriate macros. Enabled by default. |
 | Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | external/cwe/cwe-732 | This query finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Enabled by default. |
 | Cast from `char*` to `wchar_t*` | security, external/cwe/cwe-704 | Detects potentially dangerous casts from `char*` to `wchar_t*`.  Enabled by default on LGTM. |
-| Dead code due to `goto` or `break` statement (`cpp/dead-code-goto`) | maintainability, external/cwe/cwe-561 | Detects dead code following a goto or break statement. Enabled by default on LGTM. |
+| Dead code due to `goto` or `break` statement (`cpp/dead-code-goto`) | maintainability, external/cwe/cwe-561 | Detects dead code following a `goto` or `break` statement. Enabled by default on LGTM. |
+| Inconsistent direction of for loop | correctness, external/cwe/cwe-835 | This query detects `for` loops where the increment and guard condition don't appear to correspond.  Enabled by default on LGTM. |
+| Incorrect Not Operator Usage | security, external/cwe/cwe-480 | This query finds uses of the logical not (`!`) operator that look like they should be bit-wise not (`~`).  Available but not displayed by default on LGTM. |
+| NULL application name with an unquoted path in call to CreateProcess | security, external/cwe/cwe-428 | This query finds unsafe uses of the `CreateProcess` function.  Available but not displayed by default on LGTM. |
 
 ## Changes to existing queries
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Array offset used before range check | More results and fewer false positive results | The query now recognizes array accesses in different positions within the expression.  False positives where the range is checked before and after the array access have been fixed. |
 | Empty branch of conditional | Fewer false positive results | The query now recognizes commented blocks more reliably. |
 | Expression has no effect | Fewer false positive results | Expressions in template instantiations are now excluded from this query. |
+| Global could be static | Fewer false positive results | Variables with declarations in header files are now excluded from this query. |
 | Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. |
 | Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
-| Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type. |
+| Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type, or makes a non-returning call to another function. |
+| Static array access may cause overflow | More correct results | Data flow to the size argument of a buffer operation is now checked in this query. |
 | Call to memory access function may overflow buffer | More correct results | Array indexing with a negative index is now detected by this query. |
+| Self comparison | Fewer false positive results | Code inside macro invocations is now excluded from the query. |
 | Suspicious call to memset | Fewer false positive results | Types involving decltype are now correctly compared. |
 | Suspicious add with sizeof | Fewer false positive results | Arithmetic with void pointers (where allowed) is now excluded from this query. |
 | Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed.  Expected argument types are determined more accurately, especially for wide string and pointer types.  Custom (non-standard) formatting functions are also identified more accurately. |
+| AV Rule 164 | Fewer false positive results | This query now accounts for explicit casts. |
+| Negation of unsigned value | Fewer false positive results | This query now accounts for explicit casts. |
+| Variable scope too large | Fewer false positive results | Variables with declarations in header files, or that are used at file scope, are now excluded from this query. |
+| Comparison result is always the same | Fewer false positive results | Comparisons in template instantiations are now excluded from this query. |
+| Unsigned comparison to zero | Fewer false positive results | Comparisons in template instantiations are now excluded from this query. |
 
 ## Changes to QL libraries
 

change-notes/1.19/analysis-csharp.md

@@ -29,3 +29,7 @@
 
 * `getArgument()` on `AccessorCall` has been improved so it now takes tuple assignments into account. For example, the argument for the implicit `value` parameter in the setter of property `P` is `0` in `(P, x) = (0, 1)`. Additionally, the argument for the `value` parameter in compound assignments is now only the expanded value, for example, in `P += 7` the argument is `P + 7` and not `7`.
 * The predicate `isInArgument()` has been added to the `AssignableAccess` class. This holds for expressions that are passed as arguments using `in`.
+
+## Changes to the autobuilder
+
+* When determining the target of `msbuild` or `dotnet build`, first look for `.proj` files, then `.sln` files, and finally `.csproj`/`.vcxproj` files. In all three cases, choose the project/solution file closest to the root.

change-notes/1.19/analysis-java.md

@@ -16,6 +16,7 @@
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Array index out of bounds (`java/index-out-of-bounds`) | Fewer false positive results | False positives involving arrays with a length evenly divisible by 3 or some greater number and an index being increased with a similar stride length are no longer reported. |
+| Confusing overloading of methods (`java/confusing-method-signature`) | Fewer false positive results | A bugfix in the inheritance relation ensures that spurious results on certain generic classes no longer occur. |
 | Query built from user-controlled sources (`java/sql-injection`) | More results | Sql injection sinks from the Spring JDBC, MyBatis, and Hibernate frameworks are now reported. |
 | Query built without neutralizing special characters (`java/concatenated-sql-query`) | More results | Sql injection sinks from the Spring JDBC, MyBatis, and Hibernate frameworks are now reported. |
 | Unreachable catch clause (`java/unreachable-catch-clause`) | Fewer false positive results | This rule now accounts for calls to generic methods that throw generic exceptions. |

change-notes/1.19/analysis-javascript.md

@@ -4,15 +4,15 @@
 
 * Modelling of taint flow through array operations has been improved. This may give additional results for the security queries.
 
-* The taint tracking library now recognizes additional sanitization patterns. This may give fewer false-positive results for the security queries.
-
 * Support for AMD modules has been improved. This may give additional results for the security queries as well as any queries that use type inference on code bases that use such modules.
 
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
   - file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra) or [globby](https://www.npmjs.com/package/globby)
   - outbound network access, for example through the [fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API)
   - the [lodash](https://lodash.com), [underscore](https://underscorejs.org/), [async](https://www.npmjs.com/package/async) and [async-es](https://www.npmjs.com/package/async-es) libraries
 
+* The taint tracking library now recognizes additional sanitization patterns. This may give fewer false-positive results for the security queries.
+
 * Type inference for function calls has been improved. This may give additional results for queries that rely on type inference.
 
 * Where applicable, path explanations have been added to the security queries.
@@ -35,29 +35,38 @@
 
 | **Query**                      | **Expected impact**        | **Change**                                   |
 |--------------------------------|----------------------------|----------------------------------------------|
+| Ambiguous HTML id attribute | Lower severity | The severity of this rule has been revised to "warning". |
+| Clear-text logging of sensitive information | Fewer results | This rule now tracks flow more precisely. |
 | Client side cross-site scripting | More results | This rule now also flags HTML injection in the body of an email. |
+| Client-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
+| Conflicting HTML element attributes | Lower severity | The severity of this rule has been revised to "warning". |
+| Duplicate 'if' condition | Lower severity | The severity of this rule has been revised to "warning". |
+| Duplicate switch case | Lower severity | The severity of this rule has been revised to "warning". |
 | Information exposure through a stack trace | More results | This rule now also flags cases where the entire exception object (including the stack trace) may be exposed. |
 | Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |
+| Missing 'this' qualifier | Fewer false-positive results | This rule now recognizes additional intentional calls to global functions. | 
+| Missing variable declaration | Lower severity | The severity of this rule has been revised to "warning". |
 | Regular expression injection | Fewer false-positive results | This rule now identifies calls to `String.prototype.search` with more precision. |
 | Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. |
 | Self assignment | Fewer false-positive results | This rule now ignores self-assignments preceded by a JSDoc comment with a `@type` tag. |
+| Server-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
 | Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. |
 | Unbound event handler receiver | Fewer false-positive results | This rule now recognizes additional ways class methods can be bound. |
 | Uncontrolled data used in remote request | More results | This rule now recognizes additional kinds of requests. |
+| Unknown directive | Fewer false positives results | This rule now recognizes YUI compressor directives. |
 | Unused import | Fewer false-positive results | This rule no longer flags imports used by the `transform-react-jsx` Babel plugin. |
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that may be used by `eval` calls. |
 | Unused variable, import, function or class | Fewer results | This rule now flags import statements with multiple unused imports once. |
 | Useless assignment to local variable | Fewer false-positive results | This rule now recognizes additional ways default values can be set. |
 | Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. |
-| Client-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
-| Server-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
+| Wrong use of 'this' for static method | More results, fewer false-positive results | This rule now recognizes inherited methods. |
 
 ## Changes to QL libraries
 
-* The flow configuration framework now supports distinguishing and tracking different kinds of taint, specified by an extensible class `FlowLabel` (which can also be referred to by its alias `TaintKind`).
-
-* The `DataFlow::ThisNode` class now corresponds to the implicit receiver parameter of a function, as opposed to an indivdual `this` expression. This means `getALocalSource` now maps all `this` expressions within a given function to the same source. The data-flow node associated with a `ThisExpr` can no longer be cast to `DataFlow::SourceNode` or `DataFlow::ThisNode` - it is recomended to use `getALocalSource` before casting or instead of casting.
+* A `DataFlow::ParameterNode` instance now exists for all function parameters. Previously, unused parameters did not have a corresponding dataflow node.
 
 * `ReactComponent::getAThisAccess` has been renamed to `getAThisNode`. The old name is still usable but is deprecated. It no longer gets individual `this` expressions, but the `ThisNode` mentioned above.
 
-* A `DataFlow::ParameterNode` instance now exists for all function parameters. Previously, unused parameters did not have a corresponding dataflow node.
+* The `DataFlow::ThisNode` class now corresponds to the implicit receiver parameter of a function, as opposed to an indivdual `this` expression. This means `getALocalSource` now maps all `this` expressions within a given function to the same source. The data-flow node associated with a `ThisExpr` can no longer be cast to `DataFlow::SourceNode` or `DataFlow::ThisNode` - it is recomended to use `getALocalSource` before casting or instead of casting.
+
+* The flow configuration framework now supports distinguishing and tracking different kinds of taint, specified by an extensible class `FlowLabel` (which can also be referred to by its alias `TaintKind`).

change-notes/1.19/extractor-javascript.md

@@ -16,8 +16,16 @@
 
 ## General improvements
 
-> Changes that affect alerts in many files or from many queries
-> For example, changes to file classification
+* On LGTM, files whose name ends in `.min.js` or `-min.js` are no longer extracted by default, since they most likely contain minified code and results in these files would be hidden by default anyway. To extract such files anyway, you can add the following filters to your `lgtm.yml` file (or add them to existing filters):
+
+```yaml
+extraction:
+  javascript:
+    index:
+      filters:
+        - include: "**/*.min.js"
+        - include: "**/*-min.js"
+```
 
 ## Changes to code extraction
 

cpp/ql/src/Architecture/General Top-Level Information/GeneralStatistics.qhelp

@@ -15,20 +15,9 @@ functions, and the total number of source code resp. comment lines.</p>
 depends on third-party libraries: low self-containedness means that many dependencies
 are to library classes (as opposed to source classes within the same application).</p>
 
-</overview>
-<section title="How to Address the Query Results">
-
 <p>The results of this query are purely informative and more useful for getting an overall impression of the application than for
-identifying particular defects.</p>
-
-
-
-
-
-</section>
-<references>
-
+identifying particular problems with the code.</p>
 
+</overview>
 
-</references>
 </qhelp>

cpp/ql/src/Best Practices/Likely Errors/OffsetUseBeforeRangeCheck.ql

@@ -5,6 +5,7 @@
  * @kind problem
  * @id cpp/offset-use-before-range-check
  * @problem.severity warning
+ * @precision medium
  * @tags reliability
  *       security
  *       external/cwe/cwe-120
@@ -13,10 +14,29 @@
 
 import cpp
 
-from Variable v, LogicalAndExpr andexpr, ArrayExpr access, LTExpr rangecheck
-where access.getArrayOffset() = v.getAnAccess()
-  and andexpr.getLeftOperand().getAChild() = access
-  and andexpr.getRightOperand() = rangecheck
-  and rangecheck.getLeftOperand() = v.getAnAccess()
-  and not access.isInMacroExpansion()
+predicate beforeArrayAccess(Variable v, ArrayExpr access, Expr before) {
+  exists(LogicalAndExpr andexpr |
+    access.getArrayOffset() = v.getAnAccess() and
+    andexpr.getRightOperand().getAChild*() = access and
+    andexpr.getLeftOperand() = before
+  )
+}
+
+predicate afterArrayAccess(Variable v, ArrayExpr access, Expr after) {
+  exists(LogicalAndExpr andexpr |
+    access.getArrayOffset() = v.getAnAccess() and
+    andexpr.getLeftOperand().getAChild*() = access and
+    andexpr.getRightOperand() = after
+  )
+}
+
+from Variable v, ArrayExpr access, LTExpr rangecheck
+where
+  afterArrayAccess(v, access, rangecheck) and
+  rangecheck.getLeftOperand() = v.getAnAccess() and
+  not access.isInMacroExpansion() and
+  not exists(LTExpr altcheck |
+    beforeArrayAccess(v, access, altcheck) and
+    altcheck.getLeftOperand() = v.getAnAccess()
+  )
 select access, "This use of offset '" + v.getName() + "' should follow the $@.", rangecheck, "range check"

cpp/ql/src/Best Practices/Magic Constants/MagicNumbersUseConstant.ql

@@ -4,6 +4,8 @@
  * @kind problem
  * @id cpp/use-number-constant
  * @problem.severity recommendation
+ * @precision low
+ * @tags maintainability
  */
 import cpp
 import MagicConstants

cpp/ql/src/Best Practices/Magic Constants/MagicStringsUseConstant.ql

@@ -4,6 +4,8 @@
  * @kind problem
  * @id cpp/use-string-constant
  * @problem.severity recommendation
+ * @precision low
+ * @tags maintainability
  */
 import cpp
 import MagicConstants

cpp/ql/src/Best Practices/NVI.ql

@@ -4,7 +4,9 @@
  *              to enforce invariants that should hold for the whole hierarchy.
  * @kind problem
  * @id cpp/nvi
- * @problem.severity warning
+ * @problem.severity recommendation
+ * @precision low
+ * @tags maintainability
  */
 import cpp