You are viewing an old version of this page. View the current version.

Compare with Current View Page History

« Previous Version 11 Next »

Synchronizing on the return value of the Object.getClass() method, rather than a class literal can lead to unexpected behavior. Whenever the implementing class is subclassed, the subclass locks on the subclass's type, which is a completely different Class object.

Section 4.3.2 "The Class Object" of the Java Language specification [[JLS 05]] describes how method synchronization works:

A class method that is declared synchronized synchronizes on the lock associated with the Class object of the class.

This does not mean that a subclass using getClass() can only synchronize on the Class object of the base class. In fact, it will lock on its own Class object, which may or may not be what the programmer had in mind. The intent should be clearly documented or annotated. Note that if a subclass does not override an accessible noncompliant superclass's method, it inherits the method which may lead to the false conclusion that the superclass's intrinsic lock is available in the subclass.

When synchronizing on a class literal, the corresponding lock object should not be accessible to untrusted code. If the class is package-private, callers from other packages may not access the class object, ensuring its trustworthiness as an intrinsic lock object. For more information, see [CON04-J. Synchronize classes that may interact with untrusted code using a private final lock object].

Noncompliant Code Example (getClass() lock object)

In this noncompliant code example, the parse() method of class Base parses a date and synchronizes on the class object returned by getClass(). The class Derived also inherits the parse() method. However, this inherited method synchronizes on the class object of Derived because of the particular return value of getClass().

Derived also adds a method doSomethingAndParse() that locks on the class object of the base class because the developer misconstrued that the parse() method in Base always obtains a lock on the Base class object, and doSomethingAndParse() must follow the same locking policy. Consequently, the class Derived has two different locking strategies and is not thread-safe.

class Base {
  static DateFormat FORMAT =
    DateFormat.getDateInstance(DateFormat.MEDIUM);
	  
  public Date parse(String str) throws ParseException {
    synchronized (getClass()) {
      return FORMAT.parse(str);
    }
  }
}

class Derived extends Base {
  public Date doSomethingAndParse(String str) throws ParseException {	  
    synchronized(Base.class) {
      // ...
      return FORMAT.parse(str); 
    }
  }
}
Unknown macro: {mc}

// Hidden main() method to be put in class Derived
// Prints arbitrary date values and throws exceptions at times

public static void main(String[] args) {
for(int i = 0; i < 100; i++) {
new Thread(new Runnable() {
@Override public void run() {
try

Unknown macro: { System.out.println(new Derived().parse("Jan 1, 2010")); }

catch (ParseException e)

Unknown macro: { // Forward to handler }

}
}).start();

new Thread(new Runnable() {
@Override public void run() {
try

Unknown macro: { System.out.println(new Derived().doSomethingAndParse("Jan 2, 2010")); }

catch (ParseException e)

Unknown macro: { // Forward to handler }

}
}).start();
}
}

Compliant Solution (class name qualification)

Explicitly define the name of the class through name qualification (Base in this compliant solution) in the synchronized block.

class Base {
  static DateFormat FORMAT =
    DateFormat.getDateInstance(DateFormat.MEDIUM);
	  
  public Date parse(String str) throws ParseException {
    synchronized (Base.class) {
      return FORMAT.parse(str);
    }
  }
}

// ...

This code example always synchronizes on the Base.class object, even if it is called from a Derived object.

Compliant Solution (Class.forName())

This compliant solution uses the Class.forName() method to synchronize on the Base class's Class object.

class Base {
  static DateFormat FORMAT =
    DateFormat.getDateInstance(DateFormat.MEDIUM);
	  
  public Date parse(String str) throws ParseException {
    synchronized (Class.forName("Base")) {
      return FORMAT.parse(str);
    }
  }
}

// ...

Ensure that untrusted inputs are not accepted as arguments while loading classes using Class.forname(). (See [SEC05-J. Do not expose standard APIs that use the immediate caller's class loader instance to untrusted code] for more information.)

Noncompliant Code Example (getClass() lock object, inner class)

This noncompliant code example follows from the previous one in that it synchronizes on the class object returned by getClass() in the parse() method of class Base. It also uses a nested class Helper, whose doSomethingAndParse() method incorrectly synchronizes by invoking getClass().

class Base {
  static DateFormat FORMAT =
    DateFormat.getDateInstance(DateFormat.MEDIUM);
	  
  public Date parse(String str) throws ParseException {
    synchronized (getClass()) {
      return FORMAT.parse(str);
    }
  }

  public Date doSomething(String str) throws ParseException {
    return new Helper().doSomethingAndParse(str);
  }
  
  private class Helper {
    public Date doSomethingAndParse(String str) throws ParseException {	  
      synchronized(getClass()) { // Synchronizes on getClass()
        // ...
        return FORMAT.parse(str); 
      }
    }
  }
}

The call to getClass() in class Helper returns a Helper class object instead of the Base class object. Consequently, a thread that calls Base.parse() use a different lock than another that calls Base.doSomething().

Compliant Solution (class name qualification)

This compliant solution synchronizes using a Base class literal in the parse() and doSomethingAndParse() methods.

class Base {
  // ...

  public Date parse(String str) throws ParseException {
    synchronized (Base.class) {
      return FORMAT.parse(str);
    }
  }

  private class Helper {
    public Date doSomethingAndParse(String str) throws ParseException {	  
      synchronized(Base.class) { // Synchronizes on Base class literal
        // ...
        return FORMAT.parse(str); 
      }
    }
  }
}

Consequently, both Base and Helper lock on Base's intrinsic lock. Similarly, the Class.forname() method can be used instead of a class literal.

Risk Assessment

Synchronizing on the class object returned by getClass() can result in non-deterministic behavior.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

CON02- J

medium

probable

medium

P8

L2

Automated Detection

Unknown macro: {mc}

// Ignore
The following table summarizes the examples flagged as violations by FindBugs:

Noncompliant Code Example

Flagged

Checker

Message

getClass() lock object

No

WL_USING_GETCLASS_RATHER_THAN_CLASS_LITERAL

n/a

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

Noncompliant Code Example

Flagged

Message

getClass() lock object

No

No data available about field accesses

Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the CERT website.

References

[[API 06]]
[[Findbugs 08]].
[[Pugh 08]] "Synchronization"
[[Miller 09]] Locking


[!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_left.png!]      [!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_up.png!]      [!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_right.png!]

  • No labels