Page History

The {{java.util.Collections}} interface's documentation \[[API 06|AA. Java References#API 06]\] warns about the consequences of synchronizing on any view over a collection object, rather than synchronizing on the collection object itself.
{quote}
It is imperative that the user manually synchronize on the returned map when iterating over any of its collection views...  Failure to follow this advice may result in non-deterministic behavior. 
{quote}

Synchronize on the original Collection object when using synchronization wrappers to enforce atomicity ([CON07-J. Do not assume that a group of calls to independently atomic methods is atomic]).

{mc} This also applies to {{java.util.Map}} objects, even though Maps are technically not Collections. -- Good point but I think you mean the Collections interface. Acc to interface map API: "This interface is a member of the Java Collections Framework". Moreover it has collection views as stated in the API ref.  -- No, I mean the Collection interface. Collections is a class with several static methods.  Maps are part of the Collection framework even though they don't implement {{Collection}}. Hence I removed bracees ~DS {mc}

h2. Noncompliant Code Example (collection view)

This noncompliant code example incorrectly synchronizes on the {{set}} view of the synchronized map ({{map}}) instead of the collection object \[[Tutorials 08|AA. Java References#Tutorials 08]\].

{code:bgColor=#FFcccc}
// map has package-private accessibility
final Map<Integer, String> map = Collections.synchronizedMap(new HashMap<Integer, String>());
private final Set<Integer> set = map.keySet();

public void doSomething() {
  synchronized(set) {  // Incorrectly synchronizes on set
    for (Integer k : set) { 
      // ...
    }
  }
}
{code}
 

h2. Compliant Solution (collection lock object)

This compliant solution synchronizes on the original {{Collection}} object {{map}} instead of the {{Collection}} view {{set}}. 

{code:bgColor=#ccccff}
// map has package-private accessibility
final Map<Integer, String> map = Collections.synchronizedMap(new HashMap<Integer, String>());
private final Set<Integer> set = map.keySet();

public void doSomething() {
  synchronized(map) {  // Synchronize on map, not set
    for (Integer k : set) { 
      // ...
    }
  }
}
{code}

h2. Risk Assessment

Synchronizing on a collection view instead of the collection object may cause non-deterministic behavior. 

|| Rule || Severity || Likelihood || Remediation Cost || Priority || Level ||
| CON40-J | medium | probable | medium | {color:#cc9900}{*}P8{*}{color} | {color:#cc9900}{*}L2{*}{color} |


h3. Related Vulnerabilities

Search for vulnerabilities resulting from the violation of this rule on the [CERT website|https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+CON36-J].

h2. References

\[[API 06|AA. Java References#API 06]\] Class Collections
\[[Tutorials 08|AA. Java References#Tutorials 08]\] [Wrapper Implementations|http://java.sun.com/docs/books/tutorial/collections/implementations/wrapper.html]

h2. Issue Tracking

{tasklist:Review List}
||Completed||Priority||Locked||CreatedDate||CompletedDate||Assignee||Name||
|F|M|F|1269453074651|          |dmohindr|"warns about the consequences of synchronizing on any view over a collection object, rather than synchronizing on the collection object" ... it doesn't warn against it...it just says you should synchronize the collection...suggest reverting the change|
|FT|M|F|1269452979738|          |dmohindr1269635061216|rcs|The title could do away with "still"|
{tasklist}



----
[!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_left.png!|VOID CON00-J. Synchronize access to shared mutable variables]&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;[!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_up.png!|11. Concurrency (CON)]&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;[!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_right.png!|CON03-J. Do not use background threads during class initialization]