Page History

...

final class Adder {
  private int a;
  private int b;

  public synchronized int getSum() {
    return a + b;
  }

  public synchronized void setValues(int a, int b) {
    this.a = a;
    this.b = b;
  }
}

Noncompliant Code Example (Overflow Check, Atomic Integer Fields)

The issues described in the previous noncompliant code example can also occur when the a and b fields of type int are replaced with atomic integers, as shown in the noncompliant code example below.

final class Adder {
  private final AtomicInteger a = new AtomicInteger();
  private final AtomicInteger b = new AtomicInteger();

  public int getSum() throws ArithmeticException {
    // Check for integer overflow
    if (b.get() > 0 ? a.get() > Integer.MAX_VALUE - b.get() : a.get() < Integer.MIN_VALUE - b.get()) {
      throw new ArithmeticException("Not in range");
    }
    return a.get() + b.get(); // or, return a.getAndAdd(b.get());
  }

  public void setValues(int a, int b) {
    this.a.set(a);
    this.b.set(b);
  }
}

For example, when a thread is executing setValues(), another thread may invoke getSum() and retrieve an incorrect result. Furthermore, in the absence of synchronization, data races occur in the check for integer overflow. For instance, a thread can call setValues() after a second thread that is attempting to add the numbers has read a, but before it has read b. In this case, the second thread will get an improper sum.

Even worse, a thread can call setValues() after a second thread has verified that overflow will not occur, but before the second thread reads the values to be added. That would cause the second thread to add two values without checking for overflow, yielding an incorrect sum. Even though a check for integer overflow is installed, it is ineffective because of the time-of-check, time-of-use (TOCTOU) condition between the overflow check and the addition operation.

Compliant Solution (Addition, Synchronized)

This compliant solution synchronizes the setValues() and getSum() methods to ensure atomicity.

final class Adder {
  private int a;
  private int b;

  public synchronized int getSum() throws ArithmeticException {
    // Check for integer overflow
    if (b > 0 ? a > Integer.MAX_VALUE - b : a < Integer.MIN_VALUE - b) {
      throw new ArithmeticException("Not in range");
    }

    return a + b;
  }

  public synchronized void setValues(int a, int b) {
    this.a = a;
    this.b = b;
  }
}

Unlike the noncompliant code examples, if a and b currently have the value 0, and one thread calls getSum() while another calls setValues(1, 1), getSum() may return 0 or 2, depending on which thread obtains the intrinsic lock first. The locking strategy guarantees that getSum() never returns the unacceptable value 1.

This compliant solution also ensures that there is no TOCTOU condition between checking for overflow and adding the fields.

Risk Assessment

If operations on shared variables are not atomic, unexpected results can be produced. For example, information can be disclosed inadvertently because one user can receive information about other users.

Automated Detection

The following table summarizes the examples flagged as violations by SureLogic Flashlight:

The following table summarizes the examples flagged as violations by SureLogic JSure:

Details

Dynamic analysis tools with a Java concurrency focus, such as SureLogic Flashlight and Coverity Dynamic Analysis will uncover the race conditions shown in the noncompliant code examples above. To do that however, those tools must observe the noncompliant code being called by two or more threads, such as in an integration or stress-test environment. The tools use a dynamic lockset analysis to observe race conditions that occur as the program runs. This analysis intersects the set of locks observed when each piece of shared state in the program is accessed. If the lockset for a piece of shared state is empty, the tool may have observed a race condition and reports that to the user.

Heuristics-based static analysis tools, such as FindBugs and PMD, do not detect problems with the noncompliant code examples shown above without some "hint" that the program code is intended to be thread-safe. For example, consider the compliant code below where the use of a synchronized method is a hint to the analysis tool that the class is meant to be used concurrently.

...

public class Foo {
  private boolean flag = true;

  public synchronized boolean toggleAndGet() {
    flag ^= true; // Same as flag = !flag;
    return flag;
  }
}

FindBugs and PMD will not report a warning about this implementation because they do not note any problems.

SureLogic JSure, an analysis-based verification tool, will complain that the lock is unknown to the tool and ask the user to annotate what state the lock protects. In other words, the tool wants to know the locking policy that the programmer intends for this class. To express this intent, the programmer adds two annotations:

...

@RegionLock("FlagLock is this protects flag")
@Promise("@Unique(return) for new()")
public class Foo {
  private boolean flag = true;

  public synchronized boolean toggleAndGet() {
    flag ^= true; // Same as flag = !flag;
    return flag;
  }
}

Risk Assessment

If operations on shared variables are not atomic, unexpected results can be produced. For example, information can be disclosed inadvertently because one user can receive information about other users.

Automated Detection

The SureLogic Flashlight tool can diagnose violations of this guideline as instance fields with empty locksets

The @RegionLock annotation creates a locking policy, named FlagLock, that specifies that reads and writes to the flag field are to be guarded by a lock on the receiver, that is, this. The second annotation, @Promise is used to place an annotation on the default constructor generated by the compiler. The @Unique("return") annotation promises that the receiver is not aliased during object construction, that is, that a race condition cannot occur during construction. (CON14-J. Do not let the "this" reference escape during object construction provides further details.) If the constructor was explicit in the code, the annotations would be as follows:

...

@RegionLock("FlagLock is this protects flag")
public class Foo {
  private boolean flag;

  @Unique("return")
  public Foo() {
    flag = true;
  }

  public synchronized boolean toggleAndGet() {
    flag ^= true; // Same as flag = !flag;
    return flag;
  }
}

The JSure verification tool provides a strong assurance that the annotated model holds for all possible executions of the program. If the following noncompliant code is later added to the class, JSure will report the violation of the locking policy to the user.

...

  public boolean getValue() {
    return flag;
  }

If the noncompliant getValue() method shown above is defined in the code for Foo, FindBugs can also report a problem, again if the locking model is annotated. However, it uses a different annotation than JSure:

...

public class Foo {
  @GuardedBy("this")
  private boolean flag = true;

  public synchronized boolean toggleAndGet() {
    flag ^= true; // Same as flag = !flag;
    return flag;
  }

  public boolean getValue() {
    return flag;
  }
}

With the @GuardedBy annotation in place, and only with this annotation in place, FindBugs reports that the field is not guarded against concurrent access in the getValue() method.

Related Vulnerabilities

Any vulnerabilities resulting from the violation of this rule are listed on the CERT website.

...