Merge pull request #19175 from jcogs33/jcogs33/java/call-to-thread-run

Java: update `java/call-to-thread-run`

java/ql/src/Likely Bugs/Concurrency/CallsToRunnableRun.qhelp

@@ -49,6 +49,15 @@ continue while the child thread is waiting, so that "Main thread activity" is pr
 <li>
   The Java Tutorials: <a href="https://docs.oracle.com/javase/tutorial/essential/concurrency/runthread.html">Defining and Starting a Thread</a>.
 </li>
+<li>
+  SEI CERT Oracle Coding Standard for Java: <a href="https://wiki.sei.cmu.edu/confluence/display/java/THI00-J.+Do+not+invoke+Thread.run()">THI00-J. Do not invoke Thread.run()</a>.
+</li>
+<li>
+  Java API Specification: <a href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Thread.html">Thread</a>.
+</li>
+<li>
+  Java API Specification: <a href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Runnable.html">Runnable</a>.
+</li>
 
 
 </references>

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

@@ -6,6 +6,7 @@
  * @problem.severity recommendation
  * @precision high
  * @id java/call-to-thread-run
+ * @previous-id java/run-method-called-on-java-lang-thread-directly
  * @tags quality
  *       reliability
  *       concurrency

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

@@ -1 +1,5 @@
-| CallsToRunnableRun.java:15:3:15:15 | run(...) | Calling 'Thread.run()' rather than 'Thread.start()' will not spawn a new thread. |
+| CallsToRunnableRun.java:67:5:67:16 | run(...) | Calling 'Thread.run()' rather than 'Thread.start()' will not spawn a new thread. |
+| CallsToRunnableRun.java:71:5:71:24 | run(...) | Calling 'Thread.run()' rather than 'Thread.start()' will not spawn a new thread. |
+| CallsToRunnableRun.java:75:5:75:24 | run(...) | Calling 'Thread.run()' rather than 'Thread.start()' will not spawn a new thread. |
+| CallsToRunnableRun.java:79:5:79:27 | run(...) | Calling 'Thread.run()' rather than 'Thread.start()' will not spawn a new thread. |
+| CallsToRunnableRun.java:83:5:83:27 | run(...) | Calling 'Thread.run()' rather than 'Thread.start()' will not spawn a new thread. |

java/ql/test/query-tests/CallsToRunnableRun/CallsToRunnableRun.java

@@ -1,18 +1,95 @@
-import java.lang.Runnable;
-
-public class CallsToRunnableRun extends Thread implements Runnable{
-	
-	private Thread wrapped;
-	private Runnable callback;
-	
-	@Override
-	public void run() {
-		wrapped.run();
-		callback.run();
-	}
-	
-	public void bad() {
-		wrapped.run();
-		callback.run();
-	}
+class Job implements Runnable {
+  public void run() {
+    /* ... */
+  }
+}
+
+/**
+ * A class that subclasses `java.lang.Thread` and inherits its `.run()` method.
+ */
+class AnotherThread1 extends Thread {
+  AnotherThread1(Runnable runnable) {
+    super(runnable);
+  }
+}
+
+/**
+ * A class that directly subclasses `java.lang.Thread` and overrides its
+ * `.run()` method.
+ */
+class AnotherThread2 extends Thread {
+  AnotherThread2(Runnable runnable) {
+    super(runnable);
+  }
+
+  /**
+   * An overriding definition of `Thread.run`.
+   */
+  @Override
+  public void run() {
+    super.run(); // COMPLIANT: called within a `run` method
+  }
+}
+
+/**
+ * A class that indirectly subclasses `java.lang.Thread` by subclassing
+ * `AnotherThread1` and inherits its `.run()`
+ * method.
+ */
+class YetAnotherThread1 extends AnotherThread1 {
+  YetAnotherThread1(Runnable runnable) {
+    super(runnable);
+  }
+}
+
+/**
+ * A class that indirectly subclasses `java.lang.Thread` by subclassing
+ * `AnotherThread2` and overrides its `.run()`
+ * method.
+ */
+class YetAnotherThread2 extends AnotherThread2 {
+  YetAnotherThread2(Runnable runnable) {
+    super(runnable);
+  }
+
+  /**
+   * An overriding definition of `AnotherThread.run`.
+   */
+  @Override
+  public void run() {
+    super.run(); // COMPLIANT: called within a `run` method
+  }
+}
+
+class ThreadExample {
+  public void f() {
+    Thread thread = new Thread(new Job());
+    thread.run();  // $ Alert - `Thread.run()` called directly.
+    thread.start(); // COMPLIANT: Thread started with `.start()`.
+
+    AnotherThread1 anotherThread1 = new AnotherThread1(new Job());
+    anotherThread1.run(); // $ Alert - Inherited `Thread.run()` called on its instance.
+    anotherThread1.start(); // COMPLIANT: Inherited `Thread.start()` used to start the thread.
+
+    AnotherThread2 anotherThread2 = new AnotherThread2(new Job());
+    anotherThread2.run(); // $ Alert - Overriden `Thread.run()` called on its instance.
+    anotherThread2.start(); // COMPLIANT: Overriden `Thread.start()` used to start the thread.
+
+    YetAnotherThread1 yetAnotherThread1 = new YetAnotherThread1(new Job());
+    yetAnotherThread1.run(); // $ Alert - Inherited `AnotherThread1.run()` called on its instance.
+    yetAnotherThread1.start(); // COMPLIANT: Inherited `AnotherThread.start()` used to start the thread.
+
+    YetAnotherThread2 yetAnotherThread2 = new YetAnotherThread2(new Job());
+    yetAnotherThread2.run(); // $ Alert - Overriden `AnotherThread2.run()` called on its instance.
+    yetAnotherThread2.start(); // COMPLIANT: Overriden `AnotherThread2.start()` used to start the thread.
+
+    Runnable runnable = new Runnable() {
+      public void run() {
+        /* ... */ }
+    };
+    runnable.run(); // COMPLIANT: called on `Runnable` object.
+
+    Job job = new Job();
+    job.run(); // COMPLIANT: called on `Runnable` object.
+  }
 }

java/ql/test/query-tests/CallsToRunnableRun/CallsToRunnableRun.qlref

@@ -1 +1,2 @@
-Likely Bugs/Concurrency/CallsToRunnableRun.ql
+query: Likely Bugs/Concurrency/CallsToRunnableRun.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql