Uploaded image for project: 'CXF'
  1. CXF
  2. CXF-4404

atomicity violation bugs because of misusing concurrent collections

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.6.1
    • Fix Version/s: 2.6.2, 2.7.0
    • Component/s: None
    • Labels:
    • Estimated Complexity:
      Moderate

      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 CXF 2.6.1,
      which may result in potential atomicity violation bugs or harm the
      performance.

      The code below is a snapshot of the code in file
      api/src/main/java/org/apache/cxf/common/util/ASMHelper.java from line
      306 to 327.

      L306 public synchronized Class<?> defineClass(String name, byte bytes[]) {
      L307 Class<?> ret = defined.get(name.replace('/', '.'));
      L308 if (ret != null)

      { L309 return ret; L310 }

      L311 if (name.endsWith("package-info"))

      { ... L323 }

      L324
      L325 ret = super.defineClass(name.replace('/', '.'), bytes, 0, bytes.length);
      L326 defined.put(name.replace('/', '.'), ret);
      L327 return ret;

      In the code above, the synchronized key word at line 306 can be
      removed if we use putIfAbsent at line 326. If we remove the
      synchronized without using putIfAbsent, an atomicity violation may
      occur between lines <310 and 326>: Suppose a thread T1 executes line
      307 and finds out the concurrent hashmap dose not contain the key
      "name.replace('/','.')". Before it gets to execute line 326, another
      thread T2 puts a pair <name.replace('/','.'), v> in the concurrent
      hashmap "defined". 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. However, such atomicity
      violation bug can be eliminated by using putIfAbsent at line 326 (see
      the patch).

      I found some similar misusages in other files:

      In
      api/src/main/java/org/apache/cxf/configuration/spring/AbstractSpringBeanMap.java,
      synchronized key word at line 67 can be removed by using putIfAbsent
      at line 71. Also, atomicity violations may occur when T2 removes key
      "key" from the concurrent hashmap "putStore" before T1 executes line
      155; or T2 puts a pair <(X)key, v> to "putStore" before T1 executes
      line 158.

      In api/src/main/java/org/apache/cxf/endpoint/ClientImpl.java, an
      atomicity violation may occur when thread T2 removes key
      "THREAD_LOCAL_REQUEST_CONTEXT" from the concurrent hashmap
      "currentRequestContext" before thread T1 executes line 303.

      In api/src/main/java/org/apache/cxf/helpers/HttpHeaderHelper.java, an
      atomicity violation may occur when thread T2 puts a pair <enc, v> to
      concurrent hashmap "encodings" before T1 executes line 127.

      In
      rt/core/src/main/java/org/apache/cxf/bus/osgi/CXFExtensionBundleListener.java,
      an atomicity violation may occur when thread T2 puts a pair
      <bundle.getBundleId(), v> to concurrent hashmap "extensions" before T1
      executes line 93.

      In
      rt/features/clustering/src/main/java/org/apache/cxf/clustering/FailoverTargetSelector.java,
      synchronized key word at line 76 can be removed by using putIfAbsent
      at line 91.

      In
      rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/model/ClassResourceInfo.java,
      if another thread T2 puts a pair <key, v> into concurrent hashmap
      "subResources" before thread T1 executes line 136, the "cri" in T1
      won't be put into "subResources" at line 136. In this case, the
      "getSubResource" method returns the wrong "cri".

      In
      rt/rs/extensions/providers/src/main/java/org/apache/cxf/jaxrs/provider/json/JSONProvider.java,
      an atomicity violation may occur when thread T2 removes key
      "qname.getNamespaceURI()" from the concurrent hashmap "namespaceMap"
      before thread T1 executes line 405. Another atomicity violation is
      when thread T2 puts a pair <namespace, v> to concurrent hashmap
      "namespaceMap" before T1 executes line 530.

      In
      rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngineFactory.java,
      synchronized key word at line 107 can be removed by using putIfAbsent
      at line 118.

      In
      rt/transports/local/src/main/java/org/apache/cxf/transport/local/LocalTransportFactory.java,
      an atomicity violation may occur when thread T2 puts a pair <addr, v>
      to concurrent hashmap "destinations" before T1 executes line 128.

        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
        ffang Freeman Fang added a comment -

        Hi,

        Just one issue, the rt/core/src/main/java/org/apache/cxf/bus/osgi/CXFExtensionBundleListener.java change should be

        @@ -90,7 +90,10 @@
                 List<Extension> list = extensions.get(bundle.getBundleId());
                 if (list == null) {
                     list = new CopyOnWriteArrayList<Extension>();
        -            extensions.put(bundle.getBundleId(), list);
        +            List<Extension> preList = extensions.putIfAbsent(bundle.getBundleId(), list);
        +            if (preList != null) {
        +                list = preList;
        +            }
                 }
        
        

        to ensure we won't lost extensions in a pre-saved list

        Also correct checkstyle.
        Thanks for the patch
        Freeman

        Show
        ffang Freeman Fang added a comment - Hi, Just one issue, the rt/core/src/main/java/org/apache/cxf/bus/osgi/CXFExtensionBundleListener.java change should be @@ -90,7 +90,10 @@ List<Extension> list = extensions.get(bundle.getBundleId()); if (list == null ) { list = new CopyOnWriteArrayList<Extension>(); - extensions.put(bundle.getBundleId(), list); + List<Extension> preList = extensions.putIfAbsent(bundle.getBundleId(), list); + if (preList != null ) { + list = preList; + } } to ensure we won't lost extensions in a pre-saved list Also correct checkstyle. Thanks for the patch Freeman
        Hide
        ffang Freeman Fang added a comment -

        Apply patch on behalf of Yu Lin with thanks

        Show
        ffang Freeman Fang added a comment - Apply patch on behalf of Yu Lin with thanks

          People

          • Assignee:
            ffang Freeman Fang
            Reporter:
            jacklondongood Yu Lin
          • Votes:
            0 Vote for this issue
            Watchers:
            2 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