...
This noncompliant code example wraps references to BigInteger objects within thread-safe AtomicReference objects.
| Code Block | ||
|---|---|---|
| ||
final class Adder {
private final AtomicReference<BigInteger> first;
private final AtomicReference<BigInteger> second;
public Adder(BigInteger f, BigInteger s) {
first = new AtomicReference<BigInteger>(f);
second = new AtomicReference<BigInteger>(s);
}
public void update(BigInteger f, BigInteger s) { // Unsafe
first.set(f);
second.set(s);
}
public BigInteger add() { // Unsafe
return first.get().add(second.get());
}
}
|
...
This compliant solution declares the update() and add() methods synchronized to guarantee atomicity.
| Code Block | ||
|---|---|---|
| ||
final class Adder {
// ...
private final AtomicReference<BigInteger> first;
private final AtomicReference<BigInteger> second;
public Adder(BigInteger f, BigInteger s) {
first = new AtomicReference<BigInteger>(f);
second = new AtomicReference<BigInteger>(s);
}
public synchronized void update(BigInteger f, BigInteger s){
first.set(f);
second.set(s);
}
public synchronized BigInteger add() {
return first.get().add(second.get());
}
}
|
...
This noncompliant code example uses a java.util.ArrayList<E> collection, which is not thread-safe. However, the example uses Collections.synchronizedList as a synchronization wrapper for the ArrayList. It subsequently uses an array, rather than an iterator, to iterate over the ArrayList to avoid a ConcurrentModificationException.
| Code Block | ||
|---|---|---|
| ||
final class IPHolder {
private final List<InetAddress> ips =
Collections.synchronizedList(new ArrayList<InetAddress>());
public void addAndPrintIPAddresses(InetAddress address) {
ips.add(address);
InetAddress[] addressCopy =
(InetAddress[]) ips.toArray(new InetAddress[0]);
// Iterate through array addressCopy ...
}
}
|
...
The race condition can be eliminated by synchronizing on the underlying list's lock. This compliant solution encapsulates all references to the array list within synchronized blocks.
| Code Block | ||
|---|---|---|
| ||
final class IPHolder {
private final List<InetAddress> ips =
Collections.synchronizedList(new ArrayList<InetAddress>());
public void addAndPrintIPAddresses(InetAddress address) {
synchronized (ips) {
ips.add(address);
InetAddress[] addressCopy =
(InetAddress[]) ips.toArray(new InetAddress[0]);
// Iterate through array addressCopy ...
}
}
}
|
...
This noncompliant code example defines the KeyedCounter class that is not thread-safe. Although the HashMap is wrapped in a synchronizedMap(), the overall increment operation is not atomic [Lee 2009].
| Code Block | ||
|---|---|---|
| ||
final class KeyedCounter {
private final Map<String, Integer> map =
Collections.synchronizedMap(new HashMap<String, Integer>());
public void increment(String key) {
Integer old = map.get(key);
int oldValue = (old == null) ? 0 : old.intValue();
if (oldValue == Integer.MAX_VALUE) {
throw new ArithmeticException("Out of range");
}
map.put( key, oldValue + 1);
}
public Integer getCount(String key) {
return map.get(key);
}
}
|
...
This compliant solution ensures atomicity by using an internal private lock object to synchronize the statements of the increment() and getCount() methods.
| Code Block | ||
|---|---|---|
| ||
final class KeyedCounter {
private final Map<String, Integer> map =
new HashMap<String, Integer>();
private final Object lock = new Object();
public void increment(String key) {
synchronized (lock) {
Integer old = map.get(key);
int oldValue = (old == null) ? 0 : old.intValue();
if (oldValue == Integer.MAX_VALUE) {
throw new ArithmeticException("Out of range");
}
map.put(key, oldValue + 1);
}
}
public Integer getCount(String key) {
synchronized (lock) {
return map.get(key);
}
}
}
|
...
Note that this compliant solution still requires synchronization, because without it, the test to prevent overflow and the increment will not happen atomically, so two threads calling increment() can still cause overflow. The synchronization block is smaller, and does not include the lookup or addition of new values, so it has less of an impact on performance as the previous compliant solution.
| Code Block | ||
|---|---|---|
| ||
final class KeyedCounter {
private final ConcurrentMap<String, AtomicInteger> map =
new ConcurrentHashMap<String, AtomicInteger>();
private final Object lock = new Object();
public void increment(String key) {
AtomicInteger value = new AtomicInteger();
AtomicInteger old = map.putIfAbsent(key, value);
if (old != null) {
value = old;
}
synchronized (lock) {
if (value.get() == Integer.MAX_VALUE) {
throw new ArithmeticException("Out of range");
}
value.incrementAndGet(); // Increment the value atomically
}
}
public Integer getCount(String key) {
AtomicInteger value = map.get(key);
return (value == null) ? null : value.get();
}
// Other accessors ...
}
|
...
Rule | Severity | Likelihood | Remediation Cost | Priority | Level |
|---|---|---|---|---|---|
VNA03-J | low | probable | medium | P4 | L3 |
Automated Detection
Some static analysis tools are capable of detecting violations of this rule.
| Tool | Version | Description | ||||||
|---|---|---|---|---|---|---|---|---|
| ThreadSafe |
| Implemented |
Related Guidelines
CWE-362. Concurrent execution using shared resource with improper synchronization ("race condition") | |
| CWE-366. Race condition within a thread |
| CWE-662. Improper synchronization |
...
[API 2006] |
|
Section 4.4.1, Client-side Locking | |
| Section 5.2.1, ConcurrentHashMap |
Section 8.2, Synchronization and Collection Classes | |
[Lee 2009] | Map & Compound Operation |