Merge branch 'master' into pseudo-random-bytes

Max Schaefer · web-flow · commit 4fc27aaa51a5 · 2018-12-12T08:19:57.000Z
diff --git a/change-notes/1.20/analysis-javascript.md b/change-notes/1.20/analysis-javascript.md
@@ -21,6 +21,7 @@
 |--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
 | Client-side cross-site scripting           | More results                 | This rule now recognizes WinJS functions that are vulnerable to HTML injection. |
 | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. |
-| Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements. |
+| Unused parameter                           | Fewer false-positive results | This rule no longer flags parameters with leading underscore. |
+| Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |
 
 ## Changes to QL libraries
diff --git a/cpp/ql/test/library-tests/sideEffects/functions/error.cpp b/cpp/ql/test/library-tests/sideEffects/functions/error.cpp
@@ -0,0 +1,19 @@
+// semmle-extractor-options: --expect_errors
+
+void functionBeforeError()
+{
+}
+
+void functionWithError1()
+{
+	aaaaaaaaaa(); // error
+}
+
+void functionWithError2()
+{
+	int i = aaaaaaaaaa(); // error
+}
+
+void functionAfterError()
+{
+}
diff --git a/cpp/ql/test/library-tests/sideEffects/functions/sideEffects.expected b/cpp/ql/test/library-tests/sideEffects/functions/sideEffects.expected
@@ -43,6 +43,10 @@
 | cpp.cpp:87:5:87:26 | functionAccessesStatic | int | false |
 | cpp.cpp:93:6:93:14 | increment | int & -> void | false |
 | cpp.cpp:97:6:97:16 | doIncrement | void | false |
+| error.cpp:3:6:3:24 | functionBeforeError | void | true |
+| error.cpp:7:6:7:23 | functionWithError1 | void | false |
+| error.cpp:12:6:12:23 | functionWithError2 | void | false |
+| error.cpp:17:6:17:23 | functionAfterError | void | true |
 | file://:0:0:0:0 | operator= | __va_list_tag & | false |
 | file://:0:0:0:0 | operator= | __va_list_tag & | false |
 | sideEffects.c:4:5:4:6 | f1 | int | true |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/ExprHasNoEffect.expected
@@ -1,5 +1,8 @@
 | calls.cpp:8:5:8:5 | 1 | This expression has no effect. | calls.cpp:8:5:8:5 | 1 |  |
 | calls.cpp:12:5:12:16 | call to thingy | This expression has no effect (because $@ has no external side effects). | calls.cpp:7:15:7:20 | thingy | thingy |
+| expr.cpp:8:2:8:2 | 0 | This expression has no effect. | expr.cpp:8:2:8:2 | 0 |  |
+| expr.cpp:9:7:9:7 | 0 | This expression has no effect. | expr.cpp:9:7:9:7 | 0 |  |
+| expr.cpp:10:2:10:5 | ... , ... | This expression has no effect. | expr.cpp:10:2:10:5 | ... , ... |  |
 | preproc.c:89:2:89:4 | call to fn4 | This expression has no effect (because $@ has no external side effects). | preproc.c:33:5:33:7 | fn4 | fn4 |
 | preproc.c:94:2:94:4 | call to fn9 | This expression has no effect (because $@ has no external side effects). | preproc.c:78:5:78:7 | fn9 | fn9 |
 | template.cpp:19:3:19:3 | call to operator++ | This expression has no effect (because $@ has no external side effects). | template.cpp:9:10:9:19 | operator++ | operator++ |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/expr.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/ExprHasNoEffect/expr.cpp
@@ -0,0 +1,13 @@
+namespace Expr {
+
+int i;
+
+void comma_expr_test()
+{
+	i++, i++; // GOOD
+	0, i++; // BAD (first part)
+	i++, 0; // BAD (second part)
+	0, 0; // BAD (whole)
+}
+
+}
diff --git a/java/ql/src/Likely Bugs/Termination/ConstantLoopCondition.ql b/java/ql/src/Likely Bugs/Termination/ConstantLoopCondition.ql
@@ -81,5 +81,5 @@ where
   not exists(MethodAccess ma | ma.getParent*() = cond) and
   not exists(FieldRead fa | fa.getParent*() = cond) and
   not exists(ArrayAccess aa | aa.getParent*() = cond)
-select loop, "Loop might not terminate, as this $@ is constant within the loop.", cond,
-  "loop condition"
+select cond, "$@ might not terminate, as this loop condition is constant within the loop.", loop,
+  "Loop"
diff --git a/java/ql/test/query-tests/ConstantLoopCondition/ConstantLoopCondition.expected b/java/ql/test/query-tests/ConstantLoopCondition/ConstantLoopCondition.expected
@@ -1,3 +1,3 @@
-| A.java:6:5:6:16 | stmt | Loop might not terminate, as this $@ is constant within the loop. | A.java:6:11:6:15 | !... | loop condition |
-| A.java:12:5:12:19 | stmt | Loop might not terminate, as this $@ is constant within the loop. | A.java:13:11:13:15 | ... > ... | loop condition |
-| A.java:27:5:27:38 | stmt | Loop might not terminate, as this $@ is constant within the loop. | A.java:27:20:27:32 | ... < ... | loop condition |
+| A.java:6:11:6:15 | !... | $@ might not terminate, as this loop condition is constant within the loop. | A.java:6:5:6:16 | stmt | Loop |
+| A.java:13:11:13:15 | ... > ... | $@ might not terminate, as this loop condition is constant within the loop. | A.java:12:5:12:19 | stmt | Loop |
+| A.java:27:20:27:32 | ... < ... | $@ might not terminate, as this loop condition is constant within the loop. | A.java:27:5:27:38 | stmt | Loop |
diff --git a/javascript/ql/src/AngularJS/DuplicateDependency.ql b/javascript/ql/src/AngularJS/DuplicateDependency.ql
@@ -10,6 +10,7 @@
  */
 
 import javascript
+import semmle.javascript.RestrictedLocations
 
 predicate isRepeatedDependency(AngularJS::InjectableFunction f, string name, ASTNode location) {
   exists(int i, int j | i < j and
@@ -20,4 +21,4 @@ predicate isRepeatedDependency(AngularJS::InjectableFunction f, string name, AST
 from AngularJS::InjectableFunction f, ASTNode node, string name
 where isRepeatedDependency(f, name, node) and
       not count(f.asFunction().getParameterByName(name)) > 1 // avoid duplicating reports from js/duplicate-parameter-name
-select f, "This function has a duplicate dependency '$@'.", node, name
+select (FirstLineOf)f.asFunction(), "This function has a duplicate dependency '$@'.", node, name
diff --git a/javascript/ql/src/AngularJS/RepeatedInjection.ql b/javascript/ql/src/AngularJS/RepeatedInjection.ql
@@ -10,8 +10,9 @@
  */
 
 import javascript
+import semmle.javascript.RestrictedLocations
 
 from AngularJS::InjectableFunction f, ASTNode explicitInjection
 where count(f.getAnExplicitDependencyInjection()) > 1 and
       explicitInjection = f.getAnExplicitDependencyInjection()
-select f.asFunction(), "This function has $@ defined in multiple places.", explicitInjection, "dependency injections"
+select (FirstLineOf)f.asFunction(), "This function has $@ defined in multiple places.", explicitInjection, "dependency injections"
diff --git a/javascript/ql/src/AngularJS/UnusedAngularDependency.ql b/javascript/ql/src/AngularJS/UnusedAngularDependency.ql
@@ -11,6 +11,7 @@
 
 import javascript
 import Declarations.UnusedParameter
+import semmle.javascript.RestrictedLocations
 
 predicate isUnusedParameter(Function f, string msg, Parameter parameter) {
   exists(Variable pv |
@@ -36,4 +37,4 @@ predicate isMissingParameter(AngularJS::InjectableFunction f, string msg, ASTNod
 
 from AngularJS::InjectableFunction f, string message, ASTNode location
 where isUnusedParameter(f.asFunction(), message, location) or isMissingParameter(f, message, location)
-select location, message
+select (FirstLineOf)location, message
diff --git a/javascript/ql/src/Declarations/UnusedParameter.ql b/javascript/ql/src/Declarations/UnusedParameter.ql
@@ -9,7 +9,7 @@
  */
 
 import javascript
-import UnusedParameter // local library
+import UnusedParameter
 
 from Parameter p
 where isAnAccidentallyUnusedParameter(p)
diff --git a/javascript/ql/src/Declarations/UnusedParameter.qll b/javascript/ql/src/Declarations/UnusedParameter.qll
@@ -46,7 +46,9 @@ predicate isUnused(Function f, Parameter p, Variable pv, int i) {
   // functions without a body cannot use their parameters
   f.hasBody() and
   // field parameters are used to initialize a field
-  not p instanceof FieldParameter
+  not p instanceof FieldParameter and
+  // common convention: parameters with leading underscore are intentionally unused
+  pv.getName().charAt(0) != "_"
 }
 
 /**
diff --git a/javascript/ql/src/Declarations/UnusedVariable.ql b/javascript/ql/src/Declarations/UnusedVariable.ql
@@ -22,7 +22,9 @@ class UnusedLocal extends LocalVariable {
     not exists(FunctionExpr fe | this = fe.getVariable()) and
     not exists(ClassExpr ce | this = ce.getVariable()) and
     not exists(ExportDeclaration ed | ed.exportsAs(this, _)) and
-    not exists(LocalVarTypeAccess type | type.getVariable() = this)
+    not exists(LocalVarTypeAccess type | type.getVariable() = this) and
+    // common convention: variables with leading underscore are intentionally unused
+    getName().charAt(0) != "_"
   }
 }
 
diff --git a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/DuplicateDependency.expected
@@ -1,4 +1,5 @@
 | duplicates.js:2:5:2:18 | function f(){} | This function has a duplicate dependency '$@'. | duplicates.js:3:26:3:31 | 'dup5' | dup5 |
-| duplicates.js:6:14:6:57 | ['dup2a ... p2b){}] | This function has a duplicate dependency '$@'. | duplicates.js:6:24:6:30 | 'dup2a' | dup2a |
-| duplicates.js:7:14:7:57 | ['dup3b ... p3b){}] | This function has a duplicate dependency '$@'. | duplicates.js:7:24:7:30 | 'dup3b' | dup3b |
-| duplicates.js:8:14:8:79 | ['dup4' ... p4C){}] | This function has a duplicate dependency '$@'. | duplicates.js:8:35:8:40 | 'dup4' | dup4 |
+| duplicates.js:6:33:6:56 | functio ... up2b){} | This function has a duplicate dependency '$@'. | duplicates.js:6:24:6:30 | 'dup2a' | dup2a |
+| duplicates.js:7:33:7:56 | functio ... up3b){} | This function has a duplicate dependency '$@'. | duplicates.js:7:24:7:30 | 'dup3b' | dup3b |
+| duplicates.js:8:43:8:78 | functio ... up4C){} | This function has a duplicate dependency '$@'. | duplicates.js:8:35:8:40 | 'dup4' | dup4 |
+| duplicates.js:15:35:15:112 | functio ...       } | This function has a duplicate dependency '$@'. | duplicates.js:15:25:15:32 | 'dup11a' | dup11a |
diff --git a/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js b/javascript/ql/test/query-tests/AngularJS/DuplicateDependency/duplicates.js
@@ -12,5 +12,7 @@
         .run(['notDup8a', 'notDup8b', function(notDup8a, notDup8b){}]) // OK
         .run(['notDup9a', 'notDup9b', function(notDup9c, notDup9d){}]) // OK
         .run(['dup10a', 'dup10a', 'dup10a', function(dup10a, dup10a, dup10a){}]) // OK (flagged by js/duplicate-parameter-name)
+        .run(['dup11a', 'dup11a', function(dup11a, dup11b){ // NOT OK (alert formatting for multi-line function)
+        }])
     ;
 })();
diff --git a/javascript/ql/test/query-tests/AngularJS/RepeatedInjection/RepeatedInjection.expected b/javascript/ql/test/query-tests/AngularJS/RepeatedInjection/RepeatedInjection.expected
@@ -2,3 +2,5 @@
 | repeated-injection.js:6:5:6:31 | functio ... name){} | This function has $@ defined in multiple places. | repeated-injection.js:8:54:8:73 | ['name', $Injected2] | dependency injections |
 | repeated-injection.js:10:5:10:31 | functio ... name){} | This function has $@ defined in multiple places. | repeated-injection.js:11:5:11:22 | $Injected3.$inject | dependency injections |
 | repeated-injection.js:10:5:10:31 | functio ... name){} | This function has $@ defined in multiple places. | repeated-injection.js:12:5:12:22 | $Injected3.$inject | dependency injections |
+| repeated-injection.js:33:5:33:84 | functio ... )\\n    } | This function has $@ defined in multiple places. | repeated-injection.js:35:5:35:23 | $Injected10.$inject | dependency injections |
+| repeated-injection.js:33:5:33:84 | functio ... )\\n    } | This function has $@ defined in multiple places. | repeated-injection.js:36:56:36:76 | ['name' ... cted10] | dependency injections |
diff --git a/javascript/ql/test/query-tests/AngularJS/RepeatedInjection/repeated-injection.js b/javascript/ql/test/query-tests/AngularJS/RepeatedInjection/repeated-injection.js
@@ -30,4 +30,9 @@
 
     angular.module('app9').controller('controller9', ['name', function inline9(name){}]); // OK
 
+    function $Injected10(name){ // NOT OK (alert formatting for multi-line function)
+    }
+    $Injected10.$inject = ['name'];
+    angular.module('app10').controller('controller10', ['name', $Injected10]);
+
 })();
diff --git a/javascript/ql/test/query-tests/AngularJS/UnusedAngularDependency/UnusedAngularDependency.expected b/javascript/ql/test/query-tests/AngularJS/UnusedAngularDependency/UnusedAngularDependency.expected
@@ -2,3 +2,4 @@
 | unused-angular-dependency.js:14:14:14:39 | ["unuse ... n() {}] | This function has 0 parameters, but 1 dependency is injected into it. |
 | unused-angular-dependency.js:16:14:16:53 | ["used2 ... d2) {}] | This function has 1 parameter, but 2 dependencies are injected into it. |
 | unused-angular-dependency.js:17:14:17:52 | ["unuse ... n() {}] | This function has 0 parameters, but 2 dependencies are injected into it. |
+| unused-angular-dependency.js:18:14:18:105 | ["used2 ...      }] | This function has 1 parameter, but 2 dependencies are injected into it. |
diff --git a/javascript/ql/test/query-tests/AngularJS/UnusedAngularDependency/unused-angular-dependency.js b/javascript/ql/test/query-tests/AngularJS/UnusedAngularDependency/unused-angular-dependency.js
@@ -15,6 +15,8 @@
         .run(f2)
         .run(["used2", "unused9", function(used2) {}]) // NOT OK
         .run(["unused10", "unused11", function() {}]) // NOT OK
+        .run(["used2", "unused12", function(used2) { // NOT OK (alert formatting for multi-line function)
+        }])
     ;
 })();
 angular.module('app2')
diff --git a/javascript/ql/test/query-tests/Declarations/UnusedParameter/tst.js b/javascript/ql/test/query-tests/Declarations/UnusedParameter/tst.js
@@ -33,4 +33,8 @@ var g = f3;
 // OK
 define(function (require, exports, module) {
 	module.x = 23;
-});
+});
+
+// OK: starts with underscore
+function f(_p) {
+}
diff --git a/javascript/ql/test/query-tests/Declarations/UnusedVariable/UnusedVariable.expected b/javascript/ql/test/query-tests/Declarations/UnusedVariable/UnusedVariable.expected
@@ -9,5 +9,7 @@
 | multi-imports.js:2:1:2:42 | import  ... om 'x'; | Unused imports alphabetically, ordered. |
 | require-react-in-other-scope.js:2:9:2:13 | React | Unused variable React. |
 | typeoftype.ts:9:7:9:7 | y | Unused variable y. |
+| underscore.js:6:7:6:7 | e | Unused variable e. |
+| underscore.js:7:7:7:7 | f | Unused variable f. |
 | unusedShadowed.ts:1:1:1:26 | import  ... where'; | Unused import T. |
 | unusedShadowed.ts:2:1:2:31 | import  ... where'; | Unused import object. |
diff --git a/javascript/ql/test/query-tests/Declarations/UnusedVariable/underscore.js b/javascript/ql/test/query-tests/Declarations/UnusedVariable/underscore.js
@@ -0,0 +1,10 @@
+function f(a) {
+  const [a,  // OK: used
+	     _,  // OK: starts with underscore
+	     _c, // OK: starts with underscore
+	     d,  // OK: used
+	     e,  // NOT OK
+	     f]  // NOT OK
+         = a;
+  return a + d;
+}
