SEI CERT Oracle Coding Standard for Java

Space shortcuts

Page tree

Methods that can both modify a static field and be invoked from untrusted code must synchronize access to the static field. Even when client-side locking is a specified requirement of the method, untrusted clients can fail to synchronize (whether inadvertently or maliciously). Because the static field is shared by all clients, untrusted clients may violate the contract by failing to provide suitable locking.

According to Joshua Bloch [Bloch 2008]:

If a method modifies a static field, you must synchronize access to this field, even if the method is typically used only by a single thread. It is not possible for clients to perform external synchronization on such a method because there can be no guarantee that unrelated clients will do likewise.

Documented design intent is irrelevant when dealing with untrusted code because an attacker can always choose to ignore the documentation.

Noncompliant Code Example

This noncompliant code example fails to synchronize access to the static counter field:

/* This class is not thread-safe */
public final class CountHits {
  private static int counter;

  public void incrementCounter() {
    counter++;
  }
}

This class definition complies with VNA02-J. Ensure that compound operations on shared variables are atomic, which applies only to classes that promise thread-safety. However, this class has a mutable static counter field that is modified by the publicly accessible incrementCounter() method. Consequently, this class cannot be used securely by trusted client code because untrusted code can purposely fail to externally synchronize access to the field.

Compliant Solution

This compliant solution uses a static private final lock to protect the counter field and consequently lacks any dependence on external synchronization. This solution also complies with LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code.

/* This class is thread-safe */
public final class CountHits {
  private static int counter;
  private static final Object lock = new Object();

  public void incrementCounter() {
    synchronized (lock) {
      counter++;
    }
  }
}

Risk Assessment

Failure to internally synchronize access to static fields that can be modified by untrusted code risks incorrect synchronization because the author of the untrusted code can inadvertently or maliciously ignore the synchronization policy.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

LCK05-J

Low

Probable

Medium

P4

L3

Automated Detection

ToolVersionCheckerDescription
CodeSonar
8.1p0

JAVA.CONCURRENCY.UG.METH

Unguarded Method (Java)

Parasoft Jtest
2023.1
CERT.LCK05.IASFInspect accesses to "static" fields which may require synchronization

Related Guidelines

MITRE CWE

CWE-820, Missing Synchronization

Bibliography

[API 2014]


[Bloch 2008]

Item 67, "Avoid Excessive Synchronization"


9 Comments

  1. David Svoboda

    I don't understand why this rule exists. There is nothing special about 'static' fields...any publicly-accessible fields needs locks, too. Which is pretty much covered by CON00-J and CON01-J.

    Other nits:

    • The last paragraph of the intro matches with the title, but the other intro paragraphs don't...why are they there?
    • The NCCE provides no documentation that it is thread-safe, contrary to its text.
    • Needs CON04-J reference,

  2. Dhruv Mohindra

    To supplement the previous collaborative edit I suggest:

    • protecting fields is a weak word. Should be changed to synchronizing access to fields.
    • and not because it documents its lack of thread-safety; suggest removing this
    • and specifies its lack of thread-safety in the documentation. => rephrase to : and documents its lack of thread-safety.
    • The title can also be Ensure classes containing publicly accessible mutable static fields are thread-safe
    • the CS class is not declared public and the formatting needs to be fixed (extra space)

    1. David Svoboda

      I fixed the last point, as it was a no-brainer. The other points are wordsmithing issues, so I'll let RCS & Pennie deal with them, as they'll do a better job >(wink)

      1. Dhruv Mohindra

        Ok, I've added an issue tracker.

  3. Robert Seacord (Manager)

    I labeled this guideline as "untrusted" to indicate it deals with issues related to integrating with untrusted code. Consequently, anything to do with documenting design intent is irrelevant in this context because an attacker can always chose to ignore the documentation.

    Consequently, I'm getting ready to remove the following text from this guideline:

    Protecting the fields of a class is unnecessary when it is designed for single-threaded use. Such classes are required to document their lack of thread-safety. For example, the Java Platform, Standard Edition 6 API Specification for the java.lang.StringBuilder class states API 06:

    This class is designed for use as a drop-in replacement for StringBuffer in places where the string buffer was being used by a single thread (as is generally the case). Where possible, it is recommended that this class be used in preference to StringBuffer as it will be faster under most implementations.

    Multithreaded clients must synchronize accesses to classes whose documentation explicitly specifies the class is not thread-safe or fails to provide any assurances that the class is safe-thread.

    1. Dhruv Mohindra

      Ok. It might be useful to state that it is insufficient incorrect to document that the class is not thread-safe.

      1. Dhruv Mohindra

        "[Such] A class is not allowed to document its lack of thread-safety by reasoning that it is intended only for single-threaded use or unsafe for multithreaded use."

  4. Pennie Walters

    The Automated Detection section is not complete. It's marked TODO.

    Also, should "static" be italics when we say "static fields?" or the "static counter field?"

  5. Thomas Hawtin

    Mostly mutable statics should be avoided. You can just about get away with it for caches, but even that is very easy to mess up.