Merge pull request #20328 from michaelnebel/java/ql4ql

Java: Fix some Ql4Ql violations.

java/ql/lib/experimental/quantum/JCA.qll

@@ -269,7 +269,7 @@ module JCAModel {
   }
 
   /**
-   * Data-flow configuration modelling flow from a cipher string literal to a cipher algorithm consumer.
+   * Data-flow configuration modeling flow from a cipher string literal to a cipher algorithm consumer.
    */
   private module CipherAlgorithmStringToCipherConsumerConfig implements DataFlow::ConfigSig {
     predicate isSource(DataFlow::Node src) { src.asExpr() instanceof CipherStringLiteral }
@@ -404,9 +404,7 @@ module JCAModel {
    * For example, in `Cipher.getInstance(algorithm)`, this class represents `algorithm`.
    */
   class CipherGetInstanceAlgorithmArg extends CipherAlgorithmValueConsumer instanceof Expr {
-    CipherGetInstanceCall call;
-
-    CipherGetInstanceAlgorithmArg() { this = call.getAlgorithmArg() }
+    CipherGetInstanceAlgorithmArg() { this = any(CipherGetInstanceCall call).getAlgorithmArg() }
 
     override Crypto::ConsumerInputDataFlowNode getInputNode() { result.asExpr() = this }
 
@@ -1333,7 +1331,7 @@ module JCAModel {
   }
 
   /**
-   * Data-flow configuration modelling flow from a key agreement string literal to a key agreement algorithm consumer.
+   * Data-flow configuration modeling flow from a key agreement string literal to a key agreement algorithm consumer.
    */
   private module KeyAgreementAlgorithmStringToConsumerConfig implements DataFlow::ConfigSig {
     predicate isSource(DataFlow::Node src) { src.asExpr() instanceof KeyAgreementStringLiteral }
@@ -1539,11 +1537,9 @@ module JCAModel {
   }
 
   class MacOperationCall extends Crypto::MacOperationInstance instanceof MethodCall {
-    Expr output;
-
     MacOperationCall() {
       super.getMethod().getDeclaringType().hasQualifiedName("javax.crypto", "Mac") and
-      (
+      exists(Expr output |
         super.getMethod().hasStringSignature(["doFinal()", "doFinal(byte[])"]) and this = output
         or
         super.getMethod().hasStringSignature("doFinal(byte[], int)") and

java/ql/lib/experimental/quantum/Language.qll

@@ -113,7 +113,7 @@ private class ConstantDataSource extends Crypto::GenericConstantSourceInstance i
 }
 
 /**
- * An instance of random number generation, modelled as the expression
+ * An instance of random number generation, modeled as the expression
  * tied to an output node (i.e., the result of the source of randomness)
  */
 abstract class RandomnessInstance extends Crypto::RandomNumberGenerationInstance {

java/ql/lib/printAst.ql

@@ -18,8 +18,8 @@ external string selectedSourceFile();
 
 class PrintAstConfigurationOverride extends PrintAstConfiguration {
   /**
-   * Holds if the location matches the selected file in the VS Code extension and
-   * the element is `fromSource`.
+   * Holds if the location `l` matches the selected file in the VS Code extension and
+   * the element `e` is `fromSource`.
    */
   override predicate shouldPrint(Element e, Location l) {
     super.shouldPrint(e, l) and

java/ql/lib/semmle/code/java/Concurrency.qll

@@ -26,7 +26,8 @@ predicate locallySynchronizedOnThis(Expr e, RefType thisType) {
 }
 
 /**
- * Holds if `e` is synchronized by a `synchronized` modifier on the enclosing (static) method.
+ * Holds if `e` is synchronized by a `synchronized` modifier on the enclosing (static) method
+ * declared in the type `classType`.
  */
 predicate locallySynchronizedOnClass(Expr e, RefType classType) {
   exists(SynchronizedCallable c | c = e.getEnclosingCallable() |

java/ql/lib/semmle/code/java/Conversions.qll

@@ -100,12 +100,12 @@ class InvocationConversionContext extends ConversionSite {
  * See the Java Language Specification, Section 5.4.
  */
 class StringConversionContext extends ConversionSite {
-  AddExpr a;
-
   StringConversionContext() {
-    a.getAnOperand() = this and
-    not this.getType() instanceof TypeString and
-    a.getAnOperand().getType() instanceof TypeString
+    exists(AddExpr a |
+      a.getAnOperand() = this and
+      not this.getType() instanceof TypeString and
+      a.getAnOperand().getType() instanceof TypeString
+    )
   }
 
   override Type getConversionTarget() { result instanceof TypeString }

java/ql/lib/semmle/code/java/Statement.qll

@@ -291,7 +291,7 @@ class TryStmt extends Stmt, @trystmt {
   CatchClause getACatchClause() { result.getParent() = this }
 
   /**
-   * Gets the `catch` clause at the specified (zero-based) position
+   * Gets the `catch` clause at the specified (zero-based) position `index`
    * in this `try` statement.
    */
   CatchClause getCatchClause(int index) {
@@ -305,7 +305,7 @@ class TryStmt extends Stmt, @trystmt {
   /** Gets a resource variable declaration, if any. */
   LocalVariableDeclStmt getAResourceDecl() { result.getParent() = this and result.getIndex() <= -3 }
 
-  /** Gets the resource variable declaration at the specified position in this `try` statement. */
+  /** Gets the resource variable declaration at the specified position `index` in this `try` statement. */
   LocalVariableDeclStmt getResourceDecl(int index) {
     result = this.getAResourceDecl() and
     index = -3 - result.getIndex()
@@ -314,7 +314,7 @@ class TryStmt extends Stmt, @trystmt {
   /** Gets a resource expression, if any. */
   VarAccess getAResourceExpr() { result.getParent() = this and result.getIndex() <= -3 }
 
-  /** Gets the resource expression at the specified position in this `try` statement. */
+  /** Gets the resource expression at the specified position `index` in this `try` statement. */
   VarAccess getResourceExpr(int index) {
     result = this.getAResourceExpr() and
     index = -3 - result.getIndex()
@@ -323,7 +323,7 @@ class TryStmt extends Stmt, @trystmt {
   /** Gets a resource in this `try` statement, if any. */
   ExprParent getAResource() { result = this.getAResourceDecl() or result = this.getAResourceExpr() }
 
-  /** Gets the resource at the specified position in this `try` statement. */
+  /** Gets the resource at the specified position `index` in this `try` statement. */
   ExprParent getResource(int index) {
     result = this.getResourceDecl(index) or result = this.getResourceExpr(index)
   }

java/ql/lib/semmle/code/java/dataflow/internal/ContainerFlow.qll

@@ -470,7 +470,8 @@ private predicate enhancedForStmtStep(Node node1, Node node2, Type containerType
 }
 
 /**
- * Holds if the step from `node1` to `node2` reads a value from an array.
+ * Holds if the step from `node1` to `node2` reads a value from an array, where
+ * the elements are of type `elemType`.
  * This covers ordinary array reads as well as array iteration through enhanced
  * `for` statements.
  */

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll

@@ -263,8 +263,8 @@ class Content extends TContent {
 
   /**
    * Holds if this element is at the specified location.
-   * The location spans column `startcolumn` of line `startline` to
-   * column `endcolumn` of line `endline` in file `filepath`.
+   * The location spans column `sc` of line `sl` to
+   * column `ec` of line `el` in file `path`.
    * For more information, see
    * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */
@@ -362,8 +362,8 @@ class ContentSet instanceof Content {
 
   /**
    * Holds if this element is at the specified location.
-   * The location spans column `startcolumn` of line `startline` to
-   * column `endcolumn` of line `endline` in file `filepath`.
+   * The location spans column `sc` of line `sl` to
+   * column `ec` of line `el` in file `path`.
    * For more information, see
    * [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */

java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll

@@ -204,7 +204,7 @@ private module Impl {
   /** Gets the character value of expression `e`. */
   string getCharValue(Expr e) { result = e.(CharacterLiteral).getValue() }
 
-  /** Gets the constant `float` value of non-`ConstantIntegerExpr` expressions. */
+  /** Gets the constant `float` value of non-`ConstantIntegerExpr` expression `e`. */
   float getNonIntegerValue(Expr e) {
     result = e.(LongLiteral).getValue().toFloat() or
     result = e.(FloatLiteral).getValue().toFloat() or
@@ -256,12 +256,12 @@ private module Impl {
     exists(EnhancedForStmt for | def = for.getVariable())
   }
 
-  /** Returns the operand of the operation if `def` is a decrement. */
+  /** Returns the operand of the operation if `e` is a decrement. */
   Expr getDecrementOperand(Element e) {
     result = e.(PostDecExpr).getExpr() or result = e.(PreDecExpr).getExpr()
   }
 
-  /** Returns the operand of the operation if `def` is an increment. */
+  /** Returns the operand of the operation if `e` is an increment. */
   Expr getIncrementOperand(Element e) {
     result = e.(PostIncExpr).getExpr() or result = e.(PreIncExpr).getExpr()
   }

java/ql/lib/semmle/code/java/frameworks/Mockito.qll

@@ -223,10 +223,10 @@ class MockitoInjectedField extends MockitoAnnotatedField {
         // If there is no initializer for this field, and there is a most mockable constructor,
         // then we are doing a parameterized injection of mocks into a most mockable constructor.
         result = mockInjectedClass.getAMostMockableConstructor()
-      else
-        if this.usingPropertyInjection()
-        then
-          // We will call the no-arg constructor if the field wasn't initialized.
+      else (
+        this.usingPropertyInjection() and
+        // We will call the no-arg constructor if the field wasn't initialized.
+        (
           not exists(this.getInitializer()) and
           result = mockInjectedClass.getNoArgsConstructor()
           or
@@ -241,9 +241,8 @@ class MockitoInjectedField extends MockitoAnnotatedField {
             // once, but we instead assume that there are sufficient mocks to go around.
             mockedField.getType().(RefType).getAnAncestor() = result.getParameterType(0)
           )
-        else
-          // There's no instance, and no no-arg constructor we can call, so injection fails.
-          none()
+        )
+      )
     )
   }
 
@@ -253,18 +252,16 @@ class MockitoInjectedField extends MockitoAnnotatedField {
    * Field injection only occurs if property injection and not constructor injection is used.
    */
   Field getASetField() {
-    if this.usingPropertyInjection()
-    then
-      result = this.getMockInjectedClass().getASetField() and
-      exists(MockitoMockedField mockedField |
-        mockedField.getDeclaringType() = this.getDeclaringType() and
-        mockedField.isValid()
-      |
-        // We make a simplifying assumption here - in theory, each mock can only be injected
-        // once, but we instead assume that there are sufficient mocks to go around.
-        mockedField.getType().(RefType).getAnAncestor() = result.getType()
-      )
-    else none()
+    this.usingPropertyInjection() and
+    result = this.getMockInjectedClass().getASetField() and
+    exists(MockitoMockedField mockedField |
+      mockedField.getDeclaringType() = this.getDeclaringType() and
+      mockedField.isValid()
+    |
+      // We make a simplifying assumption here - in theory, each mock can only be injected
+      // once, but we instead assume that there are sufficient mocks to go around.
+      mockedField.getType().(RefType).getAnAncestor() = result.getType()
+    )
   }
 }
 

java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJB.qll

@@ -35,14 +35,14 @@ class SessionEjb extends EJB {
     // Either the EJB does not declare any business interfaces explicitly
     // and implements a single interface candidate,
     // which is then considered to be the business interface...
-    count(this.getAnExplicitBusinessInterface()) = 0 and
+    not exists(this.getAnExplicitBusinessInterface()) and
     count(this.getAnImplementedBusinessInterfaceCandidate()) = 1 and
     result = this.getAnImplementedBusinessInterfaceCandidate()
     or
     // ...or each business interface needs to be declared explicitly.
     (
       count(this.getAnImplementedBusinessInterfaceCandidate()) != 1 or
-      count(this.getAnExplicitBusinessInterface()) != 0
+      exists(this.getAnExplicitBusinessInterface())
     ) and
     result = this.getAnExplicitBusinessInterface()
   }

java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll

@@ -55,7 +55,7 @@ private predicate isVarargs(Argument arg, DataFlow::ImplicitVarargsArray varargs
   arg.isVararg() and arg.getCall() = varargs.getCall()
 }
 
-/** Holds if `store` closes `file`. */
+/** Holds if `closeCall` closes `file`. */
 private predicate closesFile(DataFlow::Node file, Call closeCall) {
   closeCall.getCallee() instanceof CloseFileMethod and
   if closeCall.getCallee().isStatic()

java/ql/lib/semmle/code/java/security/FileWritable.qll

@@ -89,7 +89,7 @@ private VarAccess getFileForPathConversion(Expr pathExpr) {
 }
 
 /**
- * Holds if `fileAccess` is used in the `setWorldWritableExpr` to set the file to be world writable.
+ * Holds if `fileAccess` is used in the `setWorldWritable` to set the file to be world writable.
  */
 private predicate fileSetWorldWritable(VarAccess fileAccess, Expr setWorldWritable) {
   // Calls to `File.setWritable(.., false)`.

java/ql/lib/semmle/code/java/security/TempDirUtils.qll

@@ -26,7 +26,8 @@ class MethodFileCreateTempFile extends Method {
 }
 
 /**
- * Holds if `expDest` is some constructor call `new java.io.File(expSource)`, where the specific `File` constructor being used has `paramCount` parameters.
+ * Holds if `expSource` is an argument to a constructor call `exprDest` (constructor from `java.io.File`), where
+ * the specific `File` constructor being used has `paramCount` parameters.
  */
 predicate isFileConstructorArgument(Expr expSource, Expr exprDest, int paramCount) {
   exists(ConstructorCall construtorCall |

java/ql/lib/semmle/code/java/security/UnsafeDeserializationQuery.qll

@@ -552,7 +552,7 @@ private DataFlow::Node getASafelyConfiguredParser() {
 }
 
 /**
- * Holds if `parseMethodQualifierExpr` is a `jodd.json.JsonParser` instance that is configured unsafely
+ * Holds if `parserExpr` is a `jodd.json.JsonParser` instance that is configured unsafely
  * and which never appears to be configured safely.
  */
 private predicate joddJsonParserConfiguredUnsafely(Expr parserExpr) {

java/ql/src/Likely Bugs/Comparison/HashedButNoHash.ql

@@ -14,7 +14,7 @@
 import java
 import Equality
 
-/** A class that defines an `equals` method but no `hashCode` method. */
+/** Holds if `c` defines an `equals` method but no `hashCode` method. */
 predicate eqNoHash(Class c) {
   exists(Method m | m = c.getAMethod() |
     m instanceof EqualsMethod and

java/ql/src/Likely Bugs/Concurrency/NotifyWithoutSynch.ql

@@ -78,7 +78,7 @@ private predicate synchronizedThisAccess(MethodCall ma, Type thisType) {
 
 /**
  * Auxiliary predicate for `unsynchronizedVarAccess`. Holds if
- * there is an enclosing `synchronized` statement on the variable.
+ * there is an enclosing `synchronized` statement on the variable access `x`.
  */
 predicate synchronizedVarAccess(VarAccess x) {
   exists(SynchronizedStmt s, VarAccess y |

java/ql/src/Likely Bugs/Termination/SpinOnField.ql

@@ -33,8 +33,8 @@ class Empty extends Stmt {
 class EmptyLoop extends Stmt {
   EmptyLoop() {
     exists(ForStmt stmt | stmt = this |
-      count(stmt.getAnInit()) = 0 and
-      count(stmt.getAnUpdate()) = 0 and
+      not exists(stmt.getAnInit()) and
+      not exists(stmt.getAnUpdate()) and
       stmt.getStmt() instanceof Empty
     )
     or

java/ql/src/Violations of Best Practice/Magic Constants/MagicConstants.qll

@@ -105,7 +105,7 @@ private predicate valueOccurrenceCount(string value, int n, string context) {
   n > 20
 }
 
-private predicate occurenceCount(Literal lit, string value, int n, string context) {
+private predicate occurrenceCount(Literal lit, string value, int n, string context) {
   valueOccurrenceCount(value, n, context) and
   value = lit.getValue() and
   nonTrivialValue(_, lit, context)
@@ -119,7 +119,7 @@ private predicate check(Literal lit, string value, int n, string context, Compil
   // Check that the literal is nontrivial
   not trivial(lit) and
   // Check that it is repeated a number of times
-  occurenceCount(lit, value, n, context) and
+  occurrenceCount(lit, value, n, context) and
   n > 20 and
   f = lit.getCompilationUnit()
 }

java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql

@@ -48,7 +48,7 @@ private predicate overloadedMethodsMostSpecific(Method n, Method m) {
 private predicate whitelist(string name) { name = "visit" }
 
 /**
- * Method `m` has name `name`, number of parameters `numParams`
+ * Method `m` has name `name`, number of parameters `numParam`
  * and is declared in `t` or inherited from a supertype of `t`.
  */
 pragma[nomagic]

java/ql/src/experimental/quantum/Analysis/ArtifactReuse.qll

@@ -10,15 +10,11 @@ import experimental.quantum.Language
  */
 private module WrapperConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
-    exists(Call c |
-      c = source.asExpr()
-      // not handling references yet, I think we want to flat say references are only ok
-      // if I know the source, otherwise, it has to be through an additional flow step, which
-      // we filter as a source, i.e., references are only allowed as sources only,
-      // no inferrece? Not sure if that would work
-      //or
-      //   source.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = c.getAnArgument()
-    ) and
+    source.asExpr() instanceof Call and
+    // not handling references yet, I think we want to flat say references are only ok
+    // if I know the source, otherwise, it has to be through an additional flow step, which
+    // we filter as a source, i.e., references are only allowed as sources only,
+    // no inferrece? Not sure if that would work
     // Filter out sources that are known additional flow steps, as these are likely not the
     // kind of wrapper source we are looking for.
     not exists(AdditionalFlowInputStep s | s.getOutput() = source)

java/ql/src/experimental/quantum/Examples/BrokenCrypto.ql

@@ -12,7 +12,7 @@
  */
 
 //THIS QUERY IS A REPLICA OF: https://github.com/github/codeql/blob/main/java/ql/src/Security/CWE/CWE-327/BrokenCryptoAlgorithm.ql
-//but uses the **NEW MODELLING**
+//but uses the **NEW MODELING**
 import experimental.quantum.Language
 
 /**

java/ql/src/utils/flowtestcasegenerator/FlowTestCase.qll

@@ -253,7 +253,7 @@ class TestCase extends TTestCase {
 
   /**
    * Returns a call to `source()` wrapped in `newWith` methods as needed according to `input`.
-   * For example, if the input specification is `ArrayElement of MapValue of Argument[0]`, this
+   * For example, if the `stack` is `Argument[0].MapValue.ArrayElement`, this
    * will return `newWithMapValue(newWithArrayElement(source()))`.
    */
   string getInput(SummaryComponentStack stack) {
@@ -269,7 +269,7 @@ class TestCase extends TTestCase {
 
   /**
    * Returns `out` wrapped in `get` methods as needed according to `output`.
-   * For example, if the output specification is `ArrayElement of MapValue of Argument[0]`, this
+   * For example, if the `componentStack` is `Argument[0].MapValue.ArrayElement`, this
    * will return `getArrayElement(getMapValue(out))`.
    */
   string getOutput(SummaryComponentStack componentStack) {