Merge branch 'main' of https://github.com/github/codeql into oscarsj/merge-back-rc-3.21

java/ql/src/Likely Bugs/Arithmetic/OctalLiteral.ql

@@ -19,6 +19,7 @@ where
   lit.getLiteral() = val and
   val.regexpMatch("0[0-7][0-7]+") and
   lit.getParent() instanceof BinaryExpr and
+  not lit.getParent() instanceof Assignment and
   not lit.getParent() instanceof BitwiseExpr and
   not lit.getParent() instanceof ComparisonExpr
 select lit, "Integer literal starts with 0."

java/ql/src/Likely Bugs/Arithmetic/WhitespaceContradictsPrecedence.ql

@@ -138,13 +138,14 @@ int operatorWS(BinaryExpr expr) {
     endOfBinaryLhs(expr, line, lcol) and
     startOfBinaryRhs(expr, line, rcol) and
     parens = getParensNextToOp(expr) and
-    result = rcol - lcol + 1 - expr.getOp().length() - parens
+    result = rcol - lcol - 1 - expr.getOp().length() - parens
   )
 }
 
 /** Find nested binary expressions where the programmer may have made a precedence mistake. */
 predicate interestingNesting(BinaryExpr inner, BinaryExpr outer) {
   inner = outer.getAChildExpr() and
+  not outer instanceof Assignment and
   not inner instanceof AssocNestedExpr and
   not inner instanceof HarmlessNestedExpr and
   not inner.isParenthesized()

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

@@ -58,6 +58,7 @@ predicate equal(Expr left, Expr right) {
     sameVariable(left, right, _)
     or
     exists(BinaryExpr bLeft, BinaryExpr bRight | bLeft = left and bRight = right |
+      not bLeft instanceof Assignment and
       bLeft.getKind() = bRight.getKind() and
       equal(bLeft.getLeftOperand(), bRight.getLeftOperand()) and
       equal(bLeft.getRightOperand(), bRight.getRightOperand())

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

@@ -101,17 +101,10 @@ Expr overFlowCand() {
   |
     bin instanceof AddExpr or
     bin instanceof MulExpr or
-    bin instanceof LeftShiftExpr
-  )
-  or
-  exists(AssignOp op |
-    result = op and
-    positive(op.getDest()) and
-    positive(op.getRhs())
-  |
-    op instanceof AssignAddExpr or
-    op instanceof AssignMulExpr or
-    op instanceof AssignLeftShiftExpr
+    bin instanceof LeftShiftExpr or
+    bin instanceof AssignAddExpr or
+    bin instanceof AssignMulExpr or
+    bin instanceof AssignLeftShiftExpr
   )
   or
   exists(AddExpr add, CompileTimeConstantExpr c |

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

@@ -57,13 +57,15 @@ predicate loopExitGuard(LoopStmt loop, Expr cond) {
  */
 predicate mainLoopCondition(LoopStmt loop, Expr cond) {
   loop.getCondition() = cond and
-  exists(Expr loopReentry, ControlFlowNode last |
-    if exists(loop.(ForStmt).getAnUpdate())
-    then loopReentry = loop.(ForStmt).getUpdate(0)
-    else loopReentry = cond
-  |
-    last.getEnclosingStmt().getEnclosingStmt*() = loop.getBody() and
-    last.getASuccessor().asExpr().getParent*() = loopReentry
+  exists(BasicBlock condBlock | condBlock.getANode().isBefore(cond) |
+    1 < strictcount(condBlock.getAPredecessor()) or loop instanceof DoStmt
+  )
+}
+
+predicate ssaDefinitionInLoop(LoopStmt loop, SsaDefinition ssa) {
+  exists(ControlFlowNode node | node = ssa.getControlFlowNode() |
+    node.getAstNode().(Stmt).getEnclosingStmt*() = loop or
+    node.getAstNode().(Expr).getEnclosingStmt().getEnclosingStmt*() = loop
   )
 }
 
@@ -76,7 +78,7 @@ where
   ) and
   // None of the ssa variables in `cond` are updated inside the loop.
   forex(SsaDefinition ssa, VarRead use | ssa.getARead() = use and use.getParent*() = cond |
-    not ssa.getControlFlowNode().getEnclosingStmt().getEnclosingStmt*() = loop or
+    not ssaDefinitionInLoop(loop, ssa) or
     ssa.getControlFlowNode().asExpr().getParent*() = loop.(ForStmt).getAnInit()
   ) and
   // And `cond` does not use method calls, field reads, or array reads.

java/ql/src/Security/CWE/CWE-295/ImproperWebViewCertificateValidation.java

@@ -1,7 +1,7 @@
 class Bad extends WebViewClient {
     // BAD: All certificates are trusted.
-    public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) { // $hasResult
-        handler.proceed(); 
+    public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) { // $ hasResult
+        handler.proceed();
     }
 }
 
@@ -9,7 +9,7 @@ class Good extends WebViewClient {
     PublicKey myPubKey = ...;
 
     // GOOD: Only certificates signed by a certain public key are trusted.
-    public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) { // $hasResult
+    public void onReceivedSslError (WebView view, SslErrorHandler handler, SslError error) { // $ hasResult
         try {
             X509Certificate cert = error.getCertificate().getX509Certificate();
             cert.verify(this.myPubKey);
@@ -18,5 +18,5 @@ class Good extends WebViewClient {
         catch (CertificateException|NoSuchAlgorithmException|InvalidKeyException|NoSuchProviderException|SignatureException e) {
             handler.cancel();
         }
-    }    
-}
+    }
+}

java/ql/src/Violations of Best Practice/Declarations/Common.qll

@@ -1,41 +1,15 @@
 import java
 
-private Stmt getASwitchChild(SwitchStmt s) {
-  result = s.getAChild()
-  or
-  exists(Stmt mid |
-    mid = getASwitchChild(s) and not mid instanceof SwitchStmt and result = mid.getAChild()
-  )
-}
-
-private predicate blockInSwitch(SwitchStmt s, BasicBlock b) {
-  b.getFirstNode().getEnclosingStmt() = getASwitchChild(s)
-}
-
-private predicate switchCaseControlFlow(SwitchStmt switch, BasicBlock b1, BasicBlock b2) {
-  blockInSwitch(switch, b1) and
-  b1.getASuccessor() = b2 and
-  blockInSwitch(switch, b2)
-}
-
-predicate switchCaseControlFlowPlus(SwitchStmt switch, BasicBlock b1, BasicBlock b2) {
-  switchCaseControlFlow(switch, b1, b2)
-  or
-  exists(BasicBlock mid |
-    switchCaseControlFlowPlus(switch, mid, b2) and
-    switchCaseControlFlow(switch, b1, mid) and
-    not mid.getFirstNode().asStmt() = switch.getACase()
-  )
-}
-
 predicate mayDropThroughWithoutComment(SwitchStmt switch, Stmt switchCase) {
-  switchCase = switch.getACase() and
-  exists(Stmt other, BasicBlock b1, BasicBlock b2 |
-    b1.getFirstNode().asStmt() = switchCase and
-    b2.getFirstNode().asStmt() = other and
-    switchCaseControlFlowPlus(switch, b1, b2) and
-    other = switch.getACase() and
-    not fallThroughCommented(other)
+  exists(int caseIx, SwitchCase next, int nextCaseStmtIx, Stmt lastInCase, ControlFlowNode node |
+    switch.getCase(caseIx) = switchCase and
+    switch.getCase(caseIx + 1) = next and
+    switch.getStmt(nextCaseStmtIx) = next and
+    switch.getStmt(nextCaseStmtIx - 1) = lastInCase and
+    lastInCase != switchCase and
+    node.isAfter(lastInCase) and
+    node.getANormalSuccessor().asStmt() = switch.getAStmt() and
+    not fallThroughCommented(next)
   )
 }
 

java/ql/src/Violations of Best Practice/legacy/AutoBoxing.ql

@@ -36,6 +36,7 @@ Variable flowTarget(Expr arg) {
  */
 predicate unboxed(BoxedExpr e) {
   exists(BinaryExpr bin | e = bin.getAnOperand() |
+    not bin instanceof Assignment and
     if bin instanceof EqualityTest or bin instanceof ComparisonExpr
     then bin.getAnOperand() instanceof PrimitiveExpr
     else bin instanceof PrimitiveExpr

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

@@ -42,7 +42,7 @@ private DataFlow::Node getGeneratingWrapperSet(Crypto::NonceArtifactNode a) {
 }
 
 private predicate ancestorOfArtifact(
-  Crypto::ArtifactNode a, Callable enclosingCallable, ControlFlow::Node midOrTarget
+  Crypto::ArtifactNode a, Callable enclosingCallable, ControlFlowNode midOrTarget
 ) {
   a.asElement().(Expr).getEnclosingCallable() = enclosingCallable and
   (
@@ -87,7 +87,7 @@ predicate isArtifactReuse(Crypto::ArtifactNode a, Crypto::ArtifactNode b) {
       ancestorOfArtifact(b, commonParent, _)
     )
     implies
-    exists(Callable commonParent, ControlFlow::Node aMid, ControlFlow::Node bMid |
+    exists(Callable commonParent, ControlFlowNode aMid, ControlFlowNode bMid |
       ancestorOfArtifact(a, commonParent, aMid) and
       ancestorOfArtifact(b, commonParent, bMid) and
       a instanceof Crypto::NonceArtifactNode and