Merge pull request #2683 from aschackmull/java/lshift32

Java: Add new query for large left shifts and bugfix ConstantExpAppearsNonConstant.

Author: yo-h

change-notes/1.24/analysis-java.md

@@ -12,14 +12,15 @@ The following changes in version 1.24 affect Java analysis in all applications.
 |-----------------------------|-----------|--------------------------------------------------------------------|
 | Disabled Spring CSRF protection (`java/spring-disabled-csrf-protection`) | security, external/cwe/cwe-352 | Finds disabled Cross-Site Request Forgery (CSRF) protection in Spring. |
 | Failure to use HTTPS or SFTP URL in Maven artifact upload/download (`java/maven/non-https-url`) | security, external/cwe/cwe-300, external/cwe/cwe-319, external/cwe/cwe-494, external/cwe/cwe-829 | Finds use of insecure protocols during Maven dependency resolution. Results are shown on LGTM by default. |
+| Left shift by more than the type width (`java/lshift-larger-than-type-width`) | correctness | Finds left shifts of ints by 32 bits or more and left shifts of longs by 64 bits or more. Results are shown on LGTM by default. |
 | Suspicious date format (`java/suspicious-date-format`) | correctness | Finds date format patterns that use placeholders that are likely to be incorrect. |
 
 ## Changes to existing queries
 
 | **Query**                    | **Expected impact**    | **Change**                        |
 |------------------------------|------------------------|-----------------------------------|
 | Dereferenced variable may be null (`java/dereferenced-value-may-be-null`) | Fewer false positives | Final fields with a non-null initializer are no longer reported. |
-| Expression always evaluates to the same value (`java/evaluation-to-constant`) | Fewer false positives | Expressions of the form `0 * x` are usually intended and no longer reported. |
+| Expression always evaluates to the same value (`java/evaluation-to-constant`) | Fewer false positives | Expressions of the form `0 * x` are usually intended and no longer reported. Also left shift of ints by 32 bits and longs by 64 bits are no longer reported as they are not constant, these results are instead reported by the new query `java/lshift-larger-than-type-width`. |
 | Useless null check (`java/useless-null-check`) | More true positives | Useless checks on final fields with a non-null initializer are now reported. |
 
 ## Changes to libraries

java/ql/src/Likely Bugs/Arithmetic/ConstantExpAppearsNonConstant.qhelp

@@ -14,10 +14,6 @@ Some expressions always evaluate to the same result, no matter what their subexp
 </li>
 <li><code>x % 1</code> always evaluates to <code>0</code>.
 </li>
-<li><code>x &lt;&lt; 64</code> (when <code>x</code> is of type <code>long</code>) always evaluates to <code>0</code>.
-</li>
-<li><code>x &lt;&lt; 32</code> (when <code>x</code> is not of type <code>long</code>) always evaluates to <code>0</code>.
-</li>
 <li><code>x &amp; 0</code> always evaluates to <code>0</code>.
 </li>
 <li><code>x || true</code> always evaluates to <code>true</code>.
@@ -58,7 +54,7 @@ an integer. The correct check is <code>x % 2 == 0</code>.
 
 <li>
   The Java Language Specification:
-  <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.17.1">Multiplication operator *</a>, <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.17.3">Remainder operator %</a>, <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.19">Left shift operator &lt;&lt;</a>, <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.22.1">Bitwise AND operator &amp;</a>, <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.23">Conditional-and operator &amp;&amp;</a> and <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.24">Conditional-or operator ||</a>.
+  <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.17.1">Multiplication operator *</a>, <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.17.3">Remainder operator %</a>, <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.22.1">Bitwise AND operator &amp;</a>, <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.23">Conditional-and operator &amp;&amp;</a> and <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.24">Conditional-or operator ||</a>.
 </li>
 
 

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

@@ -11,10 +11,6 @@
 
 import java
 
-int integralTypeWidth(IntegralType t) {
-  if t.hasName("long") or t.hasName("Long") then result = 64 else result = 32
-}
-
 int eval(Expr e) { result = e.(CompileTimeConstantExpr).getIntValue() }
 
 predicate isConstantExp(Expr e) {
@@ -48,13 +44,6 @@ predicate isConstantExp(Expr e) {
     eval(r.getRightOperand().getProperExpr()) = 1
   )
   or
-  // Left shift by 64 (longs) or 32 (all other integer types) is constant.
-  exists(LShiftExpr s | s = e |
-    exists(IntegralType t | t = s.getLeftOperand().getType() |
-      eval(s.getRightOperand().getProperExpr()) = integralTypeWidth(t)
-    )
-  )
-  or
   exists(AndBitwiseExpr a | a = e | eval(a.getAnOperand().getProperExpr()) = 0)
   or
   exists(AndLogicalExpr a | a = e |

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

@@ -0,0 +1 @@
+long longVal = intVal << 32; // BAD

java/ql/src/Likely Bugs/Arithmetic/LShiftLargerThanTypeWidth.qhelp

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>
+The maximum shift distance used for left-shift operations is determined by the promoted type of
+its left-hand side. When the promoted type is <code>int</code> only the lowest 5 bits of the
+right-hand side are used as the shift distance. When the promoted type is <code>long</code>
+the lowest 6 bits of the right-hand side are used.
+</p>
+</overview>
+<recommendation>
+
+<p>
+Restrict the amount that you shift any <code>int</code> to the range 0-31, or cast it to <code>long</code> before applying the left shift.
+</p>
+
+</recommendation>
+<example>
+<p>
+The following line tries to left-shift an <code>int</code> by 32 bits.
+</p>
+
+<sample src="LShiftLargerThanTypeWidth.java" />
+
+<p>
+However, left-shifting an <code>int</code> by 32 bits is equivalent to
+left-shifting it by 0 bits, that is, no shift is applied. Instead the value should
+be cast to <code>long</code> before the shift is applied. Then the left-shift of 32 bits will work.
+</p>
+
+<sample src="LShiftLargerThanTypeWidthGood.java" />
+
+</example>
+<references>
+
+
+<li>
+  The Java Language Specification:
+  <a href="https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.19">Shift Operators</a>.
+</li>
+
+
+</references>
+</qhelp>

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

@@ -0,0 +1,26 @@
+/**
+ * @name Left shift by more than the type width
+ * @description Left-shifting an integer by more than its type width indicates a mistake.
+ * @kind problem
+ * @problem.severity warning
+ * @precision very-high
+ * @id java/lshift-larger-than-type-width
+ * @tags correctness
+ */
+
+import java
+
+int integralTypeWidth(IntegralType t) {
+  if t.hasName("long") or t.hasName("Long") then result = 64 else result = 32
+}
+
+from LShiftExpr shift, IntegralType t, int v, string typname, int width
+where
+  shift.getLeftOperand().getType() = t and
+  shift.getRightOperand().(CompileTimeConstantExpr).getIntValue() = v and
+  width = integralTypeWidth(t) and
+  v >= width and
+  typname = ("a " + t.toString()).regexpReplaceAll("a ([aeiouAEIOU])", "an $1")
+select shift,
+  "Left-shifting " + typname + " by more than " + width + " truncates the shift amount from " + v +
+    " to " + (v % width)

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

@@ -0,0 +1 @@
+long longVal = ((long)intVal) << 32; // GOOD

java/ql/test/query-tests/ConstantExpAppearsNonConstant/ConstantExpAppearsNonConstant.expected

@@ -5,16 +5,7 @@
 | Test.java:27:28:27:44 | ... % ... | Expression always evaluates to the same value. |
 | Test.java:29:29:29:48 | ... % ... | Expression always evaluates to the same value. |
 | Test.java:30:32:30:50 | ... % ... | Expression always evaluates to the same value. |
-| Test.java:38:32:38:59 | ... << ... | Expression always evaluates to the same value. |
-| Test.java:41:32:41:59 | ... << ... | Expression always evaluates to the same value. |
-| Test.java:44:33:44:61 | ... << ... | Expression always evaluates to the same value. |
-| Test.java:50:31:50:49 | ... << ... | Expression always evaluates to the same value. |
-| Test.java:51:33:51:52 | ... << ... | Expression always evaluates to the same value. |
-| Test.java:52:35:52:55 | ... << ... | Expression always evaluates to the same value. |
-| Test.java:53:38:53:57 | ... << ... | Expression always evaluates to the same value. |
-| Test.java:54:37:54:58 | ... << ... | Expression always evaluates to the same value. |
-| Test.java:55:40:55:61 | ... << ... | Expression always evaluates to the same value. |
-| Test.java:62:30:62:46 | ... & ... | Expression always evaluates to the same value. |
-| Test.java:63:31:63:47 | ... & ... | Expression always evaluates to the same value. |
-| Test.java:78:32:78:58 | ... \|\| ... | Expression always evaluates to the same value. |
-| Test.java:79:33:79:59 | ... \|\| ... | Expression always evaluates to the same value. |
+| Test.java:37:30:37:46 | ... & ... | Expression always evaluates to the same value. |
+| Test.java:38:31:38:47 | ... & ... | Expression always evaluates to the same value. |
+| Test.java:53:32:53:58 | ... \|\| ... | Expression always evaluates to the same value. |
+| Test.java:54:33:54:59 | ... \|\| ... | Expression always evaluates to the same value. |

java/ql/test/query-tests/ConstantExpAppearsNonConstant/Test.java

@@ -29,31 +29,6 @@ public void tester(){
 	long rem_is_constant_hex = rnd.nextLong() % 0x1; //NOT OK
 	long rem_is_constant_binary = rnd.nextLong() % 01; //NOT OK
 
-	//Left shift by 32 or 64
-	int lshift_not_constant_int = 42 << 6; //OK
-	long lshift_not_constant_long = 42L << 6; //OK
-
-	int lshift_constant_cast = ((char) 42) << 16; //OK
-	int lshift_not_constant_byte = ((byte)rnd.nextInt(8)) << 8; //OK (not constant, because of conversion from byte to int)
-	int lshift_is_constant_byte = ((byte)rnd.nextInt(8)) << 32; //NOT OK
-
-	int lshift_not_constant_char = ((char)rnd.nextInt(8)) << 8; //OK (not constant, because of conversion from byte to int)
-	int lshift_is_constant_char = ((char)rnd.nextInt(8)) << 32; //NOT OK
-
-	int lshift_not_constant_short = ((byte)rnd.nextInt(42)) << 16; //OK (not constant, because of conversion from byte to int)
-	int lshift_is_constant_short = ((byte)rnd.nextInt(42)) << 32; //NOT OK
-
-	int lshift_appears_constant_int = 60 << 32; //OK
-	long lshift_appears_constant_long = 60L << 64; //OK
-
-	int lshift_is_not_constant = rnd.nextInt() << 2; //OK
-	int lshift_is_constant_int = rnd.nextInt() << 32; //NOT OK
-	long lshift_is_constant_long = rnd.nextLong() << 64; //NOT OK
-	int lshift_is_constant_int_hex = rnd.nextInt() << 0x20; //NOT OK
-	int lshift_is_constant_int_binary = rnd.nextInt() << 040; //NOT OK
-	long lshift_is_constant_long_hex = rnd.nextLong() << 0x40; //NOT OK
-	long lshift_is_constant_long_binary = rnd.nextLong() << 0100; //NOT OK
-
 	//Bitwise 'and' by 0
 	int band_not_constant = 42 & 6; //OK
 	int band_appears_constant_left = 0 & 60; //OK

java/ql/test/query-tests/LShiftLargerThanTypeWidth/A.java

@@ -0,0 +1,51 @@
+public class A {
+  void test1(byte b, char c, short s, int i, long l) {
+    long b1 = b << 31; // OK
+    long b2 = b << 32; // BAD
+    long b3 = b << 33; // BAD
+    long b4 = b << 64; // BAD
+
+    long c1 = c << 22; // OK
+    long c2 = c << 42; // BAD
+
+    long s1 = s << 22; // OK
+    long s2 = s << 42; // BAD
+
+    long i1 = i << 22; // OK
+    long i2 = i << 32; // BAD
+    long i3 = i << 42; // BAD
+    long i4 = i << 64; // BAD
+    long i5 = i << 65; // BAD
+
+    long l1 = l << 22; // OK
+    long l2 = l << 32; // OK
+    long l3 = l << 42; // OK
+    long l4 = l << 64; // BAD
+    long l5 = l << 65; // BAD
+  }
+
+  void test2(Byte b, Character c, Short s, Integer i, Long l) {
+    long b1 = b << 31; // OK
+    long b2 = b << 32; // BAD
+    long b3 = b << 33; // BAD
+    long b4 = b << 64; // BAD
+
+    long c1 = c << 22; // OK
+    long c2 = c << 42; // BAD
+
+    long s1 = s << 22; // OK
+    long s2 = s << 42; // BAD
+
+    long i1 = i << 22; // OK
+    long i2 = i << 32; // BAD
+    long i3 = i << 42; // BAD
+    long i4 = i << 64; // BAD
+    long i5 = i << 65; // BAD
+
+    long l1 = l << 22; // OK
+    long l2 = l << 32; // OK
+    long l3 = l << 42; // OK
+    long l4 = l << 64; // BAD
+    long l5 = l << 65; // BAD
+  }
+}