Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.
Comment: Dynamic Task List: Task added to list 'Review List'
Wiki Markup
According to Section 17.9, "Sleep and Yield" of the Java Language Specification \[[JLS 05|AA. Java References#JLS 05]\]:

{quote}
It is important to note that neither {{Thread.sleep}} nor {{Thread.yield}} have any synchronization semantics. In particular, the compiler does not have to flush writes cached in registers out to shared memory before a call to {{Thread.sleep}} or {{Thread.yield}}, nor does the compiler have to reload values cached in registers after a call to {{Thread.sleep}} or {{Thread.yield}}.
{quote}

Incorrectly assuming that thread suspension and yielding flush the cached registers, reload any values, or provide any [happens-before|BB. Definitions#happens-before order] relations when execution resumes can result in unexpected behavior. 


h2. Noncompliant Code Example ({{sleep()}})

This noncompliant code attempts to use a non-volatile {{boolean done}} as a flag to terminate execution of a thread. A separate thread sets {{done}} to true by calling {{shutdown()}}.

{code:bgColor=#FFCCCC}
final class ControlledStop implements Runnable {
  private boolean done = false;

  public void run() {
    while (!done) {
      try {
        Thread.sleep(1000);
      } catch (InterruptedException e) {
        Thread.currentThread().interrupt(); // Reset interrupted status
      }
    }
  }  

  public void shutdown() {
    this.done = true;
  }
}
{code}

However, the compiler is free to read the field {{this.done}} once, and reuse the cached value in each execution of the loop. Consequently, the while loop might not terminate, even if another thread calls the {{shutdown()}} method to change the value of {{this.done}} \[[JLS 05|AA. Java References#JLS 05]\].  This error could have resulted from the programmer incorrectly assuming that the call to {{Thread.sleep()}} would cause cached values to be reloaded.


h2. Compliant Solution ({{volatile}} flag)

This compliant solution declares the flag {{volatile}} to ensure that updates to it are made visible across multiple threads. 

{code:bgColor=#ccccff}
final class ControlledStop implements Runnable {
  private volatile boolean done = false;

  // ...
}
{code}

The {{volatile}} flag establishes a [happens-before|BB. Definitions#happens-before order] relation between this thread and any other thread that sets {{done}}.


h2. Compliant Solution ({{Thread.interrupt()}})

A better solution for methods that call {{sleep()}} is to use thread interruption, which causes {{sleep()}} to wake up immediately and handle the interruption.

{code:bgColor=#ccccff}
final class ControlledStop implements Runnable {
  @Override public void run() {
    while (!Thread.interrupted()) {
      try {
        Thread.sleep(1000);
      } catch (InterruptedException e) {
        Thread.currentThread().interrupt();
      }
    } 
  }  

  public void shutdown() {
    Thread.currentThread().interrupt();
  }
}
{code}


h2. Noncompliant Code Example ({{getState()}})

This noncompliant code example contains a {{doSomething()}} method that starts a thread. The thread supports interruption by checking a volatile flag and blocks waiting until notified. The {{stop()}} method notifies the thread if it is blocked on the wait and sets the flag to true so that the thread can terminate.

{code:bgColor=#FFCCCC}
public class Waiter {
  private Thread thread;
  private volatile boolean flag;
  private final Object lock = new Object(); 

  public void doSomething() {    
    thread = new Thread(new Runnable() {     
      @Override public void run() {
        synchronized(lock) {
          while (!flag) {
            try {
              lock.wait();
              // ...
            } catch (InterruptedException e) {
              // Forward to handler  
            }
          }
        }
      }
    });
    thread.start();             
  }

  public boolean stop() {
    if (thread != null) {    
      if (thread.getState() == Thread.State.WAITING) {
        flag = true;
        synchronized (lock) {
          lock.notifyAll();
        }
        return true;
      }     
    }
    return false;
  }
}
{code}

Unfortunately, the {{stop()}} method incorrectly uses the {{Thread.getState()}} method to check whether the thread is blocked and has not terminated before delivering the notification.  Using the {{Thread.getState()}} method for synchronization control such as checking whether a thread is blocked on a wait is inappropriate because a blocked thread is not always required to enter the {{WAITING}} or {{TIMED_WAITING}} state in cases where the JVM implements blocking using spin-waiting \[[Goetz 06|AA. Java References#Goetz 06]\]. Because the thread may never enter the {{WAITING}} state, the {{stop()}} method may not terminate the thread.


h2. Compliant Solution

This compliant solution removes the check for determining whether the thread is in the {{WAITING}} state. This check is unnecessary because invoking {{notifyAll()}} on a thread that is not blocked on a {{wait()}} invocation has no effect. 

{code:bgColor=#ccccff}
public class Waiter {
  // ...

  public boolean stop() {
    if (thread != null) {
      flag = true;
      synchronized (lock) {
        lock.notifyAll();
      }
      return true;
    }
    return false;
  }
}
{code}

{mc} This does not talk about invoking getState() and comparing with TERMINATE, RUNNABLE and other states. Should we? {mc}


h2. Risk Assessment

Relying on class Thread's {{sleep()}}, {{yield()}} and {{getState()}} methods for synchronization control can cause unexpected behavior.

|| Rule || Severity || Likelihood || Remediation Cost || Priority || Level ||
| CON16- J | low | probable | medium | {color:green}{*}P4{*}{color} | {color:green}{*}L3{*}{color} |

h3. Automated Detection

TODO

h3. Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the [CERT website|https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+CON44-J].

h2. References

\[[JLS 05|AA. Java References#JLS 05]\] section 17.9 "Sleep and Yield"


h2. Issue Tracking

{tasklist:Review List}
||Completed||Priority||Locked||CreatedDate||CompletedDate||Assignee||Name||
|F|M|F|1271020420270|          |dmohindr|suggested changes in bold: "which causes *the thread blocked on* sleep() to wake up immediately and handle the interruption."|
{tasklist}

----
[!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_left.png!|CON15-J. Ensure actively held locks are released on exceptional conditions]      [!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_up.png!|11. Concurrency (CON)]      [!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_right.png!|CON17-J. Do not invoke ThreadGroup methods]