Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-3845

atomicity violation bugs because of misusing concurrent collections

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.4
    • Fix Version/s: 2.3.12
    • Component/s: None
    • Labels:
    • Flags:
      Patch

      Description

      My name is Yu Lin. I'm a Ph.D. student in the CS department at
      UIUC. I'm currently doing research on mining Java concurrent library
      misusages. I found some misusages of ConcurrentHashMap in Struts
      2.3.4, which may result in potential atomicity violation bugs or harm
      the performance.

      The code below is a snapshot of the code in file
      core/src/main/java/org/apache/struts2/components/UIBean.java from line
      1227 to 1244

      L1227 Set<String> standardAttributes = standardAttributesMap.get(clz);
      L1228 if (standardAttributes == null)

      { L1229 standardAttributes = new HashSet<String>(); L1230 standardAttributesMap.put(clz, standardAttributes); ... L1244 }

      In the code above, an atomicity violation may occur between lines
      <1228 and 1230>. Suppose a thread T1 executes line 1227 and finds out
      the concurrent hashmap "standardAttributes" dose not contain the key
      "clz". Before it gets to execute line 1230, another thread T2 puts a
      pair <clz, v> in the concurrent hashmap "standardAttributes". Now
      thread T1 resumes execution and it will overwrite the value written by
      thread T2. Thus, the code no longer preserves the "put-if-absent"
      semantics.

      I found some similar misusages in other files:

      In
      xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java,
      there are similar atomicity violations as the above code at lines 242
      and 415. Note that if we change "beanInfoCache.put" to
      "beanInfoCache.putIfAbsent" at line 415, the synchronized key word on
      "beanInfoCache" can be removed so that the performance can be improved.

      Similarly, in
      xwork-core/src/main/java/com/opensymphony/xwork2/util/LocalizedTextUtil.java,
      atomicity violations may occur at line 254, 263 and 707 (thread T2 may
      put a pair <key, value> into the map before thread T1 puts). Second,
      before thread T1 executes line 257 or 266, thread T2 may remove the
      key "key" from concurrent hashmap "bundlesMap". Thus, after T1 resumes
      execution, it will get null value at line 257 or 266, which is also an
      atomicity violation.

      1. struts-2.3.4.patch
        7 kB
        Yu Lin
      2. UIBean.patch
        1.0 kB
        zhouyanming

        Activity

        Hide
        jacklondongood Yu Lin added a comment -

        This is a patch that fixes the atomicity violation problem.

        Show
        jacklondongood Yu Lin added a comment - This is a patch that fixes the atomicity violation problem.
        Hide
        quaff zhouyanming added a comment -

        agreed,although standardAttributes of UIBean doesn't need atomicity,I design it just as a cache.

        Show
        quaff zhouyanming added a comment - agreed,although standardAttributes of UIBean doesn't need atomicity,I design it just as a cache.
        Hide
        quaff zhouyanming added a comment -

        your patch of UIBean have two problems
        1.It will always new HashSet<String>()
        2.It's possible to get a empty HashSet<String>()

        Show
        quaff zhouyanming added a comment - your patch of UIBean have two problems 1.It will always new HashSet<String>() 2.It's possible to get a empty HashSet<String>()
        Hide
        quaff zhouyanming added a comment - - edited

        UIBean.patch can avoid getStandardAttributes() return a empty HashSet

        Show
        quaff zhouyanming added a comment - - edited UIBean.patch can avoid getStandardAttributes() return a empty HashSet
        Hide
        jacklondongood Yu Lin added a comment -

        Unfortunately, your patch still has atomicity violation. Whoever is the last thread to put pair <clz, standardAttributes> will overwrite the value put there by the previous thread. This destroys the put-if-absent semantics of the original code (You may read my previous explanation in the bug description).

        Show
        jacklondongood Yu Lin added a comment - Unfortunately, your patch still has atomicity violation. Whoever is the last thread to put pair <clz, standardAttributes> will overwrite the value put there by the previous thread. This destroys the put-if-absent semantics of the original code (You may read my previous explanation in the bug description).
        Hide
        quaff zhouyanming added a comment -

        atomicity is not mandatory here,it's used as a cache

        Show
        quaff zhouyanming added a comment - atomicity is not mandatory here,it's used as a cache
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Implemented, thanks for reporting

        Show
        lukaszlenart Lukasz Lenart added a comment - Implemented, thanks for reporting
        Hide
        hudson Hudson added a comment -

        Integrated in Struts2-JDK6 #626 (See https://builds.apache.org/job/Struts2-JDK6/626/)
        WW-3845 improves code to avoid empty HashSet (Revision 1436940)

        Result = SUCCESS
        lukaszlenart :
        Files :

        • /struts/struts2/trunk/core/src/main/java/org/apache/struts2/components/UIBean.java
        Show
        hudson Hudson added a comment - Integrated in Struts2-JDK6 #626 (See https://builds.apache.org/job/Struts2-JDK6/626/ ) WW-3845 improves code to avoid empty HashSet (Revision 1436940) Result = SUCCESS lukaszlenart : Files : /struts/struts2/trunk/core/src/main/java/org/apache/struts2/components/UIBean.java
        Hide
        hudson Hudson added a comment -

        Integrated in Struts2-JDK6 #627 (See https://builds.apache.org/job/Struts2-JDK6/627/)
        WW-3845 adds usage of putIfAbsent to improve atomicity (Revision 1436951)

        Result = SUCCESS
        lukaszlenart :
        Files :

        • /struts/struts2/trunk/core/src/main/java/org/apache/struts2/components/UIBean.java
        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java
        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/LocalizedTextUtil.java
        Show
        hudson Hudson added a comment - Integrated in Struts2-JDK6 #627 (See https://builds.apache.org/job/Struts2-JDK6/627/ ) WW-3845 adds usage of putIfAbsent to improve atomicity (Revision 1436951) Result = SUCCESS lukaszlenart : Files : /struts/struts2/trunk/core/src/main/java/org/apache/struts2/components/UIBean.java /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/util/LocalizedTextUtil.java

          People

          • Assignee:
            lukaszlenart Lukasz Lenart
            Reporter:
            jacklondongood Yu Lin
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 504h
              504h
              Remaining:
              Remaining Estimate - 504h
              504h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development