Java Developer Tools

Audit - Rules - Threads and Synchronization

Description
This group contains audit rules that look for possible problems having to do with the use of threads and synchronization.

Rules:

Details

Call Lock Without Unlock

Summary
Invoke unlock() method in catch clause or finally block.

Description
This audit rule looks for places where the lock() method is invoked without guaranteeing that the unlock() method will be invoked if an exception is thrown.

Security Implications
Such a mistake can cause a deadlock in the case of an exception. Deadlocks are a common way of depleting a web application's pool of threads and thus permitting a denial-of-service attack.

Example
The following invocation of the lock() method will be flagged as a violation because the unlock() method is not invoked if an exception is thrown:

    public void myMethod()
    {
            .......
            Lock l = new Lock();
            l.lock();
            try{
                    iter.next();
            }catch(Exception e)    {
            }
        l.unlock();
    }

Concurrent Modification

Summary
Removing from the collection being iterated over and continuing iteration afterwards results in an exception.

Description
This audit rule violates the cases when elements are removed from a collection while still iterating over it. Precisely, calling Iterator#next() after remove() was called results in an exception. Correct way of dealing with a necessity to do so is to iterate over the duplicate of a collection while removing found objects from the original or to use the iterators remove() method if it is known to be supported.

Security Implications
For fail-fast collections concurrent modifications result in ConcurrentModificationException, which could create an unexpected situation in the code execution and thus be a potential security threat.

Example
The following method removes elements from a collection while iterating over it and thus would be marked as violation:

    public void removeByPattern(Collection c, String pattern) {
        for (Iterator i = c.iterator(); i.hasNext();) {
            String val = (String) i.next();
            if (val.contains(pattern)) {
                c.remove(val);
            }
        }
    }

Disallow Notify Usage

Summary
The notify() method should not be used.

Description
The method notifyAll() should be used rather than notify() because it will generally lead to a better scheduling of waiting threads.

Example
The following invocation of the notify() method would be flagged as a violation:

    synchronize (monitor) {
        monitor.notify();
    }

Disallow Sleep Inside While

Summary
The sleep() method should not be used within a while loop.

Description
This audit rule looks for invocations of the sleep() method that occur inside of a while loop. Such occurrences usually indicate that the code implements a busy-wait loop, which is inefficient. Instead, the code should use wait() and notify() to block the thread until it is possible for execution to proceed.

Example
The following invocation of the sleep() method would be flagged as a violation:

    while (eventQueue.isEmpty()) {
        Thread.sleep();
    }

Disallow Sleep Usage

Summary
The sleep() method should not be used.

Description
This audit rule looks for invocations of the method Thread.sleep(). If you are waiting for some specific state to be reached, you should instead use the wait() method so that you can be notified when the state has been reached. This will improve both the performance and reliability of your code.

Example
The following invocation of the sleep() method would be flagged as a violation:

    sleep(1000);

Disallow ThreadGroup Usage

Summary
The class ThreadGroup should not be used.

Description
The class ThreadGroup should not be used in multi-threaded applications because its implementation is not thread safe.

Example
The following instance creation would be flagged as a violation because it is creating an instance of the class java.lang.ThreadGroup:

    new ThreadGroup("Background Threads")

Disallow Unnamed Thread Usage

Summary
Threads should be named to aid in debugging.

Description
This audit rule looks for the creation of threads that are not given a name. Threads should be named in order to make it easier to debug the application.

Example
The following thread creation would be flagged as a violation because a name is not provided:

    new Thread(new Runnable() { ... });

Disallow Use of Deprecated Thread Methods

Summary
Don't use deprecated methods when writing multi-threaded code.

Description
The methods Thread.resume(), Thread.stop(), Thread.suspend(), Thread.destroy() and Runtime.runFinalizersOnExit() have been deprecated and should not be used because they are inherently unsafe.

Example
The following use of the suspend() method would be flagged as a violation:

    processingThread.suspend();

Disallow Yield Usage

Summary
The method Thread.yield() should not be used.

Description
The method Thread.yield() should not be used because its behavior is not consistent across all platforms.

Example
The following invocation of the yield() method would be flagged as a violation:

    backgroundTask.yield();

Do not Catch IllegalMonitorStateException

Summary
Catching an IllegalMonitorStateException indicates a concurrency handling error in debug.

Description
This audit rule looks for code that attempts to catch IllegalMonitorStateException.

Security Implications
Catching an IllegalMonitorStateException indicates a concurrency handing flaw in the design of an application which is only partially mitigated, but not avoided. This design flaw could be exploited to bring an application to a denial of service state or even provide security-sensitive data, such as parts of an application design, to the attacker.

Example
The following code would be flagged as a violation because it catches IllegalMonitorStateException:

    public void myMethod() {
        try {
            //...
        } catch (IllegalMonitorStateException e ) {
            //...
        }
    }

Double Check Locking

Summary
Double Check Locking singleton synchronization technique should not be used.

Description
Double-checked locking is a widespread practice used in Singleton pattern implementations. It is supposed to provide a safe way for the lazy initialization of the singleton in a multi-threaded environment combined with performance optimization. But in some cases this code will not work as intended, leading to concurrency problems and unpredictable behavior. This pattern should not be used.

Security Implications
in some cases double-checked locking will not work as intended, leading to concurrency problems and unpredictable behavior.

Example
The following code would be flagged as a violation because it uses the double-checked locking pattern:

    public class Sample {
        private List data;
        public List getData() {
            if (data == null) {
                synchronized (this) {
                    if (data == null) {
                        data = new ArrayList();
                    }
                }
            }
            return data;
        }
    }

Field Access Protection

Summary
When a field that is usually accessed in a synchronized context is accessed without synchronization, this indicates an error.

Description
This audit rule looks for unsynchronized accesses to fields that are usually accessed in a synchronized way. A field is considered to match the criteria if synchronization is used to access it in at least 66% of all of the places in which it is accessed.

Security Implications
Partial or incomplete synchronization is the primary source of all deadlocks and race conditions. Multi-threaded parts of data should always be accessed in a synchronized block.

Example
The following class appears to provide synchronized access to the internal list, but remove() method is not synchronized and thus will be marked as violation:

    public class SynchronizedList {
        private final List data = new ArrayList();
        public synchronized void add(Object item) {
            data.add(item);
        }
        public synchronized int size() {
            return data.size();
        }
        public synchronized Object pop() {
            return data.remove(0);
        }
        public void remove(Object item) {
            data.remove(item);
        }
    }

Improper Use of Thread.interrupted()

Summary
The static method Thread.interrupted() can be easily confused with the instance method isInterrupted().

Description
Since interrupted() is a static method which returns whether the current thread is interrupted, it should always be invoked using Thread.interrupted(). This audit rule looks for any invocations of interrupted() on instances of Thread.instead.

Example
The following would be flagged as a violation:

    new Thread().interrupted()

Mismatched Notify

Summary
Do not use notify() or notifyAll() methods without holding locks.

Description
This audit rule looks for invocations of notify() or notifyAll() methods without holding a lock on the object.

Security Implications
Invoking notify() or notifyAll() without holding a lock can lead to throwing an IllegalMonitorStateException.

Example
The following invocation of the notify() method will be marked as a violation because the container method is not synchronized:

    public void method() {
            notify();
    }

Mismatched Wait

Summary
Do not use wait() method without holding locks.

Description
This audit rule looks for invocations of the wait() method without holding a lock on the object.

Security Implications
Invoking wait() without holding a lock can lead to throwing an IllegalMonitorStateException.

Example
The following invocation of the wait method will be marked as a violation because the container method is not synchronized:

    public void method(){
            wait();
    }

Nested Synchronized Calls

Summary
Invoking one synchronized method of an object from another synchronized method of the same object affects the performance of an application.

Description
This audit rule looks for invocations of a synchronized method from another synchronized method in the same class.

Security Implications
Such calls both affect the performance of an application and indicate a poorly designed synchronization aspect of the code, which usually results in synchronization errors that could be exploited to create unexpected states of an application.

Example
The following code would be flagged as a violation because it invokes one synchronized method of an object from another synchronized method of the same object:

    public class SyncDataSource {
        public synchronized Object getData() {
            return internalGetData();
        }
        private synchronized Object internalGetData() {
            ...
        }
    }

No Run Method

Summary
Subclasses of Thread should implement the run() method.

Description
Subclasses of Thread should implement the run() method so that they will have the behavior for which they were created.

Example
The following class will be flagged as a violation because it does not implement a run() method even though it subclasses java.lang.Thread:

    public class SuperThread extends Thread
    {
    }

Non-atomic File Operations

Summary
Checking a file for existence and then writing into it is a non-atomic operation, the assumption about it being atomic may fail, leading to unexpected consequences.

Description
This audit rule looks for conditional decisions made on the basis of an invocation of java.io.File.exists().

Security Implications
Calling exists() and then performing some file operation based on the result of this call is a non-atomic operation, i.e. the file state of existence can change in the gap of time between exists() call and the subsequent actions. This may result in an unexpected behavior of an application that could possibly compromise its security.

Example
The following code would be flagged as a violation because it makes a file writing decision based on the result of exists() call:

    File lock = new File(".lock");
    if (!lock.exists()) {
        lock.createNewFile();
        ...
        lock.delete();
    }

Notify method invoked while two locks are held

Summary
Do not invoke the notify or notifyAll methods while two locks are being held.

Description
This rule looks for places where the code invokes either notify() or notifyAll() while two locks are held.

Security Implications
If this notification is intended to wake up a wait() that is holding the same locks, it might deadlock. The wait will only give up one lock and the notify will be unable to get both locks, and thus the notify will not succeed.

Example
The following invocation of the notify method will be flagged as a violation because two lock are being held.

    public synchronized void myMethod(Object obj)
    {
            synchronize (obj) {
                    obj.notify();
            }
    }

Overriding a Synchronized Method with a Non-synchronized Method

Summary
A non-synchronized method should not override a synchronized method.

Description
This audit rule finds non-synchronized methods that override a synchronized method. Such a case usually indicates thread-unsafe code.

Example
    public class Widget
    {
        public synchronized int getPartCount()
        {
            return 0;
        }
    }

The method getPartCount() would be flagged as a violation in the following class:

    public class CompositeWidget extends Widget
    {
        public int getPartCount()
        {
            return children.size();
        }
    }

Sleep method invoked in synchronized code

Summary
Do not invoke the sleep method while a lock is held.

Description
This rule looks for places where the code invokes the method sleep(long) while a lock is held.

Security Implications
An invocation of the sleep method while a lock is being held could lead to very poor performance or deadlock. Use the wait method instead of the sleep method.

Example
The following invocation of the sleep() method will be flagged as a violation because a lock is being held.

    public synchronized void myMethod(Object obj)
    {
            while(true){
                    Thread.sleep(1000);
            }
    }

Start Method Invoked In Constructor

Summary
Do not invoke java.lang.Thread.start() method inside constructors.

Description
This audit rule looks for invocations of java.lang.Thread.start() method inside constructors.

Security Implications
If the class is extended/subclassed, the method start() will be invoked before the constructor of the subclass has finished, potentially allowing the new thread to see the object in an inconsistent state.

Example
The following invocation of the start method will be marked as a violation because it is called inside the constructor:

    public class SimleClass {
        public SimleClass() {
            new Thread() {
                public void run(){
                }
            }.start();
        }
    }

Synchronization On getClass Method

Summary
You should avoid synchronization on getClass() method.

Description
This rule looks for places where the result of the getClass() method is used for synchronization.

Security Implications
If this class is subclassed, subclasses will synchronize on a different class object, which isn't likely what was intended.

Example
The following code is synchronized on getClass method and thuswill be marked as a violation:

    public void methodOne(){
        synchronized ( getClass() ){
        }
    }

Synchronization On Non Final Fields

Summary
All fields used for synchronization of threads should be declared final.

Description
This rule looks for places where blocks are synchronized on non-final fields.

Security Implications
If an attacker gets access to the field which is used for synchronization, he could modify it and create an error in synchronization logic. Such vulnerability could result in an application misbehaviour or crash or access to otherwise protected data.

Example
The following code will be flagged as a violation because the field that is used for synchronization is not final.

    public class Test {
        Object lock;
        SOUF(Object lock) {
        this.lock = lock;
        }
        public void myMethod() {
            synchronized(lock) {
                ...
            }
        }
    }

Synchronized Method

Summary
Methods should never be marked as synchronized.

Description
This audit rule reports the use of the synchronized modifier with methods. It is too easy to miss the existence of the synchronized modifier, so following this rule leads to more readable code.

Example
    public synchronized String getName()
    {
        ...
    }

Usage Of Static Calendar

Summary
Do not use Calendar objects without synchronization.

Description
This rule looks for a places where Calendar objects are used without synchronization.

Security Implications
Calendars are inherently unsafe for multithread usage. If this object is used without proper synchronizations, ArrayIndexOutOfBoundsException can be thrown.

Example
The following usage of a Calendar object will be marked as a violation because it should be used in synchronized block:

    private static Calendar data;
    ...
    public Calendar getCalendar() {
        return data;
    }

Usage Of Static Date Format

Summary
Do not use DateFormat objects without synchronization.

Description
This rule looks for places where a DateFormat object is used without synchronization.

Security Implications
DateFormats are inherently unsafe for multithread usage. Sharing a single instance across thread boundaries without proper synchronization can result in erratic behavior of the application.

Example
The following usage of a DateFormat object will be marked as a violation because it should be used in a synchronized block:

    private static DateFormat data;
    ...
    public DateFormat getFormat() {
        return data;
    }

Use Start Rather Than Run

Summary
Threads should be started rather than run.

Description
This audit rule looks for places where a thread is run using the run() method, rather than started using the start() method. This is usually a mistake since it causes the run() method to be run in the calling thread rather than in a newly spawned thread.

Example
The following invocation would be flagged:

    thread.run();

Use Thread-safe Lazy Initialization

Summary
Static fields should be initialized in a thread safe way.

Description
Static fields are typically initialized either as part of their declaration, in a static initializer, or lazily in a static method. The first two ways are thread safe because of the way the JVM initializes classes. In order for the initialization of a lazily initialized field (such as the unique instance of a Singleton class) to be thread safe, either the method must be synchronized or the body of the method must be inside a synchronize statement. If not, and if the method is called by multiple threads, one of the threads might get a reference to the field before it is fully initialized or it might be initialized multiple times. This audit rule looks for places where a static field is initialized in a way that is not thread safe.

Example
The following singleton is not thread-safe, and would be marked as a violation.

    public class MySingleton {
        public MySingleton instance = null;
        
        public MySingleton getInstance() {
            if (instance == null) {
                instance = new MySingleton();
            }
            return instance;
        }
        
        private MySingleton() {}
    }

Wait Inside While

Summary
The wait() method should only be invoked within a while loop.

Description
This audit rule looks for invocations of the wait() method that occur outside of a while loop.

Example
The following invocation of the wait() method would be flagged as a violation:

    public void waitForEvent()
    {
        synchronize (eventQueue) {
            eventQueue.wait();
            ...
        }
    }

Wait method invoked while two locks are held

Summary
Do not invoke the wait method while two locks are being held.

Description
This rule looks for places where the code invokes the wait() method while two locks are held.

Security Implications
Situation when some threads wait on monitor while two locks are held can lead to deadlock.

Example
The following invocation of the wait method will be flagged as a violation because two lock are being held.

    public synchronized void myMethod(Object obj)
    {
            synchronize (obj) {
                    obj.wait();
            }
    }

wait() Invoked Instead of await()

Authentication required

You need to be signed in with Google+ to do that.

Signing you in...

Google Developers needs your permission to do that.