Java: Add new quality query to detect calls to Thread.run()

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

@@ -9,6 +9,7 @@ public static void main(String args[]) {
 }
 
 class NewThread extends Thread {
+    @Override
     public void run() {
         try {
             Thread.sleep(10000);
@@ -18,4 +19,4 @@ public void run() {
         }
         System.out.println("Child thread activity.");
     }
-}
+}

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

@@ -7,37 +7,37 @@
 <overview>
 <p>A direct call of a <code>Thread</code> object's <code>run</code> method
 does not start a separate thread. The method is executed within the current thread.
-This is an unusual use because <code>Thread.run()</code> is normally 
-intended to be called from within a separate thread. 
+This is an unusual use because <code>Thread.run()</code> is normally
+intended to be called from within a separate thread.
 </p>
 
 </overview>
 <recommendation>
 
-<p>To execute <code>Runnable.run</code> from within a separate thread, do one of the 
+<p>To execute <code>Runnable.run</code> from within a separate thread, do one of the
 following:</p>
 
 <ul>
-    <li>Construct a <code>Thread</code> object using the <code>Runnable</code> object, and call 
+    <li>Construct a <code>Thread</code> object using the <code>Runnable</code> object, and call
     <code>start</code> on the <code>Thread</code> object.</li>
-    <li>Define a subclass of a <code>Thread</code> object, and override the definition of its 
+    <li>Define a subclass of a <code>Thread</code> object, and override the definition of its
     <code>run</code> method. Then construct an instance of this subclass and call <code>start</code>
     on that instance directly.</li>
 </ul>
 
 </recommendation>
 <example>
 
-<p>In the following example, the main thread, <code>ThreadDemo</code>, calls the child thread, 
+<p>In the following example, the main thread, <code>ThreadDemo</code>, calls the child thread,
 <code>NewThread</code>, using <code>run</code>. This causes the child thread to run to
-completion before the rest of the main thread is executed, so that "Child thread activity" is 
+completion before the rest of the main thread is executed, so that "Child thread activity" is
 printed before "Main thread activity".</p>
 
 <sample src="CallsToRunnableRun.java" />
 
-<p>To enable the two threads to run concurrently, create the child thread and call 
+<p>To enable the two threads to run concurrently, create the child thread and call
 <code>start</code>, as shown below. This causes the main thread to
-continue while the child thread is waiting, so that "Main thread activity" is printed before 
+continue while the child thread is waiting, so that "Main thread activity" is printed before
 "Child thread activity".</p>
 
 <sample src="CallsToRunnableRunFixed.java" />
@@ -49,6 +49,12 @@ 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>
+  Java API Specification: <a href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Thread.html#run()">Thread.run()</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>
 
 
 </references>

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

@@ -1,19 +1,26 @@
 /**
- * @name Direct call to a run() method
+ * @name Direct call to a 'run()' method
  * @description Directly calling a 'Thread' object's 'run' method does not start a separate thread
- *              but executes the method within the current thread.
+ *              but executes the method within the current thread and may indicate misunderstanding
+ *              on the programmer's part.
  * @kind problem
  * @problem.severity recommendation
  * @precision high
  * @id java/call-to-thread-run
- * @tags reliability
+ * @previous-id java/run-method-called-on-java-lang-thread-directly
+ * @tags quality
+ *       reliability
  *       correctness
  *       concurrency
  *       external/cwe/cwe-572
  */
 
 import java
 
+/**
+ * The `run()` method of the class `java.lang.Thread` or
+ * of its subclasses.
+ */
 class RunMethod extends Method {
   RunMethod() {
     this.hasName("run") and

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

@@ -1,9 +1,9 @@
 public class ThreadDemo {
     public static void main(String args[]) {
     	NewThread runnable = new NewThread();
-    	
-        runnable.start();                                         // Call 'start' method
-        
+
+        runnable.start();    // Call 'start' method
+
         System.out.println("Main thread activity.");
     }
-}
+}

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 NON_COMPLIANT: `Thread.run()` called directly.
+    thread.start();  // COMPLIANT: Thread started with `.start()`.
+
+    AnotherThread1 anotherThread1 = new AnotherThread1(new Job());
+    anotherThread1.run(); // $ Alert NON_COMPLIANT: 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 NON_COMPLIANT: 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 NON_COMPLIANT: 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 NON_COMPLIANT: 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